Re: [RFC PATCH V2 0/1] x86: cpu topology fix and question on x86_max_cores

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Feb 20 2023 at 14:33, Rui Zhang wrote:
> On Mon, 2023-02-20 at 12:08 +0100, Peter Zijlstra wrote:
>> Also, perhaps you want to look at calculate_max_logical_packages().
>> That has a comment about there not being heterogeneous systems :/
>
> yeah, I noticed this previously.
>
>         ncpus = cpu_data(0).booted_cores * topology_max_smt_threads();
>         __max_logical_packages = DIV_ROUND_UP(total_cpus, ncpus);
>
> The DIV_ROUND_UP() makes it to work on systems with current asymmetric
> core systems. But
> 1. if a core can support more than 2 HT siblings, this can break if
> there are multi symmetric packages.
> 2. if the system has asymmetric packages, this can break.
> So far we don't have such platforms.
> 3. it can also be broken when using boot option 'maxcpus' as booted_cor
> es changes.
>
> But ironically, we don't have a better way to get __max_logical_packages.

Sadly enough this was communicated to Intel hard/firmware people many
years ago.

>> Anyway, the reason I went and had a look there, is because I remember
>> Thomas and me spend entirely too much time to try and figure out
>> means
>> to size an array for number of pacakges at boot time and getting it
>> wrong too many times to recount.
>> 
>> If only there was a sane way to tell these things without actually
>> bringing everything online first :-(
>
> I thought of improving this by parsing all the valid APIC-IDs in MADT
> during BSP bootup, and get such information by decoding the APIC-IDs
> using the APIC-ID layout information retrieved from BSP. But this is
> likely to be a fertile new source of bugs as Dave concerned.

The APIC-IDs are only usefull if there is an architected scheme how they
are assigned. Is there such a thing?

The SDM is not helpful at all, but according to the ACPI spec there
exists:

  Processor Properties Topology Table (PPTT)

That table actually provides pretty much what we are looking for, but
that table is optional and there is actually code for that in the
kernel, which is ARM64 specific.

So while this would be useful it's not usable on x86 because that would
make too much sense, right?

Thanks,

        tglx