| Message ID | e7c1a61de1efdee932b71fada93dd58a3ce951cb.1686522199.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Misc clean ups to target/ppc exception handling | expand |
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); > + } > +}
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); >> + } >> +} > > >
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
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 >
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
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
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(-)