[PATCH v3 0/4] Distinguish between variants of IBPB

[PATCH v3 0/4] Distinguish between variants of IBPB

[Jim Mattson]

Prior to Zen4, AMD's IBPB did not flush the RAS (or, in Intel
terminology, the RSB). Hence, the older version of AMD's IBPB was not
equivalent to Intel's IBPB. However, KVM has been treating them as
equivalent, synthesizing Intel's CPUID.(EAX=7,ECX=0):EDX[bit 26] on any
platform that supports the synthetic features X86_FEATURE_IBPB and
X86_FEATURE_IBRS.

Equivalence also requires a previously ignored feature on the AMD side,
CPUID Fn8000_0008_EBX[IBPB_RET], which is enumerated on Zen4.

v3: Pass through IBPB_RET from hardware to userspace. [Tom]
    Derive AMD_IBPB from X86_FEATURE_SPEC_CTRL rather than
    X86_FEATURE_IBPB. [Tom]
    Clarify semantics of X86_FEATURE_IBPB.

v2: Use IBPB_RET to identify semantic equality. [Venkatesh]

Jim Mattson (4):
  x86/cpufeatures: Clarify semantics of X86_FEATURE_IBPB
  x86/cpufeatures: Define X86_FEATURE_AMD_IBPB_RET
  KVM: x86: Advertise AMD_IBPB_RET to userspace
  KVM: x86: AMD's IBPB is not equivalent to Intel's IBPB

 arch/x86/include/asm/cpufeatures.h | 3 ++-
 arch/x86/kvm/cpuid.c               | 8 ++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v3 1/4] x86/cpufeatures: Clarify semantics of X86_FEATURE_IBPB

[Jim Mattson]

Since this synthetic feature bit is set on AMD CPUs that don't flush
the RSB on an IBPB, indicate as much in the comment, to avoid
potential confusion with the Intel IBPB semantics.

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/include/asm/cpufeatures.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index dd4682857c12..cabd6b58e8ec 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -215,7 +215,7 @@
 #define X86_FEATURE_SPEC_STORE_BYPASS_DISABLE	( 7*32+23) /* Disable Speculative Store Bypass. */
 #define X86_FEATURE_LS_CFG_SSBD		( 7*32+24)  /* AMD SSBD implementation via LS_CFG MSR */
 #define X86_FEATURE_IBRS		( 7*32+25) /* "ibrs" Indirect Branch Restricted Speculation */
-#define X86_FEATURE_IBPB		( 7*32+26) /* "ibpb" Indirect Branch Prediction Barrier */
+#define X86_FEATURE_IBPB		( 7*32+26) /* "ibpb" Indirect Branch Prediction Barrier without RSB flush */
 #define X86_FEATURE_STIBP		( 7*32+27) /* "stibp" Single Thread Indirect Branch Predictors */
 #define X86_FEATURE_ZEN			( 7*32+28) /* Generic flag for all Zen and newer */
 #define X86_FEATURE_L1TF_PTEINV		( 7*32+29) /* L1TF workaround PTE inversion */
-- 
2.46.0.295.g3b9ea8a38a-goog

Re: [PATCH v3 1/4] x86/cpufeatures: Clarify semantics of X86_FEATURE_IBPB

[Pawan Gupta]

On Fri, Aug 23, 2024 at 11:53:10AM -0700, Jim Mattson wrote:
> Since this synthetic feature bit is set on AMD CPUs that don't flush
> the RSB on an IBPB, indicate as much in the comment, to avoid
> potential confusion with the Intel IBPB semantics.
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/include/asm/cpufeatures.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index dd4682857c12..cabd6b58e8ec 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -215,7 +215,7 @@
>  #define X86_FEATURE_SPEC_STORE_BYPASS_DISABLE	( 7*32+23) /* Disable Speculative Store Bypass. */
>  #define X86_FEATURE_LS_CFG_SSBD		( 7*32+24)  /* AMD SSBD implementation via LS_CFG MSR */
>  #define X86_FEATURE_IBRS		( 7*32+25) /* "ibrs" Indirect Branch Restricted Speculation */
> -#define X86_FEATURE_IBPB		( 7*32+26) /* "ibpb" Indirect Branch Prediction Barrier */
> +#define X86_FEATURE_IBPB		( 7*32+26) /* "ibpb" Indirect Branch Prediction Barrier without RSB flush */

I don't think the comment is accurate for Intel. Maybe you meant to modify
X86_FEATURE_AMD_IBPB?

Re: [PATCH v3 1/4] x86/cpufeatures: Clarify semantics of X86_FEATURE_IBPB

[Jim Mattson]

On Mon, Aug 26, 2024 at 1:33 PM Pawan Gupta
<pawan.kumar.gupta@linux.intel.com> wrote:
>
> On Fri, Aug 23, 2024 at 11:53:10AM -0700, Jim Mattson wrote:
> > Since this synthetic feature bit is set on AMD CPUs that don't flush
> > the RSB on an IBPB, indicate as much in the comment, to avoid
> > potential confusion with the Intel IBPB semantics.
> >
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > ---
> >  arch/x86/include/asm/cpufeatures.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> > index dd4682857c12..cabd6b58e8ec 100644
> > --- a/arch/x86/include/asm/cpufeatures.h
> > +++ b/arch/x86/include/asm/cpufeatures.h
> > @@ -215,7 +215,7 @@
> >  #define X86_FEATURE_SPEC_STORE_BYPASS_DISABLE        ( 7*32+23) /* Disable Speculative Store Bypass. */
> >  #define X86_FEATURE_LS_CFG_SSBD              ( 7*32+24)  /* AMD SSBD implementation via LS_CFG MSR */
> >  #define X86_FEATURE_IBRS             ( 7*32+25) /* "ibrs" Indirect Branch Restricted Speculation */
> > -#define X86_FEATURE_IBPB             ( 7*32+26) /* "ibpb" Indirect Branch Prediction Barrier */
> > +#define X86_FEATURE_IBPB             ( 7*32+26) /* "ibpb" Indirect Branch Prediction Barrier without RSB flush */
>
> I don't think the comment is accurate for Intel. Maybe you meant to modify
> X86_FEATURE_AMD_IBPB?

It's perhaps a bit terse, but this is what I meant. Perhaps better
would be "without guaranteed RSB flush"?

Re: [PATCH v3 1/4] x86/cpufeatures: Clarify semantics of X86_FEATURE_IBPB

[Pawan Gupta]

