Re: [Qemu-riscv] [PATCH for 4.0 v1 1/5] riscv: plic: Fix incorrect irq calculation

[Alistair Francis]

On Wed, Mar 27, 2019 at 3:29 AM Palmer Dabbelt <address@hidden> wrote:
>
> On Wed, 20 Mar 2019 17:46:09 PDT (-0700), Alistair Francis wrote:
> > The irq is incorrectly calculated to be off by one. It has worked in the
> > past as the priority_base offset has also been set incorrectly. We are
> > about to fix the priority_base offset so first first the irq
> > calculation.
> >
> > Signed-off-by: Alistair Francis <address@hidden>
> > ---
> >  hw/riscv/sifive_plic.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
> > index 1c703e1a37..70a85cd075 100644
> > --- a/hw/riscv/sifive_plic.c
> > +++ b/hw/riscv/sifive_plic.c
> > @@ -206,7 +206,7 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr 
> > addr, unsigned size)
> >      if (addr >= plic->priority_base && /* 4 bytes per source */
> >          addr < plic->priority_base + (plic->num_sources << 2))
> >      {
> > -        uint32_t irq = (addr - plic->priority_base) >> 2;
> > +        uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
> >          if (RISCV_DEBUG_PLIC) {
> >              qemu_log("plic: read priority: irq=%d priority=%d\n",
> >                  irq, plic->source_priority[irq]);
> > @@ -279,7 +279,7 @@ static void sifive_plic_write(void *opaque, hwaddr 
> > addr, uint64_t value,
> >      if (addr >= plic->priority_base && /* 4 bytes per source */
> >          addr < plic->priority_base + (plic->num_sources << 2))
> >      {
> > -        uint32_t irq = (addr - plic->priority_base) >> 2;
> > +        uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
> >          plic->source_priority[irq] = value & 7;
> >          if (RISCV_DEBUG_PLIC) {
> >              qemu_log("plic: write priority: irq=%d priority=%d\n",
>
> I think this breaks bisectability and should be merged with the
> *_PLIC_PRIORITY_BASE modifications as otherwise we'll end up the priority 
> being
> set for the brong IRQ.

Good point, I can merge them together.

>
> I'm also not sure how this fixes the bug listed in the OpenSBI PR.  As far as 
> I
> can understand it, all this does is ensure that source 0 is actually treated 
> as
> illegal (and shrinks the number of sources to match what's actually there, but
> that shouldn't fix the bug).  That looks more like a cleanup than an actual 
> fix.

The problem is that before we where off by one. We supported 0 - (n -
1) and this patch set changes QEMU to support 1 - n. This is because
of the "addr < plic->priority_base + (plic->num_sources << 2)"
comparison. As priority_base is now 0x04 instead of 0x00 the
comparison will work correctly.

Alistair

>
> Am I missing something?