[PATCH v6 14/19] target-arm/powerctl: defer cpu reset work to CPU context

[PATCH v6 14/19] target-arm/powerctl: defer cpu reset work to CPU context

[Alex Bennée]

When switching a new vCPU on we want to complete a bunch of the setup
work before we start scheduling the vCPU thread. To do this cleanly we
defer vCPU setup to async work which will run the vCPUs execution
context as the thread is woken up. The scheduling of the work will kick
the vCPU awake.

This avoids potential races in MTTCG system emulation.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target-arm/arm-powerctl.c | 144 +++++++++++++++++++++++++++-------------------
 1 file changed, 86 insertions(+), 58 deletions(-)

diff --git a/target-arm/arm-powerctl.c b/target-arm/arm-powerctl.c
index fbb7a15..0ef4b29 100644
--- a/target-arm/arm-powerctl.c
+++ b/target-arm/arm-powerctl.c
@@ -48,11 +48,85 @@ CPUState *arm_get_cpu_by_id(uint64_t id)
     return NULL;
 }
 
+struct cpu_on_info {
+    uint64_t entry;
+    uint64_t context_id;
+    uint32_t target_el;
+    bool target_aa64;
+};
+
+
+static void arm_set_cpu_on_async_work(CPUState *target_cpu_state,
+                                      run_on_cpu_data data)
+{
+    ARMCPU *target_cpu = ARM_CPU(target_cpu_state);
+    struct cpu_on_info *info = (struct cpu_on_info *) data.host_ptr;
+
+    /* Initialize the cpu we are turning on */
+    cpu_reset(target_cpu_state);
+    target_cpu->powered_off = false;
+    target_cpu_state->halted = 0;
+
+    if (info->target_aa64) {
+        if ((info->target_el < 3) && arm_feature(&target_cpu->env, ARM_FEATURE_EL3)) {
+            /*
+             * As target mode is AArch64, we need to set lower
+             * exception level (the requested level 2) to AArch64
+             */
+            target_cpu->env.cp15.scr_el3 |= SCR_RW;
+        }
+
+        if ((info->target_el < 2) && arm_feature(&target_cpu->env, ARM_FEATURE_EL2)) {
+            /*
+             * As target mode is AArch64, we need to set lower
+             * exception level (the requested level 1) to AArch64
+             */
+            target_cpu->env.cp15.hcr_el2 |= HCR_RW;
+        }
+
+        target_cpu->env.pstate = aarch64_pstate_mode(info->target_el, true);
+    } else {
+        /* We are requested to boot in AArch32 mode */
+        static uint32_t mode_for_el[] = { 0,
+                                          ARM_CPU_MODE_SVC,
+                                          ARM_CPU_MODE_HYP,
+                                          ARM_CPU_MODE_SVC };
+
+        cpsr_write(&target_cpu->env, mode_for_el[info->target_el], CPSR_M,
+                   CPSRWriteRaw);
+    }
+
+    if (info->target_el == 3) {
+        /* Processor is in secure mode */
+        target_cpu->env.cp15.scr_el3 &= ~SCR_NS;
+    } else {
+        /* Processor is not in secure mode */
+        target_cpu->env.cp15.scr_el3 |= SCR_NS;
+    }
+
+    /* We check if the started CPU is now at the correct level */
+    assert(info->target_el == arm_current_el(&target_cpu->env));
+
+    if (info->target_aa64) {
+        target_cpu->env.xregs[0] = info->context_id;
+        target_cpu->env.thumb = false;
+    } else {
+        target_cpu->env.regs[0] = info->context_id;
+        target_cpu->env.thumb = info->entry & 1;
+        info->entry &= 0xfffffffe;
+    }
+
+    /* Start the new CPU at the requested address */
+    cpu_set_pc(target_cpu_state, info->entry);
+    g_free(info);
+}
+
 int arm_set_cpu_on(uint64_t cpuid, uint64_t entry, uint64_t context_id,
                    uint32_t target_el, bool target_aa64)
 {
     CPUState *target_cpu_state;
     ARMCPU *target_cpu;
+    struct cpu_on_info *info;
 
     DPRINTF("cpu %" PRId64 " (EL %d, %s) @ 0x%" PRIx64 " with R0 = 0x%" PRIx64
             "\n", cpuid, target_el, target_aa64 ? "aarch64" : "aarch32", entry,
@@ -109,64 +183,18 @@ int arm_set_cpu_on(uint64_t cpuid, uint64_t entry, uint64_t context_id,
         return QEMU_ARM_POWERCTL_INVALID_PARAM;
     }
 
-    /* Initialize the cpu we are turning on */
-    cpu_reset(target_cpu_state);
-    target_cpu->powered_off = false;
-    target_cpu_state->halted = 0;
-
-    if (target_aa64) {
-        if ((target_el < 3) && arm_feature(&target_cpu->env, ARM_FEATURE_EL3)) {
-            /*
-             * As target mode is AArch64, we need to set lower
-             * exception level (the requested level 2) to AArch64
-             */
-            target_cpu->env.cp15.scr_el3 |= SCR_RW;
-        }
-
-        if ((target_el < 2) && arm_feature(&target_cpu->env, ARM_FEATURE_EL2)) {
-            /*
-             * As target mode is AArch64, we need to set lower
-             * exception level (the requested level 1) to AArch64
-             */
-            target_cpu->env.cp15.hcr_el2 |= HCR_RW;
-        }
-
-        target_cpu->env.pstate = aarch64_pstate_mode(target_el, true);
-    } else {
-        /* We are requested to boot in AArch32 mode */
-        static uint32_t mode_for_el[] = { 0,
-                                          ARM_CPU_MODE_SVC,
-                                          ARM_CPU_MODE_HYP,
-                                          ARM_CPU_MODE_SVC };
-
-        cpsr_write(&target_cpu->env, mode_for_el[target_el], CPSR_M,
-                   CPSRWriteRaw);
-    }
-
-    if (target_el == 3) {
-        /* Processor is in secure mode */
-        target_cpu->env.cp15.scr_el3 &= ~SCR_NS;
-    } else {
-        /* Processor is not in secure mode */
-        target_cpu->env.cp15.scr_el3 |= SCR_NS;
-    }
-
-    /* We check if the started CPU is now at the correct level */
-    assert(target_el == arm_current_el(&target_cpu->env));
-
-    if (target_aa64) {
-        target_cpu->env.xregs[0] = context_id;
-        target_cpu->env.thumb = false;
-    } else {
-        target_cpu->env.regs[0] = context_id;
-        target_cpu->env.thumb = entry & 1;
-        entry &= 0xfffffffe;
-    }
-
-    /* Start the new CPU at the requested address */
-    cpu_set_pc(target_cpu_state, entry);
-
-    qemu_cpu_kick(target_cpu_state);
+    /* To avoid racing with a CPU we are just kicking off we do the
+     * final bit of preparation for the work in the target CPUs
+     * context.
+     */
+    info = g_new(struct cpu_on_info, 1);
+    info->entry = entry;
+    info->context_id = context_id;
+    info->target_el = target_el;
+    info->target_aa64 = target_aa64;
+
+    async_run_on_cpu(target_cpu_state, arm_set_cpu_on_async_work,
+                     RUN_ON_CPU_HOST_PTR(info));
 
     /* We are good to go */
     return QEMU_ARM_POWERCTL_RET_SUCCESS;
-- 
2.10.1

Re: [PATCH v6 14/19] target-arm/powerctl: defer cpu reset work to CPU context

[Richard Henderson]

On 11/09/2016 03:57 PM, Alex Bennée wrote:
> +        /* We are requested to boot in AArch32 mode */
> +        static uint32_t mode_for_el[] = { 0,
> +                                          ARM_CPU_MODE_SVC,
> +                                          ARM_CPU_MODE_HYP,
> +                                          ARM_CPU_MODE_SVC };

I know you're just moving most of this code, but, const.

Otherwise,

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

[PATCH v6 15/19] target-arm/cpu: don't reset TLB structures, use cputlb to do it

[Alex Bennée]

cputlb owns the TLB entries and knows how to safely update them in
MTTCG.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target-arm/cpu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 99f0dbe..990bcb1 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -122,7 +122,13 @@ static void arm_cpu_reset(CPUState *s)
 
     acc->parent_reset(s);
 
+#ifdef CONFIG_SOFTMMU
+    memset(env, 0, offsetof(CPUARMState, tlb_table));
+    tlb_flush(s, 0);
+#else
     memset(env, 0, offsetof(CPUARMState, features));
+#endif
+
     g_hash_table_foreach(cpu->cp_regs, cp_reg_reset, cpu);
     g_hash_table_foreach(cpu->cp_regs, cp_reg_check_reset, cpu);
 
-- 
2.10.1

Re: [PATCH v6 15/19] target-arm/cpu: don't reset TLB structures, use cputlb to do it

[Richard Henderson]

On 11/09/2016 03:57 PM, Alex Bennée wrote:
> +#ifdef CONFIG_SOFTMMU
> +    memset(env, 0, offsetof(CPUARMState, tlb_table));
> +    tlb_flush(s, 0);
> +#else
>      memset(env, 0, offsetof(CPUARMState, features));
> +#endif

I'd really prefer to see the tlb_flush be moved into parent_reset, so that we 
handle it identically for all targets.

As for the memset, do we really need to distinguish softmmu?  I don't like you 
picking out a variable name within CPU_COMMON.  Better to use empty struct 
markers, like the

       struct {} start_init_save;

that x86 uses.


r~

Re: [PATCH v6 15/19] target-arm/cpu: don't reset TLB structures, use cputlb to do it

[Alex Bennée]

Richard Henderson <rth@twiddle.net> writes:

> On 11/09/2016 03:57 PM, Alex Bennée wrote:
>> +#ifdef CONFIG_SOFTMMU
>> +    memset(env, 0, offsetof(CPUARMState, tlb_table));
>> +    tlb_flush(s, 0);
>> +#else
>>      memset(env, 0, offsetof(CPUARMState, features));
>> +#endif
>
> I'd really prefer to see the tlb_flush be moved into parent_reset, so that we
> handle it identically for all targets.

Yeah I'll prepare a series to do that separate from MTTCG.

>
> As for the memset, do we really need to distinguish softmmu?  I don't like you
> picking out a variable name within CPU_COMMON.  Better to use empty struct
> markers, like the
>
>        struct {} start_init_save;
>
> that x86 uses.

OK fair enough.

--
Alex Bennée

[PATCH v6 16/19] target-arm: ensure BQL taken for ARM_CP_IO register access

[Alex Bennée]

Most ARMCPRegInfo structures just allow updating of the CPU field.
However some have more complex operations that *may* be have cross vCPU
effects therefor need to be serialised. The most obvious examples at the
moment are things that affect the GICv3 IRQ controller. To avoid
applying this requirement to all registers with custom access functions
we check for if the type is marked ARM_CP_IO.

By default all MMIO access to devices already takes the BQL to serialise
hardware emulation.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/intc/arm_gicv3_cpuif.c |  3 +++
 target-arm/op_helper.c    | 39 +++++++++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index bca30c4..8ea4b5b 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -13,6 +13,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "trace.h"
 #include "gicv3_internal.h"
 #include "cpu.h"
@@ -128,6 +129,8 @@ void gicv3_cpuif_update(GICv3CPUState *cs)
     ARMCPU *cpu = ARM_CPU(cs->cpu);
     CPUARMState *env = &cpu->env;
 
+    g_assert(qemu_mutex_iothread_locked());
+
     trace_gicv3_cpuif_update(gicv3_redist_affid(cs), cs->hppi.irq,
                              cs->hppi.grp, cs->hppi.prio);
 
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index cd94216..4f0c754 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "exec/helper-proto.h"
 #include "internals.h"
@@ -734,28 +735,58 @@ void HELPER(set_cp_reg)(CPUARMState *env, void *rip, uint32_t value)
 {
     const ARMCPRegInfo *ri = rip;
 
-    ri->writefn(env, ri, value);
+    if (ri->type & ARM_CP_IO) {
+        qemu_mutex_lock_iothread();
+        ri->writefn(env, ri, value);
+        qemu_mutex_unlock_iothread();
+    } else {
+        ri->writefn(env, ri, value);
+    }
 }
 
 uint32_t HELPER(get_cp_reg)(CPUARMState *env, void *rip)
 {
     const ARMCPRegInfo *ri = rip;
+    uint32_t res;
 
-    return ri->readfn(env, ri);
+    if (ri->type & ARM_CP_IO) {
+        qemu_mutex_lock_iothread();
+        res = ri->readfn(env, ri);
+        qemu_mutex_unlock_iothread();
+    } else {
+        res = ri->readfn(env, ri);
+    }
+
+    return res;
 }
 
 void HELPER(set_cp_reg64)(CPUARMState *env, void *rip, uint64_t value)
 {
     const ARMCPRegInfo *ri = rip;
 
-    ri->writefn(env, ri, value);
+    if (ri->type & ARM_CP_IO) {
+        qemu_mutex_lock_iothread();
+        ri->writefn(env, ri, value);
+        qemu_mutex_unlock_iothread();
+    } else {
+        ri->writefn(env, ri, value);
+    }
 }
 
 uint64_t HELPER(get_cp_reg64)(CPUARMState *env, void *rip)
 {
     const ARMCPRegInfo *ri = rip;
+    uint64_t res;
+
+    if (ri->type & ARM_CP_IO) {
+        qemu_mutex_lock_iothread();
+        res = ri->readfn(env, ri);
+        qemu_mutex_unlock_iothread();
+    } else {
+        res = ri->readfn(env, ri);
+    }
 
-    return ri->readfn(env, ri);
+    return res;
 }
 
 void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm)
-- 
2.10.1

Re: [PATCH v6 16/19] target-arm: ensure BQL taken for ARM_CP_IO register access

[Richard Henderson]

On 11/09/2016 03:57 PM, Alex Bennée wrote:
> Most ARMCPRegInfo structures just allow updating of the CPU field.
> However some have more complex operations that *may* be have cross vCPU
> effects therefor need to be serialised. The most obvious examples at the
> moment are things that affect the GICv3 IRQ controller. To avoid
> applying this requirement to all registers with custom access functions
> we check for if the type is marked ARM_CP_IO.
>
> By default all MMIO access to devices already takes the BQL to serialise
> hardware emulation.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/intc/arm_gicv3_cpuif.c |  3 +++
>  target-arm/op_helper.c    | 39 +++++++++++++++++++++++++++++++++++----
>  2 files changed, 38 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

[PATCH v6 17/19] target-arm: helpers which may affect global state need the BQL

[Alex Bennée]

As the arm_call_el_change_hook may affect global state (for example with
updating the global GIC state) we need to assert/take the BQL.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target-arm/helper.c    | 6 ++++++
 target-arm/op_helper.c | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index b5b65ca..3f47fa7 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6669,6 +6669,12 @@ void arm_cpu_do_interrupt(CPUState *cs)
         arm_cpu_do_interrupt_aarch32(cs);
     }
 
