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 <drjo...@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 <drjo...@redhat.com> > > > Co-developed-by: Yanan Wang <wangyana...@huawei.com> > > > Signed-off-by: Yanan Wang <wangyana...@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