Re: [RFC PATCH v3 3/9] hw/arm/virt: Add cpu-map to device tree

[Andrew Jones]

On Mon, May 17, 2021 at 11:00:07PM +0800, wangyanan (Y) wrote:
> Hi Drew,
> 
> On 2021/5/17 14:41, Andrew Jones wrote:
> > On Sun, May 16, 2021 at 06:28:54PM +0800, Yanan Wang wrote:
> > > From: Andrew Jones <drjones@redhat.com>
> > > 
> > > Support device tree CPU topology descriptions.
> > > 
> > > In accordance with the Devicetree Specification, the Linux Doc
> > > "arm/cpus.yaml" requires that cpus and cpu nodes in the DT are
> > > present. And we meet the requirement by creating /cpus/cpu@*
> > > nodes for members within ms->smp.cpus.
> > > 
> > > Correspondingly, we should also create subnodes in cpu-map for
> > > the present cpus, each of which relates to an unique cpu node.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > > ---
> > >   hw/arm/virt.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 40 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index c07841e3a4..e5dcdebdbc 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -349,10 +349,11 @@ static void fdt_add_cpu_nodes(const 
> > > VirtMachineState *vms)
> > >       int cpu;
> > >       int addr_cells = 1;
> > >       const MachineState *ms = MACHINE(vms);
> > > +    const VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > >       int smp_cpus = ms->smp.cpus;
> > >       /*
> > > -     * From Documentation/devicetree/bindings/arm/cpus.txt
> > > +     *  See Linux Documentation/devicetree/bindings/arm/cpus.yaml
> > Rather than aligning the top line with the lower lines, we could remove
> > the extra space from the lower lines. Or, leave the formatting as it was,
> > by putting 'See' where 'From' was, like I did in my original patch.
> I think I prefer removing the extra space from the lower lines, which is
> the right thing to do.

OK

> > >        *  On ARM v8 64-bit systems value should be set to 2,
> > >        *  that corresponds to the MPIDR_EL1 register size.
> > >        *  If MPIDR_EL1[63:32] value is equal to 0 on all CPUs
> > > @@ -405,8 +406,46 @@ static void fdt_add_cpu_nodes(const VirtMachineState 
> > > *vms)
> > >                   ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
> > >           }
> > > +        if (!vmc->no_cpu_topology) {
> > > +            qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle",
> > > +                                  qemu_fdt_alloc_phandle(ms->fdt));
> > > +        }
> > > +
> > >           g_free(nodename);
> > >       }
> > > +
> > > +    if (!vmc->no_cpu_topology) {
> > > +        /*
> > > +         * See Linux 
> > > Documentation/devicetree/bindings/cpu/cpu-topology.txt
> > > +         * In a SMP system, the hierarchy of CPUs is defined through four
> > > +         * entities that are used to describe the layout of physical CPUs
> > s/entities/levels/
> Above comment was completely from Linux Doc cpu-topology.txt. See [1].
> I think entities may be more reasonable than levels here, since there can be
> multiple levels of clusters in cpu-map which makes the total not four.

OK

> 
> [1] 
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/cpu/cpu-topology.txt
> > > +         * in the system: socket/cluster/core/thread.
> > The comment says there are four levels including 'cluster', but there's no
> > 'cluster' below.
> According to Doc [1] (line 114), a socket node's child nodes must be
> *one or more* cluster nodes which means cluster is mandatory to be
> socket's child in DT.
> 
> So I think maybe we should just keep the comment as-is, and change
> the map-path from /cpus/cpu-map/socket*/cores*/threads* to
> /cpus/cpu-map/socket*/cluster0/cores*/threads* in this patch?

I agree. In fact, that's how I implemented it myself[1]

[1] 
https://github.com/rhdrjones/qemu/commit/35feecdd43475608c8f55973a0c159eac4aafefd

Thanks,
drew