Re: [PATCH 08/10] target/ppc: Fix gen_sc to use correct nip

[Nicholas Piggin]

On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
> Most exceptions are raised with nip pointing to the faulting
> instruction but the sc instruction generating a syscall exception
> leaves nip pointing to next instruction. Fix gen_sc to not use
> gen_exception_err() which sets nip back but correctly set nip to
> pc_next so we don't have to patch this in the exception handlers.
>
> This changes the nip logged in dump_syscall and dump_hcall debug
> functions but now this matches how nip would be on a real CPU.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  target/ppc/excp_helper.c | 39 ---------------------------------------
>  target/ppc/translate.c   |  8 +++++---
>  2 files changed, 5 insertions(+), 42 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 885e479301..4f6a6dfb19 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -493,12 +493,6 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>          break;
>      case POWERPC_EXCP_SYSCALL:   /* System call exception                    
> */
>          dump_syscall(env);
> -
> -        /*
> -         * We need to correct the NIP which in this case is supposed
> -         * to point to the next instruction
> -         */
> -        env->nip += 4;
>          break;
>      case POWERPC_EXCP_FIT:       /* Fixed-interval timer interrupt           
> */
>          trace_ppc_excp_print("FIT");
> @@ -609,12 +603,6 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
>          break;
>      case POWERPC_EXCP_SYSCALL:   /* System call exception                    
> */
>          dump_syscall(env);
> -
> -        /*
> -         * We need to correct the NIP which in this case is supposed
> -         * to point to the next instruction
> -         */
> -        env->nip += 4;
>          break;
>      case POWERPC_EXCP_FPU:       /* Floating-point unavailable exception     
> */
>      case POWERPC_EXCP_DECR:      /* Decrementer exception                    
> */
> @@ -757,13 +745,6 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
>          } else {
>              dump_syscall(env);
>          }
> -
> -        /*
> -         * We need to correct the NIP which in this case is supposed
> -         * to point to the next instruction
> -         */
> -        env->nip += 4;
> -
>          /*
>           * The Virtual Open Firmware (VOF) relies on the 'sc 1'
>           * instruction to communicate with QEMU. The pegasos2 machine
> @@ -908,13 +889,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>          } else {
>              dump_syscall(env);
>          }
> -
> -        /*
> -         * We need to correct the NIP which in this case is supposed
> -         * to point to the next instruction
> -         */
> -        env->nip += 4;
> -
>          /*
>           * The Virtual Open Firmware (VOF) relies on the 'sc 1'
>           * instruction to communicate with QEMU. The pegasos2 machine
> @@ -1073,12 +1047,6 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int 
> excp)
>          break;
>      case POWERPC_EXCP_SYSCALL:   /* System call exception                    
> */
>          dump_syscall(env);
> -
> -        /*
> -         * We need to correct the NIP which in this case is supposed
> -         * to point to the next instruction
> -         */
> -        env->nip += 4;
>          break;
>      case POWERPC_EXCP_FPU:       /* Floating-point unavailable exception     
> */
>      case POWERPC_EXCP_APU:       /* Auxiliary processor unavailable          
> */
> @@ -1320,13 +1288,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int 
> excp)
>          } else {
>              dump_syscall(env);
>          }
> -
> -        /*
> -         * We need to correct the NIP which in this case is supposed
> -         * to point to the next instruction
> -         */
> -        env->nip += 4;
> -
>          /* "PAPR mode" built-in hypercall emulation */
>          if (lev == 1 && books_vhyp_handles_hcall(cpu)) {
>              PPCVirtualHypervisorClass *vhc =
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index a32a9b8a5f..4260d3d66f 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4419,10 +4419,12 @@ static void gen_hrfid(DisasContext *ctx)
>  #endif
>  static void gen_sc(DisasContext *ctx)
>  {
> -    uint32_t lev;
> +    uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>  
> -    lev = (ctx->opcode >> 5) & 0x7F;
> -    gen_exception_err(ctx, POWERPC_SYSCALL, lev);
> +    gen_update_nip(ctx, ctx->base.pc_next);
> +    gen_helper_raise_exception_err(cpu_env, 
> tcg_constant_i32(POWERPC_SYSCALL),
> +                                   tcg_constant_i32(lev));
> +    ctx->base.is_jmp = DISAS_NORETURN;

Generally for blame and bisect I don't like to mix cleanup with real
change, I guess this is pretty minor though.

Great cleanup though, sc is certainly defined to set SRR0 to the
instruction past the sc unlike other interrupts so it is more natural
and less hacky feeling do it like this.

Could you do scv while you are here? It has the same semantics as
sc.

Thanks,
Nick