[PATCH 0/4] target/ppc: TCG SMT support for spapr

[PATCH 0/4] target/ppc: TCG SMT support for spapr

[Nicholas Piggin]

Previous RFC here

https://lists.gnu.org/archive/html/qemu-ppc/2023-05/msg00453.html

This series drops patch 1 from the previous, which is more of
a standalone bugfix.

Also accounted for Cedric's comments, except a nicer way to
set cpu_index vs PIR/TIR SPRs, which is not quite trivial.

This limits support for SMT to POWER8 and newer. It is also
incompatible with nested-HV so that is checked for too.

Iterating CPUs to find siblings for now I kept because similar
loops exist in a few places, and it is not conceptually
difficult for SMT, just fiddly code to improve. For now it
should not be much performane concern.

I removed hypervisor msgsnd support from patch 3, which is not
required for spapr and added significantly to the patch.

For now nobody has objected to the way shared SPR access is
handled (serialised with TCG atomics support) so we'll keep
going with it.

Thanks,
Nick

Nicholas Piggin (4):
  target/ppc: Add initial flags and helpers for SMT support
  target/ppc: Add support for SMT CTRL register
  target/ppc: Add msgsndp and DPDES SMT support
  spapr: Allow up to 8 threads SMT on POWER8 and newer

 hw/ppc/ppc.c             |  6 ++++
 hw/ppc/spapr.c           | 16 +++++++---
 hw/ppc/spapr_caps.c      | 14 ++++++++
 hw/ppc/spapr_cpu_core.c  |  7 ++--
 include/hw/ppc/ppc.h     |  1 +
 target/ppc/cpu.h         |  9 ++++++
 target/ppc/cpu_init.c    |  5 +++
 target/ppc/excp_helper.c | 30 ++++++++++++++---
 target/ppc/gdbstub.c     |  2 +-
 target/ppc/helper.h      |  2 ++
 target/ppc/misc_helper.c | 69 ++++++++++++++++++++++++++++++++++++----
 target/ppc/translate.c   | 46 ++++++++++++++++++++++++++-
 12 files changed, 188 insertions(+), 19 deletions(-)

-- 
2.40.1

[PATCH 1/4] target/ppc: Add initial flags and helpers for SMT support

[Nicholas Piggin]

SMT TCG emulation needs to be able to iterate over siblings in a core,
and needs to serialise core access to shared SPRs and state.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/cpu.h       |  9 +++++++++
 target/ppc/cpu_init.c  |  5 +++++
 target/ppc/translate.c | 20 ++++++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 1f23b81e90..b594408a8d 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -672,6 +672,8 @@ enum {
     POWERPC_FLAG_TM       = 0x00100000,
     /* Has SCV (ISA 3.00)                                                    */
     POWERPC_FLAG_SCV      = 0x00200000,
+    /* Has >1 thread per core                                                */
+    POWERPC_FLAG_SMT      = 0x00400000,
 };
 
 /*
@@ -1266,6 +1268,13 @@ struct CPUArchState {
     uint64_t pmu_base_time;
 };
 
+#define _CORE_ID(cs)                                            \
+    (POWERPC_CPU(cs)->env.spr_cb[SPR_PIR].default_value & ~(cs->nr_threads - 1))
+
+#define THREAD_SIBLING_FOREACH(cs, cs_sibling)			\
+    CPU_FOREACH(cs_sibling)                                     \
+        if (_CORE_ID(cs) == _CORE_ID(cs_sibling))
+
 #define SET_FIT_PERIOD(a_, b_, c_, d_)          \
 do {                                            \
     env->fit_period[0] = (a_);                  \
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index aa364f36f6..5035f6dada 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6754,6 +6754,7 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     PowerPCCPU *cpu = POWERPC_CPU(dev);
+    CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     Error *local_err = NULL;
 
@@ -6785,6 +6786,10 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
 
     pcc->parent_realize(dev, errp);
 
+    if (env_cpu(env)->nr_threads > 1) {
+        env->flags |= POWERPC_FLAG_SMT;
+    }
+
     return;
 
 unrealize:
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index b6bab4c234..72270c2163 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -227,6 +227,26 @@ struct opc_handler_t {
     void (*handler)(DisasContext *ctx);
 };
 
+static inline bool gen_serialize(DisasContext *ctx)
+{
+    if (tb_cflags(ctx->base.tb) & CF_PARALLEL) {
+        /* Restart with exclusive lock.  */
+        gen_helper_exit_atomic(cpu_env);
+        ctx->base.is_jmp = DISAS_NORETURN;
+        return false;
+    }
+    return true;
+}
+
+static inline bool gen_serialize_core(DisasContext *ctx)
+{
+    if (ctx->flags & POWERPC_FLAG_SMT) {
+        return gen_serialize(ctx);
+    }
+
+    return true;
+}
+
 /* SPR load/store helpers */
 static inline void gen_load_spr(TCGv t, int reg)
 {
-- 
2.40.1

[PATCH 2/4] target/ppc: Add support for SMT CTRL register

[Nicholas Piggin]

A relatively simple case to begin with, CTRL is a SMT shared register
where reads and writes need to synchronise against state changes by
other threads in the core.

Atomic serialisation operations are used to achieve this.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/helper.h      |  2 ++
 target/ppc/misc_helper.c | 25 +++++++++++++++++++++++++
 target/ppc/translate.c   | 18 +++++++++++++++++-
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 38efbc351c..fda40b8a60 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -704,6 +704,8 @@ DEF_HELPER_3(store_dcr, void, env, tl, tl)
 
 DEF_HELPER_2(load_dump_spr, void, env, i32)
 DEF_HELPER_2(store_dump_spr, void, env, i32)
+DEF_HELPER_3(spr_write_CTRL, void, env, i32, tl)
+
 DEF_HELPER_4(fscr_facility_check, void, env, i32, i32, i32)
 DEF_HELPER_4(msr_facility_check, void, env, i32, i32, i32)
 DEF_HELPER_FLAGS_1(load_tbl, TCG_CALL_NO_RWG, tl, env)
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index 40ddc5c08c..a058eb24cd 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -43,6 +43,31 @@ void helper_store_dump_spr(CPUPPCState *env, uint32_t sprn)
              env->spr[sprn]);
 }
 
