[PATCH 3/3] x86/amd: Delay amd_init_levelling() until after fixes to the CPUID MSRs

Andrew Cooper posted 3 patches 1 week, 3 days ago
[PATCH 3/3] x86/amd: Delay amd_init_levelling() until after fixes to the CPUID MSRs
Posted by Andrew Cooper 1 week, 3 days ago
There's no need for amd_init_levelling() to be specifically early.  In fact,
it must be after init_amd() edits the feature MSRs, e.g. enabling TOPOEXT on
Fam15h, or we revert the change on the next context switch.

As the only action in early_init_amd(), drop the function entirely.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/amd.c   | 16 ++++++----------
 xen/arch/x86/cpu/cpu.h   |  2 +-
 xen/arch/x86/cpu/hygon.c |  6 +++++-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 73f43b0f9174..b66601b3c51e 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -224,7 +224,7 @@ static const typeof(ctxt_switch_masking) __initconst_cf_clobber __used csm =
  * avoid this, as the accidentally-advertised features will not actually
  * function.
  */
-static void __init noinline amd_init_levelling(void)
+void __init amd_init_levelling(void)
 {
 	/*
 	 * If there's support for CpuidUserDis or CPUID faulting then
@@ -617,14 +617,6 @@ void amd_process_freq(const struct cpuinfo_x86 *c,
 		*low_mhz = amd_parse_freq(c->x86, lo);
 }
 
-void cf_check early_init_amd(struct cpuinfo_x86 *c)
-{
-	if (c == &boot_cpu_data)
-		amd_init_levelling();
-
-	ctxt_switch_levelling(NULL);
-}
-
 void amd_log_freq(const struct cpuinfo_x86 *c)
 {
 	unsigned int low_mhz = 0, nom_mhz = 0, hi_mhz = 0;
@@ -1270,10 +1262,14 @@ static void cf_check init_amd(struct cpuinfo_x86 *c)
 	check_syscfg_dram_mod_en();
 
 	amd_log_freq(c);
+
+	if (c == &boot_cpu_data)
+		amd_init_levelling(); /* After CPUID MSR adjustments. */
+
+	ctxt_switch_levelling(NULL);
 }
 
 const struct cpu_dev __initconst_cf_clobber amd_cpu_dev = {
-	.c_early_init	= early_init_amd,
 	.c_init		= init_amd,
 };
 
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 8bed3f52490f..c05c620c29bc 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -22,9 +22,9 @@ extern void display_cacheinfo(struct cpuinfo_x86 *c);
 extern void detect_ht(struct cpuinfo_x86 *c);
 extern bool detect_extended_topology(struct cpuinfo_x86 *c);
 
-void cf_check early_init_amd(struct cpuinfo_x86 *c);
 void amd_log_freq(const struct cpuinfo_x86 *c);
 void amd_init_de_cfg(const struct cpuinfo_x86 *c);
+void amd_init_levelling(void);
 void amd_init_lfence_dispatch(void);
 void amd_init_ssbd(const struct cpuinfo_x86 *c);
 void amd_init_spectral_chicken(void);
diff --git a/xen/arch/x86/cpu/hygon.c b/xen/arch/x86/cpu/hygon.c
index cab915a2fcf1..51104cbaf42c 100644
--- a/xen/arch/x86/cpu/hygon.c
+++ b/xen/arch/x86/cpu/hygon.c
@@ -90,9 +90,13 @@ static void cf_check init_hygon(struct cpuinfo_x86 *c)
 	}
 
 	amd_log_freq(c);
+
+	if (c == &boot_cpu_data)
+		amd_init_levelling(); /* After CPUID MSR adjustments. */
+
+	ctxt_switch_levelling(NULL);
 }
 
 const struct cpu_dev __initconst_cf_clobber hygon_cpu_dev = {
-	.c_early_init	= early_init_amd,
 	.c_init		= init_hygon,
 };
-- 
2.39.5


Re: [PATCH 3/3] x86/amd: Delay amd_init_levelling() until after fixes to the CPUID MSRs
Posted by Jan Beulich 4 days, 9 hours ago
On 02.12.2025 11:57, Andrew Cooper wrote:
> There's no need for amd_init_levelling() to be specifically early.  In fact,
> it must be after init_amd() edits the feature MSRs, e.g. enabling TOPOEXT on
> Fam15h, or we revert the change on the next context switch.

However, ...

> @@ -1270,10 +1262,14 @@ static void cf_check init_amd(struct cpuinfo_x86 *c)
>  	check_syscfg_dram_mod_en();
>  
>  	amd_log_freq(c);
> +
> +	if (c == &boot_cpu_data)
> +		amd_init_levelling(); /* After CPUID MSR adjustments. */
> +
> +	ctxt_switch_levelling(NULL);
>  }

... this new placement conflicts with the two RDSEED patches which have been
pending for a while / too long. Even moving up wouldn't help, as the TOPOEXT
re-enabling is after the switch() that the RDSEED changes are being fit into.
Surely I could re-base accordingly, but it kind of feels that the older
changes should go in first, with whatever adjustments necessary done either
here, or (in a preparatory and agreed upon manner) right there, or entirely
independently.

Looks like it would be possible to move the TOPOEXT re-enabling ahead of that
very switch(), for the code above then to be inserted between that and said
switch().

Jan