Re: [PATCH RFC 4/7] x86/microcode/intel: Prepare for microcode staging

[Chang S. Bae]

On 11/6/2024 5:12 PM, Thomas Gleixner wrote:
> This looks all overly complicated. The documentation says:
>
>    "There is one set of mailbox registers and internal staging buffers per
>     physical processor package. Therefore, the IA32_MCU_STAGING_MBOX_ADDR
>     MSR is package-scoped and will provide a different physical address on
>     each physical package."
>
> So why going through loops and hoops?

While the initial approach was like the one below, the aspect I bought 
was to avoid relying on topology knowledge by simply searching for 
unique addresses. But you're right -- this introduces unnecessary loops, 
complicating the code anyway.

I should have explicitly highlighted this as a review point, so thank 
you for taking a closer look and correcting the approach.

Given that the scope is clearly stated in the spec as packaged-scoped 
and should be permanent, I think there is no question in leveraging it 
to make the code simple as you suggested:

>     pkg_id = UINT_MAX;
>
>     for_each_online_cpu(cpu) {
>     	if (topology_logical_package_id(cpu) == pkg_id)
>                continue;
>          pkg_id = topology_logical_package_id(cpu);
>
>          rdmsrl_on_cpu(cpu, MSR_IA32_MCU_STAGING_MBOX_ADDR, &pa);
>          staging_work(pa, ucode_patch_late, totalsize);
>     }
>
> Something like that should just work, no?

Yes, indeed.

Then I observed the package number repeated according to the CPU number 
mapping. For example, all SMT sibling threads map in the later CPU numbers.

Unless staging redundancy is intentional, it can be avoided by tracking 
package ids. But, in this case, the loop may be streamlined to SMT 
primary threads instead of all online CPUs, since core::setup_cpus() 
already ensures primary threads are online. Perhaps,

    /*
     * The MMIO address is unique per package, and all the SMT
     * primary threads are ensured online. Find staging addresses
     * by their package ids.
     */
    for_each_cpu(cpu, cpu_primary_thread_mask) {
        ...
    }

Thanks,
Chang