+void helper_spr_write_CTRL(CPUPPCState *env, uint32_t sprn,
+                           target_ulong val)
+{
+    CPUState *cs = env_cpu(env);
+    CPUState *ccs;
+    uint32_t run = val & 1;
+    uint32_t ts, ts_mask;
+
+    assert(sprn == SPR_CTRL);
+
+    env->spr[sprn] &= ~1U;
+    env->spr[sprn] |= run;
+
+    ts_mask = ~(1U << (8 + env->spr[SPR_TIR]));
+    ts = run << (8 + env->spr[SPR_TIR]);
+
+    THREAD_SIBLING_FOREACH(cs, ccs) {
+        CPUPPCState *cenv = &POWERPC_CPU(ccs)->env;
+
+        cenv->spr[sprn] &= ts_mask;
+        cenv->spr[sprn] |= ts;
+    }
+}
+
+
 #ifdef TARGET_PPC64
 static void raise_hv_fu_exception(CPUPPCState *env, uint32_t bit,
                                   const char *caller, uint32_t cause,
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 72270c2163..31821f92f5 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -443,7 +443,7 @@ void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
 #endif
 }
 
-void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
+static void spr_write_CTRL_ST(DisasContext *ctx, int sprn, int gprn)
 {
     /* This does not implement >1 thread */
     TCGv t0 = tcg_temp_new();
@@ -452,6 +452,22 @@ void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
     tcg_gen_shli_tl(t1, t0, 8); /* Duplicate the bit in TS */
     tcg_gen_or_tl(t1, t1, t0);
     gen_store_spr(sprn, t1);
+}
+
+void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
+{
+    if (!(ctx->flags & POWERPC_FLAG_SMT)) {
+        spr_write_CTRL_ST(ctx, sprn, gprn);
+        goto out;
+    }
+
+    if (!gen_serialize(ctx)) {
+        return;
+    }
+
+    gen_helper_spr_write_CTRL(cpu_env, tcg_constant_i32(sprn),
+                              cpu_gpr[gprn]);
+out:
     spr_store_dump_spr(sprn);
 
     /*
-- 
2.40.1

[PATCH 3/4] target/ppc: Add msgsndp and DPDES SMT support

[Nicholas Piggin]

Doorbells in SMT need to coordinate msgsndp/msgclrp and DPDES access from
multiple threads that affect the same state.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/ppc.c             |  6 ++++++
 include/hw/ppc/ppc.h     |  1 +
 target/ppc/excp_helper.c | 30 ++++++++++++++++++++++-----
 target/ppc/gdbstub.c     |  2 +-
 target/ppc/misc_helper.c | 44 ++++++++++++++++++++++++++++++++++------
 target/ppc/translate.c   |  8 ++++++++
 6 files changed, 79 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 80b4706db2..5bd4f6708f 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -1434,6 +1434,12 @@ int ppc_cpu_pir(PowerPCCPU *cpu)
     return env->spr_cb[SPR_PIR].default_value;
 }
 
+int ppc_cpu_tir(PowerPCCPU *cpu)
+{
+    CPUPPCState *env = &cpu->env;
+    return env->spr_cb[SPR_TIR].default_value;
+}
+
 PowerPCCPU *ppc_get_vcpu_by_pir(int pir)
 {
     CPUState *cs;
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index 02af03ada2..e095c002dc 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -6,6 +6,7 @@
 void ppc_set_irq(PowerPCCPU *cpu, int n_IRQ, int level);
 PowerPCCPU *ppc_get_vcpu_by_pir(int pir);
 int ppc_cpu_pir(PowerPCCPU *cpu);
+int ppc_cpu_tir(PowerPCCPU *cpu);
 
 /* PowerPC hardware exceptions management helpers */
 typedef void (*clk_setup_cb)(void *opaque, uint32_t freq);
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 4925996cf3..1e04452614 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -3154,22 +3154,42 @@ void helper_book3s_msgclrp(CPUPPCState *env, target_ulong rb)
 }
 
 /*
- * sends a message to other threads that are on the same
+ * sends a message to another thread  on the same
  * multi-threaded processor
  */
 void helper_book3s_msgsndp(CPUPPCState *env, target_ulong rb)
 {
-    int pir = env->spr_cb[SPR_PIR].default_value;
+    CPUState *cs = env_cpu(env);
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUState *ccs;
+    uint32_t nr_threads = cs->nr_threads;
+    int ttir = rb & PPC_BITMASK(57, 63);
 
     helper_hfscr_facility_check(env, HFSCR_MSGP, "msgsndp", HFSCR_IC_MSGP);
 
-    if (!dbell_type_server(rb)) {
+    if (!dbell_type_server(rb) || ttir >= nr_threads) {
+        return;
+    }
+
+    if (nr_threads == 1) {
+        ppc_set_irq(cpu, PPC_INTERRUPT_DOORBELL, 1);
         return;
     }
 
-    /* TODO: TCG supports only one thread */
+    /* WHy does iothread need to be locked for walking CPU list? */
+    qemu_mutex_lock_iothread();
+    THREAD_SIBLING_FOREACH(cs, ccs) {
+        PowerPCCPU *ccpu = POWERPC_CPU(ccs);
+        uint32_t thread_id = ppc_cpu_tir(ccpu);
+
+        if (ttir == thread_id) {
+            ppc_set_irq(ccpu, PPC_INTERRUPT_DOORBELL, 1);
+            qemu_mutex_unlock_iothread();
+            return;
+        }
+    }
 
-    book3s_msgsnd_common(pir, PPC_INTERRUPT_DOORBELL);
+    assert(0);
 }
 #endif /* TARGET_PPC64 */
 
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index ca39efdc35..f0304e5bb6 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -117,7 +117,7 @@ void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len)
  * regs and PC, MSR, CR, and so forth.  We hack round this by giving
  * the FP regs zero size when talking to a newer gdb.
  */