On Mon, Aug 26, 2024 at 01:59:50PM -0700, Jim Mattson wrote:
> On Mon, Aug 26, 2024 at 1:33 PM Pawan Gupta
> <pawan.kumar.gupta@linux.intel.com> wrote:
> >
> > On Fri, Aug 23, 2024 at 11:53:10AM -0700, Jim Mattson wrote:
> > > Since this synthetic feature bit is set on AMD CPUs that don't flush
> > > the RSB on an IBPB, indicate as much in the comment, to avoid
> > > potential confusion with the Intel IBPB semantics.
> > >
> > > Signed-off-by: Jim Mattson <jmattson@google.com>
> > > ---
> > >  arch/x86/include/asm/cpufeatures.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> > > index dd4682857c12..cabd6b58e8ec 100644
> > > --- a/arch/x86/include/asm/cpufeatures.h
> > > +++ b/arch/x86/include/asm/cpufeatures.h
> > > @@ -215,7 +215,7 @@
> > >  #define X86_FEATURE_SPEC_STORE_BYPASS_DISABLE        ( 7*32+23) /* Disable Speculative Store Bypass. */
> > >  #define X86_FEATURE_LS_CFG_SSBD              ( 7*32+24)  /* AMD SSBD implementation via LS_CFG MSR */
> > >  #define X86_FEATURE_IBRS             ( 7*32+25) /* "ibrs" Indirect Branch Restricted Speculation */
> > > -#define X86_FEATURE_IBPB             ( 7*32+26) /* "ibpb" Indirect Branch Prediction Barrier */
> > > +#define X86_FEATURE_IBPB             ( 7*32+26) /* "ibpb" Indirect Branch Prediction Barrier without RSB flush */
> >
> > I don't think the comment is accurate for Intel. Maybe you meant to modify
> > X86_FEATURE_AMD_IBPB?
> 
> It's perhaps a bit terse, but this is what I meant. Perhaps better
> would be "without guaranteed RSB flush"?

That looks more accurate to me, thanks for the clarification.

[PATCH v3 2/4] x86/cpufeatures: Define X86_FEATURE_AMD_IBPB_RET

[Jim Mattson]

AMD's initial implementation of IBPB did not clear the return address
predictor. Beginning with Zen4, AMD's IBPB *does* clear the return
address predictor. This behavior is enumerated by
CPUID.80000008H:EBX.IBPB_RET[bit 30].

Define X86_FEATURE_AMD_IBPB_RET for use in KVM_GET_SUPPORTED_CPUID,
when determining cross-vendor capabilities.

Suggested-by: Venkatesh Srinivas <venkateshs@chromium.org>
Signed-off-by: Jim Mattson <jmattson@google.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 cabd6b58e8ec..0ed131f160dc 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -348,6 +348,7 @@
 #define X86_FEATURE_CPPC		(13*32+27) /* "cppc" Collaborative Processor Performance Control */
 #define X86_FEATURE_AMD_PSFD            (13*32+28) /* Predictive Store Forwarding Disable */
 #define X86_FEATURE_BTC_NO		(13*32+29) /* Not vulnerable to Branch Type Confusion */
+#define X86_FEATURE_AMD_IBPB_RET	(13*32+30) /* IBPB clears return address predictor */
 #define X86_FEATURE_BRS			(13*32+31) /* "brs" Branch Sampling available */
 
 /* Thermal and Power Management Leaf, CPUID level 0x00000006 (EAX), word 14 */
-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v3 3/4] KVM: x86: Advertise AMD_IBPB_RET to userspace

[Jim Mattson]

This is an inherent feature of IA32_PRED_CMD[0], so it is trivially
virtualizable (as long as IA32_PRED_CMD[0] is virtualized).

Suggested-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 2617be544480..ec7b2ca3b4d3 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -751,7 +751,7 @@ void kvm_set_cpu_caps(void)
 		F(CLZERO) | F(XSAVEERPTR) |
 		F(WBNOINVD) | F(AMD_IBPB) | F(AMD_IBRS) | F(AMD_SSBD) | F(VIRT_SSBD) |
 		F(AMD_SSB_NO) | F(AMD_STIBP) | F(AMD_STIBP_ALWAYS_ON) |
-		F(AMD_PSFD)
+		F(AMD_PSFD) | F(AMD_IBPB_RET)
 	);
 
 	/*
-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v3 4/4] KVM: x86: AMD's IBPB is not equivalent to Intel's IBPB

[Jim Mattson]

From Intel's documention [1], "CPUID.(EAX=07H,ECX=0):EDX[26]
enumerates support for indirect branch restricted speculation (IBRS)
and the indirect branch predictor barrier (IBPB)." Further, from [2],
"Software that executed before the IBPB command cannot control the
predicted targets of indirect branches (4) executed after the command
on the same logical processor," where footnote 4 reads, "Note that
indirect branches include near call indirect, near jump indirect and
near return instructions. Because it includes near returns, it follows
that **RSB entries created before an IBPB command cannot control the
predicted targets of returns executed after the command on the same
logical processor.**" [emphasis mine]

On the other hand, AMD's IBPB "may not prevent return branch
predictions from being specified by pre-IBPB branch targets" [3].

However, some AMD processors have an "enhanced IBPB" [terminology
mine] which does clear the return address predictor. This feature is
enumerated by CPUID.80000008:EDX.IBPB_RET[bit 30] [4].

Adjust the cross-vendor features enumerated by KVM_GET_SUPPORTED_CPUID
accordingly.

[1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html
[2] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/speculative-execution-side-channel-mitigations.html#Footnotes
[3] https://www.amd.com/en/resources/product-security/bulletin/amd-sb-1040.html
[4] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24594.pdf

Fixes: 0c54914d0c52 ("KVM: x86: use Intel speculation bugs and features as derived in generic x86 code")
Suggested-by: Venkatesh Srinivas <venkateshs@chromium.org>
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index ec7b2ca3b4d3..c8d7d928ffc7 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -690,7 +690,9 @@ void kvm_set_cpu_caps(void)
 	kvm_cpu_cap_set(X86_FEATURE_TSC_ADJUST);
 	kvm_cpu_cap_set(X86_FEATURE_ARCH_CAPABILITIES);
 
-	if (boot_cpu_has(X86_FEATURE_IBPB) && boot_cpu_has(X86_FEATURE_IBRS))
+	if (boot_cpu_has(X86_FEATURE_AMD_IBPB_RET) &&
+	    boot_cpu_has(X86_FEATURE_AMD_IBPB) &&
+	    boot_cpu_has(X86_FEATURE_AMD_IBRS))
 		kvm_cpu_cap_set(X86_FEATURE_SPEC_CTRL);
 	if (boot_cpu_has(X86_FEATURE_STIBP))
 		kvm_cpu_cap_set(X86_FEATURE_INTEL_STIBP);
@@ -759,6 +761,8 @@ void kvm_set_cpu_caps(void)
 	 * arch/x86/kernel/cpu/bugs.c is kind enough to
 	 * record that in cpufeatures so use them.
 	 */
