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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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.

> +
> +    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;

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.

> +    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.

> +    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?

> +         * 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.

Thanks, Roger.

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