[PATCH 0/3] target/ppc: Fixes for TCG TLB modeling of some MMU SPRs

[PATCH 0/3] target/ppc: Fixes for TCG TLB modeling of some MMU SPRs

[Nicholas Piggin]

Any register or memory value that is used by the .tlb_fill
function (e.g., in ppc_xlate()) can affect what gets put in TCG's
TLB, so changing it requires either: that the ISA permits cached
address translations that become incoherent vs the changed value;
that TCG TLB is "tagged" with the changing value (e.g., with mmuidx);
or that the TCG TLB is flushed.

ppc is missing a few such flushes. Other than the AMR flush, Linux/KVM
probably covers such SPR changes with other flushes (e.g., context
switching between guests or guest/host will update LPCR and LPIDR and
LPIDR update already causes a TLB flush), which explains why they
haven't caused obvious bugs.

Thanks,
Nick

Nicholas Piggin (3):
  target/ppc: flush TLB on HRMOR and LPCR SPR updates
  target/ppc: Avoid work if MMU SPRs are written with same value
  target/ppc: add missing TLB flushes for memory protection key SPR
    updates

 target/ppc/helper.h      |  3 ++
 target/ppc/spr_common.h  |  1 +
 target/ppc/cpu.c         | 12 +++++-
 target/ppc/cpu_init.c    |  6 +--
 target/ppc/misc_helper.c | 85 +++++++++++++++++++++++++++++++++++++++-
 target/ppc/translate.c   | 62 ++++++-----------------------
 6 files changed, 114 insertions(+), 55 deletions(-)

-- 
2.47.1

[PATCH 1/3] target/ppc: flush TLB on HRMOR and LPCR SPR updates

[Nicholas Piggin]

The HRMOR and LPCR registers are involved with MMU translations that
are not tagged in the TLB (i.e., with mmuidx), so the TLB needs to be
flushed when these are changed, e.g., as PIDR, LPIDR already do.
target/ppc: add missing TLB flushes for MMU SPR updates

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/helper.h      |  1 +
 target/ppc/spr_common.h  |  1 +
 target/ppc/cpu.c         |  4 ++++
 target/ppc/cpu_init.c    |  2 +-
 target/ppc/misc_helper.c | 23 +++++++++++++++++++++++
 target/ppc/translate.c   | 10 ++++++++++
 6 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 5a77e761bd3..6178ebe138f 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -723,6 +723,7 @@ DEF_HELPER_FLAGS_1(load_vtb, TCG_CALL_NO_RWG, tl, env)
 #if defined(TARGET_PPC64)
 DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
 DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl)
+DEF_HELPER_2(store_hrmor, void, env, tl)
 DEF_HELPER_2(store_ptcr, void, env, tl)
 DEF_HELPER_FLAGS_1(load_dpdes, TCG_CALL_NO_RWG, tl, env)
 DEF_HELPER_FLAGS_2(store_dpdes, TCG_CALL_NO_RWG, void, env, tl)
diff --git a/target/ppc/spr_common.h b/target/ppc/spr_common.h
index 01aff449bcc..8cac82b2dac 100644
--- a/target/ppc/spr_common.h
+++ b/target/ppc/spr_common.h
@@ -177,6 +177,7 @@ void spr_write_pidr(DisasContext *ctx, int sprn, int gprn);
 void spr_write_lpidr(DisasContext *ctx, int sprn, int gprn);
 void spr_read_hior(DisasContext *ctx, int gprn, int sprn);
 void spr_write_hior(DisasContext *ctx, int sprn, int gprn);
+void spr_write_hrmor(DisasContext *ctx, int sprn, int gprn);
 void spr_write_ptcr(DisasContext *ctx, int sprn, int gprn);
 void spr_write_pcr(DisasContext *ctx, int sprn, int gprn);
 void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn);
diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index d148cd76b47..cdd50cb36d6 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -21,6 +21,7 @@
 #include "cpu.h"
 #include "cpu-models.h"
 #include "cpu-qom.h"
+#include "exec/exec-all.h"
 #include "exec/log.h"
 #include "fpu/softfloat-helpers.h"
 #include "mmu-hash64.h"
