Re: [PATCH v3 01/12] kvm: Decouple memory attribute check from kvm_guest_memfd_supported

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> From: Xiaoyao Li <xiaoyao.li@intel.com>
>
> With the mmap support of guest memfd, KVM allows usersapce to create
> guest memfd serving as normal non-private memory for X86 DEFEAULT VM.
> However, KVM doesn't support private memory attriute for X86 DEFAULT
> VM.
>
> Make kvm_guest_memfd_supported not rely on KVM_MEMORY_ATTRIBUTE_PRIVATE
> and check KVM_MEMORY_ATTRIBUTE_PRIVATE separately when the machine
> requires guest_memfd to serve as private memory.
>
> This allows QMEU to create guest memfd with mmap to serve as the memory
> backend for X86 DEFAULT VM.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/system/kvm.h   | 1 +
>  accel/kvm/kvm-all.c    | 8 ++++++--
>  accel/stubs/kvm-stub.c | 5 +++++
>  system/physmem.c       | 8 ++++++++
>  4 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/include/system/kvm.h b/include/system/kvm.h
> index 8f9eecf044..b5811c90f1 100644
> --- a/include/system/kvm.h
> +++ b/include/system/kvm.h
> @@ -561,6 +561,7 @@ int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp);
>  
>  int kvm_set_memory_attributes_private(hwaddr start, uint64_t size);
>  int kvm_set_memory_attributes_shared(hwaddr start, uint64_t size);
> +bool kvm_private_memory_attribute_supported(void);
>  
>  int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private);
>  
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 28006d73c5..59836ebdff 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1501,6 +1501,11 @@ int kvm_set_memory_attributes_shared(hwaddr start, uint64_t size)
>      return kvm_set_memory_attributes(start, size, 0);
>  }
>  
> +bool kvm_private_memory_attribute_supported(void)
> +{
> +    return !!(kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE);
> +}
> +
>  /* Called with KVMMemoryListener.slots_lock held */
>  static void kvm_set_phys_mem(KVMMemoryListener *kml,
>                               MemoryRegionSection *section, bool add)
> @@ -2781,8 +2786,7 @@ static int kvm_init(AccelState *as, MachineState *ms)
>      kvm_supported_memory_attributes = kvm_vm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES);
>      kvm_guest_memfd_supported =
>          kvm_vm_check_extension(s, KVM_CAP_GUEST_MEMFD) &&
> -        kvm_vm_check_extension(s, KVM_CAP_USER_MEMORY2) &&
> -        (kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE);
> +        kvm_vm_check_extension(s, KVM_CAP_USER_MEMORY2);
>      kvm_pre_fault_memory_supported = kvm_vm_check_extension(s, KVM_CAP_PRE_FAULT_MEMORY);
>  
>      if (s->kernel_irqchip_split == ON_OFF_AUTO_AUTO) {
> diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
> index 68cd33ba97..73f04eb589 100644
> --- a/accel/stubs/kvm-stub.c
> +++ b/accel/stubs/kvm-stub.c
> @@ -125,3 +125,8 @@ int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp)
>  {
>      return -ENOSYS;
>  }
> +
> +bool kvm_private_memory_attribute_supported(void)
> +{
> +    return false;
> +}
> diff --git a/system/physmem.c b/system/physmem.c
> index c9869e4049..3555d2f6f7 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2211,6 +2211,14 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
>                         object_get_typename(OBJECT(current_machine->cgs)));
>              goto out_free;
>          }
> +
> +        if (!kvm_private_memory_attribute_supported()) {
> +            error_setg(errp, "cannot set up private guest memory for %s: "
> +                       " KVM does not support private memory attribute",
> +                       object_get_typename(OBJECT(current_machine->cgs)));
> +            goto out_free;
> +        }

Hm, it took me a while to understand why this is under (new_block->flags
& RAM_GUEST_MEMFD) but checking for private memory support. If it's at
all feasible I would just squash all those patches doing
s/guest_memfd/guest_memfd_private/ to avoid having intermediate patches
where the terminology is not aligned.

Anyway, up to you. For this one:

Reviewed-by: Fabiano Rosas <farosas@suse.de>