Re: [PATCH v2 1/3] x86/ucode: Remove the collect_cpu_info() call from parse_blob()

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

On 14/11/2024 11:11 am, Jan Beulich wrote:
> On 12.11.2024 22:19, Andrew Cooper wrote:
>> With the tangle of logic starting to come under control, it is now plain to
>> see that parse_blob()'s side effect of re-gathering the signature/revision is
>> pointless.
>>
>> The cpu_request_microcode() hooks need the signature only.  The BSP gathers
>> this in early_microcode_init(), the APs and S3 in microcode_update_cpu().
> That's microcode_update_one() after 502478bc1d9d if I'm not mistaken.

I wouldn't necessarily say "after".

microcode_update_cpu() has been the way the APs and S3 get this
information for ages, whether its in the function directly, or in an
immediate callee.

>  In the
> course of determining that I'm afraid I also found the first sentence of this
> paragraph rather misleading than helpful:

Do you mean "The cpu_request_microcode() hooks need the signature only" ?


> While it is true what is being said,
> in both cases it is collect_cpu_info() that is being invoked, retrieving both
> signature and revision. IOW logic needing the signature only doesn't really
> matter here (and the sentence made me hunt for cases where we would read just
> the signature, aiming at verifying that leaving the revision field unset
> would indeed not be a problem).

It probably doesn't come as a surprise that I'm intending to rework
collect_cpu_info() entirely.  It's a mess.

The signature and platform flags are invariants for a CPU.  (In fact,
Platform Flags had better be the same for an entire system).  The
revision does change with type, but apply_microcode() keeps it up to date.

Yet we had logic which was throwing the details away and re-gathering
(which is quite expensive) for basically every microcode operation.


What I'm trying to express is "this information is collected once at the
start of day, and kept up to date, so collect_cpu_info() should not be
called under any other circumstance".

Perhaps I should just say that directly?



>>  For
>> good measure, the apply_microcode() hooks also keep the revision correct as
>> load attempts are made.
>>
>> This finally gets us down to a single call per CPU on boot / S3 resume, and no
>> calls during late-load hypercalls.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Preferably with the problematic sentence dropped or clarified:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.