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

[Sean Christopherson <seanjc@google.com>]

On Wed, Aug 30, 2023, Anish Moorthy wrote:
> On Tue, Aug 29, 2023 at 3:42 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > 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.
> 
> Is the inaccuracy just because of the KVM_DEV_ARM_VGIC_GRP_CTRL
> disclaimer, or something else? I thought that "guest-context accesses"
> would capture the flag affecting memory accesses that KVM emulates for
> the guest as well, in addition to the "normal" EPT-violation -> page
> fault path. But if that's still not totally accurate then you should
> probably just spell this out for me.

A pedantic interpretation of "on guest access" could be that the flag would only
apply to accesses from the guest itself, i.e. not any emulated accesses.

In general, I think we should avoid having the name describe when KVM honors the
flag, because it'll limit our ability to extend KVM functionality, and I doubt
we'll ever be 100% accurate, e.g. guest emulation that "needs" kvm_vcpu_map() will
ignore the flag.

Regarding USERFAULT, why not lean into that instead of trying to avoid it?  The
behavior *is* related to userfaultfd; not in code, but certainly in its purpose.
I don't think it's a stretch to say that userfault doesn't _just_ mean the fault
is induced by userspace, it also means that the fault is relayed to userspace.
And we can even borrow some amount of UFFD nomenclature to make it easier for
userspace to understand the purpose.

For initial support, I'm thinking

  KVM_MEM_USERFAULT_ON_MISSING

i.e. generate a "user fault" when the mapping is missing.  That would give us
leeway for future expansion, e.g. if someday there's a use case for generating a
userfault exit on major faults but not on missing mappings or minor fault, we
could add KVM_MEM_USERFAULT_ON_MAJOR.
 
> > > On Wed, Jun 14, 2023 at 2:20 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > 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.
> 
> Uh oh, I sense another parameter to __gfn_to_pfn_memslot(). Although I
> did see that David Stevens has been proposing cleanups to that code
> [1]. Is proper practice here to take a dependency on his series, do we
> just resolve the conflicts when the series are merged, or something
> else?

No, don't take a dependency.  At this point, it's a coin toss as to which series
will be ready first, taking a dependency could unnecessarily slow this series down
and/or generate pointless work.  Whoever "loses" is likely going to have a somewhat
painful rebase to deal with, but I can help on that front if/when the time comes.

As for what is "proper practice", it's always a case-by-case basis, but a good rule
of thumb is to default to letting the maintainer handle conflicts (though definitely
call out any known conflicts to make life easier for everyone), and if you suspect
that your series will have non-trivial conflicts, ask for guidance (like you just
did).