[PATCH 0/3] x86/Intel: family 0xF tidying

[PATCH 0/3] x86/Intel: family 0xF tidying

[Jan Beulich]

While the first two patches deal with a long standing annoyance,
the last one covers yet another bit of information we can retrieve
and log, for reference with / comparison to other logged data.

1: skip PLATFORM_INFO reads on family 0xf
2: skip CORE_THREAD_COUNT read on family 0xf
3: also display CPU freq for family 0xf

Jan

[PATCH 1/3] x86/Intel: skip PLATFORM_INFO reads on family 0xf

[Jan Beulich]

This avoids unnecessary (and always somewhat scary) log messages for the
recovered from #GP(0).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Perhaps even use "!= 6" in at least the CPUID-faulting family check?

--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -127,9 +127,12 @@ bool __init probe_cpuid_faulting(void)
 
 	/*
 	 * Don't bother looking for CPUID faulting if we aren't virtualised on
-	 * AMD or Hygon hardware - it won't be present.
+	 * AMD or Hygon hardware - it won't be present.  Likewise for Fam0F
+	 * Intel hardware.
 	 */
-	if ((boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) &&
+	if (((boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ||
+	     ((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
+	      boot_cpu_data.x86 == 0xf)) &&
 	    !cpu_has_hypervisor)
 		return false;
 
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -455,7 +455,7 @@ static void intel_log_freq(const struct
         }
     }
 
-    if ( rdmsr_safe(MSR_INTEL_PLATFORM_INFO, msrval) )
+    if ( c->x86 == 0xf || rdmsr_safe(MSR_INTEL_PLATFORM_INFO, msrval) )
         return;
     max_ratio = msrval >> 8;
 

Re: [PATCH 1/3] x86/Intel: skip PLATFORM_INFO reads on family 0xf

[Roger Pau Monné]

On Thu, Feb 10, 2022 at 03:55:52PM +0100, Jan Beulich wrote:
> This avoids unnecessary (and always somewhat scary) log messages for the
> recovered from #GP(0).

Could we maybe get rid of the #GP messages for cases like this where we
are explicitly probing for MSR presence? (ie: it's expected that we
can get a #GP)

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Perhaps even use "!= 6" in at least the CPUID-faulting family check?

Likely, or else you would also need to check for family 11 (Knights
Corner?) which doesn't seem to support PLATFORM_INFO either.

Thanks, Roger.

Re: [PATCH 1/3] x86/Intel: skip PLATFORM_INFO reads on family 0xf

[Jan Beulich]

On 11.02.2022 10:40, Roger Pau Monné wrote:
> On Thu, Feb 10, 2022 at 03:55:52PM +0100, Jan Beulich wrote:
>> This avoids unnecessary (and always somewhat scary) log messages for the
>> recovered from #GP(0).
> 
> Could we maybe get rid of the #GP messages for cases like this where we
> are explicitly probing for MSR presence? (ie: it's expected that we
> can get a #GP)

This would mean some form of annotation of such RDMSR attempts (for
the recovery code to recognize in order to skip the printk()). Not
all rdmsr_safe() uses are, strictly speaking, probes, so I wouldn't
want to put such in rdmsr_safe() itself.

In any event - quite a bit more work. Plus I'm not convinced it's a
good idea to suppress any such log messages.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Perhaps even use "!= 6" in at least the CPUID-faulting family check?
> 
> Likely, or else you would also need to check for family 11 (Knights
> Corner?) which doesn't seem to support PLATFORM_INFO either.

I don't think Xen is able to run on these (likewise for IA64, which
iirc were surfacing as x86 model 7)? These are the co-processor ones,
aren't they? My question was more towards whether we would (wrongly)
exclude future processors when using != 6, if Intel decided to ever
make new CPUs with a family other than 6.

Jan

Re: [PATCH 1/3] x86/Intel: skip PLATFORM_INFO reads on family 0xf

[Roger Pau Monné]

On Fri, Feb 11, 2022 at 10:59:10AM +0100, Jan Beulich wrote:
> On 11.02.2022 10:40, Roger Pau Monné wrote:
> > On Thu, Feb 10, 2022 at 03:55:52PM +0100, Jan Beulich wrote:
> >> This avoids unnecessary (and always somewhat scary) log messages for the
> >> recovered from #GP(0).
> > 
> > Could we maybe get rid of the #GP messages for cases like this where we
> > are explicitly probing for MSR presence? (ie: it's expected that we
> > can get a #GP)
> 
> This would mean some form of annotation of such RDMSR attempts (for
> the recovery code to recognize in order to skip the printk()). Not
> all rdmsr_safe() uses are, strictly speaking, probes, so I wouldn't
> want to put such in rdmsr_safe() itself.
> 
> In any event - quite a bit more work. Plus I'm not convinced it's a
> good idea to suppress any such log messages.
> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> >> ---
> >> Perhaps even use "!= 6" in at least the CPUID-faulting family check?
> > 
> > Likely, or else you would also need to check for family 11 (Knights
> > Corner?) which doesn't seem to support PLATFORM_INFO either.
> 
> I don't think Xen is able to run on these (likewise for IA64, which
> iirc were surfacing as x86 model 7)? These are the co-processor ones,
> aren't they?

Right, Knights Corner uses a socket mount but it's still a
co-processor. It was Knights Landing the first one that could be used
as a host processor.

> My question was more towards whether we would (wrongly)
> exclude future processors when using != 6, if Intel decided to ever
> make new CPUs with a family other than 6.

In the case here I think we should only avoid the probe for family
0xf. Newer families (or even models on family 6 not supporting
PLATFORM_INFO) will just get a #GP message which is OK I think, we
could fix that in due time.

It's better to get a #GP message for probing than to just skip
detection of CPUID faulting on unknown newer families IMO.

Thanks, Roger.

[PATCH 2/3] x86/Intel: skip CORE_THREAD_COUNT read on family 0xf

[Jan Beulich]

This avoids an unnecessary (and always somewhat scary) log message for
the recovered from #GP(0).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Perhaps even use "== 6" in the family check?

--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -453,7 +453,8 @@ static bool __init check_smt_enabled(voi
      * At the time of writing, it is almost completely undocumented, so isn't
      * virtualised reliably.
      */
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL && !cpu_has_hypervisor &&
+    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
+         boot_cpu_data.x86 != 0xf && !cpu_has_hypervisor &&
          !rdmsr_safe(MSR_INTEL_CORE_THREAD_COUNT, val) )
         return (MASK_EXTR(val, MSR_CTC_CORE_MASK) !=
                 MASK_EXTR(val, MSR_CTC_THREAD_MASK));

Re: [PATCH 2/3] x86/Intel: skip CORE_THREAD_COUNT read on family 0xf

[Roger Pau Monné]

On Thu, Feb 10, 2022 at 03:56:12PM +0100, Jan Beulich wrote:
> This avoids an unnecessary (and always somewhat scary) log message for
> the recovered from #GP(0).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> Perhaps even use "== 6" in the family check?

I think it's best as is. Even on family 6 this seems to be supported
only on model 3f? (Haswell Xeon E5 v3 and E7 v3?)

The comment also seems to note this is mostly undocumented.

Thanks, Roger.

Re: [PATCH 2/3] x86/Intel: skip CORE_THREAD_COUNT read on family 0xf

[Jan Beulich]

On 11.02.2022 11:58, Roger Pau Monné wrote:
> On Thu, Feb 10, 2022 at 03:56:12PM +0100, Jan Beulich wrote:
>> This avoids an unnecessary (and always somewhat scary) log message for
>> the recovered from #GP(0).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> Perhaps even use "== 6" in the family check?
> 
> I think it's best as is. Even on family 6 this seems to be supported
> only on model 3f? (Haswell Xeon E5 v3 and E7 v3?)

Well, ...

> The comment also seems to note this is mostly undocumented.

... this same comment says "Nehalem and later". And this also
matches my observations (and, given he has written the comment, quite
clearly Andrew's as well), no matter what the (notoriously incomplete)
SDM Vol 4 may say.

Jan

[PATCH 3/3] x86/Intel: also display CPU freq for family 0xf

[Jan Beulich]

Actually we can do better than simply bailing for there not being any
PLATFORM_INFO MSR on these. The "max" part of the information is
available in another MSR, alongside the scaling factor (which is
encoded in similar ways to Core/Core2, and hence the decoding table can
be shared).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The inner switch() is left indented one level too much (and with an
extra pair of braces) to limit the diff. I'd prefer to make a follow-up
patch reducing the indentation, unless I'm told to do so right here.

--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -412,9 +412,9 @@ static int num_cpu_cores(struct cpuinfo_
 
 static void intel_log_freq(const struct cpuinfo_x86 *c)
 {
-    unsigned int eax, ebx, ecx, edx;
+    unsigned int eax, ebx, ecx, edx, factor;
     uint64_t msrval;
-    uint8_t max_ratio;
+    uint8_t max_ratio, min_ratio;
 
     if ( c->cpuid_level >= 0x15 )
     {
@@ -455,21 +455,22 @@ static void intel_log_freq(const struct
         }
     }
 
-    if ( c->x86 == 0xf || rdmsr_safe(MSR_INTEL_PLATFORM_INFO, msrval) )
-        return;
-    max_ratio = msrval >> 8;
-
-    if ( max_ratio )
+    switch ( c->x86 )
     {
-        unsigned int factor = 10000;
-        uint8_t min_ratio = msrval >> 40;
+        static const unsigned short core_factors[] =
+            { 26667, 13333, 20000, 16667, 33333, 10000, 40000 };
+
+    case 6:
+        if ( rdmsr_safe(MSR_INTEL_PLATFORM_INFO, msrval) )
+            return;
+        max_ratio = msrval >> 8;
+        min_ratio = msrval >> 40;
+        if ( !max_ratio )
+            return;
 
-        if ( c->x86 == 6 )
+        {
             switch ( c->x86_model )
             {
-                static const unsigned short core_factors[] =
-                    { 26667, 13333, 20000, 16667, 33333, 10000, 40000 };
-
             case 0x0e: /* Core */
             case 0x0f: case 0x16: case 0x17: case 0x1d: /* Core2 */
                 /*
@@ -491,13 +492,33 @@ static void intel_log_freq(const struct
             case 0x25: case 0x2c: case 0x2f: /* Westmere */
                 factor = 13333;
                 break;
+
+            default:
+                factor = 10000;
+                break;
             }
+        }
+        break;
+
+    case 0xf:
+        if ( rdmsr_safe(MSR_IA32_EBC_FREQUENCY_ID, msrval) )
+            return;
+        max_ratio = msrval >> 24;
+        min_ratio = 0;
+        msrval >>= 16;
+        if ( (msrval &= 7) > 4 )
+            return;
+        factor = core_factors[msrval];
+        break;
 
-        printk("CPU%u: ", smp_processor_id());
-        if ( min_ratio )
-            printk("%u ... ", (factor * min_ratio + 50) / 100);
-        printk("%u MHz\n", (factor * max_ratio + 50) / 100);
+    default:
+        return;
     }
+
+    printk("CPU%u: ", smp_processor_id());
+    if ( min_ratio )
+        printk("%u ... ", (factor * min_ratio + 50) / 100);
+    printk("%u MHz\n", (factor * max_ratio + 50) / 100);
 }
 
 static void init_intel(struct cpuinfo_x86 *c)

Re: [PATCH 3/3] x86/Intel: also display CPU freq for family 0xf

[Roger Pau Monné]

On Thu, Feb 10, 2022 at 03:56:48PM +0100, Jan Beulich wrote:
> Actually we can do better than simply bailing for there not being any
> PLATFORM_INFO MSR on these. The "max" part of the information is
> available in another MSR, alongside the scaling factor (which is
> encoded in similar ways to Core/Core2, and hence the decoding table can
> be shared).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> The inner switch() is left indented one level too much (and with an
> extra pair of braces) to limit the diff. I'd prefer to make a follow-up
> patch reducing the indentation, unless I'm told to do so right here.

I'm fine with a followup patch.

> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -412,9 +412,9 @@ static int num_cpu_cores(struct cpuinfo_
>  
>  static void intel_log_freq(const struct cpuinfo_x86 *c)
>  {
> -    unsigned int eax, ebx, ecx, edx;
> +    unsigned int eax, ebx, ecx, edx, factor;
>      uint64_t msrval;
> -    uint8_t max_ratio;
> +    uint8_t max_ratio, min_ratio;
>  
>      if ( c->cpuid_level >= 0x15 )
>      {
> @@ -455,21 +455,22 @@ static void intel_log_freq(const struct
>          }
>      }
>  
> -    if ( c->x86 == 0xf || rdmsr_safe(MSR_INTEL_PLATFORM_INFO, msrval) )
> -        return;
> -    max_ratio = msrval >> 8;
> -
> -    if ( max_ratio )
> +    switch ( c->x86 )
>      {
> -        unsigned int factor = 10000;
> -        uint8_t min_ratio = msrval >> 40;
> +        static const unsigned short core_factors[] =

This no longer applies to Core models only, so I wouldn't be opposed
to renaming to scaling_factors or similar.

Thanks, Roger.