Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check

[Chao Gao <chao.gao@intel.com>]

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.gao@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, &microcode_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, &microcode_cache);
>> +    return 1;
>> +}
>> +
>> +static struct microcode_header_amd *find_patch(unsigned int cpu)
>> +{
>> +    struct ucode_patch *ucode_patch;
>> +
>> +    list_for_each_entry(ucode_patch, &microcode_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, &microcode_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, &microcode_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