Re: [PATCH v2 04/33] target/riscv: Bugfix riscv_pmu_ctr_get_fixed_counters_val()

[Alistair Francis <alistair23@gmail.com>]

On Wed, Oct 15, 2025 at 7:55 PM Anton Johansson <anjo@rev.ng> wrote:
>
> On 15/10/25, Alistair Francis wrote:
> > On Tue, Oct 7, 2025 at 9:06 PM Anton Johansson <anjo@rev.ng> wrote:
> > >
> > > On 03/10/25, Alistair Francis wrote:
> > > > On Wed, Oct 1, 2025 at 5:43 PM Anton Johansson via
> > > > <qemu-devel@nongnu.org> wrote:
> > > > >
> > > > > From my understanding the upper_half argument only indicates whether the
> > > > > upper or lower 32 bits should be returned, and upper_half will only ever
> > > > > be set when MXLEN == 32.  However, the function also uses upper_half to
> > > > > determine whether the inhibit flags are located in mcyclecfgh or
> > > > > mcyclecfg, but this misses the case where MXLEN == 32, upper_half == false
> > > > > for TARGET_RISCV32 where we would also need to read the upper half field.
> > > >
> > > > If MXLEN == 32, upper_half == false then we want to read mcyclecfg,
> > > > which the code today seems to be doing correctly.
> > >
> > > Hi again, I might be missing something then, when would this function need
> > > to access mcyclecfg for MXLEN == 32?  AFAIU mcyclecfg and mcyclecfgh are
> > > modeled separately for MXLEN == 32, even when sizeof(target_ulong) == 8.
> > > Since this function only checks inhibit flags wouldn't we always want to
> > > access mcyclecfgh for MXLEN == 32?
> >
> > When MXLEN == 32 mcyclecfg is the bottom 32-bits of the mcyclecfg CSR
> > and mcyclecfgh is the top 32-bits of the CSR. The idea is that
> > target_ulong will be 32-bits (sizeof(target_ulong) == 4). It doesn't
> > really matter if target_ulong is 64-bits though, as the registers
> > should just be treated as 32-bit registers anyway.
>
> Appreciate the explanation, this makes sense to me.  But the function
> only uses cfg_val to check inhibit flags in the top 32 bits, so accessing the
> lower 32 bits when upper_half == false and MXLEN == 32 would be incorrect
> then?

Well a guest can still access the lower 32-bits and although there is
nothing there now there might be in the future.

>
> Your comment below is what's tripping me up, since the behaviour of
> accesing the lower 32 bits for MXLEN == 32 is not retained when
> mcyclecfgh and mcyclecfg are merged to a single 64 bit field

I don't follow what you mean here

Alistair

>
> > >      if (counter_idx == 0) {
> > > -        cfg_val = upper_half ? ((uint64_t)env->mcyclecfgh << 32) :
> > > +        cfg_val = rv32 ? ((uint64_t)env->mcyclecfgh << 32) :
> >
> > This doesn't look right.
> >
> > RV32 will want to access both mcyclecfgh and mcyclecfg, but this
> > change restricts access to mcyclecfg as rv32 will always be true.
> >
> > I don't think there is anything wrong with the current code.
>
> //Anton