Re: [patch V4 24/41] x86/cpu: Provide cpu_init/parse_topology()

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Thomas,

On 8/14/2023 2:24 PM, Thomas Gleixner wrote:
> Topology evaluation is a complete disaster and impenetrable mess. It's
> scattered all over the place with some vendor implementations 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>
> Tested-by: Juergen Gross <jgross@suse.com>
> Tested-by: Sohil Mehta <sohil.mehta@intel.com>
> Tested-by: Michael Kelley <mikelley@microsoft.com>
> 
> ---
> V2: Make core ID package relativ not relative to the next level - Rui
> ---
>  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        |   36 ++++++
>  arch/x86/kernel/cpu/topology_common.c |  188 ++++++++++++++++++++++++++++++++++
>  7 files changed, 295 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,
> +};

Since these enums come from the description of level type of CPUID leaf
0x1f, can we have a short description clarifying what each signify. This
will also help clarify the mappings for AMD's extended CPUID leaf
0x80000026 (specifically for CCX and CCD level types). I had following
in my mind:

diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index a95fb8b2c612..132a392a0f0c 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -104,12 +104,48 @@ static inline void setup_node_to_cpumask_map(void) { }
 
 /* Topology information */
 enum x86_topology_domains {
+	/* Represents group of threads within a core */
 	TOPO_SMT_DOMAIN,
+	/*
+	 * Represents group of cores within an instance of
+	 * the next domain
+	 */
 	TOPO_CORE_DOMAIN,
+	/*
+	 * If exists, represents a group of modules within
+	 * an instance of the next domain
+	 */
 	TOPO_MODULE_DOMAIN,
+	/*
+	 * If exists, represents a group of tiles within
+	 * an instance of the next domain
+	 *
+	 * On Intel: This level contains a group of Tile
+	 * type as described by CPUID leaf 0x1f
+	 *
+	 * On AMD: This is the group of "Complex" type
+	 * instances as described by CPUID leaf
+	 * 0x8000_0026
+	 */
 	TOPO_TILE_DOMAIN,
+	/*
+	 * If exists, represents a group of dies within an
+	 * instance of the next domain
+	 *
+	 * On Intel: This level contains group of Die
+	 * type as described by CPUID leaf 0x1f
+	 *
+	 * On AMD: This is the group of "CCD (Die)"
+	 * type instances as described by CPUID leaf
+	 * 0x8000_0026
+	 */
 	TOPO_DIE_DOMAIN,
+	/*
+	 * If exists, represents a group of packages
+	 * within the root domain
+	 */
 	TOPO_PKG_DOMAIN,
+	/* Topmost domain with a singular instance */
 	TOPO_ROOT_DOMAIN,
 	TOPO_MAX_DOMAIN,
 };
--

>
> [..snip..] 
> 

--
Thanks and Regards,
Prateek