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

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hello,

Thank you for your advice.

> > diff --git a/xen/common/device-tree/cpu-topology.c
> b/xen/common/device-tree/cpu-topology.c
> > new file mode 100644
> > index 0000000000..bbdf0d1fe8
> > --- /dev/null
> > +++ b/xen/common/device-tree/cpu-topology.c
> > @@ -0,0 +1,342 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Derived from Linux kernel 7.0's $drivers/base/arch_topology.c
> > + * Parse cpu topology information.
> > + *
> > + * Copyright (c) 2026 VA Linux Systems Japan K.K.
> > + * Author: Hirokazu Takahashi <taka@xxxxxxxxxxxxx>
> 
> We don't commonly have copyright header in the tree and instead use the
> commit message to keep track of Copyright. That said, if you want to
> keep it, I think you ought to keep the copyright from Linux because your
> code is based on it.

Ok, I will remove the copyright lines from the header.

> > +#include <xen/cpu.h>
> > +#include <xen/cpumask.h>
> > +#include <xen/delay.h>
> > +#include <xen/device_tree.h>
> > +#include <xen/cpu-topology.h>
> > +#include <xen/numa.h>
> > +#include <xen/domain_page.h>
> > +#include <xen/errno.h>
> > +#include <xen/init.h>
> > +
> > +struct cpu_map {
> > +    unsigned int thread_id;
> > +    unsigned int core_id;
> > +    unsigned int cluster_id;
> > +    unsigned int package_id;
> > +};
> > +
> > +struct cpu_topology *cpu_topology;
> 
> Looking at the use in the other patch, you seem to unconditionally use
> cpu_topology when CONFIG_DT_CPU_TOPOLOGY. However, you don't seem to
> fill it when the system is using ACPI.
> 
> So I think this either needs to be moved to common code and filled by
> ACPI or we need to make clear in the name that this is DT specific.
> 
> [...]

You are completely right, and thank you for pointing out this gap. 
I agree that the ACPI path needs to be properly handled.
I will fill the cpu information with temporary values for now though the goal 
is to
populate it by scanning the ACPI PPTT (Processor Properties Topology Table).

> > +void __init dt_init_cpu_topology(void)
> > +{
> > +    unsigned int cpu;
> > +    const unsigned int nr_cpus = cpumask_last(&cpu_possible_map) + 1U;
> > +
> > +    cpu_topology = xzalloc_array(struct cpu_topology, nr_cpus);
> > +    if ( !cpu_topology )
> > +        panic("Failed to allocate memory for cpu_topology array\n");
> > +
> > +    if (parse_dt_topology())
> 
> Style: AFAICT, this is following Xen style. So it wants to be:
> 
> if ( ... )

Ok, I will fix it.

Thank you,
Hirokazu Takahashi.