Hi David,
On Wed, 22 Jan 2025 at 22:07, David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> On 22.01.25 16:27, Fuad Tabba wrote:
> > Add support for mmap() and fault() for guest_memfd in the host
> > for VMs that support in place conversion between shared and
> > private.
> > To that end, this patch adds the ability to check> whether the
> architecture has that support, and only allows mmap()
> > if that's the case.
> > > Additionally, this is gated with a new configuration option,
> > CONFIG_KVM_GMEM_MAPPABLE.
> > > Signed-off-by: Fuad Tabba <tabba@xxxxxxxxxx>
> > ---
> > arch/x86/include/asm/kvm_host.h | 2 +
> > include/linux/kvm_host.h | 11 +++++
> > virt/kvm/Kconfig | 4 ++
> > virt/kvm/guest_memfd.c | 71 +++++++++++++++++++++++++++++++++
> > 4 files changed, 88 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index e159e44a6a1b..c0e149bc1d79 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -2206,6 +2206,8 @@ void kvm_configure_mmu(bool enable_tdp, int tdp_forced_root_level,
> > #define kvm_arch_has_private_mem(kvm) false
> > #endif
> >
> > +#define kvm_arch_private_mem_inplace(kvm) false
> > +
> > #define kvm_arch_has_readonly_mem(kvm) (!(kvm)->arch.has_protected_state)
> >
> > static inline u16 kvm_read_ldt(void)
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 401439bb21e3..ebca0ab4c5e2 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -717,6 +717,17 @@ static inline bool kvm_arch_has_private_mem(struct kvm *kvm)
> > }
> > #endif
> >
> > +/*
> > + * Arch code must define kvm_arch_private_mem_inplace if support for private
> > + * memory is enabled it supports in-place conversion between shared and private.
> > + */
> > +#if !defined(kvm_arch_private_mem_inplace) && !IS_ENABLED(CONFIG_KVM_PRIVATE_MEM)
> > +static inline bool kvm_arch_private_mem_inplace(struct kvm *kvm)
> I assume right now this would rather indicate "support for shared
> (mappable) memory in guest_memfd".
>
> Maybe there is a better way to express that :)
>
> kvm_arch_gmem_supports_shared_mem ?
>
> The in-place conversion is (at least to me) implied with support for
> shared memory.
Sounds better than what I had.
> > +
> > #ifndef kvm_arch_has_readonly_mem
> > static inline bool kvm_arch_has_readonly_mem(struct kvm *kvm)
> > {
> > diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> > index 54e959e7d68f..59400fd8f539 100644
> > --- a/virt/kvm/Kconfig
> > +++ b/virt/kvm/Kconfig
> > @@ -124,3 +124,7 @@ config HAVE_KVM_ARCH_GMEM_PREPARE
> > config HAVE_KVM_ARCH_GMEM_INVALIDATE
> > bool
> > depends on KVM_PRIVATE_MEM
> > +
> > +config KVM_GMEM_MAPPABLE
> > + select KVM_PRIVATE_MEM
>
> Easier to grasp might be:
>
> KVM_GMEM_MAPPABLE -> KVM_GMEM_SHARED_MEM
>
> Support for "shared" memory kind of imply mmap support (otherwise, how
> to make use of it :) ),
Like the one above, this makes more sense.
>
> (KVM_PRIVATE_MEM -> KVM_GMEM might also make sense, but it's a different
> discussion)
>
> > + bool
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index 47a9f68f7b24..9ee162bf6bde 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -307,7 +307,78 @@ static pgoff_t kvm_gmem_get_index(struct kvm_memory_slot *slot, gfn_t gfn)
> > return gfn - slot->base_gfn + slot->gmem.pgoff;
> > }
> >
> > +#ifdef CONFIG_KVM_GMEM_MAPPABLE
> > +static vm_fault_t kvm_gmem_fault(struct vm_fault *vmf)
> > +{
> > + struct inode *inode = file_inode(vmf->vma->vm_file);
> > + struct folio *folio;
> > + vm_fault_t ret = VM_FAULT_LOCKED;
> > +
> > + filemap_invalidate_lock_shared(inode->i_mapping);
> > +
> > + folio = kvm_gmem_get_folio(inode, vmf->pgoff);
>
>
> Would the idea be later that kvm_gmem_get_folio() would fail on private
> memory, or do you envision other checks in this code here in the future?
There would be other checks in the future, the idea is that they would
be the ones in:
https://lore.kernel.org/all/20250117163001.2326672-8-tabba@xxxxxxxxxx/
What this patch series has, but the other one doesn't, is checks on
mmap to ensure whether the VM type supports that operation in
principle or not.
Thanks,
/fuad
> --
> Cheers,
>
> David / dhildenb
>
