Re: [PATCH][x86_64] Disable TSX on some Haswell processors

[Carlos O'Donell <carlos at redhat dot com>]

On 12/13/2016 01:48 PM, Andrew Senkevich wrote:
> Hi,
> 
> this is a workaround which disables TSX on some Haswell processors to
> avoid TSX on kernel's that weren't updated with the latest microcode
> package (which is needed to disable TSX by default due to errata).
> Is this patch Ok?

Andrew,

Thank you for putting this patch together.

One serious question: What about Broadwell?

> 2016-12-13  Andrew Senkevich  <andrew.senkevich@intel.com>
> 
>         * sysdeps/x86/cpu-features.c (get_common_indeces): Add
> stepping identification.
>         (init_cpu_features): Add handle of Haswell.
> 
> diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
> index e228a76..a903390 100644
> --- a/sysdeps/x86/cpu-features.c
> +++ b/sysdeps/x86/cpu-features.c
> @@ -22,7 +22,7 @@
>  static void
>  get_common_indeces (struct cpu_features *cpu_features,
>      unsigned int *family, unsigned int *model,
> -    unsigned int *extended_model)
> +    unsigned int *extended_model, unsigned int *stepping)

OK.

>  {
>    if (family)
>      {
> @@ -34,6 +34,7 @@ get_common_indeces (struct cpu_features *cpu_features,
>        *family = (eax >> 8) & 0x0f;
>        *model = (eax >> 4) & 0x0f;
>        *extended_model = (eax >> 12) & 0xf0;
> +      *stepping = eax & 0x0f;

OK.

>        if (*family == 0x0f)
>   {
>    *family += (eax >> 20) & 0xff;
> @@ -116,11 +117,12 @@ init_cpu_features (struct cpu_features *cpu_features)
>    /* This spells out "GenuineIntel".  */
>    if (ebx == 0x756e6547 && ecx == 0x6c65746e && edx == 0x49656e69)
>      {
> -      unsigned int extended_model;
> +      unsigned int extended_model, stepping;

OK

> 
>        kind = arch_kind_intel;
> 
> -      get_common_indeces (cpu_features, &family, &model, &extended_model);
> +      get_common_indeces (cpu_features, &family, &model, &extended_model,
> +  &stepping);

OK.

> 
>        if (family == 0x06)
>   {
> @@ -201,6 +203,19 @@ init_cpu_features (struct cpu_features *cpu_features)
>      | bit_arch_Fast_Unaligned_Copy
>      | bit_arch_Prefer_PMINUB_for_stringop);
>        break;
> +

Case 0x3f needs a comment explaining why we break out early.

> +    case 0x3f:
> +      if (stepping >= 4)
> + break;
>
> +    case 0x3c:
> +    case 0x45:
> +    case 0x46:
> +      /* Disable Intel TSX on Haswell processors (except Xeon E7 v3)
> + to avoid TSX on kernel's that weren't updated with the latest
> + microcode package (which disables broken feature
> + by default).  */
> +      cpu_features->cpuid[COMMON_CPUID_INDEX_7].ebx &= ~(bit_cpu_RTM);
> +      break;

You mention only Haswell, but public announcements claimed that early
Broadwell-Y silicon also had the same problem?

Have you ruled this out or identified that those early Broadwell parts
always shiped with microcode that disabled RTM?

>      }
>   }
> 
> @@ -227,11 +242,12 @@ init_cpu_features (struct cpu_features *cpu_features)
>    /* This spells out "AuthenticAMD".  */
>    else if (ebx == 0x68747541 && ecx == 0x444d4163 && edx == 0x69746e65)
>      {
> -      unsigned int extended_model;
> +      unsigned int extended_model, stepping;

OK.

> 
>        kind = arch_kind_amd;
> 
> -      get_common_indeces (cpu_features, &family, &model, &extended_model);
> +      get_common_indeces (cpu_features, &family, &model, &extended_model,
> +  &stepping);

OK.

> 
>        ecx = cpu_features->cpuid[COMMON_CPUID_INDEX_1].ecx;
> 
> @@ -268,7 +284,7 @@ init_cpu_features (struct cpu_features *cpu_features)
>    else
>      {
>        kind = arch_kind_other;
> -      get_common_indeces (cpu_features, NULL, NULL, NULL);
> +      get_common_indeces (cpu_features, NULL, NULL, NULL, NULL);

OK.

>      }
> 
>    /* Support i586 if CX8 is available.  */
> 
> 
> 
> --
> WBR,
> Andrew
> 


-- 
Cheers,
Carlos.