Re: [PATCH v9 07/17] KVM: guest_memfd: Allow host to map guest_memfd() pages

[Gavin Shan <gshan@redhat.com>]

Hi Fuad,

On 5/14/25 2:34 AM, Fuad Tabba wrote:
> This patch enables support for shared memory in guest_memfd, including
> mapping that memory at the host userspace. This support is gated by the
> configuration option KVM_GMEM_SHARED_MEM, and toggled by the guest_memfd
> flag GUEST_MEMFD_FLAG_SUPPORT_SHARED, which can be set when creating a
> guest_memfd instance.
> 
> Co-developed-by: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>   arch/x86/include/asm/kvm_host.h | 10 ++++
>   include/linux/kvm_host.h        | 13 +++++
>   include/uapi/linux/kvm.h        |  1 +
>   virt/kvm/Kconfig                |  5 ++
>   virt/kvm/guest_memfd.c          | 88 +++++++++++++++++++++++++++++++++
>   5 files changed, 117 insertions(+)
> 

[...]

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 6db515833f61..8e6d1866b55e 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -312,7 +312,88 @@ 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_SHARED_MEM
> +
> +static bool kvm_gmem_supports_shared(struct inode *inode)
> +{
> +	uint64_t flags = (uint64_t)inode->i_private;
> +
> +	return flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED;
> +}
> +
> +static vm_fault_t kvm_gmem_fault_shared(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);
> +	if (IS_ERR(folio)) {
> +		int err = PTR_ERR(folio);
> +
> +		if (err == -EAGAIN)
> +			ret = VM_FAULT_RETRY;
> +		else
> +			ret = vmf_error(err);
> +
> +		goto out_filemap;
> +	}
> +
> +	if (folio_test_hwpoison(folio)) {
> +		ret = VM_FAULT_HWPOISON;
> +		goto out_folio;
> +	}
> +
> +	if (WARN_ON_ONCE(folio_test_large(folio))) {
> +		ret = VM_FAULT_SIGBUS;
> +		goto out_folio;
> +	}
> +

I don't think there is a large folio involved since the max/min folio order
(stored in struct address_space::flags) should have been set to 0, meaning
only order-0 is possible when the folio (page) is allocated and added to the
page-cache. More details can be referred to AS_FOLIO_ORDER_MASK. It's unnecessary
check but not harmful. Maybe a comment is needed to mention large folio isn't
around yet, but double confirm.


> +	if (!folio_test_uptodate(folio)) {
> +		clear_highpage(folio_page(folio, 0));
> +		kvm_gmem_mark_prepared(folio);
> +	}
> +

I must be missing some thing here. This chunk of code is out of sync to kvm_gmem_get_pfn(),
where kvm_gmem_prepare_folio() and kvm_arch_gmem_prepare() are executed, and then
PG_uptodate is set after that. In the latest ARM CCA series, kvm_arch_gmem_prepare()
isn't used, but it would delegate the folio (page) with the prerequisite that
the folio belongs to the private address space.

I guess that kvm_arch_gmem_prepare() is skipped here because we have the assumption that
the folio belongs to the shared address space? However, this assumption isn't always
true. We probably need to ensure the folio range is really belonging to the shared
address space by poking kvm->mem_attr_array, which can be modified by VMM through
ioctl KVM_SET_MEMORY_ATTRIBUTES.

> +	vmf->page = folio_file_page(folio, vmf->pgoff);
> +
> +out_folio:
> +	if (ret != VM_FAULT_LOCKED) {
> +		folio_unlock(folio);
> +		folio_put(folio);
> +	}
> +
> +out_filemap:
> +	filemap_invalidate_unlock_shared(inode->i_mapping);
> +
> +	return ret;
> +}
> +

Thanks,
Gavin