Re: [Xen-devel] [PATCH v2 04/23] x86: Don't use potentially incorrect CPUID values for topology information

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/08/2023 9:18 am, Simon Gaiser wrote:
> Anthony Liguori:
>> From: Jan H. Schönherr <jschoenh@xxxxxxxxx>
>>
>> Intel says for CPUID leaf 0Bh:
>>
>>   "Software must not use EBX[15:0] to enumerate processor
>>    topology of the system. This value in this field
>>    (EBX[15:0]) is only intended for display/diagnostic
>>    purposes. The actual number of logical processors
>>    available to BIOS/OS/Applications may be different from
>>    the value of EBX[15:0], depending on software and platform
>>    hardware configurations."
>>
>> And yet, we're using them to derive the number cores in a package
>> and the number of siblings in a core.
>>
>> Derive the number of siblings and cores from EAX instead, which is
>> intended for that.
>>
>> Signed-off-by: Jan H. Schönherr <jschoenh@xxxxxxxxx>
>> ---
>>  xen/arch/x86/cpu/common.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
>> index e9588b3..22f392f 100644
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -479,8 +479,8 @@ void detect_extended_topology(struct cpuinfo_x86 *c)
>>      initial_apicid = edx;
>>  
>>      /* Populate HT related information from sub-leaf level 0 */
>> -    core_level_siblings = c->x86_num_siblings = LEVEL_MAX_SIBLINGS(ebx);
>>      core_plus_mask_width = ht_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
>> +    core_level_siblings = c->x86_num_siblings = 1 << ht_mask_width;
>>  
>>      sub_index = 1;
>>      do {
>> @@ -488,8 +488,8 @@ void detect_extended_topology(struct cpuinfo_x86 *c)
>>  
>>              /* Check for the Core type in the implemented sub leaves */
>>              if ( LEAFB_SUBTYPE(ecx) == CORE_TYPE ) {
>> -                    core_level_siblings = LEVEL_MAX_SIBLINGS(ebx);
>>                      core_plus_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
>> +                    core_level_siblings = 1 << core_plus_mask_width;
>
> On the i5-1135G7 (4 cores with each 2 threads) I'm currently testing on
> I noticed that this changes leads to core_level_siblings == 16 and
> therefore x86_max_cores == 8. If read from ebx like before this change
> and what Linux is still doing [1] it reads core_level_siblings == 8 (as
> expected?).
>
> What's the expected semantic here? If it's to derive the actual number
> of siblings (and therefore cores) in a package as the commit message
> suggest, the new code can't work even ignoring the example from my test
> system. It will always produce powers of 2, so can't get it right on a
> system with, say, 6 cores.
>
> [1]: 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/cpu/topology.c?h=v6.4#n126

Topology is broken in many subtle ways, including bits Xen inherited
from Linux.

As it happens, Thomas is working on the Linux side right now, and it's
been quite an effort...

https://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git/log/?h=x86/topo-full

In some copious free time I'll be ste^W borrowing this.  It comes with a
negative diffstat too.

~Andrew