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

[Sean Christopherson <seanjc@google.com>]

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

Heh, if anyone can says they can list _all_ sources of KVM accesses to guest
memory of the top of their head, they're lying.

Finding the sources is a bit tedious, but shouldn't be too hard.  The scope is
is -EFAULT in the context of KVM_RUN that gets returned to userspace.  There are
150+ references to -EFAULT to wade through, but the vast majority are obviously
not in scope, e.g. are for uaccess failures in other ioctls().

> 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?

No, I want a standalone thing because I want to start driving toward KVM never
returning -EFAULT to host userspace for accesses to guest memory in the context
of ioctl(KVM_RUN).  E.g. I want to replace the "return -EFAULT" in
kvm_handle_error_pfn() with something like "return kvm_handle_memory_fault_exit(...)".

My hope/goal is that return useful information will allow userspace to do more
interesting things with guest memory without needing invasive, one-off changes
to KVM.  At the very least, it should get us to the point where a memory fault
from KVM_RUN exits to userspace with sane, helpful information, not a generic
"-EFAULT, have fun debugging!".

> 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?

In this case, I don't think so.  We definitely would need to document that KVM
may still return -EFAULT in certain scenarios, but we have at least one known
use case (this one) where catching the common cases is sufficient.  And if/when
use cases come along that need 100% accuracy, we can hunt down and fix the KVM
remaining "bugs".