Re: [RFC PATCH v1 1/9] KVM: guest_memfd: Allow host to mmap guest_memfd() pages

David Hildenbrand
2025-01-22T22:07:16Z

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@google.com>
> ---
> 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.

> +
> #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 :) ),

(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?

--
Cheers,

David / dhildenb