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.