[PATCH 1/6] target/ppc: Fix width of some 32-bit SPRs

[PATCH 1/6] target/ppc: Fix width of some 32-bit SPRs

[Nicholas Piggin]

Some 32-bit SPRs are incorrectly implemented as 64-bits on 64-bit
targets.

This changes VRSAVE, DSISR, HDSISR, DAWRX0, PIDR, LPIDR, DEXCR,
HDEXCR, CTRL, TSCR, MMCRH, and PMC[1-6] from to be 32-bit registers.

This only goes by the 32/64 classification in the architecture, it
does not try to implement finer details of SPR implementation (e.g.,
not all bits implemented as simple read/write storage).

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/cpu_init.c    | 18 +++++++++---------
 target/ppc/helper_regs.c |  2 +-
 target/ppc/misc_helper.c |  4 ++--
 target/ppc/power8-pmu.c  |  2 +-
 target/ppc/translate.c   |  2 +-
 5 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 0ce2e3c91d..5aa0b3f0f1 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -5085,8 +5085,8 @@ static void register_book3s_altivec_sprs(CPUPPCState *env)
     }
 
     spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
-                     &spr_read_generic, &spr_write_generic,
-                     &spr_read_generic, &spr_write_generic,
+                     &spr_read_generic, &spr_write_generic32,
+                     &spr_read_generic, &spr_write_generic32,
                      KVM_REG_PPC_VRSAVE, 0x00000000);
 
 }
