| Message ID | 20230605170817.9913-5-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Prevent attempting updates known to fail | expand |
On 05/06/2023 6:08 pm, Alejandro Vallejo wrote: > diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c > index 4f60d96d98..a4c123118b 100644 > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -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 }; I don't see how this (the logic in this_cpu_can_install_update()) can work, as ... > break; > } > > @@ -900,6 +923,10 @@ int __init early_microcode_init(unsigned long *module_map, > if ( ucode_mod.mod_end || ucode_blob.size ) > rc = early_microcode_update_cpu(); > > + /* > + * We just updated microcode so we must reload the boot_cpu_data bits > + * we read before because they might be stale after the updata. > + */ > early_read_cpuid_7d0(); > > /* ... MSR_ARCH_CAPS is read out-of-context down here. In hindsight, I think swapping patch 2 and 3 might be wise. The rev == ~0 case doesn't need any of the cpu_has_* shuffling, and it already starts to build up the if/else chain of cases where we decide to clobber the apply_microcode() hook. The call to this_cpu_can_install_update() should be lower down. In principle it's not Intel-specific. ~Andrew
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
On Mon, Jun 12, 2023 at 07:54:00PM +0100, Andrew Cooper wrote: > On 05/06/2023 6:08 pm, Alejandro Vallejo wrote: > > diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c > > index 4f60d96d98..a4c123118b 100644 > > --- a/xen/arch/x86/cpu/microcode/core.c > > +++ b/xen/arch/x86/cpu/microcode/core.c > > @@ -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 }; > > I don't see how this (the logic in this_cpu_can_install_update()) can > work, as ... > > > break; > > } > > > > @@ -900,6 +923,10 @@ int __init early_microcode_init(unsigned long *module_map, > > if ( ucode_mod.mod_end || ucode_blob.size ) > > rc = early_microcode_update_cpu(); > > > > + /* > > + * We just updated microcode so we must reload the boot_cpu_data bits > > + * we read before because they might be stale after the updata. > > + */ > > early_read_cpuid_7d0(); > > > > /* > > ... MSR_ARCH_CAPS is read out-of-context down here. Seeing how the minimal CPU state is read in early_cpu_init() I'll stash the read to MSR_ARCH_CAPS there too. Then it's a matter of reloading potentially changing leafs/MSRs after the update, which is a lot clearer rather than adding reads/writes ad-hoc elsewhere. Alejandro
On Tue, Jun 13, 2023 at 08:57:06AM +0200, Jan Beulich wrote: > 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. Well, before this patch collect_cpu_info() and apply_microcode() were both set or cleared together , whereas after this patch that's no longer the case (hence the change). I can submit it standalone patch earlier in v3, seeing as it does no harm. The commit message could also do with better wording. > > > @@ -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 I'm moving things around a little in v3. We'll have accessors for both AMD and Intel that provide the right thing, encapsulating vendor-specifics (including family checks) inside ${VENDOR}.c Alejandro
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 4f60d96d98..a4c123118b 100644 --- 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); } @@ -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); +} + int __init early_microcode_init(unsigned long *module_map, const struct multiboot_info *mbi) { const struct cpuinfo_x86 *c = &boot_cpu_data; int rc = 0; + early_read_cpuid_7d0(); + switch ( c->x86_vendor ) { case X86_VENDOR_AMD: @@ -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 }; break; } @@ -900,6 +923,10 @@ int __init early_microcode_init(unsigned long *module_map, if ( ucode_mod.mod_end || ucode_blob.size ) rc = early_microcode_update_cpu(); + /* + * We just updated microcode so we must reload the boot_cpu_data bits + * we read before because they might be stale after the updata. + */ early_read_cpuid_7d0(); /* diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h index ace31e3b1f..0118171d7e 100644 --- a/xen/arch/x86/include/asm/cpufeature.h +++ b/xen/arch/x86/include/asm/cpufeature.h @@ -192,6 +192,7 @@ static inline bool boot_cpu_has(unsigned int feat) #define cpu_has_if_pschange_mc_no boot_cpu_has(X86_FEATURE_IF_PSCHANGE_MC_NO) #define cpu_has_tsx_ctrl boot_cpu_has(X86_FEATURE_TSX_CTRL) #define cpu_has_taa_no boot_cpu_has(X86_FEATURE_TAA_NO) +#define cpu_has_mcu_ctrl boot_cpu_has(X86_FEATURE_MCU_CTRL) #define cpu_has_fb_clear boot_cpu_has(X86_FEATURE_FB_CLEAR) /* Synthesized. */ diff --git a/xen/arch/x86/include/asm/msr-index.h b/xen/arch/x86/include/asm/msr-index.h index 2749e433d2..5c1350b5f9 100644 --- a/xen/arch/x86/include/asm/msr-index.h +++ b/xen/arch/x86/include/asm/msr-index.h @@ -165,6 +165,11 @@ #define PASID_PASID_MASK 0x000fffff #define PASID_VALID (_AC(1, ULL) << 31) +#define MSR_MCU_CONTROL 0x00001406 +#define MCU_CONTROL_LOCK (_AC(1, ULL) << 0) +#define MCU_CONTROL_DIS_MCU_LOAD (_AC(1, ULL) << 1) +#define MCU_CONTROL_EN_SMM_BYPASS (_AC(1, ULL) << 2) + #define MSR_UARCH_MISC_CTRL 0x00001b01 #define UARCH_CTRL_DOITM (_AC(1, ULL) << 0)
If IA32_MSR_MCU_CONTROL exists then it's possible a CPU may be unable to perform microcode updates. This is controlled through the DIS_MCU_LOAD bit and is intended for baremetal clouds where the owner may not trust the tenant to choose the microcode version in use. This patch makes sure we only expose the microcode loading interface when it can be actually used, while also allowing reads of current microcode versions. Patch summary: * Read CPUID leaf 7d0 early so DIS_MCU_LOAD can be checked. * Hide microcode loading handlers if DIS_MCU_LOAD is set except for collect_cpu_info() * Update microcode_update_one() so APs can read their microcode version even if DIS_MCU_LOAD is set. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v2: * Moved check from apply time to init time. --- xen/arch/x86/cpu/microcode/core.c | 31 +++++++++++++++++++++++++-- xen/arch/x86/include/asm/cpufeature.h | 1 + xen/arch/x86/include/asm/msr-index.h | 5 +++++ 3 files changed, 35 insertions(+), 2 deletions(-)