Re: [PATCH v2 3/3] x86/ucode: Drop MIS_UCODE and microcode_match_result

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/11/2024 8:02 am, Jan Beulich wrote:
> On 14.11.2024 18:18, Andrew Cooper wrote:
>> On 14/11/2024 11:41 am, Jan Beulich wrote:
>>> On 12.11.2024 22:19, Andrew Cooper wrote:
>>>> @@ -199,8 +198,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *patch)
>>>>      return equiv.id == patch->processor_rev_id;
>>>>  }
>>>>  
>>>> -static enum microcode_match_result cf_check compare_patch(
>>>> -    const struct microcode_patch *new, const struct microcode_patch *old)
>>>> +static int cf_check compare_patch(
>>>> +    const struct microcode_patch *old, const struct microcode_patch *new)
>>>>  {
>>> Let's hope we won't screw up a backport because of this swapping.
>> I wasn't going to start thinking about backports until the code gets
>> into a better state.
>>
>> But if backports do happen, it will be all-or-nothing.  This code is far
>> too tangled.
> I wasn't so much worrying about backporting of this work (as of now I don't
> think it's a candidate), but anything that's yet to come.

This work is towards supporting the Intel Min-Rev header, because it's
already deployed into the world for several releases, and is also what
is likely to drive a wish for backports.

Then there's the Intel uniform loading extensions, which are needed for
GNR/SRF.  If nothing else we need to be able to parse the loading-scope
and not get surprised when cross-core loading happens.  (This already
happens since Sky Lake if SGX is active, and Intel were surprised when I
noticed and asked them about it.)


But mostly, the pre-existing logic is just irrationally complex for
something so simple.  Most of the complexity appears to be because it
was too complex to start with.

>
>> That said, in this specific case, the only thing that would go wrong is
>> with Intel debug patches.  Even I've only had a handful of those in the
>> past 8 years.
> Why would that be? Doing the check the wrong way round would lead to
> possible downgrading of ucode, wouldn't it?

After this patch, there is a singular use of the hook.

It is comparing the hypercall-provided blob to the cached blob, yielding
OLD/SAME/NEW.

Deciding to initiate patching (entering stop_machine() context) is based
on !cached || NEW || --force.

There is another check in apply_microcode() (this is why we needed to
plumb --force down), which will catch an accidental swapping of the two
arguments.

Something that we don't handle properly is that we use "I have a cached
blob" as if it means "the system is at a consistent level", but this is
not true in both directions.  We might have not had anything to cache on
boot (AMD Client platforms in particular), and what we had on boot may
not have levelled a system which was left asymmetric by the BIOS.
>>> I'd like to ask to at least consider renaming at least the functions,
>>> perhaps also the hook pointer, perhaps simply by switching from singular
>>> to plural. This would then also avoid reviewers like me to go hunt for
>>> all uses of the function/hook, in an attempt to make sure none was left
>>> out when converting.
>> In the other series I've paused for a while, I have renamed some hooks
>> (along with related cleanup), but I'm undecided on this one.
>>
>> One option is cmp(), or perhaps compare().
> Either would be fine with me as a hook name. As a function name I'm less
> certain this will (remain to) be unambiguous.
>
>> But, it occurs to me, another option would be is_newer().  We always
>> care about the operation one way around.
> is_newer() doesn't very well lend itself to a tristate return value.

Fine.  I'll just go with compare().  I don't expect this will be the
last time it's edited.

~Andrew