+	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+		kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
 	if (boot_cpu_has(X86_FEATURE_IBPB))
 		kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
 	if (boot_cpu_has(X86_FEATURE_IBRS))
-- 
2.46.0.295.g3b9ea8a38a-goog

Re: [PATCH v3 4/4] KVM: x86: AMD's IBPB is not equivalent to Intel's IBPB

[Tom Lendacky]

On 8/23/24 13:53, Jim Mattson wrote:
> From Intel's documention [1], "CPUID.(EAX=07H,ECX=0):EDX[26]
> enumerates support for indirect branch restricted speculation (IBRS)
> and the indirect branch predictor barrier (IBPB)." Further, from [2],
> "Software that executed before the IBPB command cannot control the
> predicted targets of indirect branches (4) executed after the command
> on the same logical processor," where footnote 4 reads, "Note that
> indirect branches include near call indirect, near jump indirect and
> near return instructions. Because it includes near returns, it follows
> that **RSB entries created before an IBPB command cannot control the
> predicted targets of returns executed after the command on the same
> logical processor.**" [emphasis mine]
> 
> On the other hand, AMD's IBPB "may not prevent return branch
> predictions from being specified by pre-IBPB branch targets" [3].
> 
> However, some AMD processors have an "enhanced IBPB" [terminology
> mine] which does clear the return address predictor. This feature is
> enumerated by CPUID.80000008:EDX.IBPB_RET[bit 30] [4].
> 
> Adjust the cross-vendor features enumerated by KVM_GET_SUPPORTED_CPUID
> accordingly.
> 
> [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html
> [2] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/speculative-execution-side-channel-mitigations.html#Footnotes
> [3] https://www.amd.com/en/resources/product-security/bulletin/amd-sb-1040.html
> [4] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24594.pdf
> 
> Fixes: 0c54914d0c52 ("KVM: x86: use Intel speculation bugs and features as derived in generic x86 code")
> Suggested-by: Venkatesh Srinivas <venkateshs@chromium.org>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index ec7b2ca3b4d3..c8d7d928ffc7 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -690,7 +690,9 @@ void kvm_set_cpu_caps(void)
>  	kvm_cpu_cap_set(X86_FEATURE_TSC_ADJUST);
>  	kvm_cpu_cap_set(X86_FEATURE_ARCH_CAPABILITIES);
>  
> -	if (boot_cpu_has(X86_FEATURE_IBPB) && boot_cpu_has(X86_FEATURE_IBRS))
> +	if (boot_cpu_has(X86_FEATURE_AMD_IBPB_RET) &&
> +	    boot_cpu_has(X86_FEATURE_AMD_IBPB) &&
> +	    boot_cpu_has(X86_FEATURE_AMD_IBRS))
>  		kvm_cpu_cap_set(X86_FEATURE_SPEC_CTRL);
>  	if (boot_cpu_has(X86_FEATURE_STIBP))
>  		kvm_cpu_cap_set(X86_FEATURE_INTEL_STIBP);
> @@ -759,6 +761,8 @@ void kvm_set_cpu_caps(void)
>  	 * arch/x86/kernel/cpu/bugs.c is kind enough to
>  	 * record that in cpufeatures so use them.
>  	 */
> +	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> +		kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);

If SPEC_CTRL is set, then IBPB is set, so you can't have AMD_IBPB_RET
without AMD_IBPB, but it just looks odd seeing them set with separate
checks with no relationship dependency for AMD_IBPB_RET on AMD_IBPB.
That's just me, though, not worth a v4 unless others feel the same.

Thanks,
Tom

>  	if (boot_cpu_has(X86_FEATURE_IBPB))
>  		kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
>  	if (boot_cpu_has(X86_FEATURE_IBRS))

Re: [PATCH v3 4/4] KVM: x86: AMD's IBPB is not equivalent to Intel's IBPB

[Sean Christopherson]

On Fri, Aug 23, 2024, Tom Lendacky wrote:
> On 8/23/24 13:53, Jim Mattson wrote:
> > From Intel's documention [1], "CPUID.(EAX=07H,ECX=0):EDX[26]
> > enumerates support for indirect branch restricted speculation (IBRS)
> > and the indirect branch predictor barrier (IBPB)." Further, from [2],
> > "Software that executed before the IBPB command cannot control the
> > predicted targets of indirect branches (4) executed after the command
> > on the same logical processor," where footnote 4 reads, "Note that
> > indirect branches include near call indirect, near jump indirect and
> > near return instructions. Because it includes near returns, it follows
> > that **RSB entries created before an IBPB command cannot control the
> > predicted targets of returns executed after the command on the same
> > logical processor.**" [emphasis mine]
> > 
> > On the other hand, AMD's IBPB "may not prevent return branch
> > predictions from being specified by pre-IBPB branch targets" [3].
> > 
> > However, some AMD processors have an "enhanced IBPB" [terminology
> > mine] which does clear the return address predictor. This feature is
> > enumerated by CPUID.80000008:EDX.IBPB_RET[bit 30] [4].
> > 
> > Adjust the cross-vendor features enumerated by KVM_GET_SUPPORTED_CPUID
> > accordingly.
> > 
> > [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html
> > [2] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/speculative-execution-side-channel-mitigations.html#Footnotes
> > [3] https://www.amd.com/en/resources/product-security/bulletin/amd-sb-1040.html
> > [4] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24594.pdf
> > 
> > Fixes: 0c54914d0c52 ("KVM: x86: use Intel speculation bugs and features as derived in generic x86 code")
> > Suggested-by: Venkatesh Srinivas <venkateshs@chromium.org>
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index ec7b2ca3b4d3..c8d7d928ffc7 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -690,7 +690,9 @@ void kvm_set_cpu_caps(void)
> >  	kvm_cpu_cap_set(X86_FEATURE_TSC_ADJUST);
> >  	kvm_cpu_cap_set(X86_FEATURE_ARCH_CAPABILITIES);
> >  
> > -	if (boot_cpu_has(X86_FEATURE_IBPB) && boot_cpu_has(X86_FEATURE_IBRS))
> > +	if (boot_cpu_has(X86_FEATURE_AMD_IBPB_RET) &&
> > +	    boot_cpu_has(X86_FEATURE_AMD_IBPB) &&
> > +	    boot_cpu_has(X86_FEATURE_AMD_IBRS))
> >  		kvm_cpu_cap_set(X86_FEATURE_SPEC_CTRL);
> >  	if (boot_cpu_has(X86_FEATURE_STIBP))
> >  		kvm_cpu_cap_set(X86_FEATURE_INTEL_STIBP);
> > @@ -759,6 +761,8 @@ void kvm_set_cpu_caps(void)
> >  	 * arch/x86/kernel/cpu/bugs.c is kind enough to
> >  	 * record that in cpufeatures so use them.
> >  	 */
> > +	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> > +		kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
> 
> If SPEC_CTRL is set, then IBPB is set, so you can't have AMD_IBPB_RET
> without AMD_IBPB, but it just looks odd seeing them set with separate
> checks with no relationship dependency for AMD_IBPB_RET on AMD_IBPB.
> That's just me, though, not worth a v4 unless others feel the same.

You thinking something like this (at the end, after the dust settles)?

	if (WARN_ON_ONCE(kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB_RET) &&
			 !kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB)))
		kvm_cpu_cap_clear(X86_FEATURE_AMD_IBPB_RET);		
