target/ppc: Fixes for instruction-related

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

Posted by Nicholas Piggin 2 years, 7 months ago

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

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

Posted by BALATON Zoltan 2 years, 7 months ago

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

Posted by Nicholas Piggin 2 years, 7 months ago

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

Posted by Nicholas Piggin 2 years, 7 months ago

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)