[Qemu-devel] [PATCH 0/8] spapr: Cleanups to startup and LPCR handling

[Qemu-devel] [PATCH 0/8] spapr: Cleanups to startup and LPCR handling

[David Gibson]

This is an assortment of patches cleaning up how we handle startup /
entry of CPUs for the pseries machine type.  In particular it makes a
number of cleanups to the way we manage the LPCR register.

I've posted versions of most of these patches before, however there
have been enough reworks and re-organizations that the series as a
whole isn't really a new version of an existing one.

David Gibson (8):
  target/ppc: Add ppc_store_lpcr() helper
  spapr: Clean up rtas_start_cpu() & rtas_stop_self()
  spapr: Remove unhelpful helpers from rtas_start_cpu()
  spapr: Make a helper to set up cpu entry point state
  spapr: Clean up LPCR updates from hypercalls
  target/ppc: Delay initialization of LPCR_UPRT for secondary cpus
  spapr: Move PAPR mode cpu setup fully to spapr code
  spapr: Clean up handling of LPCR power-saving exit bits

 hw/ppc/spapr.c                  |   4 +-
 hw/ppc/spapr_cpu_core.c         |  47 +++++++++++---
 hw/ppc/spapr_hcall.c            |  50 ++++++---------
 hw/ppc/spapr_rtas.c             | 108 ++++++++++++++++----------------
 include/hw/ppc/spapr_cpu_core.h |   3 +
 target/ppc/cpu.h                |   2 +-
 target/ppc/kvm.c                |   4 ++
 target/ppc/mmu-hash64.c         |  15 +++--
 target/ppc/mmu-hash64.h         |   3 +-
 target/ppc/translate_init.c     |  62 +-----------------
 10 files changed, 134 insertions(+), 164 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH 1/8] target/ppc: Add ppc_store_lpcr() helper

[David Gibson]

There are some fields in the cpu state which need to be updated when the
LPCR register is changed, which is done by ppc_hash64_update_rmls() and
ppc_hash64_update_vrma().  Code which alters env->spr[SPR_LPCR] needs to
call them afterwards to make sure the state is up to date.

That's easy to get wrong.  The normal way of dealing with sitautions like
that is to use a helper which both updates the basic register value and the
derived state.

So, do that.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/mmu-hash64.c     | 15 +++++++++++----
 target/ppc/mmu-hash64.h     |  3 +--
 target/ppc/translate_init.c |  6 +-----
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 7e0adecfd9..a1db20e3a8 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -942,7 +942,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
     cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
 }
 
-void ppc_hash64_update_rmls(PowerPCCPU *cpu)
+static void ppc_hash64_update_rmls(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
     uint64_t lpcr = env->spr[SPR_LPCR];
@@ -977,7 +977,7 @@ void ppc_hash64_update_rmls(PowerPCCPU *cpu)
     }
 }
 
-void ppc_hash64_update_vrma(PowerPCCPU *cpu)
+static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
     const PPCHash64SegmentPageSizes *sps = NULL;
@@ -1028,9 +1028,9 @@ void ppc_hash64_update_vrma(PowerPCCPU *cpu)
     slb->sps = sps;
 }
 
-void helper_store_lpcr(CPUPPCState *env, target_ulong val)
+void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
 {
-    PowerPCCPU *cpu = ppc_env_get_cpu(env);
+    CPUPPCState *env = &cpu->env;
     uint64_t lpcr = 0;
 
     /* Filter out bits */
@@ -1096,6 +1096,13 @@ void helper_store_lpcr(CPUPPCState *env, target_ulong val)
     ppc_hash64_update_vrma(cpu);
 }
 
+void helper_store_lpcr(CPUPPCState *env, target_ulong val)
+{
+    PowerPCCPU *cpu = ppc_env_get_cpu(env);
+
+    ppc_store_lpcr(cpu, val);
+}
+
 void ppc_hash64_init(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
index f6349ccdb3..53dcec5b93 100644
--- a/target/ppc/mmu-hash64.h
+++ b/target/ppc/mmu-hash64.h
@@ -17,8 +17,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
                                target_ulong pte0, target_ulong pte1);
 unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
                                           uint64_t pte0, uint64_t pte1);
-void ppc_hash64_update_vrma(PowerPCCPU *cpu);
-void ppc_hash64_update_rmls(PowerPCCPU *cpu);
+void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val);
 void ppc_hash64_init(PowerPCCPU *cpu);
 void ppc_hash64_finalize(PowerPCCPU *cpu);
 #endif
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index c83c910a29..3fd380dad6 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8940,15 +8940,11 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
     /* We should be followed by a CPU reset but update the active value
      * just in case...
      */
-    env->spr[SPR_LPCR] = lpcr->default_value;
+    ppc_store_lpcr(cpu, lpcr->default_value);
 
     /* Set a full AMOR so guest can use the AMR as it sees fit */
     env->spr[SPR_AMOR] = amor->default_value = 0xffffffffffffffffull;
 
-    /* Update some env bits based on new LPCR value */
-    ppc_hash64_update_rmls(cpu);
-    ppc_hash64_update_vrma(cpu);
-
     /* Tell KVM that we're in PAPR mode */
     if (kvm_enabled()) {
         kvmppc_set_papr(cpu);
-- 
2.17.0

[Qemu-devel] [PATCH 2/8] spapr: Clean up rtas_start_cpu() & rtas_stop_self()

[David Gibson]

This makes several minor cleanups to these functions:
  * Follow usual convention of an early exit on error, rather than having
    most of the body in an if
  * Clearer naming of cpu and cpu_.  Now callcpu is the cpu from which the
    RTAS call is invoked, newcpu is the cpu which we're starting
  * Use cpu_synchronize_state() instead of kvm_cpu_synchronize_state()
    directly
  * Remove pointless comment describing what cpu_synchronize_state() does
  * Use ppc_store_lpcr() instead of directly writing the register field

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_rtas.c | 66 ++++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 0ec5fa4cfe..b251c130cb 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -32,7 +32,7 @@
 #include "hw/qdev.h"
 #include "sysemu/device_tree.h"
 #include "sysemu/cpus.h"
-#include "sysemu/kvm.h"
+#include "sysemu/hw_accel.h"
 
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
@@ -45,6 +45,7 @@
 #include "qemu/cutils.h"
 #include "trace.h"
 #include "hw/ppc/fdt.h"
+#include "target/ppc/mmu-hash64.h"
 
 static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                                    uint32_t token, uint32_t nargs,
@@ -140,13 +141,15 @@ static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
     }
 }
 
