Re: [PATCH v6 5/5] xen/acpi: Parse PPTT to initialize CPU topology

On 14.07.2026 12:44, Hirokazu Takahashi wrote:
> --- a/xen/drivers/acpi/topology.c
> +++ b/xen/drivers/acpi/topology.c
> @@ -5,32 +5,257 @@
>  #include <xen/cpumask.h>
>  #include <xen/init.h>
>  
> +#define ACPI_PPTT_MAX_LEVELS 16
> +
> +static uint32_t __initdata map_cpu_acpiid[NR_CPUS] = {
> +    [0 ... NR_CPUS - 1] = INVALID_ACPIID
> +};
> +
> +/*
> + * The first argument `cpu` is the logical CPU ID assigned by Xen,
> + * and the second argument `acpi_id` is passed the `uid` field from
> + * the ACPI MADT Generic Interrupt subtable.
> + */
> +void __init acpi_map_cpu_acpiid(unsigned int cpu, uint32_t acpi_id)
> +{
> +    map_cpu_acpiid[cpu] = acpi_id;
> +}
> +
> +static unsigned int __init get_logical_id(unsigned int key,
> +                                          unsigned int *map,
> +                                          unsigned int *count)
> +{
> +    unsigned int id;
> +
> +    for ( id = 0; id < *count; id++ )
> +        if ( map[id] == key )
> +            return id;
> +
> +    map[*count] = key;
> +
> +    return (*count)++;
> +}
> +
> +static const struct acpi_pptt_processor *__init find_pptt_node(
> +    const struct acpi_table_pptt *pptt, uint32_t acpi_id)
> +{
> +    const struct acpi_subtable_header *entry;
> +    unsigned long table_end;
> +    const void *ptr;
> +
> +    BUG_ON(!pptt);

This being a static helper, I don't see a need for such a check.

> +    table_end = (unsigned long)pptt + pptt->header.length;
> +
> +    ptr = pptt + 1;
> +
> +    while ( (unsigned long)ptr + sizeof(struct acpi_subtable_header)

Please prefer sizeof(<expression>) over sizeof(<type>) whenever there's a
connection to an expression in use (e.g. sizeof(*entry) here).

> +            <= table_end )
> +    {
> +        entry = 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 &&
> +             entry->length >= sizeof(struct acpi_pptt_processor) )
> +        {
> +            const struct acpi_pptt_processor *proc =
> +                container_of(entry, const struct acpi_pptt_processor, header);
> +
> +            if ( (proc->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) &&
> +                 proc->acpi_processor_id == acpi_id )
> +                return proc;
> +        }
> +
> +        ptr += entry->length;
> +    }
> +
> +    return NULL;
> +}
> +
>  /*
> - * TODO: Populate the topology information by scanning the ACPI
> - *       PPTT (Processor Properties Topology Table).
> + * Populate the topology information by scanning the ACPI PPTT
> + * (Processor Properties Topology Table).
>   */
>  int __init acpi_init_cpu_topology(void)
>  {
> +    acpi_status status;
> +    struct acpi_table_header *table_header;
> +    const struct acpi_table_pptt *pptt;
> +    unsigned int num_sockets = 0;
> +    unsigned int num_clusters = 0;
> +    unsigned int num_cores = 0;
> +    unsigned int *socket_map = xmalloc_array(unsigned int, nr_cpu_ids);
> +    unsigned int *cluster_map = xmalloc_array(unsigned int, nr_cpu_ids);
> +    unsigned int *core_map = xmalloc_array(unsigned int, nr_cpu_ids);
>      unsigned int cpu;
> +    int ret = 0;
> +
> +    if ( !socket_map || !cluster_map || !core_map )
> +    {
> +        printk(XENLOG_ERR
> +               "ACPI: Failed to allocate memory for topology parsing.\n");
> +        ret = -ENOMEM;
> +        goto out;
> +    }
> +
> +    status = acpi_get_table(ACPI_SIG_PPTT, 0, &table_header);
> +    if ( ACPI_FAILURE(status) )
> +    {
> +        printk(XENLOG_WARNING
> +               "ACPI: PPTT table not found. Topology fallback will be used.\n");
> +        ret = -ENODEV;
> +        goto out;
> +    }
> +
> +    pptt = container_of(table_header, const struct acpi_table_pptt, header);
>  
> -    /*
> -     * Generate temporary cpu topology information for now.
> -     * It assumes that the cpu doesn't have SMT and all CPUs
> -     * belong to the same socket.
> -     */
>      for_each_possible_cpu(cpu)
>      {
> +        uint32_t acpi_id = map_cpu_acpiid[cpu];
>          struct cpu_topology *topo = &cpu_topology[cpu];
> +        const struct acpi_pptt_processor *proc;
> +        unsigned int level;
> +        unsigned int core_group_key = 0;
> +        unsigned int cluster_group_key = 0;
> +        unsigned int socket_group_key = 0;
> +        bool threading = false;
> +
> +        proc = find_pptt_node(pptt, acpi_id);
> +        if ( !proc )
> +        {
> +            printk(XENLOG_WARNING
> +                   "ACPI: No PPTT leaf node for CPU %u (ACPI ID 0x%u)\n",
> +                   cpu, acpi_id);
> +            ret = -ENOENT;
> +            goto out;
> +        }
> +
> +        /*
> +         * Limit the maximum loop depth to prevent an infinite loop in case
> +         * the PPTT is corrupted or contains cyclic references.
> +         */
> +        for ( level = 0; level < ACPI_PPTT_MAX_LEVELS; level++ )
> +        {
> +            const unsigned int offset = (const void *)proc - (const void *)pptt;
> +
> +            if ( proc->flags & ACPI_PPTT_PHYSICAL_PACKAGE )
> +            {
> +                socket_group_key = offset;
> +
> +                /*
> +                 * If cluster/core info is absent upon reaching the physical
> +                 * package, assume one cluster per socket and one core per
> +                 * cluster.
> +                 */
> +                if ( cluster_group_key == 0 )
> +                    cluster_group_key = socket_group_key;
> +
> +                if ( core_group_key == 0 )
> +                    core_group_key = cluster_group_key;
> +
> +                break;
> +            }
> +            else if ( level == 0 )
> +            {
> +                /*
> +                 * ACPI_PPTT_PROCESSOR_IS_THREAD is supported in PPTT
> +                 * revision 2 and later. Assume no threading support when
> +                 * PPTT revision is 1.
> +                 */
> +                if ( proc->flags & ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD )
> +                    threading = true;
> +                else
> +                    core_group_key = offset;
> +            }
> +            else if ( level == 1 )
> +            {
> +                if ( threading )
> +                    core_group_key = offset;
> +                else
> +                    cluster_group_key = offset;

Mich like you're omitting braces here, you can also ...

> +            }
> +            else if ( level == 2 && threading )
> +            {
> +                cluster_group_key = offset;
> +            }

... omit them here.

> +            if ( !proc->parent )
> +                break;
>  
> -        topo->phys_core_id = cpu;
> -        topo->num_siblings = 1;
> +            proc = (const struct acpi_pptt_processor *)
> +                   ((const void *)pptt + proc->parent);

No need for the outer cast?

And then - how do you know proc->parent points inside the table?

> +        }
>  
> -        cpumask_set_cpu(cpu, topo->thread_sibling);
> -        cpumask_copy(topo->core_sibling, &cpu_possible_map);
> -        cpumask_copy(topo->cluster_sibling, &cpu_possible_map);
> +        if ( socket_group_key == 0 )
> +        {
> +            printk(XENLOG_WARNING
> +                   "ACPI: Could not reach the physical package node for CPU %u (ACPI ID 0x%u)\n",
> +                   cpu, acpi_id);
> +            ret = -ENOENT;
> +            goto out;
> +        }
> +
> +        topo->phys_socket_id =
> +            get_logical_id(socket_group_key, socket_map, &num_sockets);
> +        topo->phys_cluster_id =
> +            get_logical_id(cluster_group_key, cluster_map, &num_clusters);
> +        topo->phys_core_id =
> +            get_logical_id(core_group_key, core_map, &num_cores);
>      }
>  
> -    return 0;
> +    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);
> +    }
> +
> +out:

Nit: Labels indented by at least one blank please. See ./CODING_STYLE.

Other comments (of the more general kind) given on earlier patches may also
apply here.

Jan