| Message ID | 20230615154834.959-6-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Prevent attempting updates known to fail | expand |
On 15.06.2023 17:48, Alejandro Vallejo wrote: > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -352,6 +352,11 @@ void __init early_cpu_init(void) > &c->x86_capability[FEATURESET_7c0], > &c->x86_capability[FEATURESET_7d0]); > > + if (test_bit(X86_FEATURE_ARCH_CAPS, c->x86_capability)) > + rdmsr(MSR_ARCH_CAPABILITIES, > + c->x86_capability[FEATURESET_m10Al], > + c->x86_capability[FEATURESET_m10Ah]); Is this still going to be needed if early_microcode_init() uniformly does this, for things to be correct for tsx_init() (as pointed out in the other patch)? > --- a/xen/arch/x86/cpu/microcode/intel.c > +++ b/xen/arch/x86/cpu/microcode/intel.c > @@ -387,8 +387,22 @@ static struct microcode_patch *cf_check cpu_request_microcode( > > void __init intel_get_ucode_ops(struct microcode_ops *ops) > { > + uint64_t mcu_ctrl; > + > ops->cpu_request_microcode = cpu_request_microcode; > ops->collect_cpu_info = collect_cpu_info; > ops->apply_microcode = apply_microcode; > ops->compare_patch = compare_patch; > + > + if ( cpu_has_mcu_ctrl ) > + { > + rdmsrl(MSR_MCU_CONTROL, mcu_ctrl); > + /* > + * If DIS_MCU_LOAD is set applying microcode updates won't work. We > + * can still query the current version and things like that, so > + * we'll leave the other handlers in place. > + */ > + if ( mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD ) > + ops->apply_microcode = NULL; > + } While this won't address Andrew's request (afaict), but only the cf_clobber requirement, I think you want to drop removing of the struct instances from patch 2, return pointers from the new per-vendor functions, and simply introduce another static instance of struct microcode_ops here, with the one hook simply left unset. This lives in init data, so the size increase is of no major concern. Jan
On Tue, Jun 20, 2023 at 11:51:00AM +0200, Jan Beulich wrote: > On 15.06.2023 17:48, Alejandro Vallejo wrote: > > --- a/xen/arch/x86/cpu/common.c > > +++ b/xen/arch/x86/cpu/common.c > > @@ -352,6 +352,11 @@ void __init early_cpu_init(void) > > &c->x86_capability[FEATURESET_7c0], > > &c->x86_capability[FEATURESET_7d0]); > > > > + if (test_bit(X86_FEATURE_ARCH_CAPS, c->x86_capability)) > > + rdmsr(MSR_ARCH_CAPABILITIES, > > + c->x86_capability[FEATURESET_m10Al], > > + c->x86_capability[FEATURESET_m10Ah]); > > Is this still going to be needed if early_microcode_init() uniformly > does this, for things to be correct for tsx_init() (as pointed out > in the other patch)? Yes. This is needed so MSR_ARCH_CAPS are available in order to check DIS_MCU_LOAD before the microcode update itself. early_cpu_init() does the read before the microcode update, while early_microcode_init() refreshes the fields that might have been modified after the update AND are needed before the general CPU detection logic. > > > --- a/xen/arch/x86/cpu/microcode/intel.c > > +++ b/xen/arch/x86/cpu/microcode/intel.c > > @@ -387,8 +387,22 @@ static struct microcode_patch *cf_check cpu_request_microcode( > > > > void __init intel_get_ucode_ops(struct microcode_ops *ops) > > { > > + uint64_t mcu_ctrl; > > + > > ops->cpu_request_microcode = cpu_request_microcode; > > ops->collect_cpu_info = collect_cpu_info; > > ops->apply_microcode = apply_microcode; > > ops->compare_patch = compare_patch; > > + > > + if ( cpu_has_mcu_ctrl ) > > + { > > + rdmsrl(MSR_MCU_CONTROL, mcu_ctrl); > > + /* > > + * If DIS_MCU_LOAD is set applying microcode updates won't work. We > > + * can still query the current version and things like that, so > > + * we'll leave the other handlers in place. > > + */ > > + if ( mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD ) > > + ops->apply_microcode = NULL; > > + } > > While this won't address Andrew's request (afaict), but only the > cf_clobber requirement, I think you want to drop removing of the struct > instances from patch 2, return pointers from the new per-vendor > functions, and simply introduce another static instance of struct > microcode_ops here, with the one hook simply left unset. This lives in > init data, so the size increase is of no major concern. > > Jan As mentioned in other email. This is my bad for not having understood the ultimate readon for cf_clobber. I'll just leave the structs as is. Alejandro
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index cfcdaace12..2f895e7c7c 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -352,6 +352,11 @@ void __init early_cpu_init(void) &c->x86_capability[FEATURESET_7c0], &c->x86_capability[FEATURESET_7d0]); + if (test_bit(X86_FEATURE_ARCH_CAPS, c->x86_capability)) + rdmsr(MSR_ARCH_CAPABILITIES, + c->x86_capability[FEATURESET_m10Al], + c->x86_capability[FEATURESET_m10Ah]); + if (max_subleaf >= 1) cpuid_count(7, 1, &eax, &ebx, &ecx, &c->x86_capability[FEATURESET_7d1]); diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c index a99e402b98..abcfdc460d 100644 --- a/xen/arch/x86/cpu/microcode/intel.c +++ b/xen/arch/x86/cpu/microcode/intel.c @@ -387,8 +387,22 @@ static struct microcode_patch *cf_check cpu_request_microcode( void __init intel_get_ucode_ops(struct microcode_ops *ops) { + uint64_t mcu_ctrl; + ops->cpu_request_microcode = cpu_request_microcode; ops->collect_cpu_info = collect_cpu_info; ops->apply_microcode = apply_microcode; ops->compare_patch = compare_patch; + + if ( cpu_has_mcu_ctrl ) + { + rdmsrl(MSR_MCU_CONTROL, mcu_ctrl); + /* + * If DIS_MCU_LOAD is set applying microcode updates won't work. We + * can still query the current version and things like that, so + * we'll leave the other handlers in place. + */ + if ( mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD ) + ops->apply_microcode = NULL; + } } 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. If we notice that bit being set then simply disable the "apply_microcode" handler so we can't even try to perform update (as it's known to be silently dropped). Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v3: * Removed microcode_update_one() hunk (Jan, from v2/patch4) * Read MSR_ARCH_CAPS in early_cpu_init(), missing in v2/patch4 (Andy) * Moved the MSR_ARCH_CAPS after-update read (it's on v3/p3 now) * Logic previouslyin this_cpu_can_install_update() is now integrated in intel_get_ucode_ops() (Jan, from v2/p4) --- xen/arch/x86/cpu/common.c | 5 +++++ xen/arch/x86/cpu/microcode/intel.c | 14 ++++++++++++++ xen/arch/x86/include/asm/cpufeature.h | 1 + xen/arch/x86/include/asm/msr-index.h | 5 +++++ 4 files changed, 25 insertions(+)