> 

> Thanks,
> Tom
> 
> >  	if (boot_cpu_has(X86_FEATURE_IBPB))
> >  		kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
> >  	if (boot_cpu_has(X86_FEATURE_IBRS))

Re: [PATCH v3 4/4] KVM: x86: AMD's IBPB is not equivalent to Intel's IBPB

[Jim Mattson]

On Fri, Aug 23, 2024 at 1:51 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Aug 23, 2024, Tom Lendacky wrote:
> > On 8/23/24 13:53, Jim Mattson wrote:
> > > From Intel's documention [1], "CPUID.(EAX=07H,ECX=0):EDX[26]
> > > enumerates support for indirect branch restricted speculation (IBRS)
> > > and the indirect branch predictor barrier (IBPB)." Further, from [2],
> > > "Software that executed before the IBPB command cannot control the
> > > predicted targets of indirect branches (4) executed after the command
> > > on the same logical processor," where footnote 4 reads, "Note that
> > > indirect branches include near call indirect, near jump indirect and
> > > near return instructions. Because it includes near returns, it follows
> > > that **RSB entries created before an IBPB command cannot control the
> > > predicted targets of returns executed after the command on the same
> > > logical processor.**" [emphasis mine]
> > >
> > > On the other hand, AMD's IBPB "may not prevent return branch
> > > predictions from being specified by pre-IBPB branch targets" [3].
> > >
> > > However, some AMD processors have an "enhanced IBPB" [terminology
> > > mine] which does clear the return address predictor. This feature is
> > > enumerated by CPUID.80000008:EDX.IBPB_RET[bit 30] [4].
> > >
> > > Adjust the cross-vendor features enumerated by KVM_GET_SUPPORTED_CPUID
> > > accordingly.
> > >
> > > [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html
> > > [2] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/speculative-execution-side-channel-mitigations.html#Footnotes
> > > [3] https://www.amd.com/en/resources/product-security/bulletin/amd-sb-1040.html
> > > [4] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24594.pdf
> > >
> > > Fixes: 0c54914d0c52 ("KVM: x86: use Intel speculation bugs and features as derived in generic x86 code")
> > > Suggested-by: Venkatesh Srinivas <venkateshs@chromium.org>
> > > Signed-off-by: Jim Mattson <jmattson@google.com>
> > > ---
> > >  arch/x86/kvm/cpuid.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index ec7b2ca3b4d3..c8d7d928ffc7 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -690,7 +690,9 @@ void kvm_set_cpu_caps(void)
> > >     kvm_cpu_cap_set(X86_FEATURE_TSC_ADJUST);
> > >     kvm_cpu_cap_set(X86_FEATURE_ARCH_CAPABILITIES);
> > >
> > > -   if (boot_cpu_has(X86_FEATURE_IBPB) && boot_cpu_has(X86_FEATURE_IBRS))
> > > +   if (boot_cpu_has(X86_FEATURE_AMD_IBPB_RET) &&
> > > +       boot_cpu_has(X86_FEATURE_AMD_IBPB) &&
> > > +       boot_cpu_has(X86_FEATURE_AMD_IBRS))
> > >             kvm_cpu_cap_set(X86_FEATURE_SPEC_CTRL);
> > >     if (boot_cpu_has(X86_FEATURE_STIBP))
> > >             kvm_cpu_cap_set(X86_FEATURE_INTEL_STIBP);
> > > @@ -759,6 +761,8 @@ void kvm_set_cpu_caps(void)
> > >      * arch/x86/kernel/cpu/bugs.c is kind enough to
> > >      * record that in cpufeatures so use them.
> > >      */
> > > +   if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> > > +           kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
> >
> > If SPEC_CTRL is set, then IBPB is set, so you can't have AMD_IBPB_RET
> > without AMD_IBPB, but it just looks odd seeing them set with separate
> > checks with no relationship dependency for AMD_IBPB_RET on AMD_IBPB.
> > That's just me, though, not worth a v4 unless others feel the same.
>
> You thinking something like this (at the end, after the dust settles)?
>
>         if (WARN_ON_ONCE(kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB_RET) &&
>                          !kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB)))
>                 kvm_cpu_cap_clear(X86_FEATURE_AMD_IBPB_RET);
> >

Ugh. No.

I think it would be better to replace the subsequent vendor-neutral
tests with something like:

    if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
        kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
        kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
        kvm_cpu_cap_set(X86_FEATURE_AMD_IBRS);
    }

Again, my real preference is to leave the cross-vendor enumeration to
userspace, but I guess that ship has sailed.

> > Thanks,
> > Tom
> >
> > >     if (boot_cpu_has(X86_FEATURE_IBPB))
> > >             kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
> > >     if (boot_cpu_has(X86_FEATURE_IBRS))

Re: [PATCH v3 4/4] KVM: x86: AMD's IBPB is not equivalent to Intel's IBPB

[Tom Lendacky]