@@ -101,6 +102,9 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
     /* The gtse bit affects hflags */
     hreg_compute_hflags(env);
 
+    /* Various untagged bits affect translation (e.g., TC, HR, etc). */
+    tlb_flush(env_cpu(env));
+
     ppc_maybe_interrupt(env);
 }
 
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 062a6e85fba..92316b55afd 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -5496,7 +5496,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
     spr_register_hv(env, SPR_HRMOR, "HRMOR",
                  SPR_NOACCESS, SPR_NOACCESS,
                  SPR_NOACCESS, SPR_NOACCESS,
-                 &spr_read_generic, &spr_core_write_generic,
+                 &spr_read_generic, &spr_write_hrmor,
                  0x00000000);
 }
 
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index f0ca80153b2..179e8b6b4d2 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -169,6 +169,29 @@ void helper_store_sdr1(CPUPPCState *env, target_ulong val)
 }
 
 #if defined(TARGET_PPC64)
+void helper_store_hrmor(CPUPPCState *env, target_ulong val)
+{
+    if (env->spr[SPR_HRMOR] != val) {
+        CPUState *cs = env_cpu(env);
+
+        qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, val);
+
+        if (ppc_cpu_lpar_single_threaded(cs)) {
+            env->spr[SPR_HRMOR] = val;
+            tlb_flush(cs);
+        } else {
+            CPUState *ccs;
+
+            THREAD_SIBLING_FOREACH(cs, ccs) {
+                PowerPCCPU *ccpu = POWERPC_CPU(ccs);
+                CPUPPCState *cenv = &ccpu->env;
+                cenv->spr[SPR_HRMOR] = val;
+                tlb_flush(ccs);
+            }
+        }
+    }
+}
+
 void helper_store_ptcr(CPUPPCState *env, target_ulong val)
 {
     if (env->spr[SPR_PTCR] != val) {
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 80638ab5359..ac910151cfa 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -909,6 +909,16 @@ void spr_write_hior(DisasContext *ctx, int sprn, int gprn)
     tcg_gen_andi_tl(t0, cpu_gpr[gprn], 0x3FFFFF00000ULL);
     tcg_gen_st_tl(t0, tcg_env, offsetof(CPUPPCState, excp_prefix));
 }
+
+void spr_write_hrmor(DisasContext *ctx, int sprn, int gprn)
+{
+    if (!gen_serialize_core(ctx)) {
+        return;
+    }
+
+    gen_helper_store_hrmor(tcg_env, cpu_gpr[gprn]);
+}
+
 void spr_write_ptcr(DisasContext *ctx, int sprn, int gprn)
 {
     if (!gen_serialize_core(ctx)) {
-- 
2.47.1

[PATCH 2/3] target/ppc: Avoid work if MMU SPRs are written with same value

[Nicholas Piggin]

Avoid TLB flushing and hflags recomputation if LPCR, LPIDR, or PIDR
are written with the same value. This is observed to happen in some
cases (e.g., in hypervisor real-mode exit fastpath handlers).

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/cpu.c         |  8 +++++++-
 target/ppc/misc_helper.c | 14 +++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index cdd50cb36d6..0fa2ccfcb2f 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -97,8 +97,14 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
+    target_ulong old, new;
 
-    env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
+    old = env->spr[SPR_LPCR];
+    new = val & pcc->lpcr_mask;
+    if (old == new) {
+        return;
+    }
+    env->spr[SPR_LPCR] = new;
     /* The gtse bit affects hflags */
     hreg_compute_hflags(env);
 
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index 179e8b6b4d2..ac439e00326 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -403,12 +403,24 @@ void helper_store_sprd(CPUPPCState *env, target_ulong val)
 
 void helper_store_pidr(CPUPPCState *env, target_ulong val)
 {
+    if (env->spr[SPR_BOOKS_PID] == (uint32_t)val) {
+        return;
+    }
+
     env->spr[SPR_BOOKS_PID] = (uint32_t)val;
-    tlb_flush(env_cpu(env));
+
+    if (env->spr[SPR_LPCR] & LPCR_HR) {
+        /* PID is only relevant to CPU translations when LPCR[HR]=1 */
+        tlb_flush(env_cpu(env));
+    }
 }
 
 void helper_store_lpidr(CPUPPCState *env, target_ulong val)
 {
+    if (env->spr[SPR_LPIDR] == (uint32_t)val) {
+        return;
+    }
+
     env->spr[SPR_LPIDR] = (uint32_t)val;
 
     /*
-- 
2.47.1

[PATCH 3/3] target/ppc: add missing TLB flushes for memory protection key SPR updates

[Nicholas Piggin]

The IAMR and AMR registers are involved with MMU translations that
are not tagged in the TLB (i.e., with mmuidx), so the TLB needs to be
flushed when these are changed, e.g., as PIDR, LPIDR already do.

This moves AMR and IAMR write to helpers rather than use tlb_need_flush
because they can be written in problem state where tlb_need_flush is not
checked.

XXX: As far as I can tell this is needed for correct memory protection
key operation, however it seems to be causing slowdowns when booting
Linux, enough to cause failures due to timeouts, so I will not merge it
at the moment. I have been considering possible ways to speed this up
e.g., with mmu indexes, but that's not entirely trivial and needs a bit
more work.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/helper.h      |  2 ++
 target/ppc/cpu_init.c    |  4 ++--
 target/ppc/misc_helper.c | 48 +++++++++++++++++++++++++++++++++++++
 target/ppc/translate.c   | 52 ++--------------------------------------
 4 files changed, 54 insertions(+), 52 deletions(-)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 6178ebe138f..e8de4f95581 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -723,6 +723,8 @@ DEF_HELPER_FLAGS_1(load_vtb, TCG_CALL_NO_RWG, tl, env)
 #if defined(TARGET_PPC64)
 DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
 DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl)
+DEF_HELPER_2(store_amr, void, env, tl)
+DEF_HELPER_2(store_iamr, void, env, tl)
 DEF_HELPER_2(store_hrmor, void, env, tl)
 DEF_HELPER_2(store_ptcr, void, env, tl)
 DEF_HELPER_FLAGS_1(load_dpdes, TCG_CALL_NO_RWG, tl, env)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 92316b55afd..43af471ae64 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -238,7 +238,7 @@ static void register_amr_sprs(CPUPPCState *env)
     spr_register_kvm_hv(env, SPR_AMR, "AMR",
                      SPR_NOACCESS, SPR_NOACCESS,
                      &spr_read_generic, &spr_write_amr,
-                     &spr_read_generic, &spr_write_generic,
+                     &spr_read_generic, &spr_write_amr,
                      KVM_REG_PPC_AMR, 0);
     spr_register_kvm_hv(env, SPR_UAMOR, "UAMOR",
                      SPR_NOACCESS, SPR_NOACCESS,
@@ -259,7 +259,7 @@ static void register_iamr_sprs(CPUPPCState *env)
     spr_register_kvm_hv(env, SPR_IAMR, "IAMR",
                         SPR_NOACCESS, SPR_NOACCESS,
                         &spr_read_generic, &spr_write_iamr,
-                        &spr_read_generic, &spr_write_generic,
+                        &spr_read_generic, &spr_write_iamr,
                         KVM_REG_PPC_IAMR, 0);
 #endif /* !CONFIG_USER_ONLY */
 }
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index ac439e00326..dfc9d806b30 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -169,6 +169,54 @@ void helper_store_sdr1(CPUPPCState *env, target_ulong val)
 }
 
 #if defined(TARGET_PPC64)
+void helper_store_amr(CPUPPCState *env, target_ulong val)
+{
+    target_ulong old, new, mask;
+
+    if (FIELD_EX64(env->msr, MSR, PR)) {
+        mask = env->spr[SPR_UAMOR];
+    } else if (FIELD_EX64(env->msr, MSR, HV)) {
+        mask = (target_ulong)-1;
+    } else {
+        mask = env->spr[SPR_AMOR];
+    }
+
+    old = env->spr[SPR_AMR];
+    /* Replace controllable bits with those in val */
+    new = (old & ~mask) | (val & mask);
+
+    if (old != new) {
+        CPUState *cs = env_cpu(env);
+        env->spr[SPR_AMR] = new;
+        /* AMR is involved in MMU translations so must flush TLB */
+        tlb_flush(cs);
+    }
+}
+
+void helper_store_iamr(CPUPPCState *env, target_ulong val)
+{
+    target_ulong old, new, mask;
+
+    if (FIELD_EX64(env->msr, MSR, PR)) {
+        g_assert_not_reached(); /* mtIAMR is privileged */
+    } else if (FIELD_EX64(env->msr, MSR, HV)) {
+        mask = (target_ulong)-1;
+    } else {
+        mask = env->spr[SPR_AMOR];
+    }
+
+    old = env->spr[SPR_IAMR];
+    /* Replace controllable bits with those in val */
+    new = (old & ~mask) | (val & mask);
+
+    if (old != new) {
+        CPUState *cs = env_cpu(env);
+        env->spr[SPR_IAMR] = new;
+        /* IAMR is involved in MMU translations so must flush TLB */
+        tlb_flush(cs);
+    }
+}
+
 void helper_store_hrmor(CPUPPCState *env, target_ulong val)
 {
     if (env->spr[SPR_HRMOR] != val) {
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index ac910151cfa..c5fe3de64e9 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1080,33 +1080,7 @@ void spr_write_excp_vector(DisasContext *ctx, int sprn, int gprn)
 #ifndef CONFIG_USER_ONLY
 void spr_write_amr(DisasContext *ctx, int sprn, int gprn)
 {
-    TCGv t0 = tcg_temp_new();
-    TCGv t1 = tcg_temp_new();
-    TCGv t2 = tcg_temp_new();
-
-    /*
-     * Note, the HV=1 PR=0 case is handled earlier by simply using
-     * spr_write_generic for HV mode in the SPR table
-     */
-
-    /* Build insertion mask into t1 based on context */
-    if (ctx->pr) {
-        gen_load_spr(t1, SPR_UAMOR);
-    } else {
-        gen_load_spr(t1, SPR_AMOR);
-    }
-
-    /* Mask new bits into t2 */
-    tcg_gen_and_tl(t2, t1, cpu_gpr[gprn]);
-
-    /* Load AMR and clear new bits in t0 */
-    gen_load_spr(t0, SPR_AMR);
-    tcg_gen_andc_tl(t0, t0, t1);
-
-    /* Or'in new bits and write it out */
-    tcg_gen_or_tl(t0, t0, t2);
-    gen_store_spr(SPR_AMR, t0);
-    spr_store_dump_spr(SPR_AMR);
+    gen_helper_store_amr(tcg_env, cpu_gpr[gprn]);
 }
 
 void spr_write_uamor(DisasContext *ctx, int sprn, int gprn)
@@ -1138,29 +1112,7 @@ void spr_write_uamor(DisasContext *ctx, int sprn, int gprn)
 
 void spr_write_iamr(DisasContext *ctx, int sprn, int gprn)
 {
-    TCGv t0 = tcg_temp_new();
-    TCGv t1 = tcg_temp_new();
-    TCGv t2 = tcg_temp_new();
-
-    /*
-     * Note, the HV=1 case is handled earlier by simply using
-     * spr_write_generic for HV mode in the SPR table
-     */
-
-    /* Build insertion mask into t1 based on context */
-    gen_load_spr(t1, SPR_AMOR);
-
-    /* Mask new bits into t2 */
-    tcg_gen_and_tl(t2, t1, cpu_gpr[gprn]);
-
-    /* Load AMR and clear new bits in t0 */
-    gen_load_spr(t0, SPR_IAMR);
-    tcg_gen_andc_tl(t0, t0, t1);
-
-    /* Or'in new bits and write it out */
-    tcg_gen_or_tl(t0, t0, t2);
-    gen_store_spr(SPR_IAMR, t0);
-    spr_store_dump_spr(SPR_IAMR);
+    gen_helper_store_iamr(tcg_env, cpu_gpr[gprn]);
 }
 #endif
 #endif
-- 
2.47.1