[PATCH 0/4] target/ppc: Fixes for instruction-related

[PATCH 0/4] target/ppc: Fixes for instruction-related

[Nicholas Piggin]

Because they got more complexities than I first thought, these patches
are broken out from the bigger series here:

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

Since then I fixed the --disable-tcg compile bug reported by Anushree
hopefully. Also added a workaround for KVM so injected interrupts
wouldn't attempt to find the prefix bit setting. I don't know how much
that is really needed, but injection callers would have to set it one
way or anohter if we need to add it.

Thanks,
Nick

Nicholas Piggin (4):
  target/ppc: Fix instruction loading endianness in alignment interrupt
  target/ppc: Change partition-scope translate interface
  target/ppc: Add SRR1 prefix indication to interrupt handlers
  target/ppc: Implement HEIR SPR

 target/ppc/cpu.h         |   1 +
 target/ppc/cpu_init.c    |  23 ++++++++
 target/ppc/excp_helper.c | 110 ++++++++++++++++++++++++++++++++++++++-
 target/ppc/mmu-radix64.c |  38 ++++++++++----
 4 files changed, 159 insertions(+), 13 deletions(-)

-- 
2.40.1

[PATCH 1/4] 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.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/excp_helper.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 12d8a7257b..a2801f6e6b 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -133,6 +133,26 @@ static void dump_hcall(CPUPPCState *env)
                   env->nip);
 }
 
