| Message ID | 1558945891-3015-4-git-send-email-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | improve late microcode loading | expand |
>>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote: > +bool microcode_update_cache(struct microcode_patch *patch) > +{ > + > + ASSERT(spin_is_locked(µcode_mutex)); > + > + if ( !microcode_ops->match_cpu(patch) ) > + return false; > + > + if ( !microcode_cache ) > + microcode_cache = patch; > + else if ( microcode_ops->compare_patch(patch, microcode_cache) == > + NEW_UCODE ) > + { > + microcode_ops->free_patch(microcode_cache); > + microcode_cache = patch; > + } Hmm, okay, the way you do things here three enumeration values may indeed be sufficient. "old" may just be a little misleading then. (As to my respective comment on the previous patch.) > +static struct microcode_patch *alloc_microcode_patch( > + const struct microcode_amd *mc_amd) > +{ > + struct microcode_patch *microcode_patch = xmalloc(struct microcode_patch); > + struct microcode_amd *cache = xmalloc(struct microcode_amd); > + void *mpb = xmalloc_bytes(mc_amd->mpb_size); > + struct equiv_cpu_entry *equiv_cpu_table = > + xmalloc_bytes(mc_amd->equiv_cpu_table_size); > + > + if ( !microcode_patch || !cache || !mpb || !equiv_cpu_table ) > + { > + xfree(microcode_patch); > + xfree(cache); > + xfree(mpb); > + xfree(equiv_cpu_table); > + printk(XENLOG_ERR "microcode: Can not allocate memory\n"); I'm not convinced this needs logging. > + return ERR_PTR(-ENOMEM); > + } > + > + cache->equiv_cpu_table = equiv_cpu_table; > + cache->mpb = mpb; > + memcpy(cache->equiv_cpu_table, mc_amd->equiv_cpu_table, Why not use the local variable here and ... > + mc_amd->equiv_cpu_table_size); > + memcpy(cache->mpb, mc_amd->mpb, mc_amd->mpb_size); here? Less source code and presumably also slightly less binary code. In fact I wonder if you wouldn't better memcpy() first anyway, and only then store the values into the fields. It won't matter much with the global lock held, but it's generally good practice to do things in an order that won't risk to confuse hypothetical consumers of the data. > +static void free_patch(struct microcode_patch *microcode_patch) > +{ > + struct microcode_amd *mc_amd = microcode_patch->mc_amd; > + > + xfree(mc_amd->equiv_cpu_table); > + xfree(mc_amd->mpb); > + xfree(mc_amd); > + xfree(microcode_patch); I think I said so before: Freeing of the generic wrapper struct would probably better be placed in generic code. > @@ -497,7 +558,20 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf, > while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, > &offset)) == 0 ) > { > - if ( microcode_fits(mc_amd, cpu) ) > + struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd); > + > + if ( IS_ERR(new_patch) ) > + { > + error = PTR_ERR(new_patch); > + break; > + } > + > + if ( match_cpu(new_patch) ) > + microcode_update_cache(new_patch); > + else > + free_patch(new_patch); Why do you re-do what microcode_update_cache() already does? It calls ->match_cpu() and ->free_patch() all by itself. It looks as if it would need to gain one more ->free_patch() invocation though. (These last two comments apply to the respective Intel code as well then.) > @@ -277,6 +319,7 @@ static int get_matching_microcode(const void *mc, unsigned int cpu) > memcpy(new_mc, mc, total_size); > xfree(uci->mc.mc_intel); > uci->mc.mc_intel = new_mc; > + > return 1; > } > Stray cosmetics? > @@ -309,19 +356,19 @@ static int apply_microcode(unsigned int cpu) > val[1] = (uint32_t)(msr_content >> 32); > > spin_unlock_irqrestore(µcode_update_lock, flags); > - if ( val[1] != uci->mc.mc_intel->hdr.rev ) > + if ( val[1] != mc_intel->hdr.rev ) > { > printk(KERN_ERR "microcode: CPU%d update from revision " > "%#x to %#x failed. Resulting revision is %#x.\n", cpu_num, > - uci->cpu_sig.rev, uci->mc.mc_intel->hdr.rev, val[1]); > + uci->cpu_sig.rev, mc_intel->hdr.rev, val[1]); > return -EIO; > } > printk(KERN_INFO "microcode: CPU%d updated from revision " > "%#x to %#x, date = %04x-%02x-%02x \n", > cpu_num, uci->cpu_sig.rev, val[1], > - uci->mc.mc_intel->hdr.year, > - uci->mc.mc_intel->hdr.month, > - uci->mc.mc_intel->hdr.day); > + mc_intel->hdr.year, > + mc_intel->hdr.month, > + mc_intel->hdr.day); The three arguments now look to all fit on a single line. Jan
On Tue, Jun 04, 2019 at 09:03:20AM -0600, Jan Beulich wrote: >>>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote: >> +bool microcode_update_cache(struct microcode_patch *patch) >> +{ >> + >> + ASSERT(spin_is_locked(µcode_mutex)); >> + >> + if ( !microcode_ops->match_cpu(patch) ) >> + return false; >> + >> + if ( !microcode_cache ) >> + microcode_cache = patch; >> + else if ( microcode_ops->compare_patch(patch, microcode_cache) == >> + NEW_UCODE ) >> + { >> + microcode_ops->free_patch(microcode_cache); >> + microcode_cache = patch; >> + } > >Hmm, okay, the way you do things here three enumeration values >may indeed be sufficient. "old" may just be a little misleading then. >(As to my respective comment on the previous patch.) > >> +static struct microcode_patch *alloc_microcode_patch( >> + const struct microcode_amd *mc_amd) >> +{ >> + struct microcode_patch *microcode_patch = xmalloc(struct microcode_patch); >> + struct microcode_amd *cache = xmalloc(struct microcode_amd); >> + void *mpb = xmalloc_bytes(mc_amd->mpb_size); >> + struct equiv_cpu_entry *equiv_cpu_table = >> + xmalloc_bytes(mc_amd->equiv_cpu_table_size); >> + >> + if ( !microcode_patch || !cache || !mpb || !equiv_cpu_table ) >> + { >> + xfree(microcode_patch); >> + xfree(cache); >> + xfree(mpb); >> + xfree(equiv_cpu_table); >> + printk(XENLOG_ERR "microcode: Can not allocate memory\n"); > >I'm not convinced this needs logging. > >> + return ERR_PTR(-ENOMEM); >> + } >> + >> + cache->equiv_cpu_table = equiv_cpu_table; >> + cache->mpb = mpb; >> + memcpy(cache->equiv_cpu_table, mc_amd->equiv_cpu_table, > >Why not use the local variable here and ... > >> + mc_amd->equiv_cpu_table_size); >> + memcpy(cache->mpb, mc_amd->mpb, mc_amd->mpb_size); > >here? Less source code and presumably also slightly less binary >code. In fact I wonder if you wouldn't better memcpy() first >anyway, and only then store the values into the fields. It won't >matter much with the global lock held, but it's generally good >practice to do things in an order that won't risk to confuse >hypothetical consumers of the data. Will do. > >> +static void free_patch(struct microcode_patch *microcode_patch) >> +{ >> + struct microcode_amd *mc_amd = microcode_patch->mc_amd; >> + >> + xfree(mc_amd->equiv_cpu_table); >> + xfree(mc_amd->mpb); >> + xfree(mc_amd); >> + xfree(microcode_patch); > >I think I said so before: Freeing of the generic wrapper struct >would probably better be placed in generic code. Do you mean something as shown below: /* in generic code */ struct microcode_patch { union { struct microcode_intel *mc_intel; struct microcode_amd *mc_amd; void *mc; }; }; void microcode_free_patch(struct microcode_patch *microcode_patch) { microcode_ops->free_patch(microcode_patch->mc); xfree(microcode_patch); } /* in vendor-specific (AMD) code */ static void free_patch(void *mc) { struct microcode_amd *mc_amd = mc; xfree(mc_amd->equiv_cpu_table); xfree(mc_amd->mpb); xfree(mc_amd); } > >> @@ -497,7 +558,20 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf, >> while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, >> &offset)) == 0 ) >> { >> - if ( microcode_fits(mc_amd, cpu) ) >> + struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd); >> + >> + if ( IS_ERR(new_patch) ) >> + { >> + error = PTR_ERR(new_patch); >> + break; >> + } >> + >> + if ( match_cpu(new_patch) ) >> + microcode_update_cache(new_patch); >> + else >> + free_patch(new_patch); > >Why do you re-do what microcode_update_cache() already does? >It calls ->match_cpu() and ->free_patch() all by itself. It looks as >if it would need to gain one more ->free_patch() invocation though. > Will remove both invocations of match_cpu(). To support the case (the broken bios) you described, a patch which needs to be stored isn't necessary to be newer than the microcode loaded to current CPU. As long as the processor's signature is covered by the patch, we will store the patch regardless the revision number. Thanks Chao
>>> On 10.06.19 at 07:33, <chao.gao@intel.com> wrote: > On Tue, Jun 04, 2019 at 09:03:20AM -0600, Jan Beulich wrote: >>>>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote: >>> +static void free_patch(struct microcode_patch *microcode_patch) >>> +{ >>> + struct microcode_amd *mc_amd = microcode_patch->mc_amd; >>> + >>> + xfree(mc_amd->equiv_cpu_table); >>> + xfree(mc_amd->mpb); >>> + xfree(mc_amd); >>> + xfree(microcode_patch); >> >>I think I said so before: Freeing of the generic wrapper struct >>would probably better be placed in generic code. > > Do you mean something as shown below: > > /* in generic code */ > > struct microcode_patch { > union { > struct microcode_intel *mc_intel; > struct microcode_amd *mc_amd; > void *mc; > }; > }; > > void microcode_free_patch(struct microcode_patch *microcode_patch) > { > microcode_ops->free_patch(microcode_patch->mc); > xfree(microcode_patch); > } > > /* in vendor-specific (AMD) code */ > > static void free_patch(void *mc) > { > struct microcode_amd *mc_amd = mc; > > xfree(mc_amd->equiv_cpu_table); > xfree(mc_amd->mpb); > xfree(mc_amd); > } Something along these lines, yes. Whether you do as above or whether instead you continue to pass struct microcode_patch * is secondary (and really up to you). Perhaps the above the (slightly) better. >>> @@ -497,7 +558,20 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf, >>> while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, >>> &offset)) == 0 ) >>> { >>> - if ( microcode_fits(mc_amd, cpu) ) >>> + struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd); >>> + >>> + if ( IS_ERR(new_patch) ) >>> + { >>> + error = PTR_ERR(new_patch); >>> + break; >>> + } >>> + >>> + if ( match_cpu(new_patch) ) >>> + microcode_update_cache(new_patch); >>> + else >>> + free_patch(new_patch); >> >>Why do you re-do what microcode_update_cache() already does? >>It calls ->match_cpu() and ->free_patch() all by itself. It looks as >>if it would need to gain one more ->free_patch() invocation though. >> > > Will remove both invocations of match_cpu(). > > To support the case (the broken bios) you described, a patch which > needs to be stored isn't necessary to be newer than the microcode loaded > to current CPU. As long as the processor's signature is covered by the > patch, we will store the patch regardless the revision number. Well, if so, then fine. I did get the impression that successful application is a required pre-condition for storing. Jan
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c index 4163f50..cff86a9 100644 --- a/xen/arch/x86/microcode.c +++ b/xen/arch/x86/microcode.c @@ -61,6 +61,9 @@ static struct ucode_mod_blob __initdata ucode_blob; */ static bool_t __initdata ucode_scan; +/* Protected by microcode_mutex */ +static struct microcode_patch *microcode_cache; + void __init microcode_set_module(unsigned int idx) { ucode_mod_idx = idx; @@ -262,6 +265,39 @@ int microcode_resume_cpu(unsigned int cpu) return err; } +const struct microcode_patch *microcode_get_cache(void) +{ + ASSERT(spin_is_locked(µcode_mutex)); + + return microcode_cache; +} + +/* Return true if cache gets updated. Otherwise, return false */ +bool microcode_update_cache(struct microcode_patch *patch) +{ + + ASSERT(spin_is_locked(µcode_mutex)); + + if ( !microcode_ops->match_cpu(patch) ) + return false; + + if ( !microcode_cache ) + microcode_cache = patch; + else if ( microcode_ops->compare_patch(patch, microcode_cache) == + NEW_UCODE ) + { + microcode_ops->free_patch(microcode_cache); + microcode_cache = patch; + } + else + { + microcode_ops->free_patch(patch); + return false; + } + + return true; +} + static int microcode_update_cpu(const void *buf, size_t size) { int err; diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c index 7a854c0..1f05899 100644 --- a/xen/arch/x86/microcode_amd.c +++ b/xen/arch/x86/microcode_amd.c @@ -190,24 +190,85 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd, return 1; } +static bool match_cpu(const struct microcode_patch *patch) +{ + if ( !patch ) + return false; + return microcode_fits(patch->mc_amd, smp_processor_id()); +} + +static struct microcode_patch *alloc_microcode_patch( + const struct microcode_amd *mc_amd) +{ + struct microcode_patch *microcode_patch = xmalloc(struct microcode_patch); + struct microcode_amd *cache = xmalloc(struct microcode_amd); + void *mpb = xmalloc_bytes(mc_amd->mpb_size); + struct equiv_cpu_entry *equiv_cpu_table = + xmalloc_bytes(mc_amd->equiv_cpu_table_size); + + if ( !microcode_patch || !cache || !mpb || !equiv_cpu_table ) + { + xfree(microcode_patch); + xfree(cache); + xfree(mpb); + xfree(equiv_cpu_table); + printk(XENLOG_ERR "microcode: Can not allocate memory\n"); + return ERR_PTR(-ENOMEM); + } + + cache->equiv_cpu_table = equiv_cpu_table; + cache->mpb = mpb; + memcpy(cache->equiv_cpu_table, mc_amd->equiv_cpu_table, + mc_amd->equiv_cpu_table_size); + memcpy(cache->mpb, mc_amd->mpb, mc_amd->mpb_size); + cache->equiv_cpu_table_size = mc_amd->equiv_cpu_table_size; + cache->mpb_size = mc_amd->mpb_size; + microcode_patch->mc_amd = cache; + + return microcode_patch; +} + +static void free_patch(struct microcode_patch *microcode_patch) +{ + struct microcode_amd *mc_amd = microcode_patch->mc_amd; + + xfree(mc_amd->equiv_cpu_table); + xfree(mc_amd->mpb); + xfree(mc_amd); + xfree(microcode_patch); +} + +static enum microcode_match_result compare_patch( + const struct microcode_patch *new, const struct microcode_patch *old) +{ + const struct microcode_amd *new_mc = new->mc_amd; + const struct microcode_header_amd *new_header = new_mc->mpb; + const struct microcode_amd *old_mc = old->mc_amd; + const struct microcode_header_amd *old_header = old_mc->mpb; + + if ( new_header->processor_rev_id == old_header->processor_rev_id ) + return (new_header->patch_id > old_header->patch_id) ? + NEW_UCODE : OLD_UCODE; + + return MIS_UCODE; +} + static int apply_microcode(unsigned int cpu) { unsigned long flags; struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); uint32_t rev; - struct microcode_amd *mc_amd = uci->mc.mc_amd; - struct microcode_header_amd *hdr; int hw_err; + const struct microcode_header_amd *hdr; + const struct microcode_patch *patch = microcode_get_cache(); /* We should bind the task to the CPU */ BUG_ON(raw_smp_processor_id() != cpu); - if ( mc_amd == NULL ) + if ( !match_cpu(patch) ) return -EINVAL; - hdr = mc_amd->mpb; - if ( hdr == NULL ) - return -EINVAL; + hdr = patch->mc_amd->mpb; spin_lock_irqsave(µcode_update_lock, flags); @@ -497,7 +558,20 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf, while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, &offset)) == 0 ) { - if ( microcode_fits(mc_amd, cpu) ) + struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd); + + if ( IS_ERR(new_patch) ) + { + error = PTR_ERR(new_patch); + break; + } + + if ( match_cpu(new_patch) ) + microcode_update_cache(new_patch); + else + free_patch(new_patch); + + if ( match_cpu(microcode_get_cache()) ) { error = apply_microcode(cpu); if ( error ) @@ -639,6 +713,9 @@ static const struct microcode_ops microcode_amd_ops = { .collect_cpu_info = collect_cpu_info, .apply_microcode = apply_microcode, .start_update = start_update, + .free_patch = free_patch, + .compare_patch = compare_patch, + .match_cpu = match_cpu, }; int __init microcode_init_amd(void) diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c index ecec83b..d3405a0 100644 --- a/xen/arch/x86/microcode_intel.c +++ b/xen/arch/x86/microcode_intel.c @@ -248,6 +248,32 @@ static int microcode_sanity_check(void *mc) return 0; } +static bool match_cpu(const struct microcode_patch *patch) +{ + const struct ucode_cpu_info *uci = &this_cpu(ucode_cpu_info); + + if ( !patch ) + return false; + + return microcode_update_match(&patch->mc_intel->hdr, uci->cpu_sig.sig, + uci->cpu_sig.pf, uci->cpu_sig.rev) == NEW_UCODE; +} + +static void free_patch(struct microcode_patch *patch) +{ + xfree(patch->mc_intel); + xfree(patch); +} + +static enum microcode_match_result compare_patch( + const struct microcode_patch *new, const struct microcode_patch *old) +{ + const struct microcode_header_intel *old_header = &old->mc_intel->hdr; + + return microcode_update_match(&new->mc_intel->hdr, old_header->sig, + old_header->pf, old_header->rev); +} + /* * return 0 - no update found * return 1 - found update @@ -258,10 +284,26 @@ static int get_matching_microcode(const void *mc, unsigned int cpu) struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); const struct microcode_header_intel *mc_header = mc; unsigned long total_size = get_totalsize(mc_header); - void *new_mc; + void *new_mc = xmalloc_bytes(total_size); + struct microcode_patch *new_patch = xmalloc(struct microcode_patch); + + if ( !new_patch || !new_mc ) + { + xfree(new_patch); + xfree(new_mc); + printk(XENLOG_ERR "microcode: Can not allocate memory\n"); + return -ENOMEM; + } + memcpy(new_mc, mc, total_size); + new_patch->mc_intel = new_mc; - if ( microcode_update_match(mc, uci->cpu_sig.sig, uci->cpu_sig.pf, - uci->cpu_sig.rev) != NEW_UCODE ) + if ( !match_cpu(new_patch) ) + { + free_patch(new_patch); + return 0; + } + + if ( !microcode_update_cache(new_patch) ) return 0; pr_debug("microcode: CPU%d found a matching microcode update with" @@ -277,6 +319,7 @@ static int get_matching_microcode(const void *mc, unsigned int cpu) memcpy(new_mc, mc, total_size); xfree(uci->mc.mc_intel); uci->mc.mc_intel = new_mc; + return 1; } @@ -287,18 +330,22 @@ static int apply_microcode(unsigned int cpu) unsigned int val[2]; unsigned int cpu_num = raw_smp_processor_id(); struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num); + const struct microcode_intel *mc_intel; + const struct microcode_patch *patch = microcode_get_cache(); /* We should bind the task to the CPU */ BUG_ON(cpu_num != cpu); - if ( uci->mc.mc_intel == NULL ) + if ( !match_cpu(patch) ) return -EINVAL; + mc_intel = patch->mc_intel; + /* serialize access to the physical write to MSR 0x79 */ spin_lock_irqsave(µcode_update_lock, flags); /* write microcode via MSR 0x79 */ - wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)uci->mc.mc_intel->bits); + wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->bits); wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL); /* As documented in the SDM: Do a CPUID 1 here */ @@ -309,19 +356,19 @@ static int apply_microcode(unsigned int cpu) val[1] = (uint32_t)(msr_content >> 32); spin_unlock_irqrestore(µcode_update_lock, flags); - if ( val[1] != uci->mc.mc_intel->hdr.rev ) + if ( val[1] != mc_intel->hdr.rev ) { printk(KERN_ERR "microcode: CPU%d update from revision " "%#x to %#x failed. Resulting revision is %#x.\n", cpu_num, - uci->cpu_sig.rev, uci->mc.mc_intel->hdr.rev, val[1]); + uci->cpu_sig.rev, mc_intel->hdr.rev, val[1]); return -EIO; } printk(KERN_INFO "microcode: CPU%d updated from revision " "%#x to %#x, date = %04x-%02x-%02x \n", cpu_num, uci->cpu_sig.rev, val[1], - uci->mc.mc_intel->hdr.year, - uci->mc.mc_intel->hdr.month, - uci->mc.mc_intel->hdr.day); + mc_intel->hdr.year, + mc_intel->hdr.month, + mc_intel->hdr.day); uci->cpu_sig.rev = val[1]; return 0; @@ -361,7 +408,6 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf, long offset = 0; int error = 0; void *mc; - unsigned int matching_count = 0; /* We should bind the task to the CPU */ BUG_ON(cpu != raw_smp_processor_id()); @@ -379,10 +425,8 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf, * lets keep searching till the latest version */ if ( error == 1 ) - { - matching_count++; error = 0; - } + xfree(mc); } if ( offset > 0 ) @@ -390,7 +434,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf, if ( offset < 0 ) error = offset; - if ( !error && matching_count ) + if ( !error && match_cpu(microcode_get_cache()) ) error = apply_microcode(cpu); return error; @@ -406,6 +450,9 @@ static const struct microcode_ops microcode_intel_ops = { .cpu_request_microcode = cpu_request_microcode, .collect_cpu_info = collect_cpu_info, .apply_microcode = apply_microcode, + .free_patch = free_patch, + .compare_patch = compare_patch, + .match_cpu = match_cpu, }; int __init microcode_init_intel(void) diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h index 73ebe9a..6541c58 100644 --- a/xen/include/asm-x86/microcode.h +++ b/xen/include/asm-x86/microcode.h @@ -12,6 +12,13 @@ enum microcode_match_result { struct cpu_signature; struct ucode_cpu_info; +struct microcode_patch { + union { + struct microcode_intel *mc_intel; + struct microcode_amd *mc_amd; + }; +}; + struct microcode_ops { int (*microcode_resume_match)(unsigned int cpu, const void *mc); int (*cpu_request_microcode)(unsigned int cpu, const void *buf, @@ -19,6 +26,11 @@ struct microcode_ops { int (*collect_cpu_info)(unsigned int cpu, struct cpu_signature *csig); int (*apply_microcode)(unsigned int cpu); int (*start_update)(void); + void (*free_patch)(struct microcode_patch *patch); + bool (*match_cpu)(const struct microcode_patch *patch); + enum microcode_match_result (*compare_patch)( + const struct microcode_patch *new, + const struct microcode_patch *old); }; struct cpu_signature { @@ -39,4 +51,7 @@ struct ucode_cpu_info { DECLARE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info); extern const struct microcode_ops *microcode_ops; +const struct microcode_patch *microcode_get_cache(void); +bool microcode_update_cache(struct microcode_patch *patch); + #endif /* ASM_X86__MICROCODE_H */
to replace the current per-cpu cache 'uci->mc'. With the assumption that all CPUs in the system have the same signature (family, model, stepping and 'pf'), one microcode update matches with one cpu should match with others. And Having multiple microcode revisions on different cpus would cause system unstable and is what should be avoided. Hence, caching only one microcode update is good enough for all cases. Introduce a global variable, microcode_cache, to store the newest matching microcode update. Whenever we get a new valid microcode update, its revision id is compared against that of the microcode update to determine whether the "microcode_cache" needs to be replaced. And now this global cache is loaded to cpu in apply_microcode(). All operations on the cache is expected to be done with the 'microcode_mutex' hold. Note that I deliberately avoid touching 'uci->mc' as I am going to remove it completely in the next patch. Signed-off-by: Chao Gao <chao.gao@intel.com> --- Changes in v7: - reworked to cache only one microcode patch rather than a list of microcode patches. Changes in v6: - constify local variables and function parameters if possible - comment that the global cache is protected by 'microcode_mutex'. and add assertions to catch violations in microcode_{save/find}_patch() Changes in v5: - reword the commit description - find_patch() and save_patch() are abstracted into common functions with some hooks for AMD and Intel --- xen/arch/x86/microcode.c | 36 ++++++++++++++++ xen/arch/x86/microcode_amd.c | 91 +++++++++++++++++++++++++++++++++++++---- xen/arch/x86/microcode_intel.c | 77 +++++++++++++++++++++++++++------- xen/include/asm-x86/microcode.h | 15 +++++++ 4 files changed, 197 insertions(+), 22 deletions(-)