Re: [PATCH v3 05/22] KVM: Add KVM_CAP_MEMORY_FAULT_INFO

From: Anish Moorthy <amoorthy@google.com>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: pbonzini@redhat.com, maz@kernel.org, seanjc@google.com,
	jthoughton@google.com, bgardon@google.com, dmatlack@google.com,
	ricarkol@google.com, axelrasmussen@google.com, peterx@redhat.com,
	kvm@vger.kernel.org, kvmarm@lists.linux.dev
Subject: Re: [PATCH v3 05/22] KVM: Add KVM_CAP_MEMORY_FAULT_INFO
Date: Thu, 1 Jun 2023 13:30:58 -0700	[thread overview]
Message-ID: <CAF7b7mrK+SgyxjYqMyJC0PA4C8SFRX_Q=x7Db+Ck8i89wzvw8w@mail.gmail.com> (raw)
In-Reply-To: <ZHj25HsCExz/uCo/@linux.dev>

On Thu, Jun 1, 2023 at 12:52 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> So the other angle of my concern w.r.t. NOWAIT exits is the fact that
> userspace gets to decide whether or not we annotate such an exit. We all
> agree that a NOWAIT exit w/o context isn't actionable, right?

Yup

> Sean is suggesting that we abuse the fact that kvm_run already contains
> junk for EFAULT exits and populate kvm_run::memory_fault unconditionally
> [*]. I agree with him, and it eliminates the odd quirk of 'bare' NOWAIT
> exits too. Old userspace will still see 'garbage' in kvm_run struct,
> but one man's trash is another man's treasure after all :)
>
> So, based on that, could you:
>
>  - Unconditionally prepare MEMORY_FAULT exits everywhere you're
>    converting here
>
>  - Redefine KVM_CAP_MEMORY_FAULT_INFO as an informational cap, and do
>    not accept an attempt to enable it. Instead, have calls to
>    KVM_CHECK_EXTENSION return a set of flags describing the supported
>    feature set.

Sure. I've been collecting feedback as it comes in, so I can send up a
v4 with everything up to now soon. The major thing left to resolve is
that the exact set of annotations is still waiting on feedback: I've
already gone ahead and dropped everything I wasn't sure of in [1], so
the next version will be quite a bit smaller. If it turns out that
I've dropped too much, then I can add things back in based on the
feedback.

[1] https://lore.kernel.org/kvm/20230412213510.1220557-1-amoorthy@google.com/T/#mfe28e6a5015b7cd8c5ea1c351b0ca194aeb33daf

>    Eventually, you can stuff a bit in there to advertise that all
>    EFAULTs are reliable.

I don't think this is an objective: the idea is to annotate efaults
tracing back to user accesses (see [2]). Although the idea of
annotating with some "unrecoverable" flag set for other efaults has
been tossed around, so we may end up with that.

[2] https://lore.kernel.org/kvm/20230412213510.1220557-1-amoorthy@google.com/T/#m5715f3a14a6a9ff9a4188918ec105592f0bfc69a

> [*] https://lore.kernel.org/kvmarm/ZHjqkdEOVUiazj5d@google.com/
>
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index cf7d3de6f3689..f3effc93cbef3 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1142,6 +1142,7 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
> >       spin_lock_init(&kvm->mn_invalidate_lock);
> >       rcuwait_init(&kvm->mn_memslots_update_rcuwait);
> >       xa_init(&kvm->vcpu_array);
> > +     kvm->fill_efault_info = false;
> >
> >       INIT_LIST_HEAD(&kvm->gpc_list);
> >       spin_lock_init(&kvm->gpc_lock);
> > @@ -4096,6 +4097,8 @@ static long kvm_vcpu_ioctl(struct file *filp,
> >                       put_pid(oldpid);
> >               }
> >               r = kvm_arch_vcpu_ioctl_run(vcpu);
> > +             WARN_ON_ONCE(r == -EFAULT &&
> > +                                      vcpu->run->exit_reason != KVM_EXIT_MEMORY_FAULT);
>
> This might be a bit overkill, as it will definitely fire on unsupported
> architectures. Instead you may want to condition this on an architecture
> actually selecting support for MEMORY_FAULT_INFO.

Ah, that's embarrassing. Thanks for the catch.