+#ifdef CONFIG_TCG
+/* Return true iff byteswap is needed to load instruction */
+static inline bool insn_need_byteswap(CPUArchState *env)
+{
+    /* SYSTEM builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */
+    return !!(env->msr & ((target_ulong)1 << MSR_LE));
+}
+
+static uint32_t ppc_ldl_code(CPUArchState *env, hwaddr addr)
+{
+    uint32_t insn = cpu_ldl_code(env, addr);
+
+    if (insn_need_byteswap(env)) {
+        insn = bswap32(insn);
+    }
+
+    return insn;
+}
+#endif
+
 static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
 {
     const char *es;
@@ -3104,7 +3124,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.40.1

[PATCH 2/4] target/ppc: Change partition-scope translate interface

[Nicholas Piggin]

Rather than always performing partition scope page table translation
with access type of 0 (MMU_DATA_LOAD), pass through the processor
access type which first initiated the translation sequence. Process-
scoped page table loads are then set to MMU_DATA_LOAD access type in
the xlate function.

This will allow more information to be passed to the exception
handler in the next patch.

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

diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 031efda0df..1fc1ba3ecf 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -380,6 +380,14 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu,
     hwaddr pte_addr;
     uint64_t pte;
 
+    if (pde_addr) {
+        /*
+         * Translation of process-scoped tables/directories is performed as
+         * a read-access.
+         */
+        access_type = MMU_DATA_LOAD;
+    }
+
     qemu_log_mask(CPU_LOG_MMU, "%s for %s @0x%"VADDR_PRIx
                   " mmu_idx %u 0x%"HWADDR_PRIx"\n",
                   __func__, access_str(access_type),
@@ -477,10 +485,10 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
          * is only used to translate the effective addresses of the
          * process table entries.
          */
-        ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr,
-                                                 pate, &h_raddr, &h_prot,
-                                                 &h_page_size, true,
-            /* mmu_idx is 5 because we're translating from hypervisor scope */
+        /* mmu_idx is 5 because we're translating from hypervisor scope */
+        ret = ppc_radix64_partition_scoped_xlate(cpu, access_type, eaddr,
+                                                 prtbe_addr, pate, &h_raddr,
+                                                 &h_prot, &h_page_size, true,
                                                  5, guest_visible);
         if (ret) {
             return ret;
@@ -519,11 +527,11 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
          * translation
          */
         do {
-            ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, pte_addr,
-                                                     pate, &h_raddr, &h_prot,
-                                                     &h_page_size, true,
             /* mmu_idx is 5 because we're translating from hypervisor scope */
-                                                     5, guest_visible);
+            ret = ppc_radix64_partition_scoped_xlate(cpu, access_type, eaddr,
+                                                     pte_addr, pate, &h_raddr,
+                                                     &h_prot, &h_page_size,
+                                                     true, 5, guest_visible);
             if (ret) {
                 return ret;
             }
-- 
2.40.1

[PATCH 3/4] 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.

The case of instruction fetch that causes an HDSI due to access of a
process-scoped table faulting on the partition scoped translation is the
tricky one. As with ISIs and HISIs, this does not try to set the prefix
bit because there is no instruction image to be loaded. The HDSI needs
the originating access type to be passed through to the handler to
distinguish this from HDSIs that fault translating process scoped tables
originating from a load or store instruction (in that case the prefix
bit should be provided).

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/excp_helper.c | 73 +++++++++++++++++++++++++++++++++++++++-
 target/ppc/mmu-radix64.c | 14 ++++++--
 2 files changed, 83 insertions(+), 4 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index a2801f6e6b..1de6ea3f03 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -28,6 +28,7 @@
 #include "trace.h"
 
 #ifdef CONFIG_TCG
+#include "sysemu/tcg.h"
 #include "exec/helper-proto.h"
 #include "exec/cpu_ldst.h"
 #endif
@@ -141,7 +142,7 @@ static inline bool insn_need_byteswap(CPUArchState *env)
     return !!(env->msr & ((target_ulong)1 << MSR_LE));
 }
 
-static uint32_t ppc_ldl_code(CPUArchState *env, hwaddr addr)
+static uint32_t ppc_ldl_code(CPUArchState *env, abi_ptr addr)
 {
     uint32_t insn = cpu_ldl_code(env, addr);
 
@@ -1348,6 +1349,72 @@ static bool books_vhyp_handles_hv_excp(PowerPCCPU *cpu)
     return false;
 }
 
+#ifdef CONFIG_TCG
+static bool is_prefix_insn(CPUPPCState *env, uint32_t insn)
+{
+    if (!(env->insns_flags2 & PPC2_ISA310)) {
+        return false;
+    }
+    return ((insn & 0xfc000000) == 0x04000000);
+}
+
+static bool is_prefix_insn_excp(PowerPCCPU *cpu, int excp)
+{
+    CPUPPCState *env = &cpu->env;
+
+    if (!tcg_enabled()) {
+        /*
+	 * This does not load instructions and set the prefix bit correctly
+	 * for injected interrupts with KVM. That may have to be discovered
+	 * and set by the KVM layer before injecting.
+         */
+        return false;
+    }
+
+    switch (excp) {
+    case POWERPC_EXCP_HDSI:
+        /* HDSI PRTABLE_FAULT has the originating access type in error_code */
+        if ((env->spr[SPR_HDSISR] & DSISR_PRTABLE_FAULT) &&
+            (env->error_code == MMU_INST_FETCH)) {
+            /*
+             * Fetch failed due to partition scope translation, so prefix
+             * indication is not relevant (and attempting to load the
+             * instruction at NIP would cause recursive faults with the same
+             * translation).
+             */
+            break;
+        }
+        /* fall through */
+    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_HV_EMU:
+    case POWERPC_EXCP_VPU:
+    case POWERPC_EXCP_VSXU:
+    case POWERPC_EXCP_FU:
+    case POWERPC_EXCP_HV_FU: {
+        uint32_t insn = ppc_ldl_code(env, env->nip);
+        if (is_prefix_insn(env, insn)) {
+            return true;
+        }
+        break;
+    }
+    default:
+        break;
+    }
+    return false;
+}
+#else
+static bool is_prefix_insn_excp(PowerPCCPU *cpu, int excp)
+{
+    return false;
+}
+#endif
+
 static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
 {
     CPUState *cs = CPU(cpu);
@@ -1395,6 +1462,10 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
 
     vector |= env->excp_prefix;
 
+    if (is_prefix_insn_excp(cpu, excp)) {
+        msr |= PPC_BIT(34);
+    }
+
     switch (excp) {
     case POWERPC_EXCP_MCHECK:    /* Machine check exception                  */
         if (!FIELD_EX64(env->msr, MSR, ME)) {
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 1fc1ba3ecf..920084bd8f 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -145,6 +145,13 @@ static void ppc_radix64_raise_hsi(PowerPCCPU *cpu, MMUAccessType access_type,
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
 
+    env->error_code = 0;
+    if (cause & DSISR_PRTABLE_FAULT) {
+        /* HDSI PRTABLE_FAULT gets the originating access type in error_code */
+        env->error_code = access_type;
+        access_type = MMU_DATA_LOAD;
+    }
+
     qemu_log_mask(CPU_LOG_MMU, "%s for %s @0x%"VADDR_PRIx" 0x%"
                   HWADDR_PRIx" cause %08x\n",
                   __func__, access_str(access_type),
@@ -166,7 +173,6 @@ static void ppc_radix64_raise_hsi(PowerPCCPU *cpu, MMUAccessType access_type,
         env->spr[SPR_HDSISR] = cause;
         env->spr[SPR_HDAR] = eaddr;
         env->spr[SPR_ASDR] = g_raddr;
-        env->error_code = 0;
         break;
     default:
         g_assert_not_reached();
@@ -369,13 +375,14 @@ static bool validate_pate(PowerPCCPU *cpu, uint64_t lpid, ppc_v3_pate_t *pate)
 }
 
 static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu,
-                                              MMUAccessType access_type,
+                                              MMUAccessType orig_access_type,
                                               vaddr eaddr, hwaddr g_raddr,
                                               ppc_v3_pate_t pate,
                                               hwaddr *h_raddr, int *h_prot,
                                               int *h_page_size, bool pde_addr,
                                               int mmu_idx, bool guest_visible)
 {
+    MMUAccessType access_type = orig_access_type;
     int fault_cause = 0;
     hwaddr pte_addr;
     uint64_t pte;
@@ -404,7 +411,8 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu,
             fault_cause |= DSISR_PRTABLE_FAULT;
         }
         if (guest_visible) {
-            ppc_radix64_raise_hsi(cpu, access_type, eaddr, g_raddr, fault_cause);
+            ppc_radix64_raise_hsi(cpu, orig_access_type,
+                                  eaddr, g_raddr, fault_cause);
         }
         return 1;
     }
-- 
2.40.1

[PATCH 4/4] 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.

Only TCG raises HEAI interrupts so this can be made TCG-only.

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

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 0ee2adc105..054edf3c80 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1647,6 +1647,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 d3d6902e6e..d4a7bf01cc 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -1630,6 +1630,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)
@@ -5523,6 +5524,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",
@@ -5951,6 +5970,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);
 
@@ -6073,6 +6093,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);
@@ -6235,6 +6256,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);
@@ -6427,6 +6449,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 1de6ea3f03..77bfc18734 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1642,13 +1642,28 @@ 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;
+#ifdef CONFIG_TCG
+    case POWERPC_EXCP_HV_EMU: {
+        uint32_t insn = ppc_ldl_code(env, env->nip);
+        env->spr[SPR_HEIR] = insn;
+        if (is_prefix_insn(env, insn)) {
+            uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
+            env->spr[SPR_HEIR] <<= 32;
+            env->spr[SPR_HEIR] |= insn2;
+        }
+        srr0 = SPR_HSRR0;
+        srr1 = SPR_HSRR1;
+        new_msr |= (target_ulong)MSR_HVB;
+        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
+        break;
+    }
+#endif
     case POWERPC_EXCP_VPU:       /* Vector unavailable exception             */
     case POWERPC_EXCP_VSXU:       /* VSX unavailable exception               */
     case POWERPC_EXCP_FU:         /* Facility unavailable exception          */
-- 
2.40.1

Re: [PATCH 1/4] target/ppc: Fix instruction loading endianness in alignment interrupt

[BALATON Zoltan]

On Tue, 20 Jun 2023, Nicholas Piggin wrote:
> 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.
>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> target/ppc/excp_helper.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 12d8a7257b..a2801f6e6b 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -133,6 +133,26 @@ static void dump_hcall(CPUPPCState *env)
>                   env->nip);
> }
>
> +#ifdef CONFIG_TCG
> +/* Return true iff byteswap is needed to load instruction */
> +static inline bool insn_need_byteswap(CPUArchState *env)
> +{
> +    /* SYSTEM builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */
> +    return !!(env->msr & ((target_ulong)1 << MSR_LE));
> +}

