Re: [Xen-devel] [PATCH v9 13/15] x86/microcode: Synchronize late microcode loading

[Chao Gao <chao.gao@xxxxxxxxx>]

On Mon, Aug 19, 2019 at 11:27:36AM +0100, Sergey Dyasli wrote:
>> +static int master_thread_fn(const struct microcode_patch *patch)
>> +{
>> +    unsigned int cpu = smp_processor_id();
>> +    int ret = 0;
>> +
>> +    while ( loading_state != LOADING_CALLIN )
>> +        cpu_relax();
>> +
>> +    cpumask_set_cpu(cpu, &cpu_callin_map);
>> +
>> +    while ( loading_state != LOADING_ENTER )
>> +        cpu_relax();
>
>If I'm reading it right, this will wait forever in case when...
>
>> +
>> +    /*
>> +     * If an error happened, control thread would set 'loading_state'
>> +     * to LOADING_EXIT. Don't perform ucode loading for this case
>> +     */
>> +    if ( loading_state == LOADING_EXIT )
>> +        return ret;

I tried to check whether there was an error here. But as you said, we
cannot reach here if 'control thread' set loading_state from LOADING_CALLIN
to LOADING_EXIT. I will do this check in the while-loop right above.

>> +
>> +    ret = microcode_ops->apply_microcode(patch);
>> +    if ( !ret )
>> +        atomic_inc(&cpu_updated);
>> +    atomic_inc(&cpu_out);
>> +
>> +    while ( loading_state != LOADING_EXIT )
>> +        cpu_relax();
>> +
>> +    return ret;
>> +}
>> +
>> +static int control_thread_fn(const struct microcode_patch *patch)
>>  {
>> -    unsigned int cpu;
>> +    unsigned int cpu = smp_processor_id(), done;
>> +    unsigned long tick;
>> +    int ret;
>>  
>> -    /* Store the patch after a successful loading */
>> -    if ( !microcode_update_cpu(patch) && patch )
>> +    /* Allow threads to call in */
>> +    loading_state = LOADING_CALLIN;
>> +    smp_mb();
>> +
>> +    cpumask_set_cpu(cpu, &cpu_callin_map);
>> +
>> +    /* Waiting for all threads calling in */
>> +    ret = wait_for_condition(wait_cpu_callin,
>> +                             (void *)(unsigned long)num_online_cpus(),
>> +                             MICROCODE_CALLIN_TIMEOUT_US);
>> +    if ( ret ) {
>> +        loading_state = LOADING_EXIT;
>> +        return ret;
>> +    }
>
>...this condition holds. Have you actually tested this case?

I didn't craft a case to verify the error-handling path. And I believe
that you are right. 

Thanks
Chao

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