Re: [PATCH v7 1/3] ACPI: Refactor get_acpi_id_for_cpu() to acpi_get_cpu_uid() on non-x86

[Jeremy Linton <jeremy.linton@arm.com>]

Hi,

On 3/12/26 9:21 PM, Chengwen Feng wrote:
> Unify CPU ACPI ID retrieval interface across architectures by
> refactoring get_acpi_id_for_cpu() to acpi_get_cpu_uid() on
> arm64/riscv/loongarch:
> - Add input parameter validation
> - Adjust interface to int acpi_get_cpu_uid(unsigned int cpu, u32 *uid)
>    (old: u32 get_acpi_id_for_cpu(unsigned int cpu), no input check)
> 
> This refactoring (not a pure rename) enhances interface robustness while
> preparing for consistent ACPI Processor UID retrieval across all
> ACPI-enabled platforms. Valid inputs retain original behavior.
> 
> Note: Move the ARM64-specific get_cpu_for_acpi_id() implementation to
>        arch/arm64/kernel/acpi_numa.c to fix compilation errors from
>        circular header dependencies introduced by the rename.

I moved get_cpu_for_acpi_id() into arm64/kernel/acpi.c, and granted 
didn't test a wide range of configs, but i'm not seeing the circular 
dependency, what is causing that?

> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> ---
>   arch/arm64/include/asm/acpi.h      | 16 +---------
>   arch/arm64/kernel/acpi.c           | 16 ++++++++++
>   arch/arm64/kernel/acpi_numa.c      | 14 +++++++++
>   arch/loongarch/include/asm/acpi.h  |  5 ---
>   arch/loongarch/kernel/acpi.c       |  9 ++++++
>   arch/riscv/include/asm/acpi.h      |  4 ---
>   arch/riscv/kernel/acpi.c           | 16 ++++++++++
>   arch/riscv/kernel/acpi_numa.c      |  9 ++++--
>   drivers/acpi/pptt.c                | 50 ++++++++++++++++++++++--------
>   drivers/acpi/riscv/rhct.c          |  7 ++++-
>   drivers/perf/arm_cspmu/arm_cspmu.c |  6 ++--
>   include/linux/acpi.h               | 13 ++++++++
>   12 files changed, 122 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index c07a58b96329..106a08556cbf 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -114,22 +114,8 @@ static inline bool acpi_has_cpu_in_madt(void)
>   }
>   
>   struct acpi_madt_generic_interrupt *acpi_cpu_get_madt_gicc(int cpu);
> -static inline u32 get_acpi_id_for_cpu(unsigned int cpu)
> -{
> -	return	acpi_cpu_get_madt_gicc(cpu)->uid;
> -}
> -
> -static inline int get_cpu_for_acpi_id(u32 uid)
> -{
> -	int cpu;
> -
> -	for (cpu = 0; cpu < nr_cpu_ids; cpu++)
> -		if (acpi_cpu_get_madt_gicc(cpu) &&
> -		    uid == get_acpi_id_for_cpu(cpu))
> -			return cpu;
>   
> -	return -EINVAL;
> -}
> +int get_cpu_for_acpi_id(u32 uid);
>   
>   static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>   void __init acpi_init_cpus(void);
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index af90128cfed5..f3866606fc46 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -458,3 +458,19 @@ int acpi_unmap_cpu(int cpu)
>   }
>   EXPORT_SYMBOL(acpi_unmap_cpu);
>   #endif /* CONFIG_ACPI_HOTPLUG_CPU */
> +
> +int acpi_get_cpu_uid(unsigned int cpu, u32 *uid)
> +{
> +	struct acpi_madt_generic_interrupt *gicc;
> +
> +	if (cpu >= nr_cpu_ids)
> +		return -EINVAL;
I think this should be cpu_possible() because we have a number of 
for_each_possible() calls that end up in here and AFAIK nr_cpu_ids can 
be more restrictive than the possible list.> +
> +	gicc = acpi_cpu_get_madt_gicc(cpu);
> +	if (!gicc)
> +		return -ENODEV;
So, on arm64, I didn't think it was possible to have a logical cpu 
lookup that didn't map to a gicc uid, because the logical core couldn't 
exist otherwise and (AFAIK) we don't have holes in the possible cpu 
mask. Once you know the logical core is less than nr_cpu_ids it must 
have a MADT mapping. So check this is redundant too, no?

But, if its possible to call this with an invalid logical cpu then we 
probably want to know that, so nr_cpu_ids/cpu_possible() check should 
have pr_warn_once() because there is a bug somewhere, particularly from 
all the pptt calls below where I guess there is an implication there is 
an ID mismatch between the MADT and the PPTT or simply that the user has 
clamped max cpus less than the for_each_possible() calls in the pptt/etc 
code.


> +
> +	*uid = gicc->uid;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_get_cpu_uid);
> diff --git a/arch/arm64/kernel/acpi_numa.c b/arch/arm64/kernel/acpi_numa.c
> index 2465f291c7e1..41d1e46a4338 100644
> --- a/arch/arm64/kernel/acpi_numa.c
> +++ b/arch/arm64/kernel/acpi_numa.c
> @@ -34,6 +34,20 @@ int __init acpi_numa_get_nid(unsigned int cpu)
>   	return acpi_early_node_map[cpu];
>   }
>   
> +int get_cpu_for_acpi_id(u32 uid)
> +{
> +	u32 cpu_uid;
> +	int ret;
> +
> +	for (int cpu = 0; cpu < nr_cpu_ids; cpu++) {
> +		ret = acpi_get_cpu_uid(cpu, &cpu_uid);
This change is redundant, no? Your walking a limited list of possible 
CPUs that have GICC entries and checking to see if one of the known 
logical cores has a matching ACPI id. AKA you can't have an invalid 
logical core here, so the previous call must have a valid acpi id. But 
at the same time so was the acpi_cpu_get_madt_gicc() check in there 
before, which was probably left over from when the tables were being 
mapped rather than cached.

> +		if (ret == 0 && uid == cpu_uid)
> +			return cpu;
> +	}
> +
> +	return -EINVAL;
> +}
> +
I didn't actually manage to hit the case here that keeps this from 
compiling cleanly in acpi.c above if its placed immediately following 
acpi_get_cpu_uid(). IMHO, these two functions are kept close to each 
other since they are so conceptually related.

