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

[Peter Xu <peterx@redhat.com>]

On Tue, Dec 16, 2025 at 10:53:04AM -0300, Fabiano Rosas wrote:
> 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.

Yeah, the hope is it'll stop being confusing after this series applied.

Keeping them separate is logically more sensible, not only to make review
easier, but the rule that each commit should be self contained and also
minimum..

> 
> Anyway, up to you. For this one:
> 
> Reviewed-by: Fabiano Rosas <farosas@suse.de>

Thanks!

-- 
Peter Xu