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

[PATCH 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. The series adds the feature support and
enables the feature on SVM.
---

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/kvm/svm/svm.c             |   17 ++++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

--

[PATCH 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. This feature is identified via CPUID 0x8000000A_EDX[20]. When present,
the SPEC_CTRL MSR is automatically virtualized and no longer requires
hypervisor intervention.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 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 dad350d42ecf..d649ac5ed7c7 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -335,6 +335,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*/

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

[Jim Mattson]

On Mon, Dec 7, 2020 at 2:38 PM Babu Moger <babu.moger@amd.com> wrote:
>
> Newer AMD processors have a feature to virtualize the use of the SPEC_CTRL
> MSR. This feature is identified via CPUID 0x8000000A_EDX[20]. When present,
> the SPEC_CTRL MSR is automatically virtualized and no longer requires
> hypervisor intervention.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  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 dad350d42ecf..d649ac5ed7c7 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -335,6 +335,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 */

Shouldn't this bit be reported by KVM_GET_SUPPORTED_CPUID when it's
enumerated on the host?

>  /* Intel-defined CPU features, CPUID level 0x00000007:0 (ECX), word 16 */
>  #define X86_FEATURE_AVX512VBMI         (16*32+ 1) /* AVX512 Vector Bit Manipulation instructions*/
>

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

[Babu Moger]

On 12/7/20 5:22 PM, Jim Mattson wrote:
> On Mon, Dec 7, 2020 at 2:38 PM Babu Moger <babu.moger@amd.com> wrote:
>>
>> Newer AMD processors have a feature to virtualize the use of the SPEC_CTRL
>> MSR. This feature is identified via CPUID 0x8000000A_EDX[20]. When present,
>> the SPEC_CTRL MSR is automatically virtualized and no longer requires
>> hypervisor intervention.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  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 dad350d42ecf..d649ac5ed7c7 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -335,6 +335,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 */
> 
> Shouldn't this bit be reported by KVM_GET_SUPPORTED_CPUID when it's
> enumerated on the host?

Jim, I am not sure if this needs to be reported by
KVM_GET_SUPPORTED_CPUID. I dont see V_VMSAVE_VMLOAD or VGIF being reported
via KVM_GET_SUPPORTED_CPUID. Do you see the need for that?


>>  /* Intel-defined CPU features, CPUID level 0x00000007:0 (ECX), word 16 */
>>  #define X86_FEATURE_AVX512VBMI         (16*32+ 1) /* AVX512 Vector Bit Manipulation instructions*/
>>

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

[Jim Mattson]

On Wed, Dec 9, 2020 at 2:39 PM Babu Moger <babu.moger@amd.com> wrote:
>
>
>
> On 12/7/20 5:22 PM, Jim Mattson wrote:
> > On Mon, Dec 7, 2020 at 2:38 PM Babu Moger <babu.moger@amd.com> wrote:
> >>
> >> Newer AMD processors have a feature to virtualize the use of the SPEC_CTRL
> >> MSR. This feature is identified via CPUID 0x8000000A_EDX[20]. When present,
> >> the SPEC_CTRL MSR is automatically virtualized and no longer requires
> >> hypervisor intervention.
> >>
> >> Signed-off-by: Babu Moger <babu.moger@amd.com>
> >> ---
> >>  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 dad350d42ecf..d649ac5ed7c7 100644
> >> --- a/arch/x86/include/asm/cpufeatures.h
> >> +++ b/arch/x86/include/asm/cpufeatures.h
> >> @@ -335,6 +335,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 */
> >
> > Shouldn't this bit be reported by KVM_GET_SUPPORTED_CPUID when it's
> > enumerated on the host?
>
> Jim, I am not sure if this needs to be reported by
> KVM_GET_SUPPORTED_CPUID. I dont see V_VMSAVE_VMLOAD or VGIF being reported
> via KVM_GET_SUPPORTED_CPUID. Do you see the need for that?

Every little bit helps. No, it isn't *needed*. But then again, this
entire patchset isn't *needed*, is it?

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

[Babu Moger]

On 12/9/20 5:11 PM, Jim Mattson wrote:
> On Wed, Dec 9, 2020 at 2:39 PM Babu Moger <babu.moger@amd.com> wrote:
>>
>>
>>
>> On 12/7/20 5:22 PM, Jim Mattson wrote:
>>> On Mon, Dec 7, 2020 at 2:38 PM Babu Moger <babu.moger@amd.com> wrote:
>>>>
>>>> Newer AMD processors have a feature to virtualize the use of the SPEC_CTRL
>>>> MSR. This feature is identified via CPUID 0x8000000A_EDX[20]. When present,
>>>> the SPEC_CTRL MSR is automatically virtualized and no longer requires
>>>> hypervisor intervention.
>>>>
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> ---
>>>>  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 dad350d42ecf..d649ac5ed7c7 100644
>>>> --- a/arch/x86/include/asm/cpufeatures.h
>>>> +++ b/arch/x86/include/asm/cpufeatures.h
>>>> @@ -335,6 +335,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 */
>>>
>>> Shouldn't this bit be reported by KVM_GET_SUPPORTED_CPUID when it's
>>> enumerated on the host?
>>
>> Jim, I am not sure if this needs to be reported by
>> KVM_GET_SUPPORTED_CPUID. I dont see V_VMSAVE_VMLOAD or VGIF being reported
>> via KVM_GET_SUPPORTED_CPUID. Do you see the need for that?
> 
> Every little bit helps. No, it isn't *needed*. But then again, this
> entire patchset isn't *needed*, is it?
> 

Working on v2 of these patches. Saw this code comment(in
arch/x86/kvm/cpuid.c) on about exposing SVM features to the guest.


        /*
         * Hide all SVM features by default, SVM will set the cap bits for
         * features it emulates and/or exposes for L1.
         */
        kvm_cpu_cap_mask(CPUID_8000_000A_EDX, 0);


Should we go ahead with the changes here?

Thanks
Babu

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

[Sean Christopherson]

On Tue, Dec 22, 2020, Babu Moger wrote:
> 
> On 12/9/20 5:11 PM, Jim Mattson wrote:
> > On Wed, Dec 9, 2020 at 2:39 PM Babu Moger <babu.moger@amd.com> wrote:
> >>
> >> On 12/7/20 5:22 PM, Jim Mattson wrote:
> >>> On Mon, Dec 7, 2020 at 2:38 PM Babu Moger <babu.moger@amd.com> wrote:
> >>>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> >>>> index dad350d42ecf..d649ac5ed7c7 100644
> >>>> --- a/arch/x86/include/asm/cpufeatures.h
> >>>> +++ b/arch/x86/include/asm/cpufeatures.h
> >>>> @@ -335,6 +335,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 */
> >>>
> >>> Shouldn't this bit be reported by KVM_GET_SUPPORTED_CPUID when it's
> >>> enumerated on the host?
> >>
> >> Jim, I am not sure if this needs to be reported by
> >> KVM_GET_SUPPORTED_CPUID. I dont see V_VMSAVE_VMLOAD or VGIF being reported
> >> via KVM_GET_SUPPORTED_CPUID. Do you see the need for that?
> > 
> > Every little bit helps. No, it isn't *needed*. But then again, this
> > entire patchset isn't *needed*, is it?
> > 
> 
> Working on v2 of these patches. Saw this code comment(in
> arch/x86/kvm/cpuid.c) on about exposing SVM features to the guest.
> 
> 
>         /*
>          * Hide all SVM features by default, SVM will set the cap bits for
>          * features it emulates and/or exposes for L1.
>          */
>         kvm_cpu_cap_mask(CPUID_8000_000A_EDX, 0);
> 
> 
> Should we go ahead with the changes here?

Probably not, as the current SVM implementation aligns with the intended use of
KVM_GET_SUPPORTED_CPUID.  The current approach is to enumerate what SVM features
KVM can virtualize or emulate for a nested VM, i.e. what SVM features an L1 VMM
can use and thus can be set in a vCPU's CPUID model.  For V_SPEC_CTRL, I'm
pretty sure Jim was providing feedback for the non-nested case of reporting
host/KVM support of the feature itself.

There is the question of whether or not KVM should have an ioctl() to report
what virtualization features are supported/enabled.  AFAIK, it's not truly
required as userspace can glean the information via /proc/cpuinfo (especially
now that vmx_features exists), raw CPUID, and KVM module params.  Providing an
ioctl() would likely be a bit cleaner for userspace, but I'm guessing that ship
has already sailed for most VMMs.

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

[Babu Moger]

On 12/22/20 11:41 AM, Sean Christopherson wrote:
> On Tue, Dec 22, 2020, Babu Moger wrote:
>>
>> On 12/9/20 5:11 PM, Jim Mattson wrote:
>>> On Wed, Dec 9, 2020 at 2:39 PM Babu Moger <babu.moger@amd.com> wrote:
>>>>
>>>> On 12/7/20 5:22 PM, Jim Mattson wrote:
>>>>> On Mon, Dec 7, 2020 at 2:38 PM Babu Moger <babu.moger@amd.com> wrote:
>>>>>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>>>>>> index dad350d42ecf..d649ac5ed7c7 100644
>>>>>> --- a/arch/x86/include/asm/cpufeatures.h
>>>>>> +++ b/arch/x86/include/asm/cpufeatures.h
>>>>>> @@ -335,6 +335,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 */
>>>>>
>>>>> Shouldn't this bit be reported by KVM_GET_SUPPORTED_CPUID when it's
>>>>> enumerated on the host?
>>>>
>>>> Jim, I am not sure if this needs to be reported by
>>>> KVM_GET_SUPPORTED_CPUID. I dont see V_VMSAVE_VMLOAD or VGIF being reported
>>>> via KVM_GET_SUPPORTED_CPUID. Do you see the need for that?
>>>
>>> Every little bit helps. No, it isn't *needed*. But then again, this
>>> entire patchset isn't *needed*, is it?
>>>
>>
>> Working on v2 of these patches. Saw this code comment(in
>> arch/x86/kvm/cpuid.c) on about exposing SVM features to the guest.
>>
>>
>>         /*
>>          * Hide all SVM features by default, SVM will set the cap bits for
>>          * features it emulates and/or exposes for L1.
>>          */
>>         kvm_cpu_cap_mask(CPUID_8000_000A_EDX, 0);
>>
>>
>> Should we go ahead with the changes here?
> 
> Probably not, as the current SVM implementation aligns with the intended use of
> KVM_GET_SUPPORTED_CPUID.  The current approach is to enumerate what SVM features
> KVM can virtualize or emulate for a nested VM, i.e. what SVM features an L1 VMM
> can use and thus can be set in a vCPU's CPUID model.  For V_SPEC_CTRL, I'm
> pretty sure Jim was providing feedback for the non-nested case of reporting
> host/KVM support of the feature itself.
> 
> There is the question of whether or not KVM should have an ioctl() to report
> what virtualization features are supported/enabled.  AFAIK, it's not truly
> required as userspace can glean the information via /proc/cpuinfo (especially
> now that vmx_features exists), raw CPUID, and KVM module params.  Providing an
> ioctl() would likely be a bit cleaner for userspace, but I'm guessing that ship
> has already sailed for most VMMs.
> 

Sean, Thanks for the clarifications.

[PATCH 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. When supported, the SPEC_CTRL MSR is automatically
virtualized and no longer requires hypervisor intervention.

This feature is detected 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 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.  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 MSR interception of SPEC_CTRL is disabled during
vmcb_init, so this will no longer be an issue.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kvm/svm/svm.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 79b3a564f1c9..3d73ec0cdb87 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1230,6 +1230,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);
 
@@ -3590,7 +3598,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);
 
@@ -3609,12 +3618,14 @@ 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);
 
 	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);
 
 	vcpu->arch.cr2 = svm->vmcb->save.cr2;
 	vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;

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

