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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.08.2023 10:18, 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.

The only use of the variable in question is in this statement:

        c->x86_max_cores = (core_level_siblings / c->x86_num_siblings);

Note the "max" in the name. This is how many _could_ be there, not how
many _are_ there, aiui.

Jan