[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z+StZt9u+p0GjZHR@intel.com>
Date: Thu, 27 Mar 2025 09:44:06 +0800
From: Chao Gao <chao.gao@...el.com>
To: "Chang S. Bae" <chang.seok.bae@...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 Wed, Mar 26, 2025 at 11:43:58AM -0700, Chang S. Bae wrote:
>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."
The check I suggested can also achieve the goal and is simpler, right?
>
>> > + 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.
Ok. I misread rdmsrl_on_cpu(). I thought it would return an error if the
MSR doesn't exist on that CPU. But that's not the case.
>
>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;
Looks good.
Powered by blists - more mailing lists