+    /* Hooks may change global state so BQL should be held, also the
+     * BQL needs to be held for any modification of
+     * cs->interrupt_request.
+     */
+    g_assert(qemu_mutex_iothread_locked());
+
     arm_call_el_change_hook(cpu);
 
     if (!kvm_enabled()) {
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 4f0c754..41beabc 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -487,7 +487,9 @@ void HELPER(cpsr_write_eret)(CPUARMState *env, uint32_t val)
      */
     env->regs[15] &= (env->thumb ? ~1 : ~3);
 
+    qemu_mutex_lock_iothread();
     arm_call_el_change_hook(arm_env_get_cpu(env));
+    qemu_mutex_unlock_iothread();
 }
 
 /* Access to user mode registers from privileged modes.  */
@@ -1013,7 +1015,9 @@ void HELPER(exception_return)(CPUARMState *env)
         env->pc = env->elr_el[cur_el];
     }
 
+    qemu_mutex_lock_iothread();
     arm_call_el_change_hook(arm_env_get_cpu(env));
+    qemu_mutex_unlock_iothread();
 
     return;
 
-- 
2.10.1

Re: [PATCH v6 17/19] target-arm: helpers which may affect global state need the BQL

[Richard Henderson]

On 11/09/2016 03:57 PM, Alex Bennée wrote:
> As the arm_call_el_change_hook may affect global state (for example with
> updating the global GIC state) we need to assert/take the BQL.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target-arm/helper.c    | 6 ++++++
>  target-arm/op_helper.c | 4 ++++
>  2 files changed, 10 insertions(+)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