>   static int __init acpi_parse_gicc_pxm(union acpi_subtable_headers *header,
>   				      const unsigned long end)
>   {
> diff --git a/arch/loongarch/include/asm/acpi.h b/arch/loongarch/include/asm/acpi.h
> index 7376840fa9f7..eda9d4d0a493 100644
> --- a/arch/loongarch/include/asm/acpi.h
> +++ b/arch/loongarch/include/asm/acpi.h
> @@ -40,11 +40,6 @@ extern struct acpi_madt_core_pic acpi_core_pic[MAX_CORE_PIC];
>   
>   extern int __init parse_acpi_topology(void);
>   
> -static inline u32 get_acpi_id_for_cpu(unsigned int cpu)
> -{
> -	return acpi_core_pic[cpu_logical_map(cpu)].processor_id;
> -}
> -
>   #endif /* !CONFIG_ACPI */
>   
>   #define ACPI_TABLE_UPGRADE_MAX_PHYS ARCH_LOW_ADDRESS_LIMIT
> diff --git a/arch/loongarch/kernel/acpi.c b/arch/loongarch/kernel/acpi.c
> index 1367ca759468..058f0dbe8e8f 100644
> --- a/arch/loongarch/kernel/acpi.c
> +++ b/arch/loongarch/kernel/acpi.c
> @@ -385,3 +385,12 @@ int acpi_unmap_cpu(int cpu)
>   EXPORT_SYMBOL(acpi_unmap_cpu);
>   
>   #endif /* CONFIG_ACPI_HOTPLUG_CPU */
> +
> +int acpi_get_cpu_uid(unsigned int cpu, u32 *uid)
> +{
> +	if (cpu >= nr_cpu_ids)
> +		return -EINVAL;
> +	*uid = acpi_core_pic[cpu_logical_map(cpu)].processor_id;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_get_cpu_uid);
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index 6e13695120bc..26ab37c171bc 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -61,10 +61,6 @@ static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>   
>   void acpi_init_rintc_map(void);
>   struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
> -static inline u32 get_acpi_id_for_cpu(int cpu)
> -{
> -	return acpi_cpu_get_madt_rintc(cpu)->uid;
> -}
>   
>   int acpi_get_riscv_isa(struct acpi_table_header *table,
>   		       unsigned int cpu, const char **isa);
> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> index 71698ee11621..322ea92aa39f 100644
> --- a/arch/riscv/kernel/acpi.c
> +++ b/arch/riscv/kernel/acpi.c
> @@ -337,3 +337,19 @@ int raw_pci_write(unsigned int domain, unsigned int bus,
>   }
>   
>   #endif	/* CONFIG_PCI */
> +
> +int acpi_get_cpu_uid(unsigned int cpu, u32 *uid)
> +{
> +	struct acpi_madt_rintc *rintc;
> +
> +	if (cpu >= nr_cpu_ids)
> +		return -EINVAL;
> +
> +	rintc = acpi_cpu_get_madt_rintc(cpu);
> +	if (!rintc)
> +		return -ENODEV;
> +
> +	*uid = rintc->uid;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_get_cpu_uid);
> diff --git a/arch/riscv/kernel/acpi_numa.c b/arch/riscv/kernel/acpi_numa.c
> index 130769e3a99c..6a2d4289f806 100644
> --- a/arch/riscv/kernel/acpi_numa.c
> +++ b/arch/riscv/kernel/acpi_numa.c
> @@ -37,11 +37,14 @@ static int __init acpi_numa_get_nid(unsigned int cpu)
>   
>   static inline int get_cpu_for_acpi_id(u32 uid)
>   {
> -	int cpu;
> +	u32 cpu_uid;
> +	int ret;
>   
> -	for (cpu = 0; cpu < nr_cpu_ids; cpu++)
> -		if (uid == get_acpi_id_for_cpu(cpu))
> +	for (int cpu = 0; cpu < nr_cpu_ids; cpu++) {
> +		ret = acpi_get_cpu_uid(cpu, &cpu_uid);
> +		if (ret == 0 && uid == cpu_uid)
>   			return cpu;
> +	}
>   
>   	return -EINVAL;
>   }
> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> index de5f8c018333..7bd5bc1f225a 100644
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -459,11 +459,14 @@ static void cache_setup_acpi_cpu(struct acpi_table_header *table,
>   {
>   	struct acpi_pptt_cache *found_cache;
>   	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> -	u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
> +	u32 acpi_cpu_id;
>   	struct cacheinfo *this_leaf;
>   	unsigned int index = 0;
>   	struct acpi_pptt_processor *cpu_node = NULL;
>   
> +	if (acpi_get_cpu_uid(cpu, &acpi_cpu_id) != 0)
> +		return;
> +
>   	while (index < get_cpu_cacheinfo(cpu)->num_leaves) {
>   		this_leaf = this_cpu_ci->info_list + index;
>   		found_cache = acpi_find_cache_node(table, acpi_cpu_id,
> @@ -546,7 +549,10 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,
>   				     unsigned int cpu, int level, int flag)
>   {
>   	struct acpi_pptt_processor *cpu_node;
> -	u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
> +	u32 acpi_cpu_id;
> +
> +	if (acpi_get_cpu_uid(cpu, &acpi_cpu_id) != 0)
> +		return -ENOENT;
>   
>   	cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>   	if (cpu_node) {
> @@ -614,18 +620,22 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
>    *
>    * Check the node representing a CPU for a given flag.
>    *
> - * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found or
> - *	   the table revision isn't new enough.
> + * Return: -ENOENT if can't get CPU's ACPI Processor UID, the PPTT doesn't
> + *	   exist, the CPU cannot be found or the table revision isn't new
> + *	   enough.
>    *	   1, any passed flag set
>    *	   0, flag unset
>    */
>   static int check_acpi_cpu_flag(unsigned int cpu, int rev, u32 flag)
>   {
>   	struct acpi_table_header *table;
> -	u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
> +	u32 acpi_cpu_id;
>   	struct acpi_pptt_processor *cpu_node = NULL;
>   	int ret = -ENOENT;
>   
> +	if (acpi_get_cpu_uid(cpu, &acpi_cpu_id) != 0)
> +		return -ENOENT;
> +
>   	table = acpi_get_pptt();
>   	if (!table)
>   		return -ENOENT;
> @@ -651,7 +661,8 @@ static int check_acpi_cpu_flag(unsigned int cpu, int rev, u32 flag)
>    * in the PPTT. Errors caused by lack of a PPTT table, or otherwise, return 0
>    * indicating we didn't find any cache levels.
>    *
> - * Return: -ENOENT if no PPTT table or no PPTT processor struct found.
> + * Return: -ENOENT if no PPTT table, can't get CPU's ACPI Process UID or no PPTT
> + *	   processor struct found.
>    *	   0 on success.
>    */
>   int acpi_get_cache_info(unsigned int cpu, unsigned int *levels,
> @@ -671,7 +682,9 @@ int acpi_get_cache_info(unsigned int cpu, unsigned int *levels,
>   
>   	pr_debug("Cache Setup: find cache levels for CPU=%d\n", cpu);
>   
> -	acpi_cpu_id = get_acpi_id_for_cpu(cpu);
> +	if (acpi_get_cpu_uid(cpu, &acpi_cpu_id))
> +		return -ENOENT;
> +
>   	cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>   	if (!cpu_node)
>   		return -ENOENT;
> @@ -780,8 +793,9 @@ int find_acpi_cpu_topology_package(unsigned int cpu)
>    * It may not exist in single CPU systems. In simple multi-CPU systems,
>    * it may be equal to the package topology level.
>    *
> - * Return: -ENOENT if the PPTT doesn't exist, the CPU cannot be found
> - * or there is no toplogy level above the CPU..
> + * Return: -ENOENT if the PPTT doesn't exist, can't get CPU's ACPI
> + * Processor UID, the CPU cannot be found or there is no toplogy level
> + * above the CPU.
>    * Otherwise returns a value which represents the package for this CPU.
>    */
>   
> @@ -797,7 +811,9 @@ int find_acpi_cpu_topology_cluster(unsigned int cpu)
>   	if (!table)
>   		return -ENOENT;
>   
> -	acpi_cpu_id = get_acpi_id_for_cpu(cpu);
> +	if (acpi_get_cpu_uid(cpu, &acpi_cpu_id) != 0)
> +		return -ENOENT;
> +
>   	cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>   	if (!cpu_node || !cpu_node->parent)
>   		return -ENOENT;
> @@ -872,7 +888,9 @@ static void acpi_pptt_get_child_cpus(struct acpi_table_header *table_hdr,
>   	cpumask_clear(cpus);
>   
>   	for_each_possible_cpu(cpu) {
> -		acpi_id = get_acpi_id_for_cpu(cpu);
> +		if (acpi_get_cpu_uid(cpu, &acpi_id) != 0)
> +			continue;
> +
>   		cpu_node = acpi_find_processor_node(table_hdr, acpi_id);
>   
>   		while (cpu_node) {
> @@ -966,10 +984,13 @@ int find_acpi_cache_level_from_id(u32 cache_id)
>   	for_each_possible_cpu(cpu) {
>   		bool empty;
>   		int level = 1;
> -		u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
> +		u32 acpi_cpu_id;
>   		struct acpi_pptt_cache *cache;
>   		struct acpi_pptt_processor *cpu_node;
>   
> +		if (acpi_get_cpu_uid(cpu, &acpi_cpu_id) != 0)
> +			continue;
> +
>   		cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>   		if (!cpu_node)
>   			continue;
> @@ -1030,10 +1051,13 @@ int acpi_pptt_get_cpumask_from_cache_id(u32 cache_id, cpumask_t *cpus)
>   	for_each_possible_cpu(cpu) {
>   		bool empty;
>   		int level = 1;
> -		u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
> +		u32 acpi_cpu_id;
>   		struct acpi_pptt_cache *cache;
>   		struct acpi_pptt_processor *cpu_node;
>   
> +		if (acpi_get_cpu_uid(cpu, &acpi_cpu_id) != 0)
> +			continue;
> +
>   		cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>   		if (!cpu_node)
>   			continue;
> diff --git a/drivers/acpi/riscv/rhct.c b/drivers/acpi/riscv/rhct.c
> index caa2c16e1697..8f3f38c64a88 100644
> --- a/drivers/acpi/riscv/rhct.c
> +++ b/drivers/acpi/riscv/rhct.c
> @@ -44,10 +44,15 @@ int acpi_get_riscv_isa(struct acpi_table_header *table, unsigned int cpu, const
>   	struct acpi_rhct_isa_string *isa_node;
>   	struct acpi_table_rhct *rhct;
>   	u32 *hart_info_node_offset;
> -	u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
> +	u32 acpi_cpu_id;
> +	int ret;
>   
>   	BUG_ON(acpi_disabled);
>   
> +	ret = acpi_get_cpu_uid(cpu, &acpi_cpu_id);
> +	if (ret != 0)
> +		return ret;
> +
>   	if (!table) {
>   		rhct = acpi_get_rhct();
>   		if (!rhct)
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 34430b68f602..ed72c3d1f796 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -1107,15 +1107,17 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
>   {
>   	struct acpi_apmt_node *apmt_node;
>   	int affinity_flag;
> +	u32 cpu_uid;
>   	int cpu;
> +	int ret;
>   
>   	apmt_node = arm_cspmu_apmt_node(cspmu->dev);
>   	affinity_flag = apmt_node->flags & ACPI_APMT_FLAGS_AFFINITY;
>   
>   	if (affinity_flag == ACPI_APMT_FLAGS_AFFINITY_PROC) {
>   		for_each_possible_cpu(cpu) {
> -			if (apmt_node->proc_affinity ==
> -			    get_acpi_id_for_cpu(cpu)) {
> +			ret = acpi_get_cpu_uid(cpu, &cpu_uid);
> +			if (ret == 0 && apmt_node->proc_affinity == cpu_uid) {
>   				cpumask_set_cpu(cpu, &cspmu->associated_cpus);
>   				break;
>   			}
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 4d2f0bed7a06..035094a55f18 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -324,6 +324,19 @@ int acpi_unmap_cpu(int cpu);
>   
>   acpi_handle acpi_get_processor_handle(int cpu);
>   
> +#ifndef CONFIG_X86
> +/*
> + * acpi_get_cpu_uid() - Get ACPI Processor UID of a specified CPU from MADT table
> + * @cpu: Logical CPU number (0-based)
> + * @uid: Pointer to store the ACPI Processor UID (valid only on successful return)
> + *
> + * Return: 0 on successful retrieval (the ACPI Processor ID is stored in *uid);
> + *         -EINVAL if the CPU number is invalid or out of range;
> + *         -ENODEV if the ACPI Processor UID for the specified CPU is not found.
> + */
> +int acpi_get_cpu_uid(unsigned int cpu, u32 *uid);
> +#endif
> +
>   #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
>   int acpi_get_ioapic_id(acpi_handle handle, u32 gsi_base, u64 *phys_addr);
>   #endif