Re: [Xen-devel] [PATCH 1/2] x86/microcode: Synchronize late microcode loading

[Chao Gao <chao.gao@xxxxxxxxx>]

On Thu, Apr 12, 2018 at 09:29:34AM -0700, Raj, Ashok wrote:
>On Fri, Mar 30, 2018 at 02:59:00PM +0800, Chao Gao wrote:
>> From: Gao Chao <chao.gao@xxxxxxxxx>
>> 
>> This patch is to backport microcode improvement patches from linux
>> kernel. Below are the original patches description:
>> 
>>     commit a5321aec6412b20b5ad15db2d6b916c05349dbff
>>     Author: Ashok Raj <ashok.raj@xxxxxxxxx>
>>     Date:   Wed Feb 28 11:28:46 2018 +0100
>> 
>>      x86/microcode: Synchronize late microcode loading
>> 
>>      Original idea by Ashok, completely rewritten by Borislav.
>> 
>>      Before you read any further: the early loading method is still the
>>      preferred one and you should always do that. The following patch is
>>      improving the late loading mechanism for long running jobs and cloud use
>>      cases.
>> 
>>      Gather all cores and serialize the microcode update on them by doing it
>>      one-by-one to make the late update process as reliable as possible and
>>      avoid potential issues caused by the microcode update.
>> 
>>      [ Borislav: Rewrite completely. ]
>> 
>>      Co-developed-by: Borislav Petkov <bp@xxxxxxx>
>>      Signed-off-by: Ashok Raj <ashok.raj@xxxxxxxxx>
>>      Signed-off-by: Borislav Petkov <bp@xxxxxxx>
>>      Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>      Tested-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>>      Tested-by: Ashok Raj <ashok.raj@xxxxxxxxx>
>
>The tested by tags were good for linux upstream. Can you make sure
>you add your name under tested-by for xen?

Tested-by doesn't mean they have tested this patch. I just put the
original commits description here.

If Xen permits such tested-by from the sender and author, I will add one.

>
>>      Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>>      Cc: Arjan Van De Ven <arjan.van.de.ven@xxxxxxxxx>
>>      Link: https://lkml.kernel.org/r/20180228102846.13447-8-bp@xxxxxxxxx
>> 
>> +static int do_microcode_update(void *_info)
>> +{
>> +    struct microcode_info *info = _info;
>> +    int error, ret = 0;
>> +
>> +    error = __wait_for_cpus(&info->cpu_in, USEC_PER_SEC);
>> +    if ( error )
>> +    {
>> +        ret = -EBUSY;
>> +        return ret;
>> +    }
>> +
>> +    error = microcode_update_cpu(info->buffer, info->buffer_size);
>> +    if ( error && !ret )
>> +        ret = error;
>
>Isn't this redundant? can ret become non-zero in the check above?

Yes, it is redundant. I will also remove 'error' field from struct
microcode_info  because stop_machine_run already has the ability to return the
first error code. Hence, don't need to implement it again here (this is the
reason why the above fragment is weird).

Thanks
Chao

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