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 11:58, Simon Gaiser wrote:
> Jan Beulich:
>> 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.
> 
> I'm indeed not quite sure on the intended semantic, hence the question
> (although it's not clear to me what case that "could" would cover here).

"Could" covers for a number of reasons why APIC IDs may not be contiguous.
Consider a 6-code system: The APIC IDs need to cover for at least 8 there.

> It doesn't have to be identical but Linux says for it's version of the
> variable:
> 
>     The number of cores in a package. This information is retrieved via
>     CPUID.
> 
> And if I look at it's usage in set_nr_sockets in Xen:
> 
>     nr_sockets = last_physid(phys_cpu_present_map)
>                  / boot_cpu_data.x86_max_cores
>                  / boot_cpu_data.x86_num_siblings + 1;

This validly uses the field in the "max" sense, not in the "actual" one.

> It seems to be also be used in this meaning. At least on my test system
> I get last_physid == 7 (as I would have expected for 8 logical cpus). So
> dividing this by the 4 (number of cores) and 2 (threads per core) is
> what I think was intended here.

Would you mind providing raw data from your system: Both the raw CPUID
output for the leaf/leaves of interest here and the APIC IDs of all
threads?

Jan