Mailing List Archive

[XEN PATCH 2/9] x86: don't access x86_cpu_to_apicid[] directly, use cpu_physical_id(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(-)

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) )
--
2.41.0
Re: [XEN PATCH 2/9] x86: don't access x86_cpu_to_apicid[] directly, use cpu_physical_id(cpu) [ In reply to ]
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,

--
Julien Grall
Re: [XEN PATCH 2/9] x86: don't access x86_cpu_to_apicid[] directly, use cpu_physical_id(cpu) [ In reply to ]
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
Re: [XEN PATCH 2/9] x86: don't access x86_cpu_to_apicid[] directly, use cpu_physical_id(cpu) [ In reply to ]
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,

--
Krystian Hebel
Firmware Engineer
https://3mdeb.com | @3mdeb_com
Re: [XEN PATCH 2/9] x86: don't access x86_cpu_to_apicid[] directly, use cpu_physical_id(cpu) [ In reply to ]
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