[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs
|
From: |
Nicholas Piggin |
|
Subject: |
Re: [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs |
|
Date: |
Mon, 15 May 2023 22:51:31 +1000 |
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 <npiggin@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
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
[PATCH v3 2/9] target/ppc: Fix PMU MMCR0[PMCjCE] bit in hflags calculation, Nicholas Piggin, 2023/05/15
[PATCH v3 3/9] target/ppc: Fix instruction loading endianness in alignment interrupt, Nicholas Piggin, 2023/05/15
[PATCH v3 4/9] target/ppc: Alignment faults do not set DSISR in ISA v3.0 onward, Nicholas Piggin, 2023/05/15