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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hi Jan,

> > --- 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).

Okay, I will move the definition in xen/acpi.h.

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

Okay.

> > +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.

Okay.

> > +    {
> > +        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).

Okay.

> > +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?

I will replace it with "if ( proc->header.length < table_size )."

> > +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?

Okay.

> > +            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.

Okay.

> > +        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.

Okay.

Thank you,
Hirokazu Takahashi.