Re: [PATCH 1/6] KVM: guest_memfd: Add DEFAULT_SHARED flag, reject user page faults if not set

Vishal Annapurve
2025-10-03T04:11:01Z

On Thu, Oct 2, 2025, 5:12 PM Sean Christopherson <seanjc@google.com> wrote:
>
> > >
> > > If the _only_ user-visible asset that is added is a KVM_CREATE_GUEST_MEMFD flag,
> > > a CAP is gross overkill. Even if there are other assets that accompany the new
> > > flag, there's no reason we couldn't say "this feature exist if XYZ flag is
> > > supported".
> > >
> > > E.g. it's functionally no different than KVM_CAP_VM_TYPES reporting support for
> > > KVM_X86_TDX_VM also effectively reporting support for a _huge_ number of things
> > > far beyond being able to create a VM of type KVM_X86_TDX_VM.
> > >
> >
> > What's your opinion about having KVM_CAP_GUEST_MEMFD_MMAP part of
> > KVM_CAP_GUEST_MEMFD_CAPS i.e. having a KVM cap covering all features
> > of guest_memfd?
>
> I'd much prefer to have both. Describing flags for an ioctl via a bitmask that
> doesn't *exactly* match the flags is asking for problems. At best, it will be
> confusing. E.g. we'll probably end up with code like this:
>
> gmem_caps = kvm_check_cap(KVM_CAP_GUEST_MEMFD_CAPS);
>
> if (gmem_caps & KVM_CAP_GUEST_MEMFD_MMAP)
> gmem_flags |= GUEST_MEMFD_FLAG_MMAP;
> if (gmem_caps & KVM_CAP_GUEST_MEMFD_INIT_SHARED)
> gmem_flags |= KVM_CAP_GUEST_MEMFD_INIT_SHARED;
>

No, I actually meant the userspace can just rely on the cap to assume
right flags to be available (not necessarily the same flags as cap
bits).

i.e. Userspace will do something like:
gmem_caps = kvm_check_cap(KVM_CAP_GUEST_MEMFD_CAPS);

if (gmem_caps & KVM_CAP_GUEST_MEMFD_MMAP)
gmem_flags |= GUEST_MEMFD_FLAG_MMAP;
if (gmem_caps & KVM_CAP_GUEST_MEMFD_HUGETLB)
gmem_flags |= GUEST_MEMFD_FLAG_HUGETLB | GUEST_MEMFD_FLAG_HUGETLB_2MB;

Userspace has to anyways assume flag values, userspace just needs to
know if a particular feature is available.

> ...
> Another issue is that, while unlikely, we could run out of KVM_CAP_GUEST_MEMFD_CAPS
> bits before we run out of flags.

I would say that's unlikely as I know of at least one feature that
needs multiple flag bits.

>
> And if we use memory attributes, we're also guaranteed to have at least one gmem
> capability that returns a bitmask separately from a dedicated one-size-fits-all
> cap, e.g.
>
> case KVM_CAP_GUEST_MEMFD_MEMORY_ATTRIBUTES:
> if (vm_memory_attributes)
> return 0;
>
> return kvm_supported_mem_attributes(kvm);

For this one, we need a separate dedicated cap.

>
> Side topic, looking at this, I don't think we need KVM_CAP_GUEST_MEMFD_CAPS, I'm
> pretty sure we can simply extend KVM_CAP_GUEST_MEMFD. E.g.
>
> #define KVM_GUEST_MEMFD_FEAT_BASIC (1ULL << 0)
> #define KVM_GUEST_MEMFD_FEAT_FANCY (1ULL << 1)
>
> case KVM_CAP_GUEST_MEMFD:
> return KVM_GUEST_MEMFD_FEAT_BASIC |
> KVM_GUEST_MEMFD_FEAT_FANCY;

This scheme seems ok to me.

>
> > That seems more consistent to me in order for userspace to deduce the
> > supported features and assume flags/ioctls/... associated with the feature
> > as a group.
>
> If we add a feature that comes with a flag, we could always add both, i.e. a
> feature flag for KVM_CAP_GUEST_MEMFD along with the natural enumeration for
> KVM_CAP_GUEST_MEMFD_FLAGS. That certainly wouldn't be my first choice, but it's
> a possibility, e.g. if it really is the most intuitive solution. But that's
> getting quite hypothetical.