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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hi Michal,

Thank you for the review.

> > @@ -245,6 +247,10 @@ static void __init dt_smp_init_cpus(void)
> >          }
> >          else
> >              tmp_map[i] = hwid;
> > +
> > +        if ( (rc >= 0) || (i == 0) )
> Please add a comment explaining the second part i.e. boot CPU may lack enable
> method.

Okay. I will add a comment.

> > +            /* Pass the info to dt_init_cpu_topology() */
> > +            map_cpu_to_dt_node(i, cpu);
> >      }


> > diff --git a/xen/common/Makefile b/xen/common/Makefile
> > index f69d47d189..d0eb39a633 100644
> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_GENERIC_BUG_FRAME) += bug.o
> >  obj-$(CONFIG_HYPFS_CONFIG) += config_data.o
> >  obj-$(CONFIG_CORE_PARKING) += core_parking.o
> >  obj-y += cpu.o
> > +obj-$(CONFIG_GENERIC_CPU_TOPOLOGY) += cpu-topology.init.o
> >  obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o
> >  obj-$(CONFIG_HAS_DEVICE_TREE_DISCOVERY) += device.o
> >  obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
> > diff --git a/xen/common/cpu-topology.c b/xen/common/cpu-topology.c
> > new file mode 100644
> > index 0000000000..ce50408858
> > --- /dev/null
> > +++ b/xen/common/cpu-topology.c
> > @@ -0,0 +1,69 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <xen/acpi.h>
> > +#include <xen/cpu-topology.h>
> > +#include <xen/cpumask.h>
> > +#include <xen/dt-cpu-topology.h>
> > +#include <xen/init.h>
> > +#include <xen/xvmalloc.h>
> > +
> > +static void __init free_topology_table(void)
> > +{
> > +    unsigned int cpu;
> > +
> > +    for ( cpu = 0; cpu < nr_cpu_ids; cpu++ )
> > +    {
> > +        free_cpumask_var(cpu_topology[cpu].thread_sibling);
> > +        free_cpumask_var(cpu_topology[cpu].core_sibling);
> > +        free_cpumask_var(cpu_topology[cpu].cluster_sibling);
> > +    }
> > +
> > +    XVFREE(cpu_topology);
> > +}
> > +
> > +void __init init_cpu_topology(void)
> > +{
> > +    unsigned int cpu;
> > +    int ret;
> > +
> > +    if ( num_possible_cpus() == 1 )
> > +        return;
> > +
> > +    cpu_topology = xvzalloc_array(struct cpu_topology, nr_cpu_ids);
> > +    if ( !cpu_topology )
> > +    {
> > +        printk(XENLOG_WARNING "CPU topology table allocation failed.\n");
> > +        return;
> > +    }
> > +
> > +    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) )
> > +        {
> > +            free_topology_table();
> > +            printk(XENLOG_WARNING "CPU topology table allocation 
> > failed.\n");
> > +            return;
> > +        }
> > +    }
> > +
> > +    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 )
> > +        free_topology_table();
> In a previous revision I asked to print an error (except for expected errors
> like -EOPNOTSUPP).

My intention is to avoid duplicate messages because dt_init_cpu_topology() and
acpi_init_cpu_topology() already emit detailed error logs upon failure.

> > +static int __init parse_core(const struct dt_device_node *core,
> > +                             unsigned int socket_id,
> > +                             unsigned int cluster_id,
> > +                             unsigned int core_id)
> > +{
> > +    bool leaf = true;
> > +    unsigned int thread_id;
> > +    unsigned int cpu;
> > +
> > +    for ( thread_id = 0; ; thread_id++ )
> > +    {
> > +        const struct dt_device_node *thread;
> > +        char name[20];
> > +
> > +        snprintf(name, sizeof(name), "thread%u", thread_id);
> > +        thread = dt_find_child_node_by_name(core, name);
> > +
> > +        if ( !thread )
> > +            break;
> > +
> > +        leaf = false;
> > +        cpu = get_cpu_for_node(thread);
> > +
> > +        if ( cpu == INVALID_TOPO_ID )
> get_cpu_for_node can return INVALID_TOPO_ID also on malformed DTs e.g. missing
> phandle which you now accept. Shouldn't we return -EINVAL?

Okay. 

> > +        {
> > +            /*
> > +             * Ignore this CPU node definition as the corresponding 
> > physical
> > +             * CPU failed to boot.
> > +             */
> > +            printk(XENLOG_INFO
> > +                   "%s: CPU for thread hasn't started\n", 
> > dt_node_name(thread));
> CPU topology is initialized before bringing CPU up, so this message is 
> incorrect
> and there are more than one scenarios we may end up with here. Maybe just "CPU
> not used by Xen: ignoring".

You are right. 

> > +            return 0;
> This stops parsing other threads which I don't think is the right way to do 
> it.
> We should just continue.

You are right.

> > +    else if ( leaf )
> > +    {
> > +        /*
> > +         * Ignore this CPU node definition as the corresponding physical
> > +         * CPU failed to boot.
> > +         */
> > +        printk(XENLOG_INFO
> > +               "%s: CPU for leaf core hasn't started\n",
> dt_node_name(core));
> Same wording issue.

Okay.

Thanks,
Hirokazu Takahashi.