[Jim Mattson]

On Mon, Dec 7, 2020 at 2:38 PM Babu Moger <babu.moger@amd.com> wrote:
>
> Newer AMD processors have a feature to virtualize the use of the
> SPEC_CTRL MSR. When supported, the SPEC_CTRL MSR is automatically
> virtualized and no longer requires hypervisor intervention.
>
> This feature is detected 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 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.  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 MSR interception of SPEC_CTRL is disabled during
> vmcb_init, so this will no longer be an issue.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

Shouldn't there be some code to initialize a new "guest SPEC_CTRL"
value in the VMCB, both at vCPU creation, and at virtual processor
reset?

>  arch/x86/kvm/svm/svm.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 79b3a564f1c9..3d73ec0cdb87 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1230,6 +1230,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);
>
> @@ -3590,7 +3598,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);

Is this correct for the nested case? Presumably, there is now a "guest
SPEC_CTRL" value somewhere in the VMCB. If L1 does not intercept this
MSR, then we need to transfer the "guest SPEC_CTRL" value from the
vmcb01 to the vmcb02, don't we?

>         svm_vcpu_enter_exit(vcpu, svm);
>
> @@ -3609,12 +3618,14 @@ 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);

Is this correct for the nested case? If L1 does not intercept this
MSR, then it might have changed while L2 is running. Presumably, the
hardware has stored the new value somewhere in the vmcb02 at #VMEXIT,
but now we need to move that value into the vmcb01, don't we?

