On Thu Jun 15, 2023 at 7:27 AM AEST, BALATON Zoltan wrote:
On Wed, 14 Jun 2023, Nicholas Piggin wrote:
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.
I've tried but scv seems to be a bit different as it can sometimes raise
POWERPC_EXCP_FU instead of POWERPC_EXCP_SYSCALL_VECTORED according to
excp_helper.c::helper_scv() which may need the nip to be different so I'd
rather leave this to somebody who knows what they are doing.
Yes and the fscr check requires a helper so no point doing that
twice, but I think it would still better match the new sc code to
have something like this (patch is tested, you can squash it in
yours if you like):
Thanks,
Nick
---
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 12d8a7257b..55c6edfacb 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1507,7 +1507,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);
@@ -2623,6 +2622,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 b591f2e496..90d07a3e56 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4432,7 +4432,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));