diff mbox series

[XEN,2/9] x86: don't access x86_cpu_to_apicid[] directly, use cpu_physical_id(cpu)

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

Commit Message

Krystian Hebel Nov. 14, 2023, 5:49 p.m. UTC
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(-)

Comments

Julien Grall Jan. 26, 2024, 6:38 p.m. UTC | #1
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,
Jan Beulich Feb. 7, 2024, 4:28 p.m. UTC | #2
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
Krystian Hebel March 12, 2024, 3:18 p.m. UTC | #3
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,
Jan Beulich March 12, 2024, 3:42 p.m. UTC | #4
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 mbox series

Patch

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