diff mbox series

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

Message ID 20190201160653.13829-36-peter.maydell@linaro.org
State New
Headers show
Series [PULL,01/47] hw/arm/nrf51_soc: set object owner in memory_region_init_ram | expand

Commit Message

Peter Maydell Feb. 1, 2019, 4:06 p.m. UTC 
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>
---
 target/arm/helper.c | 61 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 10 deletions(-)

Comments

Peter Maydell Feb. 25, 2020, 5:08 p.m. UTC | #1 
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.

thanks
-- PMM
Aaron Lindsay July 1, 2020, 3:11 p.m. UTC | #2 
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
Peter Maydell July 3, 2020, 3:14 p.m. UTC | #3 
On Wed, 1 Jul 2020 at 16:11, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
> 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,

For cases like this where you have a 'set' and a 'clr' register
that really are accessing the same state under the hood, the
way to do it is:
 * the 'set' register provides the raw_readfn/raw_writefn
   as raw_read/raw_write
 * the 'clr' register adds ARM_CP_NO_RAW to its .type flags,
   which means "don't do any raw accesses to this, it doesn't
   have any underlying state that's not already synced or
   migrated via some other mechanism".

> 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)?

Yes, the raw writes only happen for migration or for when
we're syncing state from a KVM kernel, so the state of
the device at the other end of the irq line should already
be correct.

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 66faebea8ec..c27e349dd05 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -977,6 +977,7 @@  static const ARMCPRegInfo v6_cp_reginfo[] = {
 /* Definitions for the PMU registers */
 #define PMCRN_MASK  0xf800
 #define PMCRN_SHIFT 11
+#define PMCRLC  0x40
 #define PMCRDP  0x10
 #define PMCRD   0x8
 #define PMCRC   0x4
@@ -1293,6 +1294,13 @@  static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
     return enabled && !prohibited && !filtered;
 }
 
+static void pmu_update_irq(CPUARMState *env)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    qemu_set_irq(cpu->pmu_interrupt, (env->cp15.c9_pmcr & PMCRE) &&
+            (env->cp15.c9_pminten & env->cp15.c9_pmovsr));
+}
+
 /*
  * Ensure c15_ccnt is the guest-visible count so that operations such as
  * enabling/disabling the counter or filtering, modifying the count itself,
@@ -1310,7 +1318,16 @@  void pmccntr_op_start(CPUARMState *env)
             eff_cycles /= 64;
         }
 
-        env->cp15.c15_ccnt = eff_cycles - env->cp15.c15_ccnt_delta;
+        uint64_t new_pmccntr = eff_cycles - env->cp15.c15_ccnt_delta;
+
+        uint64_t overflow_mask = env->cp15.c9_pmcr & PMCRLC ? \
+                                 1ull << 63 : 1ull << 31;
+        if (env->cp15.c15_ccnt & ~new_pmccntr & overflow_mask) {
+            env->cp15.c9_pmovsr |= (1 << 31);
+            pmu_update_irq(env);
+        }
+
+        env->cp15.c15_ccnt = new_pmccntr;
     }
     env->cp15.c15_ccnt_delta = cycles;
 }
@@ -1345,8 +1362,13 @@  static void pmevcntr_op_start(CPUARMState *env, uint8_t counter)
     }
 
     if (pmu_counter_enabled(env, counter)) {
-        env->cp15.c14_pmevcntr[counter] =
-            count - env->cp15.c14_pmevcntr_delta[counter];
+        uint32_t new_pmevcntr = count - env->cp15.c14_pmevcntr_delta[counter];
+
+        if (env->cp15.c14_pmevcntr[counter] & ~new_pmevcntr & INT32_MIN) {
+            env->cp15.c9_pmovsr |= (1 << counter);
+            pmu_update_irq(env);
+        }
+        env->cp15.c14_pmevcntr[counter] = new_pmevcntr;
     }
     env->cp15.c14_pmevcntr_delta[counter] = count;
 }
@@ -1423,7 +1445,20 @@  static void pmswinc_write(CPUARMState *env, const ARMCPRegInfo *ri,
                 /* counter is SW_INCR */
                 (env->cp15.c14_pmevtyper[i] & PMXEVTYPER_EVTCOUNT) == 0x0) {
             pmevcntr_op_start(env, i);
-            env->cp15.c14_pmevcntr[i]++;
+
+            /*
+             * Detect if this write causes an overflow since we can't predict
+             * PMSWINC overflows like we can for other events
+             */
+            uint32_t new_pmswinc = env->cp15.c14_pmevcntr[i] + 1;
+
+            if (env->cp15.c14_pmevcntr[i] & ~new_pmswinc & INT32_MIN) {
+                env->cp15.c9_pmovsr |= (1 << i);
+                pmu_update_irq(env);
+            }
+
+            env->cp15.c14_pmevcntr[i] = new_pmswinc;
+
             pmevcntr_op_finish(env, i);
         }
     }
@@ -1508,6 +1543,7 @@  static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= pmu_counter_mask(env);
     env->cp15.c9_pmovsr &= ~value;
+    pmu_update_irq(env);
 }
 
 static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1515,6 +1551,7 @@  static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= pmu_counter_mask(env);
     env->cp15.c9_pmovsr |= value;
+    pmu_update_irq(env);
 }
 
 static void pmevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1701,6 +1738,7 @@  static void pmintenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
     /* We have no event counters so only the C bit can be changed */
     value &= pmu_counter_mask(env);
     env->cp15.c9_pminten |= value;
+    pmu_update_irq(env);
 }
 
 static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1708,6 +1746,7 @@  static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= pmu_counter_mask(env);
     env->cp15.c9_pminten &= ~value;
+    pmu_update_irq(env);
 }
 
 static void vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1846,7 +1885,7 @@  static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
       .writefn = pmcntenclr_write },
     { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
-      .access = PL0_RW,
+      .access = PL0_RW, .type = ARM_CP_IO,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
       .accessfn = pmreg_access,
       .writefn = pmovsr_write,
@@ -1854,16 +1893,18 @@  static const ARMCPRegInfo v7_cp_reginfo[] = {
     { .name = "PMOVSCLR_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 3,
       .access = PL0_RW, .accessfn = pmreg_access,
-      .type = ARM_CP_ALIAS,
+      .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
       .writefn = pmovsr_write,
       .raw_writefn = raw_write },
     { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
-      .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW,
+      .access = PL0_W, .accessfn = pmreg_access_swinc,
+      .type = ARM_CP_NO_RAW | ARM_CP_IO,
       .writefn = pmswinc_write },
     { .name = "PMSWINC_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 4,
-      .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW,
+      .access = PL0_W, .accessfn = pmreg_access_swinc,
+      .type = ARM_CP_NO_RAW | ARM_CP_IO,
       .writefn = pmswinc_write },
     { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
       .access = PL0_RW, .type = ARM_CP_ALIAS,
@@ -2050,14 +2091,14 @@  static const ARMCPRegInfo pmovsset_cp_reginfo[] = {
     /* PMOVSSET is not implemented in v7 before v7ve */
     { .name = "PMOVSSET", .cp = 15, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 3,
       .access = PL0_RW, .accessfn = pmreg_access,
-      .type = ARM_CP_ALIAS,
+      .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
       .writefn = pmovsset_write,
       .raw_writefn = raw_write },
     { .name = "PMOVSSET_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 3,
       .access = PL0_RW, .accessfn = pmreg_access,
-      .type = ARM_CP_ALIAS,
+      .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
       .writefn = pmovsset_write,
       .raw_writefn = raw_write },