>         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);
>
>         vcpu->arch.cr2 = svm->vmcb->save.cr2;
>         vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
>

It would be great if you could add some tests to kvm-unit-tests.

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

[Babu Moger]

Hi Jim,

> -----Original Message-----
> From: Jim Mattson <jmattson@google.com>
> Sent: Monday, December 7, 2020 5:06 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>; Thomas Gleixner
> <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; Borislav Petkov
> <bp@alien8.de>; Yu, Fenghua <fenghua.yu@intel.com>; Tony Luck
> <tony.luck@intel.com>; Wanpeng Li <wanpengli@tencent.com>; kvm list
> <kvm@vger.kernel.org>; Lendacky, Thomas <Thomas.Lendacky@amd.com>;
> Peter Zijlstra <peterz@infradead.org>; Sean Christopherson
> <seanjc@google.com>; Joerg Roedel <joro@8bytes.org>; the arch/x86
> maintainers <x86@kernel.org>; kyung.min.park@intel.com; LKML <linux-
> kernel@vger.kernel.org>; Krish Sadhukhan <krish.sadhukhan@oracle.com>; H .
> Peter Anvin <hpa@zytor.com>; mgross@linux.intel.com; Vitaly Kuznetsov
> <vkuznets@redhat.com>; Phillips, Kim <kim.phillips@amd.com>; Huang2, Wei
> <Wei.Huang2@amd.com>
> Subject: Re: [PATCH 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL
> 
> On Mon, Dec 7, 2020 at 2:38 PM Babu Moger <babu.moger@amd.com> wrote:
> >
> > Newer AMD processors have a feature to virtualize the use of the
> > SPEC_CTRL MSR. When supported, the SPEC_CTRL MSR is automatically
> > virtualized and no longer requires hypervisor intervention.
> >
> > This feature is detected 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 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.  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 MSR interception of SPEC_CTRL is disabled during
> > vmcb_init, so this will no longer be an issue.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> 
> Shouldn't there be some code to initialize a new "guest SPEC_CTRL"
> value in the VMCB, both at vCPU creation, and at virtual processor reset?

Yes, I think so. I will check on this.

> 
> >  arch/x86/kvm/svm/svm.c |   17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index
> > 79b3a564f1c9..3d73ec0cdb87 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -1230,6 +1230,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);
> >
> > @@ -3590,7 +3598,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);
> 
> Is this correct for the nested case? Presumably, there is now a "guest
> SPEC_CTRL" value somewhere in the VMCB. If L1 does not intercept this MSR,
> then we need to transfer the "guest SPEC_CTRL" value from the
> vmcb01 to the vmcb02, don't we?

