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 <[email protected]> > > --- > > 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
Hmm, good thinking. But I'm not sure. DECAR is 32-bits so it should probably use spr_write_generic32 (I didn't convert any BookE specific regs). But the test sets 32-bit value for DECAR so I'm not sure if that would be the issue. That said I don't see where the value is getting sign extended. Thanks, Nick