Don't other places typically use FIELD_EX64 to test for msr bits now? If 
this really only tests for the LE bit and used only once do we need a new 
function for that? I don't quite like trivial one line functions unless it 
does something more complex Because if just makes code harder to read as I 
have to look up what these do when I could just see it right away where it 
used without these functions.

> +
> +static uint32_t ppc_ldl_code(CPUArchState *env, hwaddr addr)
> +{
> +    uint32_t insn = cpu_ldl_code(env, addr);
> +
> +    if (insn_need_byteswap(env)) {
> +        insn = bswap32(insn);
> +    }
> +
> +    return insn;
> +}
> +#endif

Along the same lines I'm not sure this wrapper is needed unless this is a 
recurring operation. Otherwise you could just add the if and the comment 
below at the single place where this is needed. If this will be needed at 
more places later then adding a function may make sense but otherwise I'd 
avoid making code tangled with single line functions defined away from 
where they are used as it's simpler to just have the if and swap at the 
single place where it's needed than adding two new functions that I'd had 
to look up and comprehend first to see what's happening. (It also would be 
just 3 lines instead of 20 that way.)

Regards,
BALATON Zoltan

> +
> static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
> {
>     const char *es;
> @@ -3104,7 +3124,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 1/4] target/ppc: Fix instruction loading endianness in alignment interrupt

