Re: [patch v2 21/38] x86/cpu: Provide cpu_init/parse_topology()

["Gautham R. Shenoy" <gautham.shenoy@amd.com>]

Hello Thomas,

On Fri, Jul 28, 2023 at 02:13:08PM +0200, Thomas Gleixner wrote:
> Topology evaluation is a complete disaster and impenetrable mess. It's
> scattered all over the place with some vendor implementatins doing early
> evaluation and some not. The most horrific part is the permanent
> overwriting of smt_max_siblings and __max_die_per_package, instead of
> establishing them once on the boot CPU and validating the result on the
> APs.
> 
> The goals are:
> 
>   - One topology evaluation entry point
> 
>   - Proper sharing of pointlessly duplicated code
> 
>   - Proper structuring of the evaluation logic and preferences.
> 
>   - Evaluating important system wide information only once on the boot CPU
> 
>   - Making the 0xb/0x1f leaf parsing less convoluted and actually fixing
>     the short comings of leaf 0x1f evaluation.
> 
> Start to consolidate the topology evaluation code by providing the entry
> points for the early boot CPU evaluation and for the final parsing on the
> boot CPU and the APs.
> 
> Move the trivial pieces into that new code:
> 
>    - The initialization of cpuinfo_x86::topo
> 
>    - The evaluation of CPUID leaf 1, which presets topo::initial_apicid
> 
>    - topo_apicid is set to topo::initial_apicid when invoked from early
>      boot. When invoked for the final evaluation on the boot CPU it reads
>      the actual APIC ID, which makes apic_get_initial_apicid() obsolete
>      once everything is converted over.
> 
> Provide a temporary helper function topo_converted() which shields off the
> not yet converted CPU vendors from invoking code which would break them.
> This shielding covers all vendor CPUs which support SMP, but not the
> historical pure UP ones as they only need the topology info init and
> eventually the initial APIC initialization.
> 
> Provide two new members in cpuinfo_x86::topo to store the maximum number of
> SMT siblings and the number of dies per package and add them to the debugfs
> readout. These two members will be used to populate this information on the
> boot CPU and to validate the APs against it.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/topology.h       |   19 +++
>  arch/x86/kernel/cpu/Makefile          |    3 
>  arch/x86/kernel/cpu/common.c          |   23 +---
>  arch/x86/kernel/cpu/cpu.h             |    6 +
>  arch/x86/kernel/cpu/debugfs.c         |   37 ++++++
>  arch/x86/kernel/cpu/topology.h        |   32 +++++
>  arch/x86/kernel/cpu/topology_common.c |  187 ++++++++++++++++++++++++++++++++++
>  7 files changed, 290 insertions(+), 17 deletions(-)
> 
> --- a/arch/x86/include/asm/topology.h
> +++ b/arch/x86/include/asm/topology.h
> @@ -102,6 +102,25 @@ static inline void setup_node_to_cpumask
>  
>  #include <asm-generic/topology.h>
>  
> +/* Topology information */
> +enum x86_topology_domains {
> +	TOPO_SMT_DOMAIN,
> +	TOPO_CORE_DOMAIN,
> +	TOPO_MODULE_DOMAIN,
> +	TOPO_TILE_DOMAIN,
> +	TOPO_DIE_DOMAIN,
> +	TOPO_PKG_DOMAIN,
> +	TOPO_ROOT_DOMAIN,
> +	TOPO_MAX_DOMAIN,
> +};
> +

[..snip..]

> +static void topo_set_ids(struct topo_scan *tscan)
> +{
> +	struct cpuinfo_x86 *c = tscan->c;
> +	u32 apicid = c->topo.apicid;
> +
> +	c->topo.pkg_id = topo_shift_apicid(apicid, TOPO_ROOT_DOMAIN);

Shouldn't this use TOPO_PKG_DOMAIN instead of TOPO_ROOT_DOMAIN ?


> +	c->topo.die_id = topo_shift_apicid(apicid, TOPO_DIE_DOMAIN);
> +
> +	/* Relative core ID */
> +	c->topo.core_id = topo_relative_domain_id(apicid, TOPO_CORE_DOMAIN);
> +}
> +

--
Thanks and Regards
gautham.