| Message ID | 20210516102900.28036-4-wangyanan55@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt: Introduce cpu topology support | expand |
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. > * 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/ > + * in the system: socket/cluster/core/thread. The comment says there are four levels including 'cluster', but there's no 'cluster' below. > + */ > + qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map"); > + > + for (cpu = smp_cpus - 1; cpu >= 0; cpu--) { > + char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu); > + char *map_path; > + > + if (ms->smp.threads > 1) { > + map_path = g_strdup_printf( > + "/cpus/cpu-map/%s%d/%s%d/%s%d", > + "socket", cpu / (ms->smp.cores * ms->smp.threads), > + "core", (cpu / ms->smp.threads) % ms->smp.cores, > + "thread", cpu % ms->smp.threads); > + } else { > + map_path = g_strdup_printf( > + "/cpus/cpu-map/%s%d/%s%d", > + "socket", cpu / ms->smp.cores, > + "core", cpu % ms->smp.cores); > + } > + qemu_fdt_add_path(ms->fdt, map_path); > + qemu_fdt_setprop_phandle(ms->fdt, map_path, "cpu", cpu_path); > + > + g_free(map_path); > + g_free(cpu_path); > + } > + } > } > > static void fdt_add_its_gic_node(VirtMachineState *vms) > -- > 2.19.1 > Thanks, drew
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. >> * 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. [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? Thanks, Yanan >> + */ >> + qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map"); >> + >> + for (cpu = smp_cpus - 1; cpu >= 0; cpu--) { >> + char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu); >> + char *map_path; >> + >> + if (ms->smp.threads > 1) { >> + map_path = g_strdup_printf( >> + "/cpus/cpu-map/%s%d/%s%d/%s%d", >> + "socket", cpu / (ms->smp.cores * ms->smp.threads), >> + "core", (cpu / ms->smp.threads) % ms->smp.cores, >> + "thread", cpu % ms->smp.threads); >> + } else { >> + map_path = g_strdup_printf( >> + "/cpus/cpu-map/%s%d/%s%d", >> + "socket", cpu / ms->smp.cores, >> + "core", cpu % ms->smp.cores); >> + } >> + qemu_fdt_add_path(ms->fdt, map_path); >> + qemu_fdt_setprop_phandle(ms->fdt, map_path, "cpu", cpu_path); >> + >> + g_free(map_path); >> + g_free(cpu_path); >> + } >> + } >> } >> >> static void fdt_add_its_gic_node(VirtMachineState *vms) >> -- >> 2.19.1 >> > Thanks, > drew > > .
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
On 2021/5/18 15:46, Andrew Jones wrote: > 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 Ok, will fix. Thanks! > Thanks, > drew > > .
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 * 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 + * in the system: socket/cluster/core/thread. + */ + qemu_fdt_add_subnode(ms->fdt, "/cpus/cpu-map"); + + for (cpu = smp_cpus - 1; cpu >= 0; cpu--) { + char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu); + char *map_path; + + if (ms->smp.threads > 1) { + map_path = g_strdup_printf( + "/cpus/cpu-map/%s%d/%s%d/%s%d", + "socket", cpu / (ms->smp.cores * ms->smp.threads), + "core", (cpu / ms->smp.threads) % ms->smp.cores, + "thread", cpu % ms->smp.threads); + } else { + map_path = g_strdup_printf( + "/cpus/cpu-map/%s%d/%s%d", + "socket", cpu / ms->smp.cores, + "core", cpu % ms->smp.cores); + } + qemu_fdt_add_path(ms->fdt, map_path); + qemu_fdt_setprop_phandle(ms->fdt, map_path, "cpu", cpu_path); + + g_free(map_path); + g_free(cpu_path); + } + } } static void fdt_add_its_gic_node(VirtMachineState *vms)