[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <97d77c5f-eb99-4c82-9b58-9783060c2810@intel.com>
Date: Wed, 26 Mar 2025 11:43:58 -0700
From: "Chang S. Bae" <chang.seok.bae@...el.com>
To: Chao Gao <chao.gao@...el.com>
CC: <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <tglx@...utronix.de>,
<mingo@...hat.com>, <bp@...en8.de>, <dave.hansen@...ux.intel.com>,
<colinmitchell@...gle.com>
Subject: Re: [PATCH v2a 3/6] x86/microcode/intel: Establish staging control
logic
On 3/26/2025 12:35 AM, Chao Gao wrote:
>> + for_each_cpu(cpu, cpu_online_mask) {
>
> for_each_online_cpu(cpu)?
Yes, it looks equivalent but shorter.
> So, how about:
>
> if (cpu != cpumask_first(topology_core_cpumask(cpu)))
> continue;
>
> and dropping the pkg_id?
No, the pkg_id check is intentional to prevent duplicate staging within
a package. As noted in the comment: "The MMIO address is unique per
package."
>> + rdmsrl_on_cpu(cpu, MSR_IA32_MCU_STAGING_MBOX_ADDR, &mmio_pa);
>
> Note rdmsrl_on_cpu() may return an error. please consider adding
> error-handling. Is it possible that somehow one package doesn't support
> this staging feature while others do?
rdmsrl_on_cpu() -> smp_call_function_single() -> generic_exec_single():
if ((unsigned)cpu >= nr_cpu_ids || !cpu_online(cpu)) {
csd_unlock(csd);
return -ENXIO;
}
This error condition applies to an invalid cpu, but since the function
is guarded by cpu_online_mask, it should not occur.
That said though, ignoring the return value may appear to be incorrect.
Perhaps,
err = rdmsrl_on_cpu(cpu, MSR_IA32_MCU_STAGING_MBOX_ADDR, &mmio_pa);
if (WARN_ON_ONCE(err))
return;
> Shall we print a message somewhere showing "Continuing updates without
> staging"?
>
> It could be confusing for users to see a success message following an error
> message that states "Error: staging failed ..."
This function already prints either a success or failure message based
on staging results which are variable.
But this behavior follows the established policy that loading should
continue even if staging fails, which is a known and invariant behavior
at runtime.
So, explicitly stating that updates will proceed without staging seems
redundant and could be considered noise.
Thanks,
Chang
Powered by blists - more mailing lists