Re: [PATCH v5 2/7] x86/msr: add wrmsrq_on_cpus helper

[Dave Hansen]

On 5/27/26 14:06, Borislav Petkov wrote:
> On Mon, May 18, 2026 at 09:42:15PM +0000, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@xxxxxxx>
>>
>> The existing wrmsr_on_cpus() takes a per-cpu struct msr array, requiring
>> callers to allocate and populate per-cpu storage even when every CPU
>> receives the same value. This is unnecessary overhead for the common
>> case of writing a single uniform u64 to a per-CPU MSR across multiple
>> CPUs.
>>
>> Add wrmsrq_on_cpus() which writes the same u64 value to the specified
>> MSR on all CPUs in the given cpumask.
> 
> So let's add yet another function which name differs from the other one by
> a single letter and have people go look at the implementation to know which is
> which...?
> 
> Instead of unifying what's there and extending this one to do what you want it
> to do?
> 
> And now you have a wrmsrQ_on_cpus() but no rdmsrQ_on_cpus()?
> 
> Because if you look at the code, you'll see how those are used: first you
> rdmsr on CPUs, modify each value and then wrmsr on same CPUs.

This one is my doing.

wrmsr_on_cpus() is kinda a mess. I think it only has a single user. It's
also not very flexible because it needs a 'struct msr __percpu *msrs'
argument where each MSR has a value in memory.

The use case for RMPOPT is that all CPUs get the same value. It'd be a
little awkward to go create a percpu data structure to duplcate the same
value to call wrmsr_on_cpus(). The RMPOPT case is also arguably
performance sensitive since it's done during boot. It should do the IPIs
in parallel.

toggle_ecc_err_reporting(), on the other hand, is done at module init
time. It's not really performance sensitive. It's probably pretty easy
to zap wrmsr_on_cpus() and just have toggle_ecc_err_reporting() do
something slightly less efficient.

Yeah, the

	wrmsr_on_cpus()
	wrmsrq_on_cpus()

naming pain is real. There's little chance of bugs coming from it
because the function signatures are *SO* different. But, it certainly
could confuse humans for a minute.

But the real solution to this is axing wrmsr_on_cpus(). Which I think we
could do after killing its one user which the attached (completely
untested) patch does. The only downside of the patch is that it does
RDMSR via IPIs one CPU at a time. But, looking at the code, I'm not sure
anyone would care. If anyone did, I _think_ all those MSRs have the same
value and the code could be simplified further. But that would take more
than 3 minutes.

It's also possible that my grepping was bad or I'm completely
misunderstanding amd64_edac.c. Cluebat welcome if I'm being dense.

BTW, I also don't feel the need to make Ashish go do any of this edac
cleanup. I think it can just be done in parallel. But I wouldn't stop
him if he volunteered.

---

 b/drivers/edac/amd64_edac.c |   35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff -puN drivers/edac/amd64_edac.c~axe-wrmsr_on_cpus drivers/edac/amd64_edac.c
--- a/drivers/edac/amd64_edac.c~axe-wrmsr_on_cpus	2026-05-27 14:25:07.881694152 -0700
+++ b/drivers/edac/amd64_edac.c	2026-05-27 14:33:03.278139821 -0700
@@ -14,8 +14,6 @@ static struct edac_pci_ctl_info *pci_ctl
 static int ecc_enable_override;
 module_param(ecc_enable_override, int, 0644);
 
-static struct msr __percpu *msrs;
-
 static inline u32 get_umc_reg(struct amd64_pvt *pvt, u32 reg)
 {
 	if (!pvt->flags.zn_regs_v2)
@@ -3215,14 +3213,14 @@ static bool nb_mce_bank_enabled_on_node(
 
 	get_cpus_on_this_dct_cpumask(mask, nid);
 
-	rdmsr_on_cpus(mask, MSR_IA32_MCG_CTL, msrs);
-
 	for_each_cpu(cpu, mask) {
-		struct msr *reg = per_cpu_ptr(msrs, cpu);
-		nbe = reg->l & MSR_MCGCTL_NBE;
+		u64 mcg_ctl;
+
+		rdmsrq_on_cpu(cpu, MSR_IA32_MCG_CTL, &mcg_ctl);
+		nbe = mcg_ctl & MSR_MCGCTL_NBE;
 
 		edac_dbg(0, "core: %u, MCG_CTL: 0x%llx, NB MSR is %s\n",
-			 cpu, reg->q, str_enabled_disabled(nbe));
+			 cpu, mcg_ctl, str_enabled_disabled(nbe));
 
 		if (!nbe)
 			goto out;
@@ -3246,26 +3244,25 @@ static int toggle_ecc_err_reporting(stru
 
 	get_cpus_on_this_dct_cpumask(cmask, nid);
 
-	rdmsr_on_cpus(cmask, MSR_IA32_MCG_CTL, msrs);
-
 	for_each_cpu(cpu, cmask) {
+		u64 mcg_ctl;
 
-		struct msr *reg = per_cpu_ptr(msrs, cpu);
+		rdmsrq_on_cpu(cpu, MSR_IA32_MCG_CTL, &mcg_ctl);
 
 		if (on) {
-			if (reg->l & MSR_MCGCTL_NBE)
+			if (mcg_ctl & MSR_MCGCTL_NBE)
 				s->flags.nb_mce_enable = 1;
 
-			reg->l |= MSR_MCGCTL_NBE;
+			mcg_ctl |= MSR_MCGCTL_NBE;
 		} else {
 			/*
 			 * Turn off NB MCE reporting only when it was off before
 			 */
 			if (!s->flags.nb_mce_enable)
-				reg->l &= ~MSR_MCGCTL_NBE;
+				mcg_ctl &= ~MSR_MCGCTL_NBE;
 		}
+		wrmsrq_on_cpu(cpu, MSR_IA32_MCG_CTL, mcg_ctl);
 	}
-	wrmsr_on_cpus(cmask, MSR_IA32_MCG_CTL, msrs);
 
 	free_cpumask_var(cmask);
 
@@ -4153,10 +4150,6 @@ static int __init amd64_edac_init(void)
 	if (!ecc_stngs)
 		goto err_free;
 
-	msrs = msrs_alloc();
-	if (!msrs)
-		goto err_free;
-
 	for (i = 0; i < amd_nb_num(); i++) {
 		err = probe_one_instance(i);
 		if (err) {
@@ -4190,9 +4183,6 @@ static int __init amd64_edac_init(void)
 err_pci:
 	pci_ctl_dev = NULL;
 
-	msrs_free(msrs);
-	msrs = NULL;
-
 err_free:
 	kfree(ecc_stngs);
 	ecc_stngs = NULL;
@@ -4220,9 +4210,6 @@ static void __exit amd64_edac_exit(void)
 	ecc_stngs = NULL;
 
 	pci_ctl_dev = NULL;
-
-	msrs_free(msrs);
-	msrs = NULL;
 }
 
 module_init(amd64_edac_init);
_