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

From: Marc Orr <marcorr@google.com>
To: Peter Gonda <pgonda@google.com>
Cc: kvm list <kvm@vger.kernel.org>, Borislav Petkov <bp@alien8.de>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Brijesh Singh <brijesh.singh@amd.com>,
	Joerg Roedel <jroedel@suse.de>,
	Sean Christopherson <seanjc@google.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] Add KVM_EXIT_SHUTDOWN metadata for SEV-ES
Date: Mon, 21 Mar 2022 12:45:13 -0700	[thread overview]
Message-ID: <CAA03e5GDyM1O6aEYcpUnTY4JLBvOqQugWzpXefD9YGEkSuALVA@mail.gmail.com> (raw)
In-Reply-To: <CAMkAt6qbauEn1jGUYLQc6QURhCHLu7eDmzJhfHZZXN9FGbQOMA@mail.gmail.com>

On Mon, Mar 21, 2022 at 11:08 AM Peter Gonda <pgonda@google.com> wrote:
>
> > > 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);
> > >
> > > -               ret = -EINVAL;
> > > -               break;
> > > +               vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
> > > +               vcpu->run->shutdown.reason = KVM_SHUTDOWN_SEV_TERM;
> > > +               vcpu->run->shutdown.ndata = 2;
> > > +               vcpu->run->shutdown.data[0] = reason_set;
> > > +               vcpu->run->shutdown.data[1] = reason_code;
> > > +
> > > +               return 0;
> >
> > Maybe I'm missing something, but don't we want to keep returning an error?
> >
> > rationale: Current behavior: return -EINVAL to userpsace, but
> > userpsace cannot infer where the -EINVAL came from. After this patch:
> > We should still return -EINVAL to userspace, but now userspace can
> > parse this new info in the KVM run struct to properly terminate.
> >
>
> I removed the error return code here since an SEV guest may request a
> termination due to no fault of the host at all. This is now inline
> with any other shutdown requested by the guest. I don't have a strong
> preference here but EINVAL doesn't seem correct in all cases, do
> others have any thoughts on this?

Makes sense. Yeah, let's see if others have an opinion. Otherwise, I'm
fine either way. Now that you mention it, returning an error to
userspace when the guest triggered the self-termination, and could've
done so for reasons outside the host's control, is a little odd. But
at the same time, it's just as likely that the guest is
self-terminating due to a host-side error. So I guess it's not clear
whether returning an error here is "right" or "wrong".