[PATCH v3 0/2] x86: Add the feature Virtual SPEC_CTRL

[PATCH v3 0/2] x86: Add the feature Virtual SPEC_CTRL

[Babu Moger]

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.
---
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:
  https://lore.kernel.org/kvm/160867624053.3471.7106539070175910424.stgit@bmoger-ubuntu/
  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.

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/sev.c             |    4 ++++
 arch/x86/kvm/svm/svm.c             |   19 +++++++++++++++----
 4 files changed, 23 insertions(+), 5 deletions(-)

--

[PATCH v3 1/2] x86/cpufeatures: Add the Virtual SPEC_CTRL feature

[Babu Moger]

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*/

[PATCH v3 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL

[Babu Moger]

Newer AMD processors have a feature to virtualize the use of the
SPEC_CTRL MSR. 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. 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.

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/sev.c     |    4 ++++
 arch/x86/kvm/svm/svm.c     |   19 +++++++++++++++----
 3 files changed, 22 insertions(+), 5 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/sev.c b/arch/x86/kvm/svm/sev.c
index c8ffdbc81709..959d6e47bd84 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -546,6 +546,10 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
 	save->pkru = svm->vcpu.arch.pkru;
 	save->xss  = svm->vcpu.arch.ia32_xss;
 
+	/* Update the guest SPEC_CTRL value in the save area */
+	if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
+		save->spec_ctrl = svm->spec_ctrl;
+
 	/*
 	 * SEV-ES will use a VMSA that is pointed to by the VMCB, not
 	 * the traditional VMSA that is part of the VMCB. Copy the
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7ef171790d02..a0cb01a5c8c5 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1244,6 +1244,9 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm_check_invpcid(svm);
 
+	if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
+		save->spec_ctrl = svm->spec_ctrl;
+
 	if (kvm_vcpu_apicv_active(&svm->vcpu))
 		avic_init_vmcb(svm);
 
@@ -3789,7 +3792,10 @@ 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))
+		svm->vmcb->save.spec_ctrl = svm->spec_ctrl;
+	else
+		x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);
 
 	svm_vcpu_enter_exit(vcpu, svm);
 
@@ -3808,13 +3814,18 @@ 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)))
-		svm->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
+	if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL))) {
+		if (static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
+			svm->spec_ctrl = svm->vmcb->save.spec_ctrl;
+		else
+			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;

Re: [PATCH v3 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL

[Sean Christopherson]

On Fri, Jan 15, 2021, Babu Moger wrote:
> ---
>  arch/x86/include/asm/svm.h |    4 +++-
>  arch/x86/kvm/svm/sev.c     |    4 ++++
>  arch/x86/kvm/svm/svm.c     |   19 +++++++++++++++----
>  3 files changed, 22 insertions(+), 5 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];

Don't nested_prepare_vmcb_save() and nested_vmcb_checks() need to be updated to
handle the new field, too?

>  	u32 pkru;
>  	u8 reserved_7a[20];
>  	u64 reserved_8;		/* rax already available at 0x01f8 */
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c8ffdbc81709..959d6e47bd84 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -546,6 +546,10 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>  	save->pkru = svm->vcpu.arch.pkru;
>  	save->xss  = svm->vcpu.arch.ia32_xss;
>  
> +	/* Update the guest SPEC_CTRL value in the save area */
> +	if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
> +		save->spec_ctrl = svm->spec_ctrl;

I think this can be dropped if svm->spec_ctrl is unused when V_SPEC_CTRL is
supported (see below).  IIUC, the memcpy() that's just out of sight would do
the propgation to the VMSA.

> +
>  	/*
>  	 * SEV-ES will use a VMSA that is pointed to by the VMCB, not
>  	 * the traditional VMSA that is part of the VMCB. Copy the
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7ef171790d02..a0cb01a5c8c5 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1244,6 +1244,9 @@ static void init_vmcb(struct vcpu_svm *svm)
>  
>  	svm_check_invpcid(svm);
>  
> +	if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
> +		save->spec_ctrl = svm->spec_ctrl;
> +
>  	if (kvm_vcpu_apicv_active(&svm->vcpu))
>  		avic_init_vmcb(svm);
>  
> @@ -3789,7 +3792,10 @@ 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))
> +		svm->vmcb->save.spec_ctrl = svm->spec_ctrl;
> +	else
> +		x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);

Can't we avoid functional code in svm_vcpu_run() entirely when V_SPEC_CTRL is
supported?  Make this code a nop, disable interception from time zero, and
read/write the VMBC field in svm_{get,set}_msr().  I.e. don't touch
svm->spec_ctrl if V_SPEC_CTRL is supported.  

	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);

	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);

>  	svm_vcpu_enter_exit(vcpu, svm);
>  
> @@ -3808,13 +3814,18 @@ 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)))
> -		svm->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
> +	if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL))) {
> +		if (static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
> +			svm->spec_ctrl = svm->vmcb->save.spec_ctrl;
> +		else
> +			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;
> 

Re: [PATCH v3 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL

[Babu Moger]

On 1/19/21 12:31 PM, Sean Christopherson wrote:
> On Fri, Jan 15, 2021, Babu Moger wrote:
>> ---
>>  arch/x86/include/asm/svm.h |    4 +++-
>>  arch/x86/kvm/svm/sev.c     |    4 ++++
>>  arch/x86/kvm/svm/svm.c     |   19 +++++++++++++++----
>>  3 files changed, 22 insertions(+), 5 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];
> 
> Don't nested_prepare_vmcb_save() and nested_vmcb_checks() need to be updated to
> handle the new field, too?

Ok. Sure. I will check and test few combinations to make sure of these
changes.

> 
>>  	u32 pkru;
>>  	u8 reserved_7a[20];
>>  	u64 reserved_8;		/* rax already available at 0x01f8 */
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index c8ffdbc81709..959d6e47bd84 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -546,6 +546,10 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>>  	save->pkru = svm->vcpu.arch.pkru;
>>  	save->xss  = svm->vcpu.arch.ia32_xss;
>>  
>> +	/* Update the guest SPEC_CTRL value in the save area */
>> +	if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
>> +		save->spec_ctrl = svm->spec_ctrl;
> 
> I think this can be dropped if svm->spec_ctrl is unused when V_SPEC_CTRL is
> supported (see below).  IIUC, the memcpy() that's just out of sight would do
> the propgation to the VMSA.

Yes, That is right. I will remove this.

> 
>> +
>>  	/*
>>  	 * SEV-ES will use a VMSA that is pointed to by the VMCB, not
>>  	 * the traditional VMSA that is part of the VMCB. Copy the
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 7ef171790d02..a0cb01a5c8c5 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -1244,6 +1244,9 @@ static void init_vmcb(struct vcpu_svm *svm)
>>  
>>  	svm_check_invpcid(svm);
>>  
>> +	if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL))
>> +		save->spec_ctrl = svm->spec_ctrl;
>> +
>>  	if (kvm_vcpu_apicv_active(&svm->vcpu))
>>  		avic_init_vmcb(svm);
>>  
>> @@ -3789,7 +3792,10 @@ 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))
>> +		svm->vmcb->save.spec_ctrl = svm->spec_ctrl;
>> +	else
>> +		x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);
> 
> Can't we avoid functional code in svm_vcpu_run() entirely when V_SPEC_CTRL is
> supported?  Make this code a nop, disable interception from time zero, and

Sean, I thought you mentioned earlier about not changing the interception
mechanism. Do you think we should disable the interception right away if
V_SPEC_CTRL is supported?

> read/write the VMBC field in svm_{get,set}_msr().  I.e. don't touch
> svm->spec_ctrl if V_SPEC_CTRL is supported.  
> 
> 	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);
> 
> 	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);

Ok. It appears the above code might work fine with changes in
svm_{get,set}_msr() to update save spec_ctlr. I will retest few
combinations to make sure it works.
Thanks
Babu

> 
>>  	svm_vcpu_enter_exit(vcpu, svm);
>>  
>> @@ -3808,13 +3814,18 @@ 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)))
>> -		svm->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
>> +	if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL))) {
>> +		if (static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
>> +			svm->spec_ctrl = svm->vmcb->save.spec_ctrl;
>> +		else
>> +			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;
>>

Re: [PATCH v3 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL

[Sean Christopherson]

On Tue, Jan 19, 2021, Babu Moger wrote:
> 
> On 1/19/21 12:31 PM, Sean Christopherson wrote:
> > On Fri, Jan 15, 2021, Babu Moger wrote:
> >> @@ -3789,7 +3792,10 @@ 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))
> >> +		svm->vmcb->save.spec_ctrl = svm->spec_ctrl;
> >> +	else
> >> +		x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);
> > 
> > Can't we avoid functional code in svm_vcpu_run() entirely when V_SPEC_CTRL is
> > supported?  Make this code a nop, disable interception from time zero, and
> 
> Sean, I thought you mentioned earlier about not changing the interception
> mechanism.

I assume you're referring to this comment?

  On Mon, Dec 7, 2020 at 3:13 PM Sean Christopherson <seanjc@google.com> wrote:
  >
  > On Mon, Dec 07, 2020, Babu Moger wrote:
  > > When this feature is enabled, the hypervisor no longer has to
  > > intercept the usage of the SPEC_CTRL MSR and no longer is required to
  > > save and restore the guest SPEC_CTRL setting when switching
  > > hypervisor/guest modes.
  >
  > Well, it's still required if the hypervisor wanted to allow the guest to turn
  > off mitigations that are enabled in the host.  I'd omit this entirely and focus
  > on what hardware does and how Linux/KVM utilize the new feature.

I wasn't suggesting that KVM should intercept SPEC_CTRL, I was pointing out that
there exists a scenario where a hypervisor would need/want to intercept
SPEC_CTRL, and that stating that a hypervisor is/isn't required to do something
isn't helpful in a KVM/Linux changelog because it doesn't describe the actual
change, nor does it help understand _why_ the change is correct.

> Do you think we should disable the interception right away if V_SPEC_CTRL is
> supported?

Yes, unless I'm missing an interaction somewhere, that will simplify the get/set
flows as they won't need to handle the case where the MSR is intercepted when
V_SPEC_CTRL is supported.  If the MSR is conditionally passed through, the get
flow would need to check if the MSR is intercepted to determine whether
svm->spec_ctrl or svm->vmcb->save.spec_ctrl holds the guest's value.

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index cce0143a6f80..40f1bd449cfa 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2678,7 +2678,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,6 +2782,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
                if (kvm_spec_ctrl_test_value(data))
                        return 1;
 
+               if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL)) {
+                       svm->vmcb->save.spec_ctrl = data;
+                       break;
+               }
+
                svm->spec_ctrl = data;
                if (!data)
                        break;

> > read/write the VMBC field in svm_{get,set}_msr().  I.e. don't touch
> > svm->spec_ctrl if V_SPEC_CTRL is supported.  

Potentially harebrained alternative...

From an architectural SVM perspective, what are the rules for VMCB fields that
don't exist (on the current hardware)?  E.g. are they reserved MBZ?  If not,
does the SVM architecture guarantee that reserved fields will not be modified?
I couldn't (quickly) find anything in the APM that explicitly states what
happens with defined-but-not-existent fields.

Specifically in the context of this change, ignoring nested correctness, what
would happen if KVM used the VMCB field even on CPUs without V_SPEC_CTRL?  Would
this explode on VMRUN?  Risk silent corruption?  Just Work (TM)?

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index cce0143a6f80..22a6a7c35d0a 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1285,7 +1285,6 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
        u32 dummy;
        u32 eax = 1;

-       svm->spec_ctrl = 0;
        svm->virt_spec_ctrl = 0;

        if (!init_event) {
@@ -2678,7 +2677,7 @@ 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;
+               msr_info->data = svm->vmcb->save.spec_ctrl;
                break;
        case MSR_AMD64_VIRT_SPEC_CTRL:
                if (!msr_info->host_initiated &&
@@ -2779,7 +2778,7 @@ 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;
+               svm->vmcb->save.spec_ctrl = data;
                if (!data)
                        break;

@@ -3791,7 +3790,7 @@ 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);
+       x86_spec_ctrl_set_guest(svm->vmcb->save.spec_ctrl, svm->virt_spec_ctrl);

        svm_vcpu_enter_exit(vcpu, svm);

@@ -3811,12 +3810,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
         * save it.
         */
        if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)))
