| Message ID | 20230906151449.18312-1-pgonda@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] KVM: SEV: Update SEV-ES shutdown intercepts with more metadata | expand |
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
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?
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
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;
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!
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
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
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(-)