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 <jbeul...@suse.com>
(maybe with an adjustment to the comment, as mentioned in the
earlier reply)
Jan
