Re: [PATCH v3 1/9] target/ppc: Fix width of some 32-bit SPRs

["Nicholas Piggin" <npiggin@gmail.com>]

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