2023-08-07 15:27:45

by Thomas Gleixner

[permalink] [raw]
Subject: [patch 24/53] x86/cpu/topology: Provide separate APIC registration functions

generic_processor_info() aside of being a complete misnomer is used for
both early boot registration and ACPI CPU hotplug.

While it's arguable that this can share some code, it results in code which
is hard to understand and kept around post init for no real reason.

Also the call sites do lots of manual fiddling in topology related
variables instead of having proper interfaces for the purpose which handle
the topology internals correctly.

Provide topology_register_apic(), topology_hotplug_apic() and
topology_hot_unplug_apic() which have the extra magic of the call sites
incorporated and for now are wrappers around generic_processor_info().

Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/include/asm/apic.h | 3
arch/x86/kernel/cpu/topology.c | 113 ++++++++++++++++++++++++++++++------
2 files changed, 98 insertions(+), 18 deletions(-)

--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -171,7 +171,10 @@ extern bool apic_needs_pit(void);

extern void apic_send_IPI_allbutself(unsigned int vector);

+extern void topology_register_apic(u32 apic_id, u32 acpi_id, bool present);
extern void topology_register_boot_apic(u32 apic_id);
+extern int topology_hotplug_apic(u32 apic_id, u32 acpi_id);
+extern void topology_hotunplug_apic(unsigned int cpu);

#else /* !CONFIG_X86_LOCAL_APIC */
static inline void lapic_shutdown(void) { }
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -79,32 +79,38 @@ early_initcall(smp_init_primary_thread_m
static inline void cpu_mark_primary_thread(unsigned int cpu, unsigned int apicid) { }
#endif

-/*
- * Should use this API to allocate logical CPU IDs to keep nr_logical_cpuids
- * and cpuid_to_apicid[] synchronized.
- */
-static int allocate_logical_cpuid(int apicid)
+static int topo_lookup_cpuid(u32 apic_id)
{
int i;

- /*
- * cpuid <-> apicid mapping is persistent, so when a cpu is up,
- * check if the kernel has allocated a cpuid for it.
- */
+ /* CPU# to APICID mapping is persistent once it is established */
for (i = 0; i < nr_logical_cpuids; i++) {
- if (cpuid_to_apicid[i] == apicid)
+ if (cpuid_to_apicid[i] == apic_id)
return i;
}
+ return -ENODEV;
+}
+
+/*
+ * Should use this API to allocate logical CPU IDs to keep nr_logical_cpuids
+ * and cpuid_to_apicid[] synchronized.
+ */
+static int allocate_logical_cpuid(u32 apic_id)
+{
+ int cpu = topo_lookup_cpuid(apic_id);
+
+ if (cpu >= 0)
+ return cpu;

/* Allocate a new cpuid. */
if (nr_logical_cpuids >= nr_cpu_ids) {
WARN_ONCE(1, "APIC: NR_CPUS/possible_cpus limit of %u reached. "
"Processor %d/0x%x and the rest are ignored.\n",
- nr_cpu_ids, nr_logical_cpuids, apicid);
+ nr_cpu_ids, nr_logical_cpuids, apic_id);
return -EINVAL;
}

- cpuid_to_apicid[nr_logical_cpuids] = apicid;
+ cpuid_to_apicid[nr_logical_cpuids] = apic_id;
return nr_logical_cpuids++;
}

@@ -122,12 +128,6 @@ static void cpu_update_apic(int cpu, u32
cpu_mark_primary_thread(cpu, apicid);
}

-void __init topology_register_boot_apic(u32 apic_id)
-{
- cpuid_to_apicid[0] = apic_id;
- cpu_update_apic(0, apic_id);
-}
-
int generic_processor_info(int apicid)
{
int cpu, max = nr_cpu_ids;
@@ -169,6 +169,83 @@ int generic_processor_info(int apicid)
return cpu;
}

