Re: [PATCH v4 8/9] target/riscv/kvm: read/write KVM regs via env size

[Daniel Henrique Barboza]

On 4/29/25 6:02 AM, Andrew Jones wrote:

On Mon, Apr 28, 2025 at 04:23:22PM -0300, Daniel Henrique Barboza wrote:

We're going to add support for scounteren in the next patch. KVM defines
as a target_ulong CSR, while QEMU defines env->scounteren as a 32 bit
field. This will cause the current code to read/write a 64 bit CSR in a
32 bit field when running in a 64 bit CPU.

To prevent that, change the current logic to honor the size of the QEMU
storage instead of the KVM CSR reg.

Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
Suggested-by: Andrew Jones <ajo...@ventanamicro.com>
---
  target/riscv/kvm/kvm-cpu.c | 14 +++++++++-----
  1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 5efee8adb2..53c34b43a2 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -135,6 +135,7 @@ typedef struct KVMCPUConfig {
      const char *description;
      target_ulong offset;
      uint64_t kvm_reg_id;
+    uint32_t prop_size;
      bool user_set;
      bool supported;
  } KVMCPUConfig;
@@ -237,6 +238,7 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, 
CPUState *cs)

#define KVM_CSR_CFG(_name, _env_prop, reg_id) \

      {.name = _name, .offset = ENV_CSR_OFFSET(_env_prop), \
+     .prop_size = sizeof(((CPURISCVState *)0)->_env_prop), \
       .kvm_reg_id = reg_id}

static KVMCPUConfig kvm_csr_cfgs[] = {

@@ -632,6 +634,7 @@ static int kvm_riscv_get_regs_csr(CPUState *cs)
  {
      RISCVCPU *cpu = RISCV_CPU(cs);
      uint64_t reg;
+    uint32_t reg32;

We don't need this variable.

      int i, ret;

for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {

@@ -646,9 +649,10 @@ static int kvm_riscv_get_regs_csr(CPUState *cs)
              return ret;
          }

- if (KVM_REG_SIZE(csr_cfg->kvm_reg_id) == sizeof(uint32_t)) {

-            kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
-        } else if (KVM_REG_SIZE(csr_cfg->kvm_reg_id) == sizeof(uint64_t)) {
+        if (csr_cfg->prop_size == sizeof(uint32_t)) {
+            reg32 = reg & 0xFFFF;

This is masking off too many bits. We want the lower 32, not just the
lower 16.

ooops ....

+            kvm_cpu_csr_set_u32(cpu, csr_cfg, reg32);

If the compiler warns when giving this function reg, then we can cast it.

  kvm_cpu_csr_set_u32(cpu, csr_cfg, (uint32_t)reg);

otherwise we don't need to do anything special.

I'll remove the extra var and let the compiler do its thing. I'll add a cast
for extra clarity since some toolchains might nag about it.

Daniel

+        } else if (csr_cfg->prop_size == sizeof(uint64_t)) {
              kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
          } else {
              g_assert_not_reached();
@@ -671,9 +675,9 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
              continue;
          }

- if (KVM_REG_SIZE(csr_cfg->kvm_reg_id) == sizeof(uint32_t)) {

+        if (csr_cfg->prop_size == sizeof(uint32_t)) {
              reg = kvm_cpu_csr_get_u32(cpu, csr_cfg);
-        } else if (KVM_REG_SIZE(csr_cfg->kvm_reg_id) == sizeof(uint64_t)) {
+        } else if (csr_cfg->prop_size == sizeof(uint64_t)) {
              reg = kvm_cpu_csr_get_u64(cpu, csr_cfg);
          } else {
              g_assert_not_reached();
--
2.49.0

Thanks,
drew