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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hello, 

> > --- /dev/null
> > +++ b/xen/include/xen/cpu-topology.h
> > @@ -0,0 +1,42 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef XEN_CPU_TOPOLOGY_H
> > +#define XEN_CPU_TOPOLOGY_H
> > +
> > +#include <xen/types.h>
> > +#include <xen/device_tree.h>
> > +
> > +struct cpu_topology {
> > +    int thread_id;
> > +    int core_id;
> > +    int cluster_id;
> > +    int package_id;
> 
> Can any of these go negative? If not, "unsigned int" please. Iirc I said the
> same elsewhere on v1. Please apply such comments throughout the series.

These members can actually be '-1' to represent a missing entry when parsing 
the Device Tree's 
'cpu-map' node. 

Since I imported this directly from Linux kernel 7.0 (specifically from 
include/linux/arch_topology.h 
and drivers/base/arch_topology.c), I would lean toward keeping them as 'int' to 
minimize 
any divergence from the original Linux implementation. 

Would that be acceptable?


> > +
> > +#ifdef CONFIG_DT_CPU_TOPOLOGY
> > +extern struct cpu_topology cpu_topology[NR_CPUS];
> 
> If at all possible, no new NR_CPUS dimensioned arrays please.

This is also part of the code imported from Linux kernel 7.0, which is used to 
store
unique topology data per CPU. 

I kept it as an array to match the original Linux source, but I understand the 
concern
about adding new NR_CPUS dimensioned arrays in Xen. I can refactor it to use 
Xen's
per-CPU infrastructure instead. 
 
> > +void map_cpuid_to_node(u32 cpuid, struct dt_device_node *cpu_node);
> > +void dt_init_cpu_topology(void);
> > +
> > +#elif CONFIG_DEVICE_TREE_PARSE
> > +
> > +static inline void map_cpuid_to_node(u32 cpuid, struct dt_device_node
> *cpu_node) {}
> 
> No new uses of u32 please. As to use of fixed-width type, please see
> ./CODING_STYLE.

Ok, I will use unsigned int instead.

Thank you,
Hirokazu Takahashi.