+/**
+ * topology_register_apic - Register an APIC in early topology maps
+ * @apic_id: The APIC ID to set up
+ * @acpi_id: The ACPI ID associated to the APIC
+ * @present: True if the corresponding CPU is present
+ */
+void __init topology_register_apic(u32 apic_id, u32 acpi_id, bool present)
+{
+ int cpu;
+
+ if (apic_id >= MAX_LOCAL_APIC) {
+ pr_err_once("APIC ID %x exceeds kernel limit of: %x\n", apic_id, MAX_LOCAL_APIC - 1);
+ return;
+ }
+
+ if (!present) {
+ disabled_cpus++;
+ return;
+ }
+
+ cpu = generic_processor_info(apic_id);
+ if (cpu >= 0)
+ early_per_cpu(x86_cpu_to_acpiid, cpu) = acpi_id;
+}
+
+/**
+ * topology_register_boot_apic - Register the boot CPU APIC
+ * @apic_id: The APIC ID to set up
+ *
+ * Separate so CPU #0 can be assigned
+ */
+void __init topology_register_boot_apic(u32 apic_id)
+{
+ cpuid_to_apicid[0] = apic_id;
+ cpu_update_apic(0, apic_id);
+}
+
+#ifdef CONFIG_ACPI_HOTPLUG_CPU
+/**
+ * topology_hotplug_apic - Handle a physical hotplugged APIC after boot
+ * @apic_id: The APIC ID to set up
+ * @acpi_id: The ACPI ID associated to the APIC
+ */
+int topology_hotplug_apic(u32 apic_id, u32 acpi_id)
+{
+ int cpu;
+
+ if (apic_id >= MAX_LOCAL_APIC)
+ return -EINVAL;
+
+ cpu = topo_lookup_cpuid(apic_id);
+ if (cpu < 0) {
+ cpu = generic_processor_info(apic_id);
+ if (cpu >= 0)
+ per_cpu(x86_cpu_to_acpiid, cpu) = acpi_id;
+ }
+ return cpu;
+}
+
+/**
+ * topology_hotunplug_apic - Remove a physical hotplugged APIC after boot
+ * @cpu: The CPU number for which the APIC ID is removed
+ */
+void topology_hotunplug_apic(unsigned int cpu)
+{
+ u32 apic_id = cpuid_to_apicid[cpu];
+
+ if (apic_id == BAD_APICID)
+ return;
+
+ per_cpu(x86_cpu_to_apicid, cpu) = BAD_APICID;
+ clear_bit(apic_id, phys_cpu_present_map);
+ set_cpu_present(cpu, false);
+ num_processors--;
+}
+#endif
+
static int __init apic_set_disabled_cpu_apicid(char *arg)
{
if (!arg || !get_option(&arg, &disabled_cpu_apicid))



2023-08-11 13:56:27

by Zhang, Rui

[permalink] [raw]
Subject: Re: [patch 24/53] x86/cpu/topology: Provide separate APIC registration functions

On Mon, 2023-08-07 at 15:53 +0200, Thomas Gleixner wrote:
> generic_processor_info() aside of being a complete misnomer is used
> for
> both early boot registration and ACPI CPU hotplug.
>
> While it's arguable that this can share some code, it results in code
> which
> is hard to understand and kept around post init for no real reason.
>
> Also the call sites do lots of manual fiddling in topology related
> variables instead of having proper interfaces for the purpose which
> handle
> the topology internals correctly.
>
> Provide topology_register_apic(), topology_hotplug_apic() and
> topology_hot_unplug_apic()

s/topology_hot_unplug_apic/topology_hotunplug_apic

thanks,
rui

> which have the extra magic of the call sites
> incorporated and for now are wrappers around
> generic_processor_info().
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
>  arch/x86/include/asm/apic.h         |    3
>  arch/x86/kernel/cpu/topology.c |  113
> ++++++++++++++++++++++++++++++------
>  2 files changed, 98 insertions(+), 18 deletions(-)
>
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -171,7 +171,10 @@ extern bool apic_needs_pit(void);
>  
>  extern void apic_send_IPI_allbutself(unsigned int vector);
>  
> +extern void topology_register_apic(u32 apic_id, u32 acpi_id, bool
> present);
>  extern void topology_register_boot_apic(u32 apic_id);
> +extern int topology_hotplug_apic(u32 apic_id, u32 acpi_id);
> +extern void topology_hotunplug_apic(unsigned int cpu);
>  
>  #else /* !CONFIG_X86_LOCAL_APIC */
>  static inline void lapic_shutdown(void) { }
> --- a/arch/x86/kernel/cpu/topology.c
> +++ b/arch/x86/kernel/cpu/topology.c
> @@ -79,32 +79,38 @@ early_initcall(smp_init_primary_thread_m
>  static inline void cpu_mark_primary_thread(unsigned int cpu,
> unsigned int apicid) { }
>  #endif
>  
> -/*
> - * Should use this API to allocate logical CPU IDs to keep
> nr_logical_cpuids
> - * and cpuid_to_apicid[] synchronized.
> - */
> -static int allocate_logical_cpuid(int apicid)
> +static int topo_lookup_cpuid(u32 apic_id)
>  {
>         int i;
>  
> -       /*
> -        * cpuid <-> apicid mapping is persistent, so when a cpu is
> up,
> -        * check if the kernel has allocated a cpuid for it.
> -        */
> +       /* CPU# to APICID mapping is persistent once it is
> established */
>         for (i = 0; i < nr_logical_cpuids; i++) {
> -               if (cpuid_to_apicid[i] == apicid)
> +               if (cpuid_to_apicid[i] == apic_id)
>                         return i;
>         }
> +       return -ENODEV;
> +}
> +
> +/*
> + * Should use this API to allocate logical CPU IDs to keep
> nr_logical_cpuids
> + * and cpuid_to_apicid[] synchronized.
> + */
> +static int allocate_logical_cpuid(u32 apic_id)
> +{
> +       int cpu = topo_lookup_cpuid(apic_id);
> +
> +       if (cpu >= 0)
> +               return cpu;
>  
>         /* Allocate a new cpuid. */
>         if (nr_logical_cpuids >= nr_cpu_ids) {
>                 WARN_ONCE(1, "APIC: NR_CPUS/possible_cpus limit of %u
> reached. "
>                              "Processor %d/0x%x and the rest are
> ignored.\n",
> -                            nr_cpu_ids, nr_logical_cpuids, apicid);
> +                            nr_cpu_ids, nr_logical_cpuids, apic_id);
>                 return -EINVAL;
>         }
>  
> -       cpuid_to_apicid[nr_logical_cpuids] = apicid;
> +       cpuid_to_apicid[nr_logical_cpuids] = apic_id;
>         return nr_logical_cpuids++;
>  }
>  
> @@ -122,12 +128,6 @@ static void cpu_update_apic(int cpu, u32
>                 cpu_mark_primary_thread(cpu, apicid);
>  }
>  
> -void __init topology_register_boot_apic(u32 apic_id)
> -{
> -       cpuid_to_apicid[0] = apic_id;
> -       cpu_update_apic(0, apic_id);
> -}
> -
>  int generic_processor_info(int apicid)
>  {
>         int cpu, max = nr_cpu_ids;
> @@ -169,6 +169,83 @@ int generic_processor_info(int apicid)
>         return cpu;
>  }
>  
> +/**
> + * topology_register_apic - Register an APIC in early topology maps
> + * @apic_id:   The APIC ID to set up
> + * @acpi_id:   The ACPI ID associated to the APIC
> + * @present:   True if the corresponding CPU is present
> + */
> +void __init topology_register_apic(u32 apic_id, u32 acpi_id, bool
> present)
> +{
> +       int cpu;
> +
> +       if (apic_id >= MAX_LOCAL_APIC) {
> +               pr_err_once("APIC ID %x exceeds kernel limit of:
> %x\n", apic_id, MAX_LOCAL_APIC - 1);
> +               return;
> +       }
> +
> +       if (!present) {
> +               disabled_cpus++;
> +               return;
> +       }
> +
> +       cpu = generic_processor_info(apic_id);
> +       if (cpu >= 0)
> +               early_per_cpu(x86_cpu_to_acpiid, cpu) = acpi_id;
> +}
> +
> +/**
> + * topology_register_boot_apic - Register the boot CPU APIC
> + * @apic_id:   The APIC ID to set up
> + *
> + * Separate so CPU #0 can be assigned
> + */
> +void __init topology_register_boot_apic(u32 apic_id)
> +{
> +       cpuid_to_apicid[0] = apic_id;
> +       cpu_update_apic(0, apic_id);
> +}
> +
> +#ifdef CONFIG_ACPI_HOTPLUG_CPU
> +/**
> + * topology_hotplug_apic - Handle a physical hotplugged APIC after
> boot
> + * @apic_id:   The APIC ID to set up
> + * @acpi_id:   The ACPI ID associated to the APIC
> + */
> +int topology_hotplug_apic(u32 apic_id, u32 acpi_id)
> +{
> +       int cpu;
> +
> +       if (apic_id >= MAX_LOCAL_APIC)
> +               return -EINVAL;
> +
> +       cpu = topo_lookup_cpuid(apic_id);
> +       if (cpu < 0) {
> +               cpu = generic_processor_info(apic_id);
> +               if (cpu >= 0)
> +                       per_cpu(x86_cpu_to_acpiid, cpu) = acpi_id;
> +       }
> +       return cpu;
> +}
> +
> +/**
> + * topology_hotunplug_apic - Remove a physical hotplugged APIC after
> boot
> + * @cpu:       The CPU number for which the APIC ID is removed
> + */
> +void topology_hotunplug_apic(unsigned int cpu)
> +{
> +       u32 apic_id = cpuid_to_apicid[cpu];
> +
> +       if (apic_id == BAD_APICID)
> +               return;
> +
> +       per_cpu(x86_cpu_to_apicid, cpu) = BAD_APICID;
> +       clear_bit(apic_id, phys_cpu_present_map);
> +       set_cpu_present(cpu, false);
> +       num_processors--;
> +}
> +#endif
> +
>  static int __init apic_set_disabled_cpu_apicid(char *arg)
>  {
>         if (!arg || !get_option(&arg, &disabled_cpu_apicid))
>