-static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
+static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
                            uint32_t token, uint32_t nargs,
                            target_ulong args,
                            uint32_t nret, target_ulong rets)
 {
     target_ulong id, start, r3;
-    PowerPCCPU *cpu;
+    PowerPCCPU *newcpu;
+    CPUPPCState *env;
+    PowerPCCPUClass *pcc;
 
     if (nargs != 3 || nret != 1) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
@@ -157,41 +160,37 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
     start = rtas_ld(args, 1);
     r3 = rtas_ld(args, 2);
 
-    cpu = spapr_find_cpu(id);
-    if (cpu != NULL) {
-        CPUState *cs = CPU(cpu);
-        CPUPPCState *env = &cpu->env;
-        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    newcpu = spapr_find_cpu(id);
+    if (!newcpu) {
+        /* Didn't find a matching cpu */
+        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+        return;
+    }
 
-        if (!cs->halted) {
-            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
-            return;
-        }
+    env = &newcpu->env;
+    pcc = POWERPC_CPU_GET_CLASS(newcpu);
 
-        /* This will make sure qemu state is up to date with kvm, and
-         * mark it dirty so our changes get flushed back before the
-         * new cpu enters */
-        kvm_cpu_synchronize_state(cs);
+    if (!CPU(newcpu)->halted) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
 
-        env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
+    cpu_synchronize_state(CPU(newcpu));
 
-        /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
-        env->spr[SPR_LPCR] |= pcc->lpcr_pm;
+    env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
+    spapr_cpu_set_endianness(newcpu);
+    spapr_cpu_update_tb_offset(newcpu);
+    /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
+    ppc_store_lpcr(newcpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
 
-        env->nip = start;
-        env->gpr[3] = r3;
-        cs->halted = 0;
-        spapr_cpu_set_endianness(cpu);
-        spapr_cpu_update_tb_offset(cpu);
+    env->nip = start;
+    env->gpr[3] = r3;
 
-        qemu_cpu_kick(cs);
+    CPU(newcpu)->halted = 0;
 
-        rtas_st(rets, 0, RTAS_OUT_SUCCESS);
-        return;
-    }
+    qemu_cpu_kick(CPU(newcpu));
 
-    /* Didn't find a matching cpu */
-    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
 
 static void rtas_stop_self(PowerPCCPU *cpu, sPAPRMachineState *spapr,
@@ -203,13 +202,12 @@ static void rtas_stop_self(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
 
-    cs->halted = 1;
-    qemu_cpu_kick(cs);
-
     /* Disable Power-saving mode Exit Cause exceptions for the CPU.
      * This could deliver an interrupt on a dying CPU and crash the
      * guest */
-    env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
+    ppc_store_lpcr(cpu, env->spr[SPR_LPCR] & ~pcc->lpcr_pm);
+    cs->halted = 1;
+    qemu_cpu_kick(cs);
 }
 
 static inline int sysparm_st(target_ulong addr, target_ulong len,
-- 
2.17.0

[Qemu-devel] [PATCH 3/8] spapr: Remove unhelpful helpers from rtas_start_cpu()

[David Gibson]

rtas_start_cpu() calls spapr_cpu_update_tb_offset() and
spapr_cpu_set_endianness() to initialize certain things in the new cpu's
state.  This is the only caller of those helpers, and they're each only
a few lines long, so we might as well just fold them into the caller.

In addition, those helpers initialize state on the new cpu to match that of
the first cpu.  That will generally work, but might be at least logically
incorrect if the first cpu has been set offline by the guest.  So, instead
base the state on that of the cpu invoking the RTAS call, which is
obviously active already.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_rtas.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index b251c130cb..df073447c5 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -120,27 +120,6 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
     rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
 }
 
-/*
- * Set the timebase offset of the CPU to that of first CPU.
- * This helps hotplugged CPU to have the correct timebase offset.
- */
-static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
-{
-    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
-
-    cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
-}
-
-static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
-{
-    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
-
-    if (!pcc->interrupts_big_endian(fcpu)) {
-        cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
-    }
-}
-
 static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
                            uint32_t token, uint32_t nargs,
                            target_ulong args,
@@ -150,6 +129,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
     PowerPCCPU *newcpu;
     CPUPPCState *env;
     PowerPCCPUClass *pcc;
+    target_ulong lpcr;
 
     if (nargs != 3 || nret != 1) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
@@ -178,10 +158,20 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
     cpu_synchronize_state(CPU(newcpu));
 
     env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
-    spapr_cpu_set_endianness(newcpu);
-    spapr_cpu_update_tb_offset(newcpu);
+
     /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
-    ppc_store_lpcr(newcpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
+    lpcr = env->spr[SPR_LPCR] | pcc->lpcr_pm;
+    if (!pcc->interrupts_big_endian(callcpu)) {
+        lpcr |= LPCR_ILE;
+    }
+    ppc_store_lpcr(newcpu, lpcr);
+
+    /*
+     * Set the timebase offset of the new CPU to that of the invoking
+     * CPU.  This helps hotplugged CPU to have the correct timebase
+     * offset.
+     */
+    newcpu->env.tb_env->tb_offset = callcpu->env.tb_env->tb_offset;
 
     env->nip = start;
     env->gpr[3] = r3;
-- 
2.17.0

[Qemu-devel] [PATCH 4/8] spapr: Make a helper to set up cpu entry point state

[David Gibson]

Under PAPR, only the boot CPU is active when the system starts.  Other cpus
must be explicitly activated using an RTAS call.  The entry state for the
boot and secondary cpus isn't identical, but it has some things in common.
We're going to add a bit more common setup later, too, so to simplify
make a helper which sets up the common entry state for both boot and
secondary cpu threads.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr.c                  | 4 +---
 hw/ppc/spapr_cpu_core.c         | 9 +++++++++
 hw/ppc/spapr_rtas.c             | 6 ++----
 include/hw/ppc/spapr_cpu_core.h | 3 +++
 4 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b35aff5d81..944bee7a71 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1668,10 +1668,8 @@ static void spapr_machine_reset(void)
     g_free(fdt);
 
     /* Set up the entry state */
-    first_ppc_cpu->env.gpr[3] = fdt_addr;
+    spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
     first_ppc_cpu->env.gpr[5] = 0;
-    first_cpu->halted = 0;
-    first_ppc_cpu->env.nip = SPAPR_ENTRY_POINT;
 
     spapr->cas_reboot = false;
 }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 01dbc69424..a98c7b04c6 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -52,6 +52,15 @@ static void spapr_cpu_reset(void *opaque)
 
 }
 
+void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
+{
+    CPUPPCState *env = &cpu->env;
+
+    env->nip = nip;
+    env->gpr[3] = r3;
+    CPU(cpu)->halted = 0;
+}
+
 static void spapr_cpu_destroy(PowerPCCPU *cpu)
 {
     qemu_unregister_reset(spapr_cpu_reset, cpu);
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index df073447c5..840d198a8d 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -37,6 +37,7 @@
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
 #include "hw/ppc/spapr_rtas.h"
+#include "hw/ppc/spapr_cpu_core.h"
 #include "hw/ppc/ppc.h"
 #include "hw/boards.h"
 
@@ -173,10 +174,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
      */
     newcpu->env.tb_env->tb_offset = callcpu->env.tb_env->tb_offset;
 
-    env->nip = start;
-    env->gpr[3] = r3;
-
-    CPU(newcpu)->halted = 0;
+    spapr_cpu_set_entry_state(newcpu, start, r3);
 
     qemu_cpu_kick(CPU(newcpu));
 
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 1129f344aa..47dcfda12b 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -12,6 +12,7 @@
 #include "hw/qdev.h"
 #include "hw/cpu/core.h"
 #include "target/ppc/cpu-qom.h"
+#include "target/ppc/cpu.h"
 
 #define TYPE_SPAPR_CPU_CORE "spapr-cpu-core"
 #define SPAPR_CPU_CORE(obj) \
@@ -38,4 +39,6 @@ typedef struct sPAPRCPUCoreClass {
 } sPAPRCPUCoreClass;
 
 const char *spapr_get_cpu_core_type(const char *cpu_type);
+void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
+
 #endif
-- 
2.17.0

[Qemu-devel] [PATCH 5/8] spapr: Clean up LPCR updates from hypercalls

[David Gibson]

There are several places in spapr_hcall.c where we need to update the LPCR
value on all CPUs.  We do this with the set_spr() helper.  That's not
really correct because this directly sets the SPR value, without going
through the ppc_store_lpcr() helper which may need to update state based
on the LPCR change.

In fact, set_spr() is only ever used for the LPCR, so replace it with an
explicit LPCR updated which uses the right low-level helper.  While we're
there, move the CPU_FOREACH() which was in every one of the callers into
the new helper: set_all_lpcrs().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_hcall.c | 50 ++++++++++++++++++--------------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 16bccdd5c0..ca9702e667 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -15,32 +15,35 @@
 #include "hw/ppc/spapr_ovec.h"
 #include "mmu-book3s-v3.h"
 
-struct SPRSyncState {
-    int spr;
+struct LPCRSyncState {
     target_ulong value;
     target_ulong mask;
 };
 
-static void do_spr_sync(CPUState *cs, run_on_cpu_data arg)
+static void do_lpcr_sync(CPUState *cs, run_on_cpu_data arg)
 {
-    struct SPRSyncState *s = arg.host_ptr;
+    struct LPCRSyncState *s = arg.host_ptr;
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
+    target_ulong lpcr;
 
     cpu_synchronize_state(cs);
-    env->spr[s->spr] &= ~s->mask;
-    env->spr[s->spr] |= s->value;
+    lpcr = env->spr[SPR_LPCR];
+    lpcr &= ~s->mask;
+    lpcr |= s->value;
+    ppc_store_lpcr(cpu, lpcr);
 }
 
-static void set_spr(CPUState *cs, int spr, target_ulong value,
-                    target_ulong mask)
+static void set_all_lpcrs(target_ulong value, target_ulong mask)
 {
-    struct SPRSyncState s = {
-        .spr = spr,
+    CPUState *cs;
+    struct LPCRSyncState s = {
         .value = value,
         .mask = mask
     };
-    run_on_cpu(cs, do_spr_sync, RUN_ON_CPU_HOST_PTR(&s));
+    CPU_FOREACH(cs) {
+        run_on_cpu(cs, do_lpcr_sync, RUN_ON_CPU_HOST_PTR(&s));
+    }
 }
 
 static bool has_spr(PowerPCCPU *cpu, int spr)
@@ -1235,8 +1238,6 @@ static target_ulong h_set_mode_resource_le(PowerPCCPU *cpu,
                                            target_ulong value1,
                                            target_ulong value2)
 {
-    CPUState *cs;
-
     if (value1) {
         return H_P3;
     }
@@ -1246,16 +1247,12 @@ static target_ulong h_set_mode_resource_le(PowerPCCPU *cpu,
 
     switch (mflags) {
     case H_SET_MODE_ENDIAN_BIG:
-        CPU_FOREACH(cs) {
-            set_spr(cs, SPR_LPCR, 0, LPCR_ILE);
-        }
+        set_all_lpcrs(0, LPCR_ILE);
         spapr_pci_switch_vga(true);
         return H_SUCCESS;
 
     case H_SET_MODE_ENDIAN_LITTLE:
-        CPU_FOREACH(cs) {
-            set_spr(cs, SPR_LPCR, LPCR_ILE, LPCR_ILE);
-        }
+        set_all_lpcrs(LPCR_ILE, LPCR_ILE);
         spapr_pci_switch_vga(false);
         return H_SUCCESS;
     }
@@ -1268,7 +1265,6 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
                                                         target_ulong value1,
                                                         target_ulong value2)
 {
-    CPUState *cs;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
 
     if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
@@ -1285,9 +1281,7 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
         return H_UNSUPPORTED_FLAG;
     }
 
-    CPU_FOREACH(cs) {
-        set_spr(cs, SPR_LPCR, mflags << LPCR_AIL_SHIFT, LPCR_AIL);
-    }
+    set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL);
 
     return H_SUCCESS;
 }
@@ -1364,7 +1358,6 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
                                              target_ulong opcode,
                                              target_ulong *args)
 {
-    CPUState *cs;
     target_ulong flags = args[0];
     target_ulong proc_tbl = args[1];
     target_ulong page_size = args[2];
@@ -1422,12 +1415,9 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
     spapr->patb_entry = cproc; /* Save new process table */
 
     /* Update the UPRT and GTSE bits in the LPCR for all cpus */
-    CPU_FOREACH(cs) {
-        set_spr(cs, SPR_LPCR,
-                ((flags & (FLAG_RADIX | FLAG_HASH_PROC_TBL)) ? LPCR_UPRT : 0) |
-                ((flags & FLAG_GTSE) ? LPCR_GTSE : 0),
-                LPCR_UPRT | LPCR_GTSE);
-    }
+    set_all_lpcrs(((flags & (FLAG_RADIX | FLAG_HASH_PROC_TBL)) ? LPCR_UPRT : 0) |
+                  ((flags & FLAG_GTSE) ? LPCR_GTSE : 0),
+                  LPCR_UPRT | LPCR_GTSE);
 
     if (kvm_enabled()) {
         return kvmppc_configure_v3_mmu(cpu, flags & FLAG_RADIX,
-- 
2.17.0

[Qemu-devel] [PATCH 6/8] target/ppc: Delay initialization of LPCR_UPRT for secondary cpus

[David Gibson]

In cpu_ppc_set_papr() the UPRT and GTSE bits of the LPCR default value are
initialized based on on ppc64_radix_guest().  Which seems reasonable,
except that ppc64_radix_guest() is based on spapr->patb_entry which is
only set up in spapr_machine_reset, called _after_ cpu_ppc_set_papr() for
boot cpus.  Well, and the fact that modifying the SPR default value for an
instance rather than a class is kind of yucky.

The initialization here is really only necessary or valid for
hotplugged cpus; the base cpu initialization already sets a value
that's good enough for the boot cpus until the guest uses an hcall to
configure it's preferred MMU mode.

So, move this initialization to the rtas_start_cpu() path, at which point
ppc64_radix_guest() will have a sensible value, to make sure secondary cpus
come up in an MMU mode matching the existing cpus.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_rtas.c         | 12 ++++++++++++
 target/ppc/translate_init.c | 16 ----------------
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 840d198a8d..652233bdf1 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -47,6 +47,7 @@
 #include "trace.h"
 #include "hw/ppc/fdt.h"
 #include "target/ppc/mmu-hash64.h"
+#include "target/ppc/mmu-book3s-v3.h"
 
 static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                                    uint32_t token, uint32_t nargs,
@@ -165,6 +166,17 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
     if (!pcc->interrupts_big_endian(callcpu)) {
         lpcr |= LPCR_ILE;
     }
+    if (env->mmu_model == POWERPC_MMU_3_00) {
+        /*
+         * New cpus are expected to start in the same radix/hash mode
+         * as the existing CPUs
+         */
+        if (ppc64_radix_guest(callcpu)) {
+            lpcr |= LPCR_UPRT | LPCR_GTSE;
+        } else {
+            lpcr &= ~(LPCR_UPRT | LPCR_GTSE);
+        }
+    }
     ppc_store_lpcr(newcpu, lpcr);
 
     /*
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index 3fd380dad6..d92a84c622 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8914,22 +8914,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
     lpcr->default_value &= ~LPCR_RMLS;
     lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
 
-    if (env->mmu_model == POWERPC_MMU_3_00) {
-        /* By default we choose legacy mode and switch to new hash or radix
-         * when a register process table hcall is made. So disable process
-         * tables and guest translation shootdown by default
-         *
-         * Hot-plugged CPUs inherit from the guest radix setting under
-         * KVM but not under TCG. Update the default LPCR to keep new
-         * CPUs in sync when radix is enabled.
-         */
-        if (ppc64_radix_guest(cpu)) {
-            lpcr->default_value |= LPCR_UPRT | LPCR_GTSE;
-        } else {
-            lpcr->default_value &= ~(LPCR_UPRT | LPCR_GTSE);
-        }
-    }
-
     /* Only enable Power-saving mode Exit Cause exceptions on the boot
      * CPU. The RTAS command start-cpu will enable them on secondaries.
      */
-- 
2.17.0

[Qemu-devel] [PATCH 7/8] spapr: Move PAPR mode cpu setup fully to spapr code

[David Gibson]

cpu_ppc_set_papr() does several things:
    1) it sets up the virtual hypervisor interface
    2) it prevents the cpu from ever entering hypervisor mode
    3) it tells KVM that we're emulating a cpu in PAPR mode
and 4) it configures the LPCR and AMOR (hypervisor privileged registers)
       so that TCG will behave correctly for PAPR guests, without
       attempting to emulate the cpu in hypervisor mode

(1) & (2) make sense for any virtual hypervisor (if another one ever
exists).

(3) belongs more properly in the machine type specific to a PAPR guest, so
move it to spapr_cpu_init().  While we're at it, remove an ugly test on
kvm_enabled() by making kvmppc_set_papr() a safe no-op on non-KVM.

(4) also belongs more properly in the machine type specific code.  (4) is
done by mangling the default values of the SPRs, so that they will be set
correctly at reset time.  Manipulating usually-static parameters of the cpu
model like this is kind of ugly, especially since the values used really
have more to do with the platform than the cpu.

The spapr code already has places for PAPR specific initializations of
register state in spapr_cpu_reset(), so move this handling there.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_cpu_core.c     | 36 ++++++++++++++++++++++++++++---
 target/ppc/cpu.h            |  2 +-
 target/ppc/kvm.c            |  4 ++++
 target/ppc/translate_init.c | 42 +------------------------------------
 4 files changed, 39 insertions(+), 45 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index a98c7b04c6..a52ddade5e 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -28,6 +28,7 @@ static void spapr_cpu_reset(void *opaque)
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    target_ulong lpcr;
 
     cpu_reset(cs);
 
@@ -43,13 +44,42 @@ static void spapr_cpu_reset(void *opaque)
 
     env->spr[SPR_HIOR] = 0;
 
+    lpcr = env->spr[SPR_LPCR];
+
+    /* Set emulated LPCR to not send interrupts to hypervisor. Note that
+     * under KVM, the actual HW LPCR will be set differently by KVM itself,
+     * the settings below ensure proper operations with TCG in absence of
+     * a real hypervisor.
+     *
+     * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
+     * real mode accesses, which thankfully defaults to 0 and isn't
+     * accessible in guest mode.
+     */
+    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
+    lpcr |= LPCR_LPES0 | LPCR_LPES1;
+
+    /* Set RMLS to the max (ie, 16G) */
+    lpcr &= ~LPCR_RMLS;
+    lpcr |= 1ull << LPCR_RMLS_SHIFT;
+
+    /* Only enable Power-saving mode Exit Cause exceptions on the boot
+     * CPU. The RTAS command start-cpu will enable them on secondaries.
+     */
+    if (cs == first_cpu) {
+        lpcr |= pcc->lpcr_pm;
+    }
+
     /* Disable Power-saving mode Exit Cause exceptions for the CPU.
      * This can cause issues when rebooting the guest if a secondary
      * is awaken */
     if (cs != first_cpu) {
-        env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
+        lpcr &= ~pcc->lpcr_pm;
     }
 
+    ppc_store_lpcr(cpu, lpcr);
+
+    /* Set a full AMOR so guest can use the AMR as it sees fit */
+    env->spr[SPR_AMOR] = 0xffffffffffffffffull;
 }
 
 void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
