| Message ID | 20240627-add-cpu-type-v2-1-f927bde83ad0@linux.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Add CPU-type to topology | expand |
On Thu, Jun 27, 2024 at 01:44:06PM -0700, Pawan Gupta wrote: > The hw_cpu_type is populated in the below debugfs file: > > # cat /sys/kernel/debug/x86/topo/cpus/# What "below debugfs file"? A '#'? > diff --git a/arch/x86/kernel/cpu/debugfs.c b/arch/x86/kernel/cpu/debugfs.c > index 3baf3e435834..8082e03a5976 100644 > --- a/arch/x86/kernel/cpu/debugfs.c > +++ b/arch/x86/kernel/cpu/debugfs.c > @@ -22,6 +22,7 @@ static int cpu_debug_show(struct seq_file *m, void *p) > seq_printf(m, "die_id: %u\n", c->topo.die_id); > seq_printf(m, "cu_id: %u\n", c->topo.cu_id); > seq_printf(m, "core_id: %u\n", c->topo.core_id); > + seq_printf(m, "hw_cpu_type: %x\n", c->topo.hw_cpu_type); Yeah, no, we're not going to perpetuate this silliness of printing hex values without a preceding "0x". > seq_printf(m, "logical_pkg_id: %u\n", c->topo.logical_pkg_id); > seq_printf(m, "logical_die_id: %u\n", c->topo.logical_die_id); > seq_printf(m, "llc_id: %u\n", c->topo.llc_id); > diff --git a/arch/x86/kernel/cpu/topology_common.c b/arch/x86/kernel/cpu/topology_common.c > index 9a6069e7133c..8b47bd6b0623 100644 > --- a/arch/x86/kernel/cpu/topology_common.c > +++ b/arch/x86/kernel/cpu/topology_common.c > @@ -140,6 +140,14 @@ static void parse_topology(struct topo_scan *tscan, bool early) > } > } > > +static void topo_set_hw_cpu_type(struct cpuinfo_x86 *c) > +{ > + c->topo.hw_cpu_type = X86_HW_CPU_TYPE_UNKNOWN; > + > + if (c->x86_vendor == X86_VENDOR_INTEL && c->cpuid_level >= 0x1a) > + c->topo.hw_cpu_type = cpuid_eax(0x1a) >> X86_CPU_TYPE_INTEL_SHIFT; > +} Why isn't this happening in cpu/intel.c? And then you don't need yet another silly function.
On Fri, Jun 28, 2024 at 10:03:05AM +0200, Borislav Petkov wrote: > On Thu, Jun 27, 2024 at 01:44:06PM -0700, Pawan Gupta wrote: > > The hw_cpu_type is populated in the below debugfs file: > > > > # cat /sys/kernel/debug/x86/topo/cpus/# > > What "below debugfs file"? A '#'? That is the number of the CPU. If it is causing confusion, I can will change it to N, or say # means the number of the CPU. > > diff --git a/arch/x86/kernel/cpu/debugfs.c b/arch/x86/kernel/cpu/debugfs.c > > index 3baf3e435834..8082e03a5976 100644 > > --- a/arch/x86/kernel/cpu/debugfs.c > > +++ b/arch/x86/kernel/cpu/debugfs.c > > @@ -22,6 +22,7 @@ static int cpu_debug_show(struct seq_file *m, void *p) > > seq_printf(m, "die_id: %u\n", c->topo.die_id); > > seq_printf(m, "cu_id: %u\n", c->topo.cu_id); > > seq_printf(m, "core_id: %u\n", c->topo.core_id); > > + seq_printf(m, "hw_cpu_type: %x\n", c->topo.hw_cpu_type); > > Yeah, no, we're not going to perpetuate this silliness of printing hex > values without a preceding "0x". I thought about that, but the other fields are also printed without a preceding "0x": seq_printf(m, "initial_apicid: %x\n", c->topo.initial_apicid); seq_printf(m, "apicid: %x\n", c->topo.apicid); ... I will change those too, probably in a separate patch. > > +static void topo_set_hw_cpu_type(struct cpuinfo_x86 *c) > > +{ > > + c->topo.hw_cpu_type = X86_HW_CPU_TYPE_UNKNOWN; > > + > > + if (c->x86_vendor == X86_VENDOR_INTEL && c->cpuid_level >= 0x1a) > > + c->topo.hw_cpu_type = cpuid_eax(0x1a) >> X86_CPU_TYPE_INTEL_SHIFT; > > +} > > Why isn't this happening in cpu/intel.c? And then you don't need yet > another silly function. I was preferring to keep the topology related code in one place. Would it make sense to keep it in Intel specific leg in parse_topology() as below: --- diff --git a/arch/x86/kernel/cpu/topology_common.c b/arch/x86/kernel/cpu/topology_common.c index 8b47bd6b0623..c8869e75365f 100644 --- a/arch/x86/kernel/cpu/topology_common.c +++ b/arch/x86/kernel/cpu/topology_common.c @@ -87,6 +87,7 @@ static void parse_topology(struct topo_scan *tscan, bool early) .cu_id = 0xff, .llc_id = BAD_APICID, .l2c_id = BAD_APICID, + .hw_cpu_type = X86_HW_CPU_TYPE_UNKNOWN, }; struct cpuinfo_x86 *c = tscan->c; struct { @@ -132,6 +133,8 @@ static void parse_topology(struct topo_scan *tscan, bool early) case X86_VENDOR_INTEL: if (!IS_ENABLED(CONFIG_CPU_SUP_INTEL) || !cpu_parse_topology_ext(tscan)) parse_legacy(tscan); + if (c->cpuid_level >= 0x1a) + c->topo.hw_cpu_type = cpuid_eax(0x1a) >> X86_CPU_TYPE_INTEL_SHIFT; break; case X86_VENDOR_HYGON: if (IS_ENABLED(CONFIG_CPU_SUP_HYGON))
On Fri, Jun 28, 2024 at 10:32:09AM -0700, Pawan Gupta wrote: > On Fri, Jun 28, 2024 at 10:03:05AM +0200, Borislav Petkov wrote: > > On Thu, Jun 27, 2024 at 01:44:06PM -0700, Pawan Gupta wrote: > > > The hw_cpu_type is populated in the below debugfs file: > > > > > > # cat /sys/kernel/debug/x86/topo/cpus/# > > > > What "below debugfs file"? A '#'? > > That is the number of the CPU. If it is causing confusion, I can will > change it to N, or say # means the number of the CPU. Or drop that sentence completely. For some reason lately it seems to me folks feel the need to explain what the patch does. Even if that is visible from the diff. > I thought about that, but the other fields are also printed without a > preceding "0x": > > seq_printf(m, "initial_apicid: %x\n", c->topo.initial_apicid); > seq_printf(m, "apicid: %x\n", c->topo.apicid); > ... > > I will change those too, probably in a separate patch. Yes please. > > > +static void topo_set_hw_cpu_type(struct cpuinfo_x86 *c) > > > +{ > > > + c->topo.hw_cpu_type = X86_HW_CPU_TYPE_UNKNOWN; > > > + > > > + if (c->x86_vendor == X86_VENDOR_INTEL && c->cpuid_level >= 0x1a) > > > + c->topo.hw_cpu_type = cpuid_eax(0x1a) >> X86_CPU_TYPE_INTEL_SHIFT; > > > +} > > > > Why isn't this happening in cpu/intel.c? And then you don't need yet > > another silly function. > > I was preferring to keep the topology related code in one place. Would it > make sense to keep it in Intel specific leg in parse_topology() as below: I guess. Although looking around this code, I wonder why is this hw_cpu_type part of the topology and not part of cpuinfo_x86 directly? struct cpuinfo_topology has all the IDs but the type? Feels out of place there.
On Thu, Jul 04, 2024 at 01:07:24AM +0200, Borislav Petkov wrote: > On Fri, Jun 28, 2024 at 10:32:09AM -0700, Pawan Gupta wrote: > > On Fri, Jun 28, 2024 at 10:03:05AM +0200, Borislav Petkov wrote: > > > On Thu, Jun 27, 2024 at 01:44:06PM -0700, Pawan Gupta wrote: > > > > The hw_cpu_type is populated in the below debugfs file: > > > > > > > > # cat /sys/kernel/debug/x86/topo/cpus/# > > > > > > What "below debugfs file"? A '#'? > > > > That is the number of the CPU. If it is causing confusion, I can will > > change it to N, or say # means the number of the CPU. > > Or drop that sentence completely. Ok. > > > > +static void topo_set_hw_cpu_type(struct cpuinfo_x86 *c) > > > > +{ > > > > + c->topo.hw_cpu_type = X86_HW_CPU_TYPE_UNKNOWN; > > > > + > > > > + if (c->x86_vendor == X86_VENDOR_INTEL && c->cpuid_level >= 0x1a) > > > > + c->topo.hw_cpu_type = cpuid_eax(0x1a) >> X86_CPU_TYPE_INTEL_SHIFT; > > > > +} > > > > > > Why isn't this happening in cpu/intel.c? And then you don't need yet > > > another silly function. > > > > I was preferring to keep the topology related code in one place. Would it > > make sense to keep it in Intel specific leg in parse_topology() as below: > > I guess. > > Although looking around this code, I wonder why is this hw_cpu_type part of > the topology and not part of cpuinfo_x86 directly? > > struct cpuinfo_topology has all the IDs but the type? Feels out of place > there. The draft version had it in cpuinfo_x86, but it later got moved to cpuinfo_topology as it appears to be topology related. Below is from AMD documentation: E.4.24 Function 8000_0026—Extended CPU Topology CPUID Fn8000_0026 reports extended topology information for logical processors, including asymmetric and heterogenous topology descriptions. Individual logical processors may report different values in systems with asynchronous and heterogeneous topologies. https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24594.pdf I can move it back to cpuinfo_x86 if you feel strongly about it.
On Mon, Jul 08, 2024 at 06:24:18PM -0700, Pawan Gupta wrote: > The draft version had it in cpuinfo_x86, but it later got moved to > cpuinfo_topology as it appears to be topology related. Below is from AMD > documentation: > > E.4.24 Function 8000_0026—Extended CPU Topology That's the name of the CPUID leaf. > CPUID Fn8000_0026 reports extended topology information for logical > processors, including asymmetric and heterogenous topology descriptions. > Individual logical processors may report different values in systems with > asynchronous and heterogeneous topologies. > https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24594.pdf > > I can move it back to cpuinfo_x86 if you feel strongly about it. Nah, it just felt weird. Not a biggie and we can always move it if we deem so.
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index cb4f6c513c48..d8d715fcc25c 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -105,6 +105,9 @@ struct cpuinfo_topology { // Cache level topology IDs u32 llc_id; u32 l2c_id; + + // Hardware defined CPU-type + u8 hw_cpu_type; }; struct cpuinfo_x86 { diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h index abe3a8f22cbd..717fdb928dc3 100644 --- a/arch/x86/include/asm/topology.h +++ b/arch/x86/include/asm/topology.h @@ -33,6 +33,14 @@ #include <linux/numa.h> #include <linux/cpumask.h> +#define X86_CPU_TYPE_INTEL_SHIFT 24 + +enum x86_hw_topo_cpu_type { + X86_HW_CPU_TYPE_UNKNOWN = 0, + X86_HW_CPU_TYPE_INTEL_ATOM = 0x20, + X86_HW_CPU_TYPE_INTEL_CORE = 0x40, +}; + #ifdef CONFIG_NUMA #include <asm/mpspec.h> @@ -139,6 +147,7 @@ extern const struct cpumask *cpu_clustergroup_mask(int cpu); #define topology_logical_die_id(cpu) (cpu_data(cpu).topo.logical_die_id) #define topology_die_id(cpu) (cpu_data(cpu).topo.die_id) #define topology_core_id(cpu) (cpu_data(cpu).topo.core_id) +#define topology_hw_cpu_type(cpu) (cpu_data(cpu).topo.hw_cpu_type) #define topology_ppin(cpu) (cpu_data(cpu).ppin) #define topology_amd_node_id(cpu) (cpu_data(cpu).topo.amd_node_id) diff --git a/arch/x86/kernel/cpu/debugfs.c b/arch/x86/kernel/cpu/debugfs.c index 3baf3e435834..8082e03a5976 100644 --- a/arch/x86/kernel/cpu/debugfs.c +++ b/arch/x86/kernel/cpu/debugfs.c @@ -22,6 +22,7 @@ static int cpu_debug_show(struct seq_file *m, void *p) seq_printf(m, "die_id: %u\n", c->topo.die_id); seq_printf(m, "cu_id: %u\n", c->topo.cu_id); seq_printf(m, "core_id: %u\n", c->topo.core_id); + seq_printf(m, "hw_cpu_type: %x\n", c->topo.hw_cpu_type); seq_printf(m, "logical_pkg_id: %u\n", c->topo.logical_pkg_id); seq_printf(m, "logical_die_id: %u\n", c->topo.logical_die_id); seq_printf(m, "llc_id: %u\n", c->topo.llc_id); diff --git a/arch/x86/kernel/cpu/topology_common.c b/arch/x86/kernel/cpu/topology_common.c index 9a6069e7133c..8b47bd6b0623 100644 --- a/arch/x86/kernel/cpu/topology_common.c +++ b/arch/x86/kernel/cpu/topology_common.c @@ -140,6 +140,14 @@ static void parse_topology(struct topo_scan *tscan, bool early) } } +static void topo_set_hw_cpu_type(struct cpuinfo_x86 *c) +{ + c->topo.hw_cpu_type = X86_HW_CPU_TYPE_UNKNOWN; + + if (c->x86_vendor == X86_VENDOR_INTEL && c->cpuid_level >= 0x1a) + c->topo.hw_cpu_type = cpuid_eax(0x1a) >> X86_CPU_TYPE_INTEL_SHIFT; +} + static void topo_set_ids(struct topo_scan *tscan, bool early) { struct cpuinfo_x86 *c = tscan->c; @@ -190,6 +198,7 @@ void cpu_parse_topology(struct cpuinfo_x86 *c) } topo_set_ids(&tscan, false); + topo_set_hw_cpu_type(c); } void __init cpu_init_topology(struct cpuinfo_x86 *c)
Sometimes it is required to identify the type of a core for taking specific actions e.g. intel_pstate driver uses the core type to determine CPU scaling. Also, some CPU vulnerabilities only affect a specific CPU type e.g. RFDS only affects Intel Atom. For hybrid systems that have variants P+E, P-only(Core) and E-only(Atom), it gets challenging to identify which variant is vulnerable to a specific vulnerability, as these variants share the same family, model and stepping. Such processors do have CPUID fields that uniquely identify them. Like, P+E, P-only and E-only enumerates CPUID.1A.CORE_TYPE, while P+E additionally enumerates CPUID.7.HYBRID. Linux does not currently use this field. Add a new field hw_cpu_type to struct cpuinfo_topology which can be used to match a CPU based on its type. The hw_cpu_type is populated in the below debugfs file: # cat /sys/kernel/debug/x86/topo/cpus/# Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> --- arch/x86/include/asm/processor.h | 3 +++ arch/x86/include/asm/topology.h | 9 +++++++++ arch/x86/kernel/cpu/debugfs.c | 1 + arch/x86/kernel/cpu/topology_common.c | 9 +++++++++ 4 files changed, 22 insertions(+)