Re: [PATCH v3] KVM: have hva_to_pfn_remapped write-upgrade PTEs

[Paolo Bonzini]

On 7/30/26 19:31, Sean Christopherson wrote:
> On Wed, Jul 29, 2026, Sergio Lopez wrote:
> > After 28e39181 ("drm/gem-shmem: Track folio accessed/dirty status in
> > mmap") was merged, a guest write to an unpopulated PTE from a mapping
> > backed by a DRM GEM BO triggers a VM exit with EFAULT, with
> > hva_to_pfn_remapped setting p_pfn to KVM_PFN_ERR_RO_FAULT.
> >
> > This happens because that commit implements pfn_mkwrite for
> > drm_gem_shmem_vm_ops. With that function present, vma_wants_writenotify
> > returns true in vma_set_page_prot, clearing VM_SHARED and leading to the
> > entry to be installed as read-only. This is done on purpose so the
> > fault handler gets notified when the entry is going to be written.
>
> NAK, this doesn't belong in KVM.  Expecting callers of fixup_user_fault() to
> retry a FAULT_FLAG_WRITE fault on *success* is absurd.  Either manually do the
> retry in fixup_user_fault(), or return VM_FAULT_RETRY so that KVM will naturally
> retry.  I assume the latter is the correct approach.
Doing it in fixup_user_fault(), or more precisely in __do_fault(),
seems hard.  The information about the trick (about the presence
of *_mkwrite) is only recorded in vma->vm_page_prot, which is an
opaque pgprot_t.  So it's only follow_pfnmap_start() that knows
how to retrieve it.

In the driver it would be I guess something like:

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index c989459eb215..ae913481a42c 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -658,6 +658,10 @@ static vm_fault_t drm_gem_shmem_any_fault(struct vm_fault *vmf, unsigned int ord
        if (ret == VM_FAULT_NOPAGE)
                folio_mark_accessed(folio);

+       /* Force another round to ensure that pfn_mkwrite is called.  */
+       if (!(ret & VM_FAULT_ERROR) && (vmf->flags & FAULT_FLAG_WRITE))
+               ret = VM_FAULT_RETRY;
+
 out:
        dma_resv_unlock(obj->resv);


?  but it seems that the driver is in slightly uncharted waters.
try_insert_pfn() calls vmf_insert_pfn(), which says

/**
 * vmf_insert_pfn - insert single pfn into user vma
 * @vma: user vma to map to
 * @addr: target user address of this page
 * @pfn: source kernel pfn

...

 * vma cannot be a COW mapping.

except this *is* a COW mapping in some sense, or at least it
faults like one.

Paolo