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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hi Jan,

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

I will remove it.

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

Okay, I will fix it.
 
> > +            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.

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

I will remove the cast.

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

Thank you for pointing this out.
I will add a proper bounds check here.

> > +        topo->num_siblings = cpumask_weight(topo->thread_sibling);
> > +    }
> > +
> > +out:
> 
> Nit: Labels indented by at least one blank please. See ./CODING_STYLE.

Okay, I will.

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

I will thoroughly check this patch against the general comments from earlier 
patches as well.

Thank you,
Hirokazu Takahashi.