| Message ID | 20240226213244.18441-7-john.allen@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SVM guest shadow stack support | expand |
On Mon, Feb 26, 2024, John Allen wrote: > When a guest issues a cpuid instruction for Fn0000000D_x0B > (CetUserOffset), KVM will intercept and need to access the guest > MSR_IA32_XSS value. For SEV-ES, this is encrypted and needs to be > included in the GHCB to be visible to the hypervisor. Heh, too many pronouns and implicit subjects. I read this, several times, as: When a guest issues a cpuid instruction for Fn0000000D_x0B (CetUserOffset), KVM will intercept MSR_IA32_XSS and need to access the guest MSR_IA32_XSS value. I think you mean this? When a vCPU executes CPUID.0xD.0xB (CetUserOffset), KVM will intercept and emulate CPUID. To emulate CPUID, KVM needs access to the vCPU's MSR_IA32_XSS value. For SEV-ES guests, XSS is encrypted, and so the guest must include its XSS value in the GHCB as part of the CPUID request. Hmm, I suspect that last sentence is wrong though. Question on that below. > Signed-off-by: John Allen <john.allen@amd.com> > --- > v2: > - Omit passing through XSS as this has already been properly > implemented in a26b7cd22546 ("KVM: SEV: Do not intercept > accesses to MSR_IA32_XSS for SEV-ES guests") > --- > arch/x86/include/asm/svm.h | 1 + > arch/x86/kvm/svm/sev.c | 9 +++++++-- > arch/x86/kvm/svm/svm.h | 1 + > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h > index 728c98175b9c..44cd41e2fb68 100644 > --- a/arch/x86/include/asm/svm.h > +++ b/arch/x86/include/asm/svm.h > @@ -673,5 +673,6 @@ DEFINE_GHCB_ACCESSORS(sw_exit_info_1) > DEFINE_GHCB_ACCESSORS(sw_exit_info_2) > DEFINE_GHCB_ACCESSORS(sw_scratch) > DEFINE_GHCB_ACCESSORS(xcr0) > +DEFINE_GHCB_ACCESSORS(xss) > > #endif > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index f06f9e51ad9d..c3060d2068eb 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -2458,8 +2458,13 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm) > > svm->vmcb->save.cpl = kvm_ghcb_get_cpl_if_valid(svm, ghcb); > > - if (kvm_ghcb_xcr0_is_valid(svm)) { > - vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb); > + if (kvm_ghcb_xcr0_is_valid(svm) || kvm_ghcb_xss_is_valid(svm)) { > + if (kvm_ghcb_xcr0_is_valid(svm)) > + vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb); > + > + if (kvm_ghcb_xss_is_valid(svm)) > + vcpu->arch.ia32_xss = ghcb_get_xss(ghcb); > + > kvm_update_cpuid_runtime(vcpu); Pre-existing code, but isn't updating CPUID runtime on every VMGEXIT super wasteful? Or is the guest behavior to mark XCR0 and XSS as valid only when changing XCR0/XSS? If so, the last sentence of the changelog should be something like: MSR_IA32_XSS value. For SEV-ES guests, XSS is encrypted, and so the guest must notify the host of XSS changes by performing a ??? VMGEXIT and providing its XSS value in the GHCB.
On 5/1/24 18:43, Sean Christopherson wrote: > On Mon, Feb 26, 2024, John Allen wrote: >> When a guest issues a cpuid instruction for Fn0000000D_x0B >> (CetUserOffset), KVM will intercept and need to access the guest >> MSR_IA32_XSS value. For SEV-ES, this is encrypted and needs to be >> included in the GHCB to be visible to the hypervisor. > > Heh, too many pronouns and implicit subjects. I read this, several times, as: > > When a guest issues a cpuid instruction for Fn0000000D_x0B > (CetUserOffset), KVM will intercept MSR_IA32_XSS and need to access the > guest MSR_IA32_XSS value. > > I think you mean this? > > When a vCPU executes CPUID.0xD.0xB (CetUserOffset), KVM will intercept > and emulate CPUID. To emulate CPUID, KVM needs access to the vCPU's > MSR_IA32_XSS value. For SEV-ES guests, XSS is encrypted, and so the guest > must include its XSS value in the GHCB as part of the CPUID request. > > Hmm, I suspect that last sentence is wrong though. Question on that below. > >> Signed-off-by: John Allen <john.allen@amd.com> >> --- >> v2: >> - Omit passing through XSS as this has already been properly >> implemented in a26b7cd22546 ("KVM: SEV: Do not intercept >> accesses to MSR_IA32_XSS for SEV-ES guests") >> --- >> arch/x86/include/asm/svm.h | 1 + >> arch/x86/kvm/svm/sev.c | 9 +++++++-- >> arch/x86/kvm/svm/svm.h | 1 + >> 3 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h >> index 728c98175b9c..44cd41e2fb68 100644 >> --- a/arch/x86/include/asm/svm.h >> +++ b/arch/x86/include/asm/svm.h >> @@ -673,5 +673,6 @@ DEFINE_GHCB_ACCESSORS(sw_exit_info_1) >> DEFINE_GHCB_ACCESSORS(sw_exit_info_2) >> DEFINE_GHCB_ACCESSORS(sw_scratch) >> DEFINE_GHCB_ACCESSORS(xcr0) >> +DEFINE_GHCB_ACCESSORS(xss) >> >> #endif >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >> index f06f9e51ad9d..c3060d2068eb 100644 >> --- a/arch/x86/kvm/svm/sev.c >> +++ b/arch/x86/kvm/svm/sev.c >> @@ -2458,8 +2458,13 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm) >> >> svm->vmcb->save.cpl = kvm_ghcb_get_cpl_if_valid(svm, ghcb); >> >> - if (kvm_ghcb_xcr0_is_valid(svm)) { >> - vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb); >> + if (kvm_ghcb_xcr0_is_valid(svm) || kvm_ghcb_xss_is_valid(svm)) { >> + if (kvm_ghcb_xcr0_is_valid(svm)) >> + vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb); >> + >> + if (kvm_ghcb_xss_is_valid(svm)) >> + vcpu->arch.ia32_xss = ghcb_get_xss(ghcb); >> + >> kvm_update_cpuid_runtime(vcpu); > > Pre-existing code, but isn't updating CPUID runtime on every VMGEXIT super wasteful? > Or is the guest behavior to mark XCR0 and XSS as valid only when changing XCR0/XSS? It's not really on every VMGEXIT. It's only if those values have been supplied in the GHCB will the CPUID runtime be updated. And the Linux guest code supplies XCR0 and XSS only on a CPUID VMGEXIT. Both sides of that can optimized. The guest can be optimized down to just supplying the values on CPUID 0xD or even further to only supplying the values if they have changed since the last time they were supplied. The hypervisor side could be optimized to compare the value and only update the CPUID runtime if those values are different. Thanks, Tom > If so, the last sentence of the changelog should be something like: > > MSR_IA32_XSS value. For SEV-ES guests, XSS is encrypted, and so the guest > must notify the host of XSS changes by performing a ??? VMGEXIT and > providing its XSS value in the GHCB.
On Thu, May 02, 2024, Tom Lendacky wrote: > The hypervisor side could be optimized to compare the value and only update > the CPUID runtime if those values are different. Heh, this is exactly the thought I had around dinner time yesterday :-)
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h index 728c98175b9c..44cd41e2fb68 100644 --- a/arch/x86/include/asm/svm.h +++ b/arch/x86/include/asm/svm.h @@ -673,5 +673,6 @@ DEFINE_GHCB_ACCESSORS(sw_exit_info_1) DEFINE_GHCB_ACCESSORS(sw_exit_info_2) DEFINE_GHCB_ACCESSORS(sw_scratch) DEFINE_GHCB_ACCESSORS(xcr0) +DEFINE_GHCB_ACCESSORS(xss) #endif diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index f06f9e51ad9d..c3060d2068eb 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2458,8 +2458,13 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm) svm->vmcb->save.cpl = kvm_ghcb_get_cpl_if_valid(svm, ghcb); - if (kvm_ghcb_xcr0_is_valid(svm)) { - vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb); + if (kvm_ghcb_xcr0_is_valid(svm) || kvm_ghcb_xss_is_valid(svm)) { + if (kvm_ghcb_xcr0_is_valid(svm)) + vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb); + + if (kvm_ghcb_xss_is_valid(svm)) + vcpu->arch.ia32_xss = ghcb_get_xss(ghcb); + kvm_update_cpuid_runtime(vcpu); } diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index 0741fa049fd7..eb9c9e337c43 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -723,5 +723,6 @@ DEFINE_KVM_GHCB_ACCESSORS(sw_exit_info_1) DEFINE_KVM_GHCB_ACCESSORS(sw_exit_info_2) DEFINE_KVM_GHCB_ACCESSORS(sw_scratch) DEFINE_KVM_GHCB_ACCESSORS(xcr0) +DEFINE_KVM_GHCB_ACCESSORS(xss) #endif
When a guest issues a cpuid instruction for Fn0000000D_x0B (CetUserOffset), KVM will intercept and need to access the guest MSR_IA32_XSS value. For SEV-ES, this is encrypted and needs to be included in the GHCB to be visible to the hypervisor. Signed-off-by: John Allen <john.allen@amd.com> --- v2: - Omit passing through XSS as this has already been properly implemented in a26b7cd22546 ("KVM: SEV: Do not intercept accesses to MSR_IA32_XSS for SEV-ES guests") --- arch/x86/include/asm/svm.h | 1 + arch/x86/kvm/svm/sev.c | 9 +++++++-- arch/x86/kvm/svm/svm.h | 1 + 3 files changed, 9 insertions(+), 2 deletions(-)