[PATCH V2] KVM: SEV: Update SEV-ES shutdown intercepts with more metadata

[PATCH V2] KVM: SEV: Update SEV-ES shutdown intercepts with more metadata

[Peter Gonda]

Currently if an SEV-ES VM shuts down userspace sees KVM_RUN struct with
only the INVALID_ARGUMENT. This is a very limited amount of information
to debug the situation. Instead KVM can return a
KVM_EXIT_SHUTDOWN to alert userspace the VM is shutting down and
is not usable any further.

Signed-off-by: Peter Gonda <pgonda@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
 arch/x86/kvm/svm/svm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 956726d867aa..cecf6a528c9b 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2131,12 +2131,14 @@ static int shutdown_interception(struct kvm_vcpu *vcpu)
 	 * The VM save area has already been encrypted so it
 	 * cannot be reinitialized - just terminate.
 	 */
-	if (sev_es_guest(vcpu->kvm))
-		return -EINVAL;
+	if (sev_es_guest(vcpu->kvm)) {
+		kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
+		return 0;
+	}
 
 	/*
 	 * VMCB is undefined after a SHUTDOWN intercept.  INIT the vCPU to put
-	 * the VMCB in a known good state.  Unfortuately, KVM doesn't have
+	 * the VMCB in a known good state.  Unfortunately, KVM doesn't have
 	 * KVM_MP_STATE_SHUTDOWN and can't add it without potentially breaking
 	 * userspace.  At a platform view, INIT is acceptable behavior as
 	 * there exist bare metal platforms that automatically INIT the CPU
-- 
2.42.0.283.g2d96d420d3-goog

Re: [PATCH V2] KVM: SEV: Update SEV-ES shutdown intercepts with more metadata

[Tom Lendacky]

On 9/6/23 10:14, Peter Gonda wrote:
> Currently if an SEV-ES VM shuts down userspace sees KVM_RUN struct with

s/down userspace/down, userspace/

> only the INVALID_ARGUMENT. This is a very limited amount of information
> to debug the situation. Instead KVM can return a
> KVM_EXIT_SHUTDOWN to alert userspace the VM is shutting down and
> is not usable any further.
> 
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: x86@kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> ---
>   arch/x86/kvm/svm/svm.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 956726d867aa..cecf6a528c9b 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2131,12 +2131,14 @@ static int shutdown_interception(struct kvm_vcpu *vcpu)
>   	 * The VM save area has already been encrypted so it
>   	 * cannot be reinitialized - just terminate.
>   	 */
> -	if (sev_es_guest(vcpu->kvm))
> -		return -EINVAL;
> +	if (sev_es_guest(vcpu->kvm)) {
> +		kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
> +		return 0;
> +	}

Just a nit... feel free to ignore, but, since KVM_EXIT_SHUTDOWN is also 
set at the end of the function and I don't think kvm_vcpu_reset() clears 
the value from kvm_run, you could just set kvm_run->exit_reason on entry 
and just return 0 early for an SEV-ES guest.

Overall, though:

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

Thanks,
Tom

>   
>   	/*
>   	 * VMCB is undefined after a SHUTDOWN intercept.  INIT the vCPU to put
> -	 * the VMCB in a known good state.  Unfortuately, KVM doesn't have
> +	 * the VMCB in a known good state.  Unfortunately, KVM doesn't have
>   	 * KVM_MP_STATE_SHUTDOWN and can't add it without potentially breaking
>   	 * userspace.  At a platform view, INIT is acceptable behavior as
>   	 * there exist bare metal platforms that automatically INIT the CPU

Re: [PATCH V2] KVM: SEV: Update SEV-ES shutdown intercepts with more metadata

[Sean Christopherson]

On Wed, Sep 06, 2023, Tom Lendacky wrote:
> On 9/6/23 10:14, Peter Gonda wrote:
> > Currently if an SEV-ES VM shuts down userspace sees KVM_RUN struct with
> 
> s/down userspace/down, userspace/

Heh, yeah, I read that the same way you did.

> > only the INVALID_ARGUMENT. This is a very limited amount of information
> > to debug the situation. Instead KVM can return a
> > KVM_EXIT_SHUTDOWN to alert userspace the VM is shutting down and
> > is not usable any further.
> > 
> > Signed-off-by: Peter Gonda <pgonda@google.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Sean Christopherson <seanjc@google.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Joerg Roedel <joro@8bytes.org>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: x86@kernel.org
> > Cc: kvm@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > 
> > ---
> >   arch/x86/kvm/svm/svm.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 956726d867aa..cecf6a528c9b 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -2131,12 +2131,14 @@ static int shutdown_interception(struct kvm_vcpu *vcpu)
> >   	 * The VM save area has already been encrypted so it
> >   	 * cannot be reinitialized - just terminate.
> >   	 */
> > -	if (sev_es_guest(vcpu->kvm))
> > -		return -EINVAL;
> > +	if (sev_es_guest(vcpu->kvm)) {
> > +		kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
> > +		return 0;
> > +	}
> 
> Just a nit... feel free to ignore, but, since KVM_EXIT_SHUTDOWN is also set
> at the end of the function and I don't think kvm_vcpu_reset() clears the
> value from kvm_run, you could just set kvm_run->exit_reason on entry and
> just return 0 early for an SEV-ES guest.

