diff mbox series

[3/4] target/ppc: Move common check in machne check handlers to a function

Message ID 20230623081953.290875-4-npiggin@gmail.com
State New
Headers show
Series target/ppc: Catch invalid real address accesses | expand

Commit Message

Nicholas Piggin June 23, 2023, 8:19 a.m. UTC 
From: BALATON Zoltan <balaton@eik.bme.hu>

All powerpc exception handlers share some code when handling machine
check exceptions. Move this to a common function.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
[np: tweak subject, rename function to powerpc_mcheck_test_and_checkstop]
Signed-off-by-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/excp_helper.c | 114 +++++++++------------------------------
 1 file changed, 25 insertions(+), 89 deletions(-)

Comments

Fabiano Rosas June 23, 2023, 1:20 p.m. UTC | #1 
Nicholas Piggin <npiggin@gmail.com> writes:

> From: BALATON Zoltan <balaton@eik.bme.hu>
>
> All powerpc exception handlers share some code when handling machine
> check exceptions. Move this to a common function.
>

Maybe Machine Check is simple enough, but this kind of sharing of code
has historically caused pain when people want to change something for
the modern cpus and end up affecting the old cpus by mistake.

There is also the inverse scenario where someone has access to the old
HW and just want to make an one-off contribution, but the community gets
insecure about it because it could also affect the new cpus.

Then comes the obvious "solution" which is to bring in an artificial
identifier (excp. model) to be able to have conditional code inside the
common function. And that causes problems because no one really knows
how it maps to actual hardware/ISA.

No objection, just a little cautionary tale. =)
BALATON Zoltan June 23, 2023, 4:16 p.m. UTC | #2 
On Fri, 23 Jun 2023, Fabiano Rosas wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
>> From: BALATON Zoltan <balaton@eik.bme.hu>
>>
>> All powerpc exception handlers share some code when handling machine
>> check exceptions. Move this to a common function.
>>
>
> Maybe Machine Check is simple enough, but this kind of sharing of code
> has historically caused pain when people want to change something for
> the modern cpus and end up affecting the old cpus by mistake.
>
> There is also the inverse scenario where someone has access to the old
> HW and just want to make an one-off contribution, but the community gets
> insecure about it because it could also affect the new cpus.

It's a trade off between making these independent and avoiding code 
duplication. I think if we have common things that are the same between 
different CPU models then we should not duplicate those, otherwise they 
can diverge when a problem is fixed in one copy but not in the others. We 
just have to remember that if in the future a new CPU defines these 
differently then we mayy need to add another function implementing that 
for the new CPU but the old CPUs can still share the same code without 
duplicating it. So the question is if this machine check is something that 
is the same across different CPU models or do we ever need to model this 
differently for different CPU models? At least the current implementation 
had the same code duplicated everywhere which this patch resolves.

Regards,
BALATON Zoltan

> Then comes the obvious "solution" which is to bring in an artificial
> identifier (excp. model) to be able to have conditional code inside the
> common function. And that causes problems because no one really knows
> how it maps to actual hardware/ISA.
>
> No objection, just a little cautionary tale. =)
Nicholas Piggin June 25, 2023, 9:20 a.m. UTC | #3 
On Fri Jun 23, 2023 at 11:20 PM AEST, Fabiano Rosas wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
> > From: BALATON Zoltan <balaton@eik.bme.hu>
> >
> > All powerpc exception handlers share some code when handling machine
> > check exceptions. Move this to a common function.
> >
>
> Maybe Machine Check is simple enough, but this kind of sharing of code
> has historically caused pain when people want to change something for
> the modern cpus and end up affecting the old cpus by mistake.
>
> There is also the inverse scenario where someone has access to the old
> HW and just want to make an one-off contribution, but the community gets
> insecure about it because it could also affect the new cpus.
>
> Then comes the obvious "solution" which is to bring in an artificial
> identifier (excp. model) to be able to have conditional code inside the
> common function. And that causes problems because no one really knows
> how it maps to actual hardware/ISA.
>
> No objection, just a little cautionary tale. =)

Thanks Fabiano, good point. I know you spent a lot of work on untangling
this mess. I'll think a bit more about it. Seems we need to at least make
a few fixes first before we can turn this on for upstream.

Thanks,
Nick
diff mbox series

Patch

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 1c26828d8b..4bfcfc3c3d 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -404,6 +404,25 @@  static void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong vector,
     env->reserve_addr = -1;
 }
 
+static void powerpc_mcheck_test_and_checkstop(CPUPPCState *env)
+{
+    CPUState *cs = env_cpu(env);
+
+    if (FIELD_EX64(env->msr, MSR, ME)) {
+        return;
+    }
+
+    /* 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);
@@ -446,21 +465,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_mcheck_test_and_checkstop(env);
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
 
@@ -577,21 +582,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_mcheck_test_and_checkstop(env);
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
 
@@ -750,21 +741,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_mcheck_test_and_checkstop(env);
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
 
@@ -935,21 +912,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_mcheck_test_and_checkstop(env);
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
 
@@ -1130,21 +1093,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_mcheck_test_and_checkstop(env);
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
 
@@ -1377,20 +1326,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_mcheck_test_and_checkstop(env);
         if (env->msr_mask & MSR_HVB) {
             /*
              * ISA specifies HV, but can be delivered to guest with HV