On Wed, Mar 27, 2019 at 3:29 AM Palmer Dabbelt <pal...@sifive.com> 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 <alistair.fran...@wdc.com>
> > ---
> > 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?