@@ -5120,7 +5120,7 @@ static void register_book3s_207_dbg_sprs(CPUPPCState *env)
     spr_register_kvm_hv(env, SPR_DAWRX0, "DAWRX0",
                         SPR_NOACCESS, SPR_NOACCESS,
                         SPR_NOACCESS, SPR_NOACCESS,
-                        &spr_read_generic, &spr_write_generic,
+                        &spr_read_generic, &spr_write_generic32,
                         KVM_REG_PPC_DAWRX, 0x00000000);
     spr_register_kvm_hv(env, SPR_CIABR, "CIABR",
                         SPR_NOACCESS, SPR_NOACCESS,
@@ -5376,7 +5376,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
     spr_register_hv(env, SPR_TSCR, "TSCR",
                  SPR_NOACCESS, SPR_NOACCESS,
                  SPR_NOACCESS, SPR_NOACCESS,
-                 &spr_read_generic, &spr_write_generic,
+                 &spr_read_generic, &spr_write_generic32,
                  0x00000000);
     spr_register_hv(env, SPR_HMER, "HMER",
                  SPR_NOACCESS, SPR_NOACCESS,
@@ -5406,7 +5406,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
     spr_register_hv(env, SPR_MMCRC, "MMCRC",
                  SPR_NOACCESS, SPR_NOACCESS,
                  SPR_NOACCESS, SPR_NOACCESS,
-                 &spr_read_generic, &spr_write_generic,
+                 &spr_read_generic, &spr_write_generic32,
                  0x00000000);
     spr_register_hv(env, SPR_MMCRH, "MMCRH",
                  SPR_NOACCESS, SPR_NOACCESS,
@@ -5441,7 +5441,7 @@ static void register_book3s_ids_sprs(CPUPPCState *env)
     spr_register_hv(env, SPR_HDSISR, "HDSISR",
                  SPR_NOACCESS, SPR_NOACCESS,
                  SPR_NOACCESS, SPR_NOACCESS,
-                 &spr_read_generic, &spr_write_generic,
+                 &spr_read_generic, &spr_write_generic32,
                  0x00000000);
     spr_register_hv(env, SPR_HRMOR, "HRMOR",
                  SPR_NOACCESS, SPR_NOACCESS,
@@ -5665,7 +5665,7 @@ static void register_power7_book4_sprs(CPUPPCState *env)
                      KVM_REG_PPC_ACOP, 0);
     spr_register_kvm(env, SPR_BOOKS_PID, "PID",
                      SPR_NOACCESS, SPR_NOACCESS,
-                     &spr_read_generic, &spr_write_generic,
+                     &spr_read_generic, &spr_write_generic32,
                      KVM_REG_PPC_PID, 0);
 #endif
 }
@@ -5730,7 +5730,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
 {
     spr_register(env, SPR_DEXCR, "DEXCR",
             SPR_NOACCESS, SPR_NOACCESS,
-            &spr_read_generic, &spr_write_generic,
+            &spr_read_generic, &spr_write_generic32,
             0);
 
     spr_register(env, SPR_UDEXCR, "DEXCR",
@@ -5741,7 +5741,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
     spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
             SPR_NOACCESS, SPR_NOACCESS,
             SPR_NOACCESS, SPR_NOACCESS,
-            &spr_read_generic, &spr_write_generic,
+            &spr_read_generic, &spr_write_generic32,
             0);
 
     spr_register(env, SPR_UHDEXCR, "HDEXCR",
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index 779e7db513..fb351c303f 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -448,7 +448,7 @@ void register_non_embedded_sprs(CPUPPCState *env)
     /* Exception processing */
     spr_register_kvm(env, SPR_DSISR, "DSISR",
                      SPR_NOACCESS, SPR_NOACCESS,
-                     &spr_read_generic, &spr_write_generic,
+                     &spr_read_generic, &spr_write_generic32,
                      KVM_REG_PPC_DSISR, 0x00000000);
     spr_register_kvm(env, SPR_DAR, "DAR",
                      SPR_NOACCESS, SPR_NOACCESS,
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index a9bc1522e2..40ddc5c08c 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -190,13 +190,13 @@ void helper_store_dpdes(CPUPPCState *env, target_ulong val)
 
 void helper_store_pidr(CPUPPCState *env, target_ulong val)
 {
-    env->spr[SPR_BOOKS_PID] = val;
+    env->spr[SPR_BOOKS_PID] = (uint32_t)val;
     tlb_flush(env_cpu(env));
 }
 
 void helper_store_lpidr(CPUPPCState *env, target_ulong val)
 {
-    env->spr[SPR_LPIDR] = val;
+    env->spr[SPR_LPIDR] = (uint32_t)val;
 
     /*
      * We need to flush the TLB on LPID changes as we only tag HV vs
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 1381072b9e..64a64865d7 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -272,7 +272,7 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
 {
     pmu_update_cycles(env);
 
-    env->spr[sprn] = value;
+    env->spr[sprn] = (uint32_t)value;
 
     pmc_update_overflow_timer(env, sprn);
 }
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index df324fc7ff..58fa509057 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -413,7 +413,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
 
 void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
 {
-    spr_write_generic(ctx, sprn, gprn);
+    spr_write_generic32(ctx, sprn, gprn);
 
     /*
      * SPR_CTRL writes must force a new translation block,
-- 
2.37.2

[PATCH 2/6] target/ppc: Better CTRL SPR implementation

[Nicholas Piggin]

The CTRL register is able to write bit zero, and that is reflected in a
bit field in the register that reflects the state of all threads in the
core.

TCG does not implement SMT, so this just requires mirroring that bit into
the first bit of the thread state field.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/translate.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 58fa509057..d699acb3d0 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -413,7 +413,14 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
 
 void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
 {
-    spr_write_generic32(ctx, sprn, gprn);
+    /* This does not implement >1 thread */
+    TCGv t0 = tcg_temp_new();
+    TCGv t1 = tcg_temp_new();
+    tcg_gen_extract_tl(t0, cpu_gpr[gprn], 0, 1); /* Extract RUN field */
+    tcg_gen_shli_tl(t1, t0, 8); /* Duplicate the bit in TS */
+    tcg_gen_or_tl(t1, t1, t0);
+    gen_store_spr(sprn, t1);
+    spr_store_dump_spr(sprn);
 
     /*
      * SPR_CTRL writes must force a new translation block,
-- 
2.37.2

[PATCH 3/6] target/ppc: Fix instruction loading endianness in alignment interrupt

[Nicholas Piggin]

powerpc ifetch endianness depends on MSR[LE] so it has to byteswap
after cpu_ldl_code(). This corrects DSISR bits in alignment
interrupts when running in little endian mode.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/excp_helper.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 287659c74d..5f0e363363 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -133,6 +133,31 @@ static void dump_hcall(CPUPPCState *env)
                   env->nip);
 }
 
+/* Return true iff byteswap is needed in a scalar memop */
+static inline bool need_byteswap(CPUArchState *env)
+{
+#if TARGET_BIG_ENDIAN
+     return !!(env->msr & ((target_ulong)1 << MSR_LE));
+#else
+     return !(env->msr & ((target_ulong)1 << MSR_LE));
+#endif
+}
+
+static uint32_t ppc_ldl_code(CPUArchState *env, abi_ptr addr)
+{
+    uint32_t insn = cpu_ldl_code(env, addr);
+#if TARGET_BIG_ENDIAN
+    if (env->msr & ((target_ulong)1 << MSR_LE)) {
+        insn = bswap32(insn);
+    }
+#else
+    if (!(env->msr & ((target_ulong)1 << MSR_LE))) {
+        insn = bswap32(insn);
+    }
+#endif
+    return insn;
+}
+
 static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
 {
     const char *es;
@@ -3097,7 +3122,7 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
 
     /* Restore state and reload the insn we executed, for filling in DSISR.  */
     cpu_restore_state(cs, retaddr);
-    insn = cpu_ldl_code(env, env->nip);
+    insn = ppc_ldl_code(env, env->nip);
 
     switch (env->mmu_model) {
     case POWERPC_MMU_SOFT_4xx:
-- 
2.37.2

[PATCH 4/6] target/ppc: Alignment faults do not set DSISR in ISA v3.0 onward

[Nicholas Piggin]

This optional behavior was removed from the ISA in v3.0, see
Summary of Changes preface:

  Data Storage Interrupt Status Register for Alignment Interrupt:
  Simplifies the Alignment interrupt by remov- ing the Data Storage
  Interrupt Status Register (DSISR) from the set of registers modified
  by the Alignment interrupt.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/excp_helper.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 5f0e363363..c8b8eca3b1 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1456,13 +1456,22 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
         break;
     }
     case POWERPC_EXCP_ALIGN:     /* Alignment exception                      */
-        /* Get rS/rD and rA from faulting opcode */
-        /*
-         * Note: the opcode fields will not be set properly for a
-         * direct store load/store, but nobody cares as nobody
-         * actually uses direct store segments.
-         */
-        env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16;
+        switch (env->excp_model) {
+        case POWERPC_EXCP_970:
+        case POWERPC_EXCP_POWER7:
+        case POWERPC_EXCP_POWER8:
+            /* Get rS/rD and rA from faulting opcode */
+            /*
+             * Note: the opcode fields will not be set properly for a
+             * direct store load/store, but nobody cares as nobody
+             * actually uses direct store segments.
+             */
+            env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16;
+            break;
+        default:
+            /* Optional DSISR update was removed from ISA v3.0 */
+            break;
+        }
         break;
     case POWERPC_EXCP_PROGRAM:   /* Program exception                        */
         switch (env->error_code & ~0xF) {
-- 
2.37.2

[PATCH 5/6] target/ppc: Add SRR1 prefix indication to interrupt handlers

[Nicholas Piggin]

ISA v3.1 introduced prefix instructions. Among the changes, various
synchronous interrupts report whether they were caused by a prefix
instruction in (H)SRR1.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/excp_helper.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index c8b8eca3b1..2e0321ab69 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1353,12 +1353,26 @@ static bool books_vhyp_handles_hv_excp(PowerPCCPU *cpu)
     return false;
 }
 
+static bool is_prefix_excp(CPUPPCState *env, uint32_t insn)
+{
+    switch (env->excp_model) {
+    case POWERPC_EXCP_970:
+    case POWERPC_EXCP_POWER7:
+    case POWERPC_EXCP_POWER8:
+    case POWERPC_EXCP_POWER9:
+        return false;
+    default: /* POWER10 / ISAv3.1 onward */
+        return ((insn & 0xfc000000) == 0x04000000);
+    }
+}
+
 static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     target_ulong msr, new_msr, vector;
     int srr0, srr1, lev = -1;
+    uint32_t insn = 0;
 
     /* new srr1 value excluding must-be-zero bits */
     msr = env->msr & ~0x783f0000ULL;
@@ -1397,6 +1411,29 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
 
     vector |= env->excp_prefix;
 
+    switch (excp) {
+    case POWERPC_EXCP_MCHECK:
+    case POWERPC_EXCP_DSI:
+    case POWERPC_EXCP_DSEG:
+    case POWERPC_EXCP_ALIGN:
+    case POWERPC_EXCP_PROGRAM:
+    case POWERPC_EXCP_FPU:
+    case POWERPC_EXCP_TRACE:
+    case POWERPC_EXCP_HDSI:
+    case POWERPC_EXCP_HV_EMU:
+    case POWERPC_EXCP_VPU:
+    case POWERPC_EXCP_VSXU:
+    case POWERPC_EXCP_FU:
+    case POWERPC_EXCP_HV_FU:
+        insn = ppc_ldl_code(env, env->nip);
+        if (is_prefix_excp(env, insn)) {
+            msr |= PPC_BIT(34);
+        }
+        break;
+    default:
+        break;
+    }
+
     switch (excp) {
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
-- 
2.37.2

[PATCH 6/6] target/ppc: Implement HEIR SPR

[Nicholas Piggin]

The hypervisor emulation assistance interrupt modifies HEIR to
contain the value of the instruction which caused the exception.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/cpu.h         |  1 +
 target/ppc/cpu_init.c    | 23 +++++++++++++++++++++++
 target/ppc/excp_helper.c | 12 +++++++++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 557d736dab..8c4a203ecb 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1653,6 +1653,7 @@ void ppc_compat_add_property(Object *obj, const char *name,
 #define SPR_HMER              (0x150)
 #define SPR_HMEER             (0x151)
 #define SPR_PCR               (0x152)
+#define SPR_HEIR              (0x153)
 #define SPR_BOOKE_LPIDR       (0x152)
 #define SPR_BOOKE_TCR         (0x154)
 #define SPR_BOOKE_TLB0PS      (0x158)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 5aa0b3f0f1..ff73be1812 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -1629,6 +1629,7 @@ static void register_8xx_sprs(CPUPPCState *env)
  * HSRR0   => SPR 314 (Power 2.04 hypv)
  * HSRR1   => SPR 315 (Power 2.04 hypv)
  * LPIDR   => SPR 317 (970)
+ * HEIR    => SPR 339 (Power 2.05 hypv) (64-bit reg from 3.1)
  * EPR     => SPR 702 (Power 2.04 emb)
  * perf    => 768-783 (Power 2.04)
  * perf    => 784-799 (Power 2.04)
@@ -5522,6 +5523,24 @@ static void register_power6_common_sprs(CPUPPCState *env)
                  0x00000000);
 }
 
+static void register_HEIR32_spr(CPUPPCState *env)
+{
+    spr_register_hv(env, SPR_HEIR, "HEIR",
+                 SPR_NOACCESS, SPR_NOACCESS,
+                 SPR_NOACCESS, SPR_NOACCESS,
+                 &spr_read_generic, &spr_write_generic32,
+                 0x00000000);
+}
+
+static void register_HEIR64_spr(CPUPPCState *env)
+{
+    spr_register_hv(env, SPR_HEIR, "HEIR",
+                 SPR_NOACCESS, SPR_NOACCESS,
+                 SPR_NOACCESS, SPR_NOACCESS,
+                 &spr_read_generic, &spr_write_generic,
+                 0x00000000);
+}
+
 static void register_power8_tce_address_control_sprs(CPUPPCState *env)
 {
     spr_register_kvm(env, SPR_TAR, "TAR",
@@ -5950,6 +5969,7 @@ static void init_proc_POWER7(CPUPPCState *env)
     register_power5p_ear_sprs(env);
     register_power5p_tb_sprs(env);
     register_power6_common_sprs(env);
+    register_HEIR32_spr(env);
     register_power6_dbg_sprs(env);
     register_power7_book4_sprs(env);
 
@@ -6072,6 +6092,7 @@ static void init_proc_POWER8(CPUPPCState *env)
     register_power5p_ear_sprs(env);
     register_power5p_tb_sprs(env);
     register_power6_common_sprs(env);
+    register_HEIR32_spr(env);
     register_power6_dbg_sprs(env);
     register_power8_tce_address_control_sprs(env);
     register_power8_ids_sprs(env);
@@ -6234,6 +6255,7 @@ static void init_proc_POWER9(CPUPPCState *env)
     register_power5p_ear_sprs(env);
     register_power5p_tb_sprs(env);
     register_power6_common_sprs(env);
+    register_HEIR32_spr(env);
     register_power6_dbg_sprs(env);
     register_power8_tce_address_control_sprs(env);
     register_power8_ids_sprs(env);
@@ -6409,6 +6431,7 @@ static void init_proc_POWER10(CPUPPCState *env)
     register_power5p_ear_sprs(env);
     register_power5p_tb_sprs(env);
     register_power6_common_sprs(env);
+    register_HEIR64_spr(env);
     register_power6_dbg_sprs(env);
     register_power8_tce_address_control_sprs(env);
     register_power8_ids_sprs(env);
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 2e0321ab69..d206903562 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1614,13 +1614,23 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
     case POWERPC_EXCP_HDECR:     /* Hypervisor decrementer exception         */
     case POWERPC_EXCP_HDSI:      /* Hypervisor data storage exception        */
     case POWERPC_EXCP_SDOOR_HV:  /* Hypervisor Doorbell interrupt            */
-    case POWERPC_EXCP_HV_EMU:
     case POWERPC_EXCP_HVIRT:     /* Hypervisor virtualization                */
         srr0 = SPR_HSRR0;
         srr1 = SPR_HSRR1;
         new_msr |= (target_ulong)MSR_HVB;
         new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
         break;
+    case POWERPC_EXCP_HV_EMU:
+        env->spr[SPR_HEIR] = insn;
+        if (is_prefix_excp(env, insn)) {
+            uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
+            env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32;
+        }
+        srr0 = SPR_HSRR0;
+        srr1 = SPR_HSRR1;
+        new_msr |= (target_ulong)MSR_HVB;
+        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
+        break;
     case POWERPC_EXCP_VPU:       /* Vector unavailable exception             */
     case POWERPC_EXCP_VSXU:       /* VSX unavailable exception               */
     case POWERPC_EXCP_FU:         /* Facility unavailable exception          */
-- 
2.37.2

Re: [PATCH 3/6] target/ppc: Fix instruction loading endianness in alignment interrupt

[Fabiano Rosas]

Hi Nick,

> powerpc ifetch endianness depends on MSR[LE] so it has to byteswap
> after cpu_ldl_code(). This corrects DSISR bits in alignment
> interrupts when running in little endian mode.
>

Just a thought, we have these tests that perhaps could have caught
this:  https://github.com/legoater/pnv-test

Despite the name they do have (some) support to pseries as well. Not
sure how the P8 support is these days though.

> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  target/ppc/excp_helper.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 287659c74d..5f0e363363 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -133,6 +133,31 @@ static void dump_hcall(CPUPPCState *env)
>                    env->nip);
>  }
>  
> +/* Return true iff byteswap is needed in a scalar memop */
> +static inline bool need_byteswap(CPUArchState *env)
> +{
> +#if TARGET_BIG_ENDIAN

TARGET_BIG_ENDIAN is always set for softmmu mode. See
configs/targets/ppc64-softmmu.mak

> +     return !!(env->msr & ((target_ulong)1 << MSR_LE));
> +#else
> +     return !(env->msr & ((target_ulong)1 << MSR_LE));
> +#endif
> +}
> +
> +static uint32_t ppc_ldl_code(CPUArchState *env, abi_ptr addr)
> +{
> +    uint32_t insn = cpu_ldl_code(env, addr);
> +#if TARGET_BIG_ENDIAN
> +    if (env->msr & ((target_ulong)1 << MSR_LE)) {
> +        insn = bswap32(insn);
> +    }
> +#else
> +    if (!(env->msr & ((target_ulong)1 << MSR_LE))) {
> +        insn = bswap32(insn);
> +    }
> +#endif
> +    return insn;
> +}
> +
>  static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
>  {
>      const char *es;
> @@ -3097,7 +3122,7 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>  
>      /* Restore state and reload the insn we executed, for filling in DSISR.  */
>      cpu_restore_state(cs, retaddr);
> -    insn = cpu_ldl_code(env, env->nip);
> +    insn = ppc_ldl_code(env, env->nip);
>  
>      switch (env->mmu_model) {
>      case POWERPC_MMU_SOFT_4xx:

Re: [PATCH 4/6] target/ppc: Alignment faults do not set DSISR in ISA v3.0 onward

[Fabiano Rosas]

Nicholas Piggin <npiggin@gmail.com> writes:

> This optional behavior was removed from the ISA in v3.0, see
> Summary of Changes preface:
>
>   Data Storage Interrupt Status Register for Alignment Interrupt:
>   Simplifies the Alignment interrupt by remov- ing the Data Storage
>   Interrupt Status Register (DSISR) from the set of registers modified
>   by the Alignment interrupt.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  target/ppc/excp_helper.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 5f0e363363..c8b8eca3b1 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1456,13 +1456,22 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>          break;
>      }
>      case POWERPC_EXCP_ALIGN:     /* Alignment exception                      */
> -        /* Get rS/rD and rA from faulting opcode */
> -        /*
> -         * Note: the opcode fields will not be set properly for a
> -         * direct store load/store, but nobody cares as nobody
> -         * actually uses direct store segments.
> -         */
> -        env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16;
> +        switch (env->excp_model) {

Slightly better would be to check on (env->insn_flags2 & PPC2_ISA300).
We were trying to phase out the usage of "exception models" wherever
possible in favor of specific feature/isa level flags.

> +        case POWERPC_EXCP_970:
> +        case POWERPC_EXCP_POWER7:
> +        case POWERPC_EXCP_POWER8:
> +            /* Get rS/rD and rA from faulting opcode */
> +            /*
> +             * Note: the opcode fields will not be set properly for a
> +             * direct store load/store, but nobody cares as nobody
> +             * actually uses direct store segments.
> +             */
> +            env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16;
> +            break;
> +        default:
> +            /* Optional DSISR update was removed from ISA v3.0 */
> +            break;
> +        }
>          break;
>      case POWERPC_EXCP_PROGRAM:   /* Program exception                        */
>          switch (env->error_code & ~0xF) {

Re: [PATCH 3/6] target/ppc: Fix instruction loading endianness in alignment interrupt

[Nicholas Piggin]

On Fri Mar 24, 2023 at 11:30 PM AEST, Fabiano Rosas wrote:
> Hi Nick,
>
> > powerpc ifetch endianness depends on MSR[LE] so it has to byteswap
> > after cpu_ldl_code(). This corrects DSISR bits in alignment
> > interrupts when running in little endian mode.
> >
>
> Just a thought, we have these tests that perhaps could have caught
> this:  https://github.com/legoater/pnv-test
>
> Despite the name they do have (some) support to pseries as well. Not
> sure how the P8 support is these days though.

Hey Fabiano,

Thanks for the review. Good thinking, and actually I did catch some
of these (the SPR size one) when running kvm-unit-tests with TCG. I
ported it to powernv too. I wonder if we should merge pnv-test into
kvm-unit-tests.

> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  target/ppc/excp_helper.c | 27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 287659c74d..5f0e363363 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -133,6 +133,31 @@ static void dump_hcall(CPUPPCState *env)
> >                    env->nip);
> >  }
> >  
> > +/* Return true iff byteswap is needed in a scalar memop */
> > +static inline bool need_byteswap(CPUArchState *env)
> > +{
> > +#if TARGET_BIG_ENDIAN
>
> TARGET_BIG_ENDIAN is always set for softmmu mode. See
> configs/targets/ppc64-softmmu.mak

I see, I just took it from translate.c and actually stupidly forgot
to actually call it here :) I'll fix it up.

Thanks,
Nick

Re: [PATCH 4/6] target/ppc: Alignment faults do not set DSISR in ISA v3.0 onward

[Nicholas Piggin]

On Fri Mar 24, 2023 at 11:39 PM AEST, Fabiano Rosas wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
> > This optional behavior was removed from the ISA in v3.0, see
> > Summary of Changes preface:
> >
> >   Data Storage Interrupt Status Register for Alignment Interrupt:
> >   Simplifies the Alignment interrupt by remov- ing the Data Storage
> >   Interrupt Status Register (DSISR) from the set of registers modified
> >   by the Alignment interrupt.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  target/ppc/excp_helper.c | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 5f0e363363..c8b8eca3b1 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -1456,13 +1456,22 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
> >          break;
> >      }
> >      case POWERPC_EXCP_ALIGN:     /* Alignment exception                      */
> > -        /* Get rS/rD and rA from faulting opcode */
> > -        /*
> > -         * Note: the opcode fields will not be set properly for a
> > -         * direct store load/store, but nobody cares as nobody
> > -         * actually uses direct store segments.
> > -         */
> > -        env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16;
> > +        switch (env->excp_model) {
>
> Slightly better would be to check on (env->insn_flags2 & PPC2_ISA300).
> We were trying to phase out the usage of "exception models" wherever
> possible in favor of specific feature/isa level flags.

Oh good point, thanks for catching that. Will fix and resend the series
(I've done the same thing in a few other places too).

Thanks,
Nick

Re: [PATCH 6/6] target/ppc: Implement HEIR SPR

[Harsh Prateek Bora]

On Thu, Mar 23, 2023 at 12:22:37PM +1000, Nicholas Piggin wrote:
> The hypervisor emulation assistance interrupt modifies HEIR to
> contain the value of the instruction which caused the exception.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  target/ppc/cpu.h         |  1 +
>  target/ppc/cpu_init.c    | 23 +++++++++++++++++++++++
>  target/ppc/excp_helper.c | 12 +++++++++++-
>  3 files changed, 35 insertions(+), 1 deletion(-)
> 

<snip>

> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 2e0321ab69..d206903562 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1614,13 +1614,23 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>      case POWERPC_EXCP_HDECR:     /* Hypervisor decrementer exception         */
>      case POWERPC_EXCP_HDSI:      /* Hypervisor data storage exception        */
>      case POWERPC_EXCP_SDOOR_HV:  /* Hypervisor Doorbell interrupt            */
> -    case POWERPC_EXCP_HV_EMU:
>      case POWERPC_EXCP_HVIRT:     /* Hypervisor virtualization                */
>          srr0 = SPR_HSRR0;
>          srr1 = SPR_HSRR1;
>          new_msr |= (target_ulong)MSR_HVB;
>          new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>          break;
> +    case POWERPC_EXCP_HV_EMU:
> +        env->spr[SPR_HEIR] = insn;
> +        if (is_prefix_excp(env, insn)) {
> +            uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
> +            env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32;
> +        }
> +        srr0 = SPR_HSRR0;
> +        srr1 = SPR_HSRR1;
> +        new_msr |= (target_ulong)MSR_HVB;
> +        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> +        break;

Since there is a common code, this could be better written like:
    case POWERPC_EXCP_HV_EMU:
        env->spr[SPR_HEIR] = insn;
        if (is_prefix_excp(env, insn)) {
            uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
            env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32;
        }
	/* fall through below common code for EXCP_HVIRT */
    case POWERPC_EXCP_HVIRT:     /* Hypervisor virtualization                */
        srr0 = SPR_HSRR0;
        srr1 = SPR_HSRR1;
        new_msr |= (target_ulong)MSR_HVB;
        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
        break;

regards,
Harsh
>      case POWERPC_EXCP_VPU:       /* Vector unavailable exception             */
>      case POWERPC_EXCP_VSXU:       /* VSX unavailable exception               */
>      case POWERPC_EXCP_FU:         /* Facility unavailable exception          */
> -- 
> 2.37.2
> 
> 

Re: [PATCH 6/6] target/ppc: Implement HEIR SPR

[Nicholas Piggin]

On Tue May 9, 2023 at 7:51 PM AEST, Harsh Prateek Bora wrote:
> On Thu, Mar 23, 2023 at 12:22:37PM +1000, Nicholas Piggin wrote:
> > The hypervisor emulation assistance interrupt modifies HEIR to
> > contain the value of the instruction which caused the exception.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  target/ppc/cpu.h         |  1 +
> >  target/ppc/cpu_init.c    | 23 +++++++++++++++++++++++
> >  target/ppc/excp_helper.c | 12 +++++++++++-
> >  3 files changed, 35 insertions(+), 1 deletion(-)
> > 
>
> <snip>
>
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 2e0321ab69..d206903562 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -1614,13 +1614,23 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
> >      case POWERPC_EXCP_HDECR:     /* Hypervisor decrementer exception         */
> >      case POWERPC_EXCP_HDSI:      /* Hypervisor data storage exception        */
> >      case POWERPC_EXCP_SDOOR_HV:  /* Hypervisor Doorbell interrupt            */
> > -    case POWERPC_EXCP_HV_EMU:
> >      case POWERPC_EXCP_HVIRT:     /* Hypervisor virtualization                */
> >          srr0 = SPR_HSRR0;
> >          srr1 = SPR_HSRR1;
> >          new_msr |= (target_ulong)MSR_HVB;
> >          new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> >          break;
> > +    case POWERPC_EXCP_HV_EMU:
> > +        env->spr[SPR_HEIR] = insn;
> > +        if (is_prefix_excp(env, insn)) {
> > +            uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
> > +            env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32;
> > +        }
> > +        srr0 = SPR_HSRR0;
> > +        srr1 = SPR_HSRR1;
> > +        new_msr |= (target_ulong)MSR_HVB;
> > +        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> > +        break;
>
> Since there is a common code, this could be better written like:
>     case POWERPC_EXCP_HV_EMU:
>         env->spr[SPR_HEIR] = insn;
>         if (is_prefix_excp(env, insn)) {
>             uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
>             env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32;
>         }
> 	/* fall through below common code for EXCP_HVIRT */
>     case POWERPC_EXCP_HVIRT:     /* Hypervisor virtualization                */
>         srr0 = SPR_HSRR0;
>         srr1 = SPR_HSRR1;
>         new_msr |= (target_ulong)MSR_HVB;
>         new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>         break;

That would be wrong for the other HSRR fallthroughs above it.

Thanks,
Nick

Re: [PATCH 6/6] target/ppc: Implement HEIR SPR

[Harsh Prateek Bora]

On 5/15/23 13:56, Nicholas Piggin wrote:
> On Tue May 9, 2023 at 7:51 PM AEST, Harsh Prateek Bora wrote:
>> On Thu, Mar 23, 2023 at 12:22:37PM +1000, Nicholas Piggin wrote:
>>> The hypervisor emulation assistance interrupt modifies HEIR to
>>> contain the value of the instruction which caused the exception.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>   target/ppc/cpu.h         |  1 +
>>>   target/ppc/cpu_init.c    | 23 +++++++++++++++++++++++
>>>   target/ppc/excp_helper.c | 12 +++++++++++-
>>>   3 files changed, 35 insertions(+), 1 deletion(-)
>>>
>>
>> <snip>
>>
>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>>> index 2e0321ab69..d206903562 100644
>>> --- a/target/ppc/excp_helper.c
>>> +++ b/target/ppc/excp_helper.c
>>> @@ -1614,13 +1614,23 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>>>       case POWERPC_EXCP_HDECR:     /* Hypervisor decrementer exception         */
>>>       case POWERPC_EXCP_HDSI:      /* Hypervisor data storage exception        */
>>>       case POWERPC_EXCP_SDOOR_HV:  /* Hypervisor Doorbell interrupt            */
>>> -    case POWERPC_EXCP_HV_EMU:
>>>       case POWERPC_EXCP_HVIRT:     /* Hypervisor virtualization                */
>>>           srr0 = SPR_HSRR0;
>>>           srr1 = SPR_HSRR1;
>>>           new_msr |= (target_ulong)MSR_HVB;
>>>           new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>>>           break;
>>> +    case POWERPC_EXCP_HV_EMU:
>>> +        env->spr[SPR_HEIR] = insn;
>>> +        if (is_prefix_excp(env, insn)) {
>>> +            uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
>>> +            env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32;
>>> +        }
>>> +        srr0 = SPR_HSRR0;
>>> +        srr1 = SPR_HSRR1;
>>> +        new_msr |= (target_ulong)MSR_HVB;
>>> +        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>>> +        break;
>>
>> Since there is a common code, this could be better written like:
>>      case POWERPC_EXCP_HV_EMU:
>>          env->spr[SPR_HEIR] = insn;
>>          if (is_prefix_excp(env, insn)) {
>>              uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
>>              env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32;
>>          }
>> 	/* fall through below common code for EXCP_HVIRT */
>>      case POWERPC_EXCP_HVIRT:     /* Hypervisor virtualization                */
>>          srr0 = SPR_HSRR0;
>>          srr1 = SPR_HSRR1;
>>          new_msr |= (target_ulong)MSR_HVB;
>>          new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>>          break;
> 
> That would be wrong for the other HSRR fallthroughs above it.
> 
Oh yeh, in that case, may be move it to top of the EXCP_HISI, it would 
need duplicating one line of assignment though (relatively better?).

regards,
Harsh

> Thanks,
> Nick
> 

Re: [PATCH 6/6] target/ppc: Implement HEIR SPR

[Harsh Prateek Bora]

On 5/15/23 14:02, Harsh Prateek Bora wrote:
>>
>> That would be wrong for the other HSRR fallthroughs above it.
>>
> Oh yeh, in that case, may be move it to top of the EXCP_HISI, it would 
> need duplicating one line of assignment though (relatively better?).

correcting myself, no duplication needed if keeping above EXCP_HISI.

> 
> regards,
> Harsh

Re: [PATCH 6/6] target/ppc: Implement HEIR SPR

[Nicholas Piggin]

On Mon May 15, 2023 at 7:32 PM AEST, Harsh Prateek Bora wrote:
>
>
> On 5/15/23 14:02, Harsh Prateek Bora wrote:
> >>
> >> That would be wrong for the other HSRR fallthroughs above it.
> >>
> > Oh yeh, in that case, may be move it to top of the EXCP_HISI, it would 
> > need duplicating one line of assignment though (relatively better?).
>
> correcting myself, no duplication needed if keeping above EXCP_HISI.

No, because HV_EMU interrupts get an error code that can not be put
into HSRR1.

Thanks,
Nick

Re: [PATCH 6/6] target/ppc: Implement HEIR SPR

[Harsh Prateek Bora]

On 5/15/23 16:15, Nicholas Piggin wrote:
> On Mon May 15, 2023 at 7:32 PM AEST, Harsh Prateek Bora wrote:
>>
>>
>> On 5/15/23 14:02, Harsh Prateek Bora wrote:
>>>>
>>>> That would be wrong for the other HSRR fallthroughs above it.
>>>>
>>> Oh yeh, in that case, may be move it to top of the EXCP_HISI, it would
>>> need duplicating one line of assignment though (relatively better?).
>>
>> correcting myself, no duplication needed if keeping above EXCP_HISI.
> 
> No, because HV_EMU interrupts get an error code that can not be put
> into HSRR1.
> 
Oh ok, thanks for clarifying.

regards,
Harsh

> Thanks,
> Nick
>