Re: [Xen-devel] [PATCH v7 03/10] microcode: introduce a global cache of ucode patch

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.05.19 at 10:31, <chao.gao@xxxxxxxxx> wrote:
> +bool microcode_update_cache(struct microcode_patch *patch)
> +{
> +
> +    ASSERT(spin_is_locked(&microcode_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(&microcode_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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel