Re: [Xen-devel] [PATCH v8 12/16] microcode: split out apply_microcode() from cpu_request_microcode()

[Chao Gao <chao.gao@xxxxxxxxx>]

On Mon, Aug 05, 2019 at 09:38:00AM +0000, Jan Beulich wrote:
>On 05.08.2019 09:36, Chao Gao wrote:
>> On Fri, Aug 02, 2019 at 03:40:55PM +0000, Jan Beulich wrote:
>>> On 01.08.2019 12:22, Chao Gao wrote:
>>>> --- a/xen/arch/x86/microcode.c
>>>> +++ b/xen/arch/x86/microcode.c
>>>> @@ -189,12 +189,20 @@ static DEFINE_SPINLOCK(microcode_mutex);
>>>>    
>>>>    DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
>>>>    
>>>> -struct microcode_info {
>>>> -    unsigned int cpu;
>>>> -    uint32_t buffer_size;
>>>> -    int error;
>>>> -    char buffer[1];
>>>> -};
>>>> +/*
>>>> + * Return a patch that covers current CPU. If there are multiple patches,
>>>> + * return the one with the highest revision number. Return error If no
>>>> + * patch is found and an error occurs during the parsing process. 
>>>> Otherwise
>>>> + * return NULL.
>>>> + */
>>>> +static struct microcode_patch *microcode_parse_blob(const char *buf,
>>>> +                                                    uint32_t len)
>>>
>>> Btw - you'd have less issues with line length if you omitted the
>>> "microcode_" prefix from static functions.
>>>
>>>> @@ -250,49 +251,88 @@ bool microcode_update_cache(struct microcode_patch 
>>>> *patch)
>>>>        return true;
>>>>    }
>>>>    
>>>> -static int microcode_update_cpu(const void *buf, size_t size)
>>>> +/*
>>>> + * Load a microcode update to current CPU.
>>>> + *
>>>> + * If no patch is provided, the cached patch will be loaded. Microcode 
>>>> update
>>>> + * during APs bringup and CPU resuming falls into this case.
>>>> + */
>>>> +static int microcode_update_cpu(const struct microcode_patch *patch)
>>>>    {
>>>> -    int err;
>>>> -    unsigned int cpu = smp_processor_id();
>>>> -    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>>>> +    int err = microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
>>>> +
>>>> +    if ( unlikely(err) )
>>>> +        return err;
>>>>    
>>>>        spin_lock(&microcode_mutex);
>>>>    
>>>> -    err = microcode_ops->collect_cpu_info(sig);
>>>> -    if ( likely(!err) )
>>>> -        err = microcode_ops->cpu_request_microcode(buf, size);
>>>> +    if ( patch )
>>>> +    {
>>>> +        /*
>>>> +         * If a patch is specified, it should has newer revision than
>>>> +         * that of the patch cached.
>>>> +         */
>>>> +        if ( microcode_cache &&
>>>> +             microcode_ops->compare_patch(patch, microcode_cache) != 
>>>> NEW_UCODE )
>>>
>>> While I see that you've taken care of the one case in Intel specific
>>> code, this is again a case where I don't think you can validly call
>>> this hook in the Intel case. Albeit maybe it is okay, provided that
>>> the caller has already verified it against the CPU signature. Then
>>> again I wonder why this check gets done here rather than in the
>>> caller (next to that other check) anyway. Afaics this way you'd
>>> also avoid re-checking on every CPU a CPU-independent property.
>> 
>> As said in an earlier reply to patch 6, ->compare_patch can
>> be simplified a lot. Do you think it is fine to call it here
>> with that change?
>
>As said there - yes, this looks to be an option.
>
>> About avoiding re-checking, we should check it with "microcode_mutex"
>> held otherwise we cannot ensure nobody is updating the cache. If we want
>> to avoid re-checking, then this lock is held for a long time from loading
>> on the first core to the last core. And also for early loading and late
>> loading, we pass 'NULL' to this function on following CPUs after the
>> first successful loading. I am afraid that there is no redundant checking.
>
>There should not be any updating of the cache while one (system-wide)
>ucode load operation is in progress, or else you risk leaving the
>system in a "mixed ucode versions" state in the end. As said, also

Yes. I agree.

>from a logical flow-of-events perspective it seems to me that it should
>be the caller to validate applicability of the patch before calling
>here.

Maybe we can just remove this check. The check is to avoid loading an
older update than the cached one (an error is returned if it is the
case). But actually, ->apply_microcode() will ensure the provided
update's revision is newer than current update revision by calling
match_cpu(). So probably this check is redundant.

Thanks
Chao

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