Messages in this thread | | | Date | Wed, 6 Sep 2023 15:34:15 -0500 | Subject | Re: [PATCH V2] KVM: SEV: Update SEV-ES shutdown intercepts with more metadata | From | 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
| |