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

Jan

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