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

[Peter Maydell]

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