Re: [PATCH v5 10/17] KVM: Implement KVM_CAP_USERFAULT_ON_MISSING by atomizing __gfn_to_pfn_memslot() calls

From: Sean Christopherson <seanjc@google.com>
To: Anish Moorthy <amoorthy@google.com>
Cc: David Matlack <dmatlack@google.com>,
	oliver.upton@linux.dev, kvm@vger.kernel.org,
	 kvmarm@lists.linux.dev, pbonzini@redhat.com, maz@kernel.org,
	 robert.hoo.linux@gmail.com, jthoughton@google.com,
	axelrasmussen@google.com,  peterx@redhat.com,
	nadav.amit@gmail.com, isaku.yamahata@gmail.com,
	 kconsul@linux.vnet.ibm.com
Subject: Re: [PATCH v5 10/17] KVM: Implement KVM_CAP_USERFAULT_ON_MISSING by atomizing __gfn_to_pfn_memslot() calls
Date: Fri, 3 Nov 2023 13:05:07 -0700	[thread overview]
Message-ID: <ZUVSc-Cu3LaeAcWd@google.com> (raw)
In-Reply-To: <CAF7b7mpzkjvBTybbaEUSp7iL3dVURVi+rDtkkojOcXAY=Bk9=g@mail.gmail.com>

On Thu, Nov 02, 2023, Anish Moorthy wrote:
> I'm going to squash this into patch 9: the fact that it's separated is
> a holdover from when the memslot flag check lived in arch-specific
> code.
> 
> Proposed commit message for the squashed commit
> 
> > KVM: Add KVM_CAP_EXIT_ON_MISSING which forbids page faults in stage-2 fault handlers

Please don't use "forbids page faults".  As I said earlier:

 : "Forbid fault" is rather nonsensical because a fault has already happened.  The
 : confusion between "page fault VM-Exit" and "faulting in memory in the host" is
 : the main reason we wandered away from anything with "fault" in the name.

The part that is being "forbidden" is not a "page fault", it's the act of faulting
in a page.  And "forbids" doesn't add any clarity to KVM_CAP_EXIT_ON_MISSING; if
anything, it muddies the waters by making it sound like page faults somehow become
fatal.

Regarding the capability, stating the capability name is both unnecessary and
uninteresting.  The fact that there's a new capability is an implementation detail,
it's in no way needed to understand the basic gist of the patch, which is basically
*the* role of the shortlog.

The key things to cover in the shortlog:

 * What does the feature do?   Exits when an hva isn't fully mapped
 * Who controls the behavior?  Userspace
 * How is the feature enabled? Memslot flag

I would go with something like:

  KVM: Add memslot flag to let userspace force an exit on missing hva mappings

> > Faulting in pages via the stage-2 fault handlers can be undesirable in
> > the context of userfaultfd-based postcopy: contention for userfaultfd's
> > internal wait queues can cause significant slowness when delivering
> > faults to userspace. A performant alternative is to allow the stage-2
> > fault handlers to, where they would currently page-fault on the
> > userspace mappings, exit from KVM_RUN and deliver relevant information
> > to userspace via KVM_CAP_MEMORY_FAULT_INFO. This approach avoids
> > contention-related issues.
> 
> > Add, and mostly implement, a capability through which userspace can
> > indicate to KVM (through the new KVM_MEM_EXIT_ON_MISSING memslot flag)
> > that it should not fault in pages from the stage-2 fault handlers.
> 
> > The basic implementation strategy is to check the memslot flag from
> > within __gfn_to_pfn_memslot() and override the "atomic" parameter
> > accordingly. Some callers (such as kvm_vcpu_map()) must be able to opt
> > out of this behavior, and do so by passing the new can_exit_on_missing
> > parameter as false.
> 
> > No functional change intended: nothing sets KVM_MEM_EXIT_ON_MISSING.
> 
> One comment/question though- as I have the (sqaushed) patches/new
> commit message written, I think it could mislead readers into thinking
> that callers that pass can_exit_on_missing=false to
> __gfn_to_pfn_memslot() *must* do so. But at least some of these cases,
> such as the ones in the powerpc mmu, I think we're just punting on.
> 
> I see a few options here
> 
> 1. Make all callers pass can_exit_on_missing=false, and leave the
> =true update to the x86/arm64 specific "enable/annotate
> KVM_EXIT_ON_MISSING" commits [my preference]

This one.  Nothing should pass %true until the caller fully supports
KVM_MEM_EXIT_ON_MISSING.

> 2. Make the powerpc callers pass can_exit_on_missing=true as well,
> even though we're not adding KVM_CAP_EXIT_ON_MISSING for them
> 
> 3. Add a disclaimer in the commit message that some cases where
> can_exit_on_missing=false could become =true in the future
> 
> 4. Things are fine as-is and I'm making a mountain out of a molehill
> 
> Any thoughts?