RE: [PATCH v12 1/4] xen/device-tree: Parse 'cpu-map' node for CPU topology exploration

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hi Michal,

> >  xen/common/device-tree/cpu-topology.c | 370 ++++++++++++++++++++++++++
> >  xen/drivers/acpi/Makefile             |   1 +
> >  xen/drivers/acpi/topology.c           |  43 +++
> Shouldn't this file be called cpu-topology like the DT equivalent?

Okay.

> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index 8c9072f3c1..d34396f9b1 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -9,10 +9,12 @@
> >
> >  #include <xen/acpi.h>
> >  #include <xen/cpu.h>
> > +#include <xen/cpu-topology.h>
> >  #include <xen/cpumask.h>
> >  #include <xen/delay.h>
> >  #include <xen/device_tree.h>
> >  #include <xen/domain_page.h>
> > +#include <xen/dt-cpu-topology.h>
> Nothing in this file seems to use this header

This header is required because this file uses map_cpu_to_dt_node().
This extern declaration is provided in it.

> > +void __init init_cpu_topology(void)
> > +{
> > +    unsigned int cpu;
> > +    int ret = 0;
> > +
> > +    if ( num_possible_cpus() == 1 )
> > +        return;
> > +
> > +    cpu_topology = xvzalloc_array(struct cpu_topology, nr_cpu_ids);
> > +
> > +    if ( cpu_topology )
> > +    {
> > +        for ( cpu = 0; cpu < nr_cpu_ids; cpu++ )
> > +        {
> > +            if ( !zalloc_cpumask_var(&cpu_topology[cpu].thread_sibling)
> ||
> > +                 !zalloc_cpumask_var(&cpu_topology[cpu].core_sibling)
> ||
> >
> +                 !zalloc_cpumask_var(&cpu_topology[cpu].cluster_sibling)
>  )
> > +                ret = -ENOMEM;
> The loop continues allocating after the first error which is incorrect.

You are right. I will fix it.

> > +        }
> > +    }
> > +    else
> > +        ret = -ENOMEM;
> > +
> > +    if ( ret != 0 )
> > +        printk(XENLOG_WARNING "CPU topology table allocation failed\n");
> After printing this one we will also print the "Failed to initialize..." one.
> Please goto the error.

Okay.

> > +    else if ( acpi_disabled )
> > +        ret = dt_init_cpu_topology();
> > +    else
> > +        ret = acpi_init_cpu_topology();
> > +
> > +    /* Free the CPU topology table if initialization fails. */
> > +    if ( ret != 0 )
> In v11 I asked you to print error on real failure, not all the time e.g. on -
> EOPNOTSUPP or ENOENT as a result of not finding cpu-map.

Okay
 
> > +    {
> > +        printk("Failed to initialize cpu topology (error %d)\n", ret);
> XENLOG_ERR/WARNING?

I will fix it.


> > +        if ( !thread )
> > +            break;
> > +
> > +        leaf = false;
> > +        ret = get_cpu_for_node(thread, &cpu);
> > +        if ( ret )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "%s: No CPU found for thread\n", dt_node_name(thread));
> Use dt_node_full_name(). Otherwise the printed name can be ambiguous.

Okay.

> > +            return ret;
> > +        }
> > +
> > +        if ( cpu == INVALID_TOPO_ID )
> > +        {
> > +            /*
> > +             * Ignore this CPU node definition as the corresponding 
> > physical
> > +             * CPU failed to boot.
> You updated the printk message but not the comment. Please fix.

Okay