| Message ID | 705574ddb7f18bae9ed3f60ddf2e4bda02c70388.1699982111.git.krystian.hebel@3mdeb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: parallelize AP bring-up during boot | expand |
Hi, On 14/11/2023 17:49, Krystian Hebel wrote: > This is done in preparation to move data from x86_cpu_to_apicid[] > elsewhere. NIT: I would add "No functional changes intended". > Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com> Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers,
On 14.11.2023 18:49, Krystian Hebel wrote: > --- a/xen/arch/x86/apic.c > +++ b/xen/arch/x86/apic.c > @@ -950,7 +950,7 @@ __next: > */ > if (boot_cpu_physical_apicid == -1U) > boot_cpu_physical_apicid = get_apic_id(); > - x86_cpu_to_apicid[0] = get_apic_id(); > + cpu_physical_id(0) = get_apic_id(); While personally I don't mind as much, I expect Andrew would not like this: Something that looks like a function call on the lhs is against what normal language structure would be. > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -1615,7 +1615,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) > break; > > cpu_id.phys_id = > - (uint64_t)x86_cpu_to_apicid[v->vcpu_id] | > + (uint64_t)cpu_physical_id(v->vcpu_id) | > ((uint64_t)acpi_get_processor_id(v->vcpu_id) << 32); While the cast on the 2nd line is necessary, the one on the 2st isn't and would be nice to be dropped while touching the line anyway. > --- a/xen/arch/x86/numa.c > +++ b/xen/arch/x86/numa.c > @@ -70,7 +70,7 @@ void __init init_cpu_to_node(void) > > for ( i = 0; i < nr_cpu_ids; i++ ) > { > - u32 apicid = x86_cpu_to_apicid[i]; > + u32 apicid = cpu_physical_id(i); > if ( apicid == BAD_APICID ) > continue; > node = apicid < MAX_LOCAL_APIC ? apicid_to_node[apicid] : NUMA_NO_NODE; We're in the process of phasing out u32 and friends, favoring uint32_t. Would be nice if in code being touched anyway (i.e. not just here) the conversion would be done right away. Then again fixed-width types are preferably avoided where not really needed (see ./CODING_STYLE), so quite likely it actually wants to be unsigned int here. Furthermore our style demands that declaration(s) and statement(s) are separated by a blank line. Inserting the missing one in cases like the one here would be very desirable as well. Jan
On 7.02.2024 17:28, Jan Beulich wrote: > On 14.11.2023 18:49, Krystian Hebel wrote: >> --- a/xen/arch/x86/apic.c >> +++ b/xen/arch/x86/apic.c >> @@ -950,7 +950,7 @@ __next: >> */ >> if (boot_cpu_physical_apicid == -1U) >> boot_cpu_physical_apicid = get_apic_id(); >> - x86_cpu_to_apicid[0] = get_apic_id(); >> + cpu_physical_id(0) = get_apic_id(); > While personally I don't mind as much, I expect Andrew would not like > this: Something that looks like a function call on the lhs is against > what normal language structure would be. This made me cringe as well, but I've seen something like this used in other places (per_cpu() mostly) so I thought it was OK. I can change it. >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -1615,7 +1615,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) >> break; >> >> cpu_id.phys_id = >> - (uint64_t)x86_cpu_to_apicid[v->vcpu_id] | >> + (uint64_t)cpu_physical_id(v->vcpu_id) | >> ((uint64_t)acpi_get_processor_id(v->vcpu_id) << 32); > While the cast on the 2nd line is necessary, the one on the 2st isn't > and would be nice to be dropped while touching the line anyway. Ack > >> --- a/xen/arch/x86/numa.c >> +++ b/xen/arch/x86/numa.c >> @@ -70,7 +70,7 @@ void __init init_cpu_to_node(void) >> >> for ( i = 0; i < nr_cpu_ids; i++ ) >> { >> - u32 apicid = x86_cpu_to_apicid[i]; >> + u32 apicid = cpu_physical_id(i); >> if ( apicid == BAD_APICID ) >> continue; >> node = apicid < MAX_LOCAL_APIC ? apicid_to_node[apicid] : NUMA_NO_NODE; > We're in the process of phasing out u32 and friends, favoring uint32_t. > Would be nice if in code being touched anyway (i.e. not just here) the > conversion would be done right away. Then again fixed-width types are > preferably avoided where not really needed (see ./CODING_STYLE), so > quite likely it actually wants to be unsigned int here. > > Furthermore our style demands that declaration(s) and statement(s) are > separated by a blank line. Inserting the missing one in cases like the > one here would be very desirable as well. Good to know, thanks. > Jan Best regards,
On 12.03.2024 16:18, Krystian Hebel wrote: > On 7.02.2024 17:28, Jan Beulich wrote: >> On 14.11.2023 18:49, Krystian Hebel wrote: >>> --- a/xen/arch/x86/apic.c >>> +++ b/xen/arch/x86/apic.c >>> @@ -950,7 +950,7 @@ __next: >>> */ >>> if (boot_cpu_physical_apicid == -1U) >>> boot_cpu_physical_apicid = get_apic_id(); >>> - x86_cpu_to_apicid[0] = get_apic_id(); >>> + cpu_physical_id(0) = get_apic_id(); >> While personally I don't mind as much, I expect Andrew would not like >> this: Something that looks like a function call on the lhs is against >> what normal language structure would be. > This made me cringe as well, but I've seen something like this used in > other places (per_cpu() mostly) so I thought it was OK. I can change it. Please try to get in touch with Andrew, to see what he thinks (especially with your pointing of per_cpu()'s similarity). Jan
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c index cfce4cc0408f..d03e54bcef38 100644 --- a/xen/arch/x86/acpi/cpu_idle.c +++ b/xen/arch/x86/acpi/cpu_idle.c @@ -1256,7 +1256,7 @@ int get_cpu_id(u32 acpi_id) for ( i = 0; i < nr_cpu_ids; i++ ) { - if ( apic_id == x86_cpu_to_apicid[i] ) + if ( apic_id == cpu_physical_id(i) ) return i; } @@ -1362,7 +1362,7 @@ long set_cx_pminfo(uint32_t acpi_id, struct xen_processor_power *power) if ( !cpu_online(cpu_id) ) { - uint32_t apic_id = x86_cpu_to_apicid[cpu_id]; + uint32_t apic_id = cpu_physical_id(cpu_id); /* * If we've just learned of more available C states, wake the CPU if diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c index 51cb082ca02a..87a1f38f3f5a 100644 --- a/xen/arch/x86/acpi/lib.c +++ b/xen/arch/x86/acpi/lib.c @@ -91,7 +91,7 @@ unsigned int acpi_get_processor_id(unsigned int cpu) { unsigned int acpiid, apicid; - if ((apicid = x86_cpu_to_apicid[cpu]) == BAD_APICID) + if ((apicid = cpu_physical_id(cpu)) == BAD_APICID) return INVALID_ACPIID; for (acpiid = 0; acpiid < ARRAY_SIZE(x86_acpiid_to_apicid); acpiid++) diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c index 6acdd0ec1468..63e18968e54c 100644 --- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c @@ -950,7 +950,7 @@ __next: */ if (boot_cpu_physical_apicid == -1U) boot_cpu_physical_apicid = get_apic_id(); - x86_cpu_to_apicid[0] = get_apic_id(); + cpu_physical_id(0) = get_apic_id(); ioapic_init(); } diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c index ff5c808bc952..88168da8a0ca 100644 --- a/xen/arch/x86/cpu/mwait-idle.c +++ b/xen/arch/x86/cpu/mwait-idle.c @@ -1202,8 +1202,8 @@ static void __init ivt_idle_state_table_update(void) unsigned int cpu, max_apicid = boot_cpu_physical_apicid; for_each_present_cpu(cpu) - if (max_apicid < x86_cpu_to_apicid[cpu]) - max_apicid = x86_cpu_to_apicid[cpu]; + if (max_apicid < cpu_physical_id(cpu)) + max_apicid = cpu_physical_id(cpu); switch (apicid_to_socket(max_apicid)) { case 0: case 1: /* 1 and 2 socket systems use default ivt_cstates */ diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 3712e36df930..f45437511a46 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1615,7 +1615,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg) break; cpu_id.phys_id = - (uint64_t)x86_cpu_to_apicid[v->vcpu_id] | + (uint64_t)cpu_physical_id(v->vcpu_id) | ((uint64_t)acpi_get_processor_id(v->vcpu_id) << 32); rc = -EFAULT; diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c index d8ccab2449c6..b8cabebe7bf9 100644 --- a/xen/arch/x86/mpparse.c +++ b/xen/arch/x86/mpparse.c @@ -187,7 +187,7 @@ static int MP_processor_info_x(struct mpc_config_processor *m, " Processor with apicid %i ignored\n", apicid); return cpu; } - x86_cpu_to_apicid[cpu] = apicid; + cpu_physical_id(cpu) = apicid; cpumask_set_cpu(cpu, &cpu_present_map); } @@ -822,12 +822,12 @@ void mp_unregister_lapic(uint32_t apic_id, uint32_t cpu) if (!cpu || (apic_id == boot_cpu_physical_apicid)) return; - if (x86_cpu_to_apicid[cpu] != apic_id) + if (cpu_physical_id(cpu) != apic_id) return; physid_clear(apic_id, phys_cpu_present_map); - x86_cpu_to_apicid[cpu] = BAD_APICID; + cpu_physical_id(cpu) = BAD_APICID; cpumask_clear_cpu(cpu, &cpu_present_map); } diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c index 4b0b297c7e09..39e131cb4f35 100644 --- a/xen/arch/x86/numa.c +++ b/xen/arch/x86/numa.c @@ -70,7 +70,7 @@ void __init init_cpu_to_node(void) for ( i = 0; i < nr_cpu_ids; i++ ) { - u32 apicid = x86_cpu_to_apicid[i]; + u32 apicid = cpu_physical_id(i); if ( apicid == BAD_APICID ) continue; node = apicid < MAX_LOCAL_APIC ? apicid_to_node[apicid] : NUMA_NO_NODE; diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c index 9469de9045c7..9a52e048f67c 100644 --- a/xen/arch/x86/platform_hypercall.c +++ b/xen/arch/x86/platform_hypercall.c @@ -588,7 +588,7 @@ ret_t do_platform_op( } else { - g_info->apic_id = x86_cpu_to_apicid[g_info->xen_cpuid]; + g_info->apic_id = cpu_physical_id(g_info->xen_cpuid); g_info->acpi_id = acpi_get_processor_id(g_info->xen_cpuid); ASSERT(g_info->apic_id != BAD_APICID); g_info->flags = 0; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 1285969901e0..a19fe219bbf6 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -319,7 +319,7 @@ static void __init init_idle_domain(void) void srat_detect_node(int cpu) { nodeid_t node; - u32 apicid = x86_cpu_to_apicid[cpu]; + u32 apicid = cpu_physical_id(cpu); node = apicid < MAX_LOCAL_APIC ? apicid_to_node[apicid] : NUMA_NO_NODE; if ( node == NUMA_NO_NODE ) @@ -346,7 +346,7 @@ static void __init normalise_cpu_order(void) for_each_present_cpu ( i ) { - apicid = x86_cpu_to_apicid[i]; + apicid = cpu_physical_id(i); min_diff = min_cpu = ~0u; /* @@ -357,12 +357,12 @@ static void __init normalise_cpu_order(void) j < nr_cpu_ids; j = cpumask_next(j, &cpu_present_map) ) { - diff = x86_cpu_to_apicid[j] ^ apicid; + diff = cpu_physical_id(j) ^ apicid; while ( diff & (diff-1) ) diff &= diff-1; if ( (diff < min_diff) || ((diff == min_diff) && - (x86_cpu_to_apicid[j] < x86_cpu_to_apicid[min_cpu])) ) + (cpu_physical_id(j) < cpu_physical_id(min_cpu))) ) { min_diff = diff; min_cpu = j; @@ -378,9 +378,9 @@ static void __init normalise_cpu_order(void) /* Switch the best-matching CPU with the next CPU in logical order. */ j = cpumask_next(i, &cpu_present_map); - apicid = x86_cpu_to_apicid[min_cpu]; - x86_cpu_to_apicid[min_cpu] = x86_cpu_to_apicid[j]; - x86_cpu_to_apicid[j] = apicid; + apicid = cpu_physical_id(min_cpu); + cpu_physical_id(min_cpu) = cpu_physical_id(j); + cpu_physical_id(j) = apicid; } } diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c index 4c54ecbc91d7..de87c5a41926 100644 --- a/xen/arch/x86/smpboot.c +++ b/xen/arch/x86/smpboot.c @@ -1154,7 +1154,7 @@ void __init smp_prepare_cpus(void) print_cpu_info(0); boot_cpu_physical_apicid = get_apic_id(); - x86_cpu_to_apicid[0] = boot_cpu_physical_apicid; + cpu_physical_id(0) = boot_cpu_physical_apicid; stack_base[0] = (void *)((unsigned long)stack_start & ~(STACK_SIZE - 1)); @@ -1374,7 +1374,7 @@ int __cpu_up(unsigned int cpu) { int apicid, ret; - if ( (apicid = x86_cpu_to_apicid[cpu]) == BAD_APICID ) + if ( (apicid = cpu_physical_id(cpu)) == BAD_APICID ) return -ENODEV; if ( (!x2apic_enabled && apicid >= APIC_ALL_CPUS) || diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index a8d8af22f6d8..d54c8d93cff0 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -589,7 +589,7 @@ static bool __init check_smt_enabled(void) * has a non-zero thread id component indicates that SMT is active. */ for_each_present_cpu ( cpu ) - if ( x86_cpu_to_apicid[cpu] & (boot_cpu_data.x86_num_siblings - 1) ) + if ( cpu_physical_id(cpu) & (boot_cpu_data.x86_num_siblings - 1) ) return true; return false; diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c index c107f40c6283..67d8ab3f824a 100644 --- a/xen/arch/x86/sysctl.c +++ b/xen/arch/x86/sysctl.c @@ -58,7 +58,7 @@ static long cf_check smt_up_down_helper(void *data) for_each_present_cpu ( cpu ) { /* Skip primary siblings (those whose thread id is 0). */ - if ( !(x86_cpu_to_apicid[cpu] & sibling_mask) ) + if ( !(cpu_physical_id(cpu) & sibling_mask) ) continue; if ( !up && core_parking_remove(cpu) )
This is done in preparation to move data from x86_cpu_to_apicid[] elsewhere. Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com> --- xen/arch/x86/acpi/cpu_idle.c | 4 ++-- xen/arch/x86/acpi/lib.c | 2 +- xen/arch/x86/apic.c | 2 +- xen/arch/x86/cpu/mwait-idle.c | 4 ++-- xen/arch/x86/domain.c | 2 +- xen/arch/x86/mpparse.c | 6 +++--- xen/arch/x86/numa.c | 2 +- xen/arch/x86/platform_hypercall.c | 2 +- xen/arch/x86/setup.c | 14 +++++++------- xen/arch/x86/smpboot.c | 4 ++-- xen/arch/x86/spec_ctrl.c | 2 +- xen/arch/x86/sysctl.c | 2 +- 12 files changed, 23 insertions(+), 23 deletions(-)