RE: [patch V3 30/40] x86/cpu: Provide an AMD/HYGON specific topology parser

[Michael Kelley (LINUX)]

From: Thomas Gleixner <tglx@xxxxxxxxxxxxx> Sent: Wednesday, August 2, 2023 3:22 AM
> 
> AMD/HYGON uses various methods for topology evaluation:
> 
>   - Leaf 0x80000008 and 0x8000001e based with an optional leaf 0xb,
>     which is the preferred variant for modern CPUs.
> 
>     Leaf 0xb will be superseded by leaf 0x80000026 soon, which is just
>     another variant of the Intel 0x1f leaf for whatever reasons.
> 
>   - Subleaf 0x80000008 and NODEID_MSR base
> 
>   - Legacy fallback
> 
> That code is following the principle of random bits and pieces all over the
> place which results in multiple evaluations and impenetrable code flows in
> the same way as the Intel parsing did.
> 
> Provide a sane implementation by clearly separating the three variants and
> bringing them in the proper preference order in one place.
> 
> This provides the parsing for both AMD and HYGON because there is no point
> in having a separate HYGON parser which only differs by 3 lines of
> code. Any further divergence between AMD and HYGON can be handled in
> different functions, while still sharing the existing parsers.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
> V3: Fix the off by one with leaf 0x8000001e::ebx::threads_per_cu - Michael
> ---

[snip]

> +
> +static bool parse_8000_001e(struct topo_scan *tscan, bool has_0xb)
> +{
> +	struct {
> +		// eax
> +		u32	x2apic_id	: 32;
> +		// ebx
> +		u32	cuid		:  8,
> +			threads_per_cu	:  8,
> +			__rsvd0		: 16;
> +		// ecx
> +		u32	nodeid		:  8,
> +			nodes_per_pkg	:  3,
> +			__rsvd1		: 21;
> +		// edx
> +		u32	__rsvd2		: 32;
> +	} leaf;
> +
> +	if (!boot_cpu_has(X86_FEATURE_TOPOEXT))
> +		return false;
> +
> +	cpuid_leaf(0x8000001e, &leaf);
> +
> +	tscan->c->topo.initial_apicid = leaf.x2apic_id;
> +
> +	/*
> +	 * If leaf 0xb is available, then SMT shift is set already. If not
> +	 * take it from ecx.threads_per_cu and use topo_update_dom() -
> +	 * topology_set_dom() would propagate and overwrite the already
> +	 * propagated CORE level.
> +	 */
> +	if (!has_0xb) {
> +		topology_update_dom(tscan, TOPO_SMT_DOMAIN, get_count_order(leaf.threads_per_cu),
> +				    leaf.threads_per_cu + 1);

Isn't the +1 also needed on the argument to get_count_order()?
I haven't actually run the config, but if hyper-threading is disabled,
presumably threads_per_cu is 0, and get_count_order returns -1.

Michael

> +	}
> +
> +	store_node(tscan, leaf.nodes_per_pkg + 1, leaf.nodeid);
> +
> +	if (tscan->c->x86_vendor == X86_VENDOR_AMD) {
> +		if (tscan->c->x86 == 0x15)
> +			tscan->c->topo.cu_id = leaf.cuid;
> +
> +		cacheinfo_amd_init_llc_id(tscan->c, leaf.nodeid);
> +	} else {
> +		/*
> +		 * Package ID is ApicId[6..] on Hygon CPUs. See commit
> +		 * e0ceeae708ce for explanation. The topology info is
> +		 * screwed up: The package shift is always 6 and the node
> +		 * ID is bit [4:5]. Don't touch the latter without
> +		 * confirmation from the Hygon developers.
> +		 */
> +		topology_set_dom(tscan, TOPO_CORE_DOMAIN, 6, tscan->dom_ncpus[TOPO_CORE_DOMAIN]);
> +		cacheinfo_hygon_init_llc_id(tscan->c);
> +	}
> +	return true;
> +}
> +