On 16/10/25, Alistair Francis wrote:
> On Wed, Oct 15, 2025 at 7:55 PM Anton Johansson <[email protected]> wrote:
> >
> > 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?
>
> 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
On upstream when riscv_pmu_ctr_get_fixed_counters_val() is called with
MXLEN == 32 and upper_half == false, we read the inhibit flags from the
lower 32 bits (mcyclecfg) and check against bit 62 (MINH) etc. thus
always returning false
if (counter_idx == 0) {
cfg_val = upper_half ? ((uint64_t)env->mcyclecfgh << 32) :
env->mcyclecfg;
}
[...]
if (!(cfg_val & MCYCLECFG_BIT_MINH)) {
curr_val += counter_arr[PRV_M];
}
if this behaviour is intended, we do not retain it with this patch as
we always access the higher cfg bits to check for MINH. Similarly when
merging mcyclecfgh and mcyclecfg
if (counter_idx == 0) {
cfg_val = env->mcyclecfg;
}
[...]
if (!(cfg_val & MCYCLECFG_BIT_MINH)) {
curr_val += counter_arr[PRV_M];
}
we only check for MINH in the higher bits.
//Anton
