Re: [Xen-devel] [PATCH v1 2/2] microcode: reject late ucode loading if any core is parked

[Chao Gao <chao.gao@xxxxxxxxx>]

On Thu, Nov 21, 2019 at 11:21:13AM +0100, Jan Beulich wrote:
>On 21.11.2019 00:05, Chao Gao wrote:
>> If a core with all of its thread being parked, late ucode loading
>> which currently only loads ucode on online threads would lead to
>> differing ucode revisions in the system. In general, keeping ucode
>> revision consistent would be less error-prone. To this end, if there
>> is a parked thread doesn't have an online sibling thread, late ucode
>> loading is rejected.
>
>I'm confused. I thought we had agreed that the long term solution
>would be to briefly bring online a thread of cores with all their
>threads parked.

I don't remeber that we reached such an aggrement. But if it happened,
I am really sorry for forgeting it.

Actually, I think Dom0 has the information (cpu topology and each cpu's
offline/online status) to decide if there is a parked core or not.
IMO, rejecting late loading in this case is just a defense check. Dom0
is able to correct the situation by bringing up some cpus.

>What you do here was meant to be a temporary step
>only for 4.13, for which it is too late now (unless Jürgen
>indicates otherwise).
>
>> --- a/xen/arch/x86/microcode.c
>> +++ b/xen/arch/x86/microcode.c
>> @@ -584,6 +584,51 @@ static int do_microcode_update(void *patch)
>>      return ret;
>>  }
>>  
>> +static unsigned int unique_core_id(unsigned int cpu, unsigned int 
>> socket_shift)
>> +{
>> +    unsigned int core_id = cpu_to_cu(cpu);
>> +
>> +    if ( core_id == INVALID_CUID )
>> +        core_id = cpu_to_core(cpu);
>> +
>> +    return (cpu_to_socket(cpu) << socket_shift) + core_id;
>> +}
>> +
>> +static int has_parked_core(void)
>> +{
>> +    int ret;
>> +    unsigned int cpu, max_bits, core_width;
>> +    unsigned int max_sockets = 1, max_cores = 1;
>> +    unsigned long *bitmap;
>> +
>> +    if ( !park_offline_cpus )
>> +        return 0;
>> +
>> +    for_each_parked_cpu(cpu)
>> +    {
>> +        /* Note that cpu_to_socket() get an ID starting from 0. */
>> +        max_sockets = max(max_sockets, cpu_to_socket(cpu) + 1);
>> +        max_cores = max(max_cores, cpu_data[cpu].x86_max_cores);
>> +    }
>> +
>> +    core_width = fls(max_cores);
>> +    max_bits = max_sockets << core_width;
>
>Isn't this off by one? If max_cores is 1, you don't need to shift
>max_sockets (or the cpu_to_socket() value in unique_core_id()) at
>all, for example.
>
>With this in mind, instead of the park_offline_cpus check at the
>top of the function, wouldn't it make sense to check here whether
>max_sockets and max_cores are both still 1, in which case at
>least one thread of the only core of the only socket in the system
>is obviously still online (the one we're running on)?

Agree. Will follow your suggestion.

Thanks
Chao

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