On 05.06.2023 19:08, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -749,11 +749,12 @@ __initcall(microcode_init);
> /* Load a cached update to current cpu */
> int microcode_update_one(void)
> {
> + if ( ucode_ops.collect_cpu_info )
> + alternative_vcall(ucode_ops.collect_cpu_info);
> +
> if ( !ucode_ops.apply_microcode )
> return -EOPNOTSUPP;
>
> - alternative_vcall(ucode_ops.collect_cpu_info);
> -
> return microcode_update_cpu(NULL);
> }
This adjustment (and related logic below) doesn't really fit the purpose
that the title states. I wonder if the two things wouldn't better be
split.
> @@ -849,12 +850,25 @@ static void __init early_read_cpuid_7d0(void)
> = cpuid_count_edx(7, 0);
> }
>
> +static bool __init this_cpu_can_install_update(void)
> +{
> + uint64_t mcu_ctrl;
> +
> + if ( !cpu_has_mcu_ctrl )
> + return true;
> +
> + rdmsrl(MSR_MCU_CONTROL, mcu_ctrl);
> + return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
> +}
As Andrew says, in principle AMD could have a means as well. Surely that
would be a different one, so I wonder if this shouldn't be a new hook.
> @@ -871,6 +885,15 @@ int __init early_microcode_init(unsigned long
> *module_map,
> * present.
> */
> ucode_ops = intel_ucode_ops;
> +
> + /*
> + * In the case where microcode updates are blocked by the
> + * DIS_MCU_LOAD bit we can still read the microcode version even if
> + * we can't change it.
> + */
> + if ( !this_cpu_can_install_update() )
> + ucode_ops = (struct microcode_ops){ .collect_cpu_info =
> + intel_ucode_ops.collect_cpu_info };
Similarly I'm not happy to see a direct reference to some vendor specific
field. I think it wants to be the hook to return such an override struct.
Jan
