[v3] Unaligned access for user-only

[PATCH v3 09/66] target/ppc: Move SPR_DSISR setting to powerpc_excp

Posted by Richard Henderson 2 years, 10 months ago

By doing this while sending the exception, we will have already
done the unwinding, which makes the ppc_cpu_do_unaligned_access
code a bit cleaner.

Update the comment about the expected instruction format.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/excp_helper.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index a79a0ed465..0df358f93f 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -478,13 +478,15 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, 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.
+         * Get rS/rD and rA from faulting opcode.
+         * Note: We will only invoke ALIGN for atomic operations,
+         * so all instructions are X-form.
          */
-        env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16;
+        {
+            uint32_t insn = cpu_ldl_code(env, env->nip);
+            env->spr[SPR_DSISR] |= (insn & 0x03FF0000) >> 16;
+        }
         break;
     case POWERPC_EXCP_PROGRAM:   /* Program exception                        */
         switch (env->error_code & ~0xF) {
@@ -1501,14 +1503,9 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
                                  int mmu_idx, uintptr_t retaddr)
 {
     CPUPPCState *env = cs->env_ptr;
-    uint32_t insn;
-
-    /* Restore state and reload the insn we executed, for filling in DSISR.  */
-    cpu_restore_state(cs, retaddr, true);
-    insn = cpu_ldl_code(env, env->nip);
 
     cs->exception_index = POWERPC_EXCP_ALIGN;
-    env->error_code = insn & 0x03FF0000;
-    cpu_loop_exit(cs);
+    env->error_code = 0;
+    cpu_loop_exit_restore(cs, retaddr);
 }
 #endif
-- 
2.25.1

Re: [PATCH v3 09/66] target/ppc: Move SPR_DSISR setting to powerpc_excp

Posted by Peter Maydell 2 years, 10 months ago

On Wed, 18 Aug 2021 at 20:24, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> By doing this while sending the exception, we will have already
> done the unwinding, which makes the ppc_cpu_do_unaligned_access
> code a bit cleaner.
>
> Update the comment about the expected instruction format.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/ppc/excp_helper.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index a79a0ed465..0df358f93f 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -478,13 +478,15 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, 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.
> +         * Get rS/rD and rA from faulting opcode.
> +         * Note: We will only invoke ALIGN for atomic operations,
> +         * so all instructions are X-form.
>           */
> -        env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16;
> +        {
> +            uint32_t insn = cpu_ldl_code(env, env->nip);
> +            env->spr[SPR_DSISR] |= (insn & 0x03FF0000) >> 16;
> +        }
>          break;
>      case POWERPC_EXCP_PROGRAM:   /* Program exception                        */
>          switch (env->error_code & ~0xF) {
> @@ -1501,14 +1503,9 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>                                   int mmu_idx, uintptr_t retaddr)
>  {
>      CPUPPCState *env = cs->env_ptr;
> -    uint32_t insn;
> -
> -    /* Restore state and reload the insn we executed, for filling in DSISR.  */
> -    cpu_restore_state(cs, retaddr, true);
> -    insn = cpu_ldl_code(env, env->nip);
>
>      cs->exception_index = POWERPC_EXCP_ALIGN;
> -    env->error_code = insn & 0x03FF0000;
> -    cpu_loop_exit(cs);
> +    env->error_code = 0;
> +    cpu_loop_exit_restore(cs, retaddr);

cpu_ldl_code() in the unaligned-access handler is strictly speaking
bogus, because the page might have been unmapped after translation
but before we got round to actually running it. Better would be to
stash the relevant bits of info from the insn in the insn_start param,
the way Arm does with the syndrome info.

But it's the way we currently implement this, so
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 09/66] target/ppc: Move SPR_DSISR setting to powerpc_excp

Posted by Richard Henderson 2 years, 10 months ago

On 8/19/21 5:39 AM, Peter Maydell wrote:
> cpu_ldl_code() in the unaligned-access handler is strictly speaking
> bogus, because the page might have been unmapped after translation
> but before we got round to actually running it. Better would be to
> stash the relevant bits of info from the insn in the insn_start param,
> the way Arm does with the syndrome info.

Yep.  That was more than I was prepared to do here.


r~