kvm_run is writable by userspace though, so KVM can't rely on kvm_run->exit_reason
for correctness.

And IIUC, the VMSA is also toast, i.e. doing anything other than marking the VM
dead is futile, no?

Re: [PATCH V2] KVM: SEV: Update SEV-ES shutdown intercepts with more metadata

[Tom Lendacky]

On 9/6/23 15:11, Sean Christopherson wrote:
> On Wed, Sep 06, 2023, Tom Lendacky wrote:
>> On 9/6/23 10:14, Peter Gonda wrote:
>>> Currently if an SEV-ES VM shuts down userspace sees KVM_RUN struct with
>>
>> s/down userspace/down, userspace/
> 
> Heh, yeah, I read that the same way you did.
> 
>>> only the INVALID_ARGUMENT. This is a very limited amount of information
>>> to debug the situation. Instead KVM can return a
>>> KVM_EXIT_SHUTDOWN to alert userspace the VM is shutting down and
>>> is not usable any further.
>>>
>>> Signed-off-by: Peter Gonda <pgonda@google.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Sean Christopherson <seanjc@google.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Cc: Joerg Roedel <joro@8bytes.org>
>>> Cc: Borislav Petkov <bp@alien8.de>
>>> Cc: x86@kernel.org
>>> Cc: kvm@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>>
>>> ---
>>>    arch/x86/kvm/svm/svm.c | 8 +++++---
>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index 956726d867aa..cecf6a528c9b 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -2131,12 +2131,14 @@ static int shutdown_interception(struct kvm_vcpu *vcpu)
>>>    	 * The VM save area has already been encrypted so it
>>>    	 * cannot be reinitialized - just terminate.
>>>    	 */
>>> -	if (sev_es_guest(vcpu->kvm))
>>> -		return -EINVAL;
>>> +	if (sev_es_guest(vcpu->kvm)) {
>>> +		kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
>>> +		return 0;
>>> +	}
>>
>> Just a nit... feel free to ignore, but, since KVM_EXIT_SHUTDOWN is also set
>> at the end of the function and I don't think kvm_vcpu_reset() clears the
>> value from kvm_run, you could just set kvm_run->exit_reason on entry and
>> just return 0 early for an SEV-ES guest.
> 
> kvm_run is writable by userspace though, so KVM can't rely on kvm_run->exit_reason
> for correctness.
> 
> And IIUC, the VMSA is also toast, i.e. doing anything other than marking the VM
> dead is futile, no?

I was just saying that "kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;" is in 
the shutdown_interception() function twice now (at both exit points of the 
function) and can probably just be moved to the top of the function and be 
common for both exit points, now, right?

I'm not saying to get rid of it, just set it sooner.

Thanks,
Tom

Re: [PATCH V2] KVM: SEV: Update SEV-ES shutdown intercepts with more metadata

[Sean Christopherson]

