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

K Prateek Nayak
2023-08-28T06:08:23Z

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