[PATCH] kvm: x86: Report STIBP on GET_SUPPORTED_CPUID

[PATCH] kvm: x86: Report STIBP on GET_SUPPORTED_CPUID

[Eduardo Habkost]

Months ago, we have added code to allow direct access to MSR_IA32_SPEC_CTRL
to the guest, which makes STIBP available to guests.  This was implemented
by commits d28b387fb74d ("KVM/VMX: Allow direct access to
MSR_IA32_SPEC_CTRL") and b2ac58f90540 ("KVM/SVM: Allow direct access to
MSR_IA32_SPEC_CTRL").

However, we never updated GET_SUPPORTED_CPUID to let userspace know that
STIBP can be enabled in CPUID.  Fix that by updating
kvm_cpuid_8000_0008_ebx_x86_features and kvm_cpuid_7_0_edx_x86_features.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 arch/x86/kvm/cpuid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 7bcfa61375c0..cc6dd65828a4 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -381,7 +381,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	/* cpuid 0x80000008.ebx */
 	const u32 kvm_cpuid_8000_0008_ebx_x86_features =
 		F(AMD_IBPB) | F(AMD_IBRS) | F(AMD_SSBD) | F(VIRT_SSBD) |
-		F(AMD_SSB_NO);
+		F(AMD_SSB_NO) | F(AMD_STIBP);
 
 	/* cpuid 0xC0000001.edx */
 	const u32 kvm_cpuid_C000_0001_edx_x86_features =
@@ -411,7 +411,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	/* cpuid 7.0.edx*/
 	const u32 kvm_cpuid_7_0_edx_x86_features =
 		F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
-		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES);
+		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP);
 
 	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
-- 
2.18.0.rc1.1.g3f1ff2140

Re: [PATCH] kvm: x86: Report STIBP on GET_SUPPORTED_CPUID

[Jim Mattson]

On Wed, Dec 5, 2018 at 11:19 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> Months ago, we have added code to allow direct access to MSR_IA32_SPEC_CTRL
> to the guest, which makes STIBP available to guests.  This was implemented
> by commits d28b387fb74d ("KVM/VMX: Allow direct access to
> MSR_IA32_SPEC_CTRL") and b2ac58f90540 ("KVM/SVM: Allow direct access to
> MSR_IA32_SPEC_CTRL").
>
> However, we never updated GET_SUPPORTED_CPUID to let userspace know that
> STIBP can be enabled in CPUID.  Fix that by updating
> kvm_cpuid_8000_0008_ebx_x86_features and kvm_cpuid_7_0_edx_x86_features.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH] kvm: x86: Report STIBP on GET_SUPPORTED_CPUID

[Konrad Rzeszutek Wilk]

On Wed, Dec 05, 2018 at 05:19:56PM -0200, Eduardo Habkost wrote:
> Months ago, we have added code to allow direct access to MSR_IA32_SPEC_CTRL
> to the guest, which makes STIBP available to guests.  This was implemented
> by commits d28b387fb74d ("KVM/VMX: Allow direct access to
> MSR_IA32_SPEC_CTRL") and b2ac58f90540 ("KVM/SVM: Allow direct access to
> MSR_IA32_SPEC_CTRL").
> 
> However, we never updated GET_SUPPORTED_CPUID to let userspace know that
> STIBP can be enabled in CPUID.  Fix that by updating

Ooops!
> kvm_cpuid_8000_0008_ebx_x86_features and kvm_cpuid_7_0_edx_x86_features.

Shouldn't there also be a patch in QEMU to use it? (aka, +stibp).

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you!
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  arch/x86/kvm/cpuid.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 7bcfa61375c0..cc6dd65828a4 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -381,7 +381,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  	/* cpuid 0x80000008.ebx */
>  	const u32 kvm_cpuid_8000_0008_ebx_x86_features =
>  		F(AMD_IBPB) | F(AMD_IBRS) | F(AMD_SSBD) | F(VIRT_SSBD) |
> -		F(AMD_SSB_NO);
> +		F(AMD_SSB_NO) | F(AMD_STIBP);
>  
>  	/* cpuid 0xC0000001.edx */
>  	const u32 kvm_cpuid_C000_0001_edx_x86_features =
> @@ -411,7 +411,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  	/* cpuid 7.0.edx*/
>  	const u32 kvm_cpuid_7_0_edx_x86_features =
>  		F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
> -		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES);
> +		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP);
>  
>  	/* all calls to cpuid_count() should be made on the same cpu */
>  	get_cpu();
> -- 
> 2.18.0.rc1.1.g3f1ff2140
> 

Re: [PATCH] kvm: x86: Report STIBP on GET_SUPPORTED_CPUID

[Jim Mattson]

