Re: [PATCH v3 8/9] target/riscv: widen scounteren to target_ulong

[Daniel Henrique Barboza]

On 4/27/25 2:59 AM, Andrew Jones wrote:

On Fri, Apr 25, 2025 at 01:02:02PM -0300, Daniel Henrique Barboza wrote:

We want to support scounteren as a KVM CSR. The KVM UAPI defines every
CSR size as target_ulong, and our env->scounteren is fixed at 32 bits.

The other existing cases where the property size does not match the KVM
reg size happens with uint64_t properties, like 'mstatus'. When running
a 32 bit CPU we'll write a 32 bit 'sstatus' KVM reg into the 64 bit
'mstatus' field. As long as we're consistent, i.e. we're always
reading/writing the same words, this is ok.

For scounteren, a KVM guest running in a 64 bit CPU will end up writing
a 64 bit reg in a 32 bit field. This will have all sort of funny side
effects in the KVM guest that we would rather avoid.

Increase scounteren to target_ulong to allow KVM to read/write the
scounteren CSR without any surprises.

Aside from bumping the version of the RISCVCPU vmstate no other
behavioral changes are expected.

Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
---
  target/riscv/cpu.h     | 9 ++++++++-
  target/riscv/machine.c | 6 +++---
  2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index f5a60d0c52..66d4ddfcb4 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -400,7 +400,14 @@ struct CPUArchState {
       */
      bool two_stage_indirect_lookup;

- uint32_t scounteren;

+    /*
+     * scounteren is supposed to be an uint32_t, as the spec
+     * says. We're using a target_ulong instead because the
+     * scounteren KVM CSR is defined as target_ulong in
+     * kvm_riscv_csr, and we want to avoid having to deal
+     * with an ulong reg being read/written in an uint32_t.
+     */
+    target_ulong scounteren;

I'm having second thoughts about this. It seems like it should be
avoidable with the use of an intermediary buffer (which we already
have -- the uint64_t reg) and with tracking the size of the env state
by capturing the size with the new macro used to build the array.

scounteren is an uint32_t field so I'm not sure why we need to track it.

Then,

for reading:
  1. read the kvm reg into a buffer of the size kvm says it is
  2. only write the bytes we can store from the buffer into the env state,
     using the size field to know how many that is

for writing:
  1. put the env state into a buffer of the size kvm says the register is,
     ensuring any upper unused bytes of the buffer are zero
  2. write the buffer to kvm

So in short we would just read/write the lower 32 bits of scounteren, making it
a 32 bit CSR as far as QEMU is concerned. Given that it is a 32 bit CSR in the
RISC-V ISA too I guess it's safe for QEMU to do this assumption.


Thanks,

Daniel

Thanks,
drew

      uint32_t mcounteren;

uint32_t scountinhibit;

diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index df2d5bad8d..f3477e153b 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -401,8 +401,8 @@ static const VMStateDescription vmstate_ssp = {

const VMStateDescription vmstate_riscv_cpu = {

      .name = "cpu",
-    .version_id = 10,
-    .minimum_version_id = 10,
+    .version_id = 11,
+    .minimum_version_id = 11,
      .post_load = riscv_cpu_post_load,
      .fields = (const VMStateField[]) {
          VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
@@ -445,7 +445,7 @@ const VMStateDescription vmstate_riscv_cpu = {
          VMSTATE_UINTTL(env.mtval, RISCVCPU),
          VMSTATE_UINTTL(env.miselect, RISCVCPU),
          VMSTATE_UINTTL(env.siselect, RISCVCPU),
-        VMSTATE_UINT32(env.scounteren, RISCVCPU),
+        VMSTATE_UINTTL(env.scounteren, RISCVCPU),
          VMSTATE_UINT32(env.mcounteren, RISCVCPU),
          VMSTATE_UINT32(env.scountinhibit, RISCVCPU),
          VMSTATE_UINT32(env.mcountinhibit, RISCVCPU),
--
2.49.0