On Wed, Nov 28, 2018 at 01:00:14PM +0100, Roger Pau Monné wrote: >On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote: >> ... and search caches to find a suitable one when loading. > >Why do you need to save all of them? You are only going to load a >single microcode, so I don't understand the need to cache them all. > >> With this cache, the existing 'uci->mc' structure is redundent. >> I deliberately avoid touching 'uci->mc' as I am going to remove >> it completely in the next patch. >> >> Signed-off-by: Chao Gao <chao....@intel.com> >> --- >> xen/arch/x86/microcode.c | 2 + >> xen/arch/x86/microcode_amd.c | 93 +++++++++++++++++++++++++++++++++++--- >> xen/arch/x86/microcode_intel.c | 99 >> ++++++++++++++++++++++++++++++++++++++--- >> xen/include/asm-x86/microcode.h | 11 +++++ >> 4 files changed, 193 insertions(+), 12 deletions(-) >> >> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c >> index 4163f50..4f2db88 100644 >> --- a/xen/arch/x86/microcode.c >> +++ b/xen/arch/x86/microcode.c >> @@ -61,6 +61,8 @@ static struct ucode_mod_blob __initdata ucode_blob; >> */ >> static bool_t __initdata ucode_scan; >> >> +LIST_HEAD(microcode_cache); >> + >> void __init microcode_set_module(unsigned int idx) >> { >> ucode_mod_idx = idx; >> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c >> index fba44cc..a686a87 100644 >> --- a/xen/arch/x86/microcode_amd.c >> +++ b/xen/arch/x86/microcode_amd.c >> @@ -190,22 +190,90 @@ static bool_t microcode_fits(const struct >> microcode_amd *mc_amd, >> return 1; >> } >> >> +static struct ucode_patch *alloc_ucode_patch(struct microcode_amd *mc_amd) >> +{ >> + struct ucode_patch *ucode_patch = xmalloc(struct ucode_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 ( !ucode_patch || !cache || !mpb || !equiv_cpu_table ) >> + return ERR_PTR(-ENOMEM); > >This can leak memory. For example failing to allocate only >equiv_cpu_table would leak all the other allocations. Ie: you need to >xfree all of them before returning.
Yes. I fully agree :). > >> + >> + 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); > >Don't you need to do: > >cache->equiv_cpu_table = equiv_cpu_table; >cache->mpb = mpb; Yes. I should have done this. > >Before attempting to memcpy to it? Or else you will memcpy to random >locations because the contents of cache are not zeroed. > >IMO making such modifications to the AMD code without testing it is >very dangerous. Could you get an AMD system or ask an AMD dev to test >it? I would try with the AMD SVM maintainers. It is improbable for me to find an AMD machine in my team. I will copy AMD SVM maintainers in the coming versions and ask them to help to test this series. > >> + cache->equiv_cpu_table_size = mc_amd->equiv_cpu_table_size; >> + cache->mpb_size = mc_amd->mpb_size; >> + ucode_patch->data = cache; > >Newline. > >> + return ucode_patch; >> +} >> + >> +static void free_ucode_patch(struct ucode_patch *ucode_patch) >> +{ >> + struct microcode_amd *mc_amd = ucode_patch->data; >> + >> + xfree(mc_amd->equiv_cpu_table); >> + xfree(mc_amd->mpb); >> + xfree(mc_amd); >> + xfree(ucode_patch); >> +} >> + >> +/* >> + * save a micrcode to the cache list >> + * return 1: added successfully >> + * 0: replaced an existing entry >> + * -1: failed as a newer microcode was already cached > >Using an enum (like you do in patch #1) would be better and less >cryptic IMO. > >> + */ >> +static int save_patch(struct ucode_patch *new_patch) >> +{ >> + struct ucode_patch *ucode_patch; >> + struct microcode_amd *new_mc = new_patch->data; >> + struct microcode_header_amd *new_header = new_mc->mpb; >> + >> + list_for_each_entry(ucode_patch, µcode_cache, list) >> + { >> + struct microcode_amd *old_mc = ucode_patch->data; >> + struct microcode_header_amd *old_header = old_mc->mpb; >> + >> + if ( new_header->processor_rev_id == old_header->processor_rev_id ) >> + { >> + if ( new_header->patch_id <= old_header->patch_id ) >> + return -1; >> + list_replace(&ucode_patch->list, &new_patch->list); >> + free_ucode_patch(ucode_patch); >> + return 0; >> + } >> + } > >This could be made common code with a specific hook for AMD and Intel >in order to do the comparison, so that at least the loop over the >list of ucode entries could be shared. Something like pt_pirq_iterate()? Will give it a try. > >> + list_add_tail(&new_patch->list, µcode_cache); >> + return 1; >> +} >> + >> +static struct microcode_header_amd *find_patch(unsigned int cpu) >> +{ >> + struct ucode_patch *ucode_patch; >> + >> + list_for_each_entry(ucode_patch, µcode_cache, list) >> + { >> + if ( microcode_fits(ucode_patch->data, cpu) ) >> + return ((struct microcode_amd *)ucode_patch->data)->mpb; >> + } >> + return NULL; > >This also looks suitable to be moved to common code with dedicated AMD >and Intel hooks. > >> +} >> + >> 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; >> >> /* We should bind the task to the CPU */ >> BUG_ON(raw_smp_processor_id() != cpu); >> >> - if ( mc_amd == NULL ) >> - return -EINVAL; >> - >> - hdr = mc_amd->mpb; >> + hdr = find_patch(cpu); >> if ( hdr == NULL ) >> return -EINVAL; >> >> @@ -491,6 +559,21 @@ static int cpu_request_microcode(unsigned int cpu, >> const void *buf, >> while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, >> &offset)) == 0 ) >> { >> + struct ucode_patch *ucode_patch; >> + >> + /* >> + * Save this microcode before checking the signature. It is to >> + * optimize microcode update on a mixed family system. Parsing > >Er, is it possible to have a system with CPUs of different family? >What's going to happen with CPUs having different features? I have no idea. That each cpu has a per-cpu variable to store the microcode rather than a global one gives me a feeling that the current implementation wants to make it work on a system with CPUs of different family. > >> + * microcode file is only done once on one of the CPUs, and >> + * during this process microcode cache is created. Other CPUs >> + * needn't parse the same micrcode file again and again. >> + * Instead, they just load the matched and latest microcode in >> + * the caches. >> + */ >> + ucode_patch = alloc_ucode_patch(mc_amd); >> + if ( !IS_ERR_OR_NULL(ucode_patch) && (save_patch(ucode_patch) < 0) ) >> + free_ucode_patch(ucode_patch); >> + >> if ( microcode_fits(mc_amd, cpu) ) >> { >> error = apply_microcode(cpu); >> diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c >> index 8d9a3b2..c4f812f 100644 >> --- a/xen/arch/x86/microcode_intel.c >> +++ b/xen/arch/x86/microcode_intel.c >> @@ -251,6 +251,42 @@ static int microcode_sanity_check(void *mc) >> } >> >> /* >> + * save a micrcode to the cache list >> + * return 1: added successfully >> + * 0: replaced an existing entry >> + * -1: failed as a newer microcode was already cached >> + */ >> +static int save_patch(struct ucode_patch *new_patch) >> +{ >> + void *mc; >> + struct ucode_patch *ucode_patch; >> + >> + ASSERT(new_patch); >> + >> + mc = new_patch->data; >> + list_for_each_entry(ucode_patch, µcode_cache, list) >> + { >> + struct microcode_header_intel *saved_header = ucode_patch->data; >> + int ret; >> + >> + ret = microcode_update_match(mc, saved_header->sig, >> saved_header->pf, >> + saved_header->rev); > >You can initialize ret at definition time. > >> + if ( ret == OLD_UCODE ) >> + return -1; >> + if ( ret == MIS_UCODE ) >> + continue; >> + >> + list_replace(&ucode_patch->list, &new_patch->list); >> + xfree(ucode_patch->data); >> + xfree(ucode_patch); >> + return 0; >> + } >> + >> + list_add_tail(&new_patch->list, µcode_cache); >> + return 1; >> +} >> + >> +/* >> * return 0 - no update found >> * return 1 - found update >> * return < 0 - error >> @@ -261,6 +297,30 @@ static int get_matching_microcode(const void *mc, >> unsigned int cpu) >> const struct microcode_header_intel *mc_header = mc; >> unsigned long total_size = get_totalsize(mc_header); >> void *new_mc; >> + struct ucode_patch *ucode_patch = xmalloc(struct ucode_patch); >> + void *new_mc2 = xmalloc_bytes(total_size); >> + >> + /* >> + * Save this microcode before checking the signature. It is to >> + * optimize microcode update on a mixed family system. Parsing >> + * microcode file is only done once on one of the CPUs, and >> + * during this process microcode cache is created. Other CPUs >> + * needn't parse the same micrcode file again and again. >> + * Instead, they just load the matched and latest microcode in >> + * the caches. >> + */ >> + if ( !ucode_patch || !new_mc2 ) >> + { >> + printk(KERN_ERR "microcode: error! Can not allocate memory\n"); > >Since this code is not imported from Linux please use XENLOG_ERR. You >also need to xfree both structs in order to avoid leaking memory. Will fix this. Other comments are fine with me and I will follow your suggestions. Thanks Chao _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel