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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.09.2019 15:53, 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.
> 
> Two threads are on the same core or computing unit iff they have
> the same phys_proc_id and cpu_core_id/compute_unit_id. Based on
> phys_proc_id and cpu_core_id/compute_unit_id, an unique core id
> is generated for each thread. And use a bitmap to reduce the
> number of comparison.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> Alternatively, we can mask the thread id off apicid and use it
> as the unique core id. It needs to introduce new field in cpuinfo_x86
> to record the mask for thread id. So I don't take this way.

It feels a little odd that you introduce a "custom" ID, but it
should be fine without going this alternative route. (You
wouldn't need a new field though, I think, as we've got the
x86_num_siblings one already.)

What I continue to be unconvinced of is for the chosen approach
to be better than briefly unparking a thread on each core, as
previously suggested.

> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -573,6 +573,64 @@ 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 = 0;

I don't think you need the initializer here.

> +    if ( park_offline_cpus )

    if ( !park_offline_cpus )
        return 0;

would allow one level less of indentation of the main part of
the function body.

> +    {
> +        unsigned int cpu, max_bits, core_width;
> +        unsigned int max_sockets = 1, max_cores = 1;
> +        struct cpuinfo_x86 *c = cpu_data;
> +        unsigned long *bitmap;
> +
> +        for_each_present_cpu(cpu)
> +        {
> +            if ( x86_cpu_to_apicid[cpu] == BAD_APICID )
> +                continue;
> +
> +            /* Note that cpu_to_socket() get an ID starting from 0. */
> +            if ( cpu_to_socket(cpu) + 1 > max_sockets )

Instead of "+ 1", why not >= ?

> +                max_sockets = cpu_to_socket(cpu) + 1;
> +
> +            if ( c[cpu].x86_max_cores > max_cores )
> +                max_cores = c[cpu].x86_max_cores;

What guarantees .x86_max_cores to be valid? Onlining a hot-added
CPU is a two step process afaict, XENPF_cpu_hotadd followed by
XENPF_cpu_online. In between the CPU would be marked present
(and cpu_add() would also have filled x86_cpu_to_apicid[cpu]),
but cpu_data[cpu] wouldn't have been filled yet afaict. This
also makes the results of the cpu_to_*() unreliable that you use
in unique_core_id().

However, if we assume sufficient similarity between CPU
packages (as you've done elsewhere in this series iirc), this
may not be an actual problem. But it wants mentioning in a code
comment, I think. Plus at the very least you depend on the used
cpu_data[] fields to not contain unduly large values (and hence
you e.g. depend on cpu_data[] not gaining an initializer,
setting the three fields of interest to their INVALID_* values,
as currently done by identify_cpu()).

> +        }
> +
> +        core_width = fls(max_cores);
> +        max_bits = max_sockets << core_width;
> +        bitmap = xzalloc_array(unsigned long, BITS_TO_LONGS(max_bits));
> +        if ( !bitmap )
> +            return -ENOMEM;
> +
> +        for_each_present_cpu(cpu)
> +        {
> +            if ( cpu_online(cpu) || x86_cpu_to_apicid[cpu] == BAD_APICID )
> +                continue;
> +
> +            __set_bit(unique_core_id(cpu, core_width), bitmap);
> +        }
> +
> +        for_each_online_cpu(cpu)
> +            __clear_bit(unique_core_id(cpu, core_width), bitmap);
> +
> +        ret = (find_first_bit(bitmap, max_bits) < max_bits);

I think bitmap_empty() would be a cheaper operation for the purpose
you have, especially if further up you rounded up max_bits to a
multiple of BITS_PER_LONG.

Jan

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