On 8/23/24 15:51, Sean Christopherson wrote:
> On Fri, Aug 23, 2024, Tom Lendacky wrote:
>> On 8/23/24 13:53, Jim Mattson wrote:
>>> From Intel's documention [1], "CPUID.(EAX=07H,ECX=0):EDX[26]
>>> enumerates support for indirect branch restricted speculation (IBRS)
>>> and the indirect branch predictor barrier (IBPB)." Further, from [2],
>>> "Software that executed before the IBPB command cannot control the
>>> predicted targets of indirect branches (4) executed after the command
>>> on the same logical processor," where footnote 4 reads, "Note that
>>> indirect branches include near call indirect, near jump indirect and
>>> near return instructions. Because it includes near returns, it follows
>>> that **RSB entries created before an IBPB command cannot control the
>>> predicted targets of returns executed after the command on the same
>>> logical processor.**" [emphasis mine]
>>>
>>> On the other hand, AMD's IBPB "may not prevent return branch
>>> predictions from being specified by pre-IBPB branch targets" [3].
>>>
>>> However, some AMD processors have an "enhanced IBPB" [terminology
>>> mine] which does clear the return address predictor. This feature is
>>> enumerated by CPUID.80000008:EDX.IBPB_RET[bit 30] [4].
>>>
>>> Adjust the cross-vendor features enumerated by KVM_GET_SUPPORTED_CPUID
>>> accordingly.
>>>
>>> [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html
>>> [2] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/speculative-execution-side-channel-mitigations.html#Footnotes
>>> [3] https://www.amd.com/en/resources/product-security/bulletin/amd-sb-1040.html
>>> [4] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24594.pdf
>>>
>>> Fixes: 0c54914d0c52 ("KVM: x86: use Intel speculation bugs and features as derived in generic x86 code")
>>> Suggested-by: Venkatesh Srinivas <venkateshs@chromium.org>
>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>> ---
>>>  arch/x86/kvm/cpuid.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>> index ec7b2ca3b4d3..c8d7d928ffc7 100644
>>> --- a/arch/x86/kvm/cpuid.c
>>> +++ b/arch/x86/kvm/cpuid.c
>>> @@ -690,7 +690,9 @@ void kvm_set_cpu_caps(void)
>>>  	kvm_cpu_cap_set(X86_FEATURE_TSC_ADJUST);
>>>  	kvm_cpu_cap_set(X86_FEATURE_ARCH_CAPABILITIES);
>>>  
>>> -	if (boot_cpu_has(X86_FEATURE_IBPB) && boot_cpu_has(X86_FEATURE_IBRS))
>>> +	if (boot_cpu_has(X86_FEATURE_AMD_IBPB_RET) &&
>>> +	    boot_cpu_has(X86_FEATURE_AMD_IBPB) &&
>>> +	    boot_cpu_has(X86_FEATURE_AMD_IBRS))
>>>  		kvm_cpu_cap_set(X86_FEATURE_SPEC_CTRL);
>>>  	if (boot_cpu_has(X86_FEATURE_STIBP))
>>>  		kvm_cpu_cap_set(X86_FEATURE_INTEL_STIBP);
>>> @@ -759,6 +761,8 @@ void kvm_set_cpu_caps(void)
>>>  	 * arch/x86/kernel/cpu/bugs.c is kind enough to
>>>  	 * record that in cpufeatures so use them.
>>>  	 */
>>> +	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>>> +		kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
>>
>> If SPEC_CTRL is set, then IBPB is set, so you can't have AMD_IBPB_RET
>> without AMD_IBPB, but it just looks odd seeing them set with separate
>> checks with no relationship dependency for AMD_IBPB_RET on AMD_IBPB.
>> That's just me, though, not worth a v4 unless others feel the same.
> 
> You thinking something like this (at the end, after the dust settles)?
> 
> 	if (WARN_ON_ONCE(kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB_RET) &&
> 			 !kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB)))
> 		kvm_cpu_cap_clear(X86_FEATURE_AMD_IBPB_RET);		

I was just thinking more along the lines of:

	if (boot_cpu_has(X86_FEATURE_IBPB)) {
		kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
		if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
			kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
	}

Thanks,
Tom

>>
> 
>> Thanks,
>> Tom
>>
>>>  	if (boot_cpu_has(X86_FEATURE_IBPB))
>>>  		kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
>>>  	if (boot_cpu_has(X86_FEATURE_IBRS))

Re: [PATCH v3 4/4] KVM: x86: AMD's IBPB is not equivalent to Intel's IBPB

[Jim Mattson]

On Fri, Aug 23, 2024 at 3:12 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 8/23/24 15:51, Sean Christopherson wrote:
> > On Fri, Aug 23, 2024, Tom Lendacky wrote:
> >> On 8/23/24 13:53, Jim Mattson wrote:
> >>> From Intel's documention [1], "CPUID.(EAX=07H,ECX=0):EDX[26]
> >>> enumerates support for indirect branch restricted speculation (IBRS)
> >>> and the indirect branch predictor barrier (IBPB)." Further, from [2],
> >>> "Software that executed before the IBPB command cannot control the
> >>> predicted targets of indirect branches (4) executed after the command
> >>> on the same logical processor," where footnote 4 reads, "Note that
> >>> indirect branches include near call indirect, near jump indirect and
> >>> near return instructions. Because it includes near returns, it follows
> >>> that **RSB entries created before an IBPB command cannot control the
> >>> predicted targets of returns executed after the command on the same
> >>> logical processor.**" [emphasis mine]
> >>>
> >>> On the other hand, AMD's IBPB "may not prevent return branch
> >>> predictions from being specified by pre-IBPB branch targets" [3].
> >>>
> >>> However, some AMD processors have an "enhanced IBPB" [terminology
> >>> mine] which does clear the return address predictor. This feature is
> >>> enumerated by CPUID.80000008:EDX.IBPB_RET[bit 30] [4].
> >>>
> >>> Adjust the cross-vendor features enumerated by KVM_GET_SUPPORTED_CPUID
> >>> accordingly.
> >>>
> >>> [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html
> >>> [2] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/speculative-execution-side-channel-mitigations.html#Footnotes
> >>> [3] https://www.amd.com/en/resources/product-security/bulletin/amd-sb-1040.html
> >>> [4] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24594.pdf
> >>>
> >>> Fixes: 0c54914d0c52 ("KVM: x86: use Intel speculation bugs and features as derived in generic x86 code")
> >>> Suggested-by: Venkatesh Srinivas <venkateshs@chromium.org>
> >>> Signed-off-by: Jim Mattson <jmattson@google.com>
> >>> ---
> >>>  arch/x86/kvm/cpuid.c | 6 +++++-
> >>>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> >>> index ec7b2ca3b4d3..c8d7d928ffc7 100644
> >>> --- a/arch/x86/kvm/cpuid.c
> >>> +++ b/arch/x86/kvm/cpuid.c
> >>> @@ -690,7 +690,9 @@ void kvm_set_cpu_caps(void)
> >>>     kvm_cpu_cap_set(X86_FEATURE_TSC_ADJUST);
> >>>     kvm_cpu_cap_set(X86_FEATURE_ARCH_CAPABILITIES);
> >>>
> >>> -   if (boot_cpu_has(X86_FEATURE_IBPB) && boot_cpu_has(X86_FEATURE_IBRS))
> >>> +   if (boot_cpu_has(X86_FEATURE_AMD_IBPB_RET) &&
> >>> +       boot_cpu_has(X86_FEATURE_AMD_IBPB) &&
> >>> +       boot_cpu_has(X86_FEATURE_AMD_IBRS))
> >>>             kvm_cpu_cap_set(X86_FEATURE_SPEC_CTRL);
> >>>     if (boot_cpu_has(X86_FEATURE_STIBP))
> >>>             kvm_cpu_cap_set(X86_FEATURE_INTEL_STIBP);
> >>> @@ -759,6 +761,8 @@ void kvm_set_cpu_caps(void)
> >>>      * arch/x86/kernel/cpu/bugs.c is kind enough to
> >>>      * record that in cpufeatures so use them.
> >>>      */
> >>> +   if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> >>> +           kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
> >>
> >> If SPEC_CTRL is set, then IBPB is set, so you can't have AMD_IBPB_RET
> >> without AMD_IBPB, but it just looks odd seeing them set with separate
> >> checks with no relationship dependency for AMD_IBPB_RET on AMD_IBPB.
> >> That's just me, though, not worth a v4 unless others feel the same.
> >
> > You thinking something like this (at the end, after the dust settles)?
> >
> >       if (WARN_ON_ONCE(kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB_RET) &&
> >                        !kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB)))
> >               kvm_cpu_cap_clear(X86_FEATURE_AMD_IBPB_RET);
>
> I was just thinking more along the lines of:
>
>         if (boot_cpu_has(X86_FEATURE_IBPB)) {
>                 kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
>                 if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>                         kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
>         }

