Re: [Xen-devel] [PATCH v10 12/16] x86/microcode: Synchronize late microcode loading

[Chao Gao <chao.gao@intel.com>]

On Thu, Sep 12, 2019 at 05:32:22PM +0200, Jan Beulich wrote:
>On 12.09.2019 09:22, Chao Gao wrote:
>> @@ -264,38 +336,158 @@ static int microcode_update_cpu(const struct microcode_patch *patch)
>>      return err;
>>  }
>>  
>> -static long do_microcode_update(void *patch)
>> +static bool wait_for_state(unsigned int state)
>> +{
>> +    while ( loading_state != state )
>> +    {
>> +        if ( state != LOADING_EXIT && loading_state == LOADING_EXIT )
>> +            return false;
>
>This is at least somewhat confusing: There's no indication here
>that "loading_state" may change behind the function's back. So
>in general one could be (and I initially was) tempted to suggest
>dropping the apparently redundant left side of the &&. But that
>would end up wrong if the compiler translates the above to two
>separate reads of "loading_state". Therefore I'd like to suggest
>
>static bool wait_for_state(typeof(loading_state) state)
>{
>    typeof(loading_state) cur_state;
>
>    while ( (cur_state = ACCESS_ONCE(loading_state)) != state )
>    {
>        if ( cur_state == LOADING_EXIT )
>            return false;
>        cpu_relax();
>    }
>
>    return true;
>}
>
>or something substantially similar (if, e.g., you dislike the
>use of typeof() here).

The code snippet above is terrific. Will take it.

>
>> +static int secondary_thread_fn(void)
>> +{
>> +    unsigned int primary = cpumask_first(this_cpu(cpu_sibling_mask));
>> +
>> +    if ( !wait_for_state(LOADING_CALLIN) )
>> +        return -EBUSY;
>> +
>> +    cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
>> +
>> +    if ( !wait_for_state(LOADING_EXIT) )
>> +        return -EBUSY;
>
>This return looks to be unreachable, doesn't it?

Yes. I will use a variable to hold its return value and assert the
return value is always true.

Other comments are reasonable and I will follow your suggestion.

Thanks
Chao

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