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>,
	Chao Peng <chao.p.peng@linux.intel.com>,
	kvm@vger.kernel.org
Subject: Re: [PATCH 5/8] kvm: Add cap/kvm_run field for memory fault exits
Date: Thu, 23 Feb 2023 12:55:45 -0800	[thread overview]
Message-ID: <Y/fS0eab7GG0NVKS@google.com> (raw)
In-Reply-To: <CAF7b7mpMiw=6o6vTsqFR6HUUCJL+1MSTDUsMaKLnS1NqyVf-9A@mail.gmail.com>

On Wed, Feb 22, 2023, Anish Moorthy wrote:
> On Fri, Feb 17, 2023 at 12:33 PM Sean Christopherson <seanjc@google.com> wrote
> > > > > 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 :-)
> > ...
> > 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.
> 
> Ok so I have a v2 of the series basically ready to go, but I realized
> that I should
> probably have brought up my modified API here to make sure it was
> sane: so, I'll do
> that first
> 
> In v2, I've
> (1)  renamed the kvm cap from KVM_CAP_MEM_FAULT_NOWAIT to
> KVM_CAP_MEMORY_FAULT_EXIT due to Sean's earlier comment
> 
> > 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.
> 
> (2) kept the definition of kvm_run.memory_fault as
> struct {
>     __u64 flags;
>     __u64 gpa;
>     __ u64 len;
> } memory_fault;
> which, apart from the name of the "len" field, is exactly what Chao
> has in their series.

Off-topic, please adjust whatever email client you're using to not wrap so
agressively and at seeming random times.

As written, this makes my eyes bleed, whereas formatting like so does not :-)

  Ok so I have a v2 of the series basically ready to go, but I realized that I
  should probably have brought up my modified API here to make sure it was sane:
  so, I'll do that first

  ...

  which, apart from the name of the "len" field, is exactly what Chao
  has in their series.

  Flags remains a bitfield describing the reason for the memory fault:
  in the two places this series generates this fault, it sets a bit in flags.
  Userspace is meant to check whether a memory_fault was generated due to
  KVM_CAP_MEMORY_FAULT_EXIT using the KVM_MEMORY_FAULT_EXIT_REASON_ABSENT mask.

> flags remains a bitfield describing the reason for the memory fault:
> in the two places
> this series generates this fault, it sets a bit in flags. Userspace is
> meant to check whether
> a memory_fault was generated due to KVM_CAP_MEMORY_FAULT_EXIT using the
> KVM_MEMORY_FAULT_EXIT_REASON_ABSENT mask.

Before sending a new version, let's bottom out on whether or not a
KVM_MEMORY_FAULT_EXIT_REASON_ABSENT flag is necessary.  I'm not dead set against
a flag, but as I called out earlier[*], it can have false positives.  I.e. userspace
needs to be able to suss out the real problem anyways.  And if userspace needs to
be that smart, what's the point of the flag?

[*] https://lore.kernel.org/all/Y+%2FkgMxQPOswAz%2F2@google.com

> 
> (3) switched over to a memslot flag: KVM_CAP_MEMORY_FAULT_EXIT simply
> indicates whether this flag is supported.

The new memslot flag should depend on KVM_CAP_MEMORY_FAULT_EXIT, but
KVM_CAP_MEMORY_FAULT_EXIT should be a standalone thing, i.e. should convert "all"
guest-memory -EFAULTS to KVM_CAP_MEMORY_FAULT_EXIT.  All in quotes because I would
likely be ok with a partial conversion for the initial implementation if there
are paths that would require an absurd amount of work to convert.

> As such, trying to enable the new capability directly generates an EINVAL,
> which is meant to make the incorrect usage clear.
> 
> Hopefully this all appears sane?