Re: [PATCH v5 3/9] KVM: guest_memfd: Allow host to map guest_memfd() pages

[Ackerley Tng <ackerleytng@google.com>]

Fuad Tabba <tabba@google.com> writes:

> Hi Ackerley,
>
> On Thu, 6 Mar 2025 at 00:02, Ackerley Tng <ackerleytng@google.com> wrote:
>>
>> Fuad Tabba <tabba@google.com> writes:
>>
>> > <snip>
>> >
>> > +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.
>> > +      */
>>
>> I think this is not quite accurate.
>>
>> Based on my understanding and kvm_gmem_handle_folio_put() in this other
>> patch [1], only pages *in transition* from shared to private state are
>> marked "guestmem", although it is true that no private folios or folios
>> marked guestmem are expected here.
>
> Technically, pages in transition are private as far as the host is
> concerned. This doesn't say that _all_ private pages are marked as
> guestmem. It says that only private pages are marked as guestmem. It
> could be private and _not_ be marked as guestmem :)

True, didn't think of it this way!

>
> I probably should rephrase something along the lines of, "no shared
> folios would be marked as guestmem". How does that sound?
>
> Thanks,
> /fuad
>

Works for me, thank you! 

>> > +     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);
>> > +     }
>> > +
>> > +     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;
>> > +}
>> >
>> > <snip>
>>
>> [1] https://lore.kernel.org/all/20250117163001.2326672-7-tabba@google.com/