On Tue May 16, 2023 at 5:02 PM AEST, Mark Cave-Ayland wrote: > On 15/05/2023 16:19, Nicholas Piggin wrote: > > > On Mon May 15, 2023 at 10:03 PM AEST, Mark Cave-Ayland wrote: > >> On 15/05/2023 10:26, Nicholas Piggin wrote: > >> > >>> Some 32-bit SPRs are incorrectly implemented as 64-bits on 64-bit > >>> targets. > >>> > >>> This changes VRSAVE, DSISR, HDSISR, DAWRX0, PIDR, LPIDR, DEXCR, > >>> HDEXCR, CTRL, TSCR, MMCRH, and PMC[1-6] from to be 32-bit registers. > >>> > >>> This only goes by the 32/64 classification in the architecture, it > >>> does not try to implement finer details of SPR implementation (e.g., > >>> not all bits implemented as simple read/write storage). > >>> > >>> Signed-off-by: Nicholas Piggin <npig...@gmail.com> > >>> --- > >>> Since v2: no change. > >>> > >>> target/ppc/cpu_init.c | 18 +++++++++--------- > >>> target/ppc/helper_regs.c | 2 +- > >>> target/ppc/misc_helper.c | 4 ++-- > >>> target/ppc/power8-pmu.c | 2 +- > >>> target/ppc/translate.c | 2 +- > >>> 5 files changed, 14 insertions(+), 14 deletions(-) > >>> > >>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > >>> index 0ce2e3c91d..5aa0b3f0f1 100644 > >>> --- a/target/ppc/cpu_init.c > >>> +++ b/target/ppc/cpu_init.c > >>> @@ -5085,8 +5085,8 @@ static void > >>> register_book3s_altivec_sprs(CPUPPCState *env) > >>> } > >>> > >>> spr_register_kvm(env, SPR_VRSAVE, "VRSAVE", > >>> - &spr_read_generic, &spr_write_generic, > >>> - &spr_read_generic, &spr_write_generic, > >>> + &spr_read_generic, &spr_write_generic32, > >>> + &spr_read_generic, &spr_write_generic32, > >>> KVM_REG_PPC_VRSAVE, 0x00000000); > >>> > >>> } > >>> @@ -5120,7 +5120,7 @@ static void > >>> register_book3s_207_dbg_sprs(CPUPPCState *env) > >>> spr_register_kvm_hv(env, SPR_DAWRX0, "DAWRX0", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> - &spr_read_generic, &spr_write_generic, > >>> + &spr_read_generic, &spr_write_generic32, > >>> KVM_REG_PPC_DAWRX, 0x00000000); > >>> spr_register_kvm_hv(env, SPR_CIABR, "CIABR", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> @@ -5376,7 +5376,7 @@ static void register_book3s_ids_sprs(CPUPPCState > >>> *env) > >>> spr_register_hv(env, SPR_TSCR, "TSCR", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> - &spr_read_generic, &spr_write_generic, > >>> + &spr_read_generic, &spr_write_generic32, > >>> 0x00000000); > >>> spr_register_hv(env, SPR_HMER, "HMER", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> @@ -5406,7 +5406,7 @@ static void register_book3s_ids_sprs(CPUPPCState > >>> *env) > >>> spr_register_hv(env, SPR_MMCRC, "MMCRC", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> - &spr_read_generic, &spr_write_generic, > >>> + &spr_read_generic, &spr_write_generic32, > >>> 0x00000000); > >>> spr_register_hv(env, SPR_MMCRH, "MMCRH", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> @@ -5441,7 +5441,7 @@ static void register_book3s_ids_sprs(CPUPPCState > >>> *env) > >>> spr_register_hv(env, SPR_HDSISR, "HDSISR", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> - &spr_read_generic, &spr_write_generic, > >>> + &spr_read_generic, &spr_write_generic32, > >>> 0x00000000); > >>> spr_register_hv(env, SPR_HRMOR, "HRMOR", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> @@ -5665,7 +5665,7 @@ static void register_power7_book4_sprs(CPUPPCState > >>> *env) > >>> KVM_REG_PPC_ACOP, 0); > >>> spr_register_kvm(env, SPR_BOOKS_PID, "PID", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> - &spr_read_generic, &spr_write_generic, > >>> + &spr_read_generic, &spr_write_generic32, > >>> KVM_REG_PPC_PID, 0); > >>> #endif > >>> } > >>> @@ -5730,7 +5730,7 @@ static void register_power10_dexcr_sprs(CPUPPCState > >>> *env) > >>> { > >>> spr_register(env, SPR_DEXCR, "DEXCR", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> - &spr_read_generic, &spr_write_generic, > >>> + &spr_read_generic, &spr_write_generic32, > >>> 0); > >>> > >>> spr_register(env, SPR_UDEXCR, "DEXCR", > >>> @@ -5741,7 +5741,7 @@ static void register_power10_dexcr_sprs(CPUPPCState > >>> *env) > >>> spr_register_hv(env, SPR_HDEXCR, "HDEXCR", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> - &spr_read_generic, &spr_write_generic, > >>> + &spr_read_generic, &spr_write_generic32, > >>> 0); > >>> > >>> spr_register(env, SPR_UHDEXCR, "HDEXCR", > >>> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c > >>> index 779e7db513..fb351c303f 100644 > >>> --- a/target/ppc/helper_regs.c > >>> +++ b/target/ppc/helper_regs.c > >>> @@ -448,7 +448,7 @@ void register_non_embedded_sprs(CPUPPCState *env) > >>> /* Exception processing */ > >>> spr_register_kvm(env, SPR_DSISR, "DSISR", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> - &spr_read_generic, &spr_write_generic, > >>> + &spr_read_generic, &spr_write_generic32, > >>> KVM_REG_PPC_DSISR, 0x00000000); > >>> spr_register_kvm(env, SPR_DAR, "DAR", > >>> SPR_NOACCESS, SPR_NOACCESS, > >>> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c > >>> index a9bc1522e2..40ddc5c08c 100644 > >>> --- a/target/ppc/misc_helper.c > >>> +++ b/target/ppc/misc_helper.c > >>> @@ -190,13 +190,13 @@ void helper_store_dpdes(CPUPPCState *env, > >>> target_ulong val) > >>> > >>> void helper_store_pidr(CPUPPCState *env, target_ulong val) > >>> { > >>> - env->spr[SPR_BOOKS_PID] = val; > >>> + env->spr[SPR_BOOKS_PID] = (uint32_t)val; > >>> tlb_flush(env_cpu(env)); > >>> } > >>> > >>> void helper_store_lpidr(CPUPPCState *env, target_ulong val) > >>> { > >>> - env->spr[SPR_LPIDR] = val; > >>> + env->spr[SPR_LPIDR] = (uint32_t)val; > >>> > >>> /* > >>> * We need to flush the TLB on LPID changes as we only tag HV vs > >>> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c > >>> index 1381072b9e..64a64865d7 100644 > >>> --- a/target/ppc/power8-pmu.c > >>> +++ b/target/ppc/power8-pmu.c > >>> @@ -272,7 +272,7 @@ void helper_store_pmc(CPUPPCState *env, uint32_t > >>> sprn, uint64_t value) > >>> { > >>> pmu_update_cycles(env); > >>> > >>> - env->spr[sprn] = value; > >>> + env->spr[sprn] = (uint32_t)value; > >>> > >>> pmc_update_overflow_timer(env, sprn); > >>> } > >>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c > >>> index f603f1a939..c03a6bdc9a 100644 > >>> --- a/target/ppc/translate.c > >>> +++ b/target/ppc/translate.c > >>> @@ -413,7 +413,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, > >>> int gprn) > >>> > >>> void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn) > >>> { > >>> - spr_write_generic(ctx, sprn, gprn); > >>> + spr_write_generic32(ctx, sprn, gprn); > >>> > >>> /* > >>> * SPR_CTRL writes must force a new translation block, > >> > >> Just out of curiosity, is this the same as the problem described at [1] > >> for DECAR? > >> > >> > >> ATB, > >> > >> Mark. > >> > >> [1] https://lists.nongnu.org/archive/html/qemu-ppc/2023-03/msg00451.html > > > > Oh if it's a 64-bit target running in 32-bit mode, then the compiled > > code might use something like li reg,-1 to set the 0xffffffff value, > > but that gets sign extended to 64-bits. Storing that to DECAR then > > does cause it to get stored to DECR. So DECAR should use > > spr_write_generic32. > > > > But all the store_decr calculations are unsigned and DECR gets clamped > > to 32-bits, at least when reading it back. The problem seems to be the > > timer ends up getting set for a negative expire time. > > > > So storing to DECR directly seems like it would have the same problems > > as via DECAR. This should help. > > > > Thanks, > > Nick > > --- > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > > index 4e816c68c7..35a1410c4d 100644 > > --- a/hw/ppc/ppc.c > > +++ b/hw/ppc/ppc.c > > @@ -794,14 +794,18 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, > > uint64_t *nextp, > > CPUPPCState *env = &cpu->env; > > ppc_tb_t *tb_env = env->tb_env; > > uint64_t now, next; > > + uint64_t unsigned_value; > > + uint64_t unsigned_decr; > > int64_t signed_value; > > int64_t signed_decr; > > > > /* Truncate value to decr_width and sign extend for simplicity */ > > + unsigned_value = extract64(value, 0, nr_bits); > > + unsigned_decr = extract64(decr, 0, nr_bits); > > signed_value = sextract64(value, 0, nr_bits); > > signed_decr = sextract64(decr, 0, nr_bits); > > > > - trace_ppc_decr_store(nr_bits, decr, value); > > + trace_ppc_decr_store(nr_bits, unsigned_decr, unsigned_value); > > > > if (kvm_enabled()) { > > /* KVM handles decrementer exceptions, we don't need our own > > timer */ > > @@ -821,7 +825,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, > > uint64_t *nextp, > > * On MSB edge based DEC implementations the MSB going from 0 -> 1 > > triggers > > * an edge interrupt, so raise it here too. > > */ > > - if ((value < 3) || > > + if ((unsigned_value < 3) || > > ((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && signed_value < 0) > > || > > ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && signed_value < > > 0 > > && signed_decr >= 0)) { > > @@ -836,7 +840,8 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, > > uint64_t *nextp, > > > > /* Calculate the next timer event */ > > now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > - next = now + muldiv64(value, NANOSECONDS_PER_SECOND, > > tb_env->decr_freq); > > + next = now + muldiv64(unsigned_value, NANOSECONDS_PER_SECOND, > > + tb_env->decr_freq); > > *nextp = next; > > > > /* Adjust timer */ > > Thanks Nick! I've added the original reporter on CC to see if they can > provide > testing and feedback.
Oops, thanks. To be clear making DECAR a 32-bit register did also solve the problem I could reproduce by avoiding that sign extended GPR setting it to -1LL. But if the test case didn't use DECAR instead wrote -1 to DEC in the interrupt handler, I think the above patch is needed. So we should do both patches (assuming this fixes for sdicaro@). Thanks, Nick