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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.02.19 at 04:59, <chao.gao@xxxxxxxxx> wrote:
> 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).

Would you mind extending the comment to the effect?

>>> +/* Find a ucode patch who has newer revision than the one in use */
>>> +struct microcode_patch *find_patch(unsigned int cpu)
>>> +{
>>> +    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.

I fail to see the connection between "saving the cpu signature and
ucode revision" and the apparent layering violation here. Could you
help me with this?

Jan



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