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

From: Anish Moorthy <amoorthy@google.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Oliver Upton <oliver.upton@linux.dev>,
	Chao Peng <chao.p.peng@linux.intel.com>,
	kvm@vger.kernel.org, James Houghton <jthoughton@google.com>
Subject: Re: [PATCH 5/8] kvm: Add cap/kvm_run field for memory fault exits
Date: Thu, 23 Feb 2023 15:03:35 -0800	[thread overview]
Message-ID: <CAF7b7mqV4p_t4yJx6yyFFk7AQ2w6jVDCXUQfA+aza_OQya2qfA@mail.gmail.com> (raw)
In-Reply-To: <Y/fS0eab7GG0NVKS@google.com>

On Thu, Feb 23, 2023 at 12:55 PM Sean Christopherson <seanjc@google.com> wrote:
> 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 :-)

Oof, that *is* pretty bad. I think I have the cause figured out
though, so I'll be careful about that from now on :)

>   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

My understanding of your previous message was off: I didn't realize
that fast gup would also fail for present mappings where the
read/write permission was incorrect. So I'd agree that
KVM_MEMORY_FAULT_EXIT_REASON_ABSENT should be dropped: best not to
mislead with false positives.

> >
> > (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.

I'm actually not sure where all of the sources of guest-memory
-EFAULTs are or how I'd go about finding them. Is the standalone
behavior of KVM_CAP_MEMORY_FAULT_EXIT something you're suggesting
because that new name is too broad for what it does right now? If so
then I'd rather just rename it again: but if that functionality should
be included with this series, then I'll take a look at the required
work given a couple of pointers :)

I will say, a partial implementation seems worse than no
implementation: isn't there a risk that someone ends up depending on
the incomplete behavior?