Re: [PATCH 5/8] kvm: Add cap/kvm_run field for memory fault exits

From: Sean Christopherson <seanjc@google.com>
To: Anish Moorthy <amoorthy@google.com>
Cc: Oliver Upton <oliver.upton@linux.dev>,
	Marc Zyngier <maz@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	James Houghton <jthoughton@google.com>,
	Ben Gardon <bgardon@google.com>,
	David Matlack <dmatlack@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Chao Peng <chao.p.peng@linux.intel.com>,
	Axel Rasmussen <axelrasmussen@google.com>,
	kvm@vger.kernel.org, kvmarm@lists.linux.dev, peterx@redhat.com
Subject: Re: [PATCH 5/8] kvm: Add cap/kvm_run field for memory fault exits
Date: Fri, 17 Feb 2023 12:33:04 -0800	[thread overview]
Message-ID: <Y+/kgMxQPOswAz/2@google.com> (raw)
In-Reply-To: <CAF7b7mqeXcHdFHewX3enn-vxf6y7CUWjXjB3TXithZ_PnzVLQQ@mail.gmail.com>

On Fri, Feb 17, 2023, Anish Moorthy wrote:
> On Thu, Feb 16, 2023 at 10:53 AM Anish Moorthy <amoorthy@google.com> wrote:
> >
> > On Wed, Feb 15, 2023 at 12:59 AM Oliver Upton <oliver.upton@linux.dev> wrote:
> > >
> > > How is userspace expected to differentiate the gup_fast() failed exit
> > > from the guest-private memory exit?

Sorry, missed this comment first time around.  I don't see any reason why userspace
needs to uniquely identify the gup_fast() case.  Similar to page faults from
hardware, KVM should describe the access in sufficient detail so that userspace
can resolve the issue, whatever that may be, but KVM should make any assumptions
about _why_ the failure occurred.  

gup_fast() failing in itself isn't interesting.  The _intended_ behavior is that
KVM will exit if and only if the guest accesses a valid page that hasn't yet been
transfered from the source, but the _actual_ behavior is that KVM will exit if
the page isn't faulted in for _any_ reason.  Even tagging the access NOWAIT is
speculative to some degree, as that suggests the access may have succeeded if
KVM "waited", which may or may not be true.

E.g. see the virtiofs trunctionation use case that planted the original seed for
this approach: https://lore.kernel.org/kvm/20201005161620.GC11938@linux.intel.com.

> > > I don't think flags are a good idea for this, as it comes with the
> > > illusion that both events can happen on a single exit. In reality, these
> > > are mutually exclusive.

They aren't mutually exclusive.  Obviously KVM will only detect one other the
other, but it's entirely possible that a guest could be accessing the "wrong"
flavor of a page _and_ for that flavor to not be faulted in.  A smart userspace
should see that (a) it needs to change the memory attributes and (b) that it
needs to demand the to-be-installed page from the source.

> > > A fault type/code would be better here, with the option to add flags at
> > > a later date that could be used to further describe the exit (if
> > > needed).
> >
> > Agreed.

Not agreed :-)

> > +    struct {
> > +        __u32 fault_code;
> > +        __u64 reserved;
> > +        __u64 gpa;
> > +        __u64 size;
> > +    } memory_fault;
> >
> > The "reserved" field is meant to be the placeholder for a future "flags" field.
> > Let me know if there's a better/more conventional way to achieve this.
> 
> On Thu, Feb 16, 2023 at 10:53 AM Anish Moorthy <amoorthy@google.com> wrote:
> >
> > 1. As Oliver touches on earlier, we'll probably want to use this same field for
> >    different classes of memory fault in the future (such as the ones which Chao
> >    is introducing in [1]): so it does make sense to add "code" and "flags"
> >    fields which can be used to communicate more information to the user (and
> >    which can just be set to MEM_FAULT_NOWAIT/0 in this series).
> 
> Let me walk back my responses here: I took a closer look at Chao's series, and
> it doesn't seem that I should be trying to share KVM_EXIT_MEMORY_FAULT with it
> in the first place. As far as I can understand (not that much, to be clear :)
> we're signaling unrelated things, so it makes more sense to use different exits
> (ie, rename mine to KVM_EXIT_MEMORY_FAULT_NOWAIT). That would prevent any
> potential confusion about mutual exclusivity.

Hard "no" on a separate exit reason unless someone comes up with a very compelling
argument.

Chao's UPM series is not yet merged, i.e. is not set in stone.  If the proposed
functionality in Chao's series is lacking and/or will conflict with this UFFD,
then we can and should address those issues _before_ it gets merged.