[Nicholas Piggin]

On Wed Jun 21, 2023 at 12:26 AM AEST, BALATON Zoltan wrote:
> On Tue, 20 Jun 2023, Nicholas Piggin wrote:
> > 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.
> >
> > Reviewed-by: Fabiano Rosas <farosas@suse.de>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > target/ppc/excp_helper.c | 22 +++++++++++++++++++++-
> > 1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 12d8a7257b..a2801f6e6b 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -133,6 +133,26 @@ static void dump_hcall(CPUPPCState *env)
> >                   env->nip);
> > }
> >
> > +#ifdef CONFIG_TCG
> > +/* Return true iff byteswap is needed to load instruction */
> > +static inline bool insn_need_byteswap(CPUArchState *env)
> > +{
> > +    /* SYSTEM builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */
> > +    return !!(env->msr & ((target_ulong)1 << MSR_LE));
> > +}
>
> Don't other places typically use FIELD_EX64 to test for msr bits now? If 

Yeah I should use that, good point. There's at least another case in
that file that doesn't use it but I probably added that too :/

> this really only tests for the LE bit and used only once do we need a new 
> function for that? I don't quite like trivial one line functions unless it 
> does something more complex Because if just makes code harder to read as I 
> have to look up what these do when I could just see it right away where it 
> used without these functions.

It's based on mem_helper.c, which is familiar pattern/name so I 
might keep it. Maybe not, I'll check. I'm on the fence.

> > +
> > +static uint32_t ppc_ldl_code(CPUArchState *env, hwaddr addr)
> > +{
> > +    uint32_t insn = cpu_ldl_code(env, addr);
> > +
> > +    if (insn_need_byteswap(env)) {
> > +        insn = bswap32(insn);
> > +    }
> > +
> > +    return insn;
> > +}
> > +#endif
>
> Along the same lines I'm not sure this wrapper is needed unless this is a 
> recurring operation. Otherwise you could just add the if and the comment 
> below at the single place where this is needed. If this will be needed at 
> more places later then adding a function may make sense but otherwise I'd 
> avoid making code tangled with single line functions defined away from 
> where they are used as it's simpler to just have the if and swap at the 
> single place where it's needed than adding two new functions that I'd had 
> to look up and comprehend first to see what's happening. (It also would be 
> just 3 lines instead of 20 that way.)

It does get used in a couple more places later. Few-line
"abstraction" used once isn't necessarily wrong though.

Thanks,
Nick

Re: [PATCH 1/4] target/ppc: Fix instruction loading endianness in alignment interrupt

[Nicholas Piggin]

