x86: Fix tst-cpu-features-cpuinfo on Ryzen 9 (BZ #27873)

Message ID 20210623145419.3025540-1-adhemerval.zanella@linaro.org
State Superseded
Headers
Series x86: Fix tst-cpu-features-cpuinfo on Ryzen 9 (BZ #27873) |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella Netto June 23, 2021, 2:54 p.m. UTC
  AMD define different flags for IRPB, IBRS, and STIPBP [1], so new
x86_64_cpu are added and IBRS_IBPB is only tested for Intel.

The SHSTK is also a opt-in feature, Ryzen 9 does support but the
kernel might not enable it.

The SSDB is also defined and implemented different on AMD [2],
and also a new AMD_SSDB flag is added.  It should map to the
cpuinfo 'ssdb' on recent AMD cpus.

It fixes tst-cpu-features-cpuinfo and tst-cpu-features-cpuinfo-static
on recent AMD cpus.

Checked on x86_64-linux-gnu on AMD Ryzen 9 5900X.

[1] https://developer.amd.com/wp-content/resources/Architecture_Guidelines_Update_Indirect_Branch_Control.pdf
[2] https://bugzilla.kernel.org/show_bug.cgi?id=199889
---
 sysdeps/x86/bits/platform/x86.h        |  4 +++
 sysdeps/x86/include/cpu-features.h     | 12 +++++++++
 sysdeps/x86/tst-cpu-features-cpuinfo.c | 35 ++++++++++++++++++++------
 3 files changed, 43 insertions(+), 8 deletions(-)
  

Comments

H.J. Lu June 23, 2021, 8:41 p.m. UTC | #1
On Wed, Jun 23, 2021 at 7:54 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
> AMD define different flags for IRPB, IBRS, and STIPBP [1], so new
> x86_64_cpu are added and IBRS_IBPB is only tested for Intel.
>
> The SHSTK is also a opt-in feature, Ryzen 9 does support but the
> kernel might not enable it.
>
> The SSDB is also defined and implemented different on AMD [2],
> and also a new AMD_SSDB flag is added.  It should map to the
> cpuinfo 'ssdb' on recent AMD cpus.
>
> It fixes tst-cpu-features-cpuinfo and tst-cpu-features-cpuinfo-static
> on recent AMD cpus.
>
> Checked on x86_64-linux-gnu on AMD Ryzen 9 5900X.
>
> [1] https://developer.amd.com/wp-content/resources/Architecture_Guidelines_Update_Indirect_Branch_Control.pdf
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=199889
> ---
>  sysdeps/x86/bits/platform/x86.h        |  4 +++
>  sysdeps/x86/include/cpu-features.h     | 12 +++++++++
>  sysdeps/x86/tst-cpu-features-cpuinfo.c | 35 ++++++++++++++++++++------
>  3 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/sysdeps/x86/bits/platform/x86.h b/sysdeps/x86/bits/platform/x86.h
> index fe08d8a1b6..26e3b67ede 100644
> --- a/sysdeps/x86/bits/platform/x86.h
> +++ b/sysdeps/x86/bits/platform/x86.h
> @@ -278,6 +278,10 @@ enum
>         + cpuid_register_index_ebx * 8 * sizeof (unsigned int)),
>
>    x86_cpu_WBNOINVD             = x86_cpu_index_80000008_ebx + 9,
> +  x86_cpu_AMD_IBPB             = x86_cpu_index_80000008_ebx + 12,
> +  x86_cpu_AMD_IBRS             = x86_cpu_index_80000008_ebx + 14,
> +  x86_cpu_AMD_STIBP            = x86_cpu_index_80000008_ebx + 15,
> +  x86_cpu_AMD_SSBD             = x86_cpu_index_80000008_ebx + 24,
>
>    x86_cpu_index_7_ecx_1_eax
>      = (CPUID_INDEX_7_ECX_1 * 8 * 4 * sizeof (unsigned int)
> diff --git a/sysdeps/x86/include/cpu-features.h b/sysdeps/x86/include/cpu-features.h
> index d042a2ebef..4f1c4ee402 100644
> --- a/sysdeps/x86/include/cpu-features.h
> +++ b/sysdeps/x86/include/cpu-features.h
> @@ -289,6 +289,10 @@ enum
>
>  /* EBX.  */
>  #define bit_cpu_WBNOINVD       (1u << 9)
> +#define bit_cpu_AMD_IBPB       (1u << 12)
> +#define bit_cpu_AMD_IBRS       (1u << 14)
> +#define bit_cpu_AMD_STIBP      (1u << 15)
> +#define bit_cpu_AMD_SSBD       (1u << 24)
>
>  /* CPUID_INDEX_7_ECX_1.  */
>
> @@ -519,6 +523,10 @@ enum
>
>  /* EBX.  */
>  #define index_cpu_WBNOINVD     CPUID_INDEX_80000008
> +#define index_cpu_AMD_IBPB     CPUID_INDEX_80000008
> +#define index_cpu_AMD_IBRS     CPUID_INDEX_80000008
> +#define index_cpu_AMD_STIBP    CPUID_INDEX_80000008
> +#define index_cpu_AMD_SSBD     CPUID_INDEX_80000008
>
>  /* CPUID_INDEX_7_ECX_1.  */
>
> @@ -749,6 +757,10 @@ enum
>
>  /* EBX.  */
>  #define reg_WBNOINVD           ebx
> +#define reg_AMD_IBPB           ebx
> +#define reg_AMD_IBRS           ebx
> +#define reg_AMD_STIBP          ebx
> +#define reg_AMD_SSBD           ebx
>
>  /* CPUID_INDEX_7_ECX_1.  */
>
> diff --git a/sysdeps/x86/tst-cpu-features-cpuinfo.c b/sysdeps/x86/tst-cpu-features-cpuinfo.c
> index 75e7eb9352..6535fe50e8 100644
> --- a/sysdeps/x86/tst-cpu-features-cpuinfo.c
> +++ b/sysdeps/x86/tst-cpu-features-cpuinfo.c
> @@ -16,10 +16,11 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>
> -#include <sys/platform/x86.h>
> +#include <cpu-features.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <stdbool.h>
>
>  static char *cpu_flags;
>
> @@ -54,7 +55,7 @@ get_cpuinfo (void)
>
>  int
>  check_proc (const char *proc_name, const char *search_name, int flag,
> -           int usable, const char *name)
> +           int usable, const char *name, bool optin)
>  {
>    int found = 0;
>
> @@ -78,7 +79,7 @@ check_proc (const char *proc_name, const char *search_name, int flag,
>
>    if (found != flag)
>      {
> -      if (found || usable)
> +      if (!optin && (found || usable))
>         printf (" *** failure ***\n");
>        else
>         {
> @@ -93,12 +94,18 @@ check_proc (const char *proc_name, const char *search_name, int flag,
>  #define CHECK_PROC(str, name) \
>    check_proc (#str, " "#str" ", HAS_CPU_FEATURE (name), \
>               CPU_FEATURE_USABLE (name), \
> -             "HAS_CPU_FEATURE (" #name ")")
> +             "HAS_CPU_FEATURE (" #name ")", false)
> +
> +#define CHECK_PROC_OPTIN(str, name) \
> +  check_proc (#str, " "#str" ", HAS_CPU_FEATURE (name), \
> +             CPU_FEATURE_USABLE (name), \
> +             "HAS_CPU_FEATURE (" #name ")", true)
>
>  static int
>  do_test (int argc, char **argv)
>  {
>    int fails = 0;
> +  const struct cpu_features *cpu_features = __get_cpu_features ();
>
>    get_cpuinfo ();
>    fails += CHECK_PROC (acpi, ACPI);
> @@ -159,7 +166,17 @@ do_test (int argc, char **argv)
>    fails += CHECK_PROC (hle, HLE);
>    fails += CHECK_PROC (ht, HTT);
>    fails += CHECK_PROC (hybrid, HYBRID);
> -  fails += CHECK_PROC (ibrs, IBRS_IBPB);
> +  if (cpu_features->basic.kind == arch_kind_intel)
> +    {
> +      fails += CHECK_PROC (ibrs, IBRS_IBPB);
> +      fails += CHECK_PROC (stibp, STIBP);
> +    }
> +  else if (cpu_features->basic.kind == arch_kind_amd)
> +    {
> +      fails += CHECK_PROC (ibpb, AMD_IBPB);
> +      fails += CHECK_PROC (ibrs, AMD_IBRS);
> +      fails += CHECK_PROC (stibp, AMD_STIBP);
> +    }
>    fails += CHECK_PROC (ibt, IBT);
>    fails += CHECK_PROC (invariant_tsc, INVARIANT_TSC);
>    fails += CHECK_PROC (invpcid, INVPCID);
> @@ -216,12 +233,15 @@ do_test (int argc, char **argv)
>    fails += CHECK_PROC (sgx, SGX);
>    fails += CHECK_PROC (sgx_lc, SGX_LC);
>    fails += CHECK_PROC (sha_ni, SHA);
> -  fails += CHECK_PROC (shstk, SHSTK);
> +  fails += CHECK_PROC_OPTIN (shstk, SHSTK);

Why do you need this?  If kernel doesn't support SHSTK, it will be
turned off:

 /* Check CET status.  */
  unsigned int cet_status = get_cet_status ();

  if ((cet_status & GNU_PROPERTY_X86_FEATURE_1_IBT) == 0)
    CPU_FEATURE_UNSET (cpu_features, IBT)
  if ((cet_status & GNU_PROPERTY_X86_FEATURE_1_SHSTK) == 0)
    CPU_FEATURE_UNSET (cpu_features, SHSTK)

>    fails += CHECK_PROC (smap, SMAP);
>    fails += CHECK_PROC (smep, SMEP);
>    fails += CHECK_PROC (smx, SMX);
>    fails += CHECK_PROC (ss, SS);
> -  fails += CHECK_PROC (ssbd, SSBD);
> +  if (cpu_features->basic.kind == arch_kind_intel)
> +    fails += CHECK_PROC (ssbd, SSBD);
> +  else if (cpu_features->basic.kind == arch_kind_amd)
> +    fails += CHECK_PROC (ssbd, AMD_SSBD);
>    fails += CHECK_PROC (sse, SSE);
>    fails += CHECK_PROC (sse2, SSE2);
>    fails += CHECK_PROC (pni, SSE3);
> @@ -229,7 +249,6 @@ do_test (int argc, char **argv)
>    fails += CHECK_PROC (sse4_2, SSE4_2);
>    fails += CHECK_PROC (sse4a, SSE4A);
>    fails += CHECK_PROC (ssse3, SSSE3);
> -  fails += CHECK_PROC (stibp, STIBP);
>    fails += CHECK_PROC (svm, SVM);
>  #ifdef __x86_64__
>    /* NB: SYSCALL_SYSRET is 64-bit only.  */
> --
> 2.30.2
>
  
Adhemerval Zanella Netto June 23, 2021, 9:15 p.m. UTC | #2
On 23/06/2021 17:41, H.J. Lu wrote:
>> @@ -216,12 +233,15 @@ do_test (int argc, char **argv)
>>    fails += CHECK_PROC (sgx, SGX);
>>    fails += CHECK_PROC (sgx_lc, SGX_LC);
>>    fails += CHECK_PROC (sha_ni, SHA);
>> -  fails += CHECK_PROC (shstk, SHSTK);
>> +  fails += CHECK_PROC_OPTIN (shstk, SHSTK);
> 
> Why do you need this?  If kernel doesn't support SHSTK, it will be
> turned off:
> 
>  /* Check CET status.  */
>   unsigned int cet_status = get_cet_status ();
> 
>   if ((cet_status & GNU_PROPERTY_X86_FEATURE_1_IBT) == 0)
>     CPU_FEATURE_UNSET (cpu_features, IBT)
>   if ((cet_status & GNU_PROPERTY_X86_FEATURE_1_SHSTK) == 0)
>     CPU_FEATURE_UNSET (cpu_features, SHSTK)

The problem is this is only enabled for CET_ENABLED, the configuration I am using
does not define __CET__.  So the CPU I am using does support SHSTK, but the bit
ended up not being cleared by glibc.
  

Patch

diff --git a/sysdeps/x86/bits/platform/x86.h b/sysdeps/x86/bits/platform/x86.h
index fe08d8a1b6..26e3b67ede 100644
--- a/sysdeps/x86/bits/platform/x86.h
+++ b/sysdeps/x86/bits/platform/x86.h
@@ -278,6 +278,10 @@  enum
        + cpuid_register_index_ebx * 8 * sizeof (unsigned int)),
 
   x86_cpu_WBNOINVD		= x86_cpu_index_80000008_ebx + 9,
+  x86_cpu_AMD_IBPB	        = x86_cpu_index_80000008_ebx + 12,
+  x86_cpu_AMD_IBRS	        = x86_cpu_index_80000008_ebx + 14,
+  x86_cpu_AMD_STIBP	        = x86_cpu_index_80000008_ebx + 15,
+  x86_cpu_AMD_SSBD	        = x86_cpu_index_80000008_ebx + 24,
 
   x86_cpu_index_7_ecx_1_eax
     = (CPUID_INDEX_7_ECX_1 * 8 * 4 * sizeof (unsigned int)
diff --git a/sysdeps/x86/include/cpu-features.h b/sysdeps/x86/include/cpu-features.h
index d042a2ebef..4f1c4ee402 100644
--- a/sysdeps/x86/include/cpu-features.h
+++ b/sysdeps/x86/include/cpu-features.h
@@ -289,6 +289,10 @@  enum
 
 /* EBX.  */
 #define bit_cpu_WBNOINVD	(1u << 9)
+#define bit_cpu_AMD_IBPB	(1u << 12)
+#define bit_cpu_AMD_IBRS	(1u << 14)
+#define bit_cpu_AMD_STIBP	(1u << 15)
+#define bit_cpu_AMD_SSBD	(1u << 24)
 
 /* CPUID_INDEX_7_ECX_1.  */
 
@@ -519,6 +523,10 @@  enum
 
 /* EBX.  */
 #define index_cpu_WBNOINVD	CPUID_INDEX_80000008
+#define index_cpu_AMD_IBPB	CPUID_INDEX_80000008
+#define index_cpu_AMD_IBRS	CPUID_INDEX_80000008
+#define index_cpu_AMD_STIBP	CPUID_INDEX_80000008
+#define index_cpu_AMD_SSBD	CPUID_INDEX_80000008
 
 /* CPUID_INDEX_7_ECX_1.  */
 
@@ -749,6 +757,10 @@  enum
 
 /* EBX.  */
 #define reg_WBNOINVD		ebx
+#define reg_AMD_IBPB		ebx
+#define reg_AMD_IBRS		ebx
+#define reg_AMD_STIBP		ebx
+#define reg_AMD_SSBD		ebx
 
 /* CPUID_INDEX_7_ECX_1.  */
 
diff --git a/sysdeps/x86/tst-cpu-features-cpuinfo.c b/sysdeps/x86/tst-cpu-features-cpuinfo.c
index 75e7eb9352..6535fe50e8 100644
--- a/sysdeps/x86/tst-cpu-features-cpuinfo.c
+++ b/sysdeps/x86/tst-cpu-features-cpuinfo.c
@@ -16,10 +16,11 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <sys/platform/x86.h>
+#include <cpu-features.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <stdbool.h>
 
 static char *cpu_flags;
 
@@ -54,7 +55,7 @@  get_cpuinfo (void)
 
 int
 check_proc (const char *proc_name, const char *search_name, int flag,
-	    int usable, const char *name)
+	    int usable, const char *name, bool optin)
 {
   int found = 0;
 
@@ -78,7 +79,7 @@  check_proc (const char *proc_name, const char *search_name, int flag,
 
   if (found != flag)
     {
-      if (found || usable)
+      if (!optin && (found || usable))
 	printf (" *** failure ***\n");
       else
 	{
@@ -93,12 +94,18 @@  check_proc (const char *proc_name, const char *search_name, int flag,
 #define CHECK_PROC(str, name) \
   check_proc (#str, " "#str" ", HAS_CPU_FEATURE (name), \
 	      CPU_FEATURE_USABLE (name), \
-	      "HAS_CPU_FEATURE (" #name ")")
+	      "HAS_CPU_FEATURE (" #name ")", false)
+
+#define CHECK_PROC_OPTIN(str, name) \
+  check_proc (#str, " "#str" ", HAS_CPU_FEATURE (name), \
+	      CPU_FEATURE_USABLE (name), \
+	      "HAS_CPU_FEATURE (" #name ")", true)
 
 static int
 do_test (int argc, char **argv)
 {
   int fails = 0;
+  const struct cpu_features *cpu_features = __get_cpu_features ();
 
   get_cpuinfo ();
   fails += CHECK_PROC (acpi, ACPI);
@@ -159,7 +166,17 @@  do_test (int argc, char **argv)
   fails += CHECK_PROC (hle, HLE);
   fails += CHECK_PROC (ht, HTT);
   fails += CHECK_PROC (hybrid, HYBRID);
-  fails += CHECK_PROC (ibrs, IBRS_IBPB);
+  if (cpu_features->basic.kind == arch_kind_intel)
+    {
+      fails += CHECK_PROC (ibrs, IBRS_IBPB);
+      fails += CHECK_PROC (stibp, STIBP);
+    }
+  else if (cpu_features->basic.kind == arch_kind_amd)
+    {
+      fails += CHECK_PROC (ibpb, AMD_IBPB);
+      fails += CHECK_PROC (ibrs, AMD_IBRS);
+      fails += CHECK_PROC (stibp, AMD_STIBP);
+    }
   fails += CHECK_PROC (ibt, IBT);
   fails += CHECK_PROC (invariant_tsc, INVARIANT_TSC);
   fails += CHECK_PROC (invpcid, INVPCID);
@@ -216,12 +233,15 @@  do_test (int argc, char **argv)
   fails += CHECK_PROC (sgx, SGX);
   fails += CHECK_PROC (sgx_lc, SGX_LC);
   fails += CHECK_PROC (sha_ni, SHA);
-  fails += CHECK_PROC (shstk, SHSTK);
+  fails += CHECK_PROC_OPTIN (shstk, SHSTK);
   fails += CHECK_PROC (smap, SMAP);
   fails += CHECK_PROC (smep, SMEP);
   fails += CHECK_PROC (smx, SMX);
   fails += CHECK_PROC (ss, SS);
-  fails += CHECK_PROC (ssbd, SSBD);
+  if (cpu_features->basic.kind == arch_kind_intel)
+    fails += CHECK_PROC (ssbd, SSBD);
+  else if (cpu_features->basic.kind == arch_kind_amd)
+    fails += CHECK_PROC (ssbd, AMD_SSBD);
   fails += CHECK_PROC (sse, SSE);
   fails += CHECK_PROC (sse2, SSE2);
   fails += CHECK_PROC (pni, SSE3);
@@ -229,7 +249,6 @@  do_test (int argc, char **argv)
   fails += CHECK_PROC (sse4_2, SSE4_2);
   fails += CHECK_PROC (sse4a, SSE4A);
   fails += CHECK_PROC (ssse3, SSSE3);
-  fails += CHECK_PROC (stibp, STIBP);
   fails += CHECK_PROC (svm, SVM);
 #ifdef __x86_64__
   /* NB: SYSCALL_SYSRET is 64-bit only.  */