@@ -74,8 +104,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
     /* Set time-base frequency to 512 MHz */
     cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
 
-    /* Enable PAPR mode in TCG or KVM */
-    cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
+    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
+    kvmppc_set_papr(cpu);
 
     qemu_register_reset(spapr_cpu_reset, cpu);
     spapr_cpu_reset(cpu);
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2f619f39d3..7ccd2f460e 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1332,7 +1332,7 @@ void store_booke_tcr (CPUPPCState *env, target_ulong val);
 void store_booke_tsr (CPUPPCState *env, target_ulong val);
 void ppc_tlb_invalidate_all (CPUPPCState *env);
 void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
-void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
+void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
 #endif
 #endif
 
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 6de59c5b21..28311a98e0 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2090,6 +2090,10 @@ void kvmppc_set_papr(PowerPCCPU *cpu)
     CPUState *cs = CPU(cpu);
     int ret;
 
+    if (!kvm_enabled()) {
+        return;
+    }
+
     ret = kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_PAPR, 0);
     if (ret) {
         error_report("This vCPU type or KVM version does not support PAPR");
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index d92a84c622..118631efbe 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8882,13 +8882,9 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
 }
 
 #if !defined(CONFIG_USER_ONLY)
-void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
+void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
 {
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
-    ppc_spr_t *lpcr = &env->spr_cb[SPR_LPCR];
-    ppc_spr_t *amor = &env->spr_cb[SPR_AMOR];
-    CPUState *cs = CPU(cpu);
 
     cpu->vhyp = vhyp;
 
@@ -8897,42 +8893,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
      * hypervisor mode itself
      */
     env->msr_mask &= ~MSR_HVB;
-
-    /* Set emulated LPCR to not send interrupts to hypervisor. Note that
-     * under KVM, the actual HW LPCR will be set differently by KVM itself,
-     * the settings below ensure proper operations with TCG in absence of
-     * a real hypervisor.
-     *
-     * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
-     * real mode accesses, which thankfully defaults to 0 and isn't
-     * accessible in guest mode.
-     */
-    lpcr->default_value &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
-    lpcr->default_value |= LPCR_LPES0 | LPCR_LPES1;
-
-    /* Set RMLS to the max (ie, 16G) */
-    lpcr->default_value &= ~LPCR_RMLS;
-    lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
-
-    /* Only enable Power-saving mode Exit Cause exceptions on the boot
-     * CPU. The RTAS command start-cpu will enable them on secondaries.
-     */
-    if (cs == first_cpu) {
-        lpcr->default_value |= pcc->lpcr_pm;
-    }
-
-    /* We should be followed by a CPU reset but update the active value
-     * just in case...
-     */
-    ppc_store_lpcr(cpu, lpcr->default_value);
-
-    /* Set a full AMOR so guest can use the AMR as it sees fit */
-    env->spr[SPR_AMOR] = amor->default_value = 0xffffffffffffffffull;
-
-    /* Tell KVM that we're in PAPR mode */
-    if (kvm_enabled()) {
-        kvmppc_set_papr(cpu);
-    }
 }
 
 #endif /* !defined(CONFIG_USER_ONLY) */
-- 
2.17.0

[Qemu-devel] [PATCH 8/8] spapr: Clean up handling of LPCR power-saving exit bits

[David Gibson]

To prevent spurious wakeups on cpus that are supposed to be disabled, we
need to clear the LPCR bits which control certain wakeup events.
spapr_cpu_reset() has separate cases here for boot and non-boot (initially
inactive) cpus.  rtas_start_cpu() then turns the LPCR bits on when the
non-boot cpus are activated.

But explicit checks against first_cpu are not how we usually do things:
instead spapr_cpu_reset() generally sets things up for non-boot (inactive)
cpus, then spapr_machine_reset() and/or rtas_start_cpu() override as
necessary.

So, do that instead.  Because the LPCR activation is identical for boot
cpus and non-boot cpus just activated with rtas_start_cpu() we can put the
code common in spapr_cpu_set_entry_state().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_cpu_core.c | 22 +++++++---------------
 hw/ppc/spapr_rtas.c     |  2 +-
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index a52ddade5e..f3e9b879b2 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -54,28 +54,17 @@ static void spapr_cpu_reset(void *opaque)
      * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
      * real mode accesses, which thankfully defaults to 0 and isn't
      * accessible in guest mode.
+     *
+     * Disable Power-saving mode Exit Cause exceptions for the CPU, so
+     * we don't get spurious wakups before an RTAS start-cpu call.
      */
-    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
+    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV | pcc->lpcr_pm);
     lpcr |= LPCR_LPES0 | LPCR_LPES1;
 
     /* Set RMLS to the max (ie, 16G) */
     lpcr &= ~LPCR_RMLS;
     lpcr |= 1ull << LPCR_RMLS_SHIFT;
 
-    /* Only enable Power-saving mode Exit Cause exceptions on the boot
-     * CPU. The RTAS command start-cpu will enable them on secondaries.
-     */
-    if (cs == first_cpu) {
-        lpcr |= pcc->lpcr_pm;
-    }
-
-    /* Disable Power-saving mode Exit Cause exceptions for the CPU.
-     * This can cause issues when rebooting the guest if a secondary
-     * is awaken */
-    if (cs != first_cpu) {
-        lpcr &= ~pcc->lpcr_pm;
-    }
-
     ppc_store_lpcr(cpu, lpcr);
 
     /* Set a full AMOR so guest can use the AMR as it sees fit */
