Re: [PATCH v4 03/16] KVM: Add KVM_CAP_MEMORY_FAULT_INFO

[Sean Christopherson <seanjc@google.com>]

On Fri, Aug 18, 2023, Anish Moorthy wrote:
> On Thu, Aug 17, 2023 at 4:58 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Aug 16, 2023, Anish Moorthy wrote:
> > > > but the names just need to be unique, e.g. the below compiles just fine.  So unless
> > > > someone has a better idea, using a separate union for exits that might be clobbered
> > > > seems like the way to go.
> > >
> > > Agreed. By the way, what was the reason why you wanted to avoid the
> > > exit reason canary being ABI?
> >
> > Because it doesn't need to be exposed to usersepace, and it would be quite
> > unfortunate if we ever need/want to drop the canary, but can't because it's exposed
> > to userspace.
> 
> > No need for a full 32-bit value, or even a separate field, we can use kvm_run.flags.
> > Ugh, but of course flags' usage is arch specific.  *sigh*
> 
> Ah, I realise now you're thinking of separating the canary and
> whatever userspace reads to check for an annotated memory fault. I
> think that even if one variable in kvm_run did double-duty for now,
> we'd always be able to switch to using another as the canary without
> changing the ABI. But I'm on board with separating them anyways.
> 
> > Regarding the canary, if we want to use it for WARN_ON_ONCE(), it can't be
> > exposed to userspace.  Either that or we need to guard the WARN in some way.
> 
> It's guarded behind a kconfig atm, although if we decide to drop the
> userspace-visible canary then I'll drop that bit.

Yeah, burning a kconfig for this probably overkill.

> > > On Wed, Jun 14, 2023 at 10:35 AM Sean Christopherson <seanjc@google.com> wrote:
> > > > +       __u32 speculative_exit_reason;
> > ...
> > We can either defines a flags2 (blech), or grab the upper byte of flags for
> > arch agnostic flags (slightly less blech).
> 
> Grabbing the upper byte seems reasonable: but do you anticipate KVM
> ever supporting more than eight of these speculative exits? Because if
> so then it seems like it'd be less trouble to use a separate u32 or
> u16 (or even u8, judging by the number of KVM exits). Not sure how
> much future-proofing is appropriate here :)

I don't anticipate anything beyond the memory fault case.  We essentially already
treat incomplete exits to userspace as KVM bugs.   MMIO is the only other case I
can think of where KVM doesn't complete an exit to usersapce, but that one is
essentially getting grandfathered in because of x86's flawed MMIO handling.
Userspace memory faults also get grandfathered in because of paravirt ABIs, i.e.
KVM is effectively required to ignore some faults due to external forces.

In other words, the bar for adding "speculative" exit to userspace is very high.

> > > > +       union {
> > > > +               /* KVM_SPEC_EXIT_MEMORY_FAULT */
> >
> > Definitely just KVM_EXIT_MEMORY_FAULT, the vast, vast majority of exits to
> > userspace will not be speculative in any way.
> 
> Speaking of future-proofing, this was me trying to anticipate future
> uses of the speculative exit struct: I figured that some case might
> come along where KVM_RUN returns 0 *and* the contents of the speculative
> exit struct might be useful- it'd be weird to look for KVM_EXIT_*s in
> two different fields.

That can be handled with a comment about the new flag, e.g.

/*
 * Set if KVM filled the memory_fault field since the start of KVM_RUN.  Note,
 * memory_fault is guaranteed to be fresh if and only KVM_RUN returns -EFAULT.
 * For all other return values, memory_fault may be stale and should be
 * considered informational only, e.g. can captured to aid debug, but shouldn't
 * be relied on for correctness.
 */
#define 	KVM_RUN_MEMORY_FAULT_FILLED