Re: [PATCH v8 4/4] xen/acpi: Parse PPTT to initialize CPU topology

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.07.2026 02:55, Hirokazu Takahashi wrote:
> 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 threading. For ACPI 6.2 and
> earlier, CPUs are assumed not to support threading.
> 
> Signed-off-by: Hirokazu Takahashi <taka@xxxxxxxxxxxxx>
> ---
> Changes in v8:
>   * Relaxed processor node length check to allow
>     proc->header.length >= table_size.
>   * Treated root nodes (nodes with parent == 0) as equivalent to
>     ACPI_PPTT_PHYSICAL_PACKAGE nodes.
>   * Moved INVALID_ACPIID definition to xen/acpi.h to make it available
>     cross-architecture.
>   * Removed ARM-specific references from comments in
>     architecture-agnostic code.
>   * Added explicit parentheses around bitwise operations in logical
>     expressions.
>   * Placed binary logical operators (||, &&) at the end of lines when
>     splitting conditional statements.

This rule, however, ...

> --- a/xen/drivers/acpi/topology.c
> +++ b/xen/drivers/acpi/topology.c
> @@ -4,33 +4,315 @@
>  #include <xen/cpu-topology.h>
>  #include <xen/cpumask.h>
>  #include <xen/init.h>
> +#include <xen/xvmalloc.h>
> +
> +#define ACPI_PPTT_MAX_LEVELS 16
> +
> +static uint32_t __initdata map_cpu_acpiid[NR_CPUS] = {
> +    [0 ... NR_CPUS - 1] = INVALID_ACPIID
> +};
>  
>  /*
> - * TODO: Populate the topology information by scanning the ACPI
> - *       PPTT (Processor Properties Topology Table).
> + * The first argument 'cpu' is the logical CPU ID assigned by Xen,
> + * and the second argument 'acpi_id' the 32-bit ACPI processor ID.
>   */
> -int __init acpi_init_cpu_topology(void)
> +void __init acpi_map_cpu_acpiid(unsigned int cpu, uint32_t acpi_id)
>  {
> -    unsigned int cpu;
> +    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 bool __init verify_subtable(const struct acpi_subtable_header *entry,
> +                                   const struct acpi_table_pptt *pptt)
> +{
> +    unsigned long table_end = (unsigned long)pptt + pptt->header.length;
> +
> +    if ( entry->length < sizeof(*entry) || (entry->length & 3) )
> +    {
> +        printk(XENLOG_ERR "ACPI: PPTT subtable length is invalid\n");
> +        return false;
> +    }
> +
> +    if ( (unsigned long)entry + entry->length > table_end )
> +    {
> +        printk(XENLOG_ERR "ACPI: PPTT subtable extends beyond table end\n");
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static bool __init verify_proc(const struct acpi_pptt_processor *proc)
> +{
> +    unsigned long table_size;
> +
> +    if ( proc->header.length < sizeof(*proc) )
> +    {
> +        printk(XENLOG_ERR "ACPI: PPTT processor node length is too small\n");
> +        return false;
> +    }
>  
>      /*
> -     * Generate temporary cpu topology information for now.
> -     * It assumes that the cpu doesn't have SMT and all CPUs
> -     * belong to the same socket.
> +     * Each private resource is represented by a 32-bit resource ID.
> +     * Ensure the structure length accurately accounts for the trailing 
> array.
>       */
> +    table_size = sizeof(*proc)
> +        + (unsigned long)proc->number_of_priv_resources * sizeof(uint32_t);

... holds for all binary operators, i.e. also the + here. Indentation of the
continuing line would also better be

    table_size = sizeof(*proc) +
                 (unsigned long)proc->number_of_priv_resources * 
sizeof(uint32_t);

Where I'd then still question the need for the cast: The operation is, afaict,
the same with it dropped, both in ILP32 and LP64 models. With one exception -
there is an overflow potentially getting in the way, but the cast helps with
that only in LP64. Question is whether too large a ->number_of_priv_resources
wouldn't better be rejected uniformly as presumably bogus.

> +
> +    if ( proc->header.length < table_size )
> +    {
> +        printk(XENLOG_ERR "ACPI: PPTT processor node length invalid\n");
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +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 = (unsigned long)pptt + pptt->header.length;
> +    const void *ptr = pptt + 1;
> +
> +    while ( (unsigned long)ptr + sizeof(*entry) <= table_end )
> +    {
> +        entry = ptr;
> +
> +        if ( !verify_subtable(entry, pptt) )
> +            break;
> +
> +        if ( entry->type == ACPI_PPTT_TYPE_PROCESSOR )
> +        {
> +            const struct acpi_pptt_processor *proc;
> +
> +            proc = container_of(entry, const struct acpi_pptt_processor,
> +                                header);

Have this be the initializer of the variable:

            const struct acpi_pptt_processor *proc =
                container_of(entry, const struct acpi_pptt_processor, header);

?

> +            if ( !verify_proc(proc) )
> +                break;
> +
> +            /*
> +             * Leaf node verification is only required for ACPI 6.3
> +             * (PPTT revision 2) or later.
> +             */
> +            if ( (proc->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) &&
> +                 proc->acpi_processor_id == acpi_id &&
> +                 (pptt->header.revision < 2 ||
> +                  (proc->flags & ACPI_PPTT_ACPI_LEAF_NODE)) )
> +                return proc;
> +        }
> +
> +        ptr += entry->length;
> +    }
> +
> +    return NULL;
> +}
> +
> +/*
> + * 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 = xvzalloc_array(unsigned int, nr_cpu_ids);
> +    unsigned int *cluster_map = xvzalloc_array(unsigned int, nr_cpu_ids);
> +    unsigned int *core_map = xvzalloc_array(unsigned int, nr_cpu_ids);
> +    unsigned int cpu;
> +    int ret = 0;
> +
> +    status = acpi_get_table(ACPI_SIG_PPTT, 0, &table_header);
> +    if ( ACPI_FAILURE(status) )
> +    {
> +        /* A missing PPTT is benign; fall back to the default topology. */
> +        ret = -ENODEV;
> +        goto out;
> +    }
> +
> +    if ( !socket_map || !cluster_map || !core_map )
> +    {
> +        printk(XENLOG_ERR
> +               "ACPI: Failed to allocate memory for topology parsing\n");
> +        ret = -ENOMEM;
> +        goto out;
> +    }
> +
> +    pptt = container_of(table_header, const struct acpi_table_pptt, header);
> +
> +    for_each_possible_cpu(cpu)
> +    {
> +        uint32_t acpi_id = map_cpu_acpiid[cpu];

You may end up reading INVALID_ACPIID here. Then ...

> +        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);

... a bogus table entry with that ID would be found here, potentially for
multiple values of "cpu".

> +        if ( !proc )
> +        {
> +            printk(XENLOG_WARNING
> +                   "ACPI: No PPTT leaf node for CPU %u (ACPI ID %#x)\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 this proc has no parent node, it is the root node. Treat it
> +             * as equivalent to an ACPI_PPTT_PHYSICAL_PACKAGE node.
> +             */
> +            if ( proc->flags & ACPI_PPTT_PHYSICAL_PACKAGE || !proc->parent )

You say in the revlog that you added parentheses, but at least here you
didn't.

Jan