-               svm->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
+               svm->vmcb->save.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);
+       x86_spec_ctrl_restore_host(svm->vmcb->save.spec_ctrl, svm->virt_spec_ctrl);

        if (!sev_es_guest(svm->vcpu.kvm)) {
                vcpu->arch.cr2 = svm->vmcb->save.cr2;
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 5431e6335e2e..a4f9417e3b7e 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -137,7 +137,6 @@ struct vcpu_svm {
                u64 gs_base;
        } host;

-       u64 spec_ctrl;
        /*
         * Contains guest-controlled bits of VIRT_SPEC_CTRL, which will be
         * translated into the appropriate L2_CFG bits on the host to

Re: [PATCH v3 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL

[Babu Moger]

On 1/19/21 5:45 PM, Sean Christopherson wrote:
> On Tue, Jan 19, 2021, Babu Moger wrote:
>>
>> On 1/19/21 12:31 PM, Sean Christopherson wrote:
>>> On Fri, Jan 15, 2021, Babu Moger wrote:
>>>> @@ -3789,7 +3792,10 @@ 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))
>>>> +		svm->vmcb->save.spec_ctrl = svm->spec_ctrl;
>>>> +	else
>>>> +		x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);
>>>
>>> Can't we avoid functional code in svm_vcpu_run() entirely when V_SPEC_CTRL is
>>> supported?  Make this code a nop, disable interception from time zero, and
>>
>> Sean, I thought you mentioned earlier about not changing the interception
>> mechanism.
> 
> I assume you're referring to this comment?
> 
>   On Mon, Dec 7, 2020 at 3:13 PM Sean Christopherson <seanjc@google.com> wrote:
>   >
>   > On Mon, Dec 07, 2020, Babu Moger wrote:
>   > > When this feature is enabled, the hypervisor no longer has to
>   > > intercept the usage of the SPEC_CTRL MSR and no longer is required to
>   > > save and restore the guest SPEC_CTRL setting when switching
>   > > hypervisor/guest modes.
>   >
>   > Well, it's still required if the hypervisor wanted to allow the guest to turn
>   > off mitigations that are enabled in the host.  I'd omit this entirely and focus
>   > on what hardware does and how Linux/KVM utilize the new feature.
> 
> I wasn't suggesting that KVM should intercept SPEC_CTRL, I was pointing out that
> there exists a scenario where a hypervisor would need/want to intercept
> SPEC_CTRL, and that stating that a hypervisor is/isn't required to do something
> isn't helpful in a KVM/Linux changelog because it doesn't describe the actual
> change, nor does it help understand _why_ the change is correct.

Ok. Got it.

> 
>> Do you think we should disable the interception right away if V_SPEC_CTRL is
>> supported?
> 
> Yes, unless I'm missing an interaction somewhere, that will simplify the get/set
> flows as they won't need to handle the case where the MSR is intercepted when
> V_SPEC_CTRL is supported.  If the MSR is conditionally passed through, the get
> flow would need to check if the MSR is intercepted to determine whether
> svm->spec_ctrl or svm->vmcb->save.spec_ctrl holds the guest's value.

Ok. Sure.

> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index cce0143a6f80..40f1bd449cfa 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2678,7 +2678,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,6 +2782,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>                 if (kvm_spec_ctrl_test_value(data))
>                         return 1;
>  
> +               if (boot_cpu_has(X86_FEATURE_V_SPEC_CTRL)) {
> +                       svm->vmcb->save.spec_ctrl = data;
> +                       break;
> +               }
> +
>                 svm->spec_ctrl = data;
>                 if (!data)
>                         break;
> 
>>> read/write the VMBC field in svm_{get,set}_msr().  I.e. don't touch
>>> svm->spec_ctrl if V_SPEC_CTRL is supported.  

Sure. Will make these changes.

>  
> Potentially harebrained alternative...
> 
> From an architectural SVM perspective, what are the rules for VMCB fields that
> don't exist (on the current hardware)?  E.g. are they reserved MBZ?  If not,
> does the SVM architecture guarantee that reserved fields will not be modified?
> I couldn't (quickly) find anything in the APM that explicitly states what
> happens with defined-but-not-existent fields.

I checked with our hardware design team about this. They dont want
software to make any assumptions about these fields.
thanks
Babu

> 
> Specifically in the context of this change, ignoring nested correctness, what
> would happen if KVM used the VMCB field even on CPUs without V_SPEC_CTRL?  Would
> this explode on VMRUN?  Risk silent corruption?  Just Work (TM)?
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index cce0143a6f80..22a6a7c35d0a 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1285,7 +1285,6 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>         u32 dummy;
>         u32 eax = 1;
> 
> -       svm->spec_ctrl = 0;
>         svm->virt_spec_ctrl = 0;
> 
>         if (!init_event) {
> @@ -2678,7 +2677,7 @@ 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;
> +               msr_info->data = svm->vmcb->save.spec_ctrl;
>                 break;
>         case MSR_AMD64_VIRT_SPEC_CTRL:
>                 if (!msr_info->host_initiated &&
> @@ -2779,7 +2778,7 @@ 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;
> +               svm->vmcb->save.spec_ctrl = data;
>                 if (!data)
>                         break;
> 
> @@ -3791,7 +3790,7 @@ 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);
> +       x86_spec_ctrl_set_guest(svm->vmcb->save.spec_ctrl, svm->virt_spec_ctrl);
> 
>         svm_vcpu_enter_exit(vcpu, svm);
> 
> @@ -3811,12 +3810,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
>          * save it.
>          */
>         if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)))
> -               svm->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
> +               svm->vmcb->save.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);
> +       x86_spec_ctrl_restore_host(svm->vmcb->save.spec_ctrl, svm->virt_spec_ctrl);
> 
>         if (!sev_es_guest(svm->vcpu.kvm)) {
>                 vcpu->arch.cr2 = svm->vmcb->save.cr2;
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 5431e6335e2e..a4f9417e3b7e 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -137,7 +137,6 @@ struct vcpu_svm {
>                 u64 gs_base;
>         } host;
> 
> -       u64 spec_ctrl;
>         /*
>          * Contains guest-controlled bits of VIRT_SPEC_CTRL, which will be
>          * translated into the appropriate L2_CFG bits on the host to
> 

Re: [PATCH v3 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL

[Sean Christopherson]

On Wed, Jan 20, 2021, Babu Moger wrote:
> 
> On 1/19/21 5:45 PM, Sean Christopherson wrote:

> > Potentially harebrained alternative...
> > 
> > From an architectural SVM perspective, what are the rules for VMCB fields that
> > don't exist (on the current hardware)?  E.g. are they reserved MBZ?  If not,
> > does the SVM architecture guarantee that reserved fields will not be modified?
> > I couldn't (quickly) find anything in the APM that explicitly states what
> > happens with defined-but-not-existent fields.
> 
> I checked with our hardware design team about this. They dont want
> software to make any assumptions about these fields.

Drat, I should have begged for forgiveness instead of asking for permission :-D