* [PATCH 00/10] Misc clean ups to target/ppc exception handling
@ 2023-06-11 22:42 BALATON Zoltan
2023-06-11 22:42 ` [PATCH 01/10] target/ppc: Remove some superfluous parentheses BALATON Zoltan
` (9 more replies)
0 siblings, 10 replies; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-11 22:42 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, Greg Kurz, Daniel Henrique Barboza
These are some small clean ups for target/ppc/excp_helper.c trying to
make this code a bit simpler. No functional change is intended.
Regards,
BALATON Zoltan
BALATON Zoltan (10):
target/ppc: Remove some superfluous parentheses
target/ppc: Remove unneeded parameter from powerpc_reset_wakeup()
target/ppc: Move common check in exception handlers to a function
target/ppc: Use env_cpu for cpu_abort in excp_helper
target/ppc: Change parameter of cpu_interrupt_exittb() to an env
pointer
target/ppc: Readability improvements in exception handlers
target/ppd: Remove unused define
target/ppc: Fix gen_sc to use correct nip
target/ppc: Simplify syscall exception handlers
target/ppc: Get CPUState in one step
target/ppc/cpu.h | 1 +
target/ppc/excp_helper.c | 510 +++++++++++----------------------------
target/ppc/helper_regs.c | 15 +-
target/ppc/helper_regs.h | 2 +-
target/ppc/translate.c | 9 +-
5 files changed, 156 insertions(+), 381 deletions(-)
--
2.30.9
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 01/10] target/ppc: Remove some superfluous parentheses
2023-06-11 22:42 [PATCH 00/10] Misc clean ups to target/ppc exception handling BALATON Zoltan
@ 2023-06-11 22:42 ` BALATON Zoltan
2023-06-14 3:25 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 02/10] target/ppc: Remove unneeded parameter from powerpc_reset_wakeup() BALATON Zoltan
` (8 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-11 22:42 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, Greg Kurz, Daniel Henrique Barboza
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
target/ppc/excp_helper.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 12d8a7257b..8298217e78 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1009,7 +1009,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
{
int lev = env->error_code;
- if ((lev == 1) && cpu->vhyp) {
+ if (lev == 1 && cpu->vhyp) {
dump_hcall(env);
} else {
dump_syscall(env);
@@ -1027,7 +1027,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
* uses VOF and the 74xx CPUs, so although the 74xx don't have
* HV mode, we need to keep hypercall support.
*/
- if ((lev == 1) && cpu->vhyp) {
+ if (lev == 1 && cpu->vhyp) {
PPCVirtualHypervisorClass *vhc =
PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
vhc->hypercall(cpu->vhyp, cpu);
@@ -1481,7 +1481,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
case POWERPC_EXCP_SYSCALL: /* System call exception */
lev = env->error_code;
- if ((lev == 1) && cpu->vhyp) {
+ if (lev == 1 && cpu->vhyp) {
dump_hcall(env);
} else {
dump_syscall(env);
@@ -1494,7 +1494,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
env->nip += 4;
/* "PAPR mode" built-in hypercall emulation */
- if ((lev == 1) && books_vhyp_handles_hcall(cpu)) {
+ if (lev == 1 && books_vhyp_handles_hcall(cpu)) {
PPCVirtualHypervisorClass *vhc =
PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
vhc->hypercall(cpu->vhyp, cpu);
--
2.30.9
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 02/10] target/ppc: Remove unneeded parameter from powerpc_reset_wakeup()
2023-06-11 22:42 [PATCH 00/10] Misc clean ups to target/ppc exception handling BALATON Zoltan
2023-06-11 22:42 ` [PATCH 01/10] target/ppc: Remove some superfluous parentheses BALATON Zoltan
@ 2023-06-11 22:42 ` BALATON Zoltan
2023-06-14 3:26 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 03/10] target/ppc: Move common check in exception handlers to a function BALATON Zoltan
` (7 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-11 22:42 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, Greg Kurz, Daniel Henrique Barboza
CPUState is rarely needed by this function (only for logging a fatal
error) and it's easy to get from the env parameter so passing it
separately is not necessary.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
target/ppc/excp_helper.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 8298217e78..3783315fdb 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -166,8 +166,7 @@ static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
}
#if defined(TARGET_PPC64)
-static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
- target_ulong *msr)
+static int powerpc_reset_wakeup(CPUPPCState *env, int excp, target_ulong *msr)
{
/* We no longer are in a PM state */
env->resume_as_sreset = false;
@@ -202,8 +201,8 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
*msr |= SRR1_WAKEHVI;
break;
default:
- cpu_abort(cs, "Unsupported exception %d in Power Save mode\n",
- excp);
+ cpu_abort(env_cpu(env),
+ "Unsupported exception %d in Power Save mode\n", excp);
}
return POWERPC_EXCP_RESET;
}
@@ -1353,7 +1352,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
* P7/P8/P9
*/
if (env->resume_as_sreset) {
- excp = powerpc_reset_wakeup(cs, env, excp, &msr);
+ excp = powerpc_reset_wakeup(env, excp, &msr);
}
/*
--
2.30.9
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 03/10] target/ppc: Move common check in exception handlers to a function
2023-06-11 22:42 [PATCH 00/10] Misc clean ups to target/ppc exception handling BALATON Zoltan
2023-06-11 22:42 ` [PATCH 01/10] target/ppc: Remove some superfluous parentheses BALATON Zoltan
2023-06-11 22:42 ` [PATCH 02/10] target/ppc: Remove unneeded parameter from powerpc_reset_wakeup() BALATON Zoltan
@ 2023-06-11 22:42 ` BALATON Zoltan
2023-06-12 9:28 ` Philippe Mathieu-Daudé
2023-06-14 3:35 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 04/10] target/ppc: Use env_cpu for cpu_abort in excp_helper BALATON Zoltan
` (6 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-11 22:42 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, Greg Kurz, Daniel Henrique Barboza
All powerpc exception handlers share some code when handling machine
check exceptions. Move this to a common function.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
target/ppc/excp_helper.c | 112 ++++++++-------------------------------
1 file changed, 23 insertions(+), 89 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 3783315fdb..e4532f5088 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -403,6 +403,23 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong vector,
env->reserve_addr = -1;
}
+static void powerpc_checkstop_state(CPUPPCState *env)
+{
+ if (!FIELD_EX64(env->msr, MSR, ME)) {
+ CPUState *cs = env_cpu(env);
+
+ /* Machine check exception is not enabled. Enter checkstop state. */
+ fprintf(stderr, "Machine check while not allowed. "
+ "Entering checkstop state\n");
+ if (qemu_log_separate()) {
+ qemu_log("Machine check while not allowed. "
+ "Entering checkstop state\n");
+ }
+ cs->halted = 1;
+ cpu_interrupt_exittb(cs);
+ }
+}
+
static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
{
CPUState *cs = CPU(cpu);
@@ -445,21 +462,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
srr1 = SPR_40x_SRR3;
break;
case POWERPC_EXCP_MCHECK: /* Machine check exception */
- if (!FIELD_EX64(env->msr, MSR, ME)) {
- /*
- * Machine check exception is not enabled. Enter
- * checkstop state.
- */
- fprintf(stderr, "Machine check while not allowed. "
- "Entering checkstop state\n");
- if (qemu_log_separate()) {
- qemu_log("Machine check while not allowed. "
- "Entering checkstop state\n");
- }
- cs->halted = 1;
- cpu_interrupt_exittb(cs);
- }
-
+ powerpc_checkstop_state(env);
/* machine check exceptions don't have ME set */
new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -576,21 +579,7 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
case POWERPC_EXCP_CRITICAL: /* Critical input */
break;
case POWERPC_EXCP_MCHECK: /* Machine check exception */
- if (!FIELD_EX64(env->msr, MSR, ME)) {
- /*
- * Machine check exception is not enabled. Enter
- * checkstop state.
- */
- fprintf(stderr, "Machine check while not allowed. "
- "Entering checkstop state\n");
- if (qemu_log_separate()) {
- qemu_log("Machine check while not allowed. "
- "Entering checkstop state\n");
- }
- cs->halted = 1;
- cpu_interrupt_exittb(cs);
- }
-
+ powerpc_checkstop_state(env);
/* machine check exceptions don't have ME set */
new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -749,21 +738,7 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
switch (excp) {
case POWERPC_EXCP_MCHECK: /* Machine check exception */
- if (!FIELD_EX64(env->msr, MSR, ME)) {
- /*
- * Machine check exception is not enabled. Enter
- * checkstop state.
- */
- fprintf(stderr, "Machine check while not allowed. "
- "Entering checkstop state\n");
- if (qemu_log_separate()) {
- qemu_log("Machine check while not allowed. "
- "Entering checkstop state\n");
- }
- cs->halted = 1;
- cpu_interrupt_exittb(cs);
- }
-
+ powerpc_checkstop_state(env);
/* machine check exceptions don't have ME set */
new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -934,21 +909,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
switch (excp) {
case POWERPC_EXCP_MCHECK: /* Machine check exception */
- if (!FIELD_EX64(env->msr, MSR, ME)) {
- /*
- * Machine check exception is not enabled. Enter
- * checkstop state.
- */
- fprintf(stderr, "Machine check while not allowed. "
- "Entering checkstop state\n");
- if (qemu_log_separate()) {
- qemu_log("Machine check while not allowed. "
- "Entering checkstop state\n");
- }
- cs->halted = 1;
- cpu_interrupt_exittb(cs);
- }
-
+ powerpc_checkstop_state(env);
/* machine check exceptions don't have ME set */
new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -1129,21 +1090,7 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
srr1 = SPR_BOOKE_CSRR1;
break;
case POWERPC_EXCP_MCHECK: /* Machine check exception */
- if (!FIELD_EX64(env->msr, MSR, ME)) {
- /*
- * Machine check exception is not enabled. Enter
- * checkstop state.
- */
- fprintf(stderr, "Machine check while not allowed. "
- "Entering checkstop state\n");
- if (qemu_log_separate()) {
- qemu_log("Machine check while not allowed. "
- "Entering checkstop state\n");
- }
- cs->halted = 1;
- cpu_interrupt_exittb(cs);
- }
-
+ powerpc_checkstop_state(env);
/* machine check exceptions don't have ME set */
new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -1376,20 +1323,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
switch (excp) {
case POWERPC_EXCP_MCHECK: /* Machine check exception */
- if (!FIELD_EX64(env->msr, MSR, ME)) {
- /*
- * Machine check exception is not enabled. Enter
- * checkstop state.
- */
- fprintf(stderr, "Machine check while not allowed. "
- "Entering checkstop state\n");
- if (qemu_log_separate()) {
- qemu_log("Machine check while not allowed. "
- "Entering checkstop state\n");
- }
- cs->halted = 1;
- cpu_interrupt_exittb(cs);
- }
+ powerpc_checkstop_state(env);
if (env->msr_mask & MSR_HVB) {
/*
* ISA specifies HV, but can be delivered to guest with HV
--
2.30.9
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 04/10] target/ppc: Use env_cpu for cpu_abort in excp_helper
2023-06-11 22:42 [PATCH 00/10] Misc clean ups to target/ppc exception handling BALATON Zoltan
` (2 preceding siblings ...)
2023-06-11 22:42 ` [PATCH 03/10] target/ppc: Move common check in exception handlers to a function BALATON Zoltan
@ 2023-06-11 22:42 ` BALATON Zoltan
2023-06-14 3:36 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 05/10] target/ppc: Change parameter of cpu_interrupt_exittb() to an env pointer BALATON Zoltan
` (5 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-11 22:42 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, Greg Kurz, Daniel Henrique Barboza
Use the env_cpu function to get the CPUState for cpu_abort. These are
only needed in case of fatal errors so this allows to avoid casting
and storing CPUState in a local variable wnen not needed.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
target/ppc/excp_helper.c | 118 +++++++++++++++++++++------------------
1 file changed, 63 insertions(+), 55 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index e4532f5088..51202f7028 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -422,7 +422,6 @@ static void powerpc_checkstop_state(CPUPPCState *env)
static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
{
- CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
target_ulong msr, new_msr, vector;
int srr0, srr1;
@@ -450,8 +449,8 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
vector = env->excp_vectors[excp];
if (vector == (target_ulong)-1ULL) {
- cpu_abort(cs, "Raised an exception without defined vector %d\n",
- excp);
+ cpu_abort(env_cpu(env),
+ "Raised an exception without defined vector %d\n", excp);
}
vector |= env->excp_prefix;
@@ -500,7 +499,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
env->spr[SPR_40x_ESR] = ESR_PTR;
break;
default:
- cpu_abort(cs, "Invalid program exception %d. Aborting\n",
+ cpu_abort(env_cpu(env), "Invalid program exception %d. Aborting\n",
env->error_code);
break;
}
@@ -527,11 +526,12 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
trace_ppc_excp_print("PIT");
break;
case POWERPC_EXCP_DEBUG: /* Debug interrupt */
- cpu_abort(cs, "%s exception not implemented\n",
+ cpu_abort(env_cpu(env), "%s exception not implemented\n",
powerpc_excp_name(excp));
break;
default:
- cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
+ cpu_abort(env_cpu(env), "Invalid PowerPC exception %d. Aborting\n",
+ excp);
break;
}
@@ -546,7 +546,6 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
{
- CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
target_ulong msr, new_msr, vector;
@@ -569,8 +568,8 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
vector = env->excp_vectors[excp];
if (vector == (target_ulong)-1ULL) {
- cpu_abort(cs, "Raised an exception without defined vector %d\n",
- excp);
+ cpu_abort(env_cpu(env),
+ "Raised an exception without defined vector %d\n", excp);
}
vector |= env->excp_prefix;
@@ -630,7 +629,7 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
break;
default:
/* Should never occur */
- cpu_abort(cs, "Invalid program exception %d. Aborting\n",
+ cpu_abort(env_cpu(env), "Invalid program exception %d. Aborting\n",
env->error_code);
break;
}
@@ -652,8 +651,9 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
break;
case POWERPC_EXCP_RESET: /* System reset exception */
if (FIELD_EX64(env->msr, MSR, POW)) {
- cpu_abort(cs, "Trying to deliver power-saving system reset "
- "exception %d with no HV support\n", excp);
+ cpu_abort(env_cpu(env),
+ "Trying to deliver power-saving system reset exception "
+ "%d with no HV support\n", excp);
}
break;
case POWERPC_EXCP_TRACE: /* Trace exception */
@@ -680,11 +680,12 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
case POWERPC_EXCP_SMI: /* System management interrupt */
case POWERPC_EXCP_MEXTBR: /* Maskable external breakpoint */
case POWERPC_EXCP_NMEXTBR: /* Non maskable external breakpoint */
- cpu_abort(cs, "%s exception not implemented\n",
+ cpu_abort(env_cpu(env), "%s exception not implemented\n",
powerpc_excp_name(excp));
break;
default:
- cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
+ cpu_abort(env_cpu(env), "Invalid PowerPC exception %d. Aborting\n",
+ excp);
break;
}
@@ -707,7 +708,6 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
{
- CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
target_ulong msr, new_msr, vector;
@@ -730,8 +730,8 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
vector = env->excp_vectors[excp];
if (vector == (target_ulong)-1ULL) {
- cpu_abort(cs, "Raised an exception without defined vector %d\n",
- excp);
+ cpu_abort(env_cpu(env),
+ "Raised an exception without defined vector %d\n", excp);
}
vector |= env->excp_prefix;
@@ -789,7 +789,7 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
break;
default:
/* Should never occur */
- cpu_abort(cs, "Invalid program exception %d. Aborting\n",
+ cpu_abort(env_cpu(env), "Invalid program exception %d. Aborting\n",
env->error_code);
break;
}
@@ -830,8 +830,9 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
break;
case POWERPC_EXCP_RESET: /* System reset exception */
if (FIELD_EX64(env->msr, MSR, POW)) {
- cpu_abort(cs, "Trying to deliver power-saving system reset "
- "exception %d with no HV support\n", excp);
+ cpu_abort(env_cpu(env),
+ "Trying to deliver power-saving system reset exception "
+ "%d with no HV support\n", excp);
}
break;
case POWERPC_EXCP_TRACE: /* Trace exception */
@@ -851,11 +852,12 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
case POWERPC_EXCP_SMI: /* System management interrupt */
case POWERPC_EXCP_THERM: /* Thermal interrupt */
case POWERPC_EXCP_PERFM: /* Embedded performance monitor interrupt */
- cpu_abort(cs, "%s exception not implemented\n",
+ cpu_abort(env_cpu(env), "%s exception not implemented\n",
powerpc_excp_name(excp));
break;
default:
- cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
+ cpu_abort(env_cpu(env), "Invalid PowerPC exception %d. Aborting\n",
+ excp);
break;
}
@@ -878,7 +880,6 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
{
- CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
target_ulong msr, new_msr, vector;
@@ -901,8 +902,8 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
vector = env->excp_vectors[excp];
if (vector == (target_ulong)-1ULL) {
- cpu_abort(cs, "Raised an exception without defined vector %d\n",
- excp);
+ cpu_abort(env_cpu(env),
+ "Raised an exception without defined vector %d\n", excp);
}
vector |= env->excp_prefix;
@@ -960,7 +961,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
break;
default:
/* Should never occur */
- cpu_abort(cs, "Invalid program exception %d. Aborting\n",
+ cpu_abort(env_cpu(env), "Invalid program exception %d. Aborting\n",
env->error_code);
break;
}
@@ -1001,7 +1002,8 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
break;
case POWERPC_EXCP_RESET: /* System reset exception */
if (FIELD_EX64(env->msr, MSR, POW)) {
- cpu_abort(cs, "Trying to deliver power-saving system reset "
+ cpu_abort(env_cpu(env),
+ "Trying to deliver power-saving system reset "
"exception %d with no HV support\n", excp);
}
break;
@@ -1014,11 +1016,12 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
case POWERPC_EXCP_THERM: /* Thermal interrupt */
case POWERPC_EXCP_PERFM: /* Embedded performance monitor interrupt */
case POWERPC_EXCP_VPUA: /* Vector assist exception */
- cpu_abort(cs, "%s exception not implemented\n",
+ cpu_abort(env_cpu(env), "%s exception not implemented\n",
powerpc_excp_name(excp));
break;
default:
- cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
+ cpu_abort(env_cpu(env), "Invalid PowerPC exception %d. Aborting\n",
+ excp);
break;
}
@@ -1041,7 +1044,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
{
- CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
target_ulong msr, new_msr, vector;
int srr0, srr1;
@@ -1078,8 +1080,8 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
vector = env->excp_vectors[excp];
if (vector == (target_ulong)-1ULL) {
- cpu_abort(cs, "Raised an exception without defined vector %d\n",
- excp);
+ cpu_abort(env_cpu(env),
+ "Raised an exception without defined vector %d\n", excp);
}
vector |= env->excp_prefix;
@@ -1110,6 +1112,7 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
break;
case POWERPC_EXCP_EXTERNAL: /* External input */
if (env->mpic_proxy) {
+ CPUState *cs = env_cpu(env);
/* IACK the IRQ on delivery */
env->spr[SPR_BOOKE_EPR] = ldl_phys(cs->as, env->mpic_iack);
}
@@ -1148,7 +1151,7 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
break;
default:
/* Should never occur */
- cpu_abort(cs, "Invalid program exception %d. Aborting\n",
+ cpu_abort(env_cpu(env), "Invalid program exception %d. Aborting\n",
env->error_code);
break;
}
@@ -1189,7 +1192,8 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
/* DBSR already modified by caller */
} else {
- cpu_abort(cs, "Debug exception triggered on unsupported model\n");
+ cpu_abort(env_cpu(env),
+ "Debug exception triggered on unsupported model\n");
}
break;
case POWERPC_EXCP_SPEU: /* SPE/embedded floating-point unavailable/VPU */
@@ -1203,17 +1207,19 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
break;
case POWERPC_EXCP_RESET: /* System reset exception */
if (FIELD_EX64(env->msr, MSR, POW)) {
- cpu_abort(cs, "Trying to deliver power-saving system reset "
+ cpu_abort(env_cpu(env),
+ "Trying to deliver power-saving system reset "
"exception %d with no HV support\n", excp);
}
break;
case POWERPC_EXCP_EFPDI: /* Embedded floating-point data interrupt */
case POWERPC_EXCP_EFPRI: /* Embedded floating-point round interrupt */
- cpu_abort(cs, "%s exception not implemented\n",
+ cpu_abort(env_cpu(env), "%s exception not implemented\n",
powerpc_excp_name(excp));
break;
default:
- cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
+ cpu_abort(env_cpu(env), "Invalid PowerPC exception %d. Aborting\n",
+ excp);
break;
}
@@ -1276,7 +1282,6 @@ static bool books_vhyp_handles_hv_excp(PowerPCCPU *cpu)
static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
{
- CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
target_ulong msr, new_msr, vector;
int srr0, srr1, lev = -1;
@@ -1315,8 +1320,8 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
vector = env->excp_vectors[excp];
if (vector == (target_ulong)-1ULL) {
- cpu_abort(cs, "Raised an exception without defined vector %d\n",
- excp);
+ cpu_abort(env_cpu(env),
+ "Raised an exception without defined vector %d\n", excp);
}
vector |= env->excp_prefix;
@@ -1406,7 +1411,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
break;
default:
/* Should never occur */
- cpu_abort(cs, "Invalid program exception %d. Aborting\n",
+ cpu_abort(env_cpu(env), "Invalid program exception %d. Aborting\n",
env->error_code);
break;
}
@@ -1467,7 +1472,8 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
new_msr |= (target_ulong)MSR_HVB;
} else {
if (FIELD_EX64(env->msr, MSR, POW)) {
- cpu_abort(cs, "Trying to deliver power-saving system reset "
+ cpu_abort(env_cpu(env),
+ "Trying to deliver power-saving system reset "
"exception %d with no HV support\n", excp);
}
}
@@ -1522,11 +1528,12 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
case POWERPC_EXCP_VPUA: /* Vector assist exception */
case POWERPC_EXCP_MAINT: /* Maintenance exception */
case POWERPC_EXCP_HV_MAINT: /* Hypervisor Maintenance exception */
- cpu_abort(cs, "%s exception not implemented\n",
+ cpu_abort(env_cpu(env), "%s exception not implemented\n",
powerpc_excp_name(excp));
break;
default:
- cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
+ cpu_abort(env_cpu(env), "Invalid PowerPC exception %d. Aborting\n",
+ excp);
break;
}
@@ -1559,8 +1566,8 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
} else {
/* Sanity check */
if (!(env->msr_mask & MSR_HVB) && srr0 == SPR_HSRR0) {
- cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with "
- "no HV support\n", excp);
+ cpu_abort(env_cpu(env), "Trying to deliver HV exception (HSRR) %d "
+ "with no HV support\n", excp);
}
/* This can update new_msr and vector if AIL applies */
@@ -1578,11 +1585,11 @@ static inline void powerpc_excp_books(PowerPCCPU *cpu, int excp)
static void powerpc_excp(PowerPCCPU *cpu, int excp)
{
- CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
if (excp <= POWERPC_EXCP_NONE || excp >= POWERPC_EXCP_NB) {
- cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
+ cpu_abort(env_cpu(env), "Invalid PowerPC exception %d. Aborting\n",
+ excp);
}
qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
@@ -2116,7 +2123,6 @@ void ppc_maybe_interrupt(CPUPPCState *env)
static void p7_deliver_interrupt(CPUPPCState *env, int interrupt)
{
PowerPCCPU *cpu = env_archcpu(env);
- CPUState *cs = env_cpu(env);
switch (interrupt) {
case PPC_INTERRUPT_MCK: /* Machine check exception */
@@ -2160,14 +2166,14 @@ static void p7_deliver_interrupt(CPUPPCState *env, int interrupt)
assert(!env->resume_as_sreset);
break;
default:
- cpu_abort(cs, "Invalid PowerPC interrupt %d. Aborting\n", interrupt);
+ cpu_abort(env_cpu(env), "Invalid PowerPC interrupt %d. Aborting\n",
+ interrupt);
}
}
static void p8_deliver_interrupt(CPUPPCState *env, int interrupt)
{
PowerPCCPU *cpu = env_archcpu(env);
- CPUState *cs = env_cpu(env);
switch (interrupt) {
case PPC_INTERRUPT_MCK: /* Machine check exception */
@@ -2231,7 +2237,8 @@ static void p8_deliver_interrupt(CPUPPCState *env, int interrupt)
assert(!env->resume_as_sreset);
break;
default:
- cpu_abort(cs, "Invalid PowerPC interrupt %d. Aborting\n", interrupt);
+ cpu_abort(env_cpu(env), "Invalid PowerPC interrupt %d. Aborting\n",
+ interrupt);
}
}
@@ -2310,7 +2317,8 @@ static void p9_deliver_interrupt(CPUPPCState *env, int interrupt)
assert(!env->resume_as_sreset);
break;
default:
- cpu_abort(cs, "Invalid PowerPC interrupt %d. Aborting\n", interrupt);
+ cpu_abort(env_cpu(env), "Invalid PowerPC interrupt %d. Aborting\n",
+ interrupt);
}
}
#endif
@@ -2318,7 +2326,6 @@ static void p9_deliver_interrupt(CPUPPCState *env, int interrupt)
static void ppc_deliver_interrupt_generic(CPUPPCState *env, int interrupt)
{
PowerPCCPU *cpu = env_archcpu(env);
- CPUState *cs = env_cpu(env);
switch (interrupt) {
case PPC_INTERRUPT_RESET: /* External reset */
@@ -2415,7 +2422,8 @@ static void ppc_deliver_interrupt_generic(CPUPPCState *env, int interrupt)
assert(!env->resume_as_sreset);
break;
default:
- cpu_abort(cs, "Invalid PowerPC interrupt %d. Aborting\n", interrupt);
+ cpu_abort(env_cpu(env), "Invalid PowerPC interrupt %d. Aborting\n",
+ interrupt);
}
}
--
2.30.9
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 05/10] target/ppc: Change parameter of cpu_interrupt_exittb() to an env pointer
2023-06-11 22:42 [PATCH 00/10] Misc clean ups to target/ppc exception handling BALATON Zoltan
` (3 preceding siblings ...)
2023-06-11 22:42 ` [PATCH 04/10] target/ppc: Use env_cpu for cpu_abort in excp_helper BALATON Zoltan
@ 2023-06-11 22:42 ` BALATON Zoltan
2023-06-11 22:42 ` [PATCH 06/10] target/ppc: Readability improvements in exception handlers BALATON Zoltan
` (4 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-11 22:42 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, Greg Kurz, Daniel Henrique Barboza
Changing the parameter of cpu_interrupt_exittb() from CPUState to env
allows removing some more local CPUState variables in callers.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
target/ppc/excp_helper.c | 9 +++------
target/ppc/helper_regs.c | 15 ++++++---------
target/ppc/helper_regs.h | 2 +-
3 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 51202f7028..b681edbdcf 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -416,7 +416,7 @@ static void powerpc_checkstop_state(CPUPPCState *env)
"Entering checkstop state\n");
}
cs->halted = 1;
- cpu_interrupt_exittb(cs);
+ cpu_interrupt_exittb(env);
}
}
@@ -2549,8 +2549,7 @@ void helper_store_msr(CPUPPCState *env, target_ulong val)
uint32_t excp = hreg_store_msr(env, val, 0);
if (excp != 0) {
- CPUState *cs = env_cpu(env);
- cpu_interrupt_exittb(cs);
+ cpu_interrupt_exittb(env);
raise_exception(env, excp);
}
}
@@ -2587,8 +2586,6 @@ void helper_pminsn(CPUPPCState *env, uint32_t insn)
static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
{
- CPUState *cs = env_cpu(env);
-
/* MSR:POW cannot be set by any form of rfi */
msr &= ~(1ULL << MSR_POW);
@@ -2612,7 +2609,7 @@ static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
* No need to raise an exception here, as rfi is always the last
* insn of a TB
*/
- cpu_interrupt_exittb(cs);
+ cpu_interrupt_exittb(env);
/* Reset the reservation */
env->reserve_addr = -1;
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index bc7e9d7eda..ffedd38985 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -237,7 +237,7 @@ void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
}
#endif
-void cpu_interrupt_exittb(CPUState *cs)
+void cpu_interrupt_exittb(CPUPPCState *env)
{
/*
* We don't need to worry about translation blocks
@@ -245,18 +245,14 @@ void cpu_interrupt_exittb(CPUState *cs)
*/
if (tcg_enabled()) {
QEMU_IOTHREAD_LOCK_GUARD();
- cpu_interrupt(cs, CPU_INTERRUPT_EXITTB);
+ cpu_interrupt(env_cpu(env), CPU_INTERRUPT_EXITTB);
}
}
int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv)
{
- int excp;
-#if !defined(CONFIG_USER_ONLY)
- CPUState *cs = env_cpu(env);
-#endif
+ int excp = 0;
- excp = 0;
value &= env->msr_mask;
#if !defined(CONFIG_USER_ONLY)
/* Neither mtmsr nor guest state can alter HV */
@@ -265,12 +261,12 @@ int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv)
value |= env->msr & MSR_HVB;
}
if ((value ^ env->msr) & (R_MSR_IR_MASK | R_MSR_DR_MASK)) {
- cpu_interrupt_exittb(cs);
+ cpu_interrupt_exittb(env);
}
if ((env->mmu_model == POWERPC_MMU_BOOKE ||
env->mmu_model == POWERPC_MMU_BOOKE206) &&
((value ^ env->msr) & R_MSR_GS_MASK)) {
- cpu_interrupt_exittb(cs);
+ cpu_interrupt_exittb(env);
}
if (unlikely((env->flags & POWERPC_FLAG_TGPR) &&
((value ^ env->msr) & (1 << MSR_TGPR)))) {
@@ -301,6 +297,7 @@ int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv)
if (unlikely(FIELD_EX64(env->msr, MSR, POW))) {
if (!env->pending_interrupts && (*env->check_pow)(env)) {
+ CPUState *cs = env_cpu(env);
cs->halted = 1;
excp = EXCP_HALTED;
}
diff --git a/target/ppc/helper_regs.h b/target/ppc/helper_regs.h
index 8196c1346d..3e1606f293 100644
--- a/target/ppc/helper_regs.h
+++ b/target/ppc/helper_regs.h
@@ -23,7 +23,7 @@
void hreg_swap_gpr_tgpr(CPUPPCState *env);
void hreg_compute_hflags(CPUPPCState *env);
void hreg_update_pmu_hflags(CPUPPCState *env);
-void cpu_interrupt_exittb(CPUState *cs);
+void cpu_interrupt_exittb(CPUPPCState *env);
int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv);
#ifdef CONFIG_USER_ONLY
--
2.30.9
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 06/10] target/ppc: Readability improvements in exception handlers
2023-06-11 22:42 [PATCH 00/10] Misc clean ups to target/ppc exception handling BALATON Zoltan
` (4 preceding siblings ...)
2023-06-11 22:42 ` [PATCH 05/10] target/ppc: Change parameter of cpu_interrupt_exittb() to an env pointer BALATON Zoltan
@ 2023-06-11 22:42 ` BALATON Zoltan
2023-06-14 3:42 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 07/10] target/ppd: Remove unused define BALATON Zoltan
` (3 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-11 22:42 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, Greg Kurz, Daniel Henrique Barboza
Improve readability by shortening some long comments, removing
comments that state the obvious and dropping some empty lines so they
don't distract when reading the code.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
target/ppc/cpu.h | 1 +
target/ppc/excp_helper.c | 180 +++++++--------------------------------
2 files changed, 33 insertions(+), 148 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 0ee2adc105..d7acd65176 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2739,6 +2739,7 @@ static inline bool ppc_has_spr(PowerPCCPU *cpu, int spr)
}
#if !defined(CONFIG_USER_ONLY)
+/* Sort out endianness of interrupt. Depends on the CPU, HV mode, etc. */
static inline bool ppc_interrupts_little_endian(PowerPCCPU *cpu, bool hv)
{
PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index b681edbdcf..885e479301 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -382,9 +382,8 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong vector,
* We don't use hreg_store_msr here as already have treated any
* special case that could occur. Just store MSR and update hflags
*
- * Note: We *MUST* not use hreg_store_msr() as-is anyway because it
- * will prevent setting of the HV bit which some exceptions might need
- * to do.
+ * Note: We *MUST* not use hreg_store_msr() as-is anyway because it will
+ * prevent setting of the HV bit which some exceptions might need to do.
*/
env->nip = vector;
env->msr = msr;
@@ -424,25 +423,15 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
{
CPUPPCState *env = &cpu->env;
target_ulong msr, new_msr, vector;
- int srr0, srr1;
+ int srr0 = SPR_SRR0, srr1 = SPR_SRR1;
/* new srr1 value excluding must-be-zero bits */
msr = env->msr & ~0x783f0000ULL;
- /*
- * new interrupt handler msr preserves existing ME unless
- * explicitly overriden.
- */
+ /* new interrupt handler msr preserves ME unless explicitly overriden */
new_msr = env->msr & (((target_ulong)1 << MSR_ME));
- /* target registers */
- srr0 = SPR_SRR0;
- srr1 = SPR_SRR1;
-
- /*
- * Hypervisor emulation assistance interrupt only exists on server
- * arch 2.05 server or later.
- */
+ /* HV emu assistance interrupt only exists on server arch 2.05 or later */
if (excp == POWERPC_EXCP_HV_EMU) {
excp = POWERPC_EXCP_PROGRAM;
}
@@ -452,7 +441,6 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
cpu_abort(env_cpu(env),
"Raised an exception without defined vector %d\n", excp);
}
-
vector |= env->excp_prefix;
switch (excp) {
@@ -464,7 +452,6 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
powerpc_checkstop_state(env);
/* machine check exceptions don't have ME set */
new_msr &= ~((target_ulong)1 << MSR_ME);
-
srr0 = SPR_40x_SRR2;
srr1 = SPR_40x_SRR3;
break;
@@ -535,12 +522,8 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
break;
}
- /* Save PC */
env->spr[srr0] = env->nip;
-
- /* Save MSR */
env->spr[srr1] = msr;
-
powerpc_set_excp_state(cpu, vector, new_msr);
}
@@ -552,16 +535,10 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
/* new srr1 value excluding must-be-zero bits */
msr = env->msr & ~0x783f0000ULL;
- /*
- * new interrupt handler msr preserves existing ME unless
- * explicitly overriden
- */
+ /* new interrupt handler msr preserves ME unless explicitly overriden */
new_msr = env->msr & ((target_ulong)1 << MSR_ME);
- /*
- * Hypervisor emulation assistance interrupt only exists on server
- * arch 2.05 server or later.
- */
+ /* HV emu assistance interrupt only exists on server arch 2.05 or later */
if (excp == POWERPC_EXCP_HV_EMU) {
excp = POWERPC_EXCP_PROGRAM;
}
@@ -571,7 +548,6 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
cpu_abort(env_cpu(env),
"Raised an exception without defined vector %d\n", excp);
}
-
vector |= env->excp_prefix;
switch (excp) {
@@ -581,7 +557,6 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
powerpc_checkstop_state(env);
/* machine check exceptions don't have ME set */
new_msr &= ~((target_ulong)1 << MSR_ME);
-
break;
case POWERPC_EXCP_DSI: /* Data storage exception */
trace_ppc_excp_dsi(env->spr[SPR_DSISR], env->spr[SPR_DAR]);
@@ -609,11 +584,9 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
powerpc_reset_excp_state(cpu);
return;
}
-
/*
- * FP exceptions always have NIP pointing to the faulting
- * instruction, so always use store_next and claim we are
- * precise in the MSR.
+ * NIP always points to the faulting instruction for FP exceptions,
+ * so always use store_next and claim we are precise in the MSR.
*/
msr |= 0x00100000;
break;
@@ -689,20 +662,11 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
break;
}
- /*
- * Sort out endianness of interrupt, this differs depending on the
- * CPU, the HV mode, etc...
- */
if (ppc_interrupts_little_endian(cpu, !!(new_msr & MSR_HVB))) {
new_msr |= (target_ulong)1 << MSR_LE;
}
-
- /* Save PC */
env->spr[SPR_SRR0] = env->nip;
-
- /* Save MSR */
env->spr[SPR_SRR1] = msr;
-
powerpc_set_excp_state(cpu, vector, new_msr);
}
@@ -714,16 +678,10 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
/* new srr1 value excluding must-be-zero bits */
msr = env->msr & ~0x783f0000ULL;
- /*
- * new interrupt handler msr preserves existing ME unless
- * explicitly overriden
- */
+ /* new interrupt handler msr preserves ME unless explicitly overriden */
new_msr = env->msr & ((target_ulong)1 << MSR_ME);
- /*
- * Hypervisor emulation assistance interrupt only exists on server
- * arch 2.05 server or later.
- */
+ /* HV emu assistance interrupt only exists on server arch 2.05 or later */
if (excp == POWERPC_EXCP_HV_EMU) {
excp = POWERPC_EXCP_PROGRAM;
}
@@ -733,7 +691,6 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
cpu_abort(env_cpu(env),
"Raised an exception without defined vector %d\n", excp);
}
-
vector |= env->excp_prefix;
switch (excp) {
@@ -741,7 +698,6 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
powerpc_checkstop_state(env);
/* machine check exceptions don't have ME set */
new_msr &= ~((target_ulong)1 << MSR_ME);
-
break;
case POWERPC_EXCP_DSI: /* Data storage exception */
trace_ppc_excp_dsi(env->spr[SPR_DSISR], env->spr[SPR_DAR]);
@@ -769,11 +725,9 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
powerpc_reset_excp_state(cpu);
return;
}
-
/*
- * FP exceptions always have NIP pointing to the faulting
- * instruction, so always use store_next and claim we are
- * precise in the MSR.
+ * NIP always points to the faulting instruction for FP exceptions,
+ * so always use store_next and claim we are precise in the MSR.
*/
msr |= 0x00100000;
break;
@@ -841,12 +795,10 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
case POWERPC_EXCP_DLTLB: /* Data load TLB miss */
case POWERPC_EXCP_DSTLB: /* Data store TLB miss */
ppc_excp_debug_sw_tlb(env, excp);
-
msr |= env->crf[0] << 28;
msr |= env->error_code; /* key, D/I, S/L bits */
/* Set way using a LRU mechanism */
msr |= ((env->last_way + 1) & (env->nb_ways - 1)) << 17;
-
break;
case POWERPC_EXCP_IABR: /* Instruction address breakpoint */
case POWERPC_EXCP_SMI: /* System management interrupt */
@@ -861,20 +813,11 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
break;
}
- /*
- * Sort out endianness of interrupt, this differs depending on the
- * CPU, the HV mode, etc...
- */
if (ppc_interrupts_little_endian(cpu, !!(new_msr & MSR_HVB))) {
new_msr |= (target_ulong)1 << MSR_LE;
}
-
- /* Save PC */
env->spr[SPR_SRR0] = env->nip;
-
- /* Save MSR */
env->spr[SPR_SRR1] = msr;
-
powerpc_set_excp_state(cpu, vector, new_msr);
}
@@ -886,16 +829,10 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
/* new srr1 value excluding must-be-zero bits */
msr = env->msr & ~0x783f0000ULL;
- /*
- * new interrupt handler msr preserves existing ME unless
- * explicitly overriden
- */
+ /* new interrupt handler msr preserves ME unless explicitly overriden */
new_msr = env->msr & ((target_ulong)1 << MSR_ME);
- /*
- * Hypervisor emulation assistance interrupt only exists on server
- * arch 2.05 server or later.
- */
+ /* HV emu assistance interrupt only exists on server arch 2.05 or later */
if (excp == POWERPC_EXCP_HV_EMU) {
excp = POWERPC_EXCP_PROGRAM;
}
@@ -905,7 +842,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
cpu_abort(env_cpu(env),
"Raised an exception without defined vector %d\n", excp);
}
-
vector |= env->excp_prefix;
switch (excp) {
@@ -913,7 +849,6 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
powerpc_checkstop_state(env);
/* machine check exceptions don't have ME set */
new_msr &= ~((target_ulong)1 << MSR_ME);
-
break;
case POWERPC_EXCP_DSI: /* Data storage exception */
trace_ppc_excp_dsi(env->spr[SPR_DSISR], env->spr[SPR_DAR]);
@@ -941,11 +876,9 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
powerpc_reset_excp_state(cpu);
return;
}
-
/*
- * FP exceptions always have NIP pointing to the faulting
- * instruction, so always use store_next and claim we are
- * precise in the MSR.
+ * NIP always points to the faulting instruction for FP exceptions,
+ * so always use store_next and claim we are precise in the MSR.
*/
msr |= 0x00100000;
break;
@@ -1025,20 +958,11 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
break;
}
- /*
- * Sort out endianness of interrupt, this differs depending on the
- * CPU, the HV mode, etc...
- */
if (ppc_interrupts_little_endian(cpu, !!(new_msr & MSR_HVB))) {
new_msr |= (target_ulong)1 << MSR_LE;
}
-
- /* Save PC */
env->spr[SPR_SRR0] = env->nip;
-
- /* Save MSR */
env->spr[SPR_SRR1] = msr;
-
powerpc_set_excp_state(cpu, vector, new_msr);
}
@@ -1046,24 +970,18 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
{
CPUPPCState *env = &cpu->env;
target_ulong msr, new_msr, vector;
- int srr0, srr1;
-
- msr = env->msr;
+ int srr0 = SPR_SRR0, srr1 = SPR_SRR1;
/*
- * new interrupt handler msr preserves existing ME unless
- * explicitly overriden
+ * Book E does not play games with certain bits of xSRR1 being MSR save
+ * bits and others being error status. xSRR1 is the old MSR, period.
*/
- new_msr = env->msr & ((target_ulong)1 << MSR_ME);
+ msr = env->msr;
- /* target registers */
- srr0 = SPR_SRR0;
- srr1 = SPR_SRR1;
+ /* new interrupt handler msr preserves ME unless explicitly overriden */
+ new_msr = env->msr & ((target_ulong)1 << MSR_ME);
- /*
- * Hypervisor emulation assistance interrupt only exists on server
- * arch 2.05 server or later.
- */
+ /* HV emu assistance interrupt only exists on server arch 2.05 or later */
if (excp == POWERPC_EXCP_HV_EMU) {
excp = POWERPC_EXCP_PROGRAM;
}
@@ -1083,7 +1001,6 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
cpu_abort(env_cpu(env),
"Raised an exception without defined vector %d\n", excp);
}
-
vector |= env->excp_prefix;
switch (excp) {
@@ -1127,11 +1044,9 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
powerpc_reset_excp_state(cpu);
return;
}
-
/*
- * FP exceptions always have NIP pointing to the faulting
- * instruction, so always use store_next and claim we are
- * precise in the MSR.
+ * NIP always points to the faulting instruction for FP exceptions,
+ * so always use store_next and claim we are precise in the MSR.
*/
msr |= 0x00100000;
env->spr[SPR_BOOKE_ESR] = ESR_FP;
@@ -1232,12 +1147,8 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
}
#endif
- /* Save PC */
env->spr[srr0] = env->nip;
-
- /* Save MSR */
env->spr[srr1] = msr;
-
powerpc_set_excp_state(cpu, vector, new_msr);
}
@@ -1284,21 +1195,17 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
{
CPUPPCState *env = &cpu->env;
target_ulong msr, new_msr, vector;
- int srr0, srr1, lev = -1;
+ int srr0 = SPR_SRR0, srr1 = SPR_SRR1, lev = -1;
/* new srr1 value excluding must-be-zero bits */
msr = env->msr & ~0x783f0000ULL;
/*
- * new interrupt handler msr preserves existing HV and ME unless
- * explicitly overriden
+ * new interrupt handler msr preserves HV and ME unless explicitly
+ * overriden
*/
new_msr = env->msr & (((target_ulong)1 << MSR_ME) | MSR_HVB);
- /* target registers */
- srr0 = SPR_SRR0;
- srr1 = SPR_SRR1;
-
/*
* check for special resume at 0x100 from doze/nap/sleep/winkle on
* P7/P8/P9
@@ -1323,7 +1230,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
cpu_abort(env_cpu(env),
"Raised an exception without defined vector %d\n", excp);
}
-
vector |= env->excp_prefix;
switch (excp) {
@@ -1336,10 +1242,8 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
*/
new_msr |= (target_ulong)MSR_HVB;
}
-
/* machine check exceptions don't have ME set */
new_msr &= ~((target_ulong)1 << MSR_ME);
-
break;
case POWERPC_EXCP_DSI: /* Data storage exception */
trace_ppc_excp_dsi(env->spr[SPR_DSISR], env->spr[SPR_DAR]);
@@ -1352,23 +1256,17 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
{
bool lpes0;
- /*
- * LPES0 is only taken into consideration if we support HV
- * mode for this CPU.
- */
+ /* LPES0 is only taken into consideration if we support HV mode */
if (!env->has_hv_mode) {
break;
}
-
lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0);
-
if (!lpes0) {
new_msr |= (target_ulong)MSR_HVB;
new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
srr0 = SPR_HSRR0;
srr1 = SPR_HSRR1;
}
-
break;
}
case POWERPC_EXCP_ALIGN: /* Alignment exception */
@@ -1391,11 +1289,9 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
powerpc_reset_excp_state(cpu);
return;
}
-
/*
- * FP exceptions always have NIP pointing to the faulting
- * instruction, so always use store_next and claim we are
- * precise in the MSR.
+ * NIP always points to the faulting instruction for FP exceptions,
+ * so always use store_next and claim we are precise in the MSR.
*/
msr |= 0x00100000;
break;
@@ -1537,21 +1433,13 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
break;
}
- /*
- * Sort out endianness of interrupt, this differs depending on the
- * CPU, the HV mode, etc...
- */
if (ppc_interrupts_little_endian(cpu, !!(new_msr & MSR_HVB))) {
new_msr |= (target_ulong)1 << MSR_LE;
}
-
new_msr |= (target_ulong)1 << MSR_SF;
if (excp != POWERPC_EXCP_SYSCALL_VECTORED) {
- /* Save PC */
env->spr[srr0] = env->nip;
-
- /* Save MSR */
env->spr[srr1] = msr;
}
@@ -1560,19 +1448,15 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
/* Deliver interrupt to L1 by returning from the H_ENTER_NESTED call */
vhc->deliver_hv_excp(cpu, excp);
-
powerpc_reset_excp_state(cpu);
-
} else {
/* Sanity check */
if (!(env->msr_mask & MSR_HVB) && srr0 == SPR_HSRR0) {
cpu_abort(env_cpu(env), "Trying to deliver HV exception (HSRR) %d "
"with no HV support\n", excp);
}
-
/* This can update new_msr and vector if AIL applies */
ppc_excp_apply_ail(cpu, excp, msr, &new_msr, &vector);
-
powerpc_set_excp_state(cpu, vector, new_msr);
}
}
--
2.30.9
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 07/10] target/ppd: Remove unused define
2023-06-11 22:42 [PATCH 00/10] Misc clean ups to target/ppc exception handling BALATON Zoltan
` (5 preceding siblings ...)
2023-06-11 22:42 ` [PATCH 06/10] target/ppc: Readability improvements in exception handlers BALATON Zoltan
@ 2023-06-11 22:42 ` BALATON Zoltan
2023-06-12 9:29 ` Philippe Mathieu-Daudé
2023-06-14 3:42 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 08/10] target/ppc: Fix gen_sc to use correct nip BALATON Zoltan
` (2 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-11 22:42 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, Greg Kurz, Daniel Henrique Barboza
Commit 7a3fe174b12d removed usage of POWERPC_SYSCALL_VECTORED, drop
the unused define as well.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
target/ppc/translate.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index b591f2e496..a32a9b8a5f 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4416,7 +4416,6 @@ static void gen_hrfid(DisasContext *ctx)
#define POWERPC_SYSCALL POWERPC_EXCP_SYSCALL_USER
#else
#define POWERPC_SYSCALL POWERPC_EXCP_SYSCALL
-#define POWERPC_SYSCALL_VECTORED POWERPC_EXCP_SYSCALL_VECTORED
#endif
static void gen_sc(DisasContext *ctx)
{
--
2.30.9
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 08/10] target/ppc: Fix gen_sc to use correct nip
2023-06-11 22:42 [PATCH 00/10] Misc clean ups to target/ppc exception handling BALATON Zoltan
` (6 preceding siblings ...)
2023-06-11 22:42 ` [PATCH 07/10] target/ppd: Remove unused define BALATON Zoltan
@ 2023-06-11 22:42 ` BALATON Zoltan
2023-06-14 3:47 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 09/10] target/ppc: Simplify syscall exception handlers BALATON Zoltan
2023-06-11 22:42 ` [PATCH 10/10] target/ppc: Get CPUState in one step BALATON Zoltan
9 siblings, 1 reply; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-11 22:42 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, Greg Kurz, Daniel Henrique Barboza
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;
}
#if defined(TARGET_PPC64)
--
2.30.9
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 09/10] target/ppc: Simplify syscall exception handlers
2023-06-11 22:42 [PATCH 00/10] Misc clean ups to target/ppc exception handling BALATON Zoltan
` (7 preceding siblings ...)
2023-06-11 22:42 ` [PATCH 08/10] target/ppc: Fix gen_sc to use correct nip BALATON Zoltan
@ 2023-06-11 22:42 ` BALATON Zoltan
2023-06-14 4:18 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 10/10] target/ppc: Get CPUState in one step BALATON Zoltan
9 siblings, 1 reply; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-11 22:42 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, Greg Kurz, Daniel Henrique Barboza
After previous changes the hypercall handling in 7xx and 74xx
exception handlers can be folded into one if statement to simpilfy
this code.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
target/ppc/excp_helper.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 4f6a6dfb19..089417894e 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -738,26 +738,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
break;
case POWERPC_EXCP_SYSCALL: /* System call exception */
{
- int lev = env->error_code;
+ PowerPCCPU *cpu = env_archcpu(env);
- if (lev == 1 && cpu->vhyp) {
- dump_hcall(env);
- } else {
- dump_syscall(env);
- }
/*
* The Virtual Open Firmware (VOF) relies on the 'sc 1'
* instruction to communicate with QEMU. The pegasos2 machine
* uses VOF and the 7xx CPUs, so although the 7xx don't have
* HV mode, we need to keep hypercall support.
*/
- if (lev == 1 && cpu->vhyp) {
+ if (unlikely(env->error_code == 1 && cpu->vhyp)) {
PPCVirtualHypervisorClass *vhc =
PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
+ dump_hcall(env);
vhc->hypercall(cpu->vhyp, cpu);
return;
+ } else {
+ dump_syscall(env);
}
-
break;
}
case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */
@@ -882,26 +879,23 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
break;
case POWERPC_EXCP_SYSCALL: /* System call exception */
{
- int lev = env->error_code;
+ PowerPCCPU *cpu = env_archcpu(env);
- if (lev == 1 && cpu->vhyp) {
- dump_hcall(env);
- } else {
- dump_syscall(env);
- }
/*
* The Virtual Open Firmware (VOF) relies on the 'sc 1'
* instruction to communicate with QEMU. The pegasos2 machine
* uses VOF and the 74xx CPUs, so although the 74xx don't have
* HV mode, we need to keep hypercall support.
*/
- if (lev == 1 && cpu->vhyp) {
+ if (unlikely(env->error_code == 1 && cpu->vhyp)) {
PPCVirtualHypervisorClass *vhc =
PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
+ dump_hcall(env);
vhc->hypercall(cpu->vhyp, cpu);
return;
+ } else {
+ dump_syscall(env);
}
-
break;
}
case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */
--
2.30.9
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 10/10] target/ppc: Get CPUState in one step
2023-06-11 22:42 [PATCH 00/10] Misc clean ups to target/ppc exception handling BALATON Zoltan
` (8 preceding siblings ...)
2023-06-11 22:42 ` [PATCH 09/10] target/ppc: Simplify syscall exception handlers BALATON Zoltan
@ 2023-06-11 22:42 ` BALATON Zoltan
9 siblings, 0 replies; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-11 22:42 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, Greg Kurz, Daniel Henrique Barboza
We can get CPUState from env with env_cpu without going through
PowerPCCPU and casting that.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
target/ppc/excp_helper.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 089417894e..8396fe2334 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1504,8 +1504,8 @@ static int p7_interrupt_powersave(CPUPPCState *env)
static int p7_next_unmasked_interrupt(CPUPPCState *env)
{
- PowerPCCPU *cpu = env_archcpu(env);
- CPUState *cs = CPU(cpu);
+ CPUState *cs = env_cpu(env);
+
/* Ignore MSR[EE] when coming out of some power management states */
bool msr_ee = FIELD_EX64(env->msr, MSR, EE) || env->resume_as_sreset;
@@ -1594,8 +1594,8 @@ static int p8_interrupt_powersave(CPUPPCState *env)
static int p8_next_unmasked_interrupt(CPUPPCState *env)
{
- PowerPCCPU *cpu = env_archcpu(env);
- CPUState *cs = CPU(cpu);
+ CPUState *cs = env_cpu(env);
+
/* Ignore MSR[EE] when coming out of some power management states */
bool msr_ee = FIELD_EX64(env->msr, MSR, EE) || env->resume_as_sreset;
@@ -1715,8 +1715,8 @@ static int p9_interrupt_powersave(CPUPPCState *env)
static int p9_next_unmasked_interrupt(CPUPPCState *env)
{
- PowerPCCPU *cpu = env_archcpu(env);
- CPUState *cs = CPU(cpu);
+ CPUState *cs = env_cpu(env);
+
/* Ignore MSR[EE] when coming out of some power management states */
bool msr_ee = FIELD_EX64(env->msr, MSR, EE) || env->resume_as_sreset;
@@ -2410,9 +2410,8 @@ void helper_scv(CPUPPCState *env, uint32_t lev)
void helper_pminsn(CPUPPCState *env, uint32_t insn)
{
- CPUState *cs;
+ CPUState *cs = env_cpu(env);
- cs = env_cpu(env);
cs->halted = 1;
/* Condition for waking up at 0x100 */
--
2.30.9
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 03/10] target/ppc: Move common check in exception handlers to a function
2023-06-11 22:42 ` [PATCH 03/10] target/ppc: Move common check in exception handlers to a function BALATON Zoltan
@ 2023-06-12 9:28 ` Philippe Mathieu-Daudé
2023-06-12 10:07 ` BALATON Zoltan
2023-06-14 3:35 ` Nicholas Piggin
1 sibling, 1 reply; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-06-12 9:28 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: clg, Greg Kurz, Daniel Henrique Barboza
On 12/6/23 00:42, BALATON Zoltan wrote:
> All powerpc exception handlers share some code when handling machine
> check exceptions. Move this to a common function.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> target/ppc/excp_helper.c | 112 ++++++++-------------------------------
> 1 file changed, 23 insertions(+), 89 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 3783315fdb..e4532f5088 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -403,6 +403,23 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong vector,
> env->reserve_addr = -1;
> }
>
> +static void powerpc_checkstop_state(CPUPPCState *env)
> +{
> + if (!FIELD_EX64(env->msr, MSR, ME)) {
Preferably inverting this if() and returning early:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> + CPUState *cs = env_cpu(env);
> +
> + /* Machine check exception is not enabled. Enter checkstop state. */
> + fprintf(stderr, "Machine check while not allowed. "
> + "Entering checkstop state\n");
> + if (qemu_log_separate()) {
> + qemu_log("Machine check while not allowed. "
> + "Entering checkstop state\n");
> + }
> + cs->halted = 1;
> + cpu_interrupt_exittb(cs);
> + }
> +}
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 07/10] target/ppd: Remove unused define
2023-06-11 22:42 ` [PATCH 07/10] target/ppd: Remove unused define BALATON Zoltan
@ 2023-06-12 9:29 ` Philippe Mathieu-Daudé
2023-06-14 3:42 ` Nicholas Piggin
1 sibling, 0 replies; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-06-12 9:29 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: clg, Greg Kurz, Daniel Henrique Barboza
On 12/6/23 00:42, BALATON Zoltan wrote:
> Commit 7a3fe174b12d removed usage of POWERPC_SYSCALL_VECTORED, drop
> the unused define as well.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> target/ppc/translate.c | 1 -
> 1 file changed, 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/10] target/ppc: Move common check in exception handlers to a function
2023-06-12 9:28 ` Philippe Mathieu-Daudé
@ 2023-06-12 10:07 ` BALATON Zoltan
0 siblings, 0 replies; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-12 10:07 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, qemu-ppc, clg, Greg Kurz, Daniel Henrique Barboza
[-- Attachment #1: Type: text/plain, Size: 1604 bytes --]
On Mon, 12 Jun 2023, Philippe Mathieu-Daudé wrote:
> On 12/6/23 00:42, BALATON Zoltan wrote:
>> All powerpc exception handlers share some code when handling machine
>> check exceptions. Move this to a common function.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> target/ppc/excp_helper.c | 112 ++++++++-------------------------------
>> 1 file changed, 23 insertions(+), 89 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 3783315fdb..e4532f5088 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -403,6 +403,23 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu,
>> target_ulong vector,
>> env->reserve_addr = -1;
>> }
>> +static void powerpc_checkstop_state(CPUPPCState *env)
>> +{
>> + if (!FIELD_EX64(env->msr, MSR, ME)) {
>
> Preferably inverting this if() and returning early:
Good idea, will wait for some more review in case any other changes are
needed before sending a v2 for this.
Regards,
BALATON Zoltan
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>> + CPUState *cs = env_cpu(env);
>> +
>> + /* Machine check exception is not enabled. Enter checkstop state.
>> */
>> + fprintf(stderr, "Machine check while not allowed. "
>> + "Entering checkstop state\n");
>> + if (qemu_log_separate()) {
>> + qemu_log("Machine check while not allowed. "
>> + "Entering checkstop state\n");
>> + }
>> + cs->halted = 1;
>> + cpu_interrupt_exittb(cs);
>> + }
>> +}
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 01/10] target/ppc: Remove some superfluous parentheses
2023-06-11 22:42 ` [PATCH 01/10] target/ppc: Remove some superfluous parentheses BALATON Zoltan
@ 2023-06-14 3:25 ` Nicholas Piggin
0 siblings, 0 replies; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-14 3:25 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: clg, Greg Kurz, Daniel Henrique Barboza
On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Acked-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> target/ppc/excp_helper.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 12d8a7257b..8298217e78 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1009,7 +1009,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
> {
> int lev = env->error_code;
>
> - if ((lev == 1) && cpu->vhyp) {
> + if (lev == 1 && cpu->vhyp) {
> dump_hcall(env);
> } else {
> dump_syscall(env);
> @@ -1027,7 +1027,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
> * uses VOF and the 74xx CPUs, so although the 74xx don't have
> * HV mode, we need to keep hypercall support.
> */
> - if ((lev == 1) && cpu->vhyp) {
> + if (lev == 1 && cpu->vhyp) {
> PPCVirtualHypervisorClass *vhc =
> PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> vhc->hypercall(cpu->vhyp, cpu);
> @@ -1481,7 +1481,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
> case POWERPC_EXCP_SYSCALL: /* System call exception */
> lev = env->error_code;
>
> - if ((lev == 1) && cpu->vhyp) {
> + if (lev == 1 && cpu->vhyp) {
> dump_hcall(env);
> } else {
> dump_syscall(env);
> @@ -1494,7 +1494,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
> env->nip += 4;
>
> /* "PAPR mode" built-in hypercall emulation */
> - if ((lev == 1) && books_vhyp_handles_hcall(cpu)) {
> + if (lev == 1 && books_vhyp_handles_hcall(cpu)) {
> PPCVirtualHypervisorClass *vhc =
> PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> vhc->hypercall(cpu->vhyp, cpu);
> --
> 2.30.9
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 02/10] target/ppc: Remove unneeded parameter from powerpc_reset_wakeup()
2023-06-11 22:42 ` [PATCH 02/10] target/ppc: Remove unneeded parameter from powerpc_reset_wakeup() BALATON Zoltan
@ 2023-06-14 3:26 ` Nicholas Piggin
0 siblings, 0 replies; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-14 3:26 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: clg, Greg Kurz, Daniel Henrique Barboza
On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
> CPUState is rarely needed by this function (only for logging a fatal
> error) and it's easy to get from the env parameter so passing it
> separately is not necessary.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Caller does have env already, but I don't mind much either way.
Acked-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> target/ppc/excp_helper.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 8298217e78..3783315fdb 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -166,8 +166,7 @@ static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
> }
>
> #if defined(TARGET_PPC64)
> -static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
> - target_ulong *msr)
> +static int powerpc_reset_wakeup(CPUPPCState *env, int excp, target_ulong *msr)
> {
> /* We no longer are in a PM state */
> env->resume_as_sreset = false;
> @@ -202,8 +201,8 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
> *msr |= SRR1_WAKEHVI;
> break;
> default:
> - cpu_abort(cs, "Unsupported exception %d in Power Save mode\n",
> - excp);
> + cpu_abort(env_cpu(env),
> + "Unsupported exception %d in Power Save mode\n", excp);
> }
> return POWERPC_EXCP_RESET;
> }
> @@ -1353,7 +1352,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
> * P7/P8/P9
> */
> if (env->resume_as_sreset) {
> - excp = powerpc_reset_wakeup(cs, env, excp, &msr);
> + excp = powerpc_reset_wakeup(env, excp, &msr);
> }
>
> /*
> --
> 2.30.9
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/10] target/ppc: Move common check in exception handlers to a function
2023-06-11 22:42 ` [PATCH 03/10] target/ppc: Move common check in exception handlers to a function BALATON Zoltan
2023-06-12 9:28 ` Philippe Mathieu-Daudé
@ 2023-06-14 3:35 ` Nicholas Piggin
2023-06-14 6:25 ` Cédric Le Goater
1 sibling, 1 reply; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-14 3:35 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: clg, Greg Kurz, Daniel Henrique Barboza
On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
> All powerpc exception handlers share some code when handling machine
> check exceptions. Move this to a common function.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Hah, I just did very similar to improve some checkstop code (but I can
rebase my patches on yours).
This is specifically to test checkstop due to machine check with
MSR[ME]=0 (other things can potentially case a checkstop). So
maybe rename it powerpc_mcheck_test_checkstop or something like
that?
Mechanically looks okay though, so other than the name,
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> target/ppc/excp_helper.c | 112 ++++++++-------------------------------
> 1 file changed, 23 insertions(+), 89 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 3783315fdb..e4532f5088 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -403,6 +403,23 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong vector,
> env->reserve_addr = -1;
> }
>
> +static void powerpc_checkstop_state(CPUPPCState *env)
> +{
> + if (!FIELD_EX64(env->msr, MSR, ME)) {
> + CPUState *cs = env_cpu(env);
> +
> + /* Machine check exception is not enabled. Enter checkstop state. */
> + fprintf(stderr, "Machine check while not allowed. "
> + "Entering checkstop state\n");
> + if (qemu_log_separate()) {
> + qemu_log("Machine check while not allowed. "
> + "Entering checkstop state\n");
> + }
> + cs->halted = 1;
> + cpu_interrupt_exittb(cs);
> + }
> +}
> +
> static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
> {
> CPUState *cs = CPU(cpu);
> @@ -445,21 +462,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
> srr1 = SPR_40x_SRR3;
> break;
> case POWERPC_EXCP_MCHECK: /* Machine check exception */
> - if (!FIELD_EX64(env->msr, MSR, ME)) {
> - /*
> - * Machine check exception is not enabled. Enter
> - * checkstop state.
> - */
> - fprintf(stderr, "Machine check while not allowed. "
> - "Entering checkstop state\n");
> - if (qemu_log_separate()) {
> - qemu_log("Machine check while not allowed. "
> - "Entering checkstop state\n");
> - }
> - cs->halted = 1;
> - cpu_interrupt_exittb(cs);
> - }
> -
> + powerpc_checkstop_state(env);
> /* machine check exceptions don't have ME set */
> new_msr &= ~((target_ulong)1 << MSR_ME);
>
> @@ -576,21 +579,7 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
> case POWERPC_EXCP_CRITICAL: /* Critical input */
> break;
> case POWERPC_EXCP_MCHECK: /* Machine check exception */
> - if (!FIELD_EX64(env->msr, MSR, ME)) {
> - /*
> - * Machine check exception is not enabled. Enter
> - * checkstop state.
> - */
> - fprintf(stderr, "Machine check while not allowed. "
> - "Entering checkstop state\n");
> - if (qemu_log_separate()) {
> - qemu_log("Machine check while not allowed. "
> - "Entering checkstop state\n");
> - }
> - cs->halted = 1;
> - cpu_interrupt_exittb(cs);
> - }
> -
> + powerpc_checkstop_state(env);
> /* machine check exceptions don't have ME set */
> new_msr &= ~((target_ulong)1 << MSR_ME);
>
> @@ -749,21 +738,7 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
>
> switch (excp) {
> case POWERPC_EXCP_MCHECK: /* Machine check exception */
> - if (!FIELD_EX64(env->msr, MSR, ME)) {
> - /*
> - * Machine check exception is not enabled. Enter
> - * checkstop state.
> - */
> - fprintf(stderr, "Machine check while not allowed. "
> - "Entering checkstop state\n");
> - if (qemu_log_separate()) {
> - qemu_log("Machine check while not allowed. "
> - "Entering checkstop state\n");
> - }
> - cs->halted = 1;
> - cpu_interrupt_exittb(cs);
> - }
> -
> + powerpc_checkstop_state(env);
> /* machine check exceptions don't have ME set */
> new_msr &= ~((target_ulong)1 << MSR_ME);
>
> @@ -934,21 +909,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>
> switch (excp) {
> case POWERPC_EXCP_MCHECK: /* Machine check exception */
> - if (!FIELD_EX64(env->msr, MSR, ME)) {
> - /*
> - * Machine check exception is not enabled. Enter
> - * checkstop state.
> - */
> - fprintf(stderr, "Machine check while not allowed. "
> - "Entering checkstop state\n");
> - if (qemu_log_separate()) {
> - qemu_log("Machine check while not allowed. "
> - "Entering checkstop state\n");
> - }
> - cs->halted = 1;
> - cpu_interrupt_exittb(cs);
> - }
> -
> + powerpc_checkstop_state(env);
> /* machine check exceptions don't have ME set */
> new_msr &= ~((target_ulong)1 << MSR_ME);
>
> @@ -1129,21 +1090,7 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
> srr1 = SPR_BOOKE_CSRR1;
> break;
> case POWERPC_EXCP_MCHECK: /* Machine check exception */
> - if (!FIELD_EX64(env->msr, MSR, ME)) {
> - /*
> - * Machine check exception is not enabled. Enter
> - * checkstop state.
> - */
> - fprintf(stderr, "Machine check while not allowed. "
> - "Entering checkstop state\n");
> - if (qemu_log_separate()) {
> - qemu_log("Machine check while not allowed. "
> - "Entering checkstop state\n");
> - }
> - cs->halted = 1;
> - cpu_interrupt_exittb(cs);
> - }
> -
> + powerpc_checkstop_state(env);
> /* machine check exceptions don't have ME set */
> new_msr &= ~((target_ulong)1 << MSR_ME);
>
> @@ -1376,20 +1323,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>
> switch (excp) {
> case POWERPC_EXCP_MCHECK: /* Machine check exception */
> - if (!FIELD_EX64(env->msr, MSR, ME)) {
> - /*
> - * Machine check exception is not enabled. Enter
> - * checkstop state.
> - */
> - fprintf(stderr, "Machine check while not allowed. "
> - "Entering checkstop state\n");
> - if (qemu_log_separate()) {
> - qemu_log("Machine check while not allowed. "
> - "Entering checkstop state\n");
> - }
> - cs->halted = 1;
> - cpu_interrupt_exittb(cs);
> - }
> + powerpc_checkstop_state(env);
> if (env->msr_mask & MSR_HVB) {
> /*
> * ISA specifies HV, but can be delivered to guest with HV
> --
> 2.30.9
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 04/10] target/ppc: Use env_cpu for cpu_abort in excp_helper
2023-06-11 22:42 ` [PATCH 04/10] target/ppc: Use env_cpu for cpu_abort in excp_helper BALATON Zoltan
@ 2023-06-14 3:36 ` Nicholas Piggin
2023-06-14 10:13 ` BALATON Zoltan
0 siblings, 1 reply; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-14 3:36 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: clg, Greg Kurz, Daniel Henrique Barboza
On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
> Use the env_cpu function to get the CPUState for cpu_abort. These are
> only needed in case of fatal errors so this allows to avoid casting
> and storing CPUState in a local variable wnen not needed.
I don't entirely mind keeping cs around as a variable...
Thanks,
Nick
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> target/ppc/excp_helper.c | 118 +++++++++++++++++++++------------------
> 1 file changed, 63 insertions(+), 55 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index e4532f5088..51202f7028 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -422,7 +422,6 @@ static void powerpc_checkstop_state(CPUPPCState *env)
>
> static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
> {
> - CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> target_ulong msr, new_msr, vector;
> int srr0, srr1;
[snip]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 06/10] target/ppc: Readability improvements in exception handlers
2023-06-11 22:42 ` [PATCH 06/10] target/ppc: Readability improvements in exception handlers BALATON Zoltan
@ 2023-06-14 3:42 ` Nicholas Piggin
2023-06-14 10:07 ` BALATON Zoltan
0 siblings, 1 reply; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-14 3:42 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: clg, Greg Kurz, Daniel Henrique Barboza
On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
> Improve readability by shortening some long comments, removing
> comments that state the obvious and dropping some empty lines so they
> don't distract when reading the code.
Some changes are a matter of taste, but in the interest of having
somebody do some spring cleaning of this code I don't want to nitpick
it, so I won't :)
Acked-by: Nicholas Piggin <npiggin@gmail.com>
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> target/ppc/cpu.h | 1 +
> target/ppc/excp_helper.c | 180 +++++++--------------------------------
> 2 files changed, 33 insertions(+), 148 deletions(-)
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 07/10] target/ppd: Remove unused define
2023-06-11 22:42 ` [PATCH 07/10] target/ppd: Remove unused define BALATON Zoltan
2023-06-12 9:29 ` Philippe Mathieu-Daudé
@ 2023-06-14 3:42 ` Nicholas Piggin
1 sibling, 0 replies; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-14 3:42 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: clg, Greg Kurz, Daniel Henrique Barboza
On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
> Commit 7a3fe174b12d removed usage of POWERPC_SYSCALL_VECTORED, drop
> the unused define as well.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> target/ppc/translate.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index b591f2e496..a32a9b8a5f 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4416,7 +4416,6 @@ static void gen_hrfid(DisasContext *ctx)
> #define POWERPC_SYSCALL POWERPC_EXCP_SYSCALL_USER
> #else
> #define POWERPC_SYSCALL POWERPC_EXCP_SYSCALL
> -#define POWERPC_SYSCALL_VECTORED POWERPC_EXCP_SYSCALL_VECTORED
> #endif
> static void gen_sc(DisasContext *ctx)
> {
> --
> 2.30.9
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 08/10] target/ppc: Fix gen_sc to use correct nip
2023-06-11 22:42 ` [PATCH 08/10] target/ppc: Fix gen_sc to use correct nip BALATON Zoltan
@ 2023-06-14 3:47 ` Nicholas Piggin
2023-06-14 10:05 ` BALATON Zoltan
2023-06-14 21:27 ` BALATON Zoltan
0 siblings, 2 replies; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-14 3:47 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: clg, Greg Kurz, Daniel Henrique Barboza
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 09/10] target/ppc: Simplify syscall exception handlers
2023-06-11 22:42 ` [PATCH 09/10] target/ppc: Simplify syscall exception handlers BALATON Zoltan
@ 2023-06-14 4:18 ` Nicholas Piggin
2023-06-14 21:33 ` BALATON Zoltan
0 siblings, 1 reply; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-14 4:18 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc
Cc: clg, Greg Kurz, Daniel Henrique Barboza
On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
> After previous changes the hypercall handling in 7xx and 74xx
> exception handlers can be folded into one if statement to simpilfy
> this code.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> target/ppc/excp_helper.c | 26 ++++++++++----------------
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 4f6a6dfb19..089417894e 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -738,26 +738,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
> break;
> case POWERPC_EXCP_SYSCALL: /* System call exception */
> {
> - int lev = env->error_code;
> + PowerPCCPU *cpu = env_archcpu(env);
I prefer to keep lev. Makes sense to combine the tests though
I suppose. Although with powernv it's not really clear that we
want to dump_syscall. dump_hcall is probably better (powernv
could support a non-PAPR hypervisor with different hcall
semantics, but also it could support an OS with different
syscall semantics too). I guess that could be changed back
when necessary though.
Thanks,
Nick
>
> - if (lev == 1 && cpu->vhyp) {
> - dump_hcall(env);
> - } else {
> - dump_syscall(env);
> - }
> /*
> * The Virtual Open Firmware (VOF) relies on the 'sc 1'
> * instruction to communicate with QEMU. The pegasos2 machine
> * uses VOF and the 7xx CPUs, so although the 7xx don't have
> * HV mode, we need to keep hypercall support.
> */
> - if (lev == 1 && cpu->vhyp) {
> + if (unlikely(env->error_code == 1 && cpu->vhyp)) {
> PPCVirtualHypervisorClass *vhc =
> PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> + dump_hcall(env);
> vhc->hypercall(cpu->vhyp, cpu);
> return;
> + } else {
> + dump_syscall(env);
> }
> -
> break;
> }
> case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */
> @@ -882,26 +879,23 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
> break;
> case POWERPC_EXCP_SYSCALL: /* System call exception */
> {
> - int lev = env->error_code;
> + PowerPCCPU *cpu = env_archcpu(env);
>
> - if (lev == 1 && cpu->vhyp) {
> - dump_hcall(env);
> - } else {
> - dump_syscall(env);
> - }
> /*
> * The Virtual Open Firmware (VOF) relies on the 'sc 1'
> * instruction to communicate with QEMU. The pegasos2 machine
> * uses VOF and the 74xx CPUs, so although the 74xx don't have
> * HV mode, we need to keep hypercall support.
> */
> - if (lev == 1 && cpu->vhyp) {
> + if (unlikely(env->error_code == 1 && cpu->vhyp)) {
> PPCVirtualHypervisorClass *vhc =
> PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> + dump_hcall(env);
> vhc->hypercall(cpu->vhyp, cpu);
> return;
> + } else {
> + dump_syscall(env);
> }
> -
> break;
> }
> case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */
> --
> 2.30.9
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/10] target/ppc: Move common check in exception handlers to a function
2023-06-14 3:35 ` Nicholas Piggin
@ 2023-06-14 6:25 ` Cédric Le Goater
2023-06-15 1:50 ` Nicholas Piggin
0 siblings, 1 reply; 34+ messages in thread
From: Cédric Le Goater @ 2023-06-14 6:25 UTC (permalink / raw)
To: Nicholas Piggin, BALATON Zoltan, qemu-devel, qemu-ppc
Cc: Greg Kurz, Daniel Henrique Barboza
On 6/14/23 05:35, Nicholas Piggin wrote:
> On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
>> All powerpc exception handlers share some code when handling machine
>> check exceptions. Move this to a common function.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> Hah, I just did very similar to improve some checkstop code (but I can
> rebase my patches on yours).
The mce injection was a nice addition. You sent that a while back for pseries
and powernv. Are you going to revive the series ?
C.
>
> This is specifically to test checkstop due to machine check with
> MSR[ME]=0 (other things can potentially case a checkstop). So
> maybe rename it powerpc_mcheck_test_checkstop or something like
> that?
>
> Mechanically looks okay though, so other than the name,
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
>
>> ---
>> target/ppc/excp_helper.c | 112 ++++++++-------------------------------
>> 1 file changed, 23 insertions(+), 89 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 3783315fdb..e4532f5088 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -403,6 +403,23 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong vector,
>> env->reserve_addr = -1;
>> }
>>
>> +static void powerpc_checkstop_state(CPUPPCState *env)
>> +{
>> + if (!FIELD_EX64(env->msr, MSR, ME)) {
>> + CPUState *cs = env_cpu(env);
>> +
>> + /* Machine check exception is not enabled. Enter checkstop state. */
>> + fprintf(stderr, "Machine check while not allowed. "
>> + "Entering checkstop state\n");
>> + if (qemu_log_separate()) {
>> + qemu_log("Machine check while not allowed. "
>> + "Entering checkstop state\n");
>> + }
>> + cs->halted = 1;
>> + cpu_interrupt_exittb(cs);
>> + }
>> +}
>> +
>> static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>> {
>> CPUState *cs = CPU(cpu);
>> @@ -445,21 +462,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>> srr1 = SPR_40x_SRR3;
>> break;
>> case POWERPC_EXCP_MCHECK: /* Machine check exception */
>> - if (!FIELD_EX64(env->msr, MSR, ME)) {
>> - /*
>> - * Machine check exception is not enabled. Enter
>> - * checkstop state.
>> - */
>> - fprintf(stderr, "Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - if (qemu_log_separate()) {
>> - qemu_log("Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - }
>> - cs->halted = 1;
>> - cpu_interrupt_exittb(cs);
>> - }
>> -
>> + powerpc_checkstop_state(env);
>> /* machine check exceptions don't have ME set */
>> new_msr &= ~((target_ulong)1 << MSR_ME);
>>
>> @@ -576,21 +579,7 @@ static void powerpc_excp_6xx(PowerPCCPU *cpu, int excp)
>> case POWERPC_EXCP_CRITICAL: /* Critical input */
>> break;
>> case POWERPC_EXCP_MCHECK: /* Machine check exception */
>> - if (!FIELD_EX64(env->msr, MSR, ME)) {
>> - /*
>> - * Machine check exception is not enabled. Enter
>> - * checkstop state.
>> - */
>> - fprintf(stderr, "Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - if (qemu_log_separate()) {
>> - qemu_log("Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - }
>> - cs->halted = 1;
>> - cpu_interrupt_exittb(cs);
>> - }
>> -
>> + powerpc_checkstop_state(env);
>> /* machine check exceptions don't have ME set */
>> new_msr &= ~((target_ulong)1 << MSR_ME);
>>
>> @@ -749,21 +738,7 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
>>
>> switch (excp) {
>> case POWERPC_EXCP_MCHECK: /* Machine check exception */
>> - if (!FIELD_EX64(env->msr, MSR, ME)) {
>> - /*
>> - * Machine check exception is not enabled. Enter
>> - * checkstop state.
>> - */
>> - fprintf(stderr, "Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - if (qemu_log_separate()) {
>> - qemu_log("Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - }
>> - cs->halted = 1;
>> - cpu_interrupt_exittb(cs);
>> - }
>> -
>> + powerpc_checkstop_state(env);
>> /* machine check exceptions don't have ME set */
>> new_msr &= ~((target_ulong)1 << MSR_ME);
>>
>> @@ -934,21 +909,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>>
>> switch (excp) {
>> case POWERPC_EXCP_MCHECK: /* Machine check exception */
>> - if (!FIELD_EX64(env->msr, MSR, ME)) {
>> - /*
>> - * Machine check exception is not enabled. Enter
>> - * checkstop state.
>> - */
>> - fprintf(stderr, "Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - if (qemu_log_separate()) {
>> - qemu_log("Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - }
>> - cs->halted = 1;
>> - cpu_interrupt_exittb(cs);
>> - }
>> -
>> + powerpc_checkstop_state(env);
>> /* machine check exceptions don't have ME set */
>> new_msr &= ~((target_ulong)1 << MSR_ME);
>>
>> @@ -1129,21 +1090,7 @@ static void powerpc_excp_booke(PowerPCCPU *cpu, int excp)
>> srr1 = SPR_BOOKE_CSRR1;
>> break;
>> case POWERPC_EXCP_MCHECK: /* Machine check exception */
>> - if (!FIELD_EX64(env->msr, MSR, ME)) {
>> - /*
>> - * Machine check exception is not enabled. Enter
>> - * checkstop state.
>> - */
>> - fprintf(stderr, "Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - if (qemu_log_separate()) {
>> - qemu_log("Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - }
>> - cs->halted = 1;
>> - cpu_interrupt_exittb(cs);
>> - }
>> -
>> + powerpc_checkstop_state(env);
>> /* machine check exceptions don't have ME set */
>> new_msr &= ~((target_ulong)1 << MSR_ME);
>>
>> @@ -1376,20 +1323,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>>
>> switch (excp) {
>> case POWERPC_EXCP_MCHECK: /* Machine check exception */
>> - if (!FIELD_EX64(env->msr, MSR, ME)) {
>> - /*
>> - * Machine check exception is not enabled. Enter
>> - * checkstop state.
>> - */
>> - fprintf(stderr, "Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - if (qemu_log_separate()) {
>> - qemu_log("Machine check while not allowed. "
>> - "Entering checkstop state\n");
>> - }
>> - cs->halted = 1;
>> - cpu_interrupt_exittb(cs);
>> - }
>> + powerpc_checkstop_state(env);
>> if (env->msr_mask & MSR_HVB) {
>> /*
>> * ISA specifies HV, but can be delivered to guest with HV
>> --
>> 2.30.9
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 08/10] target/ppc: Fix gen_sc to use correct nip
2023-06-14 3:47 ` Nicholas Piggin
@ 2023-06-14 10:05 ` BALATON Zoltan
2023-06-14 21:27 ` BALATON Zoltan
1 sibling, 0 replies; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-14 10:05 UTC (permalink / raw)
To: Nicholas Piggin
Cc: qemu-devel, qemu-ppc, clg, Greg Kurz, Daniel Henrique Barboza
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.
I'm not sure which part you suggest could be split out as a clean up? This
is all one change. Maybe I can split it like one patch inlining
gen_exception_err() then the rest in another patch but I don't see the
point as inlining is really trivial.
> 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 noticed that but I have no way to test it so I can add a patch but it
will be untested and you'll need to verify it won't break anything.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 06/10] target/ppc: Readability improvements in exception handlers
2023-06-14 3:42 ` Nicholas Piggin
@ 2023-06-14 10:07 ` BALATON Zoltan
2023-06-15 0:59 ` Nicholas Piggin
0 siblings, 1 reply; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-14 10:07 UTC (permalink / raw)
To: Nicholas Piggin
Cc: qemu-devel, qemu-ppc, clg, Greg Kurz, Daniel Henrique Barboza
On Wed, 14 Jun 2023, Nicholas Piggin wrote:
> On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
>> Improve readability by shortening some long comments, removing
>> comments that state the obvious and dropping some empty lines so they
>> don't distract when reading the code.
>
> Some changes are a matter of taste, but in the interest of having
> somebody do some spring cleaning of this code I don't want to nitpick
> it, so I won't :)
Thanks. I prefer functions to be less streched out vertically so more
lines fit in a single screen. Ideally the whole function should be visible
so it's easier to comprehend than having to scroll around but if there's
something really bothering you I can consider changing that. But if you
can accept this as it is then that's less work for me.
Regards,
BALATON Zoltan
> Acked-by: Nicholas Piggin <npiggin@gmail.com>
>
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> target/ppc/cpu.h | 1 +
>> target/ppc/excp_helper.c | 180 +++++++--------------------------------
>> 2 files changed, 33 insertions(+), 148 deletions(-)
>>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 04/10] target/ppc: Use env_cpu for cpu_abort in excp_helper
2023-06-14 3:36 ` Nicholas Piggin
@ 2023-06-14 10:13 ` BALATON Zoltan
2023-06-15 1:01 ` Nicholas Piggin
0 siblings, 1 reply; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-14 10:13 UTC (permalink / raw)
To: Nicholas Piggin
Cc: qemu-devel, qemu-ppc, clg, Greg Kurz, Daniel Henrique Barboza
On Wed, 14 Jun 2023, Nicholas Piggin wrote:
> On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
>> Use the env_cpu function to get the CPUState for cpu_abort. These are
>> only needed in case of fatal errors so this allows to avoid casting
>> and storing CPUState in a local variable wnen not needed.
>
> I don't entirely mind keeping cs around as a variable...
What for? This was only needed for error messages so most of the time
useless and having less locals make the function a bit simpler. If this is
needed for something else than errors later it could be added back but
then it will only be in the model specific function that needs it.
What I could change is to use CPU(cpu) instead of env_cpu(env) now that
I've dropped the rest of the series that also changed the func parameter
to get env instead of PowerPCCPU which may avoid some line breaks but not
sure if that's any better.
Regards,
BALATON Zoltan
> Thanks,
> Nick
>
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> target/ppc/excp_helper.c | 118 +++++++++++++++++++++------------------
>> 1 file changed, 63 insertions(+), 55 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index e4532f5088..51202f7028 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -422,7 +422,6 @@ static void powerpc_checkstop_state(CPUPPCState *env)
>>
>> static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>> {
>> - CPUState *cs = CPU(cpu);
>> CPUPPCState *env = &cpu->env;
>> target_ulong msr, new_msr, vector;
>> int srr0, srr1;
>
> [snip]
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 08/10] target/ppc: Fix gen_sc to use correct nip
2023-06-14 3:47 ` Nicholas Piggin
2023-06-14 10:05 ` BALATON Zoltan
@ 2023-06-14 21:27 ` BALATON Zoltan
2023-06-15 1:43 ` Nicholas Piggin
1 sibling, 1 reply; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-14 21:27 UTC (permalink / raw)
To: Nicholas Piggin
Cc: qemu-devel, qemu-ppc, clg, Greg Kurz, Daniel Henrique Barboza
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.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 09/10] target/ppc: Simplify syscall exception handlers
2023-06-14 4:18 ` Nicholas Piggin
@ 2023-06-14 21:33 ` BALATON Zoltan
2023-06-15 1:47 ` Nicholas Piggin
0 siblings, 1 reply; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-14 21:33 UTC (permalink / raw)
To: Nicholas Piggin
Cc: qemu-devel, qemu-ppc, clg, Greg Kurz, Daniel Henrique Barboza
On Wed, 14 Jun 2023, Nicholas Piggin wrote:
> On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
>> After previous changes the hypercall handling in 7xx and 74xx
>> exception handlers can be folded into one if statement to simpilfy
>> this code.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> target/ppc/excp_helper.c | 26 ++++++++++----------------
>> 1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 4f6a6dfb19..089417894e 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -738,26 +738,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
>> break;
>> case POWERPC_EXCP_SYSCALL: /* System call exception */
>> {
>> - int lev = env->error_code;
>> + PowerPCCPU *cpu = env_archcpu(env);
>
> I prefer to keep lev. Makes sense to combine the tests though
> I suppose. Although with powernv it's not really clear that we
> want to dump_syscall. dump_hcall is probably better (powernv
> could support a non-PAPR hypervisor with different hcall
> semantics, but also it could support an OS with different
> syscall semantics too). I guess that could be changed back
> when necessary though.
What do you mean changed back? This is not supposed to change when
dump_hcall and dump_syscall is called. However I've only changed the
powerpc_excp_7xx() and powerpc_excp_74xx() functions where this is only
present as a hack for VOF. I've left powerpc_excp_books() where hypercalls
really exist unchanged because that takes other bits into accound so
probably we can't combine the tests rhere.
Regards,
BALATON Zoltan
> Thanks,
> Nick
>
>>
>> - if (lev == 1 && cpu->vhyp) {
>> - dump_hcall(env);
>> - } else {
>> - dump_syscall(env);
>> - }
>> /*
>> * The Virtual Open Firmware (VOF) relies on the 'sc 1'
>> * instruction to communicate with QEMU. The pegasos2 machine
>> * uses VOF and the 7xx CPUs, so although the 7xx don't have
>> * HV mode, we need to keep hypercall support.
>> */
>> - if (lev == 1 && cpu->vhyp) {
>> + if (unlikely(env->error_code == 1 && cpu->vhyp)) {
>> PPCVirtualHypervisorClass *vhc =
>> PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
>> + dump_hcall(env);
>> vhc->hypercall(cpu->vhyp, cpu);
>> return;
>> + } else {
>> + dump_syscall(env);
>> }
>> -
>> break;
>> }
>> case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */
>> @@ -882,26 +879,23 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>> break;
>> case POWERPC_EXCP_SYSCALL: /* System call exception */
>> {
>> - int lev = env->error_code;
>> + PowerPCCPU *cpu = env_archcpu(env);
>>
>> - if (lev == 1 && cpu->vhyp) {
>> - dump_hcall(env);
>> - } else {
>> - dump_syscall(env);
>> - }
>> /*
>> * The Virtual Open Firmware (VOF) relies on the 'sc 1'
>> * instruction to communicate with QEMU. The pegasos2 machine
>> * uses VOF and the 74xx CPUs, so although the 74xx don't have
>> * HV mode, we need to keep hypercall support.
>> */
>> - if (lev == 1 && cpu->vhyp) {
>> + if (unlikely(env->error_code == 1 && cpu->vhyp)) {
>> PPCVirtualHypervisorClass *vhc =
>> PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
>> + dump_hcall(env);
>> vhc->hypercall(cpu->vhyp, cpu);
>> return;
>> + } else {
>> + dump_syscall(env);
>> }
>> -
>> break;
>> }
>> case POWERPC_EXCP_FPU: /* Floating-point unavailable exception */
>> --
>> 2.30.9
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 06/10] target/ppc: Readability improvements in exception handlers
2023-06-14 10:07 ` BALATON Zoltan
@ 2023-06-15 0:59 ` Nicholas Piggin
0 siblings, 0 replies; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-15 0:59 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, qemu-ppc, clg, Greg Kurz, Daniel Henrique Barboza
On Wed Jun 14, 2023 at 8:07 PM 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:
> >> Improve readability by shortening some long comments, removing
> >> comments that state the obvious and dropping some empty lines so they
> >> don't distract when reading the code.
> >
> > Some changes are a matter of taste, but in the interest of having
> > somebody do some spring cleaning of this code I don't want to nitpick
> > it, so I won't :)
>
> Thanks. I prefer functions to be less streched out vertically so more
> lines fit in a single screen. Ideally the whole function should be visible
> so it's easier to comprehend than having to scroll around but if there's
> something really bothering you I can consider changing that. But if you
> can accept this as it is then that's less work for me.
I don't mind vertical white space, actually like it. I don't like too
much stuff done per line.
Thanks,
Nick
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 04/10] target/ppc: Use env_cpu for cpu_abort in excp_helper
2023-06-14 10:13 ` BALATON Zoltan
@ 2023-06-15 1:01 ` Nicholas Piggin
0 siblings, 0 replies; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-15 1:01 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, qemu-ppc, clg, Greg Kurz, Daniel Henrique Barboza
On Wed Jun 14, 2023 at 8:13 PM 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:
> >> Use the env_cpu function to get the CPUState for cpu_abort. These are
> >> only needed in case of fatal errors so this allows to avoid casting
> >> and storing CPUState in a local variable wnen not needed.
> >
> > I don't entirely mind keeping cs around as a variable...
>
> What for? This was only needed for error messages so most of the time
> useless and having less locals make the function a bit simpler. If this is
Declaring cs, cpu, env, etc at the start of the function is very common
though so it doesn't take much thinking. You don't even need to look at
the declaration to know what it is.
Thanks,
Nick
> needed for something else than errors later it could be added back but
> then it will only be in the model specific function that needs it.
>
> What I could change is to use CPU(cpu) instead of env_cpu(env) now that
> I've dropped the rest of the series that also changed the func parameter
> to get env instead of PowerPCCPU which may avoid some line breaks but not
> sure if that's any better.
>
> Regards,
> BALATON Zoltan
>
> > Thanks,
> > Nick
> >
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >> target/ppc/excp_helper.c | 118 +++++++++++++++++++++------------------
> >> 1 file changed, 63 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> >> index e4532f5088..51202f7028 100644
> >> --- a/target/ppc/excp_helper.c
> >> +++ b/target/ppc/excp_helper.c
> >> @@ -422,7 +422,6 @@ static void powerpc_checkstop_state(CPUPPCState *env)
> >>
> >> static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
> >> {
> >> - CPUState *cs = CPU(cpu);
> >> CPUPPCState *env = &cpu->env;
> >> target_ulong msr, new_msr, vector;
> >> int srr0, srr1;
> >
> > [snip]
> >
> >
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 08/10] target/ppc: Fix gen_sc to use correct nip
2023-06-14 21:27 ` BALATON Zoltan
@ 2023-06-15 1:43 ` Nicholas Piggin
2023-06-15 23:02 ` BALATON Zoltan
0 siblings, 1 reply; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-15 1:43 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, qemu-ppc, clg, Greg Kurz, Daniel Henrique Barboza
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));
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 09/10] target/ppc: Simplify syscall exception handlers
2023-06-14 21:33 ` BALATON Zoltan
@ 2023-06-15 1:47 ` Nicholas Piggin
0 siblings, 0 replies; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-15 1:47 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, qemu-ppc, clg, Greg Kurz, Daniel Henrique Barboza
On Thu Jun 15, 2023 at 7:33 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:
> >> After previous changes the hypercall handling in 7xx and 74xx
> >> exception handlers can be folded into one if statement to simpilfy
> >> this code.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >> target/ppc/excp_helper.c | 26 ++++++++++----------------
> >> 1 file changed, 10 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> >> index 4f6a6dfb19..089417894e 100644
> >> --- a/target/ppc/excp_helper.c
> >> +++ b/target/ppc/excp_helper.c
> >> @@ -738,26 +738,23 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
> >> break;
> >> case POWERPC_EXCP_SYSCALL: /* System call exception */
> >> {
> >> - int lev = env->error_code;
> >> + PowerPCCPU *cpu = env_archcpu(env);
> >
> > I prefer to keep lev. Makes sense to combine the tests though
> > I suppose. Although with powernv it's not really clear that we
> > want to dump_syscall. dump_hcall is probably better (powernv
> > could support a non-PAPR hypervisor with different hcall
> > semantics, but also it could support an OS with different
> > syscall semantics too). I guess that could be changed back
> > when necessary though.
>
> What do you mean changed back? This is not supposed to change when
> dump_hcall and dump_syscall is called. However I've only changed the
> powerpc_excp_7xx() and powerpc_excp_74xx() functions where this is only
> present as a hack for VOF. I've left powerpc_excp_books() where hypercalls
> really exist unchanged because that takes other bits into accound so
> probably we can't combine the tests rhere.
Oh sorry I didn't notice it wasn't books.
Thanks,
Nick
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/10] target/ppc: Move common check in exception handlers to a function
2023-06-14 6:25 ` Cédric Le Goater
@ 2023-06-15 1:50 ` Nicholas Piggin
0 siblings, 0 replies; 34+ messages in thread
From: Nicholas Piggin @ 2023-06-15 1:50 UTC (permalink / raw)
To: Cédric Le Goater, BALATON Zoltan, qemu-devel, qemu-ppc
Cc: Greg Kurz, Daniel Henrique Barboza
On Wed Jun 14, 2023 at 4:25 PM AEST, Cédric Le Goater wrote:
> On 6/14/23 05:35, Nicholas Piggin wrote:
> > On Mon Jun 12, 2023 at 8:42 AM AEST, BALATON Zoltan wrote:
> >> All powerpc exception handlers share some code when handling machine
> >> check exceptions. Move this to a common function.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >
> > Hah, I just did very similar to improve some checkstop code (but I can
> > rebase my patches on yours).
>
> The mce injection was a nice addition. You sent that a while back for pseries
> and powernv. Are you going to revive the series ?
Yeah I do plan to.
Thanks,
Nick
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 08/10] target/ppc: Fix gen_sc to use correct nip
2023-06-15 1:43 ` Nicholas Piggin
@ 2023-06-15 23:02 ` BALATON Zoltan
0 siblings, 0 replies; 34+ messages in thread
From: BALATON Zoltan @ 2023-06-15 23:02 UTC (permalink / raw)
To: Nicholas Piggin
Cc: qemu-devel, qemu-ppc, clg, Greg Kurz, Daniel Henrique Barboza
On Thu, 15 Jun 2023, Nicholas Piggin wrote:
> 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));
I've added this to v3 but needs and SoB from you.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2023-06-15 23:05 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-11 22:42 [PATCH 00/10] Misc clean ups to target/ppc exception handling BALATON Zoltan
2023-06-11 22:42 ` [PATCH 01/10] target/ppc: Remove some superfluous parentheses BALATON Zoltan
2023-06-14 3:25 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 02/10] target/ppc: Remove unneeded parameter from powerpc_reset_wakeup() BALATON Zoltan
2023-06-14 3:26 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 03/10] target/ppc: Move common check in exception handlers to a function BALATON Zoltan
2023-06-12 9:28 ` Philippe Mathieu-Daudé
2023-06-12 10:07 ` BALATON Zoltan
2023-06-14 3:35 ` Nicholas Piggin
2023-06-14 6:25 ` Cédric Le Goater
2023-06-15 1:50 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 04/10] target/ppc: Use env_cpu for cpu_abort in excp_helper BALATON Zoltan
2023-06-14 3:36 ` Nicholas Piggin
2023-06-14 10:13 ` BALATON Zoltan
2023-06-15 1:01 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 05/10] target/ppc: Change parameter of cpu_interrupt_exittb() to an env pointer BALATON Zoltan
2023-06-11 22:42 ` [PATCH 06/10] target/ppc: Readability improvements in exception handlers BALATON Zoltan
2023-06-14 3:42 ` Nicholas Piggin
2023-06-14 10:07 ` BALATON Zoltan
2023-06-15 0:59 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 07/10] target/ppd: Remove unused define BALATON Zoltan
2023-06-12 9:29 ` Philippe Mathieu-Daudé
2023-06-14 3:42 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 08/10] target/ppc: Fix gen_sc to use correct nip BALATON Zoltan
2023-06-14 3:47 ` Nicholas Piggin
2023-06-14 10:05 ` BALATON Zoltan
2023-06-14 21:27 ` BALATON Zoltan
2023-06-15 1:43 ` Nicholas Piggin
2023-06-15 23:02 ` BALATON Zoltan
2023-06-11 22:42 ` [PATCH 09/10] target/ppc: Simplify syscall exception handlers BALATON Zoltan
2023-06-14 4:18 ` Nicholas Piggin
2023-06-14 21:33 ` BALATON Zoltan
2023-06-15 1:47 ` Nicholas Piggin
2023-06-11 22:42 ` [PATCH 10/10] target/ppc: Get CPUState in one step BALATON Zoltan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.