Re: [patch V4 28/41] x86/cpu: Provide a sane leaf 0xb/0x1f parser

[Zhang, Rui]

> +
> +static inline bool topo_subleaf(struct topo_scan *tscan, u32 leaf,
> u32 subleaf,
> +                               unsigned int *last_dom)
> +{
> +       unsigned int dom, maxtype = leaf == 0xb ? CORE_TYPE + 1 :
> MAX_TYPE;
> +       struct {
> +               // eax
> +               u32     x2apic_shift    :  5, // Number of bits to
> shift APIC ID right
> +                                             // for the topology ID
> at the next level
> +                       __rsvd0         : 27; // Reserved
> +               // ebx
> +               u32     num_processors  : 16, // Number of processors
> at current level
> +                       __rsvd1         : 16; // Reserved
> +               // ecx
> +               u32     level           :  8, // Current topology
> level. Same as sub leaf number
> +                       type            :  8, // Level type. If 0,
> invalid
> +                       __rsvd2         : 16; // Reserved
> +               // edx
> +               u32     x2apic_id       : 32; // X2APIC ID of the
> current logical processor
> +       } sl;
> +
> +       cpuid_subleaf(leaf, subleaf, &sl);
> +
> +       if (!sl.num_processors || sl.type == INVALID_TYPE)
> +               return false;
> +
> +       if (sl.type >= maxtype) {
> +               pr_err_once("Topology: leaf 0x%x:%d Unknown domain
> type %u\n",
> +                           leaf, subleaf, sl.type);
> +               /*
> +                * The subleafs are ordered in domain level order so
> +                * propagate it into the next domain level carefully:
> if
> +                * the last domain level was PKG, then overwrite PKG
> +                * as otherwise this would end up in the root domain.
> +                *
> +                * It really would have been too obvious to make the
> domain
> +                * type space sparse and leave a few reserved types
> between
> +                * the points which might change instead of following
> the
> +                * usual "this can be fixed in software" principle.
> +                */
> +               dom = *last_dom == TOPO_PKG_DOMAIN ? TOPO_PKG_DOMAIN
> : *last_dom + 1;
> +       } else {
> +               dom = topo_domain_map[sl.type];
> +               *last_dom = dom;
> +       }
> +
> +       if (!dom) {
> +               tscan->c->topo.initial_apicid = sl.x2apic_id;
> +       } else if (tscan->c->topo.initial_apicid != sl.x2apic_id) {
> +               pr_warn_once(FW_BUG "CPUID leaf 0x%x subleaf %d APIC
> ID mismatch %x != %x\n",
> +                            leaf, subleaf, tscan->c-
> >topo.initial_apicid, sl.x2apic_id);
> +       }
> +

Maybe worth a warning somewhere if sl.x2apic_shift !=
x86_topo_system.dom_shifts[dom], because invariant x2apic_shift (for
the same level) among different CPUs is critical for this patch series.
And I do see variant x2apic_shift on an Intel AlderLake NUC, which has
been identified to be a microcode bug later.

thanks,
rui