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

[Fuad Tabba <tabba@google.com>]

On Thu, 23 Jan 2025 at 10:28, David Hildenbrand <david@redhat.com> wrote:
>
> >>> +       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?
> >
> > There would be other checks in the future, the idea is that they would
> > be the ones in:
> > https://lore.kernel.org/all/20250117163001.2326672-8-tabba@google.com/
> >
>
> Thanks, so I wonder if this patch should just add necessary callback(s)
> as well, to make this patch look like it adds most of the infrastructure
> on the mmap level.
>
> kvm_gmem_is_shared() or sth like that, documenting that it must be
> called after kvm_gmem_get_folio() -- with a raised folio reference /
> folio lock.
>
> Alternatively, provide a
>
>         kvm_gmem_get_shared_folio()
>
> that abstracts that operation.
>
> We could also for now ensure that we really only get small folios back,
> and even get rid of the clearing loop.
>
>
> The "WARN_ON_ONCE(folio_test_guestmem(folio)" would be added separately.
>
> In the context of this series, the callback would be a nop and always
> say "yes".

I agree, especially if this patch series were to serve as a prelude to
the other one that adds restricted mmap() support.

Cheers,
/fuad

> --
> Cheers,
>
> David / dhildenb
>