Re: [PATCH v3 09/14] target/ppc: Move patching nip from exception handler to helper_scv

[Nicholas Piggin]

On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote:
> From: Nicholas Piggin <npiggin@gmail.com>
>
> Unlike sc, for scv a facility unavailable interrupt must be generated
> if FSCR[SCV]=0 so we can't raise the exception with nip set to next
> instruction but we can move advancing nip if the FSCR check passes to
> helper_scv so the exception handler does not need to change it.
>
> [balaton: added commit message]
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Ah you sent it, fine, thank you. But actually now I look again,
now we're off by one in the other direction for the dumps.

So... probably your way is still better because it matches the
interrupt semantics of the ISA when executing the instruction,
but it needs this patch:

For my patch you can add

Signed-off-by: Nicholas Piggin <npiggin@gmail.com

Thanks,
Nick


diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 0e21cb4451..d7f42639c8 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -117,7 +117,7 @@ static void dump_syscall(CPUPPCState *env)
                   ppc_dump_gpr(env, 0), ppc_dump_gpr(env, 3),
                   ppc_dump_gpr(env, 4), ppc_dump_gpr(env, 5),
                   ppc_dump_gpr(env, 6), ppc_dump_gpr(env, 7),
-                  ppc_dump_gpr(env, 8), env->nip);
+                  ppc_dump_gpr(env, 8), env->nip - 4);
 }

 static void dump_hcall(CPUPPCState *env)
@@ -132,7 +132,7 @@ static void dump_hcall(CPUPPCState *env)
                   ppc_dump_gpr(env, 7), ppc_dump_gpr(env, 8),
                   ppc_dump_gpr(env, 9), ppc_dump_gpr(env, 10),
                   ppc_dump_gpr(env, 11), ppc_dump_gpr(env, 12),
-                  env->nip);
+                  env->nip - 4);
 }

 #ifdef CONFIG_TCG



> ---
> This needs SoB from Nick
>
>  target/ppc/excp_helper.c | 2 +-
>  target/ppc/translate.c   | 6 +++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 903216c2a6..ef363b0285 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1304,7 +1304,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int 
> excp)
>      case POWERPC_EXCP_SYSCALL_VECTORED: /* scv exception                     
> */
>          lev = env->error_code;
>          dump_syscall(env);
> -        env->nip += 4;
>          new_msr |= env->msr & ((target_ulong)1 << MSR_EE);
>          new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>  
> @@ -2410,6 +2409,7 @@ void helper_ppc_maybe_interrupt(CPUPPCState *env)
>  void helper_scv(CPUPPCState *env, uint32_t lev)
>  {
>      if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
> +        env->nip += 4;
>          raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
>      } else {
>          raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 4260d3d66f..0360a17fb3 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4433,7 +4433,11 @@ static void gen_scv(DisasContext *ctx)
>  {
>      uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>  
> -    /* Set the PC back to the faulting instruction. */
> +    /*
> +     * Set the PC back to the scv instruction (unlike sc), because a facility
> +     * unavailable interrupt must be generated if FSCR[SCV]=0. The helper
> +     * advances nip if the FSCR check passes.
> +     */
>      gen_update_nip(ctx, ctx->cia);
>      gen_helper_scv(cpu_env, tcg_constant_i32(lev));
>  
> -- 
> 2.30.9