Here is the text from to be published documentation.
"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. For non- SNP enabled guests,
processor behavior is controlled by the logical OR of the two registers.
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."  This offset is into
the save area of the VMCB (i.e. 0x400 + 0x2E0).

The feature X86_FEATURE_V_SPEC_CTRL will not be advertised to guests.
So, the guest will use the same mechanism as today where it will save and
restore the value into/from svm->spec_ctrl. If the value saved in the VMSA
is left untouched, both an L1 and L2 guest will get the proper value.
Thing that matters is the initial setup of vmcb01 and vmcb02 when this
feature is available in host(bare metal). I am going to investigate that
part. Do you still think I am missing something here?


> 
> >         svm_vcpu_enter_exit(vcpu, svm);
> >
> > @@ -3609,12 +3618,14 @@ 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);
> 
> Is this correct for the nested case? If L1 does not intercept this MSR, then it
> might have changed while L2 is running. Presumably, the hardware has stored
> the new value somewhere in the vmcb02 at #VMEXIT, but now we need to move
> that value into the vmcb01, don't we?
> 
> >         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);
> >
> >         vcpu->arch.cr2 = svm->vmcb->save.cr2;
> >         vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
> >
> 
> It would be great if you could add some tests to kvm-unit-tests.

Yes. I will check on this part.