-
+/* XXX: read/write dpdes correctly */
 int ppc_cpu_gdb_read_register(CPUState *cs, GByteArray *buf, int n)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index a058eb24cd..9688774139 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -184,14 +184,31 @@ void helper_store_pcr(CPUPPCState *env, target_ulong value)
  */
 target_ulong helper_load_dpdes(CPUPPCState *env)
 {
+    CPUState *cs = env_cpu(env);
+    CPUState *ccs;
+    uint32_t nr_threads = cs->nr_threads;
     target_ulong dpdes = 0;
 
     helper_hfscr_facility_check(env, HFSCR_MSGP, "load DPDES", HFSCR_IC_MSGP);
 
-    /* TODO: TCG supports only one thread */
-    if (env->pending_interrupts & PPC_INTERRUPT_DOORBELL) {
-        dpdes = 1;
+    if (nr_threads == 1) {
+        if (env->pending_interrupts & PPC_INTERRUPT_DOORBELL) {
+            dpdes = 1;
+        }
+        return dpdes;
+    }
+
+    qemu_mutex_lock_iothread();
+    THREAD_SIBLING_FOREACH(cs, ccs) {
+        PowerPCCPU *ccpu = POWERPC_CPU(ccs);
+        CPUPPCState *cenv = &ccpu->env;
+        uint32_t thread_id = ppc_cpu_tir(ccpu);
+
+        if (cenv->pending_interrupts & PPC_INTERRUPT_DOORBELL) {
+            dpdes |= (0x1 << thread_id);
+        }
     }
+    qemu_mutex_unlock_iothread();
 
     return dpdes;
 }
@@ -199,17 +216,32 @@ target_ulong helper_load_dpdes(CPUPPCState *env)
 void helper_store_dpdes(CPUPPCState *env, target_ulong val)
 {
     PowerPCCPU *cpu = env_archcpu(env);
+    CPUState *cs = env_cpu(env);
+    CPUState *ccs;
+    uint32_t nr_threads = cs->nr_threads;
 
     helper_hfscr_facility_check(env, HFSCR_MSGP, "store DPDES", HFSCR_IC_MSGP);
 
-    /* TODO: TCG supports only one thread */
-    if (val & ~0x1) {
+    if (val & ~(nr_threads - 1)) {
         qemu_log_mask(LOG_GUEST_ERROR, "Invalid DPDES register value "
                       TARGET_FMT_lx"\n", val);
+        val &= (nr_threads - 1); /* Ignore the invalid bits */
+    }
+
+    if (nr_threads == 1) {
+        /* XXX: don't need iothread lock (does ppc_set_irq takes it?) */
+        ppc_set_irq(cpu, PPC_INTERRUPT_DOORBELL, val & 0x1);
         return;
     }
 
-    ppc_set_irq(cpu, PPC_INTERRUPT_DOORBELL, val & 0x1);
+    qemu_mutex_lock_iothread();
+    THREAD_SIBLING_FOREACH(cs, ccs) {
+        PowerPCCPU *ccpu = POWERPC_CPU(ccs);
+        uint32_t thread_id = ppc_cpu_tir(ccpu);
+
+        ppc_set_irq(cpu, PPC_INTERRUPT_DOORBELL, val & (0x1 << thread_id));
+    }
+    qemu_mutex_unlock_iothread();
 }
 #endif /* defined(TARGET_PPC64) */
 
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 31821f92f5..0aa49323d3 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -820,11 +820,19 @@ void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
 /* DPDES */
 void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
 {
+    if (!gen_serialize_core(ctx)) {
+        return;
+    }
+
     gen_helper_load_dpdes(cpu_gpr[gprn], cpu_env);
 }
 
 void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn)
 {
+    if (!gen_serialize_core(ctx)) {
+        return;
+    }
+
     gen_helper_store_dpdes(cpu_env, cpu_gpr[gprn]);
 }
 #endif
-- 
2.40.1

[PATCH 4/4] spapr: Allow up to 8 threads SMT on POWER8 and newer

[Nicholas Piggin]

PPC TCG now supports multi-threaded CPU configuration for non-hypervisor
state. This requires PIR and TIR be set, because that's how sibling thread
matching is done.

spapr's nested-HV capability does not currently coexist with SMT. This
is quite analogous to LPAR-per-core mode on real hardware which also
does not support KVM.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr.c          | 16 ++++++++++++----
 hw/ppc/spapr_caps.c     | 14 ++++++++++++++
 hw/ppc/spapr_cpu_core.c |  7 +++++--
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index dcb7f1c70a..deb8b507e3 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2524,10 +2524,18 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp)
     int ret;
     unsigned int smp_threads = ms->smp.threads;
 
