Re: [PATCH v5 4/4] arm/acpi: Parse PPTT to initialize CPU topology

[Jan Beulich <jbeulich@xxxxxxxx>]

On 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