Thanks
Babu

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

[Jim Mattson]

On Thu, Dec 10, 2020 at 1:26 PM Babu Moger <babu.moger@amd.com> wrote:
>
> Hi Jim,
>
> > -----Original Message-----
> > From: Jim Mattson <jmattson@google.com>
> > Sent: Monday, December 7, 2020 5:06 PM
> > To: Moger, Babu <Babu.Moger@amd.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>; Thomas Gleixner
> > <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; Borislav Petkov
> > <bp@alien8.de>; Yu, Fenghua <fenghua.yu@intel.com>; Tony Luck
> > <tony.luck@intel.com>; Wanpeng Li <wanpengli@tencent.com>; kvm list
> > <kvm@vger.kernel.org>; Lendacky, Thomas <Thomas.Lendacky@amd.com>;
> > Peter Zijlstra <peterz@infradead.org>; Sean Christopherson
> > <seanjc@google.com>; Joerg Roedel <joro@8bytes.org>; the arch/x86
> > maintainers <x86@kernel.org>; kyung.min.park@intel.com; LKML <linux-
> > kernel@vger.kernel.org>; Krish Sadhukhan <krish.sadhukhan@oracle.com>; H .
> > Peter Anvin <hpa@zytor.com>; mgross@linux.intel.com; Vitaly Kuznetsov
> > <vkuznets@redhat.com>; Phillips, Kim <kim.phillips@amd.com>; Huang2, Wei
> > <Wei.Huang2@amd.com>
> > Subject: Re: [PATCH 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL
> >
> > On Mon, Dec 7, 2020 at 2:38 PM Babu Moger <babu.moger@amd.com> wrote:
> > >
> > > Newer AMD processors have a feature to virtualize the use of the
> > > SPEC_CTRL MSR. When supported, the SPEC_CTRL MSR is automatically
> > > virtualized and no longer requires hypervisor intervention.
> > >
> > > This feature is detected 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 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.  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 MSR interception of SPEC_CTRL is disabled during
> > > vmcb_init, so this will no longer be an issue.
> > >
> > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > ---
> >
> > Shouldn't there be some code to initialize a new "guest SPEC_CTRL"
> > value in the VMCB, both at vCPU creation, and at virtual processor reset?
>
> Yes, I think so. I will check on this.
>
> >
> > >  arch/x86/kvm/svm/svm.c |   17 ++++++++++++++---
> > >  1 file changed, 14 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index
> > > 79b3a564f1c9..3d73ec0cdb87 100644
> > > --- a/arch/x86/kvm/svm/svm.c
> > > +++ b/arch/x86/kvm/svm/svm.c
> > > @@ -1230,6 +1230,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);
> > >
> > > @@ -3590,7 +3598,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);
> >
> > Is this correct for the nested case? Presumably, there is now a "guest
> > SPEC_CTRL" value somewhere in the VMCB. If L1 does not intercept this MSR,
> > then we need to transfer the "guest SPEC_CTRL" value from the
> > vmcb01 to the vmcb02, don't we?
>
> Here is the text from to be published documentation.
> "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. For non- SNP enabled guests,
> processor behavior is controlled by the logical OR of the two registers.
> 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."  This offset is into
> the save area of the VMCB (i.e. 0x400 + 0x2E0).
>
> The feature X86_FEATURE_V_SPEC_CTRL will not be advertised to guests.
> So, the guest will use the same mechanism as today where it will save and
> restore the value into/from svm->spec_ctrl. If the value saved in the VMSA
> is left untouched, both an L1 and L2 guest will get the proper value.
> Thing that matters is the initial setup of vmcb01 and vmcb02 when this
> feature is available in host(bare metal). I am going to investigate that
> part. Do you still think I am missing something here?

