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

["Gautham R. Shenoy" <gautham.shenoy@amd.com>]

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() ?

--
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
>