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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/06/2023 2:19 pm, Alejandro Vallejo wrote:
> 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.

I meant every AP on boot, where Xen initiates the ucode load.

>
>> 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 __start_xen(),

    tsx_init(); /* Needs microcode.  May change HLE/RTM feature bits. */

If you were to test such a patch, the test-tsx ought to fail on SKL/KBL
amongst others.

One of the things that tsx_init() does is select TSX_CTRL_CPUID_CLEAR
and/or TSX_CPUID_CLEAR, which hides the HLE and RTM bits in regular
CPUID, so wants to run before the general CPUID scan.  This matters for
guest performance - if TSX is actually always aborting, but reported to
the guest, then any library using RTM will be less performant than using
the non-transactional path.

Conversely if the user wants to explicitly re-activate TSX despite the
firmware defaults, those bits need clearing before the CPUID scan for
anything to work.

~Andrew