It doesn't matter whether X86_FEATURE_V_SPEC_CTRL is advertised to L1
or not. If L1 doesn't virtualize MSR_SPEC_CTRL for L2, then L1 and L2
share the same value for that MSR. With this change, the current value
in vmcb01 is only in vmcb01, and doesn't get propagated anywhere else.
Hence, if L1 changes the value of MSR_SPEC_CTRL, that change is not
visible to L2.

Thinking about what Sean said about live migration, I think the
correct solution here is that the authoritative value for this MSR
should continue to live in svm->spec_ctrl. When the CPU supports
X86_FEATURE_V_SPEC_CTRL, we should just transfer the value into the
VMCB prior to VMRUN and out of the VMCB after #VMEXIT.

>
> >
> > >         svm_vcpu_enter_exit(vcpu, svm);
> > >
> > > @@ -3609,12 +3618,14 @@ 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);
> >
> > Is this correct for the nested case? If L1 does not intercept this MSR, then it
> > might have changed while L2 is running. Presumably, the hardware has stored
> > the new value somewhere in the vmcb02 at #VMEXIT, but now we need to move
> > that value into the vmcb01, don't we?
> >
> > >         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);
> > >
> > >         vcpu->arch.cr2 = svm->vmcb->save.cr2;
> > >         vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
> > >
> >
> > It would be great if you could add some tests to kvm-unit-tests.
>
> Yes. I will check on this part.
>
> Thanks
> Babu

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

[Babu Moger]