-    if (!kvm_enabled() && (smp_threads > 1)) {
-        error_setg(errp, "TCG cannot support more than 1 thread/core "
-                   "on a pseries machine");
-        return;
+    if (!kvm_enabled()) {
+        if (!ppc_type_check_compat(ms->cpu_type, CPU_POWERPC_LOGICAL_2_07, 0,
+                                   spapr->max_compat_pvr)) {
+            error_setg(errp, "TCG only supports SMT on POWER8 or newer CPUs");
+            return;
+        }
+
+        if (smp_threads > 8) {
+            error_setg(errp, "TCG cannot support more than 8 threads/core "
+                       "on a pseries machine");
+            return;
+        }
     }
     if (!is_power_of_2(smp_threads)) {
         error_setg(errp, "Cannot support %d threads/core on a pseries "
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 3fd45a6dec..03f02b4af3 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -473,6 +473,20 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
                 error_append_hint(errp,
                                   "Try appending -machine cap-nested-hv=off\n");
         }
+    } else {
+        MachineState *ms = MACHINE(spapr);
+        unsigned int smp_threads = ms->smp.threads;
+
+        /*
+         * Nested-HV vCPU env state to L2, so SMT-shared SPR updates, for
+         * example, do not necessarily update the correct SPR value on sibling
+         * threads that are in a different guest/host context.
+         */
+        if (smp_threads > 1) {
+            error_setg(errp, "TCG does not support nested-HV with SMT");
+            error_append_hint(errp, "Try appending -machine cap-nested-hv=off "
+                                    "or use threads=1 with -smp\n");
+        }
     }
 }
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 9b88dd549a..a4e3c2fadd 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -255,7 +255,7 @@ static void spapr_cpu_core_unrealize(DeviceState *dev)
 }
 
 static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
