| Message ID | 78ecd505a8b523e236cbeab335aa0621f7834cc5.1686776990.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Misc clean ups to target/ppc exception handling | expand |
On Thu Jun 15, 2023 at 7:34 AM AEST, BALATON Zoltan wrote: > Changing the parameter of cpu_interrupt_exittb() from CPUState to env > allows removing some more local CPUState variables in callers. I think it's more consistent to keep cs, which is same as cpu_interrupt(). Thanks, Nick
On Thu, 15 Jun 2023, Nicholas Piggin wrote: > On Thu Jun 15, 2023 at 7:34 AM AEST, BALATON Zoltan wrote: >> Changing the parameter of cpu_interrupt_exittb() from CPUState to env >> allows removing some more local CPUState variables in callers. > > I think it's more consistent to keep cs, which is same as > cpu_interrupt(). But with this patch it's more consistent with the other functions devlared in helper_regs.h and gets rid of the #ifdef in hreg_store_msr() so I'd still like to keep this patch. Callers already have env so it should not matter. Regards, BALATON Zoltan
On Thu Jun 15, 2023 at 7:19 PM AEST, BALATON Zoltan wrote: > On Thu, 15 Jun 2023, Nicholas Piggin wrote: > > On Thu Jun 15, 2023 at 7:34 AM AEST, BALATON Zoltan wrote: > >> Changing the parameter of cpu_interrupt_exittb() from CPUState to env > >> allows removing some more local CPUState variables in callers. > > > > I think it's more consistent to keep cs, which is same as > > cpu_interrupt(). > > But with this patch it's more consistent with the other functions devlared > in helper_regs.h and gets rid of the #ifdef in hreg_store_msr() so I'd > still like to keep this patch. Callers already have env so it should not > matter. Being consistent with functions of the same file is not important or really makes sense. It's important to be consistent with functions of similar type. cpu_interrupt_exittb() is a helper to call cpu_interrupt() so makes sense to be similar. At best it seems like pointless churn. Thanks, Nick
On Thu, 15 Jun 2023, Nicholas Piggin wrote: > On Thu Jun 15, 2023 at 7:19 PM AEST, BALATON Zoltan wrote: >> On Thu, 15 Jun 2023, Nicholas Piggin wrote: >>> On Thu Jun 15, 2023 at 7:34 AM AEST, BALATON Zoltan wrote: >>>> Changing the parameter of cpu_interrupt_exittb() from CPUState to env >>>> allows removing some more local CPUState variables in callers. >>> >>> I think it's more consistent to keep cs, which is same as >>> cpu_interrupt(). >> >> But with this patch it's more consistent with the other functions devlared >> in helper_regs.h and gets rid of the #ifdef in hreg_store_msr() so I'd >> still like to keep this patch. Callers already have env so it should not >> matter. > > Being consistent with functions of the same file is not important or > really makes sense. It's important to be consistent with functions > of similar type. cpu_interrupt_exittb() is a helper to call > cpu_interrupt() so makes sense to be similar. At best it seems like > pointless churn. OK I've revised it in v3 and dropped most of this patch. Regards, BALATON Zoltan
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 122e2a6e41..49ed3e1825 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -419,7 +419,7 @@ static void powerpc_mcheck_checkstop(CPUPPCState *env) "Entering checkstop state\n"); } cs->halted = 1; - cpu_interrupt_exittb(cs); + cpu_interrupt_exittb(env); } static void powerpc_excp_40x(PowerPCCPU *cpu, int excp) @@ -2551,8 +2551,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); } } @@ -2589,8 +2588,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); @@ -2614,7 +2611,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
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(-)