Message ID | CAB88-qPD2OAxeg4WA65utUmFj4Y=SceeFTuStpZS4pPWevdBXA@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | [1/4] hw/watchdog: wdt_ibex_aon.c: Implement the watchdog for the OpenTitan | expand |
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 >
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 > > >
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 >> >
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 > >> > >
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 >> >> >
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;
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(-) sifive_plic_update(plic); -- 2.30.2