-                               SpaprCpuCore *sc, Error **errp)
+                               SpaprCpuCore *sc, int thread_index, Error **errp)
 {
     CPUPPCState *env = &cpu->env;
     CPUState *cs = CPU(cpu);
@@ -267,6 +267,9 @@ static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
     cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
     kvmppc_set_papr(cpu);
 
+    env->spr_cb[SPR_PIR].default_value = cs->cpu_index;
+    env->spr_cb[SPR_TIR].default_value = thread_index;
+
     /* Set time-base frequency to 512 MHz. vhyp must be set first. */
     cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
 
@@ -337,7 +340,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < cc->nr_threads; i++) {
         sc->threads[i] = spapr_create_vcpu(sc, i, errp);
         if (!sc->threads[i] ||
-            !spapr_realize_vcpu(sc->threads[i], spapr, sc, errp)) {
+            !spapr_realize_vcpu(sc->threads[i], spapr, sc, i, errp)) {
             spapr_cpu_core_unrealize(dev);
             return;
         }
-- 
2.40.1

Re: [PATCH 0/4] target/ppc: TCG SMT support for spapr

[Cédric Le Goater]

On 6/5/23 13:23, Nicholas Piggin wrote:
> Previous RFC here
> 
> https://lists.gnu.org/archive/html/qemu-ppc/2023-05/msg00453.html
> 
> This series drops patch 1 from the previous, which is more of
> a standalone bugfix.
> 
> Also accounted for Cedric's comments, except a nicer way to
> set cpu_index vs PIR/TIR SPRs, which is not quite trivial.
> 
> This limits support for SMT to POWER8 and newer. It is also
> incompatible with nested-HV so that is checked for too.
> 
> Iterating CPUs to find siblings for now I kept because similar
> loops exist in a few places, and it is not conceptually
> difficult for SMT, just fiddly code to improve. For now it
> should not be much performane concern.
> 
> I removed hypervisor msgsnd support from patch 3, which is not
> required for spapr and added significantly to the patch.
> 
> For now nobody has objected to the way shared SPR access is
> handled (serialised with TCG atomics support) so we'll keep
> going with it.

Cc:ing more people for possible feedback.

Thanks,

C.

Re: [PATCH 2/4] target/ppc: Add support for SMT CTRL register

[Cédric Le Goater]

Hello Nick,


> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -443,7 +443,7 @@ void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
>   #endif
>   }
>   
> -void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
> +static void spr_write_CTRL_ST(DisasContext *ctx, int sprn, int gprn)
>   {
>       /* This does not implement >1 thread */
>       TCGv t0 = tcg_temp_new();
> @@ -452,6 +452,22 @@ void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
>       tcg_gen_shli_tl(t1, t0, 8); /* Duplicate the bit in TS */
>       tcg_gen_or_tl(t1, t1, t0);
>       gen_store_spr(sprn, t1);

The hunk above doesn't apply cleanly. Am I missing some patch you would
have sent previously ?

Thanks,

C.



> +}
> +
> +void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
> +{
> +    if (!(ctx->flags & POWERPC_FLAG_SMT)) {
> +        spr_write_CTRL_ST(ctx, sprn, gprn);
> +        goto out;
> +    }
> +
> +    if (!gen_serialize(ctx)) {
> +        return;
> +    }
> +
> +    gen_helper_spr_write_CTRL(cpu_env, tcg_constant_i32(sprn),
> +                              cpu_gpr[gprn]);
> +out:
>       spr_store_dump_spr(sprn);
>   
>       /*

Re: [PATCH 4/4] spapr: Allow up to 8 threads SMT on POWER8 and newer

[Harsh Prateek Bora]

On 6/5/23 16:53, Nicholas Piggin wrote:
> PPC TCG now supports multi-threaded CPU configuration for non-hypervisor
> state. This requires PIR and TIR be set, because that's how sibling thread
> matching is done.
> 
> spapr's nested-HV capability does not currently coexist with SMT. This
> is quite analogous to LPAR-per-core mode on real hardware which also
> does not support KVM.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/ppc/spapr.c          | 16 ++++++++++++----
>   hw/ppc/spapr_caps.c     | 14 ++++++++++++++
>   hw/ppc/spapr_cpu_core.c |  7 +++++--
>   3 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index dcb7f1c70a..deb8b507e3 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2524,10 +2524,18 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp)
>       int ret;
>       unsigned int smp_threads = ms->smp.threads;
>   
> -    if (!kvm_enabled() && (smp_threads > 1)) {
> -        error_setg(errp, "TCG cannot support more than 1 thread/core "
> -                   "on a pseries machine");
> -        return;
> +    if (!kvm_enabled()) {

Can we make it a check for tcg_enabled() which would be more appropriate 
or as Cedric suggested, may be include this one along with your series:

https://lore.kernel.org/qemu-devel/20230620074802.86898-1-philmd@linaro.org/

regards,
Harsh
> +        if (!ppc_type_check_compat(ms->cpu_type, CPU_POWERPC_LOGICAL_2_07, 0,
> +                                   spapr->max_compat_pvr)) {
> +            error_setg(errp, "TCG only supports SMT on POWER8 or newer CPUs");
> +            return;
> +        }
> +
> +        if (smp_threads > 8) {
> +            error_setg(errp, "TCG cannot support more than 8 threads/core "
> +                       "on a pseries machine");
> +            return;
> +        }
>       }
>       if (!is_power_of_2(smp_threads)) {
>           error_setg(errp, "Cannot support %d threads/core on a pseries "
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 3fd45a6dec..03f02b4af3 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -473,6 +473,20 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
>                   error_append_hint(errp,
>                                     "Try appending -machine cap-nested-hv=off\n");
>           }
> +    } else {
> +        MachineState *ms = MACHINE(spapr);
> +        unsigned int smp_threads = ms->smp.threads;
> +
> +        /*
> +         * Nested-HV vCPU env state to L2, so SMT-shared SPR updates, for
> +         * example, do not necessarily update the correct SPR value on sibling
> +         * threads that are in a different guest/host context.
> +         */
> +        if (smp_threads > 1) {
> +            error_setg(errp, "TCG does not support nested-HV with SMT");
> +            error_append_hint(errp, "Try appending -machine cap-nested-hv=off "
> +                                    "or use threads=1 with -smp\n");
> +        }
>       }
>   }
>   
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 9b88dd549a..a4e3c2fadd 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -255,7 +255,7 @@ static void spapr_cpu_core_unrealize(DeviceState *dev)
>   }
>   
>   static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
> -                               SpaprCpuCore *sc, Error **errp)
> +                               SpaprCpuCore *sc, int thread_index, Error **errp)
>   {
>       CPUPPCState *env = &cpu->env;
>       CPUState *cs = CPU(cpu);
> @@ -267,6 +267,9 @@ static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
>       cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
>       kvmppc_set_papr(cpu);
>   
> +    env->spr_cb[SPR_PIR].default_value = cs->cpu_index;
> +    env->spr_cb[SPR_TIR].default_value = thread_index;
> +
>       /* Set time-base frequency to 512 MHz. vhyp must be set first. */
>       cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>   
> @@ -337,7 +340,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>       for (i = 0; i < cc->nr_threads; i++) {
>           sc->threads[i] = spapr_create_vcpu(sc, i, errp);
>           if (!sc->threads[i] ||
> -            !spapr_realize_vcpu(sc->threads[i], spapr, sc, errp)) {
> +            !spapr_realize_vcpu(sc->threads[i], spapr, sc, i, errp)) {
>               spapr_cpu_core_unrealize(dev);
>               return;
>           }

Re: [PATCH 4/4] spapr: Allow up to 8 threads SMT on POWER8 and newer

[Nicholas Piggin]

On Tue Jun 20, 2023 at 7:27 PM AEST, Harsh Prateek Bora wrote:
>
>
> On 6/5/23 16:53, Nicholas Piggin wrote:
> > PPC TCG now supports multi-threaded CPU configuration for non-hypervisor
> > state. This requires PIR and TIR be set, because that's how sibling thread
> > matching is done.
> > 
> > spapr's nested-HV capability does not currently coexist with SMT. This
> > is quite analogous to LPAR-per-core mode on real hardware which also
> > does not support KVM.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   hw/ppc/spapr.c          | 16 ++++++++++++----
> >   hw/ppc/spapr_caps.c     | 14 ++++++++++++++
> >   hw/ppc/spapr_cpu_core.c |  7 +++++--
> >   3 files changed, 31 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index dcb7f1c70a..deb8b507e3 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2524,10 +2524,18 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp)
> >       int ret;
> >       unsigned int smp_threads = ms->smp.threads;
> >   
> > -    if (!kvm_enabled() && (smp_threads > 1)) {
> > -        error_setg(errp, "TCG cannot support more than 1 thread/core "
> > -                   "on a pseries machine");
> > -        return;
> > +    if (!kvm_enabled()) {
>
> Can we make it a check for tcg_enabled() which would be more appropriate 
> or as Cedric suggested, may be include this one along with your series:
>
> https://lore.kernel.org/qemu-devel/20230620074802.86898-1-philmd@linaro.org/

Good point, I'll keep it in mind.

Thanks,
Nick

Re: [PATCH 0/4] target/ppc: TCG SMT support for spapr

[Nicholas Piggin]

On Wed Jun 7, 2023 at 12:09 AM AEST, Cédric Le Goater wrote:
> On 6/5/23 13:23, Nicholas Piggin wrote:
> > Previous RFC here
> > 
> > https://lists.gnu.org/archive/html/qemu-ppc/2023-05/msg00453.html
> > 
> > This series drops patch 1 from the previous, which is more of
> > a standalone bugfix.
> > 
> > Also accounted for Cedric's comments, except a nicer way to
> > set cpu_index vs PIR/TIR SPRs, which is not quite trivial.
> > 
> > This limits support for SMT to POWER8 and newer. It is also
> > incompatible with nested-HV so that is checked for too.
> > 
> > Iterating CPUs to find siblings for now I kept because similar
> > loops exist in a few places, and it is not conceptually
> > difficult for SMT, just fiddly code to improve. For now it
> > should not be much performane concern.
> > 
> > I removed hypervisor msgsnd support from patch 3, which is not
> > required for spapr and added significantly to the patch.
> > 
> > For now nobody has objected to the way shared SPR access is
> > handled (serialised with TCG atomics support) so we'll keep
> > going with it.
>
> Cc:ing more people for possible feedback.

Not much feedback so I'll plan to go with this.

A more performant implementation might try to synchronize
threads at the register level rather than serialize everything,
but SMT shared registers are not too performance critical so
this should do for now.

Thanks,
Nick

Re: [PATCH 0/4] target/ppc: TCG SMT support for spapr

[Cédric Le Goater]

On 6/20/23 12:12, Nicholas Piggin wrote:
> On Wed Jun 7, 2023 at 12:09 AM AEST, Cédric Le Goater wrote:
>> On 6/5/23 13:23, Nicholas Piggin wrote:
>>> Previous RFC here
>>>
>>> https://lists.gnu.org/archive/html/qemu-ppc/2023-05/msg00453.html
>>>
>>> This series drops patch 1 from the previous, which is more of
>>> a standalone bugfix.
>>>
>>> Also accounted for Cedric's comments, except a nicer way to
>>> set cpu_index vs PIR/TIR SPRs, which is not quite trivial.
>>>
>>> This limits support for SMT to POWER8 and newer. It is also
>>> incompatible with nested-HV so that is checked for too.
>>>
>>> Iterating CPUs to find siblings for now I kept because similar
>>> loops exist in a few places, and it is not conceptually
>>> difficult for SMT, just fiddly code to improve. For now it
>>> should not be much performane concern.
>>>
>>> I removed hypervisor msgsnd support from patch 3, which is not
>>> required for spapr and added significantly to the patch.
>>>
>>> For now nobody has objected to the way shared SPR access is
>>> handled (serialised with TCG atomics support) so we'll keep
>>> going with it.
>>
>> Cc:ing more people for possible feedback.
> 
> Not much feedback so I'll plan to go with this.
> 
> A more performant implementation might try to synchronize
> threads at the register level rather than serialize everything,
> but SMT shared registers are not too performance critical so
> this should do for now.

yes. Could you please rebase this series on upstream ?

It would be good to add tests for SMT. May be we could extend :

   tests/avocado/ppc_pseries.py

with a couple of extra QEMU configs adding 'threads=' (if possible) and
check :

   "CPU maps initialized for Y threads per core"

and

   "smp: Brought up 1 node, X*Y CPUs"

?

Thanks,

C.

Re: [PATCH 0/4] target/ppc: TCG SMT support for spapr

[Nicholas Piggin]

On Tue Jun 20, 2023 at 8:27 PM AEST, Cédric Le Goater wrote:
> On 6/20/23 12:12, Nicholas Piggin wrote:
> > On Wed Jun 7, 2023 at 12:09 AM AEST, Cédric Le Goater wrote:
> >> On 6/5/23 13:23, Nicholas Piggin wrote:
> >>> Previous RFC here
> >>>
> >>> https://lists.gnu.org/archive/html/qemu-ppc/2023-05/msg00453.html
> >>>
> >>> This series drops patch 1 from the previous, which is more of
> >>> a standalone bugfix.
> >>>
> >>> Also accounted for Cedric's comments, except a nicer way to
> >>> set cpu_index vs PIR/TIR SPRs, which is not quite trivial.
> >>>
> >>> This limits support for SMT to POWER8 and newer. It is also
> >>> incompatible with nested-HV so that is checked for too.
> >>>
> >>> Iterating CPUs to find siblings for now I kept because similar
> >>> loops exist in a few places, and it is not conceptually
> >>> difficult for SMT, just fiddly code to improve. For now it
> >>> should not be much performane concern.
> >>>
> >>> I removed hypervisor msgsnd support from patch 3, which is not
> >>> required for spapr and added significantly to the patch.
> >>>
> >>> For now nobody has objected to the way shared SPR access is
> >>> handled (serialised with TCG atomics support) so we'll keep
> >>> going with it.
> >>
> >> Cc:ing more people for possible feedback.
> > 
> > Not much feedback so I'll plan to go with this.
> > 
> > A more performant implementation might try to synchronize
> > threads at the register level rather than serialize everything,
> > but SMT shared registers are not too performance critical so
> > this should do for now.
>
> yes. Could you please rebase this series on upstream ?

Oh yeah I should have said, I will rebase and resend.

> It would be good to add tests for SMT. May be we could extend :
>
>    tests/avocado/ppc_pseries.py
>
> with a couple of extra QEMU configs adding 'threads=' (if possible) and
> check :
>
>    "CPU maps initialized for Y threads per core"
>
> and
>
>    "smp: Brought up 1 node, X*Y CPUs"
>
> ?

Yeah that could be a good idea, I'll try it.

Thanks,
Nick

Re: [PATCH 0/4] target/ppc: TCG SMT support for spapr

[Cédric Le Goater]

On 6/20/23 12:45, Nicholas Piggin wrote:
> On Tue Jun 20, 2023 at 8:27 PM AEST, Cédric Le Goater wrote:
>> On 6/20/23 12:12, Nicholas Piggin wrote:
>>> On Wed Jun 7, 2023 at 12:09 AM AEST, Cédric Le Goater wrote:
>>>> On 6/5/23 13:23, Nicholas Piggin wrote:
>>>>> Previous RFC here
>>>>>
>>>>> https://lists.gnu.org/archive/html/qemu-ppc/2023-05/msg00453.html
>>>>>
>>>>> This series drops patch 1 from the previous, which is more of
>>>>> a standalone bugfix.
>>>>>
>>>>> Also accounted for Cedric's comments, except a nicer way to
>>>>> set cpu_index vs PIR/TIR SPRs, which is not quite trivial.
>>>>>
>>>>> This limits support for SMT to POWER8 and newer. It is also
>>>>> incompatible with nested-HV so that is checked for too.
>>>>>
>>>>> Iterating CPUs to find siblings for now I kept because similar
>>>>> loops exist in a few places, and it is not conceptually
>>>>> difficult for SMT, just fiddly code to improve. For now it
>>>>> should not be much performane concern.
>>>>>
>>>>> I removed hypervisor msgsnd support from patch 3, which is not
>>>>> required for spapr and added significantly to the patch.
>>>>>
>>>>> For now nobody has objected to the way shared SPR access is
>>>>> handled (serialised with TCG atomics support) so we'll keep
>>>>> going with it.
>>>>
>>>> Cc:ing more people for possible feedback.
>>>
>>> Not much feedback so I'll plan to go with this.
>>>
>>> A more performant implementation might try to synchronize
>>> threads at the register level rather than serialize everything,
>>> but SMT shared registers are not too performance critical so
>>> this should do for now.
>>
>> yes. Could you please rebase this series on upstream ?
> 
> Oh yeah I should have said, I will rebase and resend.

Here is a tree to check the rebase on :

   https://github.com/legoater/qemu/commits/ppc-next

>> It would be good to add tests for SMT. May be we could extend :
>>
>>     tests/avocado/ppc_pseries.py
>>
>> with a couple of extra QEMU configs adding 'threads=' (if possible) and
>> check :
>>
>>     "CPU maps initialized for Y threads per core"
>>
>> and
>>
>>     "smp: Brought up 1 node, X*Y CPUs"
>>
>> ?
> 
> Yeah that could be a good idea, I'll try it.

It can come later.

Thanks,

C.