| Message ID | 20200319152622.31758-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/ucode: Cleanup - Part 1/n | expand |
On 19.03.2020 16:26, Andrew Cooper wrote: > Drop microcode_init_{intel,amd}(), export {intel,amd}_ucode_ops, and use a > switch statement in early_microcode_init() rather than probing each vendor in > turn. This allows the microcode_ops pointer to become local to core.c. > > As there are no external users of microcode_ops, there is no need for > collect_cpu_info() to implement sanity checks. Move applicable checks to > early_microcode_init() so they are performed once, rather than repeatedly. > > Items to note: > * The AMD ucode driver does have an upper familiy limit of 0x17, as a side > effect of the logic in verify_patch_size() which does need updating for > each new model. I don't see this being the case, and hence I think it is this patch which introduces such a restriction. As long a patches are less than 2k, all unspecified families are supported by verify_patch_size() through its default: case label. (Arguably the name F1XH_MPB_MAX_SIZE doesn't really fit how it is being used.) I'm happy about all other changes made here. Jan
On 20/03/2020 13:37, Jan Beulich wrote: > On 19.03.2020 16:26, Andrew Cooper wrote: >> Drop microcode_init_{intel,amd}(), export {intel,amd}_ucode_ops, and use a >> switch statement in early_microcode_init() rather than probing each vendor in >> turn. This allows the microcode_ops pointer to become local to core.c. >> >> As there are no external users of microcode_ops, there is no need for >> collect_cpu_info() to implement sanity checks. Move applicable checks to >> early_microcode_init() so they are performed once, rather than repeatedly. >> >> Items to note: >> * The AMD ucode driver does have an upper familiy limit of 0x17, as a side >> effect of the logic in verify_patch_size() which does need updating for >> each new model. > I don't see this being the case, and hence I think it is this patch > which introduces such a restriction. As long a patches are less > than 2k, all unspecified families are supported by verify_patch_size() > through its default: case label. (Arguably the name F1XH_MPB_MAX_SIZE > doesn't really fit how it is being used.) > > I'm happy about all other changes made here. Linux actually has a different algorithm which drops length restrictions on Fam15h and later, so they get forward compatibility that way. Would you be happy if I dropped just this aspect of the patch, and defer AMD adjustments to a later set of changes? ~Andrew
On 20.03.2020 14:40, Andrew Cooper wrote: > On 20/03/2020 13:37, Jan Beulich wrote: >> On 19.03.2020 16:26, Andrew Cooper wrote: >>> Drop microcode_init_{intel,amd}(), export {intel,amd}_ucode_ops, and use a >>> switch statement in early_microcode_init() rather than probing each vendor in >>> turn. This allows the microcode_ops pointer to become local to core.c. >>> >>> As there are no external users of microcode_ops, there is no need for >>> collect_cpu_info() to implement sanity checks. Move applicable checks to >>> early_microcode_init() so they are performed once, rather than repeatedly. >>> >>> Items to note: >>> * The AMD ucode driver does have an upper familiy limit of 0x17, as a side >>> effect of the logic in verify_patch_size() which does need updating for >>> each new model. >> I don't see this being the case, and hence I think it is this patch >> which introduces such a restriction. As long a patches are less >> than 2k, all unspecified families are supported by verify_patch_size() >> through its default: case label. (Arguably the name F1XH_MPB_MAX_SIZE >> doesn't really fit how it is being used.) >> >> I'm happy about all other changes made here. > > Linux actually has a different algorithm which drops length restrictions > on Fam15h and later, so they get forward compatibility that way. If that's what AMD mandates/suggests, we {c,sh}ould consider doing so too. I thought though that these length restrictions were actually put in by AMD folks. > Would you be happy if I dropped just this aspect of the patch, and defer > AMD adjustments to a later set of changes? Yes, as said - everything else looked good to me. Jan
On 20/03/2020 13:56, Jan Beulich wrote: > On 20.03.2020 14:40, Andrew Cooper wrote: >> On 20/03/2020 13:37, Jan Beulich wrote: >>> On 19.03.2020 16:26, Andrew Cooper wrote: >>>> Drop microcode_init_{intel,amd}(), export {intel,amd}_ucode_ops, and use a >>>> switch statement in early_microcode_init() rather than probing each vendor in >>>> turn. This allows the microcode_ops pointer to become local to core.c. >>>> >>>> As there are no external users of microcode_ops, there is no need for >>>> collect_cpu_info() to implement sanity checks. Move applicable checks to >>>> early_microcode_init() so they are performed once, rather than repeatedly. >>>> >>>> Items to note: >>>> * The AMD ucode driver does have an upper familiy limit of 0x17, as a side >>>> effect of the logic in verify_patch_size() which does need updating for >>>> each new model. >>> I don't see this being the case, and hence I think it is this patch >>> which introduces such a restriction. As long a patches are less >>> than 2k, all unspecified families are supported by verify_patch_size() >>> through its default: case label. (Arguably the name F1XH_MPB_MAX_SIZE >>> doesn't really fit how it is being used.) >>> >>> I'm happy about all other changes made here. >> Linux actually has a different algorithm which drops length restrictions >> on Fam15h and later, so they get forward compatibility that way. > If that's what AMD mandates/suggests, we {c,sh}ould consider doing > so too. I thought though that these length restrictions were actually > put in by AMD folks. Its on the list of questions... >> Would you be happy if I dropped just this aspect of the patch, and defer >> AMD adjustments to a later set of changes? > Yes, as said - everything else looked good to me. Can I take that as an A-by then, to save posting the patch again? ~Andrew
On 20.03.2020 15:27, Andrew Cooper wrote: > On 20/03/2020 13:56, Jan Beulich wrote: >> On 20.03.2020 14:40, Andrew Cooper wrote: >>> On 20/03/2020 13:37, Jan Beulich wrote: >>>> On 19.03.2020 16:26, Andrew Cooper wrote: >>>>> Drop microcode_init_{intel,amd}(), export {intel,amd}_ucode_ops, and use a >>>>> switch statement in early_microcode_init() rather than probing each vendor in >>>>> turn. This allows the microcode_ops pointer to become local to core.c. >>>>> >>>>> As there are no external users of microcode_ops, there is no need for >>>>> collect_cpu_info() to implement sanity checks. Move applicable checks to >>>>> early_microcode_init() so they are performed once, rather than repeatedly. >>>>> >>>>> Items to note: >>>>> * The AMD ucode driver does have an upper familiy limit of 0x17, as a side >>>>> effect of the logic in verify_patch_size() which does need updating for >>>>> each new model. >>>> I don't see this being the case, and hence I think it is this patch >>>> which introduces such a restriction. As long a patches are less >>>> than 2k, all unspecified families are supported by verify_patch_size() >>>> through its default: case label. (Arguably the name F1XH_MPB_MAX_SIZE >>>> doesn't really fit how it is being used.) >>>> >>>> I'm happy about all other changes made here. >>> Linux actually has a different algorithm which drops length restrictions >>> on Fam15h and later, so they get forward compatibility that way. >> If that's what AMD mandates/suggests, we {c,sh}ould consider doing >> so too. I thought though that these length restrictions were actually >> put in by AMD folks. > > Its on the list of questions... > >>> Would you be happy if I dropped just this aspect of the patch, and defer >>> AMD adjustments to a later set of changes? >> Yes, as said - everything else looked good to me. > > Can I take that as an A-by then, to save posting the patch again? If it's just taking out the fam <= 0x17 check - yes. Jan
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 9028889813..768fbcf322 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -76,22 +76,12 @@ struct mpbhdr { /* See comment in start_update() for cases when this routine fails */ static int collect_cpu_info(struct cpu_signature *csig) { - unsigned int cpu = smp_processor_id(); - struct cpuinfo_x86 *c = &cpu_data[cpu]; - memset(csig, 0, sizeof(*csig)); - if ( (c->x86_vendor != X86_VENDOR_AMD) || (c->x86 < 0x10) ) - { - printk(KERN_ERR "microcode: CPU%d not a capable AMD processor\n", - cpu); - return -EINVAL; - } - rdmsrl(MSR_AMD_PATCHLEVEL, csig->rev); pr_debug("microcode: CPU%d collect_cpu_info: patch_id=%#x\n", - cpu, csig->rev); + smp_processor_id(), csig->rev); return 0; } @@ -601,7 +591,7 @@ static int start_update(void) } #endif -static const struct microcode_ops microcode_amd_ops = { +const struct microcode_ops amd_ucode_ops = { .cpu_request_microcode = cpu_request_microcode, .collect_cpu_info = collect_cpu_info, .apply_microcode = apply_microcode, @@ -613,10 +603,3 @@ static const struct microcode_ops microcode_amd_ops = { .compare_patch = compare_patch, .match_cpu = match_cpu, }; - -int __init microcode_init_amd(void) -{ - if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) - microcode_ops = µcode_amd_ops; - return 0; -} diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index ac5da6b2fe..61e4b9b7ab 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -210,7 +210,7 @@ void __init microcode_grab_module( microcode_scan_module(module_map, mbi); } -const struct microcode_ops *microcode_ops; +static const struct microcode_ops __read_mostly *microcode_ops; static DEFINE_SPINLOCK(microcode_mutex); @@ -798,23 +798,32 @@ static int __init early_microcode_update_cpu(void) int __init early_microcode_init(void) { - int rc; - - rc = microcode_init_intel(); - if ( rc ) - return rc; - - rc = microcode_init_amd(); - if ( rc ) - return rc; + const struct cpuinfo_x86 *c = &boot_cpu_data; + int rc = 0; - if ( microcode_ops ) + switch ( c->x86_vendor ) { - microcode_ops->collect_cpu_info(&this_cpu(cpu_sig)); + case X86_VENDOR_AMD: + if ( c->x86 >= 0x10 && c->x86 <= 0x17 ) + microcode_ops = &amd_ucode_ops; + break; + + case X86_VENDOR_INTEL: + if ( c->x86 >= 6 ) + microcode_ops = &intel_ucode_ops; + break; + } - if ( ucode_mod.mod_end || ucode_blob.size ) - rc = early_microcode_update_cpu(); + if ( !microcode_ops ) + { + printk(XENLOG_WARNING "Microcode loading not available\n"); + return -ENODEV; } + microcode_ops->collect_cpu_info(&this_cpu(cpu_sig)); + + if ( ucode_mod.mod_end || ucode_blob.size ) + rc = early_microcode_update_cpu(); + return rc; } diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c index 90fb006c94..48544e8d6d 100644 --- a/xen/arch/x86/cpu/microcode/intel.c +++ b/xen/arch/x86/cpu/microcode/intel.c @@ -93,27 +93,14 @@ struct extended_sigtable { static int collect_cpu_info(struct cpu_signature *csig) { - unsigned int cpu_num = smp_processor_id(); - struct cpuinfo_x86 *c = &cpu_data[cpu_num]; uint64_t msr_content; memset(csig, 0, sizeof(*csig)); - if ( (c->x86_vendor != X86_VENDOR_INTEL) || (c->x86 < 6) ) - { - printk(KERN_ERR "microcode: CPU%d not a capable Intel " - "processor\n", cpu_num); - return -1; - } - csig->sig = cpuid_eax(0x00000001); - if ( (c->x86_model >= 5) || (c->x86 > 6) ) - { - /* get processor flags from MSR 0x17 */ - rdmsrl(MSR_IA32_PLATFORM_ID, msr_content); - csig->pf = 1 << ((msr_content >> 50) & 7); - } + rdmsrl(MSR_IA32_PLATFORM_ID, msr_content); + csig->pf = 1 << ((msr_content >> 50) & 7); wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL); /* As documented in the SDM: Do a CPUID 1 here */ @@ -405,7 +392,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, return patch; } -static const struct microcode_ops microcode_intel_ops = { +const struct microcode_ops intel_ucode_ops = { .cpu_request_microcode = cpu_request_microcode, .collect_cpu_info = collect_cpu_info, .apply_microcode = apply_microcode, @@ -413,10 +400,3 @@ static const struct microcode_ops microcode_intel_ops = { .compare_patch = compare_patch, .match_cpu = match_cpu, }; - -int __init microcode_init_intel(void) -{ - if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) - microcode_ops = µcode_intel_ops; - return 0; -} diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h index 459b6a4c54..c32ddc8d19 100644 --- a/xen/arch/x86/cpu/microcode/private.h +++ b/xen/arch/x86/cpu/microcode/private.h @@ -32,9 +32,6 @@ struct microcode_ops { const struct microcode_patch *new, const struct microcode_patch *old); }; -extern const struct microcode_ops *microcode_ops; - -int microcode_init_intel(void); -int microcode_init_amd(void); +extern const struct microcode_ops amd_ucode_ops, intel_ucode_ops; #endif /* ASM_X86_MICROCODE_PRIVATE_H */
Drop microcode_init_{intel,amd}(), export {intel,amd}_ucode_ops, and use a switch statement in early_microcode_init() rather than probing each vendor in turn. This allows the microcode_ops pointer to become local to core.c. As there are no external users of microcode_ops, there is no need for collect_cpu_info() to implement sanity checks. Move applicable checks to early_microcode_init() so they are performed once, rather than repeatedly. Items to note: * The AMD ucode driver does have an upper familiy limit of 0x17, as a side effect of the logic in verify_patch_size() which does need updating for each new model. * The Intel logic guarding the read of MSR_PLATFORM_ID is contrary to the SDM, which states that the MSR has been architectural since the Pentium Pro (06-01-xx), and lists no family/model restrictions in the pseudocode for microcode loading. Either way, Xen's 64bit-only nature already makes this check redundant. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> The MSR_PLATFORM_ID guard was inherited from Linux, and has existed there since the code's introduction. My best guess is that the MSR list in the SDM got altered at some point between then and now. --- xen/arch/x86/cpu/microcode/amd.c | 21 ++------------------ xen/arch/x86/cpu/microcode/core.c | 37 ++++++++++++++++++++++-------------- xen/arch/x86/cpu/microcode/intel.c | 26 +++---------------------- xen/arch/x86/cpu/microcode/private.h | 5 +---- 4 files changed, 29 insertions(+), 60 deletions(-)