[PATCH v6 18/19] target-arm: don't generate WFE/YIELD calls for MTTCG

[Alex Bennée]

The WFE and YIELD instructions are really only hints and in TCG's case
they were useful to move the scheduling on from one vCPU to the next. In
the parallel context (MTTCG) this just causes an unnecessary cpu_exit
and contention of the BQL.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target-arm/op_helper.c     |  7 +++++++
 target-arm/translate-a64.c |  8 ++++++--
 target-arm/translate.c     | 20 ++++++++++++++++----
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 41beabc..3a36bf9 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -435,6 +435,13 @@ void HELPER(yield)(CPUARMState *env)
     ARMCPU *cpu = arm_env_get_cpu(env);
     CPUState *cs = CPU(cpu);
 
+    /* When running in MTTCG we don't generate jumps to the yield and
+     * WFE helpers as it won't affect the scheduling of other vCPUs.
+     * If we wanted to more completely model WFE/SEV so we don't busy
+     * spin unnecessarily we would need to do something more involved.
+     */
+    g_assert(!parallel_cpus);
+
     /* This is a non-trappable hint instruction that generally indicates
      * that the guest is currently busy-looping. Yield control back to the
      * top level loop so that a more deserving VCPU has a chance to run.
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index de48747..6e44838 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1341,10 +1341,14 @@ static void handle_hint(DisasContext *s, uint32_t insn,
         s->is_jmp = DISAS_WFI;
         return;
     case 1: /* YIELD */
