Re: [PATCH v4 02/10] x86/cpu/topology: Add CPU type to struct cpuinfo_topology
From: Mario Limonciello
Date: Tue Oct 22 2024 - 10:42:58 EST
On 10/18/2024 16:11, Pawan Gupta wrote:
On Fri, Oct 18, 2024 at 11:28:31AM -0500, Mario Limonciello wrote:
On 9/30/2024 09:47, Pawan Gupta wrote:
Sometimes it is required to take actions based on if a CPU is a performance
or efficiency core. As an example, intel_pstate driver uses the Intel
core-type to determine CPU scaling. Also, some CPU vulnerabilities only
affect a specific CPU type, like RFDS only affects Intel Atom. Hybrid
systems that have variants P+E, P-only(Core) and E-only(Atom), it is not
straightforward to identify which variant is affected by a type specific
vulnerability.
Such processors do have CPUID field that can uniquely identify them. Like,
P+E, P-only and E-only enumerates CPUID.1A.CORE_TYPE identification, while
P+E additionally enumerates CPUID.7.HYBRID. Based on this information, it
is possible for boot CPU to identify if a system has mixed CPU types.
Add a new field hw_cpu_type to struct cpuinfo_topology that stores the
hardware specific CPU type. This saves the overhead of IPIs to get the CPU
type of a different CPU. CPU type is populated early in the boot process,
before vulnerabilities are enumerated.
Signed-off-by: Pawan Gupta <pawan.kumar.gupta@xxxxxxxxxxxxxxx>
---
arch/x86/include/asm/cpu.h | 6 ++++++
arch/x86/include/asm/processor.h | 11 +++++++++++
arch/x86/include/asm/topology.h | 8 ++++++++
arch/x86/kernel/cpu/debugfs.c | 1 +
arch/x86/kernel/cpu/intel.c | 5 +++++
arch/x86/kernel/cpu/topology_common.c | 11 +++++++++++
6 files changed, 42 insertions(+)
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index aa30fd8cad7f..2244dd86066a 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -32,6 +32,7 @@ extern bool handle_user_split_lock(struct pt_regs *regs, long error_code);
extern bool handle_guest_split_lock(unsigned long ip);
extern void handle_bus_lock(struct pt_regs *regs);
u8 get_this_hybrid_cpu_type(void);
+u32 intel_native_model_id(struct cpuinfo_x86 *c);
#else
static inline void __init sld_setup(struct cpuinfo_x86 *c) {}
static inline bool handle_user_split_lock(struct pt_regs *regs, long error_code)
@@ -50,6 +51,11 @@ static inline u8 get_this_hybrid_cpu_type(void)
{
return 0;
}
+
+static u32 intel_native_model_id(struct cpuinfo_x86 *c)
+{
+ return 0;
+}
#endif
#ifdef CONFIG_IA32_FEAT_CTL
void init_ia32_feat_ctl(struct cpuinfo_x86 *c);
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 4a686f0e5dbf..61c8336bc99b 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -105,6 +105,17 @@ struct cpuinfo_topology {
// Cache level topology IDs
u32 llc_id;
u32 l2c_id;
+
+ // Hardware defined CPU-type
+ union {
+ u32 hw_cpu_type;
+ struct {
+ /* CPUID.1A.EAX[23-0] */
+ u32 intel_core_native_model_id:24;
+ /* CPUID.1A.EAX[31-24] */
+ u32 intel_core_type:8;
+ };
+ };
};
struct cpuinfo_x86 {
diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index aef70336d624..faf7cb7f7d7e 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -114,6 +114,12 @@ enum x86_topology_domains {
TOPO_MAX_DOMAIN,
};
+enum x86_topology_hw_cpu_type {
+ TOPO_HW_CPU_TYPE_UNKNOWN = 0,
+ TOPO_HW_CPU_TYPE_INTEL_ATOM = 0x20,
+ TOPO_HW_CPU_TYPE_INTEL_CORE = 0x40,
+};
This isn't exactly generic. Unless you have a strong need to know "Atom"
The goal was not to have generic cpu_type here, but the actual CPU type
that hardware enumerates. I was asked to prepend "hw_" to cpu_type to make
is clear that this is hardware defined, and to leave scope for generic
cpu_type, if we add those in future.
instead of "Efficient" or "Core" instead of "Performance" I think it would
be better to do this as:
enum x86_topology_hw_core_type {
TOPO_HW_CORE_TYPE_UNKNOWN = 0,
TOPO_HW_CORE_TYPE_PERFORMANT,
TOPO_HW_CORE_TYPE_EFFICIENT,
};
Then you can do the mapping of 0x20 = Efficient and 0x40 = performant in the
Intel topology lookup function.
I can add a lookup function, but I wanted to understand the use case of
generic cpu_type. If we always have to lookup and map the cpu_type, then
why not have the actual cpu_type in the first place?
One case where generic cpu_type can be useful is when we expose them to
userspace, which I think is inevitable. Overall I am fine with adding generic
cpu type. It may also make sense to have separate accessors for generic and
and hardware defined cpu_type, and the generic ones when we actually have a
use case. Thoughts?
After you land the series we can do something similar to move AMD code
around and map it out to the right generic mapping.
I took your patch and made the modifications that I thought made sense
for a generic type while adding the matching AMD code and sent it out
(you're on CC). Can you take a look and see what you think? Boris
already provided some feedback that I'm going to spin it again.
I think if we can align on that one we can land that patch and you can
rebase the rest of the series on it.