Re: [PATCH 2/4] hw/intc: sifive_plic.c: Fix interrupt priority index.

[Tyler Ng <tkng@rivosinc.com>]

[-- 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 --]