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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dcb6d46d-c34d-4921-a881-ad8705d7129c@intel.com>
Date: Fri, 8 Nov 2024 14:42:58 -0800
From: "Chang S. Bae" <chang.seok.bae@...el.com>
To: Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>
CC: <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <mingo@...hat.com>,
	<dave.hansen@...ux.intel.com>
Subject: Re: [PATCH RFC 4/7] x86/microcode/intel: Prepare for microcode
 staging

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