Re: [Xen-devel] [PATCH v5 3/8] microcode: introduce the global microcode cache

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.01.19 at 08:06, <chao.gao@xxxxxxxxx> wrote:
> @@ -208,6 +210,58 @@ static void microcode_fini_cpu(unsigned int cpu)
>      spin_unlock(&microcode_mutex);
>  }
>  
> +/* Save a ucode patch to the global cache list */
> +bool save_patch(struct microcode_patch *new_patch)
> +{
> +    struct microcode_patch *microcode_patch;
> +
> +    list_for_each_entry(microcode_patch, &microcode_cache, list)
> +    {
> +        enum microcode_match_result result =
> +            microcode_ops->replace_patch(new_patch, microcode_patch);
> +
> +        switch ( result )
> +        {
> +        case OLD_UCODE:
> +            microcode_ops->free_patch(new_patch);
> +            return false;
> +
> +        case NEW_UCODE:
> +            microcode_ops->free_patch(microcode_patch);
> +            return true;
> +
> +        case MIS_UCODE:
> +            continue;
> +
> +        default:
> +            ASSERT_UNREACHABLE();
> +            return 0;

false (or true; either value is going to be fine/wrong here afaict)

Anyway I'm having some difficulty seeing what the intended
meaning of the return value is, and without that being clear I
also can't make up my mind whether I agree with the cases
here.

> +        }
> +    }
> +    list_add_tail(&new_patch->list, &microcode_cache);

Hmm, you add _every_ patch producing MIS_UCODE to the list.
This is going to be a long list then - there are over 100 blobs in
the latest dropping, and this number is only going to grow.
Saving blobs is useful only for cases where mixed steppings are
actually supported. I guess that wouldn't go much beyond the
stepping and/or "pf" differing, but family and model matching up.

Additionally list management generally requires locking. This
function doesn't even have a comment saying what lock(s) is
(are) necessary to be held (same elsewhere).

Finally please add blank lines ahead of the line above as well
as (not just here) ahead of the main return statement of the
function.

> +/* Find a ucode patch who has newer revision than the one in use */
> +struct microcode_patch *find_patch(unsigned int cpu)

Is the caller allowed to alter the returned object? If not, you want
to return a pointer to const.

> +{
> +    struct microcode_patch *microcode_patch;
> +    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> +
> +    if ( microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig) )
> +    {
> +        __microcode_fini_cpu(cpu);

This is kind of a library function - I can't see how it could legitimately
invoke "fini", the more that you do here but not ...

> +        return NULL;
> +    }
> +
> +    list_for_each_entry(microcode_patch, &microcode_cache, list)
> +    {
> +        if ( microcode_ops->match_cpu(microcode_patch, cpu) )
> +            return microcode_patch;
> +    }
> +    return NULL;

... here.

> +static enum microcode_match_result replace_patch(struct microcode_patch *new,
> +                                                 struct microcode_patch *old)
> +{
> +    struct microcode_amd *new_mc = new->data;
> +    struct microcode_header_amd *new_header = new_mc->mpb;
> +    struct microcode_amd *old_mc = old->data;
> +    struct microcode_header_amd *old_header = old_mc->mpb;

const (all four of them afaict)

> --- a/xen/arch/x86/microcode_intel.c
> +++ b/xen/arch/x86/microcode_intel.c
> @@ -147,6 +147,15 @@ static enum microcode_match_result 
> microcode_update_match(
>      return MIS_UCODE;
>  }
>  
> +static bool match_cpu(const struct microcode_patch *patch, unsigned int cpu)
> +{
> +    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);

const

> +static enum microcode_match_result replace_patch(struct microcode_patch *new,
> +                                                 struct microcode_patch *old)
> +{
> +    struct microcode_header_intel *old_header = old->data;

const

> +    enum microcode_match_result ret =
> +                microcode_update_match(new->data, old_header->sig,
> +                                       old_header->pf, old_header->rev);
> +
> +    if ( ret == NEW_UCODE )
> +        list_replace(&old->list, &new->list);

I think it would be better to leave actual list maintenance to generic
code. That way the function parameters can also be pointer-to-const.

> @@ -248,6 +277,25 @@ 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 microcode_patch *microcode_patch = xmalloc(struct 
> microcode_patch);
> +    void *new_mc2 = xmalloc_bytes(total_size);

Why don't you use the already existing new_mc here?

> @@ -276,18 +324,24 @@ 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);
> +    struct microcode_intel *mc_intel;
> +    struct microcode_patch *patch;
>  
>      /* We should bind the task to the CPU */
>      BUG_ON(cpu_num != cpu);
>  
> -    if ( uci->mc.mc_intel == NULL )
> +    patch = find_patch(cpu);
> +    if ( !patch )
>          return -EINVAL;
>  
> +    mc_intel = patch->data;
> +    BUG_ON(!mc_intel);

I'm not convinced this is a useful check - you never save_patch()
anything that has a NULL pointer here. And general corruption might
put bad values other than NULL into the field.

Jan


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