Re: [PATCH v4 09/16] KVM: Introduce KVM_CAP_NOWAIT_ON_FAULT without implementation

[Sean Christopherson <seanjc@google.com>]

On Thu, Aug 24, 2023, Anish Moorthy wrote:
> On Tue, Jul 11, 2023 at 8:29 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > Well, that description is wrong for other reasons.  As mentioned in my reply
> > (got snipped), the behavior is not tied to sleeping or waiting on I/O.
> >
> > >  Moving the nowait check out of __kvm_faultin_pfn()/user_mem_abort()
> > > and into __gfn_to_pfn_memslot() means that, obviously, other callers
> > > will start to see behavior changes. Some of that is probably actually
> > > necessary for that documentation to be accurate (since any usages of
> > > __gfn_to_pfn_memslot() under KVM_RUN should respect the memslot flag),
> > > but I think there are consumers of __gfn_to_pfn_memslot() from outside
> > > KVM_RUN.
> >
> > Yeah, replace "in response to page faults" with something along the lines of "if
> > an access in guest context ..."
> 
> Alright, how about
> 
> + KVM_MEM_NO_USERFAULT_ON_GUEST_ACCESS
> + The presence of this capability indicates that userspace may pass the
> + KVM_MEM_NO_USERFAULT_ON_GUEST_ACCESS flag to
> + KVM_SET_USER_MEMORY_REGION. Said flag will cause KVM_RUN to fail (-EFAULT)
> + in response to guest-context memory accesses which would require KVM
> + to page fault on the userspace mapping.
> 
> Although, as Wang mentioned, USERFAULT seems to suggest something
> related to userfaultfd which is a liiiiitle too specific. Perhaps we
> should use USERSPACE_FAULT (*cries*) instead?

Heh, it's not strictly on guest accesses though.

At this point, I'm tempted to come up with some completely arbitrary name for the
feature and give up on trying to describe its effects in the name itself.

> On Wed, Jun 14, 2023 at 2:20 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > However, do we actually need to force vendor code to query nowait?  At a glance,
> > the only external (relative to kvm_main.c) users of __gfn_to_pfn_memslot() are
> > in flows that play nice with nowait or that don't support it at all.  So I *think*
> > we can do this?
> 
> On Wed, Jun 14, 2023 at 2:23 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > Gah, got turned around and forgot to account for @atomic.  So this?
> >
> >         if (!atomic && memslot_is_nowait_on_fault(slot)) {
> >                 atomic = true;
> >                 if (async) {
> >                         *async = false;
> >                         async = NULL;
> >                 }
> >         }
> >
> > > +
> > >         return hva_to_pfn(addr, atomic, interruptible, async, write_fault,
> > >                           writable);
> > >  }
> 
> Took another look at this and I *think* it works too (I added my notes
> at the end here so if anyone wants to double-check they can). But
> there are a couple of quirks
> 
> 1. The memslot flag can cause new __gfn_to_pfn_memslot() failures in
> kvm_vcpu_map() (good thing!) but those failures result in an EINVAL
> (bad!). It kinda looks like the current is_error_noslot_pfn() check in
> that function should be returning EFAULT anyways though, any opinions?

Argh, it "needs" to return -EINVAL because KVM "needs" to inject a #GP if the guest
accesses a non-existent PFN in various nested SVM flows.  It's somewhat of a moot
point though, because kvm_vcpu_map() can't fail, KVM just isn't equipped to report
such failures out to userspace.

> 2. kvm_vm_ioctl_mte_copy_tags() will see new failures. This function
> has come up before (a) and it doesn't seem like an access in a guest
> context. Is this something to just be documented away?

We'll need a way to way for KVM to opt-out for kvm_vcpu_map(), at which point it
makes sense to opt-out for kvm_vm_ioctl_mte_copy_tags() as well.

> 3. I don't think I've caught parts of the who-calls tree that are in
> drivers/. The one part I know about is the gfn_to_pfn() call in
> is_2MB_gtt_possible() (drivers/gpu/drm/i915/gvt/gtt.c), and I'm not
> sure what to do about it. Again, doesn't look like a guest-context
> access.

On x86, that _was_ the only one.  You're welcome ;-)

https://lore.kernel.org/all/20230729013535.1070024-9-seanjc@google.com