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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.05.18 at 10:15, <chao.gao@xxxxxxxxx> wrote:
> On Mon, Apr 30, 2018 at 09:25:26AM -0600, Jan Beulich wrote:
>>>>> On 25.04.18 at 13:46, <chao.gao@xxxxxxxxx> wrote:
>>> +static int do_microcode_update(void *_info)
>>> +{
>>> +    struct microcode_info *info = _info;
>>> +    unsigned int cpu = smp_processor_id();
>>> +    int ret;
>>> +
>>> +    ret = wait_for_cpus(&info->cpu_in, MICROCODE_DEFAULT_TIMEOUT);
>>> +    if ( ret )
>>> +        return ret;
>>> +    /*
>>> +     * Logical threads which set the first bit in cpu_sibling_mask can do
>>> +     * the update. Other sibling threads just await the completion of
>>> +     * microcode update.
>>> +     */
>>> +    if ( cpumask_test_and_set_cpu(
>>> +                cpumask_first(per_cpu(cpu_sibling_mask, cpu)), 
>>> &info->cpus) )
>>> +        ret = microcode_update_cpu(info->buffer, info->buffer_size);
>>
>>Isn't the condition inverted (i.e. missing a ! )?
> 
> Yes.
> 
>>
>>Also I take it that you've confirmed that loading ucode in parallel on 
>>multiple
>>cores of the same socket is not a problem? The comment in the last hunk
>>suggests otherwise.
> 
> No. In microcode_update_cpu(), microcode_mutex makes the update
> sequential.

Oh, right, of course.

>>> +
>>> +    return ret;
>>>  }
>>
>>You're losing this return value (once for every CPU making it into this
>>function).
> 
> I don't understand this remark. This function is called by
> stop_machine_run(). stop_machine_run() could return error if
> any cpu failed during update. We don't care the specific CPU and
> how many CPUs failed to do the update.

Then please check your stop_machine_run() invocation again, in particular
what happens with that function's return value.

Jan



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