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

[Alistair Francis]

On Wed, Mar 27, 2019 at 8:15 PM Palmer Dabbelt <address@hidden> wrote:
>
> On Wed, 27 Mar 2019 09:29:56 PDT (-0700), address@hidden wrote:
> > 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.
>
> So something in OpenSBI is going through the entire set of sources and
> initializing the priorities?  That makes sense for the off-by-one, I was just
> wondering because the array shrank so the specific offset in that error would
> still be invalid with the new patch set.

Yep, that is what caught this. The array only shrinks for the FU540
board, the virt board doesn't shrink and that is the board that
OpenSBI caught the bug with.

Alistair

>
> >
> > Alistair
> >
> >>
> >> Am I missing something?