@@ -84,11 +73,14 @@ static void spapr_cpu_reset(void *opaque)
 
 void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
 {
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
 
     env->nip = nip;
     env->gpr[3] = r3;
     CPU(cpu)->halted = 0;
+    /* Enable Power-saving mode Exit Cause exceptions */
+    ppc_store_lpcr(cpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
 }
 
 static void spapr_cpu_destroy(PowerPCCPU *cpu)
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 652233bdf1..7f9738daed 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -162,7 +162,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
     env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
 
     /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
-    lpcr = env->spr[SPR_LPCR] | pcc->lpcr_pm;
+    lpcr = env->spr[SPR_LPCR];
     if (!pcc->interrupts_big_endian(callcpu)) {
         lpcr |= LPCR_ILE;
     }
-- 
2.17.0

Re: [Qemu-devel] [PATCH 1/8] target/ppc: Add ppc_store_lpcr() helper

[Cédric Le Goater]

On 05/03/2018 08:21 AM, David Gibson wrote:
> There are some fields in the cpu state which need to be updated when the
> LPCR register is changed, which is done by ppc_hash64_update_rmls() and
> ppc_hash64_update_vrma().  Code which alters env->spr[SPR_LPCR] needs to
> call them afterwards to make sure the state is up to date.
> 
> That's easy to get wrong.  The normal way of dealing with sitautions like
> that is to use a helper which both updates the basic register value and the
> derived state.
> 
> So, do that.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Although, I am not sure mmu-hash64.c is the right file for the ppc_store_lpcr()
helper. This is minor.

C.

> ---
>  target/ppc/mmu-hash64.c     | 15 +++++++++++----
>  target/ppc/mmu-hash64.h     |  3 +--
>  target/ppc/translate_init.c |  6 +-----
>  3 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 7e0adecfd9..a1db20e3a8 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -942,7 +942,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
>      cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
>  }
>  
> -void ppc_hash64_update_rmls(PowerPCCPU *cpu)
> +static void ppc_hash64_update_rmls(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
>      uint64_t lpcr = env->spr[SPR_LPCR];
> @@ -977,7 +977,7 @@ void ppc_hash64_update_rmls(PowerPCCPU *cpu)
>      }
>  }
>  
> -void ppc_hash64_update_vrma(PowerPCCPU *cpu)
> +static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
>      const PPCHash64SegmentPageSizes *sps = NULL;
> @@ -1028,9 +1028,9 @@ void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>      slb->sps = sps;
>  }
>  
> -void helper_store_lpcr(CPUPPCState *env, target_ulong val)
> +void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>  {
> -    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    CPUPPCState *env = &cpu->env;
>      uint64_t lpcr = 0;
>  
>      /* Filter out bits */
> @@ -1096,6 +1096,13 @@ void helper_store_lpcr(CPUPPCState *env, target_ulong val)
>      ppc_hash64_update_vrma(cpu);
>  }
>  
> +void helper_store_lpcr(CPUPPCState *env, target_ulong val)
> +{
> +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +
> +    ppc_store_lpcr(cpu, val);
> +}
> +
>  void ppc_hash64_init(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index f6349ccdb3..53dcec5b93 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -17,8 +17,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
>                                 target_ulong pte0, target_ulong pte1);
>  unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
>                                            uint64_t pte0, uint64_t pte1);
> -void ppc_hash64_update_vrma(PowerPCCPU *cpu);
> -void ppc_hash64_update_rmls(PowerPCCPU *cpu);
> +void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val);
>  void ppc_hash64_init(PowerPCCPU *cpu);
>  void ppc_hash64_finalize(PowerPCCPU *cpu);
>  #endif
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index c83c910a29..3fd380dad6 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8940,15 +8940,11 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>      /* We should be followed by a CPU reset but update the active value
>       * just in case...
>       */
> -    env->spr[SPR_LPCR] = lpcr->default_value;
> +    ppc_store_lpcr(cpu, lpcr->default_value);
>  
>      /* Set a full AMOR so guest can use the AMR as it sees fit */
>      env->spr[SPR_AMOR] = amor->default_value = 0xffffffffffffffffull;
>  
> -    /* Update some env bits based on new LPCR value */
> -    ppc_hash64_update_rmls(cpu);
> -    ppc_hash64_update_vrma(cpu);
> -
>      /* Tell KVM that we're in PAPR mode */
>      if (kvm_enabled()) {
>          kvmppc_set_papr(cpu);
> 

Re: [Qemu-devel] [PATCH 2/8] spapr: Clean up rtas_start_cpu() & rtas_stop_self()

[Cédric Le Goater]

On 05/03/2018 08:21 AM, David Gibson wrote:
> This makes several minor cleanups to these functions:
>   * Follow usual convention of an early exit on error, rather than having
>     most of the body in an if
>   * Clearer naming of cpu and cpu_.  Now callcpu is the cpu from which the
>     RTAS call is invoked, newcpu is the cpu which we're starting
>   * Use cpu_synchronize_state() instead of kvm_cpu_synchronize_state()
>     directly
>   * Remove pointless comment describing what cpu_synchronize_state() does
>   * Use ppc_store_lpcr() instead of directly writing the register field
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/spapr_rtas.c | 66 ++++++++++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 0ec5fa4cfe..b251c130cb 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -32,7 +32,7 @@
>  #include "hw/qdev.h"
>  #include "sysemu/device_tree.h"
>  #include "sysemu/cpus.h"
> -#include "sysemu/kvm.h"
> +#include "sysemu/hw_accel.h"
>  
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/spapr_vio.h"
> @@ -45,6 +45,7 @@
>  #include "qemu/cutils.h"
>  #include "trace.h"
>  #include "hw/ppc/fdt.h"
> +#include "target/ppc/mmu-hash64.h"
>  
>  static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                                     uint32_t token, uint32_t nargs,
> @@ -140,13 +141,15 @@ static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
>      }
>  }
>  
> -static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> +static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
>                             uint32_t token, uint32_t nargs,
>                             target_ulong args,
>                             uint32_t nret, target_ulong rets)
>  {
>      target_ulong id, start, r3;
> -    PowerPCCPU *cpu;
> +    PowerPCCPU *newcpu;
> +    CPUPPCState *env;
> +    PowerPCCPUClass *pcc;
>  
>      if (nargs != 3 || nret != 1) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -157,41 +160,37 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
>      start = rtas_ld(args, 1);
>      r3 = rtas_ld(args, 2);
>  
> -    cpu = spapr_find_cpu(id);
> -    if (cpu != NULL) {
> -        CPUState *cs = CPU(cpu);
> -        CPUPPCState *env = &cpu->env;
> -        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    newcpu = spapr_find_cpu(id);
> +    if (!newcpu) {
> +        /* Didn't find a matching cpu */
> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +        return;
> +    }
>  
> -        if (!cs->halted) {
> -            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> -            return;
> -        }
> +    env = &newcpu->env;
> +    pcc = POWERPC_CPU_GET_CLASS(newcpu);
>  
> -        /* This will make sure qemu state is up to date with kvm, and
> -         * mark it dirty so our changes get flushed back before the
> -         * new cpu enters */
> -        kvm_cpu_synchronize_state(cs);
> +    if (!CPU(newcpu)->halted) {
> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> +        return;
> +    }
>  
> -        env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> +    cpu_synchronize_state(CPU(newcpu));
>  
> -        /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> -        env->spr[SPR_LPCR] |= pcc->lpcr_pm;
> +    env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> +    spapr_cpu_set_endianness(newcpu);
> +    spapr_cpu_update_tb_offset(newcpu);
> +    /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> +    ppc_store_lpcr(newcpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
>  
> -        env->nip = start;
> -        env->gpr[3] = r3;
> -        cs->halted = 0;
> -        spapr_cpu_set_endianness(cpu);
> -        spapr_cpu_update_tb_offset(cpu);
> +    env->nip = start;
> +    env->gpr[3] = r3;
>  
> -        qemu_cpu_kick(cs);
> +    CPU(newcpu)->halted = 0;
>  
> -        rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> -        return;
> -    }
> +    qemu_cpu_kick(CPU(newcpu));
>  
> -    /* Didn't find a matching cpu */
> -    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
>  
>  static void rtas_stop_self(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> @@ -203,13 +202,12 @@ static void rtas_stop_self(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>  
> -    cs->halted = 1;
> -    qemu_cpu_kick(cs);
> -
>      /* Disable Power-saving mode Exit Cause exceptions for the CPU.
>       * This could deliver an interrupt on a dying CPU and crash the
>       * guest */
> -    env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
> +    ppc_store_lpcr(cpu, env->spr[SPR_LPCR] & ~pcc->lpcr_pm);
> +    cs->halted = 1;
> +    qemu_cpu_kick(cs);
>  }
>  
>  static inline int sysparm_st(target_ulong addr, target_ulong len,
> 

Re: [Qemu-devel] [PATCH 1/8] target/ppc: Add ppc_store_lpcr() helper

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]

On Thu, May 03, 2018 at 09:06:42AM +0200, Cédric Le Goater wrote:
> On 05/03/2018 08:21 AM, David Gibson wrote:
> > There are some fields in the cpu state which need to be updated when the
> > LPCR register is changed, which is done by ppc_hash64_update_rmls() and
> > ppc_hash64_update_vrma().  Code which alters env->spr[SPR_LPCR] needs to
> > call them afterwards to make sure the state is up to date.
> > 
> > That's easy to get wrong.  The normal way of dealing with sitautions like
> > that is to use a helper which both updates the basic register value and the
> > derived state.
> > 
> > So, do that.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Although, I am not sure mmu-hash64.c is the right file for the ppc_store_lpcr()
> helper. This is minor.

It really isn't.  I looked at moving it, but since the code we're
rearranging is already in that file it just made things messier.
Problem for another day.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/8] spapr: Remove unhelpful helpers from rtas_start_cpu()

[Cédric Le Goater]

On 05/03/2018 08:21 AM, David Gibson wrote:
> rtas_start_cpu() calls spapr_cpu_update_tb_offset() and
> spapr_cpu_set_endianness() to initialize certain things in the new cpu's
> state.  This is the only caller of those helpers, and they're each only
> a few lines long, so we might as well just fold them into the caller.
> 
> In addition, those helpers initialize state on the new cpu to match that of
> the first cpu.  That will generally work, but might be at least logically
> incorrect if the first cpu has been set offline by the guest.  So, instead
> base the state on that of the cpu invoking the RTAS call, which is
> obviously active already.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/spapr_rtas.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index b251c130cb..df073447c5 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -120,27 +120,6 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
>      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>  }
>  
> -/*
> - * Set the timebase offset of the CPU to that of first CPU.
> - * This helps hotplugged CPU to have the correct timebase offset.
> - */
> -static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
> -{
> -    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> -
> -    cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
> -}
> -
> -static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> -{
> -    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
> -
> -    if (!pcc->interrupts_big_endian(fcpu)) {
> -        cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
> -    }
> -}
> -
>  static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
>                             uint32_t token, uint32_t nargs,
>                             target_ulong args,
> @@ -150,6 +129,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
>      PowerPCCPU *newcpu;
>      CPUPPCState *env;
>      PowerPCCPUClass *pcc;
> +    target_ulong lpcr;
>  
>      if (nargs != 3 || nret != 1) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -178,10 +158,20 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
>      cpu_synchronize_state(CPU(newcpu));
>  
>      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> -    spapr_cpu_set_endianness(newcpu);
> -    spapr_cpu_update_tb_offset(newcpu);
> +
>      /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> -    ppc_store_lpcr(newcpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
> +    lpcr = env->spr[SPR_LPCR] | pcc->lpcr_pm;
> +    if (!pcc->interrupts_big_endian(callcpu)) {
> +        lpcr |= LPCR_ILE;
> +    }
> +    ppc_store_lpcr(newcpu, lpcr);
> +
> +    /*
> +     * Set the timebase offset of the new CPU to that of the invoking
> +     * CPU.  This helps hotplugged CPU to have the correct timebase
> +     * offset.
> +     */
> +    newcpu->env.tb_env->tb_offset = callcpu->env.tb_env->tb_offset;
>  
>      env->nip = start;
>      env->gpr[3] = r3;
> 

Re: [Qemu-devel] [PATCH 5/8] spapr: Clean up LPCR updates from hypercalls

[Cédric Le Goater]

On 05/03/2018 08:21 AM, David Gibson wrote:
> There are several places in spapr_hcall.c where we need to update the LPCR
> value on all CPUs.  We do this with the set_spr() helper.  That's not
> really correct because this directly sets the SPR value, without going
> through the ppc_store_lpcr() helper which may need to update state based
> on the LPCR change.
> 
> In fact, set_spr() is only ever used for the LPCR, so replace it with an
> explicit LPCR updated which uses the right low-level helper.  While we're
> there, move the CPU_FOREACH() which was in every one of the callers into
> the new helper: set_all_lpcrs().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/spapr_hcall.c | 50 ++++++++++++++++++--------------------------
>  1 file changed, 20 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 16bccdd5c0..ca9702e667 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -15,32 +15,35 @@
>  #include "hw/ppc/spapr_ovec.h"
>  #include "mmu-book3s-v3.h"
>  
> -struct SPRSyncState {
> -    int spr;
> +struct LPCRSyncState {
>      target_ulong value;
>      target_ulong mask;
>  };
>  
> -static void do_spr_sync(CPUState *cs, run_on_cpu_data arg)
> +static void do_lpcr_sync(CPUState *cs, run_on_cpu_data arg)
>  {
> -    struct SPRSyncState *s = arg.host_ptr;
> +    struct LPCRSyncState *s = arg.host_ptr;
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
> +    target_ulong lpcr;
>  
>      cpu_synchronize_state(cs);
> -    env->spr[s->spr] &= ~s->mask;
> -    env->spr[s->spr] |= s->value;
> +    lpcr = env->spr[SPR_LPCR];
> +    lpcr &= ~s->mask;
> +    lpcr |= s->value;
> +    ppc_store_lpcr(cpu, lpcr);
>  }
>  
> -static void set_spr(CPUState *cs, int spr, target_ulong value,
> -                    target_ulong mask)
> +static void set_all_lpcrs(target_ulong value, target_ulong mask)
>  {
> -    struct SPRSyncState s = {
> -        .spr = spr,
> +    CPUState *cs;
> +    struct LPCRSyncState s = {
>          .value = value,
>          .mask = mask
>      };
> -    run_on_cpu(cs, do_spr_sync, RUN_ON_CPU_HOST_PTR(&s));
> +    CPU_FOREACH(cs) {
> +        run_on_cpu(cs, do_lpcr_sync, RUN_ON_CPU_HOST_PTR(&s));
> +    }
>  }
>  
>  static bool has_spr(PowerPCCPU *cpu, int spr)
> @@ -1235,8 +1238,6 @@ static target_ulong h_set_mode_resource_le(PowerPCCPU *cpu,
>                                             target_ulong value1,
>                                             target_ulong value2)
>  {
> -    CPUState *cs;
> -
>      if (value1) {
>          return H_P3;
>      }
> @@ -1246,16 +1247,12 @@ static target_ulong h_set_mode_resource_le(PowerPCCPU *cpu,
>  
>      switch (mflags) {
>      case H_SET_MODE_ENDIAN_BIG:
> -        CPU_FOREACH(cs) {
> -            set_spr(cs, SPR_LPCR, 0, LPCR_ILE);
> -        }
> +        set_all_lpcrs(0, LPCR_ILE);
>          spapr_pci_switch_vga(true);
>          return H_SUCCESS;
>  
>      case H_SET_MODE_ENDIAN_LITTLE:
> -        CPU_FOREACH(cs) {
> -            set_spr(cs, SPR_LPCR, LPCR_ILE, LPCR_ILE);
> -        }
> +        set_all_lpcrs(LPCR_ILE, LPCR_ILE);
>          spapr_pci_switch_vga(false);
>          return H_SUCCESS;
>      }
> @@ -1268,7 +1265,6 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>                                                          target_ulong value1,
>                                                          target_ulong value2)
>  {
> -    CPUState *cs;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>  
>      if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
> @@ -1285,9 +1281,7 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>          return H_UNSUPPORTED_FLAG;
>      }
>  
> -    CPU_FOREACH(cs) {
> -        set_spr(cs, SPR_LPCR, mflags << LPCR_AIL_SHIFT, LPCR_AIL);
> -    }
> +    set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL);
>  
>      return H_SUCCESS;
>  }
> @@ -1364,7 +1358,6 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
>                                               target_ulong opcode,
>                                               target_ulong *args)
>  {
> -    CPUState *cs;
>      target_ulong flags = args[0];
>      target_ulong proc_tbl = args[1];
>      target_ulong page_size = args[2];
> @@ -1422,12 +1415,9 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
>      spapr->patb_entry = cproc; /* Save new process table */
>  
>      /* Update the UPRT and GTSE bits in the LPCR for all cpus */
> -    CPU_FOREACH(cs) {
> -        set_spr(cs, SPR_LPCR,
> -                ((flags & (FLAG_RADIX | FLAG_HASH_PROC_TBL)) ? LPCR_UPRT : 0) |
> -                ((flags & FLAG_GTSE) ? LPCR_GTSE : 0),
> -                LPCR_UPRT | LPCR_GTSE);
> -    }
> +    set_all_lpcrs(((flags & (FLAG_RADIX | FLAG_HASH_PROC_TBL)) ? LPCR_UPRT : 0) |
> +                  ((flags & FLAG_GTSE) ? LPCR_GTSE : 0),
> +                  LPCR_UPRT | LPCR_GTSE);
>  
>      if (kvm_enabled()) {
>          return kvmppc_configure_v3_mmu(cpu, flags & FLAG_RADIX,
> 

Re: [Qemu-devel] [PATCH 6/8] target/ppc: Delay initialization of LPCR_UPRT for secondary cpus

[Cédric Le Goater]

On 05/03/2018 08:21 AM, David Gibson wrote:
> In cpu_ppc_set_papr() the UPRT and GTSE bits of the LPCR default value are
> initialized based on on ppc64_radix_guest().  Which seems reasonable,
> except that ppc64_radix_guest() is based on spapr->patb_entry which is
> only set up in spapr_machine_reset, called _after_ cpu_ppc_set_papr() for
> boot cpus.  Well, and the fact that modifying the SPR default value for an
> instance rather than a class is kind of yucky.
> 
> The initialization here is really only necessary or valid for
> hotplugged cpus; the base cpu initialization already sets a value
> that's good enough for the boot cpus until the guest uses an hcall to
> configure it's preferred MMU mode.
> 
> So, move this initialization to the rtas_start_cpu() path, at which point
> ppc64_radix_guest() will have a sensible value, to make sure secondary cpus
> come up in an MMU mode matching the existing cpus.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ppc/spapr_rtas.c         | 12 ++++++++++++
>  target/ppc/translate_init.c | 16 ----------------
>  2 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 840d198a8d..652233bdf1 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -47,6 +47,7 @@
>  #include "trace.h"
>  #include "hw/ppc/fdt.h"
>  #include "target/ppc/mmu-hash64.h"
> +#include "target/ppc/mmu-book3s-v3.h"
>  
>  static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                                     uint32_t token, uint32_t nargs,
> @@ -165,6 +166,17 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
>      if (!pcc->interrupts_big_endian(callcpu)) {
>          lpcr |= LPCR_ILE;
>      }
> +    if (env->mmu_model == POWERPC_MMU_3_00) {
> +        /*
> +         * New cpus are expected to start in the same radix/hash mode
> +         * as the existing CPUs
> +         */
> +        if (ppc64_radix_guest(callcpu)) {
> +            lpcr |= LPCR_UPRT | LPCR_GTSE;
> +        } else {
> +            lpcr &= ~(LPCR_UPRT | LPCR_GTSE);
> +        }
> +    }
>      ppc_store_lpcr(newcpu, lpcr);
>  
>      /*
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 3fd380dad6..d92a84c622 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8914,22 +8914,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>      lpcr->default_value &= ~LPCR_RMLS;
>      lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
>  
> -    if (env->mmu_model == POWERPC_MMU_3_00) {
> -        /* By default we choose legacy mode and switch to new hash or radix
> -         * when a register process table hcall is made. So disable process
> -         * tables and guest translation shootdown by default
> -         *
> -         * Hot-plugged CPUs inherit from the guest radix setting under
> -         * KVM but not under TCG. Update the default LPCR to keep new
> -         * CPUs in sync when radix is enabled.
> -         */
> -        if (ppc64_radix_guest(cpu)) {
> -            lpcr->default_value |= LPCR_UPRT | LPCR_GTSE;
> -        } else {
> -            lpcr->default_value &= ~(LPCR_UPRT | LPCR_GTSE);
> -        }
> -    }
> -
>      /* Only enable Power-saving mode Exit Cause exceptions on the boot
>       * CPU. The RTAS command start-cpu will enable them on secondaries.
>       */
> 

Re: [Qemu-devel] [PATCH 7/8] spapr: Move PAPR mode cpu setup fully to spapr code

[Cédric Le Goater]

On 05/03/2018 08:21 AM, David Gibson wrote:
> cpu_ppc_set_papr() does several things:
>     1) it sets up the virtual hypervisor interface
>     2) it prevents the cpu from ever entering hypervisor mode
>     3) it tells KVM that we're emulating a cpu in PAPR mode
> and 4) it configures the LPCR and AMOR (hypervisor privileged registers)
>        so that TCG will behave correctly for PAPR guests, without
>        attempting to emulate the cpu in hypervisor mode
> 
> (1) & (2) make sense for any virtual hypervisor (if another one ever
> exists).
> 
> (3) belongs more properly in the machine type specific to a PAPR guest, so
> move it to spapr_cpu_init().  While we're at it, remove an ugly test on
> kvm_enabled() by making kvmppc_set_papr() a safe no-op on non-KVM.
> 
> (4) also belongs more properly in the machine type specific code.  (4) is
> done by mangling the default values of the SPRs, so that they will be set
> correctly at reset time.  Manipulating usually-static parameters of the cpu
> model like this is kind of ugly, especially since the values used really
> have more to do with the platform than the cpu.
> 
> The spapr code already has places for PAPR specific initializations of
> register state in spapr_cpu_reset(), so move this handling there.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/spapr_cpu_core.c     | 36 ++++++++++++++++++++++++++++---
>  target/ppc/cpu.h            |  2 +-
>  target/ppc/kvm.c            |  4 ++++
>  target/ppc/translate_init.c | 42 +------------------------------------
>  4 files changed, 39 insertions(+), 45 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index a98c7b04c6..a52ddade5e 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -28,6 +28,7 @@ static void spapr_cpu_reset(void *opaque)
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    target_ulong lpcr;
>  
>      cpu_reset(cs);
>  
> @@ -43,13 +44,42 @@ static void spapr_cpu_reset(void *opaque)
>  
>      env->spr[SPR_HIOR] = 0;
>  
> +    lpcr = env->spr[SPR_LPCR];
> +
> +    /* Set emulated LPCR to not send interrupts to hypervisor. Note that
> +     * under KVM, the actual HW LPCR will be set differently by KVM itself,
> +     * the settings below ensure proper operations with TCG in absence of
> +     * a real hypervisor.
> +     *
> +     * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
> +     * real mode accesses, which thankfully defaults to 0 and isn't
> +     * accessible in guest mode.
> +     */
> +    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
> +    lpcr |= LPCR_LPES0 | LPCR_LPES1;
> +
> +    /* Set RMLS to the max (ie, 16G) */
> +    lpcr &= ~LPCR_RMLS;
> +    lpcr |= 1ull << LPCR_RMLS_SHIFT;
> +
> +    /* Only enable Power-saving mode Exit Cause exceptions on the boot
> +     * CPU. The RTAS command start-cpu will enable them on secondaries.
> +     */
> +    if (cs == first_cpu) {
> +        lpcr |= pcc->lpcr_pm;
> +    }
> +
>      /* Disable Power-saving mode Exit Cause exceptions for the CPU.
>       * This can cause issues when rebooting the guest if a secondary
>       * is awaken */
>      if (cs != first_cpu) {
> -        env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
> +        lpcr &= ~pcc->lpcr_pm;
>      }
>  
> +    ppc_store_lpcr(cpu, lpcr);
> +
> +    /* Set a full AMOR so guest can use the AMR as it sees fit */
> +    env->spr[SPR_AMOR] = 0xffffffffffffffffull;
>  }
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> @@ -74,8 +104,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>      /* Set time-base frequency to 512 MHz */
>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>  
> -    /* Enable PAPR mode in TCG or KVM */
> -    cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> +    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> +    kvmppc_set_papr(cpu);
>  
>      qemu_register_reset(spapr_cpu_reset, cpu);
>      spapr_cpu_reset(cpu);
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 2f619f39d3..7ccd2f460e 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1332,7 +1332,7 @@ void store_booke_tcr (CPUPPCState *env, target_ulong val);
>  void store_booke_tsr (CPUPPCState *env, target_ulong val);
>  void ppc_tlb_invalidate_all (CPUPPCState *env);
>  void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
> -void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
> +void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
>  #endif
>  #endif
>  
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 6de59c5b21..28311a98e0 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2090,6 +2090,10 @@ void kvmppc_set_papr(PowerPCCPU *cpu)
>      CPUState *cs = CPU(cpu);
>      int ret;
>  
> +    if (!kvm_enabled()) {
> +        return;
> +    }
> +
>      ret = kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_PAPR, 0);
>      if (ret) {
>          error_report("This vCPU type or KVM version does not support PAPR");
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index d92a84c622..118631efbe 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8882,13 +8882,9 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
> +void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>  {
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
> -    ppc_spr_t *lpcr = &env->spr_cb[SPR_LPCR];
> -    ppc_spr_t *amor = &env->spr_cb[SPR_AMOR];
> -    CPUState *cs = CPU(cpu);
>  
>      cpu->vhyp = vhyp;
>  
> @@ -8897,42 +8893,6 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>       * hypervisor mode itself
>       */
>      env->msr_mask &= ~MSR_HVB;
> -
> -    /* Set emulated LPCR to not send interrupts to hypervisor. Note that
> -     * under KVM, the actual HW LPCR will be set differently by KVM itself,
> -     * the settings below ensure proper operations with TCG in absence of
> -     * a real hypervisor.
> -     *
> -     * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
> -     * real mode accesses, which thankfully defaults to 0 and isn't
> -     * accessible in guest mode.
> -     */
> -    lpcr->default_value &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
> -    lpcr->default_value |= LPCR_LPES0 | LPCR_LPES1;
> -
> -    /* Set RMLS to the max (ie, 16G) */
> -    lpcr->default_value &= ~LPCR_RMLS;
> -    lpcr->default_value |= 1ull << LPCR_RMLS_SHIFT;
> -
> -    /* Only enable Power-saving mode Exit Cause exceptions on the boot
> -     * CPU. The RTAS command start-cpu will enable them on secondaries.
> -     */
> -    if (cs == first_cpu) {
> -        lpcr->default_value |= pcc->lpcr_pm;
> -    }
> -
> -    /* We should be followed by a CPU reset but update the active value
> -     * just in case...
> -     */
> -    ppc_store_lpcr(cpu, lpcr->default_value);
> -
> -    /* Set a full AMOR so guest can use the AMR as it sees fit */
> -    env->spr[SPR_AMOR] = amor->default_value = 0xffffffffffffffffull;
> -
> -    /* Tell KVM that we're in PAPR mode */
> -    if (kvm_enabled()) {
> -        kvmppc_set_papr(cpu);
> -    }
>  }
>  
>  #endif /* !defined(CONFIG_USER_ONLY) */
> 

Re: [Qemu-devel] [PATCH 8/8] spapr: Clean up handling of LPCR power-saving exit bits

[Cédric Le Goater]

On 05/03/2018 08:21 AM, David Gibson wrote:
> To prevent spurious wakeups on cpus that are supposed to be disabled, we
> need to clear the LPCR bits which control certain wakeup events.
> spapr_cpu_reset() has separate cases here for boot and non-boot (initially
> inactive) cpus.  rtas_start_cpu() then turns the LPCR bits on when the
> non-boot cpus are activated.
> 
> But explicit checks against first_cpu are not how we usually do things:
> instead spapr_cpu_reset() generally sets things up for non-boot (inactive)
> cpus, then spapr_machine_reset() and/or rtas_start_cpu() override as
> necessary.
> 
> So, do that instead.  Because the LPCR activation is identical for boot
> cpus and non-boot cpus just activated with rtas_start_cpu() we can put the
> code common in spapr_cpu_set_entry_state().

This is much nicer.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

and for the patchset :

Tested-by: Cédric Le Goater <clg@kaod.org>

under KVM & TCG with cpu hotplug and unplug.


There is still a spapr_cpu_reset() call under spapr_cpu_init().
Is that on purpose ? 

Thanks,

C.

> ---
>  hw/ppc/spapr_cpu_core.c | 22 +++++++---------------
>  hw/ppc/spapr_rtas.c     |  2 +-
>  2 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index a52ddade5e..f3e9b879b2 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -54,28 +54,17 @@ static void spapr_cpu_reset(void *opaque)
>       * Clearing VPM0 will also cause us to use RMOR in mmu-hash64.c for
>       * real mode accesses, which thankfully defaults to 0 and isn't
>       * accessible in guest mode.
> +     *
> +     * Disable Power-saving mode Exit Cause exceptions for the CPU, so
> +     * we don't get spurious wakups before an RTAS start-cpu call.
>       */
> -    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV);
> +    lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV | pcc->lpcr_pm);
>      lpcr |= LPCR_LPES0 | LPCR_LPES1;
>  
>      /* Set RMLS to the max (ie, 16G) */
>      lpcr &= ~LPCR_RMLS;
>      lpcr |= 1ull << LPCR_RMLS_SHIFT;
>  
> -    /* Only enable Power-saving mode Exit Cause exceptions on the boot
> -     * CPU. The RTAS command start-cpu will enable them on secondaries.
> -     */
> -    if (cs == first_cpu) {
> -        lpcr |= pcc->lpcr_pm;
> -    }
> -
> -    /* Disable Power-saving mode Exit Cause exceptions for the CPU.
> -     * This can cause issues when rebooting the guest if a secondary
> -     * is awaken */
> -    if (cs != first_cpu) {
> -        lpcr &= ~pcc->lpcr_pm;
> -    }
> -
>      ppc_store_lpcr(cpu, lpcr);
>  
>      /* Set a full AMOR so guest can use the AMR as it sees fit */
> @@ -84,11 +73,14 @@ static void spapr_cpu_reset(void *opaque)
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
>  {
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
>  
>      env->nip = nip;
>      env->gpr[3] = r3;
>      CPU(cpu)->halted = 0;
> +    /* Enable Power-saving mode Exit Cause exceptions */
> +    ppc_store_lpcr(cpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
>  }
>  
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 652233bdf1..7f9738daed 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -162,7 +162,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
>      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>  
>      /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> -    lpcr = env->spr[SPR_LPCR] | pcc->lpcr_pm;
> +    lpcr = env->spr[SPR_LPCR];
>      if (!pcc->interrupts_big_endian(callcpu)) {
>          lpcr |= LPCR_ILE;
>      }
> 

Re: [Qemu-devel] [PATCH 8/8] spapr: Clean up handling of LPCR power-saving exit bits

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

On Thu, May 03, 2018 at 09:47:18AM +0200, Cédric Le Goater wrote:
> On 05/03/2018 08:21 AM, David Gibson wrote:
> > To prevent spurious wakeups on cpus that are supposed to be disabled, we
> > need to clear the LPCR bits which control certain wakeup events.
> > spapr_cpu_reset() has separate cases here for boot and non-boot (initially
> > inactive) cpus.  rtas_start_cpu() then turns the LPCR bits on when the
> > non-boot cpus are activated.
> > 
> > But explicit checks against first_cpu are not how we usually do things:
> > instead spapr_cpu_reset() generally sets things up for non-boot (inactive)
> > cpus, then spapr_machine_reset() and/or rtas_start_cpu() override as
> > necessary.
> > 
> > So, do that instead.  Because the LPCR activation is identical for boot
> > cpus and non-boot cpus just activated with rtas_start_cpu() we can put the
> > code common in spapr_cpu_set_entry_state().
> 
> This is much nicer.
> 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> and for the patchset :
> 
> Tested-by: Cédric Le Goater <clg@kaod.org>
> 
> under KVM & TCG with cpu hotplug and unplug.

Thanks.  I've folded the series into ppc-for-2.13.

> There is still a spapr_cpu_reset() call under spapr_cpu_init().
> Is that on purpose ?

Sort of.  I'd still like to remove it, but figuring out how to do so
safely is going to take a bit longer.  Well, probably quite a lot
longer, since I doubt I'll get it done before I go away.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/8] target/ppc: Add ppc_store_lpcr() helper

[Greg Kurz]

On Thu,  3 May 2018 16:21:38 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> There are some fields in the cpu state which need to be updated when the
> LPCR register is changed, which is done by ppc_hash64_update_rmls() and
> ppc_hash64_update_vrma().  Code which alters env->spr[SPR_LPCR] needs to
> call them afterwards to make sure the state is up to date.
> 
> That's easy to get wrong.  The normal way of dealing with sitautions like
> that is to use a helper which both updates the basic register value and the
> derived state.
> 
> So, do that.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  target/ppc/mmu-hash64.c     | 15 +++++++++++----
>  target/ppc/mmu-hash64.h     |  3 +--
>  target/ppc/translate_init.c |  6 +-----
>  3 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 7e0adecfd9..a1db20e3a8 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -942,7 +942,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
>      cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
>  }
>  
> -void ppc_hash64_update_rmls(PowerPCCPU *cpu)
> +static void ppc_hash64_update_rmls(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
>      uint64_t lpcr = env->spr[SPR_LPCR];
> @@ -977,7 +977,7 @@ void ppc_hash64_update_rmls(PowerPCCPU *cpu)
>      }
>  }
>  
> -void ppc_hash64_update_vrma(PowerPCCPU *cpu)
> +static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
>      const PPCHash64SegmentPageSizes *sps = NULL;
> @@ -1028,9 +1028,9 @@ void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>      slb->sps = sps;
>  }
>  
> -void helper_store_lpcr(CPUPPCState *env, target_ulong val)
> +void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>  {
> -    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    CPUPPCState *env = &cpu->env;
>      uint64_t lpcr = 0;
>  
>      /* Filter out bits */
> @@ -1096,6 +1096,13 @@ void helper_store_lpcr(CPUPPCState *env, target_ulong val)
>      ppc_hash64_update_vrma(cpu);
>  }
>  
> +void helper_store_lpcr(CPUPPCState *env, target_ulong val)
> +{
> +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +
> +    ppc_store_lpcr(cpu, val);
> +}
> +
>  void ppc_hash64_init(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index f6349ccdb3..53dcec5b93 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -17,8 +17,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
>                                 target_ulong pte0, target_ulong pte1);
>  unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
>                                            uint64_t pte0, uint64_t pte1);
> -void ppc_hash64_update_vrma(PowerPCCPU *cpu);
> -void ppc_hash64_update_rmls(PowerPCCPU *cpu);
> +void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val);
>  void ppc_hash64_init(PowerPCCPU *cpu);
>  void ppc_hash64_finalize(PowerPCCPU *cpu);
>  #endif
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index c83c910a29..3fd380dad6 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8940,15 +8940,11 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp)
>      /* We should be followed by a CPU reset but update the active value
>       * just in case...
>       */
> -    env->spr[SPR_LPCR] = lpcr->default_value;
> +    ppc_store_lpcr(cpu, lpcr->default_value);
>  
>      /* Set a full AMOR so guest can use the AMR as it sees fit */
>      env->spr[SPR_AMOR] = amor->default_value = 0xffffffffffffffffull;
>  
> -    /* Update some env bits based on new LPCR value */
> -    ppc_hash64_update_rmls(cpu);
> -    ppc_hash64_update_vrma(cpu);
> -
>      /* Tell KVM that we're in PAPR mode */
>      if (kvm_enabled()) {
>          kvmppc_set_papr(cpu);

Re: [Qemu-devel] [PATCH 8/8] spapr: Clean up handling of LPCR power-saving exit bits

[Cédric Le Goater]

On 05/03/2018 01:59 PM, David Gibson wrote:
> On Thu, May 03, 2018 at 09:47:18AM +0200, Cédric Le Goater wrote:
>> On 05/03/2018 08:21 AM, David Gibson wrote:
>>> To prevent spurious wakeups on cpus that are supposed to be disabled, we
>>> need to clear the LPCR bits which control certain wakeup events.
>>> spapr_cpu_reset() has separate cases here for boot and non-boot (initially
>>> inactive) cpus.  rtas_start_cpu() then turns the LPCR bits on when the
>>> non-boot cpus are activated.
>>>
>>> But explicit checks against first_cpu are not how we usually do things:
>>> instead spapr_cpu_reset() generally sets things up for non-boot (inactive)
>>> cpus, then spapr_machine_reset() and/or rtas_start_cpu() override as
>>> necessary.
>>>
>>> So, do that instead.  Because the LPCR activation is identical for boot
>>> cpus and non-boot cpus just activated with rtas_start_cpu() we can put the
>>> code common in spapr_cpu_set_entry_state().
>>
>> This is much nicer.
>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> and for the patchset :
>>
>> Tested-by: Cédric Le Goater <clg@kaod.org>
>>
>> under KVM & TCG with cpu hotplug and unplug.
> 
> Thanks.  I've folded the series into ppc-for-2.13.

We still need that patch for unplug to work :

	http://patchwork.ozlabs.org/patch/904229/

C.

>> There is still a spapr_cpu_reset() call under spapr_cpu_init().
>> Is that on purpose ?
> 
> Sort of.  I'd still like to remove it, but figuring out how to do so
> safely is going to take a bit longer.  Well, probably quite a lot
> longer, since I doubt I'll get it done before I go away.
> 

Re: [Qemu-devel] [PATCH 2/8] spapr: Clean up rtas_start_cpu() & rtas_stop_self()

[Greg Kurz]

On Thu,  3 May 2018 16:21:39 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> This makes several minor cleanups to these functions:
>   * Follow usual convention of an early exit on error, rather than having
>     most of the body in an if
>   * Clearer naming of cpu and cpu_.  Now callcpu is the cpu from which the
>     RTAS call is invoked, newcpu is the cpu which we're starting
>   * Use cpu_synchronize_state() instead of kvm_cpu_synchronize_state()
>     directly
>   * Remove pointless comment describing what cpu_synchronize_state() does
>   * Use ppc_store_lpcr() instead of directly writing the register field
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_rtas.c | 66 ++++++++++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 0ec5fa4cfe..b251c130cb 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -32,7 +32,7 @@
>  #include "hw/qdev.h"
>  #include "sysemu/device_tree.h"
>  #include "sysemu/cpus.h"
> -#include "sysemu/kvm.h"
> +#include "sysemu/hw_accel.h"
>  
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/spapr_vio.h"
> @@ -45,6 +45,7 @@
>  #include "qemu/cutils.h"
>  #include "trace.h"
>  #include "hw/ppc/fdt.h"
> +#include "target/ppc/mmu-hash64.h"
>  
>  static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                                     uint32_t token, uint32_t nargs,
> @@ -140,13 +141,15 @@ static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
>      }
>  }
>  
> -static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> +static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
>                             uint32_t token, uint32_t nargs,
>                             target_ulong args,
>                             uint32_t nret, target_ulong rets)
>  {
>      target_ulong id, start, r3;
> -    PowerPCCPU *cpu;
> +    PowerPCCPU *newcpu;
> +    CPUPPCState *env;
> +    PowerPCCPUClass *pcc;
>  
>      if (nargs != 3 || nret != 1) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -157,41 +160,37 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
>      start = rtas_ld(args, 1);
>      r3 = rtas_ld(args, 2);
>  
> -    cpu = spapr_find_cpu(id);
> -    if (cpu != NULL) {
> -        CPUState *cs = CPU(cpu);

The function ends up with four invocations of CPU(newcpu), ie, we'll
go down the full type checking path four times. Not very painful since
rtas_start_cpu() is not a hot path, but it would certainly not hurt
to do it only once like the current code does.

Anyway, with or without this change:

Reviewed-by: Greg Kurz <groug@kaod.org>

> -        CPUPPCState *env = &cpu->env;
> -        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    newcpu = spapr_find_cpu(id);
> +    if (!newcpu) {
> +        /* Didn't find a matching cpu */
> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +        return;
> +    }
>  
> -        if (!cs->halted) {
> -            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> -            return;
> -        }
> +    env = &newcpu->env;
> +    pcc = POWERPC_CPU_GET_CLASS(newcpu);
>  
> -        /* This will make sure qemu state is up to date with kvm, and
> -         * mark it dirty so our changes get flushed back before the
> -         * new cpu enters */
> -        kvm_cpu_synchronize_state(cs);
> +    if (!CPU(newcpu)->halted) {
> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> +        return;
> +    }
>  
> -        env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> +    cpu_synchronize_state(CPU(newcpu));
>  
> -        /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> -        env->spr[SPR_LPCR] |= pcc->lpcr_pm;
> +    env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> +    spapr_cpu_set_endianness(newcpu);
> +    spapr_cpu_update_tb_offset(newcpu);
> +    /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> +    ppc_store_lpcr(newcpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
>  
> -        env->nip = start;
> -        env->gpr[3] = r3;
> -        cs->halted = 0;
> -        spapr_cpu_set_endianness(cpu);
> -        spapr_cpu_update_tb_offset(cpu);
> +    env->nip = start;
> +    env->gpr[3] = r3;
>  
> -        qemu_cpu_kick(cs);
> +    CPU(newcpu)->halted = 0;
>  
> -        rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> -        return;
> -    }
> +    qemu_cpu_kick(CPU(newcpu));
>  
> -    /* Didn't find a matching cpu */
> -    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
>  
>  static void rtas_stop_self(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> @@ -203,13 +202,12 @@ static void rtas_stop_self(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>  
> -    cs->halted = 1;
> -    qemu_cpu_kick(cs);
> -
>      /* Disable Power-saving mode Exit Cause exceptions for the CPU.
>       * This could deliver an interrupt on a dying CPU and crash the
>       * guest */
> -    env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
> +    ppc_store_lpcr(cpu, env->spr[SPR_LPCR] & ~pcc->lpcr_pm);
> +    cs->halted = 1;
> +    qemu_cpu_kick(cs);
>  }
>  
>  static inline int sysparm_st(target_ulong addr, target_ulong len,

Re: [Qemu-devel] [PATCH 3/8] spapr: Remove unhelpful helpers from rtas_start_cpu()

[Greg Kurz]

On Thu,  3 May 2018 16:21:40 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> rtas_start_cpu() calls spapr_cpu_update_tb_offset() and
> spapr_cpu_set_endianness() to initialize certain things in the new cpu's
> state.  This is the only caller of those helpers, and they're each only
> a few lines long, so we might as well just fold them into the caller.
> 
> In addition, those helpers initialize state on the new cpu to match that of
> the first cpu.  That will generally work, but might be at least logically
> incorrect if the first cpu has been set offline by the guest.  So, instead
> base the state on that of the cpu invoking the RTAS call, which is
> obviously active already.
> 

Much better indeed !

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_rtas.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index b251c130cb..df073447c5 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -120,27 +120,6 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
>      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>  }
>  
> -/*
> - * Set the timebase offset of the CPU to that of first CPU.
> - * This helps hotplugged CPU to have the correct timebase offset.
> - */
> -static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
> -{
> -    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> -
> -    cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
> -}
> -
> -static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> -{
> -    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
> -
> -    if (!pcc->interrupts_big_endian(fcpu)) {
> -        cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
> -    }
> -}
> -
>  static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
>                             uint32_t token, uint32_t nargs,
>                             target_ulong args,
> @@ -150,6 +129,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
>      PowerPCCPU *newcpu;
>      CPUPPCState *env;
>      PowerPCCPUClass *pcc;
> +    target_ulong lpcr;
>  
>      if (nargs != 3 || nret != 1) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -178,10 +158,20 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
>      cpu_synchronize_state(CPU(newcpu));
>  
>      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> -    spapr_cpu_set_endianness(newcpu);
> -    spapr_cpu_update_tb_offset(newcpu);
> +
>      /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> -    ppc_store_lpcr(newcpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
> +    lpcr = env->spr[SPR_LPCR] | pcc->lpcr_pm;
> +    if (!pcc->interrupts_big_endian(callcpu)) {
> +        lpcr |= LPCR_ILE;
> +    }
> +    ppc_store_lpcr(newcpu, lpcr);
> +
> +    /*
> +     * Set the timebase offset of the new CPU to that of the invoking
> +     * CPU.  This helps hotplugged CPU to have the correct timebase
               ^
Extraneous space here

> +     * offset.
> +     */
> +    newcpu->env.tb_env->tb_offset = callcpu->env.tb_env->tb_offset;
>  

Maybe use env-> instead of newcpu->env. ?

Anyway,

Reviewed-by: Greg Kurz <groug@kaod.org>

>      env->nip = start;
>      env->gpr[3] = r3;

Re: [Qemu-devel] [PATCH 4/8] spapr: Make a helper to set up cpu entry point state

[Greg Kurz]

On Thu,  3 May 2018 16:21:41 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Under PAPR, only the boot CPU is active when the system starts.  Other cpus
> must be explicitly activated using an RTAS call.  The entry state for the
> boot and secondary cpus isn't identical, but it has some things in common.
> We're going to add a bit more common setup later, too, so to simplify
> make a helper which sets up the common entry state for both boot and
> secondary cpu threads.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c                  | 4 +---
>  hw/ppc/spapr_cpu_core.c         | 9 +++++++++
>  hw/ppc/spapr_rtas.c             | 6 ++----
>  include/hw/ppc/spapr_cpu_core.h | 3 +++
>  4 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b35aff5d81..944bee7a71 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1668,10 +1668,8 @@ static void spapr_machine_reset(void)
>      g_free(fdt);
>  
>      /* Set up the entry state */
> -    first_ppc_cpu->env.gpr[3] = fdt_addr;
> +    spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
>      first_ppc_cpu->env.gpr[5] = 0;
> -    first_cpu->halted = 0;
> -    first_ppc_cpu->env.nip = SPAPR_ENTRY_POINT;
>  
>      spapr->cas_reboot = false;
>  }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 01dbc69424..a98c7b04c6 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -52,6 +52,15 @@ static void spapr_cpu_reset(void *opaque)
>  
>  }
>  
> +void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> +{
> +    CPUPPCState *env = &cpu->env;
> +
> +    env->nip = nip;
> +    env->gpr[3] = r3;
> +    CPU(cpu)->halted = 0;
> +}
> +
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
>  {
>      qemu_unregister_reset(spapr_cpu_reset, cpu);
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index df073447c5..840d198a8d 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -37,6 +37,7 @@
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/spapr_vio.h"
>  #include "hw/ppc/spapr_rtas.h"
> +#include "hw/ppc/spapr_cpu_core.h"
>  #include "hw/ppc/ppc.h"
>  #include "hw/boards.h"
>  
> @@ -173,10 +174,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
>       */
>      newcpu->env.tb_env->tb_offset = callcpu->env.tb_env->tb_offset;
>  
> -    env->nip = start;
> -    env->gpr[3] = r3;
> -
> -    CPU(newcpu)->halted = 0;
> +    spapr_cpu_set_entry_state(newcpu, start, r3);
>  
>      qemu_cpu_kick(CPU(newcpu));
>  
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 1129f344aa..47dcfda12b 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -12,6 +12,7 @@
>  #include "hw/qdev.h"
>  #include "hw/cpu/core.h"
>  #include "target/ppc/cpu-qom.h"
> +#include "target/ppc/cpu.h"
>  
>  #define TYPE_SPAPR_CPU_CORE "spapr-cpu-core"
>  #define SPAPR_CPU_CORE(obj) \
> @@ -38,4 +39,6 @@ typedef struct sPAPRCPUCoreClass {
>  } sPAPRCPUCoreClass;
>  
>  const char *spapr_get_cpu_core_type(const char *cpu_type);
> +void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
> +
>  #endif

Re: [Qemu-devel] [PATCH 3/8] spapr: Remove unhelpful helpers from rtas_start_cpu()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4379 bytes --]

On Thu, May 03, 2018 at 06:34:16PM +0200, Greg Kurz wrote:
> On Thu,  3 May 2018 16:21:40 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > rtas_start_cpu() calls spapr_cpu_update_tb_offset() and
> > spapr_cpu_set_endianness() to initialize certain things in the new cpu's
> > state.  This is the only caller of those helpers, and they're each only
> > a few lines long, so we might as well just fold them into the caller.
> > 
> > In addition, those helpers initialize state on the new cpu to match that of
> > the first cpu.  That will generally work, but might be at least logically
> > incorrect if the first cpu has been set offline by the guest.  So, instead
> > base the state on that of the cpu invoking the RTAS call, which is
> > obviously active already.
> > 
> 
> Much better indeed !
> 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_rtas.c | 38 ++++++++++++++------------------------
> >  1 file changed, 14 insertions(+), 24 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index b251c130cb..df073447c5 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -120,27 +120,6 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
> >      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> >  }
> >  
> > -/*
> > - * Set the timebase offset of the CPU to that of first CPU.
> > - * This helps hotplugged CPU to have the correct timebase offset.
> > - */
> > -static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
> > -{
> > -    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> > -
> > -    cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
> > -}
> > -
> > -static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> > -{
> > -    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> > -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
> > -
> > -    if (!pcc->interrupts_big_endian(fcpu)) {
> > -        cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
> > -    }
> > -}
> > -
> >  static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
> >                             uint32_t token, uint32_t nargs,
> >                             target_ulong args,
> > @@ -150,6 +129,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
> >      PowerPCCPU *newcpu;
> >      CPUPPCState *env;
> >      PowerPCCPUClass *pcc;
> > +    target_ulong lpcr;
> >  
> >      if (nargs != 3 || nret != 1) {
> >          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > @@ -178,10 +158,20 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
> >      cpu_synchronize_state(CPU(newcpu));
> >  
> >      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> > -    spapr_cpu_set_endianness(newcpu);
> > -    spapr_cpu_update_tb_offset(newcpu);
> > +
> >      /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> > -    ppc_store_lpcr(newcpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
> > +    lpcr = env->spr[SPR_LPCR] | pcc->lpcr_pm;
> > +    if (!pcc->interrupts_big_endian(callcpu)) {
> > +        lpcr |= LPCR_ILE;
> > +    }
> > +    ppc_store_lpcr(newcpu, lpcr);
> > +
> > +    /*
> > +     * Set the timebase offset of the new CPU to that of the invoking
> > +     * CPU.  This helps hotplugged CPU to have the correct timebase
>                ^
> Extraneous space here

Oh my, the can of worms you've opened with that apparently innocuous
comment :)
    https://en.wikipedia.org/wiki/Sentence_spacing#Controversy

tl;dr: I have a habit of putting a double space after full stops.  The
merits of that are highly debatable, but there it is.

> > +     * offset.
> > +     */
> > +    newcpu->env.tb_env->tb_offset = callcpu->env.tb_env->tb_offset;
> >  
> 
> Maybe use env-> instead of newcpu->env. ?

That is better, but I'm already most of the way through my
pre-pull-request testing, so I don't want to risk any code changes
(although I still fold the new R-bs into the commit messages).

> 
> Anyway,
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> >      env->nip = start;
> >      env->gpr[3] = r3;
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 2/8] spapr: Clean up rtas_start_cpu() & rtas_stop_self()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6287 bytes --]

On Thu, May 03, 2018 at 05:13:31PM +0200, Greg Kurz wrote:
> On Thu,  3 May 2018 16:21:39 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > This makes several minor cleanups to these functions:
> >   * Follow usual convention of an early exit on error, rather than having
> >     most of the body in an if
> >   * Clearer naming of cpu and cpu_.  Now callcpu is the cpu from which the
> >     RTAS call is invoked, newcpu is the cpu which we're starting
> >   * Use cpu_synchronize_state() instead of kvm_cpu_synchronize_state()
> >     directly
> >   * Remove pointless comment describing what cpu_synchronize_state() does
> >   * Use ppc_store_lpcr() instead of directly writing the register field
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_rtas.c | 66 ++++++++++++++++++++++-----------------------
> >  1 file changed, 32 insertions(+), 34 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 0ec5fa4cfe..b251c130cb 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -32,7 +32,7 @@
> >  #include "hw/qdev.h"
> >  #include "sysemu/device_tree.h"
> >  #include "sysemu/cpus.h"
> > -#include "sysemu/kvm.h"
> > +#include "sysemu/hw_accel.h"
> >  
> >  #include "hw/ppc/spapr.h"
> >  #include "hw/ppc/spapr_vio.h"
> > @@ -45,6 +45,7 @@
> >  #include "qemu/cutils.h"
> >  #include "trace.h"
> >  #include "hw/ppc/fdt.h"
> > +#include "target/ppc/mmu-hash64.h"
> >  
> >  static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >                                     uint32_t token, uint32_t nargs,
> > @@ -140,13 +141,15 @@ static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> >      }
> >  }
> >  
> > -static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > +static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
> >                             uint32_t token, uint32_t nargs,
> >                             target_ulong args,
> >                             uint32_t nret, target_ulong rets)
> >  {
> >      target_ulong id, start, r3;
> > -    PowerPCCPU *cpu;
> > +    PowerPCCPU *newcpu;
> > +    CPUPPCState *env;
> > +    PowerPCCPUClass *pcc;
> >  
> >      if (nargs != 3 || nret != 1) {
> >          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > @@ -157,41 +160,37 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> >      start = rtas_ld(args, 1);
> >      r3 = rtas_ld(args, 2);
> >  
> > -    cpu = spapr_find_cpu(id);
> > -    if (cpu != NULL) {
> > -        CPUState *cs = CPU(cpu);
> 
> The function ends up with four invocations of CPU(newcpu), ie, we'll
> go down the full type checking path four times. Not very painful since
> rtas_start_cpu() is not a hot path, but it would certainly not hurt
> to do it only once like the current code does.
> 
> Anyway, with or without this change:

Ah, yeah, I was thinking of CPU() as a trivial offset, and forgot it
also did type checking.

Eh, as you say it's not a hot path and I don't really want to
introduce a fourth cpu parameter/variable so let's leave it as is.

> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > -        CPUPPCState *env = &cpu->env;
> > -        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > +    newcpu = spapr_find_cpu(id);
> > +    if (!newcpu) {
> > +        /* Didn't find a matching cpu */
> > +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > +        return;
> > +    }
> >  
> > -        if (!cs->halted) {
> > -            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > -            return;
> > -        }
> > +    env = &newcpu->env;
> > +    pcc = POWERPC_CPU_GET_CLASS(newcpu);
> >  
> > -        /* This will make sure qemu state is up to date with kvm, and
> > -         * mark it dirty so our changes get flushed back before the
> > -         * new cpu enters */
> > -        kvm_cpu_synchronize_state(cs);
> > +    if (!CPU(newcpu)->halted) {
> > +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > +        return;
> > +    }
> >  
> > -        env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> > +    cpu_synchronize_state(CPU(newcpu));
> >  
> > -        /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> > -        env->spr[SPR_LPCR] |= pcc->lpcr_pm;
> > +    env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> > +    spapr_cpu_set_endianness(newcpu);
> > +    spapr_cpu_update_tb_offset(newcpu);
> > +    /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> > +    ppc_store_lpcr(newcpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
> >  
> > -        env->nip = start;
> > -        env->gpr[3] = r3;
> > -        cs->halted = 0;
> > -        spapr_cpu_set_endianness(cpu);
> > -        spapr_cpu_update_tb_offset(cpu);
> > +    env->nip = start;
> > +    env->gpr[3] = r3;
> >  
> > -        qemu_cpu_kick(cs);
> > +    CPU(newcpu)->halted = 0;
> >  
> > -        rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> > -        return;
> > -    }
> > +    qemu_cpu_kick(CPU(newcpu));
> >  
> > -    /* Didn't find a matching cpu */
> > -    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >  }
> >  
> >  static void rtas_stop_self(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > @@ -203,13 +202,12 @@ static void rtas_stop_self(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >      CPUPPCState *env = &cpu->env;
> >      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> >  
> > -    cs->halted = 1;
> > -    qemu_cpu_kick(cs);
> > -
> >      /* Disable Power-saving mode Exit Cause exceptions for the CPU.
> >       * This could deliver an interrupt on a dying CPU and crash the
> >       * guest */
> > -    env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
> > +    ppc_store_lpcr(cpu, env->spr[SPR_LPCR] & ~pcc->lpcr_pm);
> > +    cs->halted = 1;
> > +    qemu_cpu_kick(cs);
> >  }
> >  
> >  static inline int sysparm_st(target_ulong addr, target_ulong len,
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/8] spapr: Remove unhelpful helpers from rtas_start_cpu()

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 4726 bytes --]

On Fri, 4 May 2018 15:00:20 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, May 03, 2018 at 06:34:16PM +0200, Greg Kurz wrote:
> > On Thu,  3 May 2018 16:21:40 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > rtas_start_cpu() calls spapr_cpu_update_tb_offset() and
> > > spapr_cpu_set_endianness() to initialize certain things in the new cpu's
> > > state.  This is the only caller of those helpers, and they're each only
> > > a few lines long, so we might as well just fold them into the caller.
> > > 
> > > In addition, those helpers initialize state on the new cpu to match that of
> > > the first cpu.  That will generally work, but might be at least logically
> > > incorrect if the first cpu has been set offline by the guest.  So, instead
> > > base the state on that of the cpu invoking the RTAS call, which is
> > > obviously active already.
> > >   
> > 
> > Much better indeed !
> >   
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/ppc/spapr_rtas.c | 38 ++++++++++++++------------------------
> > >  1 file changed, 14 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > > index b251c130cb..df073447c5 100644
> > > --- a/hw/ppc/spapr_rtas.c
> > > +++ b/hw/ppc/spapr_rtas.c
> > > @@ -120,27 +120,6 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
> > >      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > >  }
> > >  
> > > -/*
> > > - * Set the timebase offset of the CPU to that of first CPU.
> > > - * This helps hotplugged CPU to have the correct timebase offset.
> > > - */
> > > -static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
> > > -{
> > > -    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> > > -
> > > -    cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
> > > -}
> > > -
> > > -static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> > > -{
> > > -    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> > > -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
> > > -
> > > -    if (!pcc->interrupts_big_endian(fcpu)) {
> > > -        cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
> > > -    }
> > > -}
> > > -
> > >  static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
> > >                             uint32_t token, uint32_t nargs,
> > >                             target_ulong args,
> > > @@ -150,6 +129,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
> > >      PowerPCCPU *newcpu;
> > >      CPUPPCState *env;
> > >      PowerPCCPUClass *pcc;
> > > +    target_ulong lpcr;
> > >  
> > >      if (nargs != 3 || nret != 1) {
> > >          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > > @@ -178,10 +158,20 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
> > >      cpu_synchronize_state(CPU(newcpu));
> > >  
> > >      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> > > -    spapr_cpu_set_endianness(newcpu);
> > > -    spapr_cpu_update_tb_offset(newcpu);
> > > +
> > >      /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> > > -    ppc_store_lpcr(newcpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
> > > +    lpcr = env->spr[SPR_LPCR] | pcc->lpcr_pm;
> > > +    if (!pcc->interrupts_big_endian(callcpu)) {
> > > +        lpcr |= LPCR_ILE;
> > > +    }
> > > +    ppc_store_lpcr(newcpu, lpcr);
> > > +
> > > +    /*
> > > +     * Set the timebase offset of the new CPU to that of the invoking
> > > +     * CPU.  This helps hotplugged CPU to have the correct timebase  
> >                ^
> > Extraneous space here  
> 
> Oh my, the can of worms you've opened with that apparently innocuous
> comment :)
>     https://en.wikipedia.org/wiki/Sentence_spacing#Controversy
> 

Ha ha ! I now have an impression of deja-vu, probably on this very same
list :D

> tl;dr: I have a habit of putting a double space after full stops.  The
> merits of that are highly debatable, but there it is.
> 

Sure, I'll try to remember that the so-called French spacing doesn't
rule the world... yet ;)


> > > +     * offset.
> > > +     */
> > > +    newcpu->env.tb_env->tb_offset = callcpu->env.tb_env->tb_offset;
> > >    
> > 
> > Maybe use env-> instead of newcpu->env. ?  
> 
> That is better, but I'm already most of the way through my
> pre-pull-request testing, so I don't want to risk any code changes
> (although I still fold the new R-bs into the commit messages).
> 

Understandable.

> > 
> > Anyway,
> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> >   
> > >      env->nip = start;
> > >      env->gpr[3] = r3;  
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]