* [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index. @ 2022-09-01 22:50 Tyler Ng 2022-09-05 13:15 ` Andrew Jones 2022-09-05 15:09 ` Philippe Mathieu-Daudé via 0 siblings, 2 replies; 7+ messages in thread From: Tyler Ng @ 2022-09-01 22:50 UTC (permalink / raw) To: qemu-riscv, qemu-devel Cc: Alistair.Francis, bin.meng, palmer, thuth, pbonzini, lvivier Fixes a bug in which the index of the interrupt priority is off by 1. For example, using an IRQ number of 3 with a priority of 1 is supposed to set plic->source_priority[2] = 1, but instead it sets plic->source_priority[3] = 1. When an interrupt is claimed to be serviced, it checks the index 2 instead of 3. Signed-off-by: Tyler Ng <tkng@rivosinc.com> --- hw/intc/sifive_plic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c index af4ae3630e..e75c47300a 100644 --- a/hw/intc/sifive_plic.c +++ b/hw/intc/sifive_plic.c @@ -178,7 +178,7 @@ static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value, SiFivePLICState *plic = opaque; if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) { - uint32_t irq = ((addr - plic->priority_base) >> 2) + 1; + uint32_t irq = ((addr - plic->priority_base) >> 2) + 0; plic->source_priority[irq] = value & 7; sifive_plic_update(plic); -- 2.30.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index. 2022-09-01 22:50 [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index Tyler Ng @ 2022-09-05 13:15 ` Andrew Jones 2022-09-06 15:13 ` Tyler Ng 2022-09-05 15:09 ` Philippe Mathieu-Daudé via 1 sibling, 1 reply; 7+ messages in thread From: Andrew Jones @ 2022-09-05 13:15 UTC (permalink / raw) To: Tyler Ng Cc: qemu-riscv, qemu-devel, Alistair.Francis, bin.meng, palmer, thuth, pbonzini, lvivier On Thu, Sep 01, 2022 at 03:50:06PM -0700, Tyler Ng wrote: > Fixes a bug in which the index of the interrupt priority is off by 1. > For example, using an IRQ number of 3 with a priority of 1 is supposed to set > plic->source_priority[2] = 1, but instead it sets > plic->source_priority[3] = 1. When an interrupt is claimed to be > serviced, it checks the index 2 instead of 3. > > Signed-off-by: Tyler Ng <tkng@rivosinc.com> Fixes tag? Thanks, drew > --- > hw/intc/sifive_plic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c > index af4ae3630e..e75c47300a 100644 > --- a/hw/intc/sifive_plic.c > +++ b/hw/intc/sifive_plic.c > @@ -178,7 +178,7 @@ static void sifive_plic_write(void *opaque, hwaddr > addr, uint64_t value, > SiFivePLICState *plic = opaque; > > if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) { > - uint32_t irq = ((addr - plic->priority_base) >> 2) + 1; > + uint32_t irq = ((addr - plic->priority_base) >> 2) + 0; > > plic->source_priority[irq] = value & 7; > sifive_plic_update(plic); > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index. 2022-09-05 13:15 ` Andrew Jones @ 2022-09-06 15:13 ` Tyler Ng 2022-09-25 13:47 ` Jim Shu 0 siblings, 1 reply; 7+ messages in thread From: Tyler Ng @ 2022-09-06 15:13 UTC (permalink / raw) To: Andrew Jones Cc: qemu-riscv, qemu-devel, Alistair.Francis, bin.meng, palmer, thuth, pbonzini, lvivier [-- Attachment #1: Type: text/plain, Size: 1406 bytes --] Here's the patch SHA that introduced the offset: 0feb4a7129eb4f120c75849ddc9e50495c50cb63 -Tyler On Mon, Sep 5, 2022 at 6:15 AM Andrew Jones <ajones@ventanamicro.com> wrote: > On Thu, Sep 01, 2022 at 03:50:06PM -0700, Tyler Ng wrote: > > Fixes a bug in which the index of the interrupt priority is off by 1. > > For example, using an IRQ number of 3 with a priority of 1 is supposed > to set > > plic->source_priority[2] = 1, but instead it sets > > plic->source_priority[3] = 1. When an interrupt is claimed to be > > serviced, it checks the index 2 instead of 3. > > > > Signed-off-by: Tyler Ng <tkng@rivosinc.com> > > Fixes tag? > > Thanks, > drew > > > --- > > hw/intc/sifive_plic.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c > > index af4ae3630e..e75c47300a 100644 > > --- a/hw/intc/sifive_plic.c > > +++ b/hw/intc/sifive_plic.c > > @@ -178,7 +178,7 @@ static void sifive_plic_write(void *opaque, hwaddr > > addr, uint64_t value, > > SiFivePLICState *plic = opaque; > > > > if (addr_between(addr, plic->priority_base, plic->num_sources << > 2)) { > > - uint32_t irq = ((addr - plic->priority_base) >> 2) + 1; > > + uint32_t irq = ((addr - plic->priority_base) >> 2) + 0; > > > > plic->source_priority[irq] = value & 7; > > sifive_plic_update(plic); > > -- > > 2.30.2 > > > [-- Attachment #2: Type: text/html, Size: 2039 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index. 2022-09-06 15:13 ` Tyler Ng @ 2022-09-25 13:47 ` Jim Shu 2022-09-26 20:03 ` Tyler Ng 0 siblings, 1 reply; 7+ messages in thread From: Jim Shu @ 2022-09-25 13:47 UTC (permalink / raw) To: Tyler Ng Cc: Andrew Jones, qemu-riscv, qemu-devel, Alistair.Francis, bin.meng, palmer, thuth, pbonzini, lvivier Hi Tyler, This fix is incorrect. In PLIC spec, Interrupt Source Priority Memory Map is 0x000000: Reserved (interrupt source 0 does not exist) 0x000004: Interrupt source 1 priority 0x000008: Interrupt source 2 priority Current RISC-V machines (virt, sifive_u) use 0x4 as priority_base, so current formula "irq = ((addr - plic->priority_base) >> 2) + 1" will take offset 0x4 as IRQ source 1, which is correct. Your fix will cause the bug in existing machines. Thanks, Jim Shu On Tue, Sep 6, 2022 at 11:21 PM Tyler Ng <tkng@rivosinc.com> wrote: > > Here's the patch SHA that introduced the offset: 0feb4a7129eb4f120c75849ddc9e50495c50cb63 > > -Tyler > > On Mon, Sep 5, 2022 at 6:15 AM Andrew Jones <ajones@ventanamicro.com> wrote: >> >> On Thu, Sep 01, 2022 at 03:50:06PM -0700, Tyler Ng wrote: >> > Fixes a bug in which the index of the interrupt priority is off by 1. >> > For example, using an IRQ number of 3 with a priority of 1 is supposed to set >> > plic->source_priority[2] = 1, but instead it sets >> > plic->source_priority[3] = 1. When an interrupt is claimed to be >> > serviced, it checks the index 2 instead of 3. >> > >> > Signed-off-by: Tyler Ng <tkng@rivosinc.com> >> >> Fixes tag? >> >> Thanks, >> drew >> >> > --- >> > hw/intc/sifive_plic.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c >> > index af4ae3630e..e75c47300a 100644 >> > --- a/hw/intc/sifive_plic.c >> > +++ b/hw/intc/sifive_plic.c >> > @@ -178,7 +178,7 @@ static void sifive_plic_write(void *opaque, hwaddr >> > addr, uint64_t value, >> > SiFivePLICState *plic = opaque; >> > >> > if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) { >> > - uint32_t irq = ((addr - plic->priority_base) >> 2) + 1; >> > + uint32_t irq = ((addr - plic->priority_base) >> 2) + 0; >> > >> > plic->source_priority[irq] = value & 7; >> > sifive_plic_update(plic); >> > -- >> > 2.30.2 >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index. 2022-09-25 13:47 ` Jim Shu @ 2022-09-26 20:03 ` Tyler Ng 2022-09-27 3:34 ` Jim Shu 0 siblings, 1 reply; 7+ messages in thread From: Tyler Ng @ 2022-09-26 20:03 UTC (permalink / raw) To: Jim Shu Cc: Andrew Jones, open list:RISC-V, qemu-devel@nongnu.org Developers, Alistair Francis, Bin Meng, Palmer Dabbelt, Thomas Huth, Paolo Bonzini, Laurent Vivier [-- Attachment #1: Type: text/plain, Size: 2493 bytes --] Hi Jim, Thanks for raising this comment. I think I understand where the confusion happens and it's because in the OpenTitan machine (which uses the sifive plic), it uses 0x00 as the priority base by default, which was the source of the problems. I'll drop this commit in the next version. -Tyler On Sun, Sep 25, 2022 at 6:47 AM Jim Shu <jim.shu@sifive.com> wrote: > Hi Tyler, > > This fix is incorrect. > > In PLIC spec, Interrupt Source Priority Memory Map is > 0x000000: Reserved (interrupt source 0 does not exist) > 0x000004: Interrupt source 1 priority > 0x000008: Interrupt source 2 priority > > Current RISC-V machines (virt, sifive_u) use 0x4 as priority_base, so > current formula "irq = ((addr - plic->priority_base) >> 2) + 1" will > take offset 0x4 as IRQ source 1, which is correct. > Your fix will cause the bug in existing machines. > > Thanks, > Jim Shu > > > > > On Tue, Sep 6, 2022 at 11:21 PM Tyler Ng <tkng@rivosinc.com> wrote: > > > > Here's the patch SHA that introduced the offset: > 0feb4a7129eb4f120c75849ddc9e50495c50cb63 > > > > -Tyler > > > > On Mon, Sep 5, 2022 at 6:15 AM Andrew Jones <ajones@ventanamicro.com> > wrote: > >> > >> On Thu, Sep 01, 2022 at 03:50:06PM -0700, Tyler Ng wrote: > >> > Fixes a bug in which the index of the interrupt priority is off by 1. > >> > For example, using an IRQ number of 3 with a priority of 1 is > supposed to set > >> > plic->source_priority[2] = 1, but instead it sets > >> > plic->source_priority[3] = 1. When an interrupt is claimed to be > >> > serviced, it checks the index 2 instead of 3. > >> > > >> > Signed-off-by: Tyler Ng <tkng@rivosinc.com> > >> > >> Fixes tag? > >> > >> Thanks, > >> drew > >> > >> > --- > >> > hw/intc/sifive_plic.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c > >> > index af4ae3630e..e75c47300a 100644 > >> > --- a/hw/intc/sifive_plic.c > >> > +++ b/hw/intc/sifive_plic.c > >> > @@ -178,7 +178,7 @@ static void sifive_plic_write(void *opaque, hwaddr > >> > addr, uint64_t value, > >> > SiFivePLICState *plic = opaque; > >> > > >> > if (addr_between(addr, plic->priority_base, plic->num_sources << > 2)) { > >> > - uint32_t irq = ((addr - plic->priority_base) >> 2) + 1; > >> > + uint32_t irq = ((addr - plic->priority_base) >> 2) + 0; > >> > > >> > plic->source_priority[irq] = value & 7; > >> > sifive_plic_update(plic); > >> > -- > >> > 2.30.2 > >> > > [-- Attachment #2: Type: text/html, Size: 3659 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index. 2022-09-26 20:03 ` Tyler Ng @ 2022-09-27 3:34 ` Jim Shu 0 siblings, 0 replies; 7+ messages in thread From: Jim Shu @ 2022-09-27 3:34 UTC (permalink / raw) To: Tyler Ng Cc: Andrew Jones, open list:RISC-V, qemu-devel@nongnu.org Developers, Alistair Francis, Bin Meng, Palmer Dabbelt, Thomas Huth, Paolo Bonzini, Laurent Vivier Hi Tyler, Thanks for the explanation. I understand the issue here. I think we should align the priority base in each RISC-V platform to the same value (no matter 0x0 or 0x4) if they use PLIC in the same way. Thanks, Jim Shu On Tue, Sep 27, 2022 at 4:04 AM Tyler Ng <tkng@rivosinc.com> wrote: > > Hi Jim, > > Thanks for raising this comment. I think I understand where the confusion happens and it's because in the OpenTitan machine (which uses the sifive plic), it uses 0x00 as the priority base by default, which was the source of the problems. I'll drop this commit in the next version. > > -Tyler > > On Sun, Sep 25, 2022 at 6:47 AM Jim Shu <jim.shu@sifive.com> wrote: >> >> Hi Tyler, >> >> This fix is incorrect. >> >> In PLIC spec, Interrupt Source Priority Memory Map is >> 0x000000: Reserved (interrupt source 0 does not exist) >> 0x000004: Interrupt source 1 priority >> 0x000008: Interrupt source 2 priority >> >> Current RISC-V machines (virt, sifive_u) use 0x4 as priority_base, so >> current formula "irq = ((addr - plic->priority_base) >> 2) + 1" will >> take offset 0x4 as IRQ source 1, which is correct. >> Your fix will cause the bug in existing machines. >> >> Thanks, >> Jim Shu >> >> >> >> >> On Tue, Sep 6, 2022 at 11:21 PM Tyler Ng <tkng@rivosinc.com> wrote: >> > >> > Here's the patch SHA that introduced the offset: 0feb4a7129eb4f120c75849ddc9e50495c50cb63 >> > >> > -Tyler >> > >> > On Mon, Sep 5, 2022 at 6:15 AM Andrew Jones <ajones@ventanamicro.com> wrote: >> >> >> >> On Thu, Sep 01, 2022 at 03:50:06PM -0700, Tyler Ng wrote: >> >> > Fixes a bug in which the index of the interrupt priority is off by 1. >> >> > For example, using an IRQ number of 3 with a priority of 1 is supposed to set >> >> > plic->source_priority[2] = 1, but instead it sets >> >> > plic->source_priority[3] = 1. When an interrupt is claimed to be >> >> > serviced, it checks the index 2 instead of 3. >> >> > >> >> > Signed-off-by: Tyler Ng <tkng@rivosinc.com> >> >> >> >> Fixes tag? >> >> >> >> Thanks, >> >> drew >> >> >> >> > --- >> >> > hw/intc/sifive_plic.c | 2 +- >> >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >> > >> >> > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c >> >> > index af4ae3630e..e75c47300a 100644 >> >> > --- a/hw/intc/sifive_plic.c >> >> > +++ b/hw/intc/sifive_plic.c >> >> > @@ -178,7 +178,7 @@ static void sifive_plic_write(void *opaque, hwaddr >> >> > addr, uint64_t value, >> >> > SiFivePLICState *plic = opaque; >> >> > >> >> > if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) { >> >> > - uint32_t irq = ((addr - plic->priority_base) >> 2) + 1; >> >> > + uint32_t irq = ((addr - plic->priority_base) >> 2) + 0; >> >> > >> >> > plic->source_priority[irq] = value & 7; >> >> > sifive_plic_update(plic); >> >> > -- >> >> > 2.30.2 >> >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index. 2022-09-01 22:50 [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index Tyler Ng 2022-09-05 13:15 ` Andrew Jones @ 2022-09-05 15:09 ` Philippe Mathieu-Daudé via 1 sibling, 0 replies; 7+ messages in thread From: Philippe Mathieu-Daudé via @ 2022-09-05 15:09 UTC (permalink / raw) To: Tyler Ng, qemu-riscv, qemu-devel Cc: Alistair.Francis, bin.meng, palmer, thuth, pbonzini, lvivier On 2/9/22 00:50, Tyler Ng wrote: > Fixes a bug in which the index of the interrupt priority is off by 1. > For example, using an IRQ number of 3 with a priority of 1 is supposed to set > plic->source_priority[2] = 1, but instead it sets > plic->source_priority[3] = 1. When an interrupt is claimed to be > serviced, it checks the index 2 instead of 3. > > Signed-off-by: Tyler Ng <tkng@rivosinc.com> > --- > hw/intc/sifive_plic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c > index af4ae3630e..e75c47300a 100644 > --- a/hw/intc/sifive_plic.c > +++ b/hw/intc/sifive_plic.c > @@ -178,7 +178,7 @@ static void sifive_plic_write(void *opaque, hwaddr > addr, uint64_t value, > SiFivePLICState *plic = opaque; > > if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) { > - uint32_t irq = ((addr - plic->priority_base) >> 2) + 1; > + uint32_t irq = ((addr - plic->priority_base) >> 2) + 0; > > plic->source_priority[irq] = value & 7; > sifive_plic_update(plic); > -- > 2.30.2 > What about sifive_plic_read()? ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-09-27 3:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-01 22:50 [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index Tyler Ng 2022-09-05 13:15 ` Andrew Jones 2022-09-06 15:13 ` Tyler Ng 2022-09-25 13:47 ` Jim Shu 2022-09-26 20:03 ` Tyler Ng 2022-09-27 3:34 ` Jim Shu 2022-09-05 15:09 ` Philippe Mathieu-Daudé via
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).