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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/06/2023 11:54 am, Andrew Cooper wrote:
> 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".

So, having fallen over "x86/ucode: Exit early from early_update_cache()
if loading not available" for other reasons, I've realised that this
isn't a completely sensible suggestion.

By not filling in ucode_ops, nothing ever calls collect_info(), meaning
that external components which peek at this_cpu(cpu_sig).rev get 0's
back in place of the actual microcode revision.  That's probably the
best we can do for genuinely no ucode facilities available.

But there's another case we ought to cope with.  Some hypervisors
deliberately report a microcode revision of ~0, and we should take to
mean "no microcode loading available" too.

For this MCU_CONTROL_DIS_MCU_LOAD case, we don't want to be trying to
load new microcode because that's a waste of time, but we absolutely
should query the current microcode revision.  It is frequently relevant
for security reasons.

So I think we want to fine-grain things a little, and separate the
concepts of "ucode info available" and "ucode loading available".  Per
the current mechanism, that would involve supporting a case where
ucode_ops.collect_cpu_info() is available but
ucode_ops.apply_microcode() is not.

~Andrew

P.S. also in our copious free time, we need to start supporting the
Intel min_rev field, which is more complicated than it sounds.

min_rev is vaguely defined as being relevant to block updates "after
you've evaluated CPUID and made decisions based on it", but here in Xen
we do also do livepatching and late loading to explicitly make use of
newly enumerated features.

So we need a way of xen-ucode saying "please really do load this,
because I as the admin think it will be fine in combination with the
livepatch I'm about to apply".

My best idea for this is to have a `--force` option to pass to Xen to
skip the revision checks, which will require either a new hypercall, or
perhaps borrowing a high bit from the size field in the current hypercall.

With a force option in place, the boot time ucode=allow-same can go
away.  It has become distinctly less useful now that we were forced do
this unilaterally on AMD CPUs, and separating "allow same because of HW
bugs" from "the Admin promised they knew what they were doing" would be
better for testing.