| Message ID | 7efcf90aa1cc78d1ce2c5f823a7ff9ac96c379ed.1553129005.git.alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update the QEMU PLIC addresses | expand |
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.francis@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. 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. Am I missing something?
On Wed, Mar 27, 2019 at 3:29 AM Palmer Dabbelt <palmer@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.francis@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?
On Wed, 27 Mar 2019 09:29:56 PDT (-0700), alistair23@gmail.com wrote: > On Wed, Mar 27, 2019 at 3:29 AM Palmer Dabbelt <palmer@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.francis@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. 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. > > Alistair > >> >> Am I missing something?
On Wed, Mar 27, 2019 at 8:15 PM Palmer Dabbelt <palmer@sifive.com> wrote: > > On Wed, 27 Mar 2019 09:29:56 PDT (-0700), alistair23@gmail.com wrote: > > On Wed, Mar 27, 2019 at 3:29 AM Palmer Dabbelt <palmer@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.francis@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. > > 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?
On Thu, 28 Mar 2019 10:34:08 PDT (-0700), alistair23@gmail.com wrote: > On Wed, Mar 27, 2019 at 8:15 PM Palmer Dabbelt <palmer@sifive.com> wrote: >> >> On Wed, 27 Mar 2019 09:29:56 PDT (-0700), alistair23@gmail.com wrote: >> > On Wed, Mar 27, 2019 at 3:29 AM Palmer Dabbelt <palmer@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.francis@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. >> >> 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. OK, that makes sense. > > Alistair > >> >> > >> > Alistair >> > >> >> >> >> Am I missing something?
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",
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.francis@wdc.com> --- hw/riscv/sifive_plic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)