Re: [RFC PATCH v2 04/11] KVM: guest_memfd: Add KVM capability to check if guest_memfd is shared

[Fuad Tabba <tabba@google.com>]

Hi David,

On Fri, 31 Jan 2025 at 09:11, David Hildenbrand <david@redhat.com> wrote:
>
> On 29.01.25 18:23, Fuad Tabba wrote:
> > Add the KVM capability KVM_CAP_GMEM_SHARED_MEM, which indicates
> > that the VM supports shared memory in guest_memfd, or that the
> > host can create VMs that support shared memory. Supporting shared
> > memory implies that memory can be mapped when shared with the
> > host.
> >
> > For now, this checks only whether the VM type supports sharing
> > guest_memfd backed memory. In the future, it will be expanded to
> > check whether the specific memory address is shared with the
> > host.
> >
> > Signed-off-by: Fuad Tabba <tabba@google.com>
> > ---
> >   include/uapi/linux/kvm.h |  1 +
> >   virt/kvm/guest_memfd.c   | 13 +++++++++++++
> >   virt/kvm/kvm_main.c      |  4 ++++
> >   3 files changed, 18 insertions(+)
> >
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index 502ea63b5d2e..3ac805c5abf1 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -933,6 +933,7 @@ struct kvm_enable_cap {
> >   #define KVM_CAP_PRE_FAULT_MEMORY 236
> >   #define KVM_CAP_X86_APIC_BUS_CYCLES_NS 237
> >   #define KVM_CAP_X86_GUEST_MODE 238
> > +#define KVM_CAP_GMEM_SHARED_MEM 239
> >
> >   struct kvm_irq_routing_irqchip {
> >       __u32 irqchip;
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index 86441581c9ae..4e1144ed3446 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -308,6 +308,13 @@ static pgoff_t kvm_gmem_get_index(struct kvm_memory_slot *slot, gfn_t gfn)
> >   }
> >
> >   #ifdef CONFIG_KVM_GMEM_SHARED_MEM
> > +static bool kvm_gmem_is_shared(struct file *file, pgoff_t pgoff)
>
> I assume you want to call this something like:
>
> kvm_gmem_folio_is_shared
> kvm_gmem_offset_is_shared
> kvm_gmem_range_is_shared
> ...
>
> To make it clearer that you are only checking one piece and not the
> whole thing.
>
> But then, I wonder if that could be handled in kvm_gmem_get_folio(),
> and e.g., specified via a flag?
>
> kvm_gmem_get_folio(inode, vmf->pgoff, KVM_GMEM_GF_SHARED);
>
> Maybe existing callers would want to pass KVM_GMEM_GF_PRIVATE, and the
> ones that "don't care" don't pas anything?

I agree that naming this function to make it clearer would be a good
idea. That said, I think with the patches I removed, it doesn't even
need to be exposed at all, even looking at future patches --- it has
no callers other than kvm_gmem_fault(). Therefore, I don't think we
need to add a flag to kvm_gmem_get_folio().

I'll have a closer look while preparing the respin and fix it either way.

Thanks,
/fuad



> --
> Cheers,
>
> David / dhildenb
>