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

[Gavin Shan <gshan@redhat.com>]

Hi Fuad,

On 5/16/25 5:56 PM, Fuad Tabba wrote:
> On Fri, 16 May 2025 at 08:09, Gavin Shan <gshan@redhat.com> wrote:
>> 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.
> 
> The idea is to document the lack of hugepage support in code, but if
> you think it's necessary, I could add a comment.
> 

Ok, I was actually nit-picky since we're at v9, which is close to integration,
I guess. If another respin is needed, a comment wouldn't be harmful, but it's
also perfectly fine without it :)

> 
>>
>>> +     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.
> 
> This series only supports shared memory, and the idea is not to use
> the attributes to check. We ensure that only certain VM types can set
> the flag (e.g., VM_TYPE_DEFAULT and KVM_X86_SW_PROTECTED_VM).
> 
> In the patch series that builds on it, with in-place conversion
> between private and shared, we do add a check that the memory faulted
> in is in-fact shared.
> 

Ok, thanks for your clarification. I plan to review that series, but not
getting a chance yet. Right, it's sensible to limit the capability of modifying
page's attribute (private vs shared) to the particular machine types since
the whole feature (restricted mmap and in-place conversion) is applicable
to particular machine types. I can understand KVM_X86_SW_PROTECTED_VM
(similar to pKVM) needs the feature, but I don't understand why VM_TYPE_DEFAULT
needs the feature. I guess we may want to use guest-memfd as to tmpfs or
shmem, meaning all the address space associated with a guest-memfd is shared,
but without the corresponding private space pointed by struct kvm_userspace_memory_region2
::userspace_addr. Instead, the 'userspace_addr' will be mmap(guest-memfd) from
VMM's perspective if I'm correct.

Thanks,
Gavin

> Thanks,
> /fuad
> 
>>> +     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
>>
>