-        s->is_jmp = DISAS_YIELD;
+        if (!parallel_cpus) {
+            s->is_jmp = DISAS_YIELD;
+        }
         return;
     case 2: /* WFE */
-        s->is_jmp = DISAS_WFE;
+        if (!parallel_cpus) {
+            s->is_jmp = DISAS_WFE;
+        }
         return;
     case 4: /* SEV */
     case 5: /* SEVL */
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 0ad9070..9417e8e 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4368,20 +4368,32 @@ static void gen_exception_return(DisasContext *s, TCGv_i32 pc)
     gen_rfe(s, pc, load_cpu_field(spsr));
 }
 
+/*
+ * For WFI we will halt the vCPU until an IRQ. For WFE and YIELD we
+ * only call the helper when running single threaded TCG code to ensure
+ * the next round-robin scheduled vCPU gets a crack. In MTTCG mode we
+ * just skip this instruction. Currently the SEV/SEVL instructions
+ * which are *one* of many ways to wake the CPU from WFE are not
+ * implemented so we can't sleep like WFI does.
+ */
 static void gen_nop_hint(DisasContext *s, int val)
 {
     switch (val) {
     case 1: /* yield */
-        gen_set_pc_im(s, s->pc);
-        s->is_jmp = DISAS_YIELD;
+        if (!parallel_cpus) {
+            gen_set_pc_im(s, s->pc);
+            s->is_jmp = DISAS_YIELD;
+        }
         break;
     case 3: /* wfi */
         gen_set_pc_im(s, s->pc);
         s->is_jmp = DISAS_WFI;
         break;
     case 2: /* wfe */
-        gen_set_pc_im(s, s->pc);
-        s->is_jmp = DISAS_WFE;
+        if (!parallel_cpus) {
+            gen_set_pc_im(s, s->pc);
+            s->is_jmp = DISAS_WFE;
+        }
         break;
     case 4: /* sev */
     case 5: /* sevl */
-- 
2.10.1

Re: [PATCH v6 18/19] target-arm: don't generate WFE/YIELD calls for MTTCG

[Richard Henderson]

On 11/09/2016 03:57 PM, Alex Bennée wrote:
> The WFE and YIELD instructions are really only hints and in TCG's case
> they were useful to move the scheduling on from one vCPU to the next. In
> the parallel context (MTTCG) this just causes an unnecessary cpu_exit
> and contention of the BQL.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target-arm/op_helper.c     |  7 +++++++
>  target-arm/translate-a64.c |  8 ++++++--
>  target-arm/translate.c     | 20 ++++++++++++++++----
>  3 files changed, 29 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~