On Thu, Oct 16, 2025 at 4:16 AM Pierrick Bouvier <[email protected]> wrote: > > On 10/14/25 1:34 PM, Anton Johansson 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
Correct > > 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. Ok, I think I understand the confusion here. It's a mistake that we have two registers in QEMU (mcyclecfgh/mcyclecfg for example). It should be one single 64-bit mcyclecfg register. For RV32 (MXLEN == 32) the guest can read the top or the bottom 32-bits, depending on the operation. So it is valid for MXLEN == 32 and upper_half == false (although in this case there is nothing actually stored there, but that's not our problem). The fact that we have split the * and *h registers makes this hard to read and understand, but MXLEN == 32 and upper_half == false is a valid use case. > > > > Minor simplifications are also made along with some formatting fixes. > > > > Signed-off-by: Anton Johansson <[email protected]> > > > > --- > > NOTE: I've not included any reviewed-bys or modified this patch as it's > > still unclear to me whether this change is correct or not. Alistair > > mentioned that this can get called for MXLEN == 32 and upper_half == > > false, meaning the lower field would be accessed. I'm sure I'm missing Correct, riscv_pmu_write_ctr() does this if the guest wants to read the lower 32-bits. Alistair > > something but this is still not clear to me, it seems to me like we > > always want to access the upper half for MXLEN == 32 since that's were > > the inhibit flags are stored. > > In case there is a doubt, having an assert for this situation would make > it future proof. > g_assert(!(rv32 && !upper_half)); > > As well, maybe Alistair can point a call site where this combination is > possible. > > > --- > > target/riscv/csr.c | 22 ++++++++++------------ > > 1 file changed, 10 insertions(+), 12 deletions(-) > > >