On Wed, Sep 06, 2023, Tom Lendacky wrote:
> On 9/6/23 15:11, Sean Christopherson wrote:
> > On Wed, Sep 06, 2023, Tom Lendacky wrote:
> > > On 9/6/23 10:14, Peter Gonda wrote:
> > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > > index 956726d867aa..cecf6a528c9b 100644
> > > > --- a/arch/x86/kvm/svm/svm.c
> > > > +++ b/arch/x86/kvm/svm/svm.c
> > > > @@ -2131,12 +2131,14 @@ static int shutdown_interception(struct kvm_vcpu *vcpu)
> > > >    	 * The VM save area has already been encrypted so it
> > > >    	 * cannot be reinitialized - just terminate.
> > > >    	 */
> > > > -	if (sev_es_guest(vcpu->kvm))
> > > > -		return -EINVAL;
> > > > +	if (sev_es_guest(vcpu->kvm)) {
> > > > +		kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
> > > > +		return 0;
> > > > +	}
> > > 
> > > Just a nit... feel free to ignore, but, since KVM_EXIT_SHUTDOWN is also set
> > > at the end of the function and I don't think kvm_vcpu_reset() clears the
> > > value from kvm_run, you could just set kvm_run->exit_reason on entry and
> > > just return 0 early for an SEV-ES guest.
> > 
> > kvm_run is writable by userspace though, so KVM can't rely on kvm_run->exit_reason
> > for correctness.
> > 
> > And IIUC, the VMSA is also toast, i.e. doing anything other than marking the VM
> > dead is futile, no?
> 
> I was just saying that "kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;" is in the
> shutdown_interception() function twice now (at both exit points of the
> function) and can probably just be moved to the top of the function and be
> common for both exit points, now, right?
> 
> I'm not saying to get rid of it, just set it sooner.

Ah, I thought you were saying bail early from kvm_vcpu_reset().  I agree that not
having completely split logic would be ideal.  What about this?

	/*
	 * VMCB is undefined after a SHUTDOWN intercept.  INIT the vCPU to put
	 * the VMCB in a known good state.  Unfortuately, KVM doesn't have
	 * KVM_MP_STATE_SHUTDOWN and can't add it without potentially breaking
	 * userspace.  At a platform view, INIT is acceptable behavior as
	 * there exist bare metal platforms that automatically INIT the CPU
	 * in response to shutdown.
	 *
	 * The VM save area for SEV-ES guests has already been encrypted so it
	 * cannot be reinitialized, i.e. synthesizing INIT is futile.
	 */
	if (!sev_es_guest(vcpu->kvm)) {
		clear_page(svm->vmcb);
		kvm_vcpu_reset(vcpu, true);
	}

	kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
	return 0;

Re: [PATCH V2] KVM: SEV: Update SEV-ES shutdown intercepts with more metadata

[Peter Gonda]

On Wed, Sep 6, 2023 at 2:26 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Sep 06, 2023, Tom Lendacky wrote:
> > On 9/6/23 15:11, Sean Christopherson wrote:
> > > On Wed, Sep 06, 2023, Tom Lendacky wrote:
> > > > On 9/6/23 10:14, Peter Gonda wrote:
> > > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > > > index 956726d867aa..cecf6a528c9b 100644
> > > > > --- a/arch/x86/kvm/svm/svm.c
> > > > > +++ b/arch/x86/kvm/svm/svm.c
> > > > > @@ -2131,12 +2131,14 @@ static int shutdown_interception(struct kvm_vcpu *vcpu)
> > > > >          * The VM save area has already been encrypted so it
> > > > >          * cannot be reinitialized - just terminate.
> > > > >          */
> > > > > -       if (sev_es_guest(vcpu->kvm))
> > > > > -               return -EINVAL;
> > > > > +       if (sev_es_guest(vcpu->kvm)) {
> > > > > +               kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
> > > > > +               return 0;
> > > > > +       }
> > > >
> > > > Just a nit... feel free to ignore, but, since KVM_EXIT_SHUTDOWN is also set
> > > > at the end of the function and I don't think kvm_vcpu_reset() clears the
> > > > value from kvm_run, you could just set kvm_run->exit_reason on entry and
> > > > just return 0 early for an SEV-ES guest.
> > >
> > > kvm_run is writable by userspace though, so KVM can't rely on kvm_run->exit_reason
> > > for correctness.
> > >
> > > And IIUC, the VMSA is also toast, i.e. doing anything other than marking the VM
> > > dead is futile, no?
> >
> > I was just saying that "kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;" is in the
> > shutdown_interception() function twice now (at both exit points of the
> > function) and can probably just be moved to the top of the function and be
> > common for both exit points, now, right?
> >
> > I'm not saying to get rid of it, just set it sooner.
>
> Ah, I thought you were saying bail early from kvm_vcpu_reset().  I agree that not
> having completely split logic would be ideal.  What about this?
>
>         /*
>          * VMCB is undefined after a SHUTDOWN intercept.  INIT the vCPU to put
>          * the VMCB in a known good state.  Unfortuately, KVM doesn't have
>          * KVM_MP_STATE_SHUTDOWN and can't add it without potentially breaking
>          * userspace.  At a platform view, INIT is acceptable behavior as
>          * there exist bare metal platforms that automatically INIT the CPU
>          * in response to shutdown.
>          *
>          * The VM save area for SEV-ES guests has already been encrypted so it
>          * cannot be reinitialized, i.e. synthesizing INIT is futile.
>          */
>         if (!sev_es_guest(vcpu->kvm)) {
>                 clear_page(svm->vmcb);
>                 kvm_vcpu_reset(vcpu, true);
>         }
>
>         kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
>         return 0;

