* Re: [Qemu-devel] [PATCH] riscv: virt: Add cpu-topology DT node.
@ 2019-06-24 23:24 ` Alistair Francis
0 siblings, 0 replies; 12+ messages in thread
From: Alistair Francis @ 2019-06-24 23:24 UTC (permalink / raw)
To: Atish Patra
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alistair Francis
On Mon, Jun 24, 2019 at 3:57 PM Atish Patra <atish.patra@wdc.com> wrote:
>
> Currently, there is no cpu topology defined in RISC-V.
> Define a device tree node that clearly describes the
> entire topology. This saves the trouble of scanning individual
> cache to figure out the topology.
>
> Here is the linux kernel patch series that enables topology
> for RISC-V.
>
> http://lists.infradead.org/pipermail/linux-riscv/2019-June/005072.html
>
> CPU topology after applying this patch in QEMU & above series in kernel
>
> / # cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
> 2
> / # cat /sys/devices/system/cpu/cpu2/topology/physical_package_id
> 0
> / # cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
> 0-7
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
> hw/riscv/virt.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 84d94d0c42d8..da0b8aa18747 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -203,9 +203,12 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
> qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
> qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
> qemu_fdt_setprop_string(fdt, nodename, "device_type", "cpu");
> + qemu_fdt_setprop_cell(fdt, nodename, "phandle", cpu_phandle);
> + qemu_fdt_setprop_cell(fdt, nodename, "linux,phandle", cpu_phandle);
> + int intc_phandle = phandle++;
Don't declare variables in the middle of code. The variable must be
declared at the start of a block.
With that fixed:
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> qemu_fdt_add_subnode(fdt, intc);
> - qemu_fdt_setprop_cell(fdt, intc, "phandle", cpu_phandle);
> - qemu_fdt_setprop_cell(fdt, intc, "linux,phandle", cpu_phandle);
> + qemu_fdt_setprop_cell(fdt, intc, "phandle", intc_phandle);
> + qemu_fdt_setprop_cell(fdt, intc, "linux,phandle", intc_phandle);
> qemu_fdt_setprop_string(fdt, intc, "compatible", "riscv,cpu-intc");
> qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL, 0);
> qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
> @@ -214,6 +217,20 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
> g_free(nodename);
> }
>
> + /* Add cpu-topology node */
> + qemu_fdt_add_subnode(fdt, "/cpus/cpu-map");
> + qemu_fdt_add_subnode(fdt, "/cpus/cpu-map/cluster0");
> + for (cpu = s->soc.num_harts - 1; cpu >= 0; cpu--) {
> + char *core_nodename = g_strdup_printf("/cpus/cpu-map/cluster0/core%d",
> + cpu);
> + char *cpu_nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
> + uint32_t intc_phandle = qemu_fdt_get_phandle(fdt, cpu_nodename);
> + qemu_fdt_add_subnode(fdt, core_nodename);
> + qemu_fdt_setprop_cell(fdt, core_nodename, "cpu", intc_phandle);
> + g_free(core_nodename);
> + g_free(cpu_nodename);
> + }
> +
> cells = g_new0(uint32_t, s->soc.num_harts * 4);
> for (cpu = 0; cpu < s->soc.num_harts; cpu++) {
> nodename =
> --
> 2.21.0
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: virt: Add cpu-topology DT node.
2019-06-24 23:24 ` Alistair Francis
@ 2019-06-24 23:42 ` Atish Patra
-1 siblings, 0 replies; 12+ messages in thread
From: Atish Patra @ 2019-06-24 23:42 UTC (permalink / raw)
To: alistair23@gmail.com
Cc: qemu-riscv@nongnu.org, kbastian@mail.uni-paderborn.de,
Alistair Francis, palmer@sifive.com, qemu-devel@nongnu.org,
sagark@eecs.berkeley.edu
On Mon, 2019-06-24 at 16:24 -0700, Alistair Francis wrote:
> On Mon, Jun 24, 2019 at 3:57 PM Atish Patra <atish.patra@wdc.com>
> wrote:
> > Currently, there is no cpu topology defined in RISC-V.
> > Define a device tree node that clearly describes the
> > entire topology. This saves the trouble of scanning individual
> > cache to figure out the topology.
> >
> > Here is the linux kernel patch series that enables topology
> > for RISC-V.
> >
> > http://lists.infradead.org/pipermail/linux-riscv/2019-June/005072.html
> >
> > CPU topology after applying this patch in QEMU & above series in
> > kernel
> >
> > / # cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
> > 2
> > / # cat /sys/devices/system/cpu/cpu2/topology/physical_package_id
> > 0
> > / # cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
> > 0-7
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> > hw/riscv/virt.c | 21 +++++++++++++++++++--
> > 1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 84d94d0c42d8..da0b8aa18747 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -203,9 +203,12 @@ static void *create_fdt(RISCVVirtState *s,
> > const struct MemmapEntry *memmap,
> > qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
> > qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
> > qemu_fdt_setprop_string(fdt, nodename, "device_type",
> > "cpu");
> > + qemu_fdt_setprop_cell(fdt, nodename, "phandle",
> > cpu_phandle);
> > + qemu_fdt_setprop_cell(fdt, nodename, "linux,phandle",
> > cpu_phandle);
> > + int intc_phandle = phandle++;
>
> Don't declare variables in the middle of code. The variable must be
> declared at the start of a block.
>
My bad. Fixed in v2.
> With that fixed:
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair
>
> > qemu_fdt_add_subnode(fdt, intc);
> > - qemu_fdt_setprop_cell(fdt, intc, "phandle", cpu_phandle);
> > - qemu_fdt_setprop_cell(fdt, intc, "linux,phandle",
> > cpu_phandle);
> > + qemu_fdt_setprop_cell(fdt, intc, "phandle", intc_phandle);
> > + qemu_fdt_setprop_cell(fdt, intc, "linux,phandle",
> > intc_phandle);
> > qemu_fdt_setprop_string(fdt, intc, "compatible",
> > "riscv,cpu-intc");
> > qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL,
> > 0);
> > qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
> > @@ -214,6 +217,20 @@ static void *create_fdt(RISCVVirtState *s,
> > const struct MemmapEntry *memmap,
> > g_free(nodename);
> > }
> >
> > + /* Add cpu-topology node */
> > + qemu_fdt_add_subnode(fdt, "/cpus/cpu-map");
> > + qemu_fdt_add_subnode(fdt, "/cpus/cpu-map/cluster0");
> > + for (cpu = s->soc.num_harts - 1; cpu >= 0; cpu--) {
> > + char *core_nodename = g_strdup_printf("/cpus/cpu-
> > map/cluster0/core%d",
> > + cpu);
> > + char *cpu_nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
> > + uint32_t intc_phandle = qemu_fdt_get_phandle(fdt,
> > cpu_nodename);
> > + qemu_fdt_add_subnode(fdt, core_nodename);
> > + qemu_fdt_setprop_cell(fdt, core_nodename, "cpu",
> > intc_phandle);
> > + g_free(core_nodename);
> > + g_free(cpu_nodename);
> > + }
> > +
> > cells = g_new0(uint32_t, s->soc.num_harts * 4);
> > for (cpu = 0; cpu < s->soc.num_harts; cpu++) {
> > nodename =
> > --
> > 2.21.0
> >
> >
--
Regards,
Atish
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH] riscv: virt: Add cpu-topology DT node.
@ 2019-06-24 23:42 ` Atish Patra
0 siblings, 0 replies; 12+ messages in thread
From: Atish Patra @ 2019-06-24 23:42 UTC (permalink / raw)
To: alistair23@gmail.com
Cc: qemu-riscv@nongnu.org, sagark@eecs.berkeley.edu,
kbastian@mail.uni-paderborn.de, palmer@sifive.com,
qemu-devel@nongnu.org, Alistair Francis
On Mon, 2019-06-24 at 16:24 -0700, Alistair Francis wrote:
> On Mon, Jun 24, 2019 at 3:57 PM Atish Patra <atish.patra@wdc.com>
> wrote:
> > Currently, there is no cpu topology defined in RISC-V.
> > Define a device tree node that clearly describes the
> > entire topology. This saves the trouble of scanning individual
> > cache to figure out the topology.
> >
> > Here is the linux kernel patch series that enables topology
> > for RISC-V.
> >
> > http://lists.infradead.org/pipermail/linux-riscv/2019-June/005072.html
> >
> > CPU topology after applying this patch in QEMU & above series in
> > kernel
> >
> > / # cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
> > 2
> > / # cat /sys/devices/system/cpu/cpu2/topology/physical_package_id
> > 0
> > / # cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
> > 0-7
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> > hw/riscv/virt.c | 21 +++++++++++++++++++--
> > 1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 84d94d0c42d8..da0b8aa18747 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -203,9 +203,12 @@ static void *create_fdt(RISCVVirtState *s,
> > const struct MemmapEntry *memmap,
> > qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
> > qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
> > qemu_fdt_setprop_string(fdt, nodename, "device_type",
> > "cpu");
> > + qemu_fdt_setprop_cell(fdt, nodename, "phandle",
> > cpu_phandle);
> > + qemu_fdt_setprop_cell(fdt, nodename, "linux,phandle",
> > cpu_phandle);
> > + int intc_phandle = phandle++;
>
> Don't declare variables in the middle of code. The variable must be
> declared at the start of a block.
>
My bad. Fixed in v2.
> With that fixed:
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair
>
> > qemu_fdt_add_subnode(fdt, intc);
> > - qemu_fdt_setprop_cell(fdt, intc, "phandle", cpu_phandle);
> > - qemu_fdt_setprop_cell(fdt, intc, "linux,phandle",
> > cpu_phandle);
> > + qemu_fdt_setprop_cell(fdt, intc, "phandle", intc_phandle);
> > + qemu_fdt_setprop_cell(fdt, intc, "linux,phandle",
> > intc_phandle);
> > qemu_fdt_setprop_string(fdt, intc, "compatible",
> > "riscv,cpu-intc");
> > qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL,
> > 0);
> > qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
> > @@ -214,6 +217,20 @@ static void *create_fdt(RISCVVirtState *s,
> > const struct MemmapEntry *memmap,
> > g_free(nodename);
> > }
> >
> > + /* Add cpu-topology node */
> > + qemu_fdt_add_subnode(fdt, "/cpus/cpu-map");
> > + qemu_fdt_add_subnode(fdt, "/cpus/cpu-map/cluster0");
> > + for (cpu = s->soc.num_harts - 1; cpu >= 0; cpu--) {
> > + char *core_nodename = g_strdup_printf("/cpus/cpu-
> > map/cluster0/core%d",
> > + cpu);
> > + char *cpu_nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
> > + uint32_t intc_phandle = qemu_fdt_get_phandle(fdt,
> > cpu_nodename);
> > + qemu_fdt_add_subnode(fdt, core_nodename);
> > + qemu_fdt_setprop_cell(fdt, core_nodename, "cpu",
> > intc_phandle);
> > + g_free(core_nodename);
> > + g_free(cpu_nodename);
> > + }
> > +
> > cells = g_new0(uint32_t, s->soc.num_harts * 4);
> > for (cpu = 0; cpu < s->soc.num_harts; cpu++) {
> > nodename =
> > --
> > 2.21.0
> >
> >
--
Regards,
Atish
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: virt: Add cpu-topology DT node.
2019-06-24 23:24 ` Alistair Francis
@ 2019-06-25 10:36 ` Philippe Mathieu-Daudé
-1 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-25 10:36 UTC (permalink / raw)
To: Alistair Francis, Atish Patra
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, qemu-devel@nongnu.org Developers,
Alistair Francis, Thomas Huth, Richard Henderson,
Daniel P. Berrange
On 6/25/19 1:24 AM, Alistair Francis wrote:
> On Mon, Jun 24, 2019 at 3:57 PM Atish Patra <atish.patra@wdc.com> wrote:
>>
>> Currently, there is no cpu topology defined in RISC-V.
>> Define a device tree node that clearly describes the
>> entire topology. This saves the trouble of scanning individual
>> cache to figure out the topology.
>>
>> Here is the linux kernel patch series that enables topology
>> for RISC-V.
>>
>> http://lists.infradead.org/pipermail/linux-riscv/2019-June/005072.html
>>
>> CPU topology after applying this patch in QEMU & above series in kernel
>>
>> / # cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
>> 2
>> / # cat /sys/devices/system/cpu/cpu2/topology/physical_package_id
>> 0
>> / # cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
>> 0-7
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> ---
>> hw/riscv/virt.c | 21 +++++++++++++++++++--
>> 1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 84d94d0c42d8..da0b8aa18747 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -203,9 +203,12 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>> qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
>> qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
>> qemu_fdt_setprop_string(fdt, nodename, "device_type", "cpu");
>> + qemu_fdt_setprop_cell(fdt, nodename, "phandle", cpu_phandle);
>> + qemu_fdt_setprop_cell(fdt, nodename, "linux,phandle", cpu_phandle);
>> + int intc_phandle = phandle++;
>
> Don't declare variables in the middle of code. The variable must be
> declared at the start of a block.
I guess this has been relaxed since we allow GNU C99:
https://git.qemu.org/?p=qemu.git;a=commit;h=7be41675f7cb16be7c8d2554add7a63fa43781a8
>
> With that fixed:
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair
>
>> qemu_fdt_add_subnode(fdt, intc);
>> - qemu_fdt_setprop_cell(fdt, intc, "phandle", cpu_phandle);
>> - qemu_fdt_setprop_cell(fdt, intc, "linux,phandle", cpu_phandle);
>> + qemu_fdt_setprop_cell(fdt, intc, "phandle", intc_phandle);
>> + qemu_fdt_setprop_cell(fdt, intc, "linux,phandle", intc_phandle);
>> qemu_fdt_setprop_string(fdt, intc, "compatible", "riscv,cpu-intc");
>> qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL, 0);
>> qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
>> @@ -214,6 +217,20 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>> g_free(nodename);
>> }
>>
>> + /* Add cpu-topology node */
>> + qemu_fdt_add_subnode(fdt, "/cpus/cpu-map");
>> + qemu_fdt_add_subnode(fdt, "/cpus/cpu-map/cluster0");
>> + for (cpu = s->soc.num_harts - 1; cpu >= 0; cpu--) {
>> + char *core_nodename = g_strdup_printf("/cpus/cpu-map/cluster0/core%d",
>> + cpu);
>> + char *cpu_nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
>> + uint32_t intc_phandle = qemu_fdt_get_phandle(fdt, cpu_nodename);
>> + qemu_fdt_add_subnode(fdt, core_nodename);
>> + qemu_fdt_setprop_cell(fdt, core_nodename, "cpu", intc_phandle);
>> + g_free(core_nodename);
>> + g_free(cpu_nodename);
>> + }
>> +
>> cells = g_new0(uint32_t, s->soc.num_harts * 4);
>> for (cpu = 0; cpu < s->soc.num_harts; cpu++) {
>> nodename =
>> --
>> 2.21.0
>>
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH] riscv: virt: Add cpu-topology DT node.
@ 2019-06-25 10:36 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-25 10:36 UTC (permalink / raw)
To: Alistair Francis, Atish Patra
Cc: Thomas Huth, open list:RISC-V, Sagar Karandikar,
Bastian Koppelmann, Palmer Dabbelt, Richard Henderson,
qemu-devel@nongnu.org Developers, Alistair Francis,
Daniel P. Berrange
On 6/25/19 1:24 AM, Alistair Francis wrote:
> On Mon, Jun 24, 2019 at 3:57 PM Atish Patra <atish.patra@wdc.com> wrote:
>>
>> Currently, there is no cpu topology defined in RISC-V.
>> Define a device tree node that clearly describes the
>> entire topology. This saves the trouble of scanning individual
>> cache to figure out the topology.
>>
>> Here is the linux kernel patch series that enables topology
>> for RISC-V.
>>
>> http://lists.infradead.org/pipermail/linux-riscv/2019-June/005072.html
>>
>> CPU topology after applying this patch in QEMU & above series in kernel
>>
>> / # cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
>> 2
>> / # cat /sys/devices/system/cpu/cpu2/topology/physical_package_id
>> 0
>> / # cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
>> 0-7
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> ---
>> hw/riscv/virt.c | 21 +++++++++++++++++++--
>> 1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 84d94d0c42d8..da0b8aa18747 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -203,9 +203,12 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>> qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
>> qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
>> qemu_fdt_setprop_string(fdt, nodename, "device_type", "cpu");
>> + qemu_fdt_setprop_cell(fdt, nodename, "phandle", cpu_phandle);
>> + qemu_fdt_setprop_cell(fdt, nodename, "linux,phandle", cpu_phandle);
>> + int intc_phandle = phandle++;
>
> Don't declare variables in the middle of code. The variable must be
> declared at the start of a block.
I guess this has been relaxed since we allow GNU C99:
https://git.qemu.org/?p=qemu.git;a=commit;h=7be41675f7cb16be7c8d2554add7a63fa43781a8
>
> With that fixed:
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair
>
>> qemu_fdt_add_subnode(fdt, intc);
>> - qemu_fdt_setprop_cell(fdt, intc, "phandle", cpu_phandle);
>> - qemu_fdt_setprop_cell(fdt, intc, "linux,phandle", cpu_phandle);
>> + qemu_fdt_setprop_cell(fdt, intc, "phandle", intc_phandle);
>> + qemu_fdt_setprop_cell(fdt, intc, "linux,phandle", intc_phandle);
>> qemu_fdt_setprop_string(fdt, intc, "compatible", "riscv,cpu-intc");
>> qemu_fdt_setprop(fdt, intc, "interrupt-controller", NULL, 0);
>> qemu_fdt_setprop_cell(fdt, intc, "#interrupt-cells", 1);
>> @@ -214,6 +217,20 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>> g_free(nodename);
>> }
>>
>> + /* Add cpu-topology node */
>> + qemu_fdt_add_subnode(fdt, "/cpus/cpu-map");
>> + qemu_fdt_add_subnode(fdt, "/cpus/cpu-map/cluster0");
>> + for (cpu = s->soc.num_harts - 1; cpu >= 0; cpu--) {
>> + char *core_nodename = g_strdup_printf("/cpus/cpu-map/cluster0/core%d",
>> + cpu);
>> + char *cpu_nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
>> + uint32_t intc_phandle = qemu_fdt_get_phandle(fdt, cpu_nodename);
>> + qemu_fdt_add_subnode(fdt, core_nodename);
>> + qemu_fdt_setprop_cell(fdt, core_nodename, "cpu", intc_phandle);
>> + g_free(core_nodename);
>> + g_free(cpu_nodename);
>> + }
>> +
>> cells = g_new0(uint32_t, s->soc.num_harts * 4);
>> for (cpu = 0; cpu < s->soc.num_harts; cpu++) {
>> nodename =
>> --
>> 2.21.0
>>
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: virt: Add cpu-topology DT node.
2019-06-25 10:36 ` Philippe Mathieu-Daudé
@ 2019-06-25 10:41 ` Daniel P. Berrangé
-1 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2019-06-25 10:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Atish Patra, open list:RISC-V, Sagar Karandikar,
Bastian Koppelmann, Palmer Dabbelt,
qemu-devel@nongnu.org Developers, Alistair Francis, Thomas Huth,
Richard Henderson
On Tue, Jun 25, 2019 at 12:36:35PM +0200, Philippe Mathieu-Daudé wrote:
> On 6/25/19 1:24 AM, Alistair Francis wrote:
> > On Mon, Jun 24, 2019 at 3:57 PM Atish Patra <atish.patra@wdc.com> wrote:
> >>
> >> Currently, there is no cpu topology defined in RISC-V.
> >> Define a device tree node that clearly describes the
> >> entire topology. This saves the trouble of scanning individual
> >> cache to figure out the topology.
> >>
> >> Here is the linux kernel patch series that enables topology
> >> for RISC-V.
> >>
> >> http://lists.infradead.org/pipermail/linux-riscv/2019-June/005072.html
> >>
> >> CPU topology after applying this patch in QEMU & above series in kernel
> >>
> >> / # cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
> >> 2
> >> / # cat /sys/devices/system/cpu/cpu2/topology/physical_package_id
> >> 0
> >> / # cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
> >> 0-7
> >>
> >> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> >> ---
> >> hw/riscv/virt.c | 21 +++++++++++++++++++--
> >> 1 file changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >> index 84d94d0c42d8..da0b8aa18747 100644
> >> --- a/hw/riscv/virt.c
> >> +++ b/hw/riscv/virt.c
> >> @@ -203,9 +203,12 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
> >> qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
> >> qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
> >> qemu_fdt_setprop_string(fdt, nodename, "device_type", "cpu");
> >> + qemu_fdt_setprop_cell(fdt, nodename, "phandle", cpu_phandle);
> >> + qemu_fdt_setprop_cell(fdt, nodename, "linux,phandle", cpu_phandle);
> >> + int intc_phandle = phandle++;
> >
> > Don't declare variables in the middle of code. The variable must be
> > declared at the start of a block.
>
> I guess this has been relaxed since we allow GNU C99:
Even though we allow GNU C99 I think it is undesirable to declare variables
in the middle of methods. This is especially true when combined with "goto"
as you end up with undefined / uninitialized vairable contents at the jump
target, if we've jumped over the variable declaration.
We can't enforce location of variable declarations, but I'd really
recommend we keep them all at the start of code blocks.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] riscv: virt: Add cpu-topology DT node.
@ 2019-06-25 10:41 ` Daniel P. Berrangé
0 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2019-06-25 10:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Thomas Huth, open list:RISC-V, Sagar Karandikar,
Bastian Koppelmann, Palmer Dabbelt, Richard Henderson,
qemu-devel@nongnu.org Developers, Atish Patra, Alistair Francis,
Alistair Francis
On Tue, Jun 25, 2019 at 12:36:35PM +0200, Philippe Mathieu-Daudé wrote:
> On 6/25/19 1:24 AM, Alistair Francis wrote:
> > On Mon, Jun 24, 2019 at 3:57 PM Atish Patra <atish.patra@wdc.com> wrote:
> >>
> >> Currently, there is no cpu topology defined in RISC-V.
> >> Define a device tree node that clearly describes the
> >> entire topology. This saves the trouble of scanning individual
> >> cache to figure out the topology.
> >>
> >> Here is the linux kernel patch series that enables topology
> >> for RISC-V.
> >>
> >> http://lists.infradead.org/pipermail/linux-riscv/2019-June/005072.html
> >>
> >> CPU topology after applying this patch in QEMU & above series in kernel
> >>
> >> / # cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
> >> 2
> >> / # cat /sys/devices/system/cpu/cpu2/topology/physical_package_id
> >> 0
> >> / # cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
> >> 0-7
> >>
> >> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> >> ---
> >> hw/riscv/virt.c | 21 +++++++++++++++++++--
> >> 1 file changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >> index 84d94d0c42d8..da0b8aa18747 100644
> >> --- a/hw/riscv/virt.c
> >> +++ b/hw/riscv/virt.c
> >> @@ -203,9 +203,12 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
> >> qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
> >> qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
> >> qemu_fdt_setprop_string(fdt, nodename, "device_type", "cpu");
> >> + qemu_fdt_setprop_cell(fdt, nodename, "phandle", cpu_phandle);
> >> + qemu_fdt_setprop_cell(fdt, nodename, "linux,phandle", cpu_phandle);
> >> + int intc_phandle = phandle++;
> >
> > Don't declare variables in the middle of code. The variable must be
> > declared at the start of a block.
>
> I guess this has been relaxed since we allow GNU C99:
Even though we allow GNU C99 I think it is undesirable to declare variables
in the middle of methods. This is especially true when combined with "goto"
as you end up with undefined / uninitialized vairable contents at the jump
target, if we've jumped over the variable declaration.
We can't enforce location of variable declarations, but I'd really
recommend we keep them all at the start of code blocks.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: virt: Add cpu-topology DT node.
2019-06-25 10:41 ` Daniel P. Berrangé
@ 2019-06-25 11:27 ` Philippe Mathieu-Daudé
-1 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-25 11:27 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Alistair Francis, Atish Patra, open list:RISC-V, Sagar Karandikar,
Bastian Koppelmann, Palmer Dabbelt,
qemu-devel@nongnu.org Developers, Alistair Francis, Thomas Huth,
Richard Henderson
On 6/25/19 12:41 PM, Daniel P. Berrangé wrote:
> On Tue, Jun 25, 2019 at 12:36:35PM +0200, Philippe Mathieu-Daudé wrote:
>> On 6/25/19 1:24 AM, Alistair Francis wrote:
>>> On Mon, Jun 24, 2019 at 3:57 PM Atish Patra <atish.patra@wdc.com> wrote:
>>>>
>>>> Currently, there is no cpu topology defined in RISC-V.
>>>> Define a device tree node that clearly describes the
>>>> entire topology. This saves the trouble of scanning individual
>>>> cache to figure out the topology.
>>>>
>>>> Here is the linux kernel patch series that enables topology
>>>> for RISC-V.
>>>>
>>>> http://lists.infradead.org/pipermail/linux-riscv/2019-June/005072.html
>>>>
>>>> CPU topology after applying this patch in QEMU & above series in kernel
>>>>
>>>> / # cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
>>>> 2
>>>> / # cat /sys/devices/system/cpu/cpu2/topology/physical_package_id
>>>> 0
>>>> / # cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
>>>> 0-7
>>>>
>>>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>>>> ---
>>>> hw/riscv/virt.c | 21 +++++++++++++++++++--
>>>> 1 file changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>>> index 84d94d0c42d8..da0b8aa18747 100644
>>>> --- a/hw/riscv/virt.c
>>>> +++ b/hw/riscv/virt.c
>>>> @@ -203,9 +203,12 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>>>> qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
>>>> qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
>>>> qemu_fdt_setprop_string(fdt, nodename, "device_type", "cpu");
>>>> + qemu_fdt_setprop_cell(fdt, nodename, "phandle", cpu_phandle);
>>>> + qemu_fdt_setprop_cell(fdt, nodename, "linux,phandle", cpu_phandle);
>>>> + int intc_phandle = phandle++;
>>>
>>> Don't declare variables in the middle of code. The variable must be
>>> declared at the start of a block.
>>
>> I guess this has been relaxed since we allow GNU C99:
>
> Even though we allow GNU C99 I think it is undesirable to declare variables
> in the middle of methods. This is especially true when combined with "goto"
> as you end up with undefined / uninitialized vairable contents at the jump
> target, if we've jumped over the variable declaration.
>
> We can't enforce location of variable declarations, but I'd really
> recommend we keep them all at the start of code blocks.
In this case I find it desirable:
for (int i = 0; ...) {
...
}
For the rest, I agree to keep them at the start of code block.
Regards,
Phil.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH] riscv: virt: Add cpu-topology DT node.
@ 2019-06-25 11:27 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-25 11:27 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Thomas Huth, open list:RISC-V, Sagar Karandikar,
Bastian Koppelmann, Palmer Dabbelt, Richard Henderson,
qemu-devel@nongnu.org Developers, Atish Patra, Alistair Francis,
Alistair Francis
On 6/25/19 12:41 PM, Daniel P. Berrangé wrote:
> On Tue, Jun 25, 2019 at 12:36:35PM +0200, Philippe Mathieu-Daudé wrote:
>> On 6/25/19 1:24 AM, Alistair Francis wrote:
>>> On Mon, Jun 24, 2019 at 3:57 PM Atish Patra <atish.patra@wdc.com> wrote:
>>>>
>>>> Currently, there is no cpu topology defined in RISC-V.
>>>> Define a device tree node that clearly describes the
>>>> entire topology. This saves the trouble of scanning individual
>>>> cache to figure out the topology.
>>>>
>>>> Here is the linux kernel patch series that enables topology
>>>> for RISC-V.
>>>>
>>>> http://lists.infradead.org/pipermail/linux-riscv/2019-June/005072.html
>>>>
>>>> CPU topology after applying this patch in QEMU & above series in kernel
>>>>
>>>> / # cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
>>>> 2
>>>> / # cat /sys/devices/system/cpu/cpu2/topology/physical_package_id
>>>> 0
>>>> / # cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
>>>> 0-7
>>>>
>>>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>>>> ---
>>>> hw/riscv/virt.c | 21 +++++++++++++++++++--
>>>> 1 file changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>>> index 84d94d0c42d8..da0b8aa18747 100644
>>>> --- a/hw/riscv/virt.c
>>>> +++ b/hw/riscv/virt.c
>>>> @@ -203,9 +203,12 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>>>> qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
>>>> qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
>>>> qemu_fdt_setprop_string(fdt, nodename, "device_type", "cpu");
>>>> + qemu_fdt_setprop_cell(fdt, nodename, "phandle", cpu_phandle);
>>>> + qemu_fdt_setprop_cell(fdt, nodename, "linux,phandle", cpu_phandle);
>>>> + int intc_phandle = phandle++;
>>>
>>> Don't declare variables in the middle of code. The variable must be
>>> declared at the start of a block.
>>
>> I guess this has been relaxed since we allow GNU C99:
>
> Even though we allow GNU C99 I think it is undesirable to declare variables
> in the middle of methods. This is especially true when combined with "goto"
> as you end up with undefined / uninitialized vairable contents at the jump
> target, if we've jumped over the variable declaration.
>
> We can't enforce location of variable declarations, but I'd really
> recommend we keep them all at the start of code blocks.
In this case I find it desirable:
for (int i = 0; ...) {
...
}
For the rest, I agree to keep them at the start of code block.
Regards,
Phil.
^ permalink raw reply [flat|nested] 12+ messages in thread