| Message ID | 20220723090335.671105-1-atishp@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/intc: sifive_plic: Fix multi-socket plic configuraiton | expand |
On Sat, Jul 23, 2022 at 7:22 PM Atish Patra <atishp@rivosinc.com> wrote: > > Since commit 40244040a7ac, multi-socket configuration with plic is > broken as the hartid for second socket is calculated incorrectly. > The hartid stored in addr_config already includes the offset > for the base hartid for that socket. Adding it again would lead > to segfault while creating the plic device for the virt machine. > qdev_connect_gpio_out was also invoked with incorrect number of gpio > lines. > > Fixes: 40244040a7ac (hw/intc: sifive_plic: Avoid overflowing the addr_config buffer) > > Signed-off-by: Atish Patra <atishp@rivosinc.com> Can you share the -cpu options that causes the segfault? I'll add it to my test case Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > hw/intc/sifive_plic.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c > index 56d60e9ac935..fdac028a521f 100644 > --- a/hw/intc/sifive_plic.c > +++ b/hw/intc/sifive_plic.c > @@ -454,10 +454,10 @@ DeviceState *sifive_plic_create(hwaddr addr, char *hart_config, > > for (i = 0; i < plic->num_addrs; i++) { > int cpu_num = plic->addr_config[i].hartid; > - CPUState *cpu = qemu_get_cpu(hartid_base + cpu_num); > + CPUState *cpu = qemu_get_cpu(cpu_num); > > if (plic->addr_config[i].mode == PLICMode_M) { > - qdev_connect_gpio_out(dev, num_harts + cpu_num, > + qdev_connect_gpio_out(dev, cpu_num, > qdev_get_gpio_in(DEVICE(cpu), IRQ_M_EXT)); > } > if (plic->addr_config[i].mode == PLICMode_S) { > -- > 2.25.1 > >
On Sun, Jul 24, 2022 at 6:14 PM Alistair Francis <alistair23@gmail.com> wrote: > On Sat, Jul 23, 2022 at 7:22 PM Atish Patra <atishp@rivosinc.com> wrote: > > > > Since commit 40244040a7ac, multi-socket configuration with plic is > > broken as the hartid for second socket is calculated incorrectly. > > The hartid stored in addr_config already includes the offset > > for the base hartid for that socket. Adding it again would lead > > to segfault while creating the plic device for the virt machine. > > qdev_connect_gpio_out was also invoked with incorrect number of gpio > > lines. > > > > Fixes: 40244040a7ac (hw/intc: sifive_plic: Avoid overflowing the > addr_config buffer) > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > Can you share the -cpu options that causes the segfault? I'll add it > to my test case > > "-cpu rv64 -M virt -m 2G -smp 4 -object memory-backend-ram,size=1G,policy=bind,host-nodes=0,id=ram-node0 \ -numa node,memdev=ram-node0 \ -object memory-backend-ram,size=1G,policy=bind,host-nodes=0,id=ram-node1 \ -numa node,memdev=ram-node1" You also need to enable CONFIG_NUMA in kernel. Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > Alistair > > > --- > > hw/intc/sifive_plic.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c > > index 56d60e9ac935..fdac028a521f 100644 > > --- a/hw/intc/sifive_plic.c > > +++ b/hw/intc/sifive_plic.c > > @@ -454,10 +454,10 @@ DeviceState *sifive_plic_create(hwaddr addr, char > *hart_config, > > > > for (i = 0; i < plic->num_addrs; i++) { > > int cpu_num = plic->addr_config[i].hartid; > > - CPUState *cpu = qemu_get_cpu(hartid_base + cpu_num); > > + CPUState *cpu = qemu_get_cpu(cpu_num); > > > > if (plic->addr_config[i].mode == PLICMode_M) { > > - qdev_connect_gpio_out(dev, num_harts + cpu_num, > > + qdev_connect_gpio_out(dev, cpu_num, > > qdev_get_gpio_in(DEVICE(cpu), > IRQ_M_EXT)); > > } > > if (plic->addr_config[i].mode == PLICMode_S) { > > -- > > 2.25.1 > > > > >
On Tue, Jul 26, 2022 at 8:30 AM Atish Kumar Patra <atishp@rivosinc.com> wrote: > > > > > On Sun, Jul 24, 2022 at 6:14 PM Alistair Francis <alistair23@gmail.com> wrote: >> >> On Sat, Jul 23, 2022 at 7:22 PM Atish Patra <atishp@rivosinc.com> wrote: >> > >> > Since commit 40244040a7ac, multi-socket configuration with plic is >> > broken as the hartid for second socket is calculated incorrectly. >> > The hartid stored in addr_config already includes the offset >> > for the base hartid for that socket. Adding it again would lead >> > to segfault while creating the plic device for the virt machine. >> > qdev_connect_gpio_out was also invoked with incorrect number of gpio >> > lines. >> > >> > Fixes: 40244040a7ac (hw/intc: sifive_plic: Avoid overflowing the addr_config buffer) >> > >> > Signed-off-by: Atish Patra <atishp@rivosinc.com> >> >> Can you share the -cpu options that causes the segfault? I'll add it >> to my test case >> > > "-cpu rv64 -M virt -m 2G -smp 4 -object memory-backend-ram,size=1G,policy=bind,host-nodes=0,id=ram-node0 \ > -numa node,memdev=ram-node0 \ > -object memory-backend-ram,size=1G,policy=bind,host-nodes=0,id=ram-node1 \ > -numa node,memdev=ram-node1" > > You also need to enable CONFIG_NUMA in kernel. Thanks! Applied to riscv-to-apply.next Alistair
On Sat, Jul 23, 2022 at 7:22 PM Atish Patra <atishp@rivosinc.com> wrote: > > Since commit 40244040a7ac, multi-socket configuration with plic is > broken as the hartid for second socket is calculated incorrectly. > The hartid stored in addr_config already includes the offset > for the base hartid for that socket. Adding it again would lead > to segfault while creating the plic device for the virt machine. > qdev_connect_gpio_out was also invoked with incorrect number of gpio > lines. > > Fixes: 40244040a7ac (hw/intc: sifive_plic: Avoid overflowing the addr_config buffer) > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > --- > hw/intc/sifive_plic.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c > index 56d60e9ac935..fdac028a521f 100644 > --- a/hw/intc/sifive_plic.c > +++ b/hw/intc/sifive_plic.c > @@ -454,10 +454,10 @@ DeviceState *sifive_plic_create(hwaddr addr, char *hart_config, > > for (i = 0; i < plic->num_addrs; i++) { > int cpu_num = plic->addr_config[i].hartid; > - CPUState *cpu = qemu_get_cpu(hartid_base + cpu_num); > + CPUState *cpu = qemu_get_cpu(cpu_num); > > if (plic->addr_config[i].mode == PLICMode_M) { > - qdev_connect_gpio_out(dev, num_harts + cpu_num, > + qdev_connect_gpio_out(dev, cpu_num, Argh! I was trying to get this ready to go into 7.1. I have been working on updating my tests to catch this failure in the future as well. While testing this change I noticed that it breaks the noMMU test case. I think the correct fix is actually this (on top of your patch): diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index fdac028a52..af4ae3630e 100644 --- a/hw/intc/sifive_plic.c +++ b/hw/intc/sifive_plic.c @@ -457,7 +457,7 @@ DeviceState *sifive_plic_create(hwaddr addr, char *hart_config, CPUState *cpu = qemu_get_cpu(cpu_num); if (plic->addr_config[i].mode == PLICMode_M) { - qdev_connect_gpio_out(dev, cpu_num, + qdev_connect_gpio_out(dev, num_harts - plic->hartid_base + cpu_num, qdev_get_gpio_in(DEVICE(cpu), IRQ_M_EXT)); } if (plic->addr_config[i].mode == PLICMode_S) { The idea is that we need to increment the second argument to qdev_connect_gpio_out() for the PLICMode_M compared to the PLICMode_S case. This ensures that we do that correctly without breaking anything. How does that look to you? Alistair > qdev_get_gpio_in(DEVICE(cpu), IRQ_M_EXT)); > } > if (plic->addr_config[i].mode == PLICMode_S) { > -- > 2.25.1 > >
On Wed, Jul 27, 2022 at 5:23 AM Alistair Francis <alistair23@gmail.com> wrote: > On Sat, Jul 23, 2022 at 7:22 PM Atish Patra <atishp@rivosinc.com> wrote: > > > > Since commit 40244040a7ac, multi-socket configuration with plic is > > broken as the hartid for second socket is calculated incorrectly. > > The hartid stored in addr_config already includes the offset > > for the base hartid for that socket. Adding it again would lead > > to segfault while creating the plic device for the virt machine. > > qdev_connect_gpio_out was also invoked with incorrect number of gpio > > lines. > > > > Fixes: 40244040a7ac (hw/intc: sifive_plic: Avoid overflowing the > addr_config buffer) > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > --- > > hw/intc/sifive_plic.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c > > index 56d60e9ac935..fdac028a521f 100644 > > --- a/hw/intc/sifive_plic.c > > +++ b/hw/intc/sifive_plic.c > > @@ -454,10 +454,10 @@ DeviceState *sifive_plic_create(hwaddr addr, char > *hart_config, > > > > for (i = 0; i < plic->num_addrs; i++) { > > int cpu_num = plic->addr_config[i].hartid; > > - CPUState *cpu = qemu_get_cpu(hartid_base + cpu_num); > > + CPUState *cpu = qemu_get_cpu(cpu_num); > > > > if (plic->addr_config[i].mode == PLICMode_M) { > > - qdev_connect_gpio_out(dev, num_harts + cpu_num, > > + qdev_connect_gpio_out(dev, cpu_num, > > Argh! > > I was trying to get this ready to go into 7.1. I have been working on > updating my tests to catch this failure in the future as well. > > While testing this change I noticed that it breaks the noMMU test case. > > I think the correct fix is actually this (on top of your patch): > > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c > index fdac028a52..af4ae3630e 100644 > --- a/hw/intc/sifive_plic.c > +++ b/hw/intc/sifive_plic.c > @@ -457,7 +457,7 @@ DeviceState *sifive_plic_create(hwaddr addr, char > *hart_config, > CPUState *cpu = qemu_get_cpu(cpu_num); > > if (plic->addr_config[i].mode == PLICMode_M) { > - qdev_connect_gpio_out(dev, cpu_num, > + qdev_connect_gpio_out(dev, num_harts - plic->hartid_base + > cpu_num, > qdev_get_gpio_in(DEVICE(cpu), > IRQ_M_EXT)); > } > if (plic->addr_config[i].mode == PLICMode_S) { > > The idea is that we need to increment the second argument to > qdev_connect_gpio_out() for the PLICMode_M compared to the PLICMode_S > case. > > This ensures that we do that correctly without breaking anything. > > How does that look to you? > > Ahh yes. That makes sense. Tested the updated change on multi-socket as well. > Alistair > > > qdev_get_gpio_in(DEVICE(cpu), > IRQ_M_EXT)); > > } > > if (plic->addr_config[i].mode == PLICMode_S) { > > -- > > 2.25.1 > > > > >
diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index 56d60e9ac935..fdac028a521f 100644 --- a/hw/intc/sifive_plic.c +++ b/hw/intc/sifive_plic.c @@ -454,10 +454,10 @@ DeviceState *sifive_plic_create(hwaddr addr, char *hart_config, for (i = 0; i < plic->num_addrs; i++) { int cpu_num = plic->addr_config[i].hartid; - CPUState *cpu = qemu_get_cpu(hartid_base + cpu_num); + CPUState *cpu = qemu_get_cpu(cpu_num); if (plic->addr_config[i].mode == PLICMode_M) { - qdev_connect_gpio_out(dev, num_harts + cpu_num, + qdev_connect_gpio_out(dev, cpu_num, qdev_get_gpio_in(DEVICE(cpu), IRQ_M_EXT)); } if (plic->addr_config[i].mode == PLICMode_S) {
Since commit 40244040a7ac, multi-socket configuration with plic is broken as the hartid for second socket is calculated incorrectly. The hartid stored in addr_config already includes the offset for the base hartid for that socket. Adding it again would lead to segfault while creating the plic device for the virt machine. qdev_connect_gpio_out was also invoked with incorrect number of gpio lines. Fixes: 40244040a7ac (hw/intc: sifive_plic: Avoid overflowing the addr_config buffer) Signed-off-by: Atish Patra <atishp@rivosinc.com> --- hw/intc/sifive_plic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)