| Message ID | 20231025180630.3230010-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: ucode and CPU Kconfig | expand |
On Wed, 25 Oct 2023, Andrew Cooper wrote: > I know it was me who dropped microcode_init_{intel,amd}() in c/s > dd5f07997f29 ("x86/ucode: Rationalise startup and family/model checks"), but > times have moved on. We've gained new conditional support, and a wish to > compile-time specialise Xen to single platform. > > (Re)introduce ucode_probe_{amd,intel}() and move the recent vendor specific > additions back out. Encode the conditional support state in the NULL-ness of > hooks as it's already done on other paths. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Alejandro Vallejo <alejandro.vallejo@cloud.com> > CC: Stefano Stabellini <stefano.stabellini@amd.com> > CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com> > > CC Stefano/Xenia as I know you want to go down this line, but I don't recall > patches in this area yet. Yep, anything we can do to specialized the Xen build just to the components needed (e.g. AMD drivers yes, Intel drivers no) it is an improvement
On 25.10.2023 20:06, Andrew Cooper wrote: > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -847,25 +847,19 @@ int __init early_microcode_init(unsigned long *module_map, > { > const struct cpuinfo_x86 *c = &boot_cpu_data; > int rc = 0; > - bool can_load = false; > > switch ( c->x86_vendor ) > { > case X86_VENDOR_AMD: > - if ( c->x86 >= 0x10 ) > - { > - ucode_ops = amd_ucode_ops; > - can_load = true; > - } > + ucode_probe_amd(&ucode_ops); > break; > > case X86_VENDOR_INTEL: > - ucode_ops = intel_ucode_ops; > - can_load = intel_can_load_microcode(); > + ucode_probe_intel(&ucode_ops); > break; > } > > - if ( !ucode_ops.apply_microcode ) > + if ( !ucode_ops.collect_cpu_info ) > { > printk(XENLOG_INFO "Microcode loading not available\n"); > return -ENODEV; > @@ -882,10 +876,10 @@ int __init early_microcode_init(unsigned long *module_map, > * > * Take the hint in either case and ignore the microcode interface. > */ > - if ( this_cpu(cpu_sig).rev == ~0 || !can_load ) > + if ( !ucode_ops.apply_microcode || this_cpu(cpu_sig).rev == ~0 ) Here ucode_ops.apply_microcode simply replaces can_load, as expected. > { > printk(XENLOG_INFO "Microcode loading disabled due to: %s\n", > - can_load ? "rev = ~0" : "HW toggle"); > + ucode_ops.apply_microcode ? "HW toggle" : "rev = ~0"); Here, otoh, you invert sense, which I don't think is right. With 2nd and 3rd operands swapped back Reviewed-by: Jan Beulich <jbeulich@suse.com> > @@ -398,9 +398,17 @@ bool __init intel_can_load_microcode(void) > return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD); > } > > -const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = { > +static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = { > .cpu_request_microcode = cpu_request_microcode, > .collect_cpu_info = collect_cpu_info, > .apply_microcode = apply_microcode, > .compare_patch = compare_patch, > }; > + > +void __init ucode_probe_intel(struct microcode_ops *ops) > +{ > + *ops = intel_ucode_ops; > + > + if ( !can_load_microcode() ) > + ops->apply_microcode = NULL; > +} I was wondering why you didn't use the return value to supply the pointer to the caller, but this override explains it. Jan
On 26/10/2023 8:45 am, Jan Beulich wrote: > On 25.10.2023 20:06, Andrew Cooper wrote: >> --- a/xen/arch/x86/cpu/microcode/core.c >> +++ b/xen/arch/x86/cpu/microcode/core.c >> @@ -847,25 +847,19 @@ int __init early_microcode_init(unsigned long *module_map, >> { >> const struct cpuinfo_x86 *c = &boot_cpu_data; >> int rc = 0; >> - bool can_load = false; >> >> switch ( c->x86_vendor ) >> { >> case X86_VENDOR_AMD: >> - if ( c->x86 >= 0x10 ) >> - { >> - ucode_ops = amd_ucode_ops; >> - can_load = true; >> - } >> + ucode_probe_amd(&ucode_ops); >> break; >> >> case X86_VENDOR_INTEL: >> - ucode_ops = intel_ucode_ops; >> - can_load = intel_can_load_microcode(); >> + ucode_probe_intel(&ucode_ops); >> break; >> } >> >> - if ( !ucode_ops.apply_microcode ) >> + if ( !ucode_ops.collect_cpu_info ) >> { >> printk(XENLOG_INFO "Microcode loading not available\n"); >> return -ENODEV; >> @@ -882,10 +876,10 @@ int __init early_microcode_init(unsigned long *module_map, >> * >> * Take the hint in either case and ignore the microcode interface. >> */ >> - if ( this_cpu(cpu_sig).rev == ~0 || !can_load ) >> + if ( !ucode_ops.apply_microcode || this_cpu(cpu_sig).rev == ~0 ) > Here ucode_ops.apply_microcode simply replaces can_load, as expected. > >> { >> printk(XENLOG_INFO "Microcode loading disabled due to: %s\n", >> - can_load ? "rev = ~0" : "HW toggle"); >> + ucode_ops.apply_microcode ? "HW toggle" : "rev = ~0"); > Here, otoh, you invert sense, which I don't think is right. With 2nd > and 3rd operands swapped back > Reviewed-by: Jan Beulich <jbeulich@suse.com> Oh, right. This did get adjusted several times. I'll fix. It's a weird construct anyway, and it gets cleaned up differently by some of my later work. > >> @@ -398,9 +398,17 @@ bool __init intel_can_load_microcode(void) >> return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD); >> } >> >> -const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = { >> +static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = { >> .cpu_request_microcode = cpu_request_microcode, >> .collect_cpu_info = collect_cpu_info, >> .apply_microcode = apply_microcode, >> .compare_patch = compare_patch, >> }; >> + >> +void __init ucode_probe_intel(struct microcode_ops *ops) >> +{ >> + *ops = intel_ucode_ops; >> + >> + if ( !can_load_microcode() ) >> + ops->apply_microcode = NULL; >> +} > I was wondering why you didn't use the return value to supply the pointer > to the caller, but this override explains it. Yes. The other option was to export (in private.h at least) the root ucode_ops, which I decided against in the end. I also toyed with the idea of having the probe functions return int, so we could get EOPNOTSUPP or so in the compiled-out case, but that's almost completely redundant with clobbering the hook, and it's added complexity for somethign that only randconfig is going to care about. The only thing I'm not entirely happy about is the volume of the ifdefary for these ucode_probe() functions in the following patch, but it's the least invasive option overall IMO. ~Andrew
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 75fc84e445ce..17e68697d5bf 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -434,9 +434,17 @@ static struct microcode_patch *cf_check cpu_request_microcode( return patch; } -const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = { +static const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = { .cpu_request_microcode = cpu_request_microcode, .collect_cpu_info = collect_cpu_info, .apply_microcode = apply_microcode, .compare_patch = compare_patch, }; + +void __init ucode_probe_amd(struct microcode_ops *ops) +{ + if ( boot_cpu_data.x86 < 0x10 ) + return; + + *ops = amd_ucode_ops; +} diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 65ebeb50deea..09575b19d6dc 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -847,25 +847,19 @@ int __init early_microcode_init(unsigned long *module_map, { const struct cpuinfo_x86 *c = &boot_cpu_data; int rc = 0; - bool can_load = false; switch ( c->x86_vendor ) { case X86_VENDOR_AMD: - if ( c->x86 >= 0x10 ) - { - ucode_ops = amd_ucode_ops; - can_load = true; - } + ucode_probe_amd(&ucode_ops); break; case X86_VENDOR_INTEL: - ucode_ops = intel_ucode_ops; - can_load = intel_can_load_microcode(); + ucode_probe_intel(&ucode_ops); break; } - if ( !ucode_ops.apply_microcode ) + if ( !ucode_ops.collect_cpu_info ) { printk(XENLOG_INFO "Microcode loading not available\n"); return -ENODEV; @@ -882,10 +876,10 @@ int __init early_microcode_init(unsigned long *module_map, * * Take the hint in either case and ignore the microcode interface. */ - if ( this_cpu(cpu_sig).rev == ~0 || !can_load ) + if ( !ucode_ops.apply_microcode || this_cpu(cpu_sig).rev == ~0 ) { printk(XENLOG_INFO "Microcode loading disabled due to: %s\n", - can_load ? "rev = ~0" : "HW toggle"); + ucode_ops.apply_microcode ? "HW toggle" : "rev = ~0"); ucode_ops.apply_microcode = NULL; return -ENODEV; } diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c index 060c529a6e5d..96f34b336b21 100644 --- a/xen/arch/x86/cpu/microcode/intel.c +++ b/xen/arch/x86/cpu/microcode/intel.c @@ -385,7 +385,7 @@ static struct microcode_patch *cf_check cpu_request_microcode( return patch; } -bool __init intel_can_load_microcode(void) +static bool __init can_load_microcode(void) { uint64_t mcu_ctrl; @@ -398,9 +398,17 @@ bool __init intel_can_load_microcode(void) return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD); } -const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = { +static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = { .cpu_request_microcode = cpu_request_microcode, .collect_cpu_info = collect_cpu_info, .apply_microcode = apply_microcode, .compare_patch = compare_patch, }; + +void __init ucode_probe_intel(struct microcode_ops *ops) +{ + *ops = intel_ucode_ops; + + if ( !can_load_microcode() ) + ops->apply_microcode = NULL; +} diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h index d80787205a5e..b58611e908aa 100644 --- a/xen/arch/x86/cpu/microcode/private.h +++ b/xen/arch/x86/cpu/microcode/private.h @@ -60,13 +60,17 @@ struct microcode_ops { const struct microcode_patch *new, const struct microcode_patch *old); }; -/** - * Checks whether we can perform microcode updates on this Intel system +/* + * Microcode loading falls into one of 3 states. + * - No support at all + * - Read-only (locked by firmware, or we're virtualised) + * - Loading available * - * @return True iff the microcode update facilities are enabled + * These are encoded by (not) filling in ops->collect_cpu_info (i.e. no + * support available) and (not) ops->apply_microcode (i.e. read only). + * Otherwise, all hooks must be filled in. */ -bool intel_can_load_microcode(void); - -extern const struct microcode_ops amd_ucode_ops, intel_ucode_ops; +void ucode_probe_amd(struct microcode_ops *ops); +void ucode_probe_intel(struct microcode_ops *ops); #endif /* ASM_X86_MICROCODE_PRIVATE_H */
I know it was me who dropped microcode_init_{intel,amd}() in c/s dd5f07997f29 ("x86/ucode: Rationalise startup and family/model checks"), but times have moved on. We've gained new conditional support, and a wish to compile-time specialise Xen to single platform. (Re)introduce ucode_probe_{amd,intel}() and move the recent vendor specific additions back out. Encode the conditional support state in the NULL-ness of hooks as it's already done on other paths. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com> CC: Stefano Stabellini <stefano.stabellini@amd.com> CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com> CC Stefano/Xenia as I know you want to go down this line, but I don't recall patches in this area yet. --- xen/arch/x86/cpu/microcode/amd.c | 10 +++++++++- xen/arch/x86/cpu/microcode/core.c | 16 +++++----------- xen/arch/x86/cpu/microcode/intel.c | 12 ++++++++++-- xen/arch/x86/cpu/microcode/private.h | 16 ++++++++++------ 4 files changed, 34 insertions(+), 20 deletions(-)