Looks better to me. Thanks!

Re: [PATCH V2] KVM: SEV: Update SEV-ES shutdown intercepts with more metadata

[Tom Lendacky]

On 9/6/23 15:26, Sean Christopherson wrote:
> On Wed, Sep 06, 2023, Tom Lendacky wrote:
>> On 9/6/23 15:11, Sean Christopherson wrote:
>>> On Wed, Sep 06, 2023, Tom Lendacky wrote:
>>>> On 9/6/23 10:14, Peter Gonda wrote:
>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>>> index 956726d867aa..cecf6a528c9b 100644
>>>>> --- a/arch/x86/kvm/svm/svm.c
>>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>>> @@ -2131,12 +2131,14 @@ static int shutdown_interception(struct kvm_vcpu *vcpu)
>>>>>     	 * The VM save area has already been encrypted so it
>>>>>     	 * cannot be reinitialized - just terminate.
>>>>>     	 */
>>>>> -	if (sev_es_guest(vcpu->kvm))
>>>>> -		return -EINVAL;
>>>>> +	if (sev_es_guest(vcpu->kvm)) {
>>>>> +		kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
>>>>> +		return 0;
>>>>> +	}
>>>>
>>>> Just a nit... feel free to ignore, but, since KVM_EXIT_SHUTDOWN is also set
>>>> at the end of the function and I don't think kvm_vcpu_reset() clears the
>>>> value from kvm_run, you could just set kvm_run->exit_reason on entry and
>>>> just return 0 early for an SEV-ES guest.
>>>
>>> kvm_run is writable by userspace though, so KVM can't rely on kvm_run->exit_reason
>>> for correctness.
>>>
>>> And IIUC, the VMSA is also toast, i.e. doing anything other than marking the VM
>>> dead is futile, no?
>>
>> I was just saying that "kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;" is in the
>> shutdown_interception() function twice now (at both exit points of the
>> function) and can probably just be moved to the top of the function and be
>> common for both exit points, now, right?
>>
>> I'm not saying to get rid of it, just set it sooner.
> 
> Ah, I thought you were saying bail early from kvm_vcpu_reset().  I agree that not
> having completely split logic would be ideal.  What about this?
> 
> 	/*
> 	 * VMCB is undefined after a SHUTDOWN intercept.  INIT the vCPU to put
> 	 * the VMCB in a known good state.  Unfortuately, KVM doesn't have
> 	 * KVM_MP_STATE_SHUTDOWN and can't add it without potentially breaking
> 	 * userspace.  At a platform view, INIT is acceptable behavior as
> 	 * there exist bare metal platforms that automatically INIT the CPU
> 	 * in response to shutdown.
> 	 *
> 	 * The VM save area for SEV-ES guests has already been encrypted so it
> 	 * cannot be reinitialized, i.e. synthesizing INIT is futile.
> 	 */
> 	if (!sev_es_guest(vcpu->kvm)) {
> 		clear_page(svm->vmcb);
> 		kvm_vcpu_reset(vcpu, true);
> 	}
> 
> 	kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
> 	return 0;

That looks good to me!

Thanks,
Tom