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

[David Hildenbrand <david@redhat.com>]

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