Re: [PATCH 2/3] x86/ucode: Fold microcode_update_cpu() and fix error handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.11.2024 13:55, Andrew Cooper wrote:
> On 12/11/2024 10:45 am, Jan Beulich wrote:
>> On 07.11.2024 13:21, Andrew Cooper wrote:
>>> Fold microcode_update_cpu() into its single remaining caller and simplify 
>>> the
>>> logic by removing the patch != NULL path with microcode_mutex held.
>>>
>>> Explain why we bother grabbing the microcode revision even if we can't load
>>> microcode.
>>>
>>> Furthermore, delete the -EIO path.  An error updating microcode on AP boot 
>>> or
>>> S3 resume is certainly bad, but freeing the cache is about the worst 
>>> possible
>>> action we can take in response; it prevents subsequent APs from taking an
>>> update they might have accepted.
>> I'm afraid I disagree here, but I also disagree with the present error 
>> handling.
>> -EIO indicates the patch didn't apply. Why would there be any hope that any
>> other CPU would accept it?
> 
> -EIO is "something went wrong".
> 
> On modern systems this can include "checksum didn't match because
> there's a bad SRAM cell".  This is literally one of the failures leading
> to the introduction of In-Field-Scan.
> 
> Individual cores really can fail in a way which won't be the same
> elsewhere in the system.

Hmm, well, slightly hesitantly
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Ideally with a remark added to the description that there is known room
for further improvement.

>> Keeping what's cached might be an option, but then followed by cleaning the
>> cache unless at least one CPU actually accepted the ucode.
> 
> We already have that behaviour.
> 
> 
> We cache speculatively on boot, even if the BSP doesn't need to load,
> because APs might need to.  This really is the best we can do.

That's a different scenario. If we ended up with ucode which no single
CPU accepts, there's hardly much point in caching that ucode. This
specifically is meant not to include the case where simply all CPUs are
already up-to-date. The one largely theoretical case where caching may
still make sense is for CPU hotplug, where the hot-plugged CPU(s) may
accept what all boot-time CPUs refused.

Jan