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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hi Jan,

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

Okay,

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

I will add code to check if the number of private resources fits within the 
limit 
derived from the maximum subtable size (255 bytes).

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

Ok.

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

Understood, I will add code to ensure 'acpi_id' is valid.

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

Oops, I will fix it.

Thank you,
Hirokazu Takahashi.