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

[Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>]

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).

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;

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.

Simon

Attachment: OpenPGP_signature
Description: OpenPGP digital signature