AFAICT, there are just two reasons that X86_FEATURE_IBPB gets set:
1. The CPU reports CPUID.(EAX=7,ECX=0):EDX[bit 26] (aka X86_FEATURE_SPEC_CTRL)
2. The CPU reports CPUID Fn8000_0008_EBX[IBPB] (aka X86_FEATURE_AMD_IBPB)

Clearly, in the second case, the KVM cpu capability for AMD_IBPB will
already be set, since it's specified in the mask for
CPUID_8000_0008_EBX.

If this block of code is just trying to populate CPUID Fn8000_0008_EBX
on Intel processors, I'd rather change all of the predicates to test
for Intel features, rather than vendor-neutral features, so that the
derivation is clear. But maybe this block of code is also trying to
populate CPUID Fn8000_0008_EBX on AMD processors that may have some of
these features, but don't enumerate them via CPUID?

> Thanks,
> Tom
>
> >>
> >
> >> Thanks,
> >> Tom
> >>
> >>>     if (boot_cpu_has(X86_FEATURE_IBPB))
> >>>             kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
> >>>     if (boot_cpu_has(X86_FEATURE_IBRS))

Re: [PATCH v3 4/4] KVM: x86: AMD's IBPB is not equivalent to Intel's IBPB

[Jim Mattson]

On Fri, Aug 23, 2024 at 3:48 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Fri, Aug 23, 2024 at 3:12 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >
> > On 8/23/24 15:51, Sean Christopherson wrote:
> > > On Fri, Aug 23, 2024, Tom Lendacky wrote:
> > >> On 8/23/24 13:53, Jim Mattson wrote:
> > >>> From Intel's documention [1], "CPUID.(EAX=07H,ECX=0):EDX[26]
> > >>> enumerates support for indirect branch restricted speculation (IBRS)
> > >>> and the indirect branch predictor barrier (IBPB)." Further, from [2],
> > >>> "Software that executed before the IBPB command cannot control the
> > >>> predicted targets of indirect branches (4) executed after the command
> > >>> on the same logical processor," where footnote 4 reads, "Note that
> > >>> indirect branches include near call indirect, near jump indirect and
> > >>> near return instructions. Because it includes near returns, it follows
> > >>> that **RSB entries created before an IBPB command cannot control the
> > >>> predicted targets of returns executed after the command on the same
> > >>> logical processor.**" [emphasis mine]
> > >>>
> > >>> On the other hand, AMD's IBPB "may not prevent return branch
> > >>> predictions from being specified by pre-IBPB branch targets" [3].
> > >>>
> > >>> However, some AMD processors have an "enhanced IBPB" [terminology
> > >>> mine] which does clear the return address predictor. This feature is
> > >>> enumerated by CPUID.80000008:EDX.IBPB_RET[bit 30] [4].
> > >>>
> > >>> Adjust the cross-vendor features enumerated by KVM_GET_SUPPORTED_CPUID
> > >>> accordingly.
> > >>>
> > >>> [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html
> > >>> [2] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/speculative-execution-side-channel-mitigations.html#Footnotes
> > >>> [3] https://www.amd.com/en/resources/product-security/bulletin/amd-sb-1040.html
> > >>> [4] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24594.pdf
> > >>>
> > >>> Fixes: 0c54914d0c52 ("KVM: x86: use Intel speculation bugs and features as derived in generic x86 code")
> > >>> Suggested-by: Venkatesh Srinivas <venkateshs@chromium.org>
> > >>> Signed-off-by: Jim Mattson <jmattson@google.com>
> > >>> ---
> > >>>  arch/x86/kvm/cpuid.c | 6 +++++-
> > >>>  1 file changed, 5 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > >>> index ec7b2ca3b4d3..c8d7d928ffc7 100644
> > >>> --- a/arch/x86/kvm/cpuid.c
> > >>> +++ b/arch/x86/kvm/cpuid.c
> > >>> @@ -690,7 +690,9 @@ void kvm_set_cpu_caps(void)
> > >>>     kvm_cpu_cap_set(X86_FEATURE_TSC_ADJUST);
> > >>>     kvm_cpu_cap_set(X86_FEATURE_ARCH_CAPABILITIES);
> > >>>
> > >>> -   if (boot_cpu_has(X86_FEATURE_IBPB) && boot_cpu_has(X86_FEATURE_IBRS))
> > >>> +   if (boot_cpu_has(X86_FEATURE_AMD_IBPB_RET) &&
> > >>> +       boot_cpu_has(X86_FEATURE_AMD_IBPB) &&
> > >>> +       boot_cpu_has(X86_FEATURE_AMD_IBRS))
> > >>>             kvm_cpu_cap_set(X86_FEATURE_SPEC_CTRL);
> > >>>     if (boot_cpu_has(X86_FEATURE_STIBP))
> > >>>             kvm_cpu_cap_set(X86_FEATURE_INTEL_STIBP);
> > >>> @@ -759,6 +761,8 @@ void kvm_set_cpu_caps(void)
> > >>>      * arch/x86/kernel/cpu/bugs.c is kind enough to
> > >>>      * record that in cpufeatures so use them.
> > >>>      */
> > >>> +   if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> > >>> +           kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
> > >>
> > >> If SPEC_CTRL is set, then IBPB is set, so you can't have AMD_IBPB_RET
> > >> without AMD_IBPB, but it just looks odd seeing them set with separate
> > >> checks with no relationship dependency for AMD_IBPB_RET on AMD_IBPB.
> > >> That's just me, though, not worth a v4 unless others feel the same.
> > >
> > > You thinking something like this (at the end, after the dust settles)?
> > >
> > >       if (WARN_ON_ONCE(kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB_RET) &&
> > >                        !kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB)))
> > >               kvm_cpu_cap_clear(X86_FEATURE_AMD_IBPB_RET);
> >
> > I was just thinking more along the lines of:
> >
> >         if (boot_cpu_has(X86_FEATURE_IBPB)) {
> >                 kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
> >                 if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> >                         kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
> >         }
>
> AFAICT, there are just two reasons that X86_FEATURE_IBPB gets set:
> 1. The CPU reports CPUID.(EAX=7,ECX=0):EDX[bit 26] (aka X86_FEATURE_SPEC_CTRL)
> 2. The CPU reports CPUID Fn8000_0008_EBX[IBPB] (aka X86_FEATURE_AMD_IBPB)
>
> Clearly, in the second case, the KVM cpu capability for AMD_IBPB will
> already be set, since it's specified in the mask for
> CPUID_8000_0008_EBX.
>
> If this block of code is just trying to populate CPUID Fn8000_0008_EBX
> on Intel processors, I'd rather change all of the predicates to test
> for Intel features, rather than vendor-neutral features, so that the
> derivation is clear. But maybe this block of code is also trying to
> populate CPUID Fn8000_0008_EBX on AMD processors that may have some of
> these features, but don't enumerate them via CPUID?

