* [PATCH v4 0/2] x86: Add the feature Virtual SPEC_CTRL
@ 2021-01-29 0:43 Babu Moger
2021-01-29 0:43 ` [PATCH v4 1/2] x86/cpufeatures: Add the Virtual SPEC_CTRL feature Babu Moger
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Babu Moger @ 2021-01-29 0:43 UTC (permalink / raw)
To: pbonzini, tglx, mingo, bp
Cc: fenghua.yu, tony.luck, wanpengli, kvm, thomas.lendacky, peterz,
seanjc, joro, x86, kyung.min.park, linux-kernel, krish.sadhukhan,
hpa, mgross, vkuznets, kim.phillips, wei.huang2, jmattson
Newer AMD processors have a feature to virtualize the use of the
SPEC_CTRL MSR on the guest. The series adds the feature support
and enables the feature on SVM.
---
v4:
1. Taken care of comments from Sean Christopherson.
a. Updated svm_set_msr/svm_get_msr to read/write the spec_ctrl value
directly from save spec_ctrl.
b. Disabled the msr_interception in init_vmcb when V_SPEC_CTRL is
present.
c. Added the save restore for nested vm. Also tested to make sure
the nested SPEC_CTRL settings properly saved and restored between
L2 and L1 guests.
2. Added the kvm-unit-tests to verify that. Sent those patches separately.
v3:
1. Taken care of recent changes in vmcb_save_area. Needed to adjust the save
area spec_ctrl definition.
2. Taken care of few comments from Tom.
a. Initialised the save area spec_ctrl in case of SEV-ES.
b. Removed the changes in svm_get_msr/svm_set_msr.
c. Reverted the changes to disable the msr interception to avoid compatibility
issue.
3. Updated the patch #1 with Acked-by from Boris.
v2:
NOTE: This is not final yet. Sending out the patches to make
sure I captured all the comments correctly.
1. Most of the changes are related to Jim and Sean's feedback.
2. Improved the description of patch #2.
3. Updated the vmcb save area's guest spec_ctrl value(offset 0x2E0)
properly. Initialized during init_vmcb and svm_set_msr and
returned the value from save area for svm_get_msr.
4. As Jim commented, transferred the value into the VMCB prior
to VMRUN and out of the VMCB after #VMEXIT.
5. Added kvm-unit-test to detect the SPEC CTRL feature.
https://lore.kernel.org/kvm/160865324865.19910.5159218511905134908.stgit@bmoger-ubuntu/
6. Sean mantioned of renaming MSR_AMD64_VIRT_SPEC_CTRL. But, it might
create even more confusion, so dropped the idea for now.
v3: https://lore.kernel.org/kvm/161073115461.13848.18035972823733547803.stgit@bmoger-ubuntu/
v2: https://lore.kernel.org/kvm/160867624053.3471.7106539070175910424.stgit@bmoger-ubuntu/
v1: https://lore.kernel.org/kvm/160738054169.28590.5171339079028237631.stgit@bmoger-ubuntu/
Babu Moger (2):
x86/cpufeatures: Add the Virtual SPEC_CTRL feature
KVM: SVM: Add support for Virtual SPEC_CTRL
arch/x86/include/asm/cpufeatures.h | 1 +
arch/x86/include/asm/svm.h | 4 +++-
arch/x86/kvm/svm/nested.c | 2 ++
arch/x86/kvm/svm/svm.c | 27 ++++++++++++++++++++++-----
4 files changed, 28 insertions(+), 6 deletions(-)
--
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 1/2] x86/cpufeatures: Add the Virtual SPEC_CTRL feature
2021-01-29 0:43 [PATCH v4 0/2] x86: Add the feature Virtual SPEC_CTRL Babu Moger
@ 2021-01-29 0:43 ` Babu Moger
2021-01-29 0:43 ` [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL Babu Moger
2021-02-10 23:40 ` [PATCH v4 0/2] x86: Add the feature " Babu Moger
2 siblings, 0 replies; 11+ messages in thread
From: Babu Moger @ 2021-01-29 0:43 UTC (permalink / raw)
To: pbonzini, tglx, mingo, bp
Cc: fenghua.yu, tony.luck, wanpengli, kvm, thomas.lendacky, peterz,
seanjc, joro, x86, kyung.min.park, linux-kernel, krish.sadhukhan,
hpa, mgross, vkuznets, kim.phillips, wei.huang2, jmattson
Newer AMD processors have a feature to virtualize the use of the
SPEC_CTRL MSR. Presence of this feature is indicated via CPUID
function 0x8000000A_EDX[20]: GuestSpecCtrl. When present, the
SPEC_CTRL MSR is automatically virtualized.
Signed-off-by: Babu Moger <babu.moger@amd.com>
Acked-by: Borislav Petkov <bp@suse.de>
---
arch/x86/include/asm/cpufeatures.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 84b887825f12..3fcd0624b1bc 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -337,6 +337,7 @@
#define X86_FEATURE_AVIC (15*32+13) /* Virtual Interrupt Controller */
#define X86_FEATURE_V_VMSAVE_VMLOAD (15*32+15) /* Virtual VMSAVE VMLOAD */
#define X86_FEATURE_VGIF (15*32+16) /* Virtual GIF */
+#define X86_FEATURE_V_SPEC_CTRL (15*32+20) /* Virtual SPEC_CTRL */
/* Intel-defined CPU features, CPUID level 0x00000007:0 (ECX), word 16 */
#define X86_FEATURE_AVX512VBMI (16*32+ 1) /* AVX512 Vector Bit Manipulation instructions*/
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL
2021-01-29 0:43 [PATCH v4 0/2] x86: Add the feature Virtual SPEC_CTRL Babu Moger
2021-01-29 0:43 ` [PATCH v4 1/2] x86/cpufeatures: Add the Virtual SPEC_CTRL feature Babu Moger
@ 2021-01-29 0:43 ` Babu Moger
2021-02-11 8:56 ` Paolo Bonzini
2022-06-04 3:11 ` Jim Mattson
2021-02-10 23:40 ` [PATCH v4 0/2] x86: Add the feature " Babu Moger
2 siblings, 2 replies; 11+ messages in thread
From: Babu Moger @ 2021-01-29 0:43 UTC (permalink / raw)
To: pbonzini, tglx, mingo, bp
Cc: fenghua.yu, tony.luck, wanpengli, kvm, thomas.lendacky, peterz,
seanjc, joro, x86, kyung.min.park, linux-kernel, krish.sadhukhan,
hpa, mgross, vkuznets, kim.phillips, wei.huang2, jmattson
Newer AMD processors have a feature to virtualize the use of the
SPEC_CTRL MSR. Presence of this feature is indicated via CPUID
function 0x8000000A_EDX[20]: GuestSpecCtrl. Hypervisors are not
required to enable this feature since it is automatically enabled on
processors that support it.
A hypervisor may wish to impose speculation controls on guest
execution or a guest may want to impose its own speculation controls.
Therefore, the processor implements both host and guest
versions of SPEC_CTRL.
When in host mode, the host SPEC_CTRL value is in effect and writes
update only the host version of SPEC_CTRL. On a VMRUN, the processor
loads the guest version of SPEC_CTRL from the VMCB. When the guest
writes SPEC_CTRL, only the guest version is updated. On a VMEXIT,
the guest version is saved into the VMCB and the processor returns
to only using the host SPEC_CTRL for speculation control. The guest
SPEC_CTRL is located at offset 0x2E0 in the VMCB.
The effective SPEC_CTRL setting is the guest SPEC_CTRL setting or'ed
with the hypervisor SPEC_CTRL setting. This allows the hypervisor to
ensure a minimum SPEC_CTRL if desired.
This support also fixes an issue where a guest may sometimes see an
inconsistent value for the SPEC_CTRL MSR on processors that support
this feature. With the current SPEC_CTRL support, the first write to
SPEC_CTRL is intercepted and the virtualized version of the SPEC_CTRL
MSR is not updated. When the guest reads back the SPEC_CTRL MSR, it
will be 0x0, instead of the actual expected value. There isn’t a
security concern here, because the host SPEC_CTRL value is or’ed with
the Guest SPEC_CTRL value to generate the effective SPEC_CTRL value.
KVM writes with the guest's virtualized SPEC_CTRL value to SPEC_CTRL
MSR just before the VMRUN, so it will always have the actual value
even though it doesn’t appear that way in the guest. The guest will
only see the proper value for the SPEC_CTRL register if the guest was
to write to the SPEC_CTRL register again. With Virtual SPEC_CTRL
support, the save area spec_ctrl is properly saved and restored.
So, the guest will always see the proper value when it is read back.
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
arch/x86/include/asm/svm.h | 4 +++-
arch/x86/kvm/svm/nested.c | 2 ++
arch/x86/kvm/svm/svm.c | 27 ++++++++++++++++++++++-----
3 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 1c561945b426..772e60efe243 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -269,7 +269,9 @@ struct vmcb_save_area {
* SEV-ES guests when referenced through the GHCB or for
* saving to the host save area.
*/
- u8 reserved_7[80];
+ u8 reserved_7[72];
+ u32 spec_ctrl; /* Guest version of SPEC_CTRL at 0x2E0 */
+ u8 reserved_7b[4];
u32 pkru;
u8 reserved_7a[20];
u64 reserved_8; /* rax already available at 0x01f8 */
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 7a605ad8254d..9e51f9e4f631 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -534,6 +534,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
hsave->save.cr3 = vmcb->save.cr3;
else
hsave->save.cr3 = kvm_read_cr3(&svm->vcpu);
+ hsave->save.spec_ctrl = vmcb->save.spec_ctrl;
copy_vmcb_control_area(&hsave->control, &vmcb->control);
@@ -675,6 +676,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
kvm_rip_write(&svm->vcpu, hsave->save.rip);
svm->vmcb->save.dr7 = DR7_FIXED_1;
svm->vmcb->save.cpl = 0;
+ svm->vmcb->save.spec_ctrl = hsave->save.spec_ctrl;
svm->vmcb->control.exit_int_info = 0;
vmcb_mark_all_dirty(svm->vmcb);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index f923e14e87df..756129caa611 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1244,6 +1244,14 @@ static void init_vmcb(struct vcpu_svm *svm)
svm_check_invpcid(svm);
+ /*
+ * If the host supports V_SPEC_CTRL then disable the interception
+ * of MSR_IA32_SPEC_CTRL.
+ */
+ if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
+ set_msr_interception(&svm->vcpu, svm->msrpm, MSR_IA32_SPEC_CTRL,
+ 1, 1);
+
if (kvm_vcpu_apicv_active(&svm->vcpu))
avic_init_vmcb(svm);
@@ -2678,7 +2686,10 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
!guest_has_spec_ctrl_msr(vcpu))
return 1;
- msr_info->data = svm->spec_ctrl;
+ if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
+ msr_info->data = svm->vmcb->save.spec_ctrl;
+ else
+ msr_info->data = svm->spec_ctrl;
break;
case MSR_AMD64_VIRT_SPEC_CTRL:
if (!msr_info->host_initiated &&
@@ -2779,7 +2790,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
if (kvm_spec_ctrl_test_value(data))
return 1;
- svm->spec_ctrl = data;
+ if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
+ svm->vmcb->save.spec_ctrl = data;
+ else
+ svm->spec_ctrl = data;
if (!data)
break;
@@ -3791,7 +3805,8 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
* is no need to worry about the conditional branch over the wrmsr
* being speculatively taken.
*/
- x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);
+ if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
+ x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);
svm_vcpu_enter_exit(vcpu, svm);
@@ -3810,13 +3825,15 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
* If the L02 MSR bitmap does not intercept the MSR, then we need to
* save it.
*/
- if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)))
+ if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL) &&
+ unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)))
svm->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
if (!sev_es_guest(svm->vcpu.kvm))
reload_tss(vcpu);
- x86_spec_ctrl_restore_host(svm->spec_ctrl, svm->virt_spec_ctrl);
+ if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
+ x86_spec_ctrl_restore_host(svm->spec_ctrl, svm->virt_spec_ctrl);
if (!sev_es_guest(svm->vcpu.kvm)) {
vcpu->arch.cr2 = svm->vmcb->save.cr2;
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4 0/2] x86: Add the feature Virtual SPEC_CTRL
2021-01-29 0:43 [PATCH v4 0/2] x86: Add the feature Virtual SPEC_CTRL Babu Moger
2021-01-29 0:43 ` [PATCH v4 1/2] x86/cpufeatures: Add the Virtual SPEC_CTRL feature Babu Moger
2021-01-29 0:43 ` [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL Babu Moger
@ 2021-02-10 23:40 ` Babu Moger
2 siblings, 0 replies; 11+ messages in thread
From: Babu Moger @ 2021-02-10 23:40 UTC (permalink / raw)
To: pbonzini, tglx, mingo, bp
Cc: fenghua.yu, tony.luck, wanpengli, kvm, thomas.lendacky, peterz,
seanjc, joro, x86, kyung.min.park, linux-kernel, krish.sadhukhan,
hpa, mgross, vkuznets, kim.phillips, wei.huang2, jmattson
Paolo/Sean,
Any comments on these patches?
Thanks
Babu
On 1/28/21 6:43 PM, Babu Moger wrote:
> Newer AMD processors have a feature to virtualize the use of the
> SPEC_CTRL MSR on the guest. The series adds the feature support
> and enables the feature on SVM.
> ---
> v4:
> 1. Taken care of comments from Sean Christopherson.
> a. Updated svm_set_msr/svm_get_msr to read/write the spec_ctrl value
> directly from save spec_ctrl.
> b. Disabled the msr_interception in init_vmcb when V_SPEC_CTRL is
> present.
> c. Added the save restore for nested vm. Also tested to make sure
> the nested SPEC_CTRL settings properly saved and restored between
> L2 and L1 guests.
> 2. Added the kvm-unit-tests to verify that. Sent those patches separately.
>
> v3:
> 1. Taken care of recent changes in vmcb_save_area. Needed to adjust the save
> area spec_ctrl definition.
> 2. Taken care of few comments from Tom.
> a. Initialised the save area spec_ctrl in case of SEV-ES.
> b. Removed the changes in svm_get_msr/svm_set_msr.
> c. Reverted the changes to disable the msr interception to avoid compatibility
> issue.
> 3. Updated the patch #1 with Acked-by from Boris.
>
> v2:
> NOTE: This is not final yet. Sending out the patches to make
> sure I captured all the comments correctly.
>
> 1. Most of the changes are related to Jim and Sean's feedback.
> 2. Improved the description of patch #2.
> 3. Updated the vmcb save area's guest spec_ctrl value(offset 0x2E0)
> properly. Initialized during init_vmcb and svm_set_msr and
> returned the value from save area for svm_get_msr.
> 4. As Jim commented, transferred the value into the VMCB prior
> to VMRUN and out of the VMCB after #VMEXIT.
> 5. Added kvm-unit-test to detect the SPEC CTRL feature.
> https://lore.kernel.org/kvm/160865324865.19910.5159218511905134908.stgit@bmoger-ubuntu/
> 6. Sean mantioned of renaming MSR_AMD64_VIRT_SPEC_CTRL. But, it might
> create even more confusion, so dropped the idea for now.
>
> v3: https://lore.kernel.org/kvm/161073115461.13848.18035972823733547803.stgit@bmoger-ubuntu/
> v2: https://lore.kernel.org/kvm/160867624053.3471.7106539070175910424.stgit@bmoger-ubuntu/
> v1: https://lore.kernel.org/kvm/160738054169.28590.5171339079028237631.stgit@bmoger-ubuntu/
>
> Babu Moger (2):
> x86/cpufeatures: Add the Virtual SPEC_CTRL feature
> KVM: SVM: Add support for Virtual SPEC_CTRL
>
>
> arch/x86/include/asm/cpufeatures.h | 1 +
> arch/x86/include/asm/svm.h | 4 +++-
> arch/x86/kvm/svm/nested.c | 2 ++
> arch/x86/kvm/svm/svm.c | 27 ++++++++++++++++++++++-----
> 4 files changed, 28 insertions(+), 6 deletions(-)
>
> --
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL
2021-01-29 0:43 ` [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL Babu Moger
@ 2021-02-11 8:56 ` Paolo Bonzini
2021-02-11 22:42 ` Babu Moger
2022-06-04 3:11 ` Jim Mattson
1 sibling, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2021-02-11 8:56 UTC (permalink / raw)
To: Babu Moger, tglx, mingo, bp
Cc: fenghua.yu, tony.luck, wanpengli, kvm, thomas.lendacky, peterz,
seanjc, joro, x86, kyung.min.park, linux-kernel, krish.sadhukhan,
hpa, mgross, vkuznets, kim.phillips, wei.huang2, jmattson
On 29/01/21 01:43, Babu Moger wrote:
> This support also fixes an issue where a guest may sometimes see an
> inconsistent value for the SPEC_CTRL MSR on processors that support this
> feature. With the current SPEC_CTRL support, the first write to
> SPEC_CTRL is intercepted and the virtualized version of the SPEC_CTRL
> MSR is not updated.
This is a bit ugly, new features should always be enabled manually (AMD
did it right for vVMLOAD/vVMSAVE for example, even though _in theory_
assuming that all hypervisors were intercepting VMLOAD/VMSAVE would have
been fine).
Also regarding nested virtualization:
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 7a605ad8254d..9e51f9e4f631 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -534,6 +534,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
> hsave->save.cr3 = vmcb->save.cr3;
> else
> hsave->save.cr3 = kvm_read_cr3(&svm->vcpu);
> + hsave->save.spec_ctrl = vmcb->save.spec_ctrl;
>
> copy_vmcb_control_area(&hsave->control, &vmcb->control);
>
> @@ -675,6 +676,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
> kvm_rip_write(&svm->vcpu, hsave->save.rip);
> svm->vmcb->save.dr7 = DR7_FIXED_1;
> svm->vmcb->save.cpl = 0;
> + svm->vmcb->save.spec_ctrl = hsave->save.spec_ctrl;
> svm->vmcb->control.exit_int_info = 0;
>
> vmcb_mark_all_dirty(svm->vmcb);
I think this is incorrect. Since we don't support this feature in the
nested hypervisor, any writes to the SPEC_CTRL MSR while L2 (nested
guest) runs have to be reflected to L1 (nested hypervisor). In other
words, this new field is more like VMLOAD/VMSAVE state, in that it
doesn't change across VMRUN and VMEXIT. These two hunks can be removed.
If we want to do it, exposing this feature to the nested hypervisor will
be a bit complicated, because one has to write host SPEC_CTRL |
vmcb01.GuestSpecCtrl in the host MSR, in order to free the vmcb02
GuestSpecCtrl for the vmcb12 GuestSpecCtrl.
It would also be possible to emulate it on processors that don't have
it. However I'm not sure it's a good idea because of the problem that
you mentioned with running old kernels on new processors.
I have queued the patches with the small fix above. However I plan to
only include them in 5.13 because I have a bunch of other SVM patches,
those have been tested already but I need to send them out for review
before "officially" getting them in kvm.git.
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL
2021-02-11 8:56 ` Paolo Bonzini
@ 2021-02-11 22:42 ` Babu Moger
0 siblings, 0 replies; 11+ messages in thread
From: Babu Moger @ 2021-02-11 22:42 UTC (permalink / raw)
To: Paolo Bonzini, tglx, mingo, bp
Cc: fenghua.yu, tony.luck, wanpengli, kvm, thomas.lendacky, peterz,
seanjc, joro, x86, kyung.min.park, linux-kernel, krish.sadhukhan,
hpa, mgross, vkuznets, kim.phillips, wei.huang2, jmattson
On 2/11/21 2:56 AM, Paolo Bonzini wrote:
> On 29/01/21 01:43, Babu Moger wrote:
>> This support also fixes an issue where a guest may sometimes see an
>> inconsistent value for the SPEC_CTRL MSR on processors that support this
>> feature. With the current SPEC_CTRL support, the first write to
>> SPEC_CTRL is intercepted and the virtualized version of the SPEC_CTRL
>> MSR is not updated.
>
> This is a bit ugly, new features should always be enabled manually (AMD
> did it right for vVMLOAD/vVMSAVE for example, even though _in theory_
> assuming that all hypervisors were intercepting VMLOAD/VMSAVE would have
> been fine).
>
> Also regarding nested virtualization:
>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 7a605ad8254d..9e51f9e4f631 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -534,6 +534,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
>> hsave->save.cr3 = vmcb->save.cr3;
>> else
>> hsave->save.cr3 = kvm_read_cr3(&svm->vcpu);
>> + hsave->save.spec_ctrl = vmcb->save.spec_ctrl;
>>
>> copy_vmcb_control_area(&hsave->control, &vmcb->control);
>>
>> @@ -675,6 +676,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
>> kvm_rip_write(&svm->vcpu, hsave->save.rip);
>> svm->vmcb->save.dr7 = DR7_FIXED_1;
>> svm->vmcb->save.cpl = 0;
>> + svm->vmcb->save.spec_ctrl = hsave->save.spec_ctrl;
>> svm->vmcb->control.exit_int_info = 0;
>>
>> vmcb_mark_all_dirty(svm->vmcb);
>
> I think this is incorrect. Since we don't support this feature in the
> nested hypervisor, any writes to the SPEC_CTRL MSR while L2 (nested guest)
> runs have to be reflected to L1 (nested hypervisor). In other words, this
> new field is more like VMLOAD/VMSAVE state, in that it doesn't change
> across VMRUN and VMEXIT. These two hunks can be removed.
Makes sense. I have tested removing these two hunks and it worked fine.
>
> If we want to do it, exposing this feature to the nested hypervisor will
> be a bit complicated, because one has to write host SPEC_CTRL |
> vmcb01.GuestSpecCtrl in the host MSR, in order to free the vmcb02
> GuestSpecCtrl for the vmcb12 GuestSpecCtrl.
>
> It would also be possible to emulate it on processors that don't have it.
> However I'm not sure it's a good idea because of the problem that you
> mentioned with running old kernels on new processors.
>
> I have queued the patches with the small fix above. However I plan to
> only include them in 5.13 because I have a bunch of other SVM patches,
Yes. 5.13 is fine.
thanks
Babu
> those have been tested already but I need to send them out for review
> before "officially" getting them in kvm.git.
>
> Paolo
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL
2021-01-29 0:43 ` [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL Babu Moger
2021-02-11 8:56 ` Paolo Bonzini
@ 2022-06-04 3:11 ` Jim Mattson
2022-06-13 15:09 ` Tom Lendacky
1 sibling, 1 reply; 11+ messages in thread
From: Jim Mattson @ 2022-06-04 3:11 UTC (permalink / raw)
To: Babu Moger
Cc: pbonzini, tglx, mingo, bp, fenghua.yu, tony.luck, wanpengli, kvm,
thomas.lendacky, peterz, seanjc, joro, x86, kyung.min.park,
linux-kernel, krish.sadhukhan, hpa, mgross, vkuznets,
kim.phillips, wei.huang2
On Thu, Jan 28, 2021 at 4:43 PM Babu Moger <babu.moger@amd.com> wrote:
> This support also fixes an issue where a guest may sometimes see an
> inconsistent value for the SPEC_CTRL MSR on processors that support
> this feature. With the current SPEC_CTRL support, the first write to
> SPEC_CTRL is intercepted and the virtualized version of the SPEC_CTRL
> MSR is not updated. When the guest reads back the SPEC_CTRL MSR, it
> will be 0x0, instead of the actual expected value. There isn’t a
> security concern here, because the host SPEC_CTRL value is or’ed with
> the Guest SPEC_CTRL value to generate the effective SPEC_CTRL value.
> KVM writes with the guest's virtualized SPEC_CTRL value to SPEC_CTRL
> MSR just before the VMRUN, so it will always have the actual value
> even though it doesn’t appear that way in the guest. The guest will
> only see the proper value for the SPEC_CTRL register if the guest was
> to write to the SPEC_CTRL register again. With Virtual SPEC_CTRL
> support, the save area spec_ctrl is properly saved and restored.
> So, the guest will always see the proper value when it is read back.
Note that there are actually two significant problems with the way the
new feature interacts with the KVM code before this patch:
1) All bits set by the first non-zero write become sticky until the
vCPU is reset (because svm->spec_ctrl is never modified after the
first non-zero write).
2) The current guest IA32_SPEC_CTRL value isn't actually known to the
hypervisor. It thinks that there are no writes to the MSR after the
first non-zero write, so that sticky value will be returned to
KVM_GET_MSRS. This breaks live migration.
Basically, an always-on V_SPEC_CTRL breaks existing hypervisors. It
must, therefore, default to off. However, I see that our Rome and
Milan CPUs already report the existence of this feature.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL
2022-06-04 3:11 ` Jim Mattson
@ 2022-06-13 15:09 ` Tom Lendacky
2022-06-13 19:23 ` Jim Mattson
0 siblings, 1 reply; 11+ messages in thread
From: Tom Lendacky @ 2022-06-13 15:09 UTC (permalink / raw)
To: Jim Mattson, Babu Moger
Cc: pbonzini, tglx, mingo, bp, fenghua.yu, tony.luck, wanpengli, kvm,
peterz, seanjc, joro, x86, kyung.min.park, linux-kernel,
krish.sadhukhan, hpa, mgross, vkuznets, kim.phillips, wei.huang2
On 6/3/22 22:11, Jim Mattson wrote:
> On Thu, Jan 28, 2021 at 4:43 PM Babu Moger <babu.moger@amd.com> wrote:
>
>> This support also fixes an issue where a guest may sometimes see an
>> inconsistent value for the SPEC_CTRL MSR on processors that support
>> this feature. With the current SPEC_CTRL support, the first write to
>> SPEC_CTRL is intercepted and the virtualized version of the SPEC_CTRL
>> MSR is not updated. When the guest reads back the SPEC_CTRL MSR, it
>> will be 0x0, instead of the actual expected value. There isn’t a
>> security concern here, because the host SPEC_CTRL value is or’ed with
>> the Guest SPEC_CTRL value to generate the effective SPEC_CTRL value.
>> KVM writes with the guest's virtualized SPEC_CTRL value to SPEC_CTRL
>> MSR just before the VMRUN, so it will always have the actual value
>> even though it doesn’t appear that way in the guest. The guest will
>> only see the proper value for the SPEC_CTRL register if the guest was
>> to write to the SPEC_CTRL register again. With Virtual SPEC_CTRL
>> support, the save area spec_ctrl is properly saved and restored.
>> So, the guest will always see the proper value when it is read back.
>
> Note that there are actually two significant problems with the way the
> new feature interacts with the KVM code before this patch:
> 1) All bits set by the first non-zero write become sticky until the
> vCPU is reset (because svm->spec_ctrl is never modified after the
> first non-zero write).
When X86_FEATURE_V_SPEC_CTRL is set, then svm->spec_ctrl isn't used.
> 2) The current guest IA32_SPEC_CTRL value isn't actually known to the
> hypervisor.
The hypervisor can read the value as long as it is not an SEV-ES or
SEV-SNP guest.
> It thinks that there are no writes to the MSR after the
> first non-zero write, so that sticky value will be returned to
> KVM_GET_MSRS. This breaks live migration.
KVM_GET_MSRS should go through the normal read MSR path, which will read
the guest MSR value from either svm->vmcb->save.spec_ctrl if
X86_FEATURE_V_SPEC_CTRL is set or from svm->spec_ctrl, otherwise. And the
write MSR path will do similar.
I'm probably missing something here, because I'm not good with the whole
live migration thing as it relates to host and guest features.
Thanks,
Tom
>
> Basically, an always-on V_SPEC_CTRL breaks existing hypervisors. It
> must, therefore, default to off. However, I see that our Rome and
> Milan CPUs already report the existence of this feature.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL
2022-06-13 15:09 ` Tom Lendacky
@ 2022-06-13 19:23 ` Jim Mattson
2022-06-13 20:16 ` Tom Lendacky
0 siblings, 1 reply; 11+ messages in thread
From: Jim Mattson @ 2022-06-13 19:23 UTC (permalink / raw)
To: Tom Lendacky
Cc: Babu Moger, pbonzini, tglx, mingo, bp, fenghua.yu, tony.luck,
wanpengli, kvm, peterz, seanjc, joro, x86, kyung.min.park,
linux-kernel, krish.sadhukhan, hpa, mgross, vkuznets,
kim.phillips, wei.huang2
On Mon, Jun 13, 2022 at 8:10 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 6/3/22 22:11, Jim Mattson wrote:
> > On Thu, Jan 28, 2021 at 4:43 PM Babu Moger <babu.moger@amd.com> wrote:
> >
> >> This support also fixes an issue where a guest may sometimes see an
> >> inconsistent value for the SPEC_CTRL MSR on processors that support
> >> this feature. With the current SPEC_CTRL support, the first write to
> >> SPEC_CTRL is intercepted and the virtualized version of the SPEC_CTRL
> >> MSR is not updated. When the guest reads back the SPEC_CTRL MSR, it
> >> will be 0x0, instead of the actual expected value. There isn’t a
> >> security concern here, because the host SPEC_CTRL value is or’ed with
> >> the Guest SPEC_CTRL value to generate the effective SPEC_CTRL value.
> >> KVM writes with the guest's virtualized SPEC_CTRL value to SPEC_CTRL
> >> MSR just before the VMRUN, so it will always have the actual value
> >> even though it doesn’t appear that way in the guest. The guest will
> >> only see the proper value for the SPEC_CTRL register if the guest was
> >> to write to the SPEC_CTRL register again. With Virtual SPEC_CTRL
> >> support, the save area spec_ctrl is properly saved and restored.
> >> So, the guest will always see the proper value when it is read back.
> >
> > Note that there are actually two significant problems with the way the
> > new feature interacts with the KVM code before this patch:
> > 1) All bits set by the first non-zero write become sticky until the
> > vCPU is reset (because svm->spec_ctrl is never modified after the
> > first non-zero write).
>
> When X86_FEATURE_V_SPEC_CTRL is set, then svm->spec_ctrl isn't used.
Post-patch, yes. I'm talking about how this new hardware feature broke
versions of KVM *before* this patch was submitted.
> > 2) The current guest IA32_SPEC_CTRL value isn't actually known to the
> > hypervisor.
>
> The hypervisor can read the value as long as it is not an SEV-ES or
> SEV-SNP guest.
Yes, it can, but KVM prior to this patch did not. Again, I'm talking
about how this new hardware feature broke *existing* versions of KVM.
> > It thinks that there are no writes to the MSR after the
> > first non-zero write, so that sticky value will be returned to
> > KVM_GET_MSRS. This breaks live migration.
>
> KVM_GET_MSRS should go through the normal read MSR path, which will read
> the guest MSR value from either svm->vmcb->save.spec_ctrl if
> X86_FEATURE_V_SPEC_CTRL is set or from svm->spec_ctrl, otherwise. And the
> write MSR path will do similar.
You really are gaslighting me, aren't you?
> I'm probably missing something here, because I'm not good with the whole
> live migration thing as it relates to host and guest features.
AMD added a new VMCB field that existing hypervisors knew nothing
about. This VMCB field contains the current value of the guest's
IA32_SPEC_CTRL. Since the hypervisor doesn't know that this field even
exists, it cannot migrate it.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL
2022-06-13 19:23 ` Jim Mattson
@ 2022-06-13 20:16 ` Tom Lendacky
2022-06-13 20:28 ` Jim Mattson
0 siblings, 1 reply; 11+ messages in thread
From: Tom Lendacky @ 2022-06-13 20:16 UTC (permalink / raw)
To: Jim Mattson
Cc: Babu Moger, pbonzini, tglx, mingo, bp, fenghua.yu, tony.luck,
wanpengli, kvm, peterz, seanjc, joro, x86, kyung.min.park,
linux-kernel, krish.sadhukhan, hpa, mgross, vkuznets,
kim.phillips, wei.huang2
On 6/13/22 14:23, Jim Mattson wrote:
> On Mon, Jun 13, 2022 at 8:10 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 6/3/22 22:11, Jim Mattson wrote:
>>> On Thu, Jan 28, 2021 at 4:43 PM Babu Moger <babu.moger@amd.com> wrote:
>>>
>>>> This support also fixes an issue where a guest may sometimes see an
>>>> inconsistent value for the SPEC_CTRL MSR on processors that support
>>>> this feature. With the current SPEC_CTRL support, the first write to
>>>> SPEC_CTRL is intercepted and the virtualized version of the SPEC_CTRL
>>>> MSR is not updated. When the guest reads back the SPEC_CTRL MSR, it
>>>> will be 0x0, instead of the actual expected value. There isn’t a
>>>> security concern here, because the host SPEC_CTRL value is or’ed with
>>>> the Guest SPEC_CTRL value to generate the effective SPEC_CTRL value.
>>>> KVM writes with the guest's virtualized SPEC_CTRL value to SPEC_CTRL
>>>> MSR just before the VMRUN, so it will always have the actual value
>>>> even though it doesn’t appear that way in the guest. The guest will
>>>> only see the proper value for the SPEC_CTRL register if the guest was
>>>> to write to the SPEC_CTRL register again. With Virtual SPEC_CTRL
>>>> support, the save area spec_ctrl is properly saved and restored.
>>>> So, the guest will always see the proper value when it is read back.
>>>
>>> Note that there are actually two significant problems with the way the
>>> new feature interacts with the KVM code before this patch:
>>> 1) All bits set by the first non-zero write become sticky until the
>>> vCPU is reset (because svm->spec_ctrl is never modified after the
>>> first non-zero write).
>>
>> When X86_FEATURE_V_SPEC_CTRL is set, then svm->spec_ctrl isn't used.
>
> Post-patch, yes. I'm talking about how this new hardware feature broke
> versions of KVM *before* this patch was submitted.
Ah, yes, I get it now. I wasn't picking up on the aspect of running older
KVM versions on the newer hardware, sorry.
I understand what you're driving at, now. We do tell the hardware teams
that add this type of feature that we need a VMCB enable bit, e.g. make it
an opt in feature. I'll be sure to communicate that to them again so that
this type of issue can be avoided in the future.
Thanks,
Tom
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL
2022-06-13 20:16 ` Tom Lendacky
@ 2022-06-13 20:28 ` Jim Mattson
0 siblings, 0 replies; 11+ messages in thread
From: Jim Mattson @ 2022-06-13 20:28 UTC (permalink / raw)
To: Tom Lendacky
Cc: Babu Moger, pbonzini, tglx, mingo, bp, fenghua.yu, tony.luck,
wanpengli, kvm, peterz, seanjc, joro, x86, kyung.min.park,
linux-kernel, krish.sadhukhan, hpa, mgross, vkuznets,
kim.phillips, wei.huang2
On Mon, Jun 13, 2022 at 1:16 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> Ah, yes, I get it now. I wasn't picking up on the aspect of running older
> KVM versions on the newer hardware, sorry.
>
> I understand what you're driving at, now. We do tell the hardware teams
> that add this type of feature that we need a VMCB enable bit, e.g. make it
> an opt in feature. I'll be sure to communicate that to them again so that
> this type of issue can be avoided in the future.
Thank you so much. Might I also ask that new features get promptly
documented in the APM?
It took us an incredibly long time to figure out why just one vCPU
thread would run slow on every GCE AMD instance. It wasn't always the
same thread, but the slow vCPU thread would still be slow even after
live migration. On a guest reboot, the slowness might migrate to a
different vCPU thread. How bizarre, right?
It turns out that, on UEFI-enabled images, one vCPU makes an EFI
firmware call, which sets IBRS. You can't see that IBRS is on from
within the guest, but it is, because of the sticky first non-zero
write behavior induced by virtual SPEC_CTRL.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-06-13 20:59 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-29 0:43 [PATCH v4 0/2] x86: Add the feature Virtual SPEC_CTRL Babu Moger
2021-01-29 0:43 ` [PATCH v4 1/2] x86/cpufeatures: Add the Virtual SPEC_CTRL feature Babu Moger
2021-01-29 0:43 ` [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL Babu Moger
2021-02-11 8:56 ` Paolo Bonzini
2021-02-11 22:42 ` Babu Moger
2022-06-04 3:11 ` Jim Mattson
2022-06-13 15:09 ` Tom Lendacky
2022-06-13 19:23 ` Jim Mattson
2022-06-13 20:16 ` Tom Lendacky
2022-06-13 20:28 ` Jim Mattson
2021-02-10 23:40 ` [PATCH v4 0/2] x86: Add the feature " Babu Moger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.