RE: [PATCH v3 06/70] kvm: Introduce support for memory_attributes

[Wang, Wei W]

On Thursday, December 21, 2023 2:11 PM, Li, Xiaoyao wrote:
> On 12/12/2023 9:56 PM, Wang, Wei W wrote:
> > On Wednesday, November 15, 2023 3:14 PM, Xiaoyao Li wrote:
> >> Introduce the helper functions to set the attributes of a range of
> >> memory to private or shared.
> >>
> >> This is necessary to notify KVM the private/shared attribute of each gpa
> range.
> >> KVM needs the information to decide the GPA needs to be mapped at
> >> hva- based shared memory or guest_memfd based private memory.
> >>
> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> >> ---
> >>   accel/kvm/kvm-all.c  | 42
> ++++++++++++++++++++++++++++++++++++++++++
> >>   include/sysemu/kvm.h |  3 +++
> >>   2 files changed, 45 insertions(+)
> >>
> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
> >> 69afeb47c9c0..76e2404d54d2 100644
> >> --- a/accel/kvm/kvm-all.c
> >> +++ b/accel/kvm/kvm-all.c
> >> @@ -102,6 +102,7 @@ bool kvm_has_guest_debug;  static int
> >> kvm_sstep_flags; static bool kvm_immediate_exit;  static bool
> >> kvm_guest_memfd_supported;
> >> +static uint64_t kvm_supported_memory_attributes;
> >>   static hwaddr kvm_max_slot_size = ~0;
> >>
> >>   static const KVMCapabilityInfo kvm_required_capabilites[] = { @@
> >> -1305,6
> >> +1306,44 @@ void kvm_set_max_memslot_size(hwaddr max_slot_size)
> >>       kvm_max_slot_size = max_slot_size;
> >>   }
> >>
> >> +static int kvm_set_memory_attributes(hwaddr start, hwaddr size,
> >> +uint64_t attr) {
> >> +    struct kvm_memory_attributes attrs;
> >> +    int r;
> >> +
> >> +    attrs.attributes = attr;
> >> +    attrs.address = start;
> >> +    attrs.size = size;
> >> +    attrs.flags = 0;
> >> +
> >> +    r = kvm_vm_ioctl(kvm_state, KVM_SET_MEMORY_ATTRIBUTES, &attrs);
> >> +    if (r) {
> >> +        warn_report("%s: failed to set memory (0x%lx+%#zx) with attr
> >> + 0x%lx
> >> error '%s'",
> >> +                     __func__, start, size, attr, strerror(errno));
> >> +    }
> >> +    return r;
> >> +}
> >> +
> >> +int kvm_set_memory_attributes_private(hwaddr start, hwaddr size) {
> >> +    if (!(kvm_supported_memory_attributes &
> >> KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
> >> +        error_report("KVM doesn't support PRIVATE memory attribute\n");
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    return kvm_set_memory_attributes(start, size,
> >> +KVM_MEMORY_ATTRIBUTE_PRIVATE); }
> >> +
> >> +int kvm_set_memory_attributes_shared(hwaddr start, hwaddr size) {
> >> +    if (!(kvm_supported_memory_attributes &
> >> KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
> >> +        error_report("KVM doesn't support PRIVATE memory attribute\n");
> >> +        return -EINVAL;
> >> +    }
> >
> > Duplicate code in kvm_set_memory_attributes_shared/private.
> > Why not move the check into kvm_set_memory_attributes?
> 
> Because it's not easy to put the check into there.
> 
> Both setting and clearing one bit require the capability check. If moving the
> check into kvm_set_memory_attributes(), the check of
> KVM_MEMORY_ATTRIBUTE_PRIVATE will have to become unconditionally,
> which is not aligned to the function name because the name is not restricted 
> to
> shared/private attribute only.

No need to specifically check for KVM_MEMORY_ATTRIBUTE_PRIVATE there.
I'm suggesting below:

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 2d9a2455de..63ba74b221 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1375,6 +1375,11 @@ static int kvm_set_memory_attributes(hwaddr start, 
hwaddr size, uint64_t attr)
     struct kvm_memory_attributes attrs;
     int r;

+    if ((attr & kvm_supported_memory_attributes) != attr) {
+        error_report("KVM doesn't support memory attr %lx\n", attr);
+        return -EINVAL;
+    }
+
     attrs.attributes = attr;
     attrs.address = start;
     attrs.size = size;
@@ -1390,21 +1395,11 @@ static int kvm_set_memory_attributes(hwaddr start, 
hwaddr size, uint64_t attr)

 int kvm_set_memory_attributes_private(hwaddr start, hwaddr size)
 {
-    if (!(kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
-        error_report("KVM doesn't support PRIVATE memory attribute\n");
-        return -EINVAL;
-    }
-
     return kvm_set_memory_attributes(start, size, 
KVM_MEMORY_ATTRIBUTE_PRIVATE);
 }

 int kvm_set_memory_attributes_shared(hwaddr start, hwaddr size)
 {
-    if (!(kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
-        error_report("KVM doesn't support PRIVATE memory attribute\n");
-        return -EINVAL;
-    }
-
     return kvm_set_memory_attributes(start, size, 0);
 }

Maybe you don't even need the kvm_set_memory_attributes_shared/private wrappers.