| Message ID | 20230515092655.171206-2-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: Assorted ppc target fixes | expand |
On 5/15/23 14:56, 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); > > } This change broke linux-user build, could you please check once? [1776/2718] Compiling C object libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu_init.c.o FAILED: libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu_init.c.o gcc -m64 -mcx16 -Ilibqemu-ppc64le-linux-user.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -I../common-user/host/x86_64 -I../linux-user/include/host/x86_64 -I../linux-user/include -Ilinux-user -I../linux-user -Ilinux-user/ppc -I../linux-user/ppc -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /home/travis/build/Harsh-Prateek-Bora/qemu/linux-headers -isystem linux-headers -iquote . -iquote /home/travis/build/Harsh-Prateek-Bora/qemu -iquote /home/travis/build/Harsh-Prateek-Bora/qemu/include -iquote /home/travis/build/Harsh-Prateek-Bora/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc64le-linux-user-config-target.h"' '-DCONFIG_DEVICES="ppc64le-linux-user-config-devices.h"' -MD -MQ libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu_init.c.o -MF libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu_init.c.o.d -o libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu_init.c.o -c ../target/ppc/cpu_init.c In file included from ../target/ppc/cpu_init.c:46: ../target/ppc/cpu_init.c: In function ‘register_book3s_altivec_sprs’: ../target/ppc/cpu_init.c:5089:42: error: ‘spr_write_generic32’ undeclared (first use in this function); did you mean ‘spr_write_generic’? 5089 | &spr_read_generic, &spr_write_generic32, | ^~~~~~~~~~~~~~~~~~~ ../target/ppc/spr_common.h:25:24: note: in definition of macro ‘USR_ARG’ 25 | # define USR_ARG(X) X, | ^ ../target/ppc/spr_common.h:66:5: note: in expansion of macro ‘spr_register_kvm_hv’ 66 | spr_register_kvm_hv(env, num, name, uea_read, uea_write, oea_read, \ | ^~~~~~~~~~~~~~~~~~~ ../target/ppc/cpu_init.c:5088:5: note: in expansion of macro ‘spr_register_kvm’ 5088 | spr_register_kvm(env, SPR_VRSAVE, "VRSAVE", | ^~~~~~~~~~~~~~~~ ../target/ppc/cpu_init.c:5089:42: note: each undeclared identifier is reported only once for each function it appears in 5089 | &spr_read_generic, &spr_write_generic32, | ^~~~~~~~~~~~~~~~~~~ ../target/ppc/spr_common.h:25:24: note: in definition of macro ‘USR_ARG’ 25 | # define USR_ARG(X) X, | ^ ../target/ppc/spr_common.h:66:5: note: in expansion of macro ‘spr_register_kvm_hv’ 66 | spr_register_kvm_hv(env, num, name, uea_read, uea_write, oea_read, \ | ^~~~~~~~~~~~~~~~~~~ ../target/ppc/cpu_init.c:5088:5: note: in expansion of macro ‘spr_register_kvm’ 5088 | spr_register_kvm(env, SPR_VRSAVE, "VRSAVE", | ^~~~~~~~~~~~~~~~ [1777/2718] Compiling C object libqemu-ppc64le-linux-user.fa.p/target_ppc_cpu-models.c.o [1778/2718] Compiling C object libqemu-ppc64le-linux-user.fa.p/target_ppc_fpu_helper.c.o ninja: build stopped: subcommand failed. make: *** [Makefile:165: run-ninja] Error 1 regards, Harsh > @@ -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,
On Mon May 15, 2023 at 8:14 PM AEST, Harsh Prateek Bora wrote: > > > On 5/15/23 14:56, 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); > > > > } > > This change broke linux-user build, could you please check once? Sorry I did notice you reported that already, must have lost it along the way somewhere. This incremental patch should work? Thanks, Nick --- diff --git a/target/ppc/spr_common.h b/target/ppc/spr_common.h index 8437eb0340..4c0f2bed77 100644 --- a/target/ppc/spr_common.h +++ b/target/ppc/spr_common.h @@ -81,6 +81,7 @@ void _spr_register(CPUPPCState *env, int num, const char *name, void spr_noaccess(DisasContext *ctx, int gprn, int sprn); void spr_read_generic(DisasContext *ctx, int gprn, int sprn); void spr_write_generic(DisasContext *ctx, int sprn, int gprn); +void spr_write_generic32(DisasContext *ctx, int sprn, int gprn); void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn); void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn); void spr_write_PMC(DisasContext *ctx, int sprn, int gprn); @@ -109,7 +110,6 @@ void spr_write_PMC14_ureg(DisasContext *ctx, int sprn, int gprn); void spr_write_PMC56_ureg(DisasContext *ctx, int sprn, int gprn); #ifndef CONFIG_USER_ONLY -void spr_write_generic32(DisasContext *ctx, int sprn, int gprn); void spr_write_clear(DisasContext *ctx, int sprn, int gprn); void spr_access_nop(DisasContext *ctx, int sprn, int gprn); void spr_read_decr(DisasContext *ctx, int gprn, int sprn); diff --git a/target/ppc/translate.c b/target/ppc/translate.c index c03a6bdc9a..f5cf1457db 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -411,6 +411,18 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn) spr_store_dump_spr(sprn); } +void spr_write_generic32(DisasContext *ctx, int sprn, int gprn) +{ +#ifdef TARGET_PPC64 + TCGv t0 = tcg_temp_new(); + tcg_gen_ext32u_tl(t0, cpu_gpr[gprn]); + gen_store_spr(sprn, t0); + spr_store_dump_spr(sprn); +#else + spr_write_generic(ctx, sprn, gprn); +#endif +} + void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn) { spr_write_generic32(ctx, sprn, gprn); @@ -424,18 +436,6 @@ void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn) } #if !defined(CONFIG_USER_ONLY) -void spr_write_generic32(DisasContext *ctx, int sprn, int gprn) -{ -#ifdef TARGET_PPC64 - TCGv t0 = tcg_temp_new(); - tcg_gen_ext32u_tl(t0, cpu_gpr[gprn]); - gen_store_spr(sprn, t0); - spr_store_dump_spr(sprn); -#else - spr_write_generic(ctx, sprn, gprn); -#endif -} - void spr_write_clear(DisasContext *ctx, int sprn, int gprn) { TCGv t0 = tcg_temp_new();
On 5/15/23 16:44, Nicholas Piggin wrote: > On Mon May 15, 2023 at 8:14 PM AEST, Harsh Prateek Bora wrote: >> >> >> On 5/15/23 14:56, 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); >>> >>> } >> >> This change broke linux-user build, could you please check once? > > Sorry I did notice you reported that already, must have lost it > along the way somewhere. This incremental patch should work? > > Thanks, > Nick > > --- > diff --git a/target/ppc/spr_common.h b/target/ppc/spr_common.h > index 8437eb0340..4c0f2bed77 100644 > --- a/target/ppc/spr_common.h > +++ b/target/ppc/spr_common.h > @@ -81,6 +81,7 @@ void _spr_register(CPUPPCState *env, int num, const char *name, > void spr_noaccess(DisasContext *ctx, int gprn, int sprn); > void spr_read_generic(DisasContext *ctx, int gprn, int sprn); > void spr_write_generic(DisasContext *ctx, int sprn, int gprn); > +void spr_write_generic32(DisasContext *ctx, int sprn, int gprn); > void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn); > void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn); > void spr_write_PMC(DisasContext *ctx, int sprn, int gprn); > @@ -109,7 +110,6 @@ void spr_write_PMC14_ureg(DisasContext *ctx, int sprn, int gprn); > void spr_write_PMC56_ureg(DisasContext *ctx, int sprn, int gprn); > > #ifndef CONFIG_USER_ONLY > -void spr_write_generic32(DisasContext *ctx, int sprn, int gprn); > void spr_write_clear(DisasContext *ctx, int sprn, int gprn); > void spr_access_nop(DisasContext *ctx, int sprn, int gprn); > void spr_read_decr(DisasContext *ctx, int gprn, int sprn); > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index c03a6bdc9a..f5cf1457db 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -411,6 +411,18 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn) > spr_store_dump_spr(sprn); > } > > +void spr_write_generic32(DisasContext *ctx, int sprn, int gprn) > +{ > +#ifdef TARGET_PPC64 > + TCGv t0 = tcg_temp_new(); > + tcg_gen_ext32u_tl(t0, cpu_gpr[gprn]); > + gen_store_spr(sprn, t0); > + spr_store_dump_spr(sprn); > +#else > + spr_write_generic(ctx, sprn, gprn); > +#endif > +} > + > void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn) > { > spr_write_generic32(ctx, sprn, gprn); > @@ -424,18 +436,6 @@ void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn) > } > > #if !defined(CONFIG_USER_ONLY) > -void spr_write_generic32(DisasContext *ctx, int sprn, int gprn) > -{ > -#ifdef TARGET_PPC64 > - TCGv t0 = tcg_temp_new(); > - tcg_gen_ext32u_tl(t0, cpu_gpr[gprn]); > - gen_store_spr(sprn, t0); > - spr_store_dump_spr(sprn); > -#else > - spr_write_generic(ctx, sprn, gprn); > -#endif > -} > - > void spr_write_clear(DisasContext *ctx, int sprn, int gprn) > { > TCGv t0 = tcg_temp_new(); thanks, this resolves the build break, please squash it in. Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
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
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
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 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 */
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 <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 > > 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. ATB, Mark.
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 <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 > > > > 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
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,
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(-)