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

On 20.07.2026 18:28, Hirokazu Takahashi wrote:
> --- 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)

This way the constant is becoming available only on Arm, but ...

> --- a/xen/drivers/acpi/topology.c
> +++ b/xen/drivers/acpi/topology.c
> @@ -4,33 +4,319 @@
>  #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

... you use it in code which (in principle) isn't Arm-specific (it only
happens to be right now).

> +};
>  
>  /*
> - * 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' is passed the 'uid' field from
> + * the ACPI MADT Generic Interrupt subtable.

This comment also looks to be (in part) Arm-specific, which it shouldn't be
here.

>   */
> -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 )

Parentheses please around & or alike within || or alike.

> +    {
> +        printk(XENLOG_ERR "ACPI: PPTT subtable length is invalid\n");

Like you have it here, ...

> +        return false;
> +    }
> +
> +    if ( (unsigned long)entry + entry->length > table_end )
> +    {
> +        printk(XENLOG_ERR "ACPI: PPTT subtable extends beyond table end.\n");

... no full stop please in log messages (applies elsewhere as well).

> +        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);
> +
> +    if ( proc->header.length != table_size )

Isn't != overly strict?

> +    {
> +        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;
> +    const void *ptr;
> +
> +    table_end = (unsigned long)pptt + pptt->header.length;
> +
> +    ptr = pptt + 1;

Make these the initializers of their variables?

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

Misplaced binary operators (also elsewhere). On continued lines they go at
the end of the earlier line.

> +                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];
>          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",

Please prefer the slightly shorter %#x (and definitely please don't mix a 0x
prefix with %u). Again applies elsewhere as well.

Jan