Re: [PATCH] x86/microcode: Prevent attempting updates known to fail

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Thu, Jun 01, 2023 at 11:54:31AM +0100, Andrew Cooper wrote:
> I had something else in mind here.  Right now, this will read
> MSR_MCU_CONTROL and emit a printk() on every microcode load, which will
> be every AP, and every time the user uses the xen-ucode tool.
Not every AP. The hypercall would return with an error before the APs are
brought in. It is true that the error on dmesg would appear on every
microcode load attempt though.

> 
> Instead, I recommend the following:
> 
> 1) One patch moving the early-cpuid/msr read from tsx_init() into
> early_microcode_init(), adjusting the comment as it goes.  No point
> duplicating that logic, and we need it earlier on boot now.
> 2) This patch, adjusting early_microcode_init() only.  Have a printk()
> saying "microcode loading disabled by firmware" and avoid filling in
> ucode_ops.  Every other part of ucode handling understands "loading not
> available".
Sure. Going on a tangent though, I do wonder why tsx_init() is preceding
identify_cpu(). It's reading cpuid leaf 7d0 simply because it hasn't been
read yet, but it's not obvious why this rush in invoking tsx_init(). I
can't see any obvious marker that affect the following identify_cpu() call,
and swapping them gets rid of the cpuid read.

> 
> In terms of the commit message, you should call out the usecase
> explicitly.  This feature is intended for baremetal clouds where the
> platform owner doesn't trust the tenant to choose the microcode version
> in use.
> 
Sure.

> Also, I'm really not sure what your 3rd paragraph is trying to say.
That the case where CPU_i.DIS_MCU_LOAD != CPU_j.DIS_MCU_LOAD where i != j
is not specifically handled on the grounds that sane firmware ensures that
condition doesn't happen, and we already notify when the system reached a
nonsensical state with different CPUs having different microcode versions.

I'll rewrite it better.

Alejandro