On 15/10/25, Alistair Francis wrote:
> On Tue, Oct 7, 2025 at 9:06 PM Anton Johansson <[email protected]> wrote:
> >
> > On 03/10/25, Alistair Francis wrote:
> > > On Wed, Oct 1, 2025 at 5:43 PM Anton Johansson via
> > > <[email protected]> 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?
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
> > 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