On Wed, Dec 5, 2018 at 2:02 PM Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>
> On Wed, Dec 05, 2018 at 05:19:56PM -0200, Eduardo Habkost wrote:
> > Months ago, we have added code to allow direct access to MSR_IA32_SPEC_CTRL
> > to the guest, which makes STIBP available to guests.  This was implemented
> > by commits d28b387fb74d ("KVM/VMX: Allow direct access to
> > MSR_IA32_SPEC_CTRL") and b2ac58f90540 ("KVM/SVM: Allow direct access to
> > MSR_IA32_SPEC_CTRL").
> >
> > However, we never updated GET_SUPPORTED_CPUID to let userspace know that
> > STIBP can be enabled in CPUID.  Fix that by updating
>
> Ooops!
> > kvm_cpuid_8000_0008_ebx_x86_features and kvm_cpuid_7_0_edx_x86_features.
>
> Shouldn't there also be a patch in QEMU to use it? (aka, +stibp).

I don't see how that's relevant here. GET_SUPPORTED_CPUID should
report all of the supported CPUID features regardless of what
particular consumers choose to do with them.

Re: [PATCH] kvm: x86: Report STIBP on GET_SUPPORTED_CPUID

[Eduardo Habkost]

On Wed, Dec 05, 2018 at 05:02:06PM -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Dec 05, 2018 at 05:19:56PM -0200, Eduardo Habkost wrote:
> > Months ago, we have added code to allow direct access to MSR_IA32_SPEC_CTRL
> > to the guest, which makes STIBP available to guests.  This was implemented
> > by commits d28b387fb74d ("KVM/VMX: Allow direct access to
> > MSR_IA32_SPEC_CTRL") and b2ac58f90540 ("KVM/SVM: Allow direct access to
> > MSR_IA32_SPEC_CTRL").
> > 
> > However, we never updated GET_SUPPORTED_CPUID to let userspace know that
> > STIBP can be enabled in CPUID.  Fix that by updating
> 
> Ooops!
> > kvm_cpuid_8000_0008_ebx_x86_features and kvm_cpuid_7_0_edx_x86_features.
> 
> Shouldn't there also be a patch in QEMU to use it? (aka, +stibp).

I will submit the QEMU patch soon.

A patch exists on some downstream QEMU distributions, already,
but it was never merged upstream because GET_SUPPORTED_CPUID
never supported STIBP in the upstream kernel.  (And because in
the end it was not used for mitigating Spectre)

> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thanks!

-- 
Eduardo

Re: [PATCH] kvm: x86: Report STIBP on GET_SUPPORTED_CPUID

[Paolo Bonzini]

On 05/12/18 20:19, Eduardo Habkost wrote:
> Months ago, we have added code to allow direct access to MSR_IA32_SPEC_CTRL
> to the guest, which makes STIBP available to guests.  This was implemented
> by commits d28b387fb74d ("KVM/VMX: Allow direct access to
> MSR_IA32_SPEC_CTRL") and b2ac58f90540 ("KVM/SVM: Allow direct access to
> MSR_IA32_SPEC_CTRL").
> 
> However, we never updated GET_SUPPORTED_CPUID to let userspace know that
> STIBP can be enabled in CPUID.  Fix that by updating
> kvm_cpuid_8000_0008_ebx_x86_features and kvm_cpuid_7_0_edx_x86_features.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  arch/x86/kvm/cpuid.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 7bcfa61375c0..cc6dd65828a4 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -381,7 +381,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  	/* cpuid 0x80000008.ebx */
>  	const u32 kvm_cpuid_8000_0008_ebx_x86_features =
>  		F(AMD_IBPB) | F(AMD_IBRS) | F(AMD_SSBD) | F(VIRT_SSBD) |
> -		F(AMD_SSB_NO);
> +		F(AMD_SSB_NO) | F(AMD_STIBP);
>  
>  	/* cpuid 0xC0000001.edx */
>  	const u32 kvm_cpuid_C000_0001_edx_x86_features =
> @@ -411,7 +411,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  	/* cpuid 7.0.edx*/
>  	const u32 kvm_cpuid_7_0_edx_x86_features =
>  		F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
> -		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES);
> +		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP);
>  
>  	/* all calls to cpuid_count() should be made on the same cpu */
>  	get_cpu();
> 

Queued, thanks.

Paolo

Re: [PATCH] kvm: x86: Report STIBP on GET_SUPPORTED_CPUID

[Jim Mattson]

On Fri, Dec 14, 2018 at 2:55 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 05/12/18 20:19, Eduardo Habkost wrote:
> > Months ago, we have added code to allow direct access to MSR_IA32_SPEC_CTRL
> > to the guest, which makes STIBP available to guests.  This was implemented
> > by commits d28b387fb74d ("KVM/VMX: Allow direct access to
> > MSR_IA32_SPEC_CTRL") and b2ac58f90540 ("KVM/SVM: Allow direct access to
> > MSR_IA32_SPEC_CTRL").
> >
> > However, we never updated GET_SUPPORTED_CPUID to let userspace know that
> > STIBP can be enabled in CPUID.  Fix that by updating
> > kvm_cpuid_8000_0008_ebx_x86_features and kvm_cpuid_7_0_edx_x86_features.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ...
> Queued, thanks.
>
> Paolo


On second thought, I believe this is premature. KVM does not currently
support Intel's STIBP.

From volume 4 of the SDM, "Prevents indirect branch predictions on
*all* logical processors on the core from being controlled by any
sibling logical processor in the same core." (emphasis mine)

In particular, if two virtual HT siblings are running on different
physical cores, and one of them sets IA32_SPEC_CTRL.STIBP, KVM must
intercept the MSR write, track down the sibling vCPU thread, and
ensure that IA32_SPEC_CTRL.STIBP is set on its logical processor.
Moreover, whenever a vCPU thread migrates to a new logical processor,
IA32_SPEC_CTRL.STIBP on the logical processor must be set to the
logical or of the vCPU thread's own IA32_SPEC_CTRL.STIBP value and its
sibling vCPU thread's IA32_SPEC_CTRL.STIBP value. Note that this
implies that IA32_SPEC_CTRL cannot be a pass-through MSR.