Re: [PATCH v4 02/10] x86/cpu/topology: Add CPU type to struct cpuinfo_topology

[Mario Limonciello <mario.limonciello@amd.com>]

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@linux.intel.com>
>>> ---
>>>    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.