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

[Palmer Dabbelt]

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.

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?