[PULL,02/63] riscv: plic: Add a couple of mising sifive_plic_update calls

Commit Message

Alistair Francis June 26, 2020, 9:43 p.m. UTC

From: Jessica Clarke <[e-mail]>

Claiming an interrupt and changing the source priority both potentially
affect whether an interrupt is pending, thus we must re-compute xEIP.
Note that we don't put the sifive_plic_update inside sifive_plic_claim
so that the logging of a claim (and the resulting IRQ) happens before
the state update, making the causal effect clear, and that we drop the
explicit call to sifive_plic_print_state when claiming since
sifive_plic_update already does that automatically at the end for us.

This can result in both spurious interrupt storms if you fail to
complete an IRQ before enabling interrupts (and no other actions occur
that result in a call to sifive_plic_update), but also more importantly
lost interrupts if a disabled interrupt is pending and then becomes
enabled.

Signed-off-by: Jessica Clarke <[e-mail]>
Reviewed-by: Alistair Francis <[e-mail]>
Message-id: [e-mail]
Message-Id: <[e-mail]>
Signed-off-by: Alistair Francis <[e-mail]>
---
 hw/riscv/sifive_plic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Alistair Francis June 26, 2020, 9:58 p.m. UTC | #1

On Fri, Jun 26, 2020 at 3:01 PM Jessica Clarke <[e-mail]> wrote:
>
> On 26 Jun 2020, at 22:43, Alistair Francis <[e-mail]> wrote:
> >
> > From: Jessica Clarke <[e-mail]>
> >
> > Claiming an interrupt and changing the source priority both potentially
> > affect whether an interrupt is pending, thus we must re-compute xEIP.
> > Note that we don't put the sifive_plic_update inside sifive_plic_claim
> > so that the logging of a claim (and the resulting IRQ) happens before
> > the state update, making the causal effect clear, and that we drop the
> > explicit call to sifive_plic_print_state when claiming since
> > sifive_plic_update already does that automatically at the end for us.
> >
> > This can result in both spurious interrupt storms if you fail to
> > complete an IRQ before enabling interrupts (and no other actions occur
> > that result in a call to sifive_plic_update), but also more importantly
> > lost interrupts if a disabled interrupt is pending and then becomes
> > enabled.
> >
> > Signed-off-by: Jessica Clarke <[e-mail]>
> > Reviewed-by: Alistair Francis <[e-mail]>
> > Message-id: [e-mail]
> > Message-Id: <[e-mail]>
>
> Something went a bit weird here.

Argh! Patches seems to apply two Message-id tags and I forgot to
remove them from these two.

I don't think this is a blocker though. Let me know if you or Peter
thinks it is and I can send a v2.

Alistair

>
> Jess
>

Jessica Clarke June 26, 2020, 10:01 p.m. UTC | #2

On 26 Jun 2020, at 22:43, Alistair Francis <[e-mail]> wrote:
> 
> From: Jessica Clarke <[e-mail]>
> 
> Claiming an interrupt and changing the source priority both potentially
> affect whether an interrupt is pending, thus we must re-compute xEIP.
> Note that we don't put the sifive_plic_update inside sifive_plic_claim
> so that the logging of a claim (and the resulting IRQ) happens before
> the state update, making the causal effect clear, and that we drop the
> explicit call to sifive_plic_print_state when claiming since
> sifive_plic_update already does that automatically at the end for us.
> 
> This can result in both spurious interrupt storms if you fail to
> complete an IRQ before enabling interrupts (and no other actions occur
> that result in a call to sifive_plic_update), but also more importantly
> lost interrupts if a disabled interrupt is pending and then becomes
> enabled.
> 
> Signed-off-by: Jessica Clarke <[e-mail]>
> Reviewed-by: Alistair Francis <[e-mail]>
> Message-id: [e-mail]
> Message-Id: <[e-mail]>

Something went a bit weird here.

Jess

Patch

diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
index d91e82b8ab..c20c192034 100644
--- a/hw/riscv/sifive_plic.c
+++ b/hw/riscv/sifive_plic.c
@@ -255,8 +255,8 @@  static uint64_t sifive_plic_read(void *opaque, hwaddr addr, unsigned size)
                     plic->addr_config[addrid].hartid,
                     mode_to_char(plic->addr_config[addrid].mode),
                     value);
-                sifive_plic_print_state(plic);
             }
+            sifive_plic_update(plic);
             return value;
         }
     }
@@ -287,6 +287,7 @@  static void sifive_plic_write(void *opaque, hwaddr addr, uint64_t value,
             qemu_log("plic: write priority: irq=%d priority=%d\n",
                 irq, plic->source_priority[irq]);
         }
+        sifive_plic_update(plic);
         return;
     } else if (addr >= plic->pending_base && /* 1 bit per source */
                addr < plic->pending_base + (plic->num_sources >> 3))