On Wed Jun 21, 2023 at 2:54 AM AEST, Nicholas Piggin wrote:
> On Wed Jun 21, 2023 at 12:26 AM AEST, BALATON Zoltan wrote:
> > On Tue, 20 Jun 2023, Nicholas Piggin wrote:
> > > 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.
> > >
> > > Reviewed-by: Fabiano Rosas <farosas@suse.de>
> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > ---
> > > target/ppc/excp_helper.c | 22 +++++++++++++++++++++-
> > > 1 file changed, 21 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > > index 12d8a7257b..a2801f6e6b 100644
> > > --- a/target/ppc/excp_helper.c
> > > +++ b/target/ppc/excp_helper.c
> > > @@ -133,6 +133,26 @@ static void dump_hcall(CPUPPCState *env)
> > >                   env->nip);
> > > }
> > >
> > > +#ifdef CONFIG_TCG
> > > +/* Return true iff byteswap is needed to load instruction */
> > > +static inline bool insn_need_byteswap(CPUArchState *env)
> > > +{
> > > +    /* SYSTEM builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */
> > > +    return !!(env->msr & ((target_ulong)1 << MSR_LE));
> > > +}
> >
> > Don't other places typically use FIELD_EX64 to test for msr bits now? If 
>
> Yeah I should use that, good point. There's at least another case in
> that file that doesn't use it but I probably added that too :/

This incremental patch fixes it:

Thanks,
Nick

---
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index ff7166adf9..cfdbeb0da5 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -138,7 +138,7 @@ static void dump_hcall(CPUPPCState *env)
 static inline bool insn_need_byteswap(CPUArchState *env)
 {
     /* SYSTEM builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */
-    return !!(env->msr & ((target_ulong)1 << MSR_LE));
+    return FIELD_EX64(env->msr, MSR, LE);
 }
 
 static uint32_t ppc_ldl_code(CPUArchState *env, hwaddr addr)

Re: [PATCH 0/4] target/ppc: Fixes for instruction-related

[Cédric Le Goater]

On 6/20/23 15:10, Nicholas Piggin wrote:
> Because they got more complexities than I first thought, these patches
> are broken out from the bigger series here:
> 
> https://lists.gnu.org/archive/html/qemu-ppc/2023-05/msg00425.html
> 
> Since then I fixed the --disable-tcg compile bug reported by Anushree
> hopefully. Also added a workaround for KVM so injected interrupts
> wouldn't attempt to find the prefix bit setting. I don't know how much
> that is really needed, but injection callers would have to set it one
> way or anohter if we need to add it.
> 
> Thanks,
> Nick
> 
> Nicholas Piggin (4):
>    target/ppc: Fix instruction loading endianness in alignment interrupt
>    target/ppc: Change partition-scope translate interface
>    target/ppc: Add SRR1 prefix indication to interrupt handlers
>    target/ppc: Implement HEIR SPR
> 
>   target/ppc/cpu.h         |   1 +
>   target/ppc/cpu_init.c    |  23 ++++++++
>   target/ppc/excp_helper.c | 110 ++++++++++++++++++++++++++++++++++++++-
>   target/ppc/mmu-radix64.c |  38 ++++++++++----
>   4 files changed, 159 insertions(+), 13 deletions(-)
> 

Applied to ppc-next.

Thanks,

C.

Re: [PATCH 0/4] target/ppc: Fixes for instruction-related

[Anushree Mathur]

On 6/20/23 18:40, Nicholas Piggin wrote:
> Because they got more complexities than I first thought, these patches
> are broken out from the bigger series here:
>
> https://lists.gnu.org/archive/html/qemu-ppc/2023-05/msg00425.html
>
> Since then I fixed the --disable-tcg compile bug reported by Anushree
> hopefully. Also added a workaround for KVM so injected interrupts
> wouldn't attempt to find the prefix bit setting. I don't know how much
> that is really needed, but injection callers would have to set it one
> way or anohter if we need to add it.
>
> Thanks,
> Nick
>
> Nicholas Piggin (4):
>    target/ppc: Fix instruction loading endianness in alignment interrupt
>    target/ppc: Change partition-scope translate interface
>    target/ppc: Add SRR1 prefix indication to interrupt handlers
>    target/ppc: Implement HEIR SPR
>
>   target/ppc/cpu.h         |   1 +
>   target/ppc/cpu_init.c    |  23 ++++++++
>   target/ppc/excp_helper.c | 110 ++++++++++++++++++++++++++++++++++++++-
>   target/ppc/mmu-radix64.c |  38 ++++++++++----
>   4 files changed, 159 insertions(+), 13 deletions(-)
>
Hye Nick,

I tried this patch-set and the compilation of qemu with --disable-tcg 
parameter happened successfully!

Thanks & Regards,

Anushree-Mathur