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

[Jim Shu]

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