Parse the ACPI PPTT (Processor Properties Topology Table) to
initialize the CPU topology.
For ACPI 6.3 and later, the ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD flag
is checked to determine the presence of SMT. For ACPI 6.2 and
earlier, CPUs are assumed not to support SMT.
Signed-off-by: Hirokazu Takahashi <taka@valinux.co.jp>
---
Changes in v5:
Extracted CPU topology information from the ACPI PPTT.
xen/arch/arm/acpi/boot.c | 2 +
xen/arch/arm/include/asm/acpi.h | 2 +
xen/drivers/acpi/topology.c | 230 ++++++++++++++++++++++++++++++--
xen/include/acpi/actbl3.h | 30 +++++
xen/include/xen/acpi.h | 8 ++
5 files changed, 264 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 4ac0fd8f51..fc7ecb5749 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -85,6 +85,7 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
return;
}
bootcpu_valid = true;
+ acpi_map_cpu_acpiid(0, processor->uid);
return;
}
@@ -119,6 +120,7 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
/* map the logical cpu id to cpu MPIDR */
cpu_logical_map(enabled_cpus) = mpidr;
+ acpi_map_cpu_acpiid(enabled_cpus, processor->uid);
enabled_cpus++;
}
diff --git a/xen/arch/arm/include/asm/acpi.h b/xen/arch/arm/include/asm/acpi.h
index 13756dd341..b2e156e131 100644
--- a/xen/arch/arm/include/asm/acpi.h
+++ b/xen/arch/arm/include/asm/acpi.h
@@ -61,6 +61,8 @@ paddr_t acpi_get_table_offset(struct membank tbl_add[], EFI_MEM_RES index);
(!(entry) || (unsigned long)(entry) + sizeof(*(entry)) > (end) || \
(entry)->header.length != ACPI_MADT_GICC_LENGTH)
+#define INVALID_ACPIID (-1U)
+
#ifdef CONFIG_ACPI
extern bool acpi_disabled;
/* Basic configuration for ACPI */
diff --git a/xen/drivers/acpi/topology.c b/xen/drivers/acpi/topology.c
index 9155edc0be..37e7c70625 100644
--- a/xen/drivers/acpi/topology.c
+++ b/xen/drivers/acpi/topology.c
@@ -5,18 +5,90 @@
#include <xen/cpumask.h>
#include <xen/init.h>
-/*
- * TODO: Populate the topology information by scanning the ACPI
- * PPTT (Processor Properties Topology Table).
- */
-void __init acpi_init_cpu_topology(void)
+uint32_t map_cpu_acpiid[NR_CPUS] __initdata =
+ { [0 ... NR_CPUS - 1] = INVALID_ACPIID };
+uint32_t socket_map[NR_CPUS] __initdata;
+uint32_t cluster_map[NR_CPUS] __initdata;
+uint32_t core_map[NR_CPUS] __initdata;
+uint32_t thread_map[NR_CPUS] __initdata;
+unsigned int __initdata num_sockets;
+unsigned int __initdata num_clusters;
+unsigned int __initdata num_cores;
+
+static unsigned int __init get_logical_id(uint32_t phys_offset,
+ uint32_t *map,
+ unsigned int *count)
+{
+ unsigned int id;
+
+ for ( id = 0; id < *count; id++ )
+ if ( map[id] == phys_offset )
+ return id;
+
+ map[*count] = phys_offset;
+ id = *count;
+ (*count)++;
+
+ return id;
+}
+
+static struct acpi_pptt_processor *__init find_pptt_node(
+ const struct acpi_table_header *table_hdr, unsigned int acpi_id)
+{
+ const struct acpi_subtable_header *entry;
+ unsigned long table_end;
+ const char *ptr;
+
+ if ( !table_hdr )
+ return NULL;
+
+ table_end = (unsigned long)table_hdr + table_hdr->length;
+
+ ptr = (const char *)table_hdr + sizeof(struct acpi_table_pptt);
+
+ while ( (unsigned long)ptr + sizeof(struct acpi_subtable_header)
+ <= table_end )
+ {
+ entry = (const struct acpi_subtable_header *)ptr;
+
+ if ( entry->length == 0 )
+ {
+ printk(XENLOG_ERR
+ "ACPI: PPTT has an invalid zero-length subtable.\n");
+ break;
+ }
+
+ if ( (unsigned long)ptr + entry->length > table_end )
+ {
+ printk(XENLOG_ERR
+ "ACPI: PPTT subtable extends beyond table end.\n");
+ break;
+ }
+
+ if ( entry->type == ACPI_PPTT_TYPE_PROCESSOR )
+ if ( entry->length >= sizeof(struct acpi_pptt_processor) )
+ {
+ struct acpi_pptt_processor *proc =
+ (struct acpi_pptt_processor *)entry;
+
+ if ( (proc->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) &&
+ proc->acpi_processor_id == acpi_id )
+ return proc;
+ }
+
+ ptr += entry->length;
+ }
+
+ return NULL;
+}
+
+static void __init setup_fake_topology(void)
{
unsigned int cpu;
/*
- * Generate temporary cpu topology information for now.
- * It assumes that the cpu doesn't have SMT and all CPUs
- * belong to the same socket.
+ * Generate temporary cpu topology information. It assumes that
+ * the cpu doesn't have SMT and all CPUs belong to the same socket.
*/
for_each_possible_cpu(cpu)
{
@@ -30,6 +102,148 @@ void __init acpi_init_cpu_topology(void)
}
}
+/*
+ * Populate the topology information by scanning the ACPI PPTT
+ * (Processor Properties Topology Table).
+ */
+void __init acpi_init_cpu_topology(void)
+{
+ acpi_status status;
+ struct acpi_table_header *header;
+ const struct acpi_table_pptt *pptt;
+ unsigned int cpu;
+
+ status = acpi_get_table(ACPI_SIG_PPTT, 0, &header);
+ if ( ACPI_FAILURE(status) )
+ {
+ printk(XENLOG_WARNING
+ "ACPI: PPTT table not found. Topology fallback will be used.\n");
+ setup_fake_topology();
+ return;
+ }
+
+ pptt = (struct acpi_table_pptt *)header;
+
+ for_each_possible_cpu(cpu)
+ {
+ unsigned int acpi_id = map_cpu_acpiid[cpu];
+ struct cpu_topology *topo = &cpu_topology[cpu];
+ const struct acpi_pptt_processor *proc;
+ unsigned int level = 0;
+ uint32_t thread_offset = 0;
+ uint32_t core_offset = 0;
+ uint32_t cluster_offset = 0;
+ uint32_t socket_offset = 0;
+ bool threading = true;
+
+ proc = find_pptt_node(&pptt->header, acpi_id);
+ if ( !proc )
+ {
+ printk(XENLOG_WARNING
+ "ACPI: No PPTT leaf node for CPU %u (ACPI ID 0x%u)\n",
+ cpu, acpi_id);
+ continue;
+ }
+
+ while ( proc )
+ {
+ if ( proc->flags & ACPI_PPTT_PHYSICAL_PACKAGE )
+ {
+ socket_offset = (char *)proc - (char *)pptt;
+ break;
+ }
+ else if ( level == 0 )
+ /*
+ * ACPI_PPTT_PROCESSOR_IS_THREAD is supported in PPTT
+ * revision 2 and later.
+ */
+ if ( proc->flags & ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD )
+ thread_offset = (char *)proc - (char *)pptt;
+ else
+ {
+ /* Assume no threading support when PPTT revision is 1. */
+ threading = false;
+ core_offset = (char *)proc - (char *)pptt;
+ }
+ else if ( level == 1 )
+ if ( threading )
+ core_offset = (char *)proc - (char *)pptt;
+ else
+ cluster_offset = (char *)proc - (char *)pptt;
+ else if ( level == 2 )
+ if ( threading )
+ cluster_offset = (char *)proc - (char *)pptt;
+
+ if ( proc->parent )
+ {
+ proc = (const struct acpi_pptt_processor *)
+ ((char *)pptt + proc->parent);
+ level++;
+ }
+ else
+ break;
+ }
+
+ topo->phys_socket_id =
+ get_logical_id(socket_offset, socket_map, &num_sockets);
+ topo->phys_cluster_id =
+ get_logical_id(cluster_offset, cluster_map, &num_clusters);
+ topo->phys_core_id =
+ get_logical_id(core_offset, core_map, &num_cores);
+
+ /* Fall back to socket ID if PPTT lacks cluster information. */
+ if ( topo->phys_cluster_id == 0 )
+ topo->phys_cluster_id = topo->phys_socket_id;
+ }
+
+ for_each_possible_cpu(cpu)
+ {
+ struct cpu_topology *topo = &cpu_topology[cpu];
+ unsigned int tcpu;
+
+ for_each_possible_cpu(tcpu)
+ {
+ struct cpu_topology *ttopo = &cpu_topology[tcpu];
+
+ if ( cpu > tcpu )
+ continue;
+
+ if ( topo->phys_core_id == ttopo->phys_core_id )
+ {
+ cpumask_set_cpu(tcpu, topo->thread_sibling);
+ cpumask_set_cpu(cpu, ttopo->thread_sibling);
+ }
+
+ if ( topo->phys_cluster_id == ttopo->phys_cluster_id )
+ {
+ cpumask_set_cpu(tcpu, topo->cluster_sibling);
+ cpumask_set_cpu(cpu, ttopo->cluster_sibling);
+ }
+
+ if ( topo->phys_socket_id == ttopo->phys_socket_id )
+ {
+ cpumask_set_cpu(tcpu, topo->core_sibling);
+ cpumask_set_cpu(cpu, ttopo->core_sibling);
+ }
+ }
+
+ topo->num_siblings = cpumask_weight(topo->thread_sibling);
+ }
+
+ for_each_possible_cpu(cpu)
+ {
+ const struct cpu_topology *topo = &cpu_topology[cpu];
+
+ printk(XENLOG_DEBUG
+ "ACPI: acpi_id[%u] CPU-%u Socket-%u Cluster-%u Core-%u\n",
+ map_cpu_acpiid[cpu],
+ cpu,
+ topo->phys_socket_id,
+ topo->phys_cluster_id,
+ topo->phys_core_id);
+ }
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/acpi/actbl3.h b/xen/include/acpi/actbl3.h
index 636d3f5f5b..48907d0532 100644
--- a/xen/include/acpi/actbl3.h
+++ b/xen/include/acpi/actbl3.h
@@ -72,6 +72,7 @@
#define ACPI_SIG_S3PT "S3PT" /* S3 Performance (sub)Table */
#define ACPI_SIG_PCCS "PCC" /* PCC Shared Memory Region */
+#define ACPI_SIG_PPTT "PPTT" /* Processor Properties Topology Table */
/* Reserved table signatures */
@@ -637,6 +638,35 @@ struct acpi_table_stao {
u8 ignore_uart;
};
+/*******************************************************************************
+ *
+ * PPTT - Processor Properties Topology Table - ACPI 6.3
+ * Version 1
+ *
+ ******************************************************************************/
+struct acpi_table_pptt {
+ struct acpi_table_header header;
+};
+
+#define ACPI_PPTT_TYPE_PROCESSOR 0
+#define ACPI_PPTT_TYPE_CACHE 1
+#define ACPI_PPTT_TYPE_ID 2
+
+struct acpi_pptt_processor {
+ struct acpi_subtable_header header;
+ u16 reserved;
+ u32 flags;
+ u32 parent;
+ u32 acpi_processor_id;
+ u32 number_of_priv_resources;
+};
+
+#define ACPI_PPTT_PHYSICAL_PACKAGE (1)
+#define ACPI_PPTT_ACPI_PROCESSOR_ID_VALID (1 << 1)
+#define ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD (1 << 2) /* ACPI 6.3 */
+#define ACPI_PPTT_ACPI_LEAF_NODE (1 << 3) /* ACPI 6.3 */
+#define ACPI_PPTT_ACPI_IDENTICAL (1 << 4) /* ACPI 6.3 */
+
/* Reset to default packing */
#pragma pack()
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index bd982ee836..e717f36151 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -139,8 +139,16 @@ static inline int acpi_boot_table_init(void)
void acpi_init_cpu_topology(void);
+extern uint32_t map_cpu_acpiid[NR_CPUS];
+
+static inline void acpi_map_cpu_acpiid(unsigned int cpu, uint32_t acpi_id)
+{
+ map_cpu_acpiid[cpu] = acpi_id;
+}
+
#else /* CONFIG_ACPI_CPU_TOPOLOGY */
+static inline void acpi_map_cpu_acpiid(unsigned int cpu, uint32_t acpi_id) {}
static inline void acpi_init_cpu_topology(void) {}
#endif /* CONFIG_ACPI_CPU_TOPOLOGY */
--
2.43.0On 10.07.2026 00:05, Hirokazu Takahashi wrote:
> --- a/xen/drivers/acpi/topology.c
> +++ b/xen/drivers/acpi/topology.c
> @@ -5,18 +5,90 @@
> #include <xen/cpumask.h>
> #include <xen/init.h>
>
> -/*
> - * TODO: Populate the topology information by scanning the ACPI
> - * PPTT (Processor Properties Topology Table).
> - */
> -void __init acpi_init_cpu_topology(void)
> +uint32_t map_cpu_acpiid[NR_CPUS] __initdata =
> + { [0 ... NR_CPUS - 1] = INVALID_ACPIID };
> +uint32_t socket_map[NR_CPUS] __initdata;
> +uint32_t cluster_map[NR_CPUS] __initdata;
> +uint32_t core_map[NR_CPUS] __initdata;
> +uint32_t thread_map[NR_CPUS] __initdata;
> +unsigned int __initdata num_sockets;
> +unsigned int __initdata num_clusters;
> +unsigned int __initdata num_cores;
static for almost all of these? And please place __initdata uniformly,
between type and identifier.
For large NR_CPUS this also looks to be adding quite a bit of data. Is all
of this really needed?
Finally please see ./CODING_STYLE as to the use of fixed-width types.
> +static unsigned int __init get_logical_id(uint32_t phys_offset,
> + uint32_t *map,
> + unsigned int *count)
> +{
> + unsigned int id;
> +
> + for ( id = 0; id < *count; id++ )
> + if ( map[id] == phys_offset )
> + return id;
> +
> + map[*count] = phys_offset;
> + id = *count;
> + (*count)++;
Imo better as either
id = (*count)++;
(or yet more simply
return (*count)++;
) or
id = *count;
++*count;
> + return id;
> +}
> +
> +static struct acpi_pptt_processor *__init find_pptt_node(
> + const struct acpi_table_header *table_hdr, unsigned int acpi_id)
Nit: Bad indentation; should be identical to ...
> +{
> + const struct acpi_subtable_header *entry;
> + unsigned long table_end;
> + const char *ptr;
... that of function-scope local variables.
> + if ( !table_hdr )
> + return NULL;
Isn't this dead code?
> + table_end = (unsigned long)table_hdr + table_hdr->length;
> +
> + ptr = (const char *)table_hdr + sizeof(struct acpi_table_pptt);
There's way too much casting and other type-unsafe code in the function. For
example, if the caller passed the full const struct acpi_table_pptt * into
here, the above (ptr being const void *) could become
ptr = pptt + 1;
> + while ( (unsigned long)ptr + sizeof(struct acpi_subtable_header)
> + <= table_end )
> + {
> + entry = (const struct acpi_subtable_header *)ptr;
Then no cast would be needed here either.
> + if ( entry->length == 0 )
> + {
> + printk(XENLOG_ERR
> + "ACPI: PPTT has an invalid zero-length subtable.\n");
> + break;
> + }
> +
> + if ( (unsigned long)ptr + entry->length > table_end )
> + {
> + printk(XENLOG_ERR
> + "ACPI: PPTT subtable extends beyond table end.\n");
> + break;
> + }
> +
> + if ( entry->type == ACPI_PPTT_TYPE_PROCESSOR )
> + if ( entry->length >= sizeof(struct acpi_pptt_processor) )
Please fold two if()s like these ones. Then again - isn't there an "else"
wanted for the inner if()? It doesn't look appropriate to continue the
loop when the length doesn't fit the type.
> + {
> + struct acpi_pptt_processor *proc =
> + (struct acpi_pptt_processor *)entry;
Please use container_of(). That'll (I think) also avoid you casting away
const-ness (which Misra objects to for a good reason).
> @@ -30,6 +102,148 @@ void __init acpi_init_cpu_topology(void)
> }
> }
>
> +/*
> + * Populate the topology information by scanning the ACPI PPTT
> + * (Processor Properties Topology Table).
> + */
> +void __init acpi_init_cpu_topology(void)
> +{
> + acpi_status status;
> + struct acpi_table_header *header;
> + const struct acpi_table_pptt *pptt;
> + unsigned int cpu;
> +
> + status = acpi_get_table(ACPI_SIG_PPTT, 0, &header);
> + if ( ACPI_FAILURE(status) )
> + {
> + printk(XENLOG_WARNING
> + "ACPI: PPTT table not found. Topology fallback will be used.\n");
> + setup_fake_topology();
> + return;
> + }
> +
> + pptt = (struct acpi_table_pptt *)header;
Again container_of() please.
> + for_each_possible_cpu(cpu)
> + {
> + unsigned int acpi_id = map_cpu_acpiid[cpu];
> + struct cpu_topology *topo = &cpu_topology[cpu];
> + const struct acpi_pptt_processor *proc;
> + unsigned int level = 0;
> + uint32_t thread_offset = 0;
> + uint32_t core_offset = 0;
> + uint32_t cluster_offset = 0;
> + uint32_t socket_offset = 0;
> + bool threading = true;
> +
> + proc = find_pptt_node(&pptt->header, acpi_id);
> + if ( !proc )
> + {
> + printk(XENLOG_WARNING
> + "ACPI: No PPTT leaf node for CPU %u (ACPI ID 0x%u)\n",
> + cpu, acpi_id);
> + continue;
> + }
> +
> + while ( proc )
> + {
> + if ( proc->flags & ACPI_PPTT_PHYSICAL_PACKAGE )
> + {
> + socket_offset = (char *)proc - (char *)pptt;
> + break;
> + }
> + else if ( level == 0 )
> + /*
> + * ACPI_PPTT_PROCESSOR_IS_THREAD is supported in PPTT
> + * revision 2 and later.
> + */
> + if ( proc->flags & ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD )
> + thread_offset = (char *)proc - (char *)pptt;
This variable is only ever set, never read.
Also I think casts to const void * are to be preferred for code like this
(if such offset calculations are needed in the first place). Or casts to
unsigned long.
> + else
> + {
> + /* Assume no threading support when PPTT revision is 1. */
> + threading = false;
> + core_offset = (char *)proc - (char *)pptt;
> + }
> + else if ( level == 1 )
> + if ( threading )
> + core_offset = (char *)proc - (char *)pptt;
> + else
> + cluster_offset = (char *)proc - (char *)pptt;
> + else if ( level == 2 )
> + if ( threading )
> + cluster_offset = (char *)proc - (char *)pptt;
PLease add braces to separate inner if/else from outer ones.
> + if ( proc->parent )
> + {
> + proc = (const struct acpi_pptt_processor *)
> + ((char *)pptt + proc->parent);
> + level++;
> + }
> + else
> + break;
> + }
> +
> + topo->phys_socket_id =
> + get_logical_id(socket_offset, socket_map, &num_sockets);
> + topo->phys_cluster_id =
> + get_logical_id(cluster_offset, cluster_map, &num_clusters);
> + topo->phys_core_id =
> + get_logical_id(core_offset, core_map, &num_cores);
What if any of the ..._offset is still 0?
> + /* Fall back to socket ID if PPTT lacks cluster information. */
> + if ( topo->phys_cluster_id == 0 )
> + topo->phys_cluster_id = topo->phys_socket_id;
Why would 0 indicate the absence of cluster information? Isn't it
cluster_offset being 0 which does so?
> + }
> +
> + for_each_possible_cpu(cpu)
> + {
> + struct cpu_topology *topo = &cpu_topology[cpu];
> + unsigned int tcpu;
> +
> + for_each_possible_cpu(tcpu)
> + {
> + struct cpu_topology *ttopo = &cpu_topology[tcpu];
> +
> + if ( cpu > tcpu )
> + continue;
> +
> + if ( topo->phys_core_id == ttopo->phys_core_id )
> + {
> + cpumask_set_cpu(tcpu, topo->thread_sibling);
> + cpumask_set_cpu(cpu, ttopo->thread_sibling);
> + }
> +
> + if ( topo->phys_cluster_id == ttopo->phys_cluster_id )
> + {
> + cpumask_set_cpu(tcpu, topo->cluster_sibling);
> + cpumask_set_cpu(cpu, ttopo->cluster_sibling);
> + }
> +
> + if ( topo->phys_socket_id == ttopo->phys_socket_id )
> + {
> + cpumask_set_cpu(tcpu, topo->core_sibling);
> + cpumask_set_cpu(cpu, ttopo->core_sibling);
> + }
> + }
> +
> + topo->num_siblings = cpumask_weight(topo->thread_sibling);
> + }
> +
> + for_each_possible_cpu(cpu)
> + {
> + const struct cpu_topology *topo = &cpu_topology[cpu];
> +
> + printk(XENLOG_DEBUG
> + "ACPI: acpi_id[%u] CPU-%u Socket-%u Cluster-%u Core-%u\n",
> + map_cpu_acpiid[cpu],
> + cpu,
> + topo->phys_socket_id,
> + topo->phys_cluster_id,
> + topo->phys_core_id);
> + }
Is this meant to stay? It can be a lot of output with many CPUs.
> --- a/xen/include/acpi/actbl3.h
> +++ b/xen/include/acpi/actbl3.h
> @@ -72,6 +72,7 @@
>
> #define ACPI_SIG_S3PT "S3PT" /* S3 Performance (sub)Table */
> #define ACPI_SIG_PCCS "PCC" /* PCC Shared Memory Region */
> +#define ACPI_SIG_PPTT "PPTT" /* Processor Properties Topology Table */
>
> /* Reserved table signatures */
>
> @@ -637,6 +638,35 @@ struct acpi_table_stao {
> u8 ignore_uart;
> };
>
> +/*******************************************************************************
> + *
> + * PPTT - Processor Properties Topology Table - ACPI 6.3
> + * Version 1
> + *
> + ******************************************************************************/
> +struct acpi_table_pptt {
> + struct acpi_table_header header;
> +};
> +
> +#define ACPI_PPTT_TYPE_PROCESSOR 0
> +#define ACPI_PPTT_TYPE_CACHE 1
> +#define ACPI_PPTT_TYPE_ID 2
> +
> +struct acpi_pptt_processor {
> + struct acpi_subtable_header header;
> + u16 reserved;
> + u32 flags;
> + u32 parent;
> + u32 acpi_processor_id;
> + u32 number_of_priv_resources;
> +};
> +
> +#define ACPI_PPTT_PHYSICAL_PACKAGE (1)
> +#define ACPI_PPTT_ACPI_PROCESSOR_ID_VALID (1 << 1)
> +#define ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD (1 << 2) /* ACPI 6.3 */
> +#define ACPI_PPTT_ACPI_LEAF_NODE (1 << 3) /* ACPI 6.3 */
> +#define ACPI_PPTT_ACPI_IDENTICAL (1 << 4) /* ACPI 6.3 */
> +
> /* Reset to default packing */
>
> #pragma pack()
Linux, which presumably still takes it from ACPI CA, has this in actbl2.h.
Please match placement as closely as possible. Ideally take (as a separate,
prereq patch) the Linux commit(s) adding the definitions. See
docs/process/sending-patches.pandoc for formal aspects of doing so.
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -139,8 +139,16 @@ static inline int acpi_boot_table_init(void)
>
> void acpi_init_cpu_topology(void);
>
> +extern uint32_t map_cpu_acpiid[NR_CPUS];
Since this is __initdata, imo ...
> +static inline void acpi_map_cpu_acpiid(unsigned int cpu, uint32_t acpi_id)
... this would better be annotated __init as well, even if for an inline
function that's unlikely to take any effect. Other than the important one
here: Documentation.
Jan
Hello,
> > --- a/xen/drivers/acpi/topology.c
> > +++ b/xen/drivers/acpi/topology.c
> > @@ -5,18 +5,90 @@
> > #include <xen/cpumask.h>
> > #include <xen/init.h>
> >
> > -/*
> > - * TODO: Populate the topology information by scanning the ACPI
> > - * PPTT (Processor Properties Topology Table).
> > - */
> > -void __init acpi_init_cpu_topology(void)
> > +uint32_t map_cpu_acpiid[NR_CPUS] __initdata =
> > + { [0 ... NR_CPUS - 1] = INVALID_ACPIID };
> > +uint32_t socket_map[NR_CPUS] __initdata;
> > +uint32_t cluster_map[NR_CPUS] __initdata;
> > +uint32_t core_map[NR_CPUS] __initdata;
> > +uint32_t thread_map[NR_CPUS] __initdata;
> > +unsigned int __initdata num_sockets;
> > +unsigned int __initdata num_clusters;
> > +unsigned int __initdata num_cores;
>
> static for almost all of these? And please place __initdata uniformly,
> between type and identifier.
Okay.
> For large NR_CPUS this also looks to be adding quite a bit of data. Is all
> of this really needed?
The map_cpu_acpiid[NR_CPUS] array is required. This is because acpi_smp_init_cpus()
parses the MADT and populates map_cpu_acpiid[] with the discovered CPUs. At the
point when acpi_smp_init_cpus() is called, the total number of CPUs has not yet been
determined.
The memory for socket_map[NR_CPUS], cluster_map[NR_CPUS], and core_map[NR_CPUS]
is automatically freed after Xen initialization completes, as they are marked as __initdata.
Therefore, I am not sure if there is any clear advantage to switching them to dynamic
allocation and deallocation.
> Finally please see ./CODING_STYLE as to the use of fixed-width types.
Okay, I will fix them.
> > +static unsigned int __init get_logical_id(uint32_t phys_offset,
> > + uint32_t *map,
> > + unsigned int *count)
> > +{
> > + unsigned int id;
> > +
> > + for ( id = 0; id < *count; id++ )
> > + if ( map[id] == phys_offset )
> > + return id;
> > +
> > + map[*count] = phys_offset;
> > + id = *count;
> > + (*count)++;
>
> Imo better as either
>
> id = (*count)++;
>
> (or yet more simply
>
> return (*count)++;
Okay.
> ) or
>
> id = *count;
> ++*count;
>
> > + return id;
> > +}
> > +
> > +static struct acpi_pptt_processor *__init find_pptt_node(
> > + const struct acpi_table_header *table_hdr, unsigned int acpi_id)
>
> Nit: Bad indentation; should be identical to ...
Okay.
> > +{
> > + const struct acpi_subtable_header *entry;
> > + unsigned long table_end;
> > + const char *ptr;
>
> ... that of function-scope local variables.
>
> > + if ( !table_hdr )
> > + return NULL;
>
> Isn't this dead code?
Indeed, this is redundant as the caller already validates the table status.
I will drop the code or replace it with BUG_ON(!table_hdr );
> > + table_end = (unsigned long)table_hdr + table_hdr->length;
> > +
> > + ptr = (const char *)table_hdr + sizeof(struct acpi_table_pptt);
>
> There's way too much casting and other type-unsafe code in the function. For
> example, if the caller passed the full const struct acpi_table_pptt * into
> here, the above (ptr being const void *) could become
>
> ptr = pptt + 1;
Okay.
> > + while ( (unsigned long)ptr + sizeof(struct acpi_subtable_header)
> > + <= table_end )
> > + {
> > + entry = (const struct acpi_subtable_header *)ptr;
>
> Then no cast would be needed here either.
>
> > + if ( entry->length == 0 )
> > + {
> > + printk(XENLOG_ERR
> > + "ACPI: PPTT has an invalid zero-length subtable.\n");
> > + break;
> > + }
> > +
> > + if ( (unsigned long)ptr + entry->length > table_end )
> > + {
> > + printk(XENLOG_ERR
> > + "ACPI: PPTT subtable extends beyond table end.\n");
> > + break;
> > + }
> > +
> > + if ( entry->type == ACPI_PPTT_TYPE_PROCESSOR )
> > + if ( entry->length >= sizeof(struct acpi_pptt_processor) )
>
> Please fold two if()s like these ones. Then again - isn't there an "else"
> wanted for the inner if()? It doesn't look appropriate to continue the
> loop when the length doesn't fit the type.
Okay.
> > + {
> > + struct acpi_pptt_processor *proc =
> > + (struct acpi_pptt_processor *)entry;
>
> Please use container_of(). That'll (I think) also avoid you casting away
> const-ness (which Misra objects to for a good reason).
Okay.
> > @@ -30,6 +102,148 @@ void __init acpi_init_cpu_topology(void)
> > }
> > }
> >
> > +/*
> > + * Populate the topology information by scanning the ACPI PPTT
> > + * (Processor Properties Topology Table).
> > + */
> > +void __init acpi_init_cpu_topology(void)
> > +{
> > + acpi_status status;
> > + struct acpi_table_header *header;
> > + const struct acpi_table_pptt *pptt;
> > + unsigned int cpu;
> > +
> > + status = acpi_get_table(ACPI_SIG_PPTT, 0, &header);
> > + if ( ACPI_FAILURE(status) )
> > + {
> > + printk(XENLOG_WARNING
> > + "ACPI: PPTT table not found. Topology fallback will be used.\n");
> > + setup_fake_topology();
> > + return;
> > + }
> > +
> > + pptt = (struct acpi_table_pptt *)header;
>
> Again container_of() please.
Okay.
> > + for_each_possible_cpu(cpu)
> > + {
> > + unsigned int acpi_id = map_cpu_acpiid[cpu];
> > + struct cpu_topology *topo = &cpu_topology[cpu];
> > + const struct acpi_pptt_processor *proc;
> > + unsigned int level = 0;
> > + uint32_t thread_offset = 0;
> > + uint32_t core_offset = 0;
> > + uint32_t cluster_offset = 0;
> > + uint32_t socket_offset = 0;
> > + bool threading = true;
> > +
> > + proc = find_pptt_node(&pptt->header, acpi_id);
> > + if ( !proc )
> > + {
> > + printk(XENLOG_WARNING
> > + "ACPI: No PPTT leaf node for CPU %u (ACPI ID 0x%u)\n",
> > + cpu, acpi_id);
> > + continue;
> > + }
> > +
> > + while ( proc )
> > + {
> > + if ( proc->flags & ACPI_PPTT_PHYSICAL_PACKAGE )
> > + {
> > + socket_offset = (char *)proc - (char *)pptt;
> > + break;
> > + }
> > + else if ( level == 0 )
> > + /*
> > + * ACPI_PPTT_PROCESSOR_IS_THREAD is supported in PPTT
> > + * revision 2 and later.
> > + */
> > + if ( proc->flags & ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD )
> > + thread_offset = (char *)proc - (char *)pptt;
>
> This variable is only ever set, never read.
>
> Also I think casts to const void * are to be preferred for code like this
> (if such offset calculations are needed in the first place). Or casts to
> unsigned long.
Okay.
> > + else
> > + {
> > + /* Assume no threading support when PPTT revision is 1. */
> > + threading = false;
> > + core_offset = (char *)proc - (char *)pptt;
> > + }
> > + else if ( level == 1 )
> > + if ( threading )
> > + core_offset = (char *)proc - (char *)pptt;
> > + else
> > + cluster_offset = (char *)proc - (char *)pptt;
> > + else if ( level == 2 )
> > + if ( threading )
> > + cluster_offset = (char *)proc - (char *)pptt;
>
> PLease add braces to separate inner if/else from outer ones.
Okay.
> > + if ( proc->parent )
> > + {
> > + proc = (const struct acpi_pptt_processor *)
> > + ((char *)pptt + proc->parent);
> > + level++;
> > + }
> > + else
> > + break;
> > + }
> > +
> > + topo->phys_socket_id =
> > + get_logical_id(socket_offset, socket_map, &num_sockets);
> > + topo->phys_cluster_id =
> > + get_logical_id(cluster_offset, cluster_map, &num_clusters);
> > + topo->phys_core_id =
> > + get_logical_id(core_offset, core_map, &num_cores);
>
> What if any of the ..._offset is still 0?
A value of `cluster_offset == 0` does not mean that cluster information is absent; rather,
it happens when the PPTT is malformed or corrupted.
The same applies to `socket_offset == 0` and `core_offset == 0`. It would be much
better to treat all of these as error cases and reject them. I will add proper validation
and error handling for these zero offsets.
> > + /* Fall back to socket ID if PPTT lacks cluster information. */
> > + if ( topo->phys_cluster_id == 0 )
> > + topo->phys_cluster_id = topo->phys_socket_id;
>
> Why would 0 indicate the absence of cluster information? Isn't it
> cluster_offset being 0 which does so?
If the PPTT lacks cluster description, all CPUs reuse the same offset, causing
get_logical_id() to always return 0.
Even when clusters do exist and the actual cluster ID 0 is assigned, the socket ID
will also be 0, so the fallback assignment still results in the correct topology.
I will add a comment to explain this tricky part.
> > + }
> > +
> > + for_each_possible_cpu(cpu)
> > + {
> > + struct cpu_topology *topo = &cpu_topology[cpu];
> > + unsigned int tcpu;
> > +
> > + for_each_possible_cpu(tcpu)
> > + {
> > + struct cpu_topology *ttopo = &cpu_topology[tcpu];
> > +
> > + if ( cpu > tcpu )
> > + continue;
> > +
> > + if ( topo->phys_core_id == ttopo->phys_core_id )
> > + {
> > + cpumask_set_cpu(tcpu, topo->thread_sibling);
> > + cpumask_set_cpu(cpu, ttopo->thread_sibling);
> > + }
> > +
> > + if ( topo->phys_cluster_id == ttopo->phys_cluster_id )
> > + {
> > + cpumask_set_cpu(tcpu, topo->cluster_sibling);
> > + cpumask_set_cpu(cpu, ttopo->cluster_sibling);
> > + }
> > +
> > + if ( topo->phys_socket_id == ttopo->phys_socket_id )
> > + {
> > + cpumask_set_cpu(tcpu, topo->core_sibling);
> > + cpumask_set_cpu(cpu, ttopo->core_sibling);
> > + }
> > + }
> > +
> > + topo->num_siblings = cpumask_weight(topo->thread_sibling);
> > + }
> > +
> > + for_each_possible_cpu(cpu)
> > + {
> > + const struct cpu_topology *topo = &cpu_topology[cpu];
> > +
> > + printk(XENLOG_DEBUG
> > + "ACPI: acpi_id[%u] CPU-%u Socket-%u Cluster-%u Core-%u\n",
> > + map_cpu_acpiid[cpu],
> > + cpu,
> > + topo->phys_socket_id,
> > + topo->phys_cluster_id,
> > + topo->phys_core_id);
> > + }
>
> Is this meant to stay? It can be a lot of output with many CPUs.
This is just for debugging.
I will delete it.
> > --- a/xen/include/acpi/actbl3.h
> > +++ b/xen/include/acpi/actbl3.h
> > @@ -72,6 +72,7 @@
> >
> > #define ACPI_SIG_S3PT "S3PT" /* S3 Performance (sub)Table */
> > #define ACPI_SIG_PCCS "PCC" /* PCC Shared Memory Region */
> > +#define ACPI_SIG_PPTT "PPTT" /* Processor Properties Topology Table */
> >
> > /* Reserved table signatures */
> >
> > @@ -637,6 +638,35 @@ struct acpi_table_stao {
> > u8 ignore_uart;
> > };
> >
> >
> +/*************************************************************************
> ******
> > + *
> > + * PPTT - Processor Properties Topology Table - ACPI 6.3
> > + * Version 1
> > + *
> > +
> ***************************************************************************
> ***/
> > +struct acpi_table_pptt {
> > + struct acpi_table_header header;
> > +};
> > +
> > +#define ACPI_PPTT_TYPE_PROCESSOR 0
> > +#define ACPI_PPTT_TYPE_CACHE 1
> > +#define ACPI_PPTT_TYPE_ID 2
> > +
> > +struct acpi_pptt_processor {
> > + struct acpi_subtable_header header;
> > + u16 reserved;
> > + u32 flags;
> > + u32 parent;
> > + u32 acpi_processor_id;
> > + u32 number_of_priv_resources;
> > +};
> > +
> > +#define ACPI_PPTT_PHYSICAL_PACKAGE (1)
> > +#define ACPI_PPTT_ACPI_PROCESSOR_ID_VALID (1 << 1)
> > +#define ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD (1 << 2) /* ACPI 6.3 */
> > +#define ACPI_PPTT_ACPI_LEAF_NODE (1 << 3) /* ACPI 6.3 */
> > +#define ACPI_PPTT_ACPI_IDENTICAL (1 << 4) /* ACPI 6.3 */
> > +
> > /* Reset to default packing */
> >
> > #pragma pack()
>
> Linux, which presumably still takes it from ACPI CA, has this in actbl2.h.
> Please match placement as closely as possible. Ideally take (as a separate,
> prereq patch) the Linux commit(s) adding the definitions. See
> docs/process/sending-patches.pandoc for formal aspects of doing so.
The locations where macros and structures are defined in actbl1.h, actbl2.h, and
actbl3.h are completely different between the Linux kernel and Xen.
For example, linux's actbl2.h and Xen's actbl2.h have an entirely different set of
table signatures. Given this mismatch, attempting to fully align with Linux's placement might
require updating or reorganizing Xen's ACPI headers first.
Linux kernel: actbl2.h
/*
* Values for description table header signatures for tables defined in this
* file. Useful because they make it more difficult to inadvertently type in
* the wrong signature.
*/
#define ACPI_SIG_AGDI "AGDI" /* Arm Generic Diagnostic Dump and Reset Device Interface */
#define ACPI_SIG_APMT "APMT" /* Arm Performance Monitoring Unit table */
#define ACPI_SIG_BDAT "BDAT" /* BIOS Data ACPI Table */
#define ACPI_SIG_CCEL "CCEL" /* CC Event Log Table */
#define ACPI_SIG_CDAT "CDAT" /* Coherent Device Attribute Table */
#define ACPI_SIG_ERDT "ERDT" /* Enhanced Resource Director Technology */
#define ACPI_SIG_IORT "IORT" /* IO Remapping Table */
#define ACPI_SIG_IOVT "IOVT" /* I/O Virtualization Table */
#define ACPI_SIG_IVRS "IVRS" /* I/O Virtualization Reporting Structure */
#define ACPI_SIG_KEYP "KEYP" /* Key Programming Interface for IDE */
#define ACPI_SIG_LPIT "LPIT" /* Low Power Idle Table */
#define ACPI_SIG_MADT "APIC" /* Multiple APIC Description Table */
#define ACPI_SIG_MCFG "MCFG" /* PCI Memory Mapped Configuration table */
#define ACPI_SIG_MCHI "MCHI" /* Management Controller Host Interface table */
#define ACPI_SIG_MPAM "MPAM" /* Memory System Resource Partitioning and Monitoring Table */
#define ACPI_SIG_MPST "MPST" /* Memory Power State Table */
#define ACPI_SIG_MRRM "MRRM" /* Memory Range and Region Mapping table */
#define ACPI_SIG_MSDM "MSDM" /* Microsoft Data Management Table */
#define ACPI_SIG_NFIT "NFIT" /* NVDIMM Firmware Interface Table */
#define ACPI_SIG_NHLT "NHLT" /* Non HD Audio Link Table */
#define ACPI_SIG_PCCT "PCCT" /* Platform Communications Channel Table */
#define ACPI_SIG_PDTT "PDTT" /* Platform Debug Trigger Table */
#define ACPI_SIG_PHAT "PHAT" /* Platform Health Assessment Table */
#define ACPI_SIG_PMTT "PMTT" /* Platform Memory Topology Table */
#define ACPI_SIG_PPTT "PPTT" /* Processor Properties Topology Table */
#define ACPI_SIG_PRMT "PRMT" /* Platform Runtime Mechanism Table */
#define ACPI_SIG_RASF "RASF" /* RAS Feature table */
#define ACPI_SIG_RAS2 "RAS2" /* RAS2 Feature table */
#define ACPI_SIG_RGRT "RGRT" /* Regulatory Graphics Resource Table */
#define ACPI_SIG_RHCT "RHCT" /* RISC-V Hart Capabilities Table */
#define ACPI_SIG_RIMT "RIMT" /* RISC-V IO Mapping Table */
#define ACPI_SIG_SBST "SBST" /* Smart Battery Specification Table */
#define ACPI_SIG_SDEI "SDEI" /* Software Delegated Exception Interface Table */
#define ACPI_SIG_SDEV "SDEV" /* Secure Devices table */
#define ACPI_SIG_SVKL "SVKL" /* Storage Volume Key Location Table */
#define ACPI_SIG_SWFT "SWFT" /* SoundWire File Table */
#define ACPI_SIG_TDEL "TDEL" /* TD Event Log Table */
Xen: actbl2.h
/*
* Values for description table header signatures for tables defined in this
* file. Useful because they make it more difficult to inadvertently type in
* the wrong signature.
*/
#define ACPI_SIG_ASF "ASF!" /* Alert Standard Format table */
#define ACPI_SIG_BOOT "BOOT" /* Simple Boot Flag Table */
#define ACPI_SIG_DBG2 "DBG2" /* Debug Port table type 2 */
#define ACPI_SIG_DBGP "DBGP" /* Debug Port table */
#define ACPI_SIG_DMAR "DMAR" /* DMA Remapping table */
#define ACPI_SIG_HPET "HPET" /* High Precision Event Timer table */
#define ACPI_SIG_IBFT "IBFT" /* i_sCSI Boot Firmware Table */
#define ACPI_SIG_IORT "IORT" /* IO Remapping Table */
#define ACPI_SIG_IVRS "IVRS" /* I/O Virtualization Reporting Structure */
#define ACPI_SIG_MCFG "MCFG" /* PCI Memory Mapped Configuration table */
#define ACPI_SIG_MCHI "MCHI" /* Management Controller Host Interface table */
#define ACPI_SIG_SLIC "SLIC" /* Software Licensing Description Table */
#define ACPI_SIG_SPCR "SPCR" /* Serial Port Console Redirection table */
#define ACPI_SIG_SPMI "SPMI" /* Server Platform Management Interface table */
#define ACPI_SIG_TCPA "TCPA" /* Trusted Computing Platform Alliance table */
#define ACPI_SIG_UEFI "UEFI" /* Uefi Boot Optimization Table */
#define ACPI_SIG_WAET "WAET" /* Windows ACPI Emulated devices Table */
#define ACPI_SIG_WDAT "WDAT" /* Watchdog Action Table */
#define ACPI_SIG_WDDT "WDDT" /* Watchdog Timer Description Table */
#define ACPI_SIG_WDRT "WDRT" /* Watchdog Resource Table */
#ifdef ACPI_UNDEFINED_TABLES
/*
* These tables have been seen in the field, but no definition has been found
*/
#define ACPI_SIG_ATKG "ATKG"
#define ACPI_SIG_GSCI "GSCI" /* GMCH SCI table */
#define ACPI_SIG_IEIT "IEIT"
#endif
> > --- a/xen/include/xen/acpi.h
> > +++ b/xen/include/xen/acpi.h
> > @@ -139,8 +139,16 @@ static inline int acpi_boot_table_init(void)
> >
> > void acpi_init_cpu_topology(void);
> >
> > +extern uint32_t map_cpu_acpiid[NR_CPUS];
>
> Since this is __initdata, imo ...
>
> > +static inline void acpi_map_cpu_acpiid(unsigned int cpu, uint32_t acpi_id)
>
> ... this would better be annotated __init as well, even if for an inline
> function that's unlikely to take any effect. Other than the important one
> here: Documentation.
Okay.
Thank you,
Hirokazu Takahashi.
On 10.07.2026 13:59, Hirokazu Takahashi wrote:
>>> --- a/xen/drivers/acpi/topology.c
>>> +++ b/xen/drivers/acpi/topology.c
>>> @@ -5,18 +5,90 @@
>>> #include <xen/cpumask.h>
>>> #include <xen/init.h>
>>>
>>> -/*
>>> - * TODO: Populate the topology information by scanning the ACPI
>>> - * PPTT (Processor Properties Topology Table).
>>> - */
>>> -void __init acpi_init_cpu_topology(void)
>>> +uint32_t map_cpu_acpiid[NR_CPUS] __initdata =
>>> + { [0 ... NR_CPUS - 1] = INVALID_ACPIID };
>>> +uint32_t socket_map[NR_CPUS] __initdata;
>>> +uint32_t cluster_map[NR_CPUS] __initdata;
>>> +uint32_t core_map[NR_CPUS] __initdata;
>>> +uint32_t thread_map[NR_CPUS] __initdata;
>>> +unsigned int __initdata num_sockets;
>>> +unsigned int __initdata num_clusters;
>>> +unsigned int __initdata num_cores;
>>
>> static for almost all of these? And please place __initdata uniformly,
>> between type and identifier.
>
> Okay.
>
>> For large NR_CPUS this also looks to be adding quite a bit of data. Is all
>> of this really needed?
>
> The map_cpu_acpiid[NR_CPUS] array is required. This is because acpi_smp_init_cpus()
> parses the MADT and populates map_cpu_acpiid[] with the discovered CPUs. At the
> point when acpi_smp_init_cpus() is called, the total number of CPUs has not yet been
> determined.
>
> The memory for socket_map[NR_CPUS], cluster_map[NR_CPUS], and core_map[NR_CPUS]
> is automatically freed after Xen initialization completes, as they are marked as __initdata.
As is map_cpu_acpiid[]'s memory, sure.
> Therefore, I am not sure if there is any clear advantage to switching them to dynamic
> allocation and deallocation.
I wasn't necessarily trying to hint at dynamic allocation, but yes, that may
be an option. The main point is that, as previously indicated, the number of
NR_CPUS-dimensioned arrays we have would better go down, not up. While here
it's memory that's freed post-init, it still grows image size: This isn't
.bss-like data; it's .data-like. You may want to take a look at the BRK
allocator (patches to have it in x86 have been pending for quite a while [1]).
That may be possible to leverage here (requiring it to become generic though,
not x86-specific): No space taken in image, but (almost) arbitrary amounts of
space reservable at build time.
Of course, none of the above eliminates the request to try to get away with
less memory overall.
Jan
[1] https://lists.xen.org/archives/html/xen-devel/2025-11/msg00390.html
© 2016 - 2026 Red Hat, Inc.