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

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

[Tyler Ng]

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

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

[Andrew Jones]

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
> 

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

[Philippe Mathieu-Daudé via]

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()?

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

[Tyler Ng]

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

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

[Jim Shu]

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

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

[Tyler Ng]

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

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