Re: [PATCH v4 03/10] KVM: guest_memfd: Allow host to map guest_memfd() pages

[David Hildenbrand <david@redhat.com>]

On 20.02.25 18:10, Fuad Tabba wrote:
> On Thu, 20 Feb 2025 at 15:58, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 20.02.25 16:45, Fuad Tabba wrote:
>>> Hi David,
>>>
>>> On Thu, 20 Feb 2025 at 12:04, Fuad Tabba <tabba@google.com> wrote:
>>>>
>>>> On Thu, 20 Feb 2025 at 11:58, David Hildenbrand <david@redhat.com> wrote:
>>>>>
>>>>> On 18.02.25 18:24, Fuad Tabba wrote:
>>>>>> Add support for mmap() and fault() for guest_memfd backed memory
>>>>>> 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 VM type supports in-place conversion, and only
>>>>>> allows mapping its memory if that's the case.
>>>>>>
>>>>>> This behavior is also gated by the configuration option
>>>>>> KVM_GMEM_SHARED_MEM.
>>>>>>
>>>>>> Signed-off-by: Fuad Tabba <tabba@google.com>
>>>>>> ---
>>>>>>     include/linux/kvm_host.h |  11 +++++
>>>>>>     virt/kvm/guest_memfd.c   | 103 +++++++++++++++++++++++++++++++++++++++
>>>>>>     2 files changed, 114 insertions(+)
>>>>>>
>>>>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>>>>>> index 3ad0719bfc4f..f9e8b10a4b09 100644
>>>>>> --- a/include/linux/kvm_host.h
>>>>>> +++ b/include/linux/kvm_host.h
>>>>>> @@ -728,6 +728,17 @@ static inline bool kvm_arch_has_private_mem(struct kvm *kvm)
>>>>>>     }
>>>>>>     #endif
>>>>>>
>>>>>> +/*
>>>>>> + * Arch code must define kvm_arch_gmem_supports_shared_mem if support for
>>>>>> + * private memory is enabled and it supports in-place shared/private conversion.
>>>>>> + */
>>>>>> +#if !defined(kvm_arch_gmem_supports_shared_mem) && !IS_ENABLED(CONFIG_KVM_PRIVATE_MEM)
>>>>>> +static inline bool kvm_arch_gmem_supports_shared_mem(struct kvm *kvm)
>>>>>> +{
>>>>>> +     return false;
>>>>>> +}
>>>>>> +#endif
>>>>>> +
>>>>>>     #ifndef kvm_arch_has_readonly_mem
>>>>>>     static inline bool kvm_arch_has_readonly_mem(struct kvm *kvm)
>>>>>>     {
>>>>>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>>>>>> index c6f6792bec2a..30b47ff0e6d2 100644
>>>>>> --- a/virt/kvm/guest_memfd.c
>>>>>> +++ b/virt/kvm/guest_memfd.c
>>>>>> @@ -317,9 +317,112 @@ void kvm_gmem_handle_folio_put(struct folio *folio)
>>>>>>     {
>>>>>>         WARN_ONCE(1, "A placeholder that shouldn't trigger. Work in progress.");
>>>>>>     }
>>>>>> +
>>>>>> +static bool kvm_gmem_offset_is_shared(struct file *file, pgoff_t index)
>>>>>> +{
>>>>>> +     struct kvm_gmem *gmem = file->private_data;
>>>>>> +
>>>>>> +     /* For now, VMs that support shared memory share all their memory. */
>>>>>> +     return kvm_arch_gmem_supports_shared_mem(gmem->kvm);
>>>>>> +}
>>>>>> +
>>>>>> +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);
>>>>>> +     if (IS_ERR(folio)) {
>>>>>> +             switch (PTR_ERR(folio)) {
>>>>>> +             case -EAGAIN:
>>>>>> +                     ret = VM_FAULT_RETRY;
>>>>>> +                     break;
>>>>>> +             case -ENOMEM:
>>>>>> +                     ret = VM_FAULT_OOM;
>>>>>> +                     break;
>>>>>> +             default:
>>>>>> +                     ret = VM_FAULT_SIGBUS;
>>>>>> +                     break;
>>>>>> +             }
>>>>>> +             goto out_filemap;
>>>>>> +     }
>>>>>> +
>>>>>> +     if (folio_test_hwpoison(folio)) {
>>>>>> +             ret = VM_FAULT_HWPOISON;
>>>>>> +             goto out_folio;
>>>>>> +     }
>>>>>> +
>>>>>> +     /* Must be called with folio lock held, i.e., after kvm_gmem_get_folio() */
>>>>>> +     if (!kvm_gmem_offset_is_shared(vmf->vma->vm_file, vmf->pgoff)) {
>>>>>> +             ret = VM_FAULT_SIGBUS;
>>>>>> +             goto out_folio;
>>>>>> +     }
>>>>>> +
>>>>>> +     /*
>>>>>> +      * Only private folios are marked as "guestmem" so far, and we never
>>>>>> +      * expect private folios at this point.
>>>>>> +      */
>>>>>> +     if (WARN_ON_ONCE(folio_test_guestmem(folio)))  {
>>>>>> +             ret = VM_FAULT_SIGBUS;
>>>>>> +             goto out_folio;
>>>>>> +     }
>>>>>> +
>>>>>> +     /* No support for huge pages. */
>>>>>> +     if (WARN_ON_ONCE(folio_test_large(folio))) {
>>>>>> +             ret = VM_FAULT_SIGBUS;
>>>>>> +             goto out_folio;
>>>>>> +     }
>>>>>> +
>>>>>> +     if (!folio_test_uptodate(folio)) {
>>>>>> +             clear_highpage(folio_page(folio, 0));
>>>>>> +             kvm_gmem_mark_prepared(folio);
>>>>>> +     }
>>>>>
>>>>> kvm_gmem_get_pfn()->__kvm_gmem_get_pfn() seems to call
>>>>> kvm_gmem_prepare_folio() instead.
>>>>>
>>>>> Could we do the same here?
>>>>
>>>> Will do.
>>>
>>> I realized it's not that straightforward. __kvm_gmem_prepare_folio()
>>> requires the kvm_memory_slot, which is used to calculate the gfn. At
>>> that point we have neither, and it's not just an issue of access, but
>>> there might not be a slot associated with that yet.
>>
>> Hmm, right ... I wonder if that might be problematic. I assume no
>> memslot == no memory attribute telling us if it is private or shared at
>> least for now?
>>
>> Once guest_memfd maintains that state, it might be "cleaner" ? What's
>> your thought?
> 
> The idea is that this doesn't determine whether it's shared or private
> by the guest_memfd's attributes, but by the new state added in the
> other patch series. That's independent of memslots and guest addresses
> altogether.
> 
> One scenario you can imagine is the host wanting to fault in memory to
> initialize it before associating it with a memslot. I guess we could
> make it a requirement that you cannot fault-in pages unless they are
> associated with a memslot, but that might be too restrictive.

Okay, just what I thought, thanks!

-- 
Cheers,

David / dhildenb