There's another argument for just nuking these cross-vendor
derivations. How do we factor in CVE-2022-26373 (Post-barrier Return
Stack Buffer Predictions)?
Intel CPUs without IA32_ARCH_CAPABILITIES.PBRSB_NO[bit 24] have a
weaker IBPB than AMD CPUs with CPUID Fn8000_0008_EBX[IBPB_RET], and
probably should not be enumerating that CPUID bit.

Trying to derive cross-vendor mitigation equivalence is just going to
end in tears.

> > Thanks,
> > Tom
> >
> > >>
> > >
> > >> Thanks,
> > >> Tom
> > >>
> > >>>     if (boot_cpu_has(X86_FEATURE_IBPB))
> > >>>             kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
> > >>>     if (boot_cpu_has(X86_FEATURE_IBRS))

Re: [PATCH v3 4/4] KVM: x86: AMD's IBPB is not equivalent to Intel's IBPB

[Sean Christopherson]

On Fri, Aug 23, 2024, Jim Mattson wrote:
> On Fri, Aug 23, 2024 at 3:48 PM Jim Mattson <jmattson@google.com> wrote:
> >
> > On Fri, Aug 23, 2024 at 3:12 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> > >
> > > On 8/23/24 15:51, Sean Christopherson wrote:
> > > > On Fri, Aug 23, 2024, Tom Lendacky wrote:
> > > >> On 8/23/24 13:53, Jim Mattson wrote:
> > > >>> From Intel's documention [1], "CPUID.(EAX=07H,ECX=0):EDX[26]
> > > >>> enumerates support for indirect branch restricted speculation (IBRS)
> > > >>> and the indirect branch predictor barrier (IBPB)." Further, from [2],
> > > >>> "Software that executed before the IBPB command cannot control the
> > > >>> predicted targets of indirect branches (4) executed after the command
> > > >>> on the same logical processor," where footnote 4 reads, "Note that
> > > >>> indirect branches include near call indirect, near jump indirect and
> > > >>> near return instructions. Because it includes near returns, it follows
> > > >>> that **RSB entries created before an IBPB command cannot control the
> > > >>> predicted targets of returns executed after the command on the same
> > > >>> logical processor.**" [emphasis mine]
> > > >>>
> > > >>> On the other hand, AMD's IBPB "may not prevent return branch
> > > >>> predictions from being specified by pre-IBPB branch targets" [3].
> > > >>>
> > > >>> However, some AMD processors have an "enhanced IBPB" [terminology
> > > >>> mine] which does clear the return address predictor. This feature is
> > > >>> enumerated by CPUID.80000008:EDX.IBPB_RET[bit 30] [4].
> > > >>>
> > > >>> Adjust the cross-vendor features enumerated by KVM_GET_SUPPORTED_CPUID
> > > >>> accordingly.
> > > >>>
> > > >>> [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/cpuid-enumeration-and-architectural-msrs.html
> > > >>> [2] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/speculative-execution-side-channel-mitigations.html#Footnotes
> > > >>> [3] https://www.amd.com/en/resources/product-security/bulletin/amd-sb-1040.html
> > > >>> [4] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24594.pdf
> > > >>>
> > > >>> Fixes: 0c54914d0c52 ("KVM: x86: use Intel speculation bugs and features as derived in generic x86 code")
> > > >>> Suggested-by: Venkatesh Srinivas <venkateshs@chromium.org>
> > > >>> Signed-off-by: Jim Mattson <jmattson@google.com>
> > > >>> ---
> > > >>>  arch/x86/kvm/cpuid.c | 6 +++++-
> > > >>>  1 file changed, 5 insertions(+), 1 deletion(-)
> > > >>>
> > > >>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > >>> index ec7b2ca3b4d3..c8d7d928ffc7 100644
> > > >>> --- a/arch/x86/kvm/cpuid.c
> > > >>> +++ b/arch/x86/kvm/cpuid.c
> > > >>> @@ -690,7 +690,9 @@ void kvm_set_cpu_caps(void)
> > > >>>     kvm_cpu_cap_set(X86_FEATURE_TSC_ADJUST);
> > > >>>     kvm_cpu_cap_set(X86_FEATURE_ARCH_CAPABILITIES);
> > > >>>
> > > >>> -   if (boot_cpu_has(X86_FEATURE_IBPB) && boot_cpu_has(X86_FEATURE_IBRS))
> > > >>> +   if (boot_cpu_has(X86_FEATURE_AMD_IBPB_RET) &&
> > > >>> +       boot_cpu_has(X86_FEATURE_AMD_IBPB) &&
> > > >>> +       boot_cpu_has(X86_FEATURE_AMD_IBRS))
> > > >>>             kvm_cpu_cap_set(X86_FEATURE_SPEC_CTRL);
> > > >>>     if (boot_cpu_has(X86_FEATURE_STIBP))
> > > >>>             kvm_cpu_cap_set(X86_FEATURE_INTEL_STIBP);
> > > >>> @@ -759,6 +761,8 @@ void kvm_set_cpu_caps(void)
> > > >>>      * arch/x86/kernel/cpu/bugs.c is kind enough to
> > > >>>      * record that in cpufeatures so use them.
> > > >>>      */
> > > >>> +   if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> > > >>> +           kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
> > > >>
> > > >> If SPEC_CTRL is set, then IBPB is set, so you can't have AMD_IBPB_RET
> > > >> without AMD_IBPB, but it just looks odd seeing them set with separate
> > > >> checks with no relationship dependency for AMD_IBPB_RET on AMD_IBPB.
> > > >> That's just me, though, not worth a v4 unless others feel the same.
> > > >
> > > > You thinking something like this (at the end, after the dust settles)?
> > > >
> > > >       if (WARN_ON_ONCE(kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB_RET) &&
> > > >                        !kvm_cpu_cap_has(X86_FEATURE_AMD_IBPB)))
> > > >               kvm_cpu_cap_clear(X86_FEATURE_AMD_IBPB_RET);
> > >
> > > I was just thinking more along the lines of:
> > >
> > >         if (boot_cpu_has(X86_FEATURE_IBPB)) {
> > >                 kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB);
> > >                 if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> > >                         kvm_cpu_cap_set(X86_FEATURE_AMD_IBPB_RET);
> > >         }
> >
> > AFAICT, there are just two reasons that X86_FEATURE_IBPB gets set:
> > 1. The CPU reports CPUID.(EAX=7,ECX=0):EDX[bit 26] (aka X86_FEATURE_SPEC_CTRL)
> > 2. The CPU reports CPUID Fn8000_0008_EBX[IBPB] (aka X86_FEATURE_AMD_IBPB)
> >
> > Clearly, in the second case, the KVM cpu capability for AMD_IBPB will
> > already be set, since it's specified in the mask for
> > CPUID_8000_0008_EBX.
> >
> > If this block of code is just trying to populate CPUID Fn8000_0008_EBX
> > on Intel processors, I'd rather change all of the predicates to test
> > for Intel features, rather than vendor-neutral features, so that the
> > derivation is clear. But maybe this block of code is also trying to
> > populate CPUID Fn8000_0008_EBX on AMD processors that may have some of
> > these features, but don't enumerate them via CPUID?
> 
> There's another argument for just nuking these cross-vendor
> derivations. How do we factor in CVE-2022-26373 (Post-barrier Return
> Stack Buffer Predictions)?
> Intel CPUs without IA32_ARCH_CAPABILITIES.PBRSB_NO[bit 24] have a
> weaker IBPB than AMD CPUs with CPUID Fn8000_0008_EBX[IBPB_RET], and
> probably should not be enumerating that CPUID bit.
> 
> Trying to derive cross-vendor mitigation equivalence is just going to
> end in tears.

