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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hello,

> > --- /dev/null
> > +++ b/xen/common/cpu-topology.c
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <xen/cpumask.h>
> > +#include <xen/cpu-topology.h>
> > +#include <xen/init.h>
> > +#include <xen/acpi.h>
> > +
> > +void __init init_cpu_topology(void)
> > +{
> > +    const unsigned int nr_cpus = cpumask_last(&cpu_possible_map) +
> 1U;
> > +
> > +    cpu_topology = xzalloc_array(struct cpu_topology, nr_cpus);
> 
> cpu_topology exists as a global variable only when DT is in use. I think the
> definition needs to move here (from common/device-tree/cpu-topology.c).

Okay

> As to the size of the array, it's not quite clear to me whether by doing it
> this way (instead of using nr_cpu_ids) we're not setting ourselves up for
> trouble.

On ARM64 Xen, nr_cpu_ids represents the total number of populated/available 
CPUs, but unfortunately it cannot be relied upon as the maximum CPU ID. 

For instance, if a CPU node in the Device Tree has an invalid 'enable-method' 
property, that CPU ID slot is still consumed during the initial parsing, but 
the CPU is not counted towards nr_cpu_ids. This can result in a sparse CPU ID 
allocation where the maximum CPU ID actually exceeds. 

If we were to use nr_cpu_ids as the array size here, we would risk an 
out-of-bounds access under such faulty Device Tree configurations. This is 
why I used "cpumask_last(&cpu_possible_map) + 1U" to ensure the array is 
large enough to cover the highest allocated CPU ID.

Consequently, there might actually be potential bugs in other parts of Xen 
where nr_cpu_ids is incorrectly assumed to be the upper bound for CPU ID 
indexing on ARM.

> > +    if ( !cpu_topology )
> > +        panic("Failed to allocate memory for cpu_topology array\n");
> 
> I question such uses of panic(): Surely we can do without any NUMA info,
> it's only performance which is going to suffer.

Okay, I will replace the panic() with a XENLOG_WARNING printk.

> > +
> > +static struct dt_device_node * __init dt_find_child_node_by_name(struct 
> > dt_device_node *from, const char *name)
> 
> Nit: Overlong line here, and ...

Okay, I will fix it.

> > +static int __init parse_core(struct dt_device_node *core,
> > +                unsigned int package_id, unsigned int cluster_id,
> > +                unsigned int core_id)
> 
> ... bogus indentation e.g. here. Please go though yourself to check style.

Okay.

> > --- /dev/null
> > +++ b/xen/drivers/acpi/topology.c
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <xen/cpumask.h>
> > +#include <xen/cpu-topology.h>
> > +#include <xen/init.h>
> > +#include <xen/acpi.h>
> > +
> > +/*
> > + * ToDo: Populate the topology information by scanning the ACPI
> > + *       PPTT (Processor Properties Topology Table).
> 
> Please can this be spelled "TODO:", to stand out more and to be a hit also
> with case sensitive grep for "TODO"?

Okay.

> > --- a/xen/include/xen/acpi.h
> > +++ b/xen/include/xen/acpi.h
> > @@ -101,6 +101,8 @@ void acpi_table_print (struct acpi_table_header
> *header, unsigned long phys_addr
> >  void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
> >  void acpi_table_print_srat_entry (struct acpi_subtable_header *srat);
> >
> > +void acpi_init_cpu_topology(void);
> > +
> >  /* the following four functions are architecture-dependent */
> >  void acpi_numa_slit_init (struct acpi_table_slit *slit);
> >  void acpi_numa_processor_affinity_init(const struct
> acpi_srat_cpu_affinity *);
> > @@ -133,6 +135,8 @@ static inline int acpi_boot_table_init(void)
> >     return 0;
> >  }
> >
> > +static inline void acpi_init_cpu_topology(void) {}
> 
> This shouldn't be needed. When ACPI=y, acpi_disabled is compile-time true,
> and hence the compiler can and will DCE the call. All it needs to see is a
> declaration, which therefore wants to move outside of the CONFIG_ACPI
> conditional.

okay
 
> > --- /dev/null
> > +++ b/xen/include/xen/cpu-topology.h
> > @@ -0,0 +1,36 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef XEN_CPU_TOPOLOGY_H
> > +#define XEN_CPU_TOPOLOGY_H
> > +
> > +#include <xen/types.h>
> > +#include <xen/dt-cpu-topology.h>
> > +
> > +struct cpu_topology {
> > +    cpumask_t thread_sibling;
> > +    cpumask_t core_sibling;
> > +    cpumask_t cluster_sibling;
> > +};
> 
> With huge NR_CPUS this can be pretty large a struct (of which in 
> init_cpu_topology()
> you allocate an array). Imo you want to use cpumask_var_t here, with 
> allocation added
> as needed.

Okay.

> > +
> > +
> 
> Nit: No double blank lines please.

Okay.

Thank you,
Hirokazu Takahashi.