On 12/10/20 3:36 PM, Jim Mattson wrote:
> On Thu, Dec 10, 2020 at 1:26 PM Babu Moger <babu.moger@amd.com> wrote:
>>
>> Hi Jim,
>>
>>> -----Original Message-----
>>> From: Jim Mattson <jmattson@google.com>
>>> Sent: Monday, December 7, 2020 5:06 PM
>>> To: Moger, Babu <Babu.Moger@amd.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>; Thomas Gleixner
>>> <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; Borislav Petkov
>>> <bp@alien8.de>; Yu, Fenghua <fenghua.yu@intel.com>; Tony Luck
>>> <tony.luck@intel.com>; Wanpeng Li <wanpengli@tencent.com>; kvm list
>>> <kvm@vger.kernel.org>; Lendacky, Thomas <Thomas.Lendacky@amd.com>;
>>> Peter Zijlstra <peterz@infradead.org>; Sean Christopherson
>>> <seanjc@google.com>; Joerg Roedel <joro@8bytes.org>; the arch/x86
>>> maintainers <x86@kernel.org>; kyung.min.park@intel.com; LKML <linux-
>>> kernel@vger.kernel.org>; Krish Sadhukhan <krish.sadhukhan@oracle.com>; H .
>>> Peter Anvin <hpa@zytor.com>; mgross@linux.intel.com; Vitaly Kuznetsov
>>> <vkuznets@redhat.com>; Phillips, Kim <kim.phillips@amd.com>; Huang2, Wei
>>> <Wei.Huang2@amd.com>
>>> Subject: Re: [PATCH 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL
>>>
>>> On Mon, Dec 7, 2020 at 2:38 PM Babu Moger <babu.moger@amd.com> wrote:
>>>>
>>>> Newer AMD processors have a feature to virtualize the use of the
>>>> SPEC_CTRL MSR. When supported, the SPEC_CTRL MSR is automatically
>>>> virtualized and no longer requires hypervisor intervention.
>>>>
>>>> This feature is detected 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 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.  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 MSR interception of SPEC_CTRL is disabled during
>>>> vmcb_init, so this will no longer be an issue.
>>>>
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> ---
>>>
>>> Shouldn't there be some code to initialize a new "guest SPEC_CTRL"
>>> value in the VMCB, both at vCPU creation, and at virtual processor reset?
>>
>> Yes, I think so. I will check on this.
>>
>>>
>>>>  arch/x86/kvm/svm/svm.c |   17 ++++++++++++++---
>>>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index
>>>> 79b3a564f1c9..3d73ec0cdb87 100644
>>>> --- a/arch/x86/kvm/svm/svm.c
>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>> @@ -1230,6 +1230,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);
>>>>
>>>> @@ -3590,7 +3598,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);
>>>
>>> Is this correct for the nested case? Presumably, there is now a "guest
>>> SPEC_CTRL" value somewhere in the VMCB. If L1 does not intercept this MSR,
>>> then we need to transfer the "guest SPEC_CTRL" value from the
>>> vmcb01 to the vmcb02, don't we?
>>
>> Here is the text from to be published documentation.
>> "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. For non- SNP enabled guests,
>> processor behavior is controlled by the logical OR of the two registers.
>> 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."  This offset is into
>> the save area of the VMCB (i.e. 0x400 + 0x2E0).
>>
>> The feature X86_FEATURE_V_SPEC_CTRL will not be advertised to guests.
>> So, the guest will use the same mechanism as today where it will save and
>> restore the value into/from svm->spec_ctrl. If the value saved in the VMSA
>> is left untouched, both an L1 and L2 guest will get the proper value.
>> Thing that matters is the initial setup of vmcb01 and vmcb02 when this
>> feature is available in host(bare metal). I am going to investigate that
>> part. Do you still think I am missing something here?
> 
> It doesn't matter whether X86_FEATURE_V_SPEC_CTRL is advertised to L1
> or not. If L1 doesn't virtualize MSR_SPEC_CTRL for L2, then L1 and L2
> share the same value for that MSR. With this change, the current value
> in vmcb01 is only in vmcb01, and doesn't get propagated anywhere else.
> Hence, if L1 changes the value of MSR_SPEC_CTRL, that change is not
> visible to L2.
> 
> Thinking about what Sean said about live migration, I think the
> correct solution here is that the authoritative value for this MSR
> should continue to live in svm->spec_ctrl. When the CPU supports
> X86_FEATURE_V_SPEC_CTRL, we should just transfer the value into the
> VMCB prior to VMRUN and out of the VMCB after #VMEXIT.

Ok. Got it. I will try this approach. Thanks for the suggestion.

> 
>>
>>>
>>>>         svm_vcpu_enter_exit(vcpu, svm);
>>>>
>>>> @@ -3609,12 +3618,14 @@ 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);
>>>
>>> Is this correct for the nested case? If L1 does not intercept this MSR, then it
>>> might have changed while L2 is running. Presumably, the hardware has stored
>>> the new value somewhere in the vmcb02 at #VMEXIT, but now we need to move
>>> that value into the vmcb01, don't we?
>>>
>>>>         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);
>>>>
>>>>         vcpu->arch.cr2 = svm->vmcb->save.cr2;
>>>>         vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
>>>>
>>>
>>> It would be great if you could add some tests to kvm-unit-tests.
>>
>> Yes. I will check on this part.
>>
>> Thanks
>> Babu

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

[Babu Moger]

