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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Fri, Feb 08, 2019 at 04:41:19AM -0700, Jan Beulich wrote:
>>>> 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.

The return value means whether saving a ucode patch succeeded or failed.
In another word, it also means whether the ucode cache is updated or
not. According to the return value, the caller decides not to do the
expensive late ucode update for some cases (i.e. when admin provides a
old ucode).

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

We can introduce another type (i.e. MIX_UCODE) to denote the ucode
patch which mismatches any saved patches but only differs from current
cpu in stepping or "pf" (or model for AMD).

And save ucodes of MIX_UCODE type and discard ucodes of MIS_UCODE type.

Arch specific callback can determine the type (MIX_UCODE or MIS_UCODE)
according to the supported mixes.

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

Will do

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

No. Will constify the return value.

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

My understanding is saving the cpu signature and ucode revision can
reduce MSR reads to get such information. So "fini" is invoked when
"collect" failed. The apply_microcode() following find_patch() can
access "uci" without invoking "collect" again.

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

Will do.

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

Will reuse the 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.

Then I will remove this check.

Thanks
Chao

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