Agreed, but I also don't want to break existing setups.  Is there a bare minimum
of sorts that we can advertise to userspace?  E.g. something that might be
imperfect, but has acceptable tradeoffs/risks for the existing code?

And then put a stake in the ground saying no more of these shenanigans.

Re: [PATCH v3 0/4] Distinguish between variants of IBPB

[Tom Lendacky]

On 8/23/24 13:53, Jim Mattson wrote:
> Prior to Zen4, AMD's IBPB did not flush the RAS (or, in Intel
> terminology, the RSB). Hence, the older version of AMD's IBPB was not
> equivalent to Intel's IBPB. However, KVM has been treating them as
> equivalent, synthesizing Intel's CPUID.(EAX=7,ECX=0):EDX[bit 26] on any
> platform that supports the synthetic features X86_FEATURE_IBPB and
> X86_FEATURE_IBRS.
> 
> Equivalence also requires a previously ignored feature on the AMD side,
> CPUID Fn8000_0008_EBX[IBPB_RET], which is enumerated on Zen4.
> 
> v3: Pass through IBPB_RET from hardware to userspace. [Tom]
>     Derive AMD_IBPB from X86_FEATURE_SPEC_CTRL rather than
>     X86_FEATURE_IBPB. [Tom]
>     Clarify semantics of X86_FEATURE_IBPB.
> 
> v2: Use IBPB_RET to identify semantic equality. [Venkatesh]
> 
> Jim Mattson (4):
>   x86/cpufeatures: Clarify semantics of X86_FEATURE_IBPB
>   x86/cpufeatures: Define X86_FEATURE_AMD_IBPB_RET
>   KVM: x86: Advertise AMD_IBPB_RET to userspace
>   KVM: x86: AMD's IBPB is not equivalent to Intel's IBPB
> 
>  arch/x86/include/asm/cpufeatures.h | 3 ++-
>  arch/x86/kvm/cpuid.c               | 8 ++++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)

For the series:

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> 

Re: [PATCH v3 0/4] Distinguish between variants of IBPB

[Thomas Gleixner]

On Fri, Aug 23 2024 at 11:53, Jim Mattson wrote:

> Prior to Zen4, AMD's IBPB did not flush the RAS (or, in Intel
> terminology, the RSB). Hence, the older version of AMD's IBPB was not
> equivalent to Intel's IBPB. However, KVM has been treating them as
> equivalent, synthesizing Intel's CPUID.(EAX=7,ECX=0):EDX[bit 26] on any
> platform that supports the synthetic features X86_FEATURE_IBPB and
> X86_FEATURE_IBRS.
>
> Equivalence also requires a previously ignored feature on the AMD side,
> CPUID Fn8000_0008_EBX[IBPB_RET], which is enumerated on Zen4.
>
> v3: Pass through IBPB_RET from hardware to userspace. [Tom]
>     Derive AMD_IBPB from X86_FEATURE_SPEC_CTRL rather than
>     X86_FEATURE_IBPB. [Tom]
>     Clarify semantics of X86_FEATURE_IBPB.
>
> v2: Use IBPB_RET to identify semantic equality. [Venkatesh]

Assuming this goes through the KVM tree:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>