| Message ID | 1569506015-26938-8-git-send-email-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | improve late microcode loading | expand |
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@intel.com> > --- > 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
On Fri, Sep 27, 2019 at 01:19:16PM +0200, Jan Beulich wrote: >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@intel.com> >> --- >> 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. It isn't so easy to go the same way as set_cx_pminfo(). 1. NMI handler on parked threads is changed to a nop. To load ucode in NMI handler, we have to switch back to normal NMI handler in default_idle(). But it conflicts with what the comments in play_dead() implies: it is not safe to call normal NMI handler after cpu_exit_clear(). 2. A precondition of unparking a thread on each core, we need to find out exactly all parked cores and wake up one thread of each of them. Then in theory, what this patch does is only part of unparking a thread on each core. I don't mean they are hard to address. But we need to take care of them. Given that, IMO, this patch is much straightforward. > >> --- 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(). Indeed. I agree. > >However, if we assume sufficient similarity between CPU >packages (as you've done elsewhere in this series iirc), this Yes. >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()). Can we skip those threads whose socket ID is invalid and initialize the three fields in cpu_add()? Or maintain a bitmap for parked threads to help distinguish them from real offlined threads, and go through parked threads here? Thanks Chao
On 30.09.2019 08:43, Chao Gao wrote: > On Fri, Sep 27, 2019 at 01:19:16PM +0200, Jan Beulich wrote: >> 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. > > It isn't so easy to go the same way as set_cx_pminfo(). > > 1. NMI handler on parked threads is changed to a nop. To load ucode in > NMI handler, we have to switch back to normal NMI handler in > default_idle(). But it conflicts with what the comments in play_dead() > implies: it is not safe to call normal NMI handler after > cpu_exit_clear(). Right - pointing at the Cx handling for reference was perhaps not the best choice. Here we'd need to truly online a core, remembering to offline it again right after the ucode update. > 2. A precondition of unparking a thread on each core, we need to find > out exactly all parked cores and wake up one thread of each of them. > Then in theory, what this patch does is only part of unparking a thread > on each core. Possibly, but you've suggested a possibly better alternative further down. >> 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()). > > Can we skip those threads whose socket ID is invalid and initialize > the three fields in cpu_add()? What would you initialize them to in cpu_add()? You don't know their values yet, do you? > Or maintain a bitmap for parked threads to help distinguish them from > real offlined threads, and go through parked threads here? I think this is the better approach in the long run. I've been at least considering addition of such a bitmap for other reasons as well. Jan
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c index b9fa8bb..b70eb16 100644 --- 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; + + if ( park_offline_cpus ) + { + 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 ) + max_sockets = cpu_to_socket(cpu) + 1; + + if ( c[cpu].x86_max_cores > max_cores ) + max_cores = c[cpu].x86_max_cores; + } + + 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); + xfree(bitmap); + } + + return ret; +} + int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len) { int ret; @@ -611,6 +669,23 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len) */ ASSERT(cpumask_first(&cpu_online_map) == nmi_cpu); + /* + * If there is a core with all of its threads parked, late loading may + * cause differing ucode revisions in the system. Refuse this operation. + */ + ret = has_parked_core(); + if ( ret ) + { + if ( ret > 0 ) + { + printk(XENLOG_WARNING + "Ucode loading aborted: found a parked core\n"); + ret = -EPERM; + } + xfree(buffer); + goto put; + } + patch = parse_blob(buffer, len); xfree(buffer); if ( IS_ERR(patch) ) diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h index c92956f..753deec 100644 --- a/xen/include/asm-x86/processor.h +++ b/xen/include/asm-x86/processor.h @@ -171,6 +171,7 @@ extern unsigned int init_intel_cacheinfo(struct cpuinfo_x86 *c); #define cpu_to_core(_cpu) (cpu_data[_cpu].cpu_core_id) #define cpu_to_socket(_cpu) (cpu_data[_cpu].phys_proc_id) +#define cpu_to_cu(_cpu) (cpu_data[_cpu].compute_unit_id) unsigned int apicid_to_socket(unsigned int);
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@intel.com> --- 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. --- xen/arch/x86/microcode.c | 75 +++++++++++++++++++++++++++++++++++++++++ xen/include/asm-x86/processor.h | 1 + 2 files changed, 76 insertions(+)