Re: [PATCH v4 10/13] x86/process: Clear hardware feedback history for AMD processors

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

On 10/22/2024 01:28, Gautham R. Shenoy wrote:
> Hello Mario,
> 
> On Mon, Oct 21, 2024 at 01:02:49PM -0500, Mario Limonciello wrote:
>> From: Perry Yuan <perry.yuan@amd.com>
>>
>> Incorporate a mechanism within the context switching code to reset
>> the hardware history for AMD processors. Specifically, when a task
>> is switched in, the class ID was read and reset the hardware workload
>> classification history of CPU firmware and then it start to trigger
>> workload classification for the next running thread.
>>
>> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
>> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   arch/x86/include/asm/hreset.h |  6 ++++++
>>   arch/x86/kernel/cpu/common.c  | 15 +++++++++++++++
>>   arch/x86/kernel/process_32.c  |  3 +++
>>   arch/x86/kernel/process_64.c  |  3 +++
>>   4 files changed, 27 insertions(+)
>>   create mode 100644 arch/x86/include/asm/hreset.h
>>
>> diff --git a/arch/x86/include/asm/hreset.h b/arch/x86/include/asm/hreset.h
>> new file mode 100644
>> index 0000000000000..ae1f72602bbd0
>> --- /dev/null
>> +++ b/arch/x86/include/asm/hreset.h
>> @@ -0,0 +1,6 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_X86_HRESET_H
>> +
>> +void reset_hardware_history_hetero(void);
>> +
>> +#endif /* _ASM_X86_HRESET_H */
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index 07a34d7235057..658c8fb4e25df 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -57,6 +57,7 @@
>>   #include <asm/mce.h>
>>   #include <asm/msr.h>
>>   #include <asm/cacheinfo.h>
>> +#include <asm/hreset.h>
>>   #include <asm/memtype.h>
>>   #include <asm/microcode.h>
>>   #include <asm/intel-family.h>
>> @@ -403,6 +404,7 @@ static const unsigned long cr4_pinned_mask = X86_CR4_SMEP | X86_CR4_SMAP | X86_C
>>   					     X86_CR4_FSGSBASE | X86_CR4_CET | X86_CR4_FRED;
>>   static DEFINE_STATIC_KEY_FALSE_RO(cr_pinning);
>>   static unsigned long cr4_pinned_bits __ro_after_init;
>> +static DEFINE_STATIC_KEY_FALSE_RO(hardware_history_features);
>>   
>>   void native_write_cr0(unsigned long val)
>>   {
>> @@ -481,6 +483,12 @@ void cr4_init(void)
>>   	this_cpu_write(cpu_tlbstate.cr4, cr4);
>>   }
>>   
>> +static void __init setup_hreset(struct cpuinfo_x86 *c)
>> +{
>> +	if (cpu_feature_enabled(X86_FEATURE_AMD_WORKLOAD_CLASS))
>> +		static_key_enable_cpuslocked(&hardware_history_features.key);
>> +}
>> +
>>   /*
>>    * Once CPU feature detection is finished (and boot params have been
>>    * parsed), record any of the sensitive CR bits that are set, and
>> @@ -1844,6 +1852,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>>   	setup_smep(c);
>>   	setup_smap(c);
>>   	setup_umip(c);
>> +	setup_hreset(c);
> 
> Since setup_hreset() just enables the static key once when the
> AMD_WORKLOAD_CLASS feature is enabled, does it make sense to call
> setup_hreset() in identify_boot_cpu() instead of in identify_cpu() ?
> 

Good suggestion.  I'll change this for the next revision.

> --
> Thanks and Regards
> gautham.
> 
>>   
>>   	/* Enable FSGSBASE instructions if available. */
>>   	if (cpu_has(c, X86_FEATURE_FSGSBASE)) {
>> @@ -2410,3 +2419,9 @@ void __init arch_cpu_finalize_init(void)
>>   	 */
>>   	mem_encrypt_init();
>>   }
>> +
>> +__always_inline void reset_hardware_history_hetero(void)
>> +{
>> +	if (static_branch_unlikely(&hardware_history_features))
>> +		wrmsrl(AMD_WORKLOAD_HRST, 0x1);
>> +}
>> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
>> index 0917c7f25720b..6a3a1339f7a77 100644
>> --- a/arch/x86/kernel/process_32.c
>> +++ b/arch/x86/kernel/process_32.c
>> @@ -52,6 +52,7 @@
>>   #include <asm/switch_to.h>
>>   #include <asm/vm86.h>
>>   #include <asm/resctrl.h>
>> +#include <asm/hreset.h>
>>   #include <asm/proto.h>
>>   
>>   #include "process.h"
>> @@ -213,6 +214,8 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
>>   	/* Load the Intel cache allocation PQR MSR. */
>>   	resctrl_sched_in(next_p);
>>   
>> +	reset_hardware_history_hetero();
>> +
>>   	return prev_p;
>>   }
>>   
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 226472332a70d..ea7f765c6262a 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -54,6 +54,7 @@
>>   #include <asm/xen/hypervisor.h>
>>   #include <asm/vdso.h>
>>   #include <asm/resctrl.h>
>> +#include <asm/hreset.h>
>>   #include <asm/unistd.h>
>>   #include <asm/fsgsbase.h>
>>   #include <asm/fred.h>
>> @@ -709,6 +710,8 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
>>   	/* Load the Intel cache allocation PQR MSR. */
>>   	resctrl_sched_in(next_p);
>>   
>> +	reset_hardware_history_hetero();
>> +
>>   	return prev_p;
>>   }
>>   
>> -- 
>> 2.43.0
>>