Re: [PATCH v5 4/4] x86/microcode: Disable microcode update handler if DIS_MCU_UPDATE is set

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.07.2023 16:03, Alejandro Vallejo wrote:
> On Wed, Jul 05, 2023 at 12:51:47PM +0200, Jan Beulich wrote:
>>> --- a/xen/arch/x86/cpu/microcode/intel.c
>>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>>> @@ -385,6 +385,19 @@ static struct microcode_patch *cf_check 
>>> cpu_request_microcode(
>>>      return patch;
>>>  }
>>>  
>>> +bool __init intel_can_load_microcode(void)
>>> +{
>>> +    uint64_t mcu_ctrl;
>>> +
>>> +    if ( !cpu_has_mcu_ctrl )
>>> +        return true;
>>> +
>>> +    rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
>>
>> While one would hope that feature bit and MSR access working come in
>> matched pairs, I still wonder whether - just to be on the safe side -
>> the caller wouldn't better avoid calling here when rev == ~0 (and
>> hence we won't try to load ucode anyway). I would envision can_load's
>> initializer to become this_cpu(cpu_sig).rev != ~0, with other logic
>> adjusted as necessary in early_microcode_init().
>>
> We only know about the ucode revision after the collect_cpu_info() call,
> and we can only make that call after the vendor-specific section that sets
> the function pointers up (and calls intel_can_load_microcode()).

Hmm, right, that wasn't quite visible from looking at patch and
current tree, because of what earlier patches in the series do.

> One could imagine turning can_load into a function pointer so that its
> execution is deferred until after the revision check (and skipped
> altogether if `rev==~0`).

Perhaps not worth going this far, and instead stay with what you
have until we know (if ever) that further tweaking is necessary.

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
(maybe with an adjustment to the comment, as mentioned in the
earlier reply)

Jan