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

On Fri, Jul 31, 2026 at 3:37 PM Sergio Lopez Pascual <[email protected]> wrote:
> > IMO, that's a bug in the APIs.  If a normal #PF handler looked at a write fault
> > and decided a read-only mapping sufficed, that would be a bug.  I don't see why
> > this is any different.
>
> In the scenario I've described above, were a read-only PTE has been
> installed on a fault generated by a read access, and a second fault is
> generated later when writting to the page, we aren't invoking any #PF
> handler. The call graph looks (roughly) like this:
>
>           kvm_handle_guest_abort()
>             ├─ gfn_to_memslot() → gfn_to_hva()
>             └─ user_mem_abort()
>                 └─ kvm_s2_fault_pin_pfn()
>                     └─ __kvm_faultin_pfn()
>                         └─ hva_to_pfn()
>                             ├─ hva_to_pfn_fast()
>                             ├─ hva_to_pfn_slow()
>                             └─ hva_to_pfn_remapped()

There are two different bugs. First, let's look at this case where, as
you said, fixup_user_fault() is not executed. To force execution of
fixup_user_fault(), you'd need something like this in KVM:

     r = follow_pfnmap_start(&args);
    if (!r && write_fault && !args.writable) {
        follow_pfnmap_end(&args);
        r = -EACCES;
    }

     if (r) {

But that's also a bad idea for two reasons. First because you'd need
it for all callers.  Second, because all of follow_pfnmap_start()'s
callers anyway check something like "write_fault && !args.writable",
so the function might as well receive a new args.write_fault, and fail
with -EFAULT if args.write_fault && !writable.

Separately there is the case where fixup_user_fault() is executed, and
which right now would have to be done twice. The drm code already has
a solution in try_insert_pfn():

      /* Unlike PTEs which are automatically upgraded to
       * writeable entries, the PMD upgrades go through
       * .huge_fault(). Make sure we pass the "write" info
       * along in that case.
       * This also means we have to record the write fault
       * here, instead of in .pfn_mkwrite().
       */
      ret = vmf_insert_pfn_pmd(vmf, pfn,
             vmf->flags & FAULT_FLAG_WRITE);

The driver could do the same even for PTEs, not just for PMDs. The
issue of code duplication is now on the other side, because every
caller has to do the job of .pfn_mkwrite() and in fact the same bug is
there in vmwgfx. But the exact kind of processing to be done in
.pfn_mkwrite() is different per driver, as opposed to
fixup_user_fault() users, and there is a precedent for the API with
vm_insert_page_mkwrite().

There is no function to do it, but that's fixable. I will send an RFC
series shortly.

Paolo