On 12/7/20 5:06 PM, Jim Mattson wrote:
> On Mon, Dec 7, 2020 at 2:38 PM Babu Moger <babu.moger@amd.com> wrote:
>>
>> Newer AMD processors have a feature to virtualize the use of the
>> SPEC_CTRL MSR. When supported, the SPEC_CTRL MSR is automatically
>> virtualized and no longer requires hypervisor intervention.
>>
>> This feature is detected 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 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.  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 MSR interception of SPEC_CTRL is disabled during
>> vmcb_init, so this will no longer be an issue.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> Shouldn't there be some code to initialize a new "guest SPEC_CTRL"
> value in the VMCB, both at vCPU creation, and at virtual processor
> reset?
> 
>>  arch/x86/kvm/svm/svm.c |   17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 79b3a564f1c9..3d73ec0cdb87 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -1230,6 +1230,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);
>>
>> @@ -3590,7 +3598,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);
> 
> Is this correct for the nested case? Presumably, there is now a "guest
> SPEC_CTRL" value somewhere in the VMCB. If L1 does not intercept this
> MSR, then we need to transfer the "guest SPEC_CTRL" value from the
> vmcb01 to the vmcb02, don't we?
> 
>>         svm_vcpu_enter_exit(vcpu, svm);
>>
>> @@ -3609,12 +3618,14 @@ 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);
> 
> Is this correct for the nested case? If L1 does not intercept this
> MSR, then it might have changed while L2 is running. Presumably, the
> hardware has stored the new value somewhere in the vmcb02 at #VMEXIT,
> but now we need to move that value into the vmcb01, don't we?
> 
>>         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);
>>
>>         vcpu->arch.cr2 = svm->vmcb->save.cr2;
>>         vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
>>
> 
> It would be great if you could add some tests to kvm-unit-tests.
> 

Posted the kvm unit tests. Let me know the feedback.
https://lore.kernel.org/kvm/160865324865.19910.5159218511905134908.stgit@bmoger-ubuntu/

Thanks
Babu

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

[Sean Christopherson]

On Mon, Dec 07, 2020, Babu Moger wrote:
> Newer AMD processors have a feature to virtualize the use of the
> SPEC_CTRL MSR. When supported, the SPEC_CTRL MSR is automatically
> virtualized and no longer requires hypervisor intervention.

Hrm, is MSR_AMD64_VIRT_SPEC_CTRL only for SSBD?  Should that MSR be renamed to
avoid confusion with the new form of VIRT_SPEC_CTRL?

> This feature is detected 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 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.

> The effective SPEC_CTRL setting is the guest SPEC_CTRL setting or'ed with the
> hypervisor SPEC_CTRL setting. 

This line needs to be higher in the changelog, it's easily the most relevant
info for understanding the mechanics.  Please also explicitly state the context
switching mechanics, e.g. is it tracked in the VMCB, loaded on VMRUN, saved on
VM-Exit, etc...

> 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 MSR interception of SPEC_CTRL is disabled during
> vmcb_init, so this will no longer be an issue.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kvm/svm/svm.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 79b3a564f1c9..3d73ec0cdb87 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1230,6 +1230,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);
>  
> @@ -3590,7 +3598,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);
>  
> @@ -3609,12 +3618,14 @@ 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)))

This will break migration, or maybe just cause wierdness, as userspace will
always see '0' when reading SPEC_CTRL and its writes will be ignored.  Is there
a VMCB field that holds the guest's value?  If so, this read can be skipped, and
instead the MSR set/get flows probably need to poke into the VMCB.

>  		svm->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
>  
>  	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);
>  
>  	vcpu->arch.cr2 = svm->vmcb->save.cr2;
>  	vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
> 

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

[Babu Moger]

Sean, Your response did not land in my mailbox for some reason.
 Replying using In-reply-to option.

>Hrm, is MSR_AMD64_VIRT_SPEC_CTRL only for SSBD?  Should that MSR be renamed to
>avoid confusion with the new form of VIRT_SPEC_CTRL?

We can rename it to MSR_AMD64_VIRT_SSBD_SPEC_CTRL if that is any better.

>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.

Ok. Sure.

>This line needs to be higher in the changelog, it's easily the most relevant
>info for understanding the mechanics.  Please also explicitly state the context
>switching mechanics, e.g. is it tracked in the VMCB, loaded on VMRUN, saved on
>VM-Exit, etc...

Will add more details.

>This will break migration, or maybe just cause wierdness, as userspace will
>always see '0' when reading SPEC_CTRL and its writes will be ignored.  Is there
>a VMCB field that holds the guest's value?  If so, this read can be skipped, and
>instead the MSR set/get flows probably need to poke into the VMCB.

Yes. The guest SEPC_CTRL value is saved in VMCB save area(i.e. 0x400 + 0x2E0).
Yes, will look into setting VMCB with the desired values in msr set/get if 
that helps.
Thanks
Babu