Re: [PULL 35/47] target/arm: Send interrupts on PMU counter overflow

[Aaron Lindsay]

On Feb 25 17:08, Peter Maydell wrote:
> On Fri, 1 Feb 2019 at 16:07, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > From: Aaron Lindsay OS <aaron@os.amperecomputing.com>
> >
> > Whenever we notice that a counter overflow has occurred, send an
> > interrupt. This is made more reliable with the addition of a timer in a
> > follow-on commit.
> >
> > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Message-id: 20190124162401.5111-2-aaron@os.amperecomputing.com
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Hi Aaron -- I've just noticed a problem with this patch that
> went into QEMU recently. The problem is that we can end up
> calling pmu_update_irq(), which injects an interrupt, from
> a raw-write function for some of the PMU registers. This is
> bad because when we're using KVM the raw-write functions are
> called as part of syncing state to and from the kernel. In
> particular, if using '-cpu host,pmu=off' we don't set up the
> PMU interrupt because we don't want to provide the guest a
> PMU but then we can still find ourselves in this function,
> and then we assert because we try to set a bogus interrupt.
> Here's the backtrace:
> 
> #1  0x0000fffff6be68b4 in __GI_abort () at abort.c:79
> #2  0x0000aaaaaae20820 in kvm_set_irq (s=0xaaaaabf8a020, irq=33554455, 
> level=0)
>     at /home/pm/qemu-bisect/accel/kvm/kvm-all.c:1277
> #3  0x0000aaaaaafb7890 in kvm_arm_set_irq (cpu=0, irqtype=2, irq=23, level=0)
>     at /home/pm/qemu-bisect/target/arm/kvm.c:897
> #4  0x0000aaaaaae729dc in kvm_arm_gic_set_irq (num_irq=288, irq=23, level=0)
>     at /home/pm/qemu-bisect/hw/intc/arm_gic_kvm.c:75
> #5  0x0000aaaaaae72a1c in kvm_arm_gicv2_set_irq
> (opaque=0xaaaaac169ff0, irq=279, level=0)
>     at /home/pm/qemu-bisect/hw/intc/arm_gic_kvm.c:82
> #6  0x0000aaaaab1ba15c in qemu_set_irq (irq=0xaaaaac186090, level=0)
>     at /home/pm/qemu-bisect/hw/core/irq.c:44
> #7  0x0000aaaaaaf86050 in pmu_update_irq (env=0xaaaaac0fa470)
>     at /home/pm/qemu-bisect/target/arm/helper.c:1412
> #8  0x0000aaaaaaf8747c in pmintenclr_write (env=0xaaaaac0fa470,
> ri=0xaaaaac12c3e0, value=2154950974777589790) at
> /home/pm/qemu-bisect/target/arm/helper.c:1903
> #9  0x0000aaaaaaf83e68 in write_raw_cp_reg (env=0xaaaaac0fa470,
> ri=0xaaaaac12c3e0, v=2154950976315703518) at
> /home/pm/qemu-bisect/target/arm/helper.c:206
> #10 0x0000aaaaaaf840d4 in write_cpustate_to_list (cpu=0xaaaaac0f0b90,
> kvm_sync=true)
>     at /home/pm/qemu-bisect/target/arm/helper.c:290
> #11 0x0000aaaaaafbb1ac in kvm_arch_put_registers (cs=0xaaaaac0f0b90, level=3)
>     at /home/pm/qemu-bisect/target/arm/kvm64.c:1108
> #12 0x0000aaaaaae22ea0 in do_kvm_cpu_synchronize_post_init
> (cpu=0xaaaaac0f0b90, arg=...)
>     at /home/pm/qemu-bisect/accel/kvm/kvm-all.c:2223
> #13 0x0000aaaaab107fa0 in process_queued_cpu_work (cpu=0xaaaaac0f0b90)
>     at /home/pm/qemu-bisect/cpus-common.c:338
> #14 0x0000aaaaaadf4ff4 in qemu_wait_io_event_common (cpu=0xaaaaac0f0b90)
>     at /home/pm/qemu-bisect/cpus.c:1175
> #15 0x0000aaaaaadf51a8 in qemu_wait_io_event (cpu=0xaaaaac0f0b90)
>     at /home/pm/qemu-bisect/cpus.c:1215
> #16 0x0000aaaaaadf52cc in qemu_kvm_cpu_thread_fn (arg=0xaaaaac0f0b90)
>     at /home/pm/qemu-bisect/cpus.c:1251
> #17 0x0000aaaaab690268 in qemu_thread_start (args=0xaaaaac14b1d0)
>     at /home/pm/qemu-bisect/util/qemu-thread-posix.c:519
> 
> 
> The point of the 'raw_read/write' accessors is that they're supposed
> to not have side effects but just to be usable to read and write
> any underlying register state. If the regdef doesn't define them
> we fall back to the usual readfn/writefn on the assumption that
> they're side-effect-free. So I think the fix here would be to
> provide a raw_writefn everywhere where we've made
> the normal writefn have a "sets an interrupt" side effect.

Ouch - I'm sorry this slipped through the cracks in my inbox for so
long.

I assume you mean something like:

diff --git a/target/arm/helper.c b/target/arm/helper.c
index dc9c29f998..9b917f9425 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2271,13 +2271,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL1_RW, .accessfn = access_tpm,
       .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
-      .writefn = pmintenclr_write, },
+      .writefn = pmintenclr_write, .raw_writefn = raw_write },
     { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 2,
       .access = PL1_RW, .accessfn = access_tpm,
       .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
-      .writefn = pmintenclr_write },
+      .writefn = pmintenclr_write, .raw_writefn = raw_write },
     { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
       .access = PL1_R,

One thing I'm trying to figure out (talking non-KVM here) is whether
skipping calling pmu_update_irq() can mean an interrupt would not be set
when it should have been. It looks like the ARMCPRegInfo's for
PMINTENSET already do `.raw_writefn = raw_write`, so I suppose at least
we would be consistent with this change. But I can never remember - is
it guaranteed that the raw functions are only ever called when the
interrupt state would already be taken care of separately (i.e. when
restoring a checkpoint)?

-Aaron