Re: [PATCH v3] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Sean Christopherson <seanjc@google.com>]

On Thu, Mar 31, 2022, Peter Gonda wrote:
> On Thu, Mar 31, 2022 at 11:48 AM Marc Orr <marcorr@google.com> wrote:
> >
> > On Thu, Mar 31, 2022 at 10:40 AM Marc Orr <marcorr@google.com> wrote:
> > >
> > > On Thu, Mar 31, 2022 at 10:11 AM Sean Christopherson <seanjc@google.com> wrote:
> > > >
> > > > +Paolo and Vitaly
> > > >
> > > > In the future, I highly recommend using scripts/get_maintainers.pl.
> > > >
> > > > On Wed, Mar 30, 2022, Peter Gonda wrote:
> > > > > SEV-ES guests can request termination using the GHCB's MSR protocol. See
> > > > > AMD's GHCB spec section '4.1.13 Termination Request'. Currently when a
> > > > > guest does this the userspace VMM sees an KVM_EXIT_UNKNOWN (-EVINAL)
> > > > > return code from KVM_RUN. By adding a KVM_EXIT_SHUTDOWN_ENTRY to kvm_run
> > > > > struct the userspace VMM can clearly see the guest has requested a SEV-ES
> > > > > termination including the termination reason code set and reason code.
> > > > >
> > > > > Signed-off-by: Peter Gonda <pgonda@google.com>
> > > > >
> > > > > ---
> > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > > > index 75fa6dd268f0..5f9d37dd3f6f 100644
> > > > > --- a/arch/x86/kvm/svm/sev.c
> > > > > +++ b/arch/x86/kvm/svm/sev.c
> > > > > @@ -2735,8 +2735,13 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
> > > > >               pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
> > > > >                       reason_set, reason_code);
> > > >
> > > > This pr_info() should be removed.  A malicious usersepace could spam the kernel
> > > > by constantly running a vCPU that requests termination.
> >
> > Though... this patch makes this pr_info redundant! Since we'll now
> > report this in userspace. Actually, I'd be OK to remove it.
> 
> I'll make this 2 patches. This current patch and another to rate limit
> this pr_info() I think this patch is doing a lot already so would
> prefer to just add a second. Is that reasonable?

I strongly prefer removing the pr_info() entirely.  As Marc pointed out, the
info is redundant when KVM properly reports the issue.  And worse, the info is
useless unless there's exactly one VM running.  Even then, it doesn't capture
which vCPU failed.  This is exactly why Jim, myself, and others, have been pushing
to avoid using dmesg to report guest errors.  They're helpful for initial
development, but dead weight for production, and if they're helpful for development
then odds are good that having proper reporting in production would also be valuable.

> > > Quoting the spec:
> > > The reason code set is meant to provide hypervisors with their own
> > > termination SEV-ES Guest-Hypervisor Communication Block
> > > Standardization reason codes. This document defines and owns reason
> > > code set 0x0 and the following reason codes (GHCBData[23:16]):
> > > 0x00 – General termination request
> > > 0x01 – SEV-ES / GHCB Protocol range is not supported.
> > > 0x02 – SEV-SNP features not supported
> >
> > Reading this again, I now see that "reason_set" sounds like "The
> > reason code is set". I bet that's how Sean read it during his review.
> > So yeah, this needs comments :-)!
> 
> I'll add comments but I agree with Marc. These are part of the GHCB
> spec so for the very specific SEV-ES termination reason we should
> include all the data the spec allows. Sounds OK?

Ugh, so "set" means "set of reason codes"?  That's heinous naming.  I don't have a
strong objection to splitting, but at the same time, why not punt it to userspace?
Userspace is obviously going to have to understand what the hell "set" means
anyways...