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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.11.2019 12:51, Chao Gao wrote:
> 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.

Dom0 can _fully_ bring up CPUs, but not with the intent of _just_
getting their ucode updated.

>> 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.

Aiui it'll be correct again only if the parked map gets allocated
unconditionally.

Jan

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