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

[Tyler Ng]

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