Re: [PATCH 5/6] xen/x86: Derive topologically correct x2APIC IDs from the policy

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 02/05/2024 07:57, Jan Beulich wrote:
> On 02.05.2024 08:55, Jan Beulich wrote:
>> On 01.05.2024 18:35, Alejandro Vallejo wrote:
>>> Hi,
>>>
>>> On 26/03/2024 16:41, Jan Beulich wrote:
>>>> On 09.01.2024 16:38, Alejandro Vallejo wrote:
>>>>> --- a/xen/lib/x86/policy.c
>>>>> +++ b/xen/lib/x86/policy.c
>>>>> @@ -2,15 +2,78 @@
>>>>>  
>>>>>  #include <xen/lib/x86/cpu-policy.h>
>>>>>  
>>>>> -uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t 
>>>>> vcpu_id)
>>>>> +static uint32_t parts_per_higher_scoped_level(const struct cpu_policy 
>>>>> *p, size_t lvl)
>>>>>  {
>>>>>      /*
>>>>> -     * TODO: Derive x2APIC ID from the topology information inside `p`
>>>>> -     *       rather than from vCPU ID. This bodge is a temporary measure
>>>>> -     *       until all infra is in place to retrieve or derive the 
>>>>> initial
>>>>> -     *       x2APIC ID from migrated domains.
>>>>> +     * `nr_logical` reported by Intel is the number of THREADS contained 
>>>>> in
>>>>> +     * the next topological scope. For example, assuming a system with 2
>>>>> +     * threads/core and 3 cores/module in a fully symmetric topology,
>>>>> +     * `nr_logical` at the core level will report 6. Because it's 
>>>>> reporting
>>>>> +     * the number of threads in a module.
>>>>> +     *
>>>>> +     * On AMD/Hygon, nr_logical is already normalized by the higher 
>>>>> scoped
>>>>> +     * level (cores/complex, etc) so we can return it as-is.
>>>>>       */
>>>>> -    return vcpu_id * 2;
>>>>> +    if ( p->x86_vendor != X86_VENDOR_INTEL || !lvl )
>>>>> +        return p->topo.subleaf[lvl].nr_logical;
>>>>
>>>> Is "!= Intel" really appropriate here? I'd rather see this being "AMD || 
>>>> Hygon".
>>>
>>> Sure, I don't particularly mind, but why? As far as we know only Intel
>>> has this interpretation for the part counts. I definitely haven't seen
>>> any non-Intel CPUID dump in which the part count is the total number of
>>> threads (Centaur/Zhaoxin are not multithreaded, and don't expose leaves
>>> 1f or e26, as far as I could see).
>>
>> Because of x86'es origin and perhaps other historical aspects, cloning
>> Intel behavior is far more likely.

That claim doesn't hold very well seeing how...

>> The fact that Hygon matches AMD is
>> simply because they took AMD's design wholesale.

... this statement contradicts it. We can't predict which new vendor (if
any) will be cloned/mimicked next, so that's not a very plausible reason
to prioritise a specific vendor in conditionals.

It remains to be seen what a Zhaoxin actually looks like, because I
couldn't get ahold of a complete cpuid dump.

> 
> Perhaps: See how many dead ends AMD have created, i.e. stuff they proudly
> introduced into the architecture, but then gave up again (presumably for
> diverging too far from Intel, and hence lacking long term acceptance):
> 3DNow!, LWP, and XOP just to name those that come to mind right away.
> 
> Jan

I can't say I agree on the cause; Regardless I'd rather not discuss the
relative merits of vendors with regards to backwards compatibility, as
that's besides the point. The point is whether there's a credible
technical reason to prefer this...

  if ( !(a & (B | C)) )
      foo();

... to this...

  if ( a == A )
      foo();

..., as is the case in patch6.

I argue there's not, and in fact legibility-wise the latter is very
clearly superior.

There's also a compelling reason to keep the check coherent on both
generators to avoid bad surprises down the line.

Cheers,
Alejandro