[PATCH 0/7] target/riscv/kvm: CSR related fixes

[PATCH 0/7] target/riscv/kvm: CSR related fixes

[Daniel Henrique Barboza]

Hi,

This series contains fixes to address a problem reported by Andrea in
[1]. We're now handling CSRs like regular KVM extensions, i.e. we'll
verify if the CSR is available before attempting to read/write it.

A considerable amount of boilerplate had to be added, but as a reward
we've automated the process to a point where new CSRs can be added by
just adding them in an static array, and everything else is taken care
of.

Patch 6 re-introduces scounteren and senvcfg KVM CSRs, the patch that
we've reverted during the 10.0 cycle. We're ready to deal with older
kernels that don't know about them, so reintroduce it.

Patch 7 is a fix for an issue I've found with scounteren during testing.
It is a kernel issue at heart but QEMU must be able to deal with it due
to older kernels in the wild.



Andrea, if you could take these patches and test it with your setup that
would be terrific.



Patches based on alistair/riscv-to-apply.next.


[1] https://lore.kernel.org/qemu-riscv/CABJz62OfUDHYkQ0T3rGHStQprf1c7_E0qBLbLKhfv=+jb0SYAw@mail.gmail.com/

Daniel Henrique Barboza (7):
  target/riscv/kvm: minor fixes/tweaks
  target/riscv/kvm: turn u32/u64 reg functions in macros
  target/riscv/kvm: turn kvm_riscv_reg_id_ulong() in a macro
  target/riscv/kvm: add kvm_csr_cfgs[]
  target/riscv/kvm: do not read unavailable CSRs
  target/riscv/kvm: add missing KVM CSRs
  target/riscv/kvm: reset 'scounteren' with host val

 target/riscv/cpu.h         |   1 +
 target/riscv/kvm/kvm-cpu.c | 366 +++++++++++++++++++++++++------------
 2 files changed, 247 insertions(+), 120 deletions(-)

-- 
2.49.0

[PATCH 1/7] target/riscv/kvm: minor fixes/tweaks

[Daniel Henrique Barboza]

Remove an unused 'KVMScratchCPU' pointer argument in
kvm_riscv_check_sbi_dbcn_support().

Put kvm_riscv_reset_regs_csr() after kvm_riscv_put_regs_csr(). This will
make a future patch diff easier to read, when changes in
kvm_riscv_reset_regs_csr() and kvm_riscv_get_regs_csr() will be made.

Fixes: a6b53378f5 ("target/riscv/kvm: implement SBI debug console (DBCN) calls")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 0f4997a918..afe3d3e609 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -613,19 +613,6 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
     return ret;
 }
 
-static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
-{
-    env->mstatus = 0;
-    env->mie = 0;
-    env->stvec = 0;
-    env->sscratch = 0;
-    env->sepc = 0;
-    env->scause = 0;
-    env->stval = 0;
-    env->mip = 0;
-    env->satp = 0;
-}
-
 static int kvm_riscv_get_regs_csr(CPUState *cs)
 {
     CPURISCVState *env = &RISCV_CPU(cs)->env;
@@ -660,6 +647,19 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
     return 0;
 }
 
+static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
+{
+    env->mstatus = 0;
+    env->mie = 0;
+    env->stvec = 0;
+    env->sscratch = 0;
+    env->sepc = 0;
+    env->scause = 0;
+    env->stval = 0;
+    env->mip = 0;
+    env->satp = 0;
+}
+
 static int kvm_riscv_get_regs_fp(CPUState *cs)
 {
     int ret = 0;
@@ -1078,7 +1078,6 @@ static int uint64_cmp(const void *a, const void *b)
 }
 
 static void kvm_riscv_check_sbi_dbcn_support(RISCVCPU *cpu,
-                                             KVMScratchCPU *kvmcpu,
                                              struct kvm_reg_list *reglist)
 {
     struct kvm_reg_list *reg_search;
@@ -1197,7 +1196,7 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
         kvm_riscv_read_vlenb(cpu, kvmcpu, reglist);
     }
 
-    kvm_riscv_check_sbi_dbcn_support(cpu, kvmcpu, reglist);
+    kvm_riscv_check_sbi_dbcn_support(cpu, reglist);
 }
 
 static void riscv_init_kvm_registers(Object *cpu_obj)
-- 
2.49.0

[PATCH 2/7] target/riscv/kvm: turn u32/u64 reg functions in macros

[Daniel Henrique Barboza]

This change is motivated by a future change w.r.t CSRs management. We
want to handle them the same way as KVM extensions, i.e. a static array
with KVMCPUConfig objs that will be read/write during init and so on.
But to do that properly we must be able to declare a static array that
hold KVM regs.

C does not allow to init static arrays and use functions as
initializers, e.g. we can't do:

.kvm_reg_id = kvm_riscv_reg_id_ulong(...)

When instantiating the array. We can do that with macros though, so our
goal is turn kvm_riscv_reg_ulong() in a macro. It is cleaner to turn
every other reg_id_*() function in macros, and ulong will end up using
the macros for u32 and u64, so we'll start with them.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index afe3d3e609..9d5f54f270 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -58,6 +58,12 @@ void riscv_kvm_aplic_request(void *opaque, int irq, int level)
 
 static bool cap_has_mp_state;
 
+#define KVM_RISCV_REG_ID_U32(type, idx) (KVM_REG_RISCV | KVM_REG_SIZE_U32 | \
+                                         type | idx)
+
+#define KVM_RISCV_REG_ID_U64(type, idx) (KVM_REG_RISCV | KVM_REG_SIZE_U64 | \
+                                         type | idx)
+
 static uint64_t kvm_riscv_reg_id_ulong(CPURISCVState *env, uint64_t type,
                                  uint64_t idx)
 {
@@ -76,16 +82,6 @@ static uint64_t kvm_riscv_reg_id_ulong(CPURISCVState *env, uint64_t type,
     return id;
 }
 
-static uint64_t kvm_riscv_reg_id_u32(uint64_t type, uint64_t idx)
-{
-    return KVM_REG_RISCV | KVM_REG_SIZE_U32 | type | idx;
-}
-
-static uint64_t kvm_riscv_reg_id_u64(uint64_t type, uint64_t idx)
-{
-    return KVM_REG_RISCV | KVM_REG_SIZE_U64 | type | idx;
-}
-
 static uint64_t kvm_encode_reg_size_id(uint64_t id, size_t size_b)
 {
     uint64_t size_ctz = __builtin_ctz(size_b);
@@ -119,12 +115,12 @@ static uint64_t kvm_riscv_vector_reg_id(RISCVCPU *cpu,
     kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG, \
                            KVM_REG_RISCV_CONFIG_REG(name))
 
-#define RISCV_TIMER_REG(name)  kvm_riscv_reg_id_u64(KVM_REG_RISCV_TIMER, \
+#define RISCV_TIMER_REG(name)  KVM_RISCV_REG_ID_U64(KVM_REG_RISCV_TIMER, \
                  KVM_REG_RISCV_TIMER_REG(name))
 
-#define RISCV_FP_F_REG(idx)  kvm_riscv_reg_id_u32(KVM_REG_RISCV_FP_F, idx)
+#define RISCV_FP_F_REG(idx)  KVM_RISCV_REG_ID_U32(KVM_REG_RISCV_FP_F, idx)
 
-#define RISCV_FP_D_REG(idx)  kvm_riscv_reg_id_u64(KVM_REG_RISCV_FP_D, idx)
+#define RISCV_FP_D_REG(idx)  KVM_RISCV_REG_ID_U64(KVM_REG_RISCV_FP_D, idx)
 
 #define RISCV_VECTOR_CSR_REG(env, name) \
     kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_VECTOR, \
-- 
2.49.0

[PATCH 3/7] target/riscv/kvm: turn kvm_riscv_reg_id_ulong() in a macro

[Daniel Henrique Barboza]

We need the reg_id_ulong() helper to be a macro to be able to create a
static array of KVMCPUConfig that will hold CSR information.

Despite the amount of changes all of them are tedious/trivial:

- replace instances of "kvm_riscv_reg_id_ulong" with
  "KVM_RISCV_REG_ID_ULONG";

- RISCV_CORE_REG(), RISCV_CSR_REG(), RISCV_CONFIG_REG() and
  RISCV_VECTOR_CSR_REG() only receives one 'name' arg. Remove unneeded
  'env' variables when applicable.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 99 ++++++++++++++++----------------------
 1 file changed, 41 insertions(+), 58 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 9d5f54f270..0bcadab977 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -64,23 +64,11 @@ static bool cap_has_mp_state;
 #define KVM_RISCV_REG_ID_U64(type, idx) (KVM_REG_RISCV | KVM_REG_SIZE_U64 | \
                                          type | idx)
 
-static uint64_t kvm_riscv_reg_id_ulong(CPURISCVState *env, uint64_t type,
-                                 uint64_t idx)
-{
-    uint64_t id = KVM_REG_RISCV | type | idx;
-
-    switch (riscv_cpu_mxl(env)) {
-    case MXL_RV32:
-        id |= KVM_REG_SIZE_U32;
-        break;
-    case MXL_RV64:
-        id |= KVM_REG_SIZE_U64;
-        break;
-    default:
-        g_assert_not_reached();
-    }
-    return id;
-}
+#if defined(TARGET_RISCV64)
+#define KVM_RISCV_REG_ID_ULONG(type, idx) KVM_RISCV_REG_ID_U64(type, idx)
+#else
+#define KVM_RISCV_REG_ID_ULONG(type, idx) KVM_RISCV_REG_ID_U32(type, idx)
+#endif
 
 static uint64_t kvm_encode_reg_size_id(uint64_t id, size_t size_b)
 {
@@ -103,16 +91,16 @@ static uint64_t kvm_riscv_vector_reg_id(RISCVCPU *cpu,
     return kvm_encode_reg_size_id(id, size_b);
 }
 
-#define RISCV_CORE_REG(env, name) \
-    kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CORE, \
+#define RISCV_CORE_REG(name) \
+    KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_CORE, \
                            KVM_REG_RISCV_CORE_REG(name))
 
-#define RISCV_CSR_REG(env, name) \
-    kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CSR, \
+#define RISCV_CSR_REG(name) \
+    KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_CSR, \
                            KVM_REG_RISCV_CSR_REG(name))
 
-#define RISCV_CONFIG_REG(env, name) \
-    kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG, \
+#define RISCV_CONFIG_REG(name) \
+    KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_CONFIG, \
                            KVM_REG_RISCV_CONFIG_REG(name))
 
 #define RISCV_TIMER_REG(name)  KVM_RISCV_REG_ID_U64(KVM_REG_RISCV_TIMER, \
@@ -122,13 +110,13 @@ static uint64_t kvm_riscv_vector_reg_id(RISCVCPU *cpu,
 
 #define RISCV_FP_D_REG(idx)  KVM_RISCV_REG_ID_U64(KVM_REG_RISCV_FP_D, idx)
 
-#define RISCV_VECTOR_CSR_REG(env, name) \
-    kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_VECTOR, \
+#define RISCV_VECTOR_CSR_REG(name) \
+    KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_VECTOR, \
                            KVM_REG_RISCV_VECTOR_CSR_REG(name))
 
 #define KVM_RISCV_GET_CSR(cs, env, csr, reg) \
     do { \
-        int _ret = kvm_get_one_reg(cs, RISCV_CSR_REG(env, csr), &reg); \
+        int _ret = kvm_get_one_reg(cs, RISCV_CSR_REG(csr), &reg); \
         if (_ret) { \
             return _ret; \
         } \
@@ -136,7 +124,7 @@ static uint64_t kvm_riscv_vector_reg_id(RISCVCPU *cpu,
 
 #define KVM_RISCV_SET_CSR(cs, env, csr, reg) \
     do { \
-        int _ret = kvm_set_one_reg(cs, RISCV_CSR_REG(env, csr), &reg); \
+        int _ret = kvm_set_one_reg(cs, RISCV_CSR_REG(csr), &reg); \
         if (_ret) { \
             return _ret; \
         } \
@@ -244,7 +232,7 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
 
         /* If we're here we're going to disable the MISA bit */
         reg = 0;
-        id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_ISA_EXT,
+        id = KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_ISA_EXT,
                                     misa_cfg->kvm_reg_id);
         ret = kvm_set_one_reg(cs, id, &reg);
         if (ret != 0) {
@@ -430,7 +418,6 @@ static KVMCPUConfig kvm_sbi_dbcn = {
 
 static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
 {
-    CPURISCVState *env = &cpu->env;
     uint64_t id, reg;
     int i, ret;
 
@@ -441,7 +428,7 @@ static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
             continue;
         }
 
-        id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_ISA_EXT,
+        id = KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_ISA_EXT,
                                     multi_ext_cfg->kvm_reg_id);
         reg = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
         ret = kvm_set_one_reg(cs, id, &reg);
@@ -566,14 +553,14 @@ static int kvm_riscv_get_regs_core(CPUState *cs)
     target_ulong reg;
     CPURISCVState *env = &RISCV_CPU(cs)->env;
 
-    ret = kvm_get_one_reg(cs, RISCV_CORE_REG(env, regs.pc), &reg);
+    ret = kvm_get_one_reg(cs, RISCV_CORE_REG(regs.pc), &reg);
     if (ret) {
         return ret;
     }
     env->pc = reg;
 
     for (i = 1; i < 32; i++) {
-        uint64_t id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CORE, i);
+        uint64_t id = KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_CORE, i);
         ret = kvm_get_one_reg(cs, id, &reg);
         if (ret) {
             return ret;
@@ -592,13 +579,13 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
     CPURISCVState *env = &RISCV_CPU(cs)->env;
 
     reg = env->pc;
-    ret = kvm_set_one_reg(cs, RISCV_CORE_REG(env, regs.pc), &reg);
+    ret = kvm_set_one_reg(cs, RISCV_CORE_REG(regs.pc), &reg);
     if (ret) {
         return ret;
     }
 
     for (i = 1; i < 32; i++) {
-        uint64_t id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CORE, i);
+        uint64_t id = KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_CORE, i);
         reg = env->gpr[i];
         ret = kvm_set_one_reg(cs, id, &reg);
         if (ret) {
@@ -796,26 +783,26 @@ static int kvm_riscv_get_regs_vector(CPUState *cs)
         return 0;
     }
 
-    ret = kvm_get_one_reg(cs, RISCV_VECTOR_CSR_REG(env, vstart), &reg);
+    ret = kvm_get_one_reg(cs, RISCV_VECTOR_CSR_REG(vstart), &reg);
     if (ret) {
         return ret;
     }
     env->vstart = reg;
 
-    ret = kvm_get_one_reg(cs, RISCV_VECTOR_CSR_REG(env, vl), &reg);
+    ret = kvm_get_one_reg(cs, RISCV_VECTOR_CSR_REG(vl), &reg);
     if (ret) {
         return ret;
     }
     env->vl = reg;
 
-    ret = kvm_get_one_reg(cs, RISCV_VECTOR_CSR_REG(env, vtype), &reg);
+    ret = kvm_get_one_reg(cs, RISCV_VECTOR_CSR_REG(vtype), &reg);
     if (ret) {
         return ret;
     }
     env->vtype = reg;
 
     if (kvm_v_vlenb.supported) {
-        ret = kvm_get_one_reg(cs, RISCV_VECTOR_CSR_REG(env, vlenb), &reg);
+        ret = kvm_get_one_reg(cs, RISCV_VECTOR_CSR_REG(vlenb), &reg);
         if (ret) {
             return ret;
         }
@@ -853,26 +840,26 @@ static int kvm_riscv_put_regs_vector(CPUState *cs)
     }
 
     reg = env->vstart;
-    ret = kvm_set_one_reg(cs, RISCV_VECTOR_CSR_REG(env, vstart), &reg);
+    ret = kvm_set_one_reg(cs, RISCV_VECTOR_CSR_REG(vstart), &reg);
     if (ret) {
         return ret;
     }
 
     reg = env->vl;
-    ret = kvm_set_one_reg(cs, RISCV_VECTOR_CSR_REG(env, vl), &reg);
+    ret = kvm_set_one_reg(cs, RISCV_VECTOR_CSR_REG(vl), &reg);
     if (ret) {
         return ret;
     }
 
     reg = env->vtype;
-    ret = kvm_set_one_reg(cs, RISCV_VECTOR_CSR_REG(env, vtype), &reg);
+    ret = kvm_set_one_reg(cs, RISCV_VECTOR_CSR_REG(vtype), &reg);
     if (ret) {
         return ret;
     }
 
     if (kvm_v_vlenb.supported) {
         reg = cpu->cfg.vlenb;
-        ret = kvm_set_one_reg(cs, RISCV_VECTOR_CSR_REG(env, vlenb), &reg);
+        ret = kvm_set_one_reg(cs, RISCV_VECTOR_CSR_REG(vlenb), &reg);
 
         for (int i = 0; i < 32; i++) {
             /*
@@ -951,25 +938,24 @@ static void kvm_riscv_destroy_scratch_vcpu(KVMScratchCPU *scratch)
 
 static void kvm_riscv_init_machine_ids(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
 {
-    CPURISCVState *env = &cpu->env;
     struct kvm_one_reg reg;
     int ret;
 
-    reg.id = RISCV_CONFIG_REG(env, mvendorid);
+    reg.id = RISCV_CONFIG_REG(mvendorid);
     reg.addr = (uint64_t)&cpu->cfg.mvendorid;
     ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
     if (ret != 0) {
         error_report("Unable to retrieve mvendorid from host, error %d", ret);
     }
 
-    reg.id = RISCV_CONFIG_REG(env, marchid);
+    reg.id = RISCV_CONFIG_REG(marchid);
     reg.addr = (uint64_t)&cpu->cfg.marchid;
     ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
     if (ret != 0) {
         error_report("Unable to retrieve marchid from host, error %d", ret);
     }
 
-    reg.id = RISCV_CONFIG_REG(env, mimpid);
+    reg.id = RISCV_CONFIG_REG(mimpid);
     reg.addr = (uint64_t)&cpu->cfg.mimpid;
     ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
     if (ret != 0) {
@@ -984,7 +970,7 @@ static void kvm_riscv_init_misa_ext_mask(RISCVCPU *cpu,
     struct kvm_one_reg reg;
     int ret;
 
-    reg.id = RISCV_CONFIG_REG(env, isa);
+    reg.id = RISCV_CONFIG_REG(isa);
     reg.addr = (uint64_t)&env->misa_ext_mask;
     ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
 
@@ -1001,11 +987,10 @@ static void kvm_riscv_init_misa_ext_mask(RISCVCPU *cpu,
 static void kvm_riscv_read_cbomz_blksize(RISCVCPU *cpu, KVMScratchCPU *kvmcpu,
                                          KVMCPUConfig *cbomz_cfg)
 {
-    CPURISCVState *env = &cpu->env;
     struct kvm_one_reg reg;
     int ret;
 
-    reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
+    reg.id = KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_CONFIG,
                                     cbomz_cfg->kvm_reg_id);
     reg.addr = (uint64_t)kvmconfig_get_cfg_addr(cpu, cbomz_cfg);
     ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
@@ -1019,7 +1004,6 @@ static void kvm_riscv_read_cbomz_blksize(RISCVCPU *cpu, KVMScratchCPU *kvmcpu,
 static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
                                            KVMScratchCPU *kvmcpu)
 {
-    CPURISCVState *env = &cpu->env;
     uint64_t val;
     int i, ret;
 
@@ -1027,7 +1011,7 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
         KVMCPUConfig *multi_ext_cfg = &kvm_multi_ext_cfgs[i];
         struct kvm_one_reg reg;
 
-        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_ISA_EXT,
+        reg.id = KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_ISA_EXT,
                                         multi_ext_cfg->kvm_reg_id);
         reg.addr = (uint64_t)&val;
         ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
@@ -1159,7 +1143,7 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
 
     for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
         multi_ext_cfg = &kvm_multi_ext_cfgs[i];
-        reg_id = kvm_riscv_reg_id_ulong(&cpu->env, KVM_REG_RISCV_ISA_EXT,
+        reg_id = KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_ISA_EXT,
                                         multi_ext_cfg->kvm_reg_id);
         reg_search = bsearch(&reg_id, reglist->reg, reglist->n,
                              sizeof(uint64_t), uint64_cmp);
@@ -1338,12 +1322,11 @@ void kvm_arch_init_irq_routing(KVMState *s)
 
 static int kvm_vcpu_set_machine_ids(RISCVCPU *cpu, CPUState *cs)
 {
-    CPURISCVState *env = &cpu->env;
     target_ulong reg;
     uint64_t id;
     int ret;
 
-    id = RISCV_CONFIG_REG(env, mvendorid);
+    id = RISCV_CONFIG_REG(mvendorid);
     /*
      * cfg.mvendorid is an uint32 but a target_ulong will
      * be written. Assign it to a target_ulong var to avoid
@@ -1355,13 +1338,13 @@ static int kvm_vcpu_set_machine_ids(RISCVCPU *cpu, CPUState *cs)
         return ret;
     }
 
-    id = RISCV_CONFIG_REG(env, marchid);
+    id = RISCV_CONFIG_REG(marchid);
     ret = kvm_set_one_reg(cs, id, &cpu->cfg.marchid);
     if (ret != 0) {
         return ret;
     }
 
-    id = RISCV_CONFIG_REG(env, mimpid);
+    id = RISCV_CONFIG_REG(mimpid);
     ret = kvm_set_one_reg(cs, id, &cpu->cfg.mimpid);
 
     return ret;
@@ -1911,7 +1894,7 @@ void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
     if (cpu->cfg.ext_zicbom &&
         riscv_cpu_option_set(kvm_cbom_blocksize.name)) {
 
-        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
+        reg.id = KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_CONFIG,
                                         kvm_cbom_blocksize.kvm_reg_id);
         reg.addr = (uint64_t)&val;
         ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
@@ -1930,7 +1913,7 @@ void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
     if (cpu->cfg.ext_zicboz &&
         riscv_cpu_option_set(kvm_cboz_blocksize.name)) {
 
-        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
+        reg.id = KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_CONFIG,
                                         kvm_cboz_blocksize.kvm_reg_id);
         reg.addr = (uint64_t)&val;
         ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
-- 
2.49.0

[PATCH 4/7] target/riscv/kvm: add kvm_csr_cfgs[]

[Daniel Henrique Barboza]

At this moment we're not checking if the host has support for any
specific CSR before doing get/put regs. This will cause problems if the
host KVM doesn't support it (see [1] as an example).

We'll use the same approach done with the CPU extensions: read all known
KVM CSRs during init() to check for availability, then read/write them
if they are present. This will be made by either using get-reglist or by
directly reading the CSRs.

For now we'll just convert the CSRs to use a kvm_csr_cfg[] array,
reusing the same KVMCPUConfig abstraction we use for extensions, and use
the array in (get|put)_csr_regs() instead of manually listing them. A
lot of boilerplate will be added but at least we'll automate the get/put
procedure for CSRs, i.e. adding a new CSR in the future will be a matter
of adding it in kvm_csr_regs[] and everything else will be taken care
of.

Despite all the code changes no behavioral change is made.

[1] https://lore.kernel.org/qemu-riscv/CABJz62OfUDHYkQ0T3rGHStQprf1c7_E0qBLbLKhfv=+jb0SYAw@mail.gmail.com/

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.h         |   1 +
 target/riscv/kvm/kvm-cpu.c | 119 ++++++++++++++++++++++++++-----------
 2 files changed, 84 insertions(+), 36 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 51e49e03de..7a56666f9a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -79,6 +79,7 @@ const char *riscv_get_misa_ext_name(uint32_t bit);
 const char *riscv_get_misa_ext_description(uint32_t bit);
 
 #define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
+#define ENV_CSR_OFFSET(_csr) offsetof(CPURISCVState, _csr)
 
 typedef struct riscv_cpu_profile {
     struct riscv_cpu_profile *u_parent;
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 0bcadab977..99a4f01b15 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -114,22 +114,6 @@ static uint64_t kvm_riscv_vector_reg_id(RISCVCPU *cpu,
     KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_VECTOR, \
                            KVM_REG_RISCV_VECTOR_CSR_REG(name))
 
-#define KVM_RISCV_GET_CSR(cs, env, csr, reg) \
-    do { \
-        int _ret = kvm_get_one_reg(cs, RISCV_CSR_REG(csr), &reg); \
-        if (_ret) { \
-            return _ret; \
-        } \
-    } while (0)
-
-#define KVM_RISCV_SET_CSR(cs, env, csr, reg) \
-    do { \
-        int _ret = kvm_set_one_reg(cs, RISCV_CSR_REG(csr), &reg); \
-        if (_ret) { \
-            return _ret; \
-        } \
-    } while (0)
-
 #define KVM_RISCV_GET_TIMER(cs, name, reg) \
     do { \
         int ret = kvm_get_one_reg(cs, RISCV_TIMER_REG(name), &reg); \
@@ -150,6 +134,7 @@ typedef struct KVMCPUConfig {
     const char *name;
     const char *description;
     target_ulong offset;
+    uint32_t prop_size;
     uint64_t kvm_reg_id;
     bool user_set;
     bool supported;
@@ -251,6 +236,54 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
     }
 }
 
+#define KVM_CSR_CFG(_name, _env_prop, _env_prop_size, reg_id) \
+    {.name = _name, .offset = ENV_CSR_OFFSET(_env_prop), \
+     .prop_size = _env_prop_size, .kvm_reg_id = reg_id}
+
+static KVMCPUConfig kvm_csr_cfgs[] = {
+    KVM_CSR_CFG("sstatus", mstatus, sizeof(uint64_t), RISCV_CSR_REG(sstatus)),
+    KVM_CSR_CFG("sie", mie, sizeof(uint64_t), RISCV_CSR_REG(sie)),
+    KVM_CSR_CFG("stvec", stvec, sizeof(target_ulong), RISCV_CSR_REG(stvec)),
+    KVM_CSR_CFG("sscratch", sscratch, sizeof(target_ulong),
+                RISCV_CSR_REG(sscratch)),
+    KVM_CSR_CFG("sepc", sepc, sizeof(target_ulong), RISCV_CSR_REG(sepc)),
+    KVM_CSR_CFG("scause", scause, sizeof(target_ulong), RISCV_CSR_REG(scause)),
+    KVM_CSR_CFG("stval", stval, sizeof(target_ulong), RISCV_CSR_REG(stval)),
+    KVM_CSR_CFG("sip", mip, sizeof(uint64_t), RISCV_CSR_REG(sip)),
+    KVM_CSR_CFG("satp", satp, sizeof(target_ulong), RISCV_CSR_REG(satp)),
+};
+
+static void *kvmconfig_get_env_addr(RISCVCPU *cpu, KVMCPUConfig *csr_cfg)
+{
+    return (void *)&cpu->env + csr_cfg->offset;
+}
+
+static uint64_t kvm_cpu_csr_get_u32(RISCVCPU *cpu, KVMCPUConfig *csr_cfg)
+{
+    uint32_t *val32 = kvmconfig_get_env_addr(cpu, csr_cfg);
+    return *val32;
+}
+
+static uint64_t kvm_cpu_csr_get_u64(RISCVCPU *cpu, KVMCPUConfig *csr_cfg)
+{
+    uint64_t *val64 = kvmconfig_get_env_addr(cpu, csr_cfg);
+    return *val64;
+}
+
+static void kvm_cpu_csr_set_u32(RISCVCPU *cpu, KVMCPUConfig *csr_cfg,
+                                uint32_t val)
+{
+    uint32_t *val32 = kvmconfig_get_env_addr(cpu, csr_cfg);
+    *val32 = val;
+}
+
+static void kvm_cpu_csr_set_u64(RISCVCPU *cpu, KVMCPUConfig *csr_cfg,
+                                uint64_t val)
+{
+    uint64_t *val64 = kvmconfig_get_env_addr(cpu, csr_cfg);
+    *val64 = val;
+}
+
 #define KVM_EXT_CFG(_name, _prop, _reg_id) \
     {.name = _name, .offset = CPU_CFG_OFFSET(_prop), \
      .kvm_reg_id = _reg_id}
@@ -598,34 +631,48 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
 
 static int kvm_riscv_get_regs_csr(CPUState *cs)
 {
-    CPURISCVState *env = &RISCV_CPU(cs)->env;
+    RISCVCPU *cpu = RISCV_CPU(cs);
+    uint64_t reg;
+    int i, ret;
+
+    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
+        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
 
-    KVM_RISCV_GET_CSR(cs, env, sstatus, env->mstatus);
-    KVM_RISCV_GET_CSR(cs, env, sie, env->mie);
-    KVM_RISCV_GET_CSR(cs, env, stvec, env->stvec);
-    KVM_RISCV_GET_CSR(cs, env, sscratch, env->sscratch);
-    KVM_RISCV_GET_CSR(cs, env, sepc, env->sepc);
-    KVM_RISCV_GET_CSR(cs, env, scause, env->scause);
-    KVM_RISCV_GET_CSR(cs, env, stval, env->stval);
-    KVM_RISCV_GET_CSR(cs, env, sip, env->mip);
-    KVM_RISCV_GET_CSR(cs, env, satp, env->satp);
+        ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
+        if (ret) {
+            return ret;
+        }
+
+        if (csr_cfg->prop_size == sizeof(uint32_t)) {
+            kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
+        } else {
+            kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
+        }
+    }
 
     return 0;
 }
 
 static int kvm_riscv_put_regs_csr(CPUState *cs)
 {
-    CPURISCVState *env = &RISCV_CPU(cs)->env;
+    RISCVCPU *cpu = RISCV_CPU(cs);
+    uint64_t reg;
+    int i, ret;
+
+    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
+        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
+
+        if (csr_cfg->prop_size == sizeof(uint32_t)) {
+            reg = kvm_cpu_csr_get_u32(cpu, csr_cfg);
+        } else {
+            reg = kvm_cpu_csr_get_u64(cpu, csr_cfg);
+        }
 
-    KVM_RISCV_SET_CSR(cs, env, sstatus, env->mstatus);
-    KVM_RISCV_SET_CSR(cs, env, sie, env->mie);
-    KVM_RISCV_SET_CSR(cs, env, stvec, env->stvec);
-    KVM_RISCV_SET_CSR(cs, env, sscratch, env->sscratch);
-    KVM_RISCV_SET_CSR(cs, env, sepc, env->sepc);
-    KVM_RISCV_SET_CSR(cs, env, scause, env->scause);
-    KVM_RISCV_SET_CSR(cs, env, stval, env->stval);
-    KVM_RISCV_SET_CSR(cs, env, sip, env->mip);
-    KVM_RISCV_SET_CSR(cs, env, satp, env->satp);
+        ret = kvm_set_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
+        if (ret) {
+            return ret;
+        }
+    }
 
     return 0;
 }
-- 
2.49.0

Re: [PATCH 4/7] target/riscv/kvm: add kvm_csr_cfgs[]

[Andrew Jones]

On Thu, Apr 17, 2025 at 09:48:36AM -0300, Daniel Henrique Barboza wrote:
> At this moment we're not checking if the host has support for any
> specific CSR before doing get/put regs. This will cause problems if the
> host KVM doesn't support it (see [1] as an example).
> 
> We'll use the same approach done with the CPU extensions: read all known
> KVM CSRs during init() to check for availability, then read/write them
> if they are present. This will be made by either using get-reglist or by
> directly reading the CSRs.
> 
> For now we'll just convert the CSRs to use a kvm_csr_cfg[] array,
> reusing the same KVMCPUConfig abstraction we use for extensions, and use
> the array in (get|put)_csr_regs() instead of manually listing them. A
> lot of boilerplate will be added but at least we'll automate the get/put
> procedure for CSRs, i.e. adding a new CSR in the future will be a matter
> of adding it in kvm_csr_regs[] and everything else will be taken care
> of.
> 
> Despite all the code changes no behavioral change is made.
> 
> [1] https://lore.kernel.org/qemu-riscv/CABJz62OfUDHYkQ0T3rGHStQprf1c7_E0qBLbLKhfv=+jb0SYAw@mail.gmail.com/
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.h         |   1 +
>  target/riscv/kvm/kvm-cpu.c | 119 ++++++++++++++++++++++++++-----------
>  2 files changed, 84 insertions(+), 36 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 51e49e03de..7a56666f9a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -79,6 +79,7 @@ const char *riscv_get_misa_ext_name(uint32_t bit);
>  const char *riscv_get_misa_ext_description(uint32_t bit);
>  
>  #define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
> +#define ENV_CSR_OFFSET(_csr) offsetof(CPURISCVState, _csr)
>  
>  typedef struct riscv_cpu_profile {
>      struct riscv_cpu_profile *u_parent;
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 0bcadab977..99a4f01b15 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -114,22 +114,6 @@ static uint64_t kvm_riscv_vector_reg_id(RISCVCPU *cpu,
>      KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_VECTOR, \
>                             KVM_REG_RISCV_VECTOR_CSR_REG(name))
>  
> -#define KVM_RISCV_GET_CSR(cs, env, csr, reg) \
> -    do { \
> -        int _ret = kvm_get_one_reg(cs, RISCV_CSR_REG(csr), &reg); \
> -        if (_ret) { \
> -            return _ret; \
> -        } \
> -    } while (0)
> -
> -#define KVM_RISCV_SET_CSR(cs, env, csr, reg) \
> -    do { \
> -        int _ret = kvm_set_one_reg(cs, RISCV_CSR_REG(csr), &reg); \
> -        if (_ret) { \
> -            return _ret; \
> -        } \
> -    } while (0)
> -
>  #define KVM_RISCV_GET_TIMER(cs, name, reg) \
>      do { \
>          int ret = kvm_get_one_reg(cs, RISCV_TIMER_REG(name), &reg); \
> @@ -150,6 +134,7 @@ typedef struct KVMCPUConfig {
>      const char *name;
>      const char *description;
>      target_ulong offset;
> +    uint32_t prop_size;
>      uint64_t kvm_reg_id;
>      bool user_set;
>      bool supported;
> @@ -251,6 +236,54 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
>      }
>  }
>  
> +#define KVM_CSR_CFG(_name, _env_prop, _env_prop_size, reg_id) \
> +    {.name = _name, .offset = ENV_CSR_OFFSET(_env_prop), \
> +     .prop_size = _env_prop_size, .kvm_reg_id = reg_id}
> +
> +static KVMCPUConfig kvm_csr_cfgs[] = {
> +    KVM_CSR_CFG("sstatus", mstatus, sizeof(uint64_t), RISCV_CSR_REG(sstatus)),
> +    KVM_CSR_CFG("sie", mie, sizeof(uint64_t), RISCV_CSR_REG(sie)),
> +    KVM_CSR_CFG("stvec", stvec, sizeof(target_ulong), RISCV_CSR_REG(stvec)),
> +    KVM_CSR_CFG("sscratch", sscratch, sizeof(target_ulong),
> +                RISCV_CSR_REG(sscratch)),
> +    KVM_CSR_CFG("sepc", sepc, sizeof(target_ulong), RISCV_CSR_REG(sepc)),
> +    KVM_CSR_CFG("scause", scause, sizeof(target_ulong), RISCV_CSR_REG(scause)),
> +    KVM_CSR_CFG("stval", stval, sizeof(target_ulong), RISCV_CSR_REG(stval)),
> +    KVM_CSR_CFG("sip", mip, sizeof(uint64_t), RISCV_CSR_REG(sip)),
> +    KVM_CSR_CFG("satp", satp, sizeof(target_ulong), RISCV_CSR_REG(satp)),

We don't need to pass in sizeof(env_prop). We can just define KVM_CSR_CFG
to do it for us.

 #define KVM_CSR_CFG(_name, csr, reg_id) \
     { .name = _name, .offset = ENV_CSR_OFFSET(csr), \
       .prop_size = sizeof(((CPURISCVState *)0)->csr), \
       .kvm_reg_id = reg_id, }

But I don't think we need it at all. See below.

> +};
> +
> +static void *kvmconfig_get_env_addr(RISCVCPU *cpu, KVMCPUConfig *csr_cfg)
> +{
> +    return (void *)&cpu->env + csr_cfg->offset;
> +}
> +
> +static uint64_t kvm_cpu_csr_get_u32(RISCVCPU *cpu, KVMCPUConfig *csr_cfg)

This should return a uint32_t.

> +{
> +    uint32_t *val32 = kvmconfig_get_env_addr(cpu, csr_cfg);
> +    return *val32;
> +}
> +
> +static uint64_t kvm_cpu_csr_get_u64(RISCVCPU *cpu, KVMCPUConfig *csr_cfg)
> +{
> +    uint64_t *val64 = kvmconfig_get_env_addr(cpu, csr_cfg);
> +    return *val64;
> +}
> +
> +static void kvm_cpu_csr_set_u32(RISCVCPU *cpu, KVMCPUConfig *csr_cfg,
> +                                uint32_t val)
> +{
> +    uint32_t *val32 = kvmconfig_get_env_addr(cpu, csr_cfg);
> +    *val32 = val;
> +}
> +
> +static void kvm_cpu_csr_set_u64(RISCVCPU *cpu, KVMCPUConfig *csr_cfg,
> +                                uint64_t val)
> +{
> +    uint64_t *val64 = kvmconfig_get_env_addr(cpu, csr_cfg);
> +    *val64 = val;
> +}
> +
>  #define KVM_EXT_CFG(_name, _prop, _reg_id) \
>      {.name = _name, .offset = CPU_CFG_OFFSET(_prop), \
>       .kvm_reg_id = _reg_id}
> @@ -598,34 +631,48 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
>  
>  static int kvm_riscv_get_regs_csr(CPUState *cs)
>  {
> -    CPURISCVState *env = &RISCV_CPU(cs)->env;
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +    uint64_t reg;
> +    int i, ret;
> +
> +    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
> +        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
>  
> -    KVM_RISCV_GET_CSR(cs, env, sstatus, env->mstatus);
> -    KVM_RISCV_GET_CSR(cs, env, sie, env->mie);
> -    KVM_RISCV_GET_CSR(cs, env, stvec, env->stvec);
> -    KVM_RISCV_GET_CSR(cs, env, sscratch, env->sscratch);
> -    KVM_RISCV_GET_CSR(cs, env, sepc, env->sepc);
> -    KVM_RISCV_GET_CSR(cs, env, scause, env->scause);
> -    KVM_RISCV_GET_CSR(cs, env, stval, env->stval);
> -    KVM_RISCV_GET_CSR(cs, env, sip, env->mip);
> -    KVM_RISCV_GET_CSR(cs, env, satp, env->satp);
> +        ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
> +        if (ret) {
> +            return ret;
> +        }
> +
> +        if (csr_cfg->prop_size == sizeof(uint32_t)) {

 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)) {
     kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
 } else {
     uh, oh...
 }

> +            kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
> +        } else {
> +            kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
> +        }
> +    }
>  
>      return 0;
>  }
>  
>  static int kvm_riscv_put_regs_csr(CPUState *cs)
>  {
> -    CPURISCVState *env = &RISCV_CPU(cs)->env;
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +    uint64_t reg;
> +    int i, ret;
> +
> +    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
> +        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
> +
> +        if (csr_cfg->prop_size == sizeof(uint32_t)) {
> +            reg = kvm_cpu_csr_get_u32(cpu, csr_cfg);
> +        } else {
> +            reg = kvm_cpu_csr_get_u64(cpu, csr_cfg);
> +        }

same comment as above

>  
> -    KVM_RISCV_SET_CSR(cs, env, sstatus, env->mstatus);
> -    KVM_RISCV_SET_CSR(cs, env, sie, env->mie);
> -    KVM_RISCV_SET_CSR(cs, env, stvec, env->stvec);
> -    KVM_RISCV_SET_CSR(cs, env, sscratch, env->sscratch);
> -    KVM_RISCV_SET_CSR(cs, env, sepc, env->sepc);
> -    KVM_RISCV_SET_CSR(cs, env, scause, env->scause);
> -    KVM_RISCV_SET_CSR(cs, env, stval, env->stval);
> -    KVM_RISCV_SET_CSR(cs, env, sip, env->mip);
> -    KVM_RISCV_SET_CSR(cs, env, satp, env->satp);
> +        ret = kvm_set_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
>  
>      return 0;
>  }
> -- 
> 2.49.0
> 
>

Thanks,
drew

Re: [PATCH 4/7] target/riscv/kvm: add kvm_csr_cfgs[]

[Daniel Henrique Barboza]

On 4/23/25 12:15 PM, Andrew Jones wrote:
> On Thu, Apr 17, 2025 at 09:48:36AM -0300, Daniel Henrique Barboza wrote:
>> At this moment we're not checking if the host has support for any
>> specific CSR before doing get/put regs. This will cause problems if the
>> host KVM doesn't support it (see [1] as an example).
>>
>> We'll use the same approach done with the CPU extensions: read all known
>> KVM CSRs during init() to check for availability, then read/write them
>> if they are present. This will be made by either using get-reglist or by
>> directly reading the CSRs.
>>
>> For now we'll just convert the CSRs to use a kvm_csr_cfg[] array,
>> reusing the same KVMCPUConfig abstraction we use for extensions, and use
>> the array in (get|put)_csr_regs() instead of manually listing them. A
>> lot of boilerplate will be added but at least we'll automate the get/put
>> procedure for CSRs, i.e. adding a new CSR in the future will be a matter
>> of adding it in kvm_csr_regs[] and everything else will be taken care
>> of.
>>
>> Despite all the code changes no behavioral change is made.
>>
>> [1] https://lore.kernel.org/qemu-riscv/CABJz62OfUDHYkQ0T3rGHStQprf1c7_E0qBLbLKhfv=+jb0SYAw@mail.gmail.com/
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.h         |   1 +
>>   target/riscv/kvm/kvm-cpu.c | 119 ++++++++++++++++++++++++++-----------
>>   2 files changed, 84 insertions(+), 36 deletions(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 51e49e03de..7a56666f9a 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -79,6 +79,7 @@ const char *riscv_get_misa_ext_name(uint32_t bit);
>>   const char *riscv_get_misa_ext_description(uint32_t bit);
>>   
>>   #define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
>> +#define ENV_CSR_OFFSET(_csr) offsetof(CPURISCVState, _csr)
>>   
>>   typedef struct riscv_cpu_profile {
>>       struct riscv_cpu_profile *u_parent;
>> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
>> index 0bcadab977..99a4f01b15 100644
>> --- a/target/riscv/kvm/kvm-cpu.c
>> +++ b/target/riscv/kvm/kvm-cpu.c
>> @@ -114,22 +114,6 @@ static uint64_t kvm_riscv_vector_reg_id(RISCVCPU *cpu,
>>       KVM_RISCV_REG_ID_ULONG(KVM_REG_RISCV_VECTOR, \
>>                              KVM_REG_RISCV_VECTOR_CSR_REG(name))
>>   
>> -#define KVM_RISCV_GET_CSR(cs, env, csr, reg) \
>> -    do { \
>> -        int _ret = kvm_get_one_reg(cs, RISCV_CSR_REG(csr), &reg); \
>> -        if (_ret) { \
>> -            return _ret; \
>> -        } \
>> -    } while (0)
>> -
>> -#define KVM_RISCV_SET_CSR(cs, env, csr, reg) \
>> -    do { \
>> -        int _ret = kvm_set_one_reg(cs, RISCV_CSR_REG(csr), &reg); \
>> -        if (_ret) { \
>> -            return _ret; \
>> -        } \
>> -    } while (0)
>> -
>>   #define KVM_RISCV_GET_TIMER(cs, name, reg) \
>>       do { \
>>           int ret = kvm_get_one_reg(cs, RISCV_TIMER_REG(name), &reg); \
>> @@ -150,6 +134,7 @@ typedef struct KVMCPUConfig {
>>       const char *name;
>>       const char *description;
>>       target_ulong offset;
>> +    uint32_t prop_size;
>>       uint64_t kvm_reg_id;
>>       bool user_set;
>>       bool supported;
>> @@ -251,6 +236,54 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
>>       }
>>   }
>>   
>> +#define KVM_CSR_CFG(_name, _env_prop, _env_prop_size, reg_id) \
>> +    {.name = _name, .offset = ENV_CSR_OFFSET(_env_prop), \
>> +     .prop_size = _env_prop_size, .kvm_reg_id = reg_id}
>> +
>> +static KVMCPUConfig kvm_csr_cfgs[] = {
>> +    KVM_CSR_CFG("sstatus", mstatus, sizeof(uint64_t), RISCV_CSR_REG(sstatus)),
>> +    KVM_CSR_CFG("sie", mie, sizeof(uint64_t), RISCV_CSR_REG(sie)),
>> +    KVM_CSR_CFG("stvec", stvec, sizeof(target_ulong), RISCV_CSR_REG(stvec)),
>> +    KVM_CSR_CFG("sscratch", sscratch, sizeof(target_ulong),
>> +                RISCV_CSR_REG(sscratch)),
>> +    KVM_CSR_CFG("sepc", sepc, sizeof(target_ulong), RISCV_CSR_REG(sepc)),
>> +    KVM_CSR_CFG("scause", scause, sizeof(target_ulong), RISCV_CSR_REG(scause)),
>> +    KVM_CSR_CFG("stval", stval, sizeof(target_ulong), RISCV_CSR_REG(stval)),
>> +    KVM_CSR_CFG("sip", mip, sizeof(uint64_t), RISCV_CSR_REG(sip)),
>> +    KVM_CSR_CFG("satp", satp, sizeof(target_ulong), RISCV_CSR_REG(satp)),
> 
> We don't need to pass in sizeof(env_prop). We can just define KVM_CSR_CFG
> to do it for us.
> 
>   #define KVM_CSR_CFG(_name, csr, reg_id) \
>       { .name = _name, .offset = ENV_CSR_OFFSET(csr), \
>         .prop_size = sizeof(((CPURISCVState *)0)->csr), \
>         .kvm_reg_id = reg_id, }
> 
> But I don't think we need it at all. See below.
> 
>> +};
>> +
>> +static void *kvmconfig_get_env_addr(RISCVCPU *cpu, KVMCPUConfig *csr_cfg)
>> +{
>> +    return (void *)&cpu->env + csr_cfg->offset;
>> +}
>> +
>> +static uint64_t kvm_cpu_csr_get_u32(RISCVCPU *cpu, KVMCPUConfig *csr_cfg)
> 
> This should return a uint32_t.
> 
>> +{
>> +    uint32_t *val32 = kvmconfig_get_env_addr(cpu, csr_cfg);
>> +    return *val32;
>> +}
>> +
>> +static uint64_t kvm_cpu_csr_get_u64(RISCVCPU *cpu, KVMCPUConfig *csr_cfg)
>> +{
>> +    uint64_t *val64 = kvmconfig_get_env_addr(cpu, csr_cfg);
>> +    return *val64;
>> +}
>> +
>> +static void kvm_cpu_csr_set_u32(RISCVCPU *cpu, KVMCPUConfig *csr_cfg,
>> +                                uint32_t val)
>> +{
>> +    uint32_t *val32 = kvmconfig_get_env_addr(cpu, csr_cfg);
>> +    *val32 = val;
>> +}
>> +
>> +static void kvm_cpu_csr_set_u64(RISCVCPU *cpu, KVMCPUConfig *csr_cfg,
>> +                                uint64_t val)
>> +{
>> +    uint64_t *val64 = kvmconfig_get_env_addr(cpu, csr_cfg);
>> +    *val64 = val;
>> +}
>> +
>>   #define KVM_EXT_CFG(_name, _prop, _reg_id) \
>>       {.name = _name, .offset = CPU_CFG_OFFSET(_prop), \
>>        .kvm_reg_id = _reg_id}
>> @@ -598,34 +631,48 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
>>   
>>   static int kvm_riscv_get_regs_csr(CPUState *cs)
>>   {
>> -    CPURISCVState *env = &RISCV_CPU(cs)->env;
>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>> +    uint64_t reg;
>> +    int i, ret;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
>> +        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
>>   
>> -    KVM_RISCV_GET_CSR(cs, env, sstatus, env->mstatus);
>> -    KVM_RISCV_GET_CSR(cs, env, sie, env->mie);
>> -    KVM_RISCV_GET_CSR(cs, env, stvec, env->stvec);
>> -    KVM_RISCV_GET_CSR(cs, env, sscratch, env->sscratch);
>> -    KVM_RISCV_GET_CSR(cs, env, sepc, env->sepc);
>> -    KVM_RISCV_GET_CSR(cs, env, scause, env->scause);
>> -    KVM_RISCV_GET_CSR(cs, env, stval, env->stval);
>> -    KVM_RISCV_GET_CSR(cs, env, sip, env->mip);
>> -    KVM_RISCV_GET_CSR(cs, env, satp, env->satp);
>> +        ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
>> +        if (ret) {
>> +            return ret;
>> +        }
>> +
>> +        if (csr_cfg->prop_size == sizeof(uint32_t)) {
> 
>   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)) {
>       kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
>   } else {
>       uh, oh...
>   }

The idea with this logic is to handle the cases where there is a mismatch
between the size of the KVM reg and the size of the flag we're using to
store it. All CSR regs are of size target_ulong but not all CPURISCVState
flags we're using are target_ulong. We're not storing the size of the KVM
CSR, we're storing the flag size.

E.g:

KVM_CSR_CFG("sstatus", mstatus, sizeof(uint64_t), RISCV_CSR_REG(sstatus)),

For a 32 bit CPU we're writing a 32 bit ulong sstatus into the 64 bit mstatus
field. If we assume that they're the same size we'll be read/writing a 32 bit val
inside a 64 bit field, i.e. we'll be reading/writing the upper words only.

In theory this would be ok if we always read/write the same way but it can be
a nuisance when trying to debug the value inside CPURISCVState.

One may argue whether we should change the type of these fields to match what
Linux/ISA expect of them. Not sure how much work that is.


scounteren is a more serious case because we're writing an ulong KVM reg into
a 32 bit field:

     KVM_CSR_CFG("scounteren", scounteren, sizeof(uint32_t),
                 RISCV_CSR_REG(scounteren)),

struct CPUArchState {
     target_ulong gpr[32];
     (...)
     uint32_t scounteren;
     uint32_t mcounteren;
     (...)


Perhaps it's a good idea to change this CPURISCVState field to ulong before
adding support for the scouteren KVM CSR.


Thanks,

Daniel


> 
>> +            kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
>> +        } else {
>> +            kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
>> +        }
>> +    }
>>   
>>       return 0;
>>   }
>>   
>>   static int kvm_riscv_put_regs_csr(CPUState *cs)
>>   {
>> -    CPURISCVState *env = &RISCV_CPU(cs)->env;
>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>> +    uint64_t reg;
>> +    int i, ret;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
>> +        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
>> +
>> +        if (csr_cfg->prop_size == sizeof(uint32_t)) {
>> +            reg = kvm_cpu_csr_get_u32(cpu, csr_cfg);
>> +        } else {
>> +            reg = kvm_cpu_csr_get_u64(cpu, csr_cfg);
>> +        }
> 
> same comment as above
> 
>>   
>> -    KVM_RISCV_SET_CSR(cs, env, sstatus, env->mstatus);
>> -    KVM_RISCV_SET_CSR(cs, env, sie, env->mie);
>> -    KVM_RISCV_SET_CSR(cs, env, stvec, env->stvec);
>> -    KVM_RISCV_SET_CSR(cs, env, sscratch, env->sscratch);
>> -    KVM_RISCV_SET_CSR(cs, env, sepc, env->sepc);
>> -    KVM_RISCV_SET_CSR(cs, env, scause, env->scause);
>> -    KVM_RISCV_SET_CSR(cs, env, stval, env->stval);
>> -    KVM_RISCV_SET_CSR(cs, env, sip, env->mip);
>> -    KVM_RISCV_SET_CSR(cs, env, satp, env->satp);
>> +        ret = kvm_set_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
>> +        if (ret) {
>> +            return ret;
>> +        }
>> +    }
>>   
>>       return 0;
>>   }
>> -- 
>> 2.49.0
>>
>>
> 
> Thanks,
> drew

Re: [PATCH 4/7] target/riscv/kvm: add kvm_csr_cfgs[]

[Andrew Jones]

On Wed, Apr 23, 2025 at 04:45:29PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 4/23/25 12:15 PM, Andrew Jones wrote:
...
> >   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)) {
> >       kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
> >   } else {
> >       uh, oh...
> >   }
> 
> The idea with this logic is to handle the cases where there is a mismatch
> between the size of the KVM reg and the size of the flag we're using to
> store it. All CSR regs are of size target_ulong but not all CPURISCVState
> flags we're using are target_ulong. We're not storing the size of the KVM
> CSR, we're storing the flag size.
> 
> E.g:
> 
> KVM_CSR_CFG("sstatus", mstatus, sizeof(uint64_t), RISCV_CSR_REG(sstatus)),
> 
> For a 32 bit CPU we're writing a 32 bit ulong sstatus into the 64 bit mstatus
> field. If we assume that they're the same size we'll be read/writing a 32 bit val
> inside a 64 bit field, i.e. we'll be reading/writing the upper words only.

Hmm. I don't think this should be a problem since we're writing the field
offset, which is the low word. A problem may be that we don't clear the
upper word with the write, but it should have been initialized to zero and
never touched.

> 
> In theory this would be ok if we always read/write the same way but it can be
> a nuisance when trying to debug the value inside CPURISCVState.
> 
> One may argue whether we should change the type of these fields to match what
> Linux/ISA expect of them. Not sure how much work that is.

It shouldn't be a problem to use wider fields, we just need to ensure our
framework will be able to write the upper words when necessary, i.e. when
there's an *H counterpart CSR that needs to be written there or when the
upper word should be zeroed out.

> 
> 
> scounteren is a more serious case because we're writing an ulong KVM reg into
> a 32 bit field:

Sigh... it is a problem to use narrower fields... KVM should have defined
that as a __u32.

> 
>     KVM_CSR_CFG("scounteren", scounteren, sizeof(uint32_t),
>                 RISCV_CSR_REG(scounteren)),
> 
> struct CPUArchState {
>     target_ulong gpr[32];
>     (...)
>     uint32_t scounteren;
>     uint32_t mcounteren;
>     (...)
> 
> 
> Perhaps it's a good idea to change this CPURISCVState field to ulong before
> adding support for the scouteren KVM CSR.

Yes, I think we have to now. QEMU's fields need to have widths >= what
KVM says the register sizes are and we should write the amount of bytes
that KVM says we should write (even if we know it's wrong, like in the
case of scounteren). We'll just have to ignore the upper word that
shouldn't be there.

Thanks,
drew

[PATCH 5/7] target/riscv/kvm: do not read unavailable CSRs

[Daniel Henrique Barboza]

[1] reports that commit 4db19d5b21 broke a KVM guest running kernel 6.6.
This happens because the kernel does not know 'senvcfg', making it
unable to boot because QEMU is reading/wriiting it without any checks.

After converting the CSRs to do "automated" get/put reg procedures in
the previous patch we can now scan for availability. Two functions are
created:

- kvm_riscv_read_csr_cfg_legacy() will check if the CSR exists by brute
  forcing KVM_GET_ONE_REG in each one of them, interpreting an EINVAL
  return as indication that the CSR isn't available. This will be use in
  absence of KVM_GET_REG_LIST;

- kvm_riscv_read_csr_cfg() will use the existing result of get_reg_list
  to check if the CSRs ids are present.

kvm_riscv_init_multiext_cfg() is now kvm_riscv_init_multiext_csr_cfg()
to reflect that the function is also dealing with CSRs.

[1] https://lore.kernel.org/qemu-riscv/CABJz62OfUDHYkQ0T3rGHStQprf1c7_E0qBLbLKhfv=+jb0SYAw@mail.gmail.com/

Fixes: 4db19d5b21 ("target/riscv/kvm: add missing KVM CSRs")
Reported-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 63 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 3 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 99a4f01b15..ec74520872 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -638,6 +638,10 @@ static int kvm_riscv_get_regs_csr(CPUState *cs)
     for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
         KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
 
+        if (!csr_cfg->supported) {
+            continue;
+        }
+
         ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
         if (ret) {
             return ret;
@@ -662,6 +666,10 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
     for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
         KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
 
+        if (!csr_cfg->supported) {
+            continue;
+        }
+
         if (csr_cfg->prop_size == sizeof(uint32_t)) {
             reg = kvm_cpu_csr_get_u32(cpu, csr_cfg);
         } else {
@@ -1088,6 +1096,32 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
     }
 }
 
+static void kvm_riscv_read_csr_cfg_legacy(KVMScratchCPU *kvmcpu)
+{
+    uint64_t val;
+    int i, ret;
+
+    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
+        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
+        struct kvm_one_reg reg;
+
+        reg.id = csr_cfg->kvm_reg_id;
+        reg.addr = (uint64_t)&val;
+        ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
+        if (ret != 0) {
+            if (errno == EINVAL) {
+                csr_cfg->supported = false;
+            } else {
+                error_report("Unable to read KVM CSR %s: %s",
+                             csr_cfg->name, strerror(errno));
+                exit(EXIT_FAILURE);
+            }
+        } else {
+            csr_cfg->supported = true;
+        }
+    }
+}
+
 static int uint64_cmp(const void *a, const void *b)
 {
     uint64_t val1 = *(const uint64_t *)a;
@@ -1144,7 +1178,27 @@ static void kvm_riscv_read_vlenb(RISCVCPU *cpu, KVMScratchCPU *kvmcpu,
     }
 }
 
-static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
+static void kvm_riscv_read_csr_cfg(struct kvm_reg_list *reglist)
+{
+    struct kvm_reg_list *reg_search;
+    uint64_t reg_id;
+
+    for (int i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
+        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
+
+        reg_id = csr_cfg->kvm_reg_id;
+        reg_search = bsearch(&reg_id, reglist->reg, reglist->n,
+                             sizeof(uint64_t), uint64_cmp);
+        if (!reg_search) {
+            continue;
+        }
+
+        csr_cfg->supported = true;
+    }
+}
+
+static void kvm_riscv_init_multiext_csr_cfg(RISCVCPU *cpu,
+                                            KVMScratchCPU *kvmcpu)
 {
     KVMCPUConfig *multi_ext_cfg;
     struct kvm_one_reg reg;
@@ -1161,7 +1215,9 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
      * (EINVAL). Use read_legacy() in this case.
      */
     if (errno == EINVAL) {
-        return kvm_riscv_read_multiext_legacy(cpu, kvmcpu);
+        kvm_riscv_read_multiext_legacy(cpu, kvmcpu);
+        kvm_riscv_read_csr_cfg_legacy(kvmcpu);
+        return;
     } else if (errno != E2BIG) {
         /*
          * E2BIG is an expected error message for the API since we
@@ -1224,6 +1280,7 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
     }
 
     kvm_riscv_check_sbi_dbcn_support(cpu, reglist);
+    kvm_riscv_read_csr_cfg(reglist);
 }
 
 static void riscv_init_kvm_registers(Object *cpu_obj)
@@ -1237,7 +1294,7 @@ static void riscv_init_kvm_registers(Object *cpu_obj)
 
     kvm_riscv_init_machine_ids(cpu, &kvmcpu);
     kvm_riscv_init_misa_ext_mask(cpu, &kvmcpu);
-    kvm_riscv_init_multiext_cfg(cpu, &kvmcpu);
+    kvm_riscv_init_multiext_csr_cfg(cpu, &kvmcpu);
 
     kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
 }
-- 
2.49.0

Re: [PATCH 5/7] target/riscv/kvm: do not read unavailable CSRs

[Andrew Jones]

On Thu, Apr 17, 2025 at 09:48:37AM -0300, Daniel Henrique Barboza wrote:
> [1] reports that commit 4db19d5b21 broke a KVM guest running kernel 6.6.
> This happens because the kernel does not know 'senvcfg', making it
> unable to boot because QEMU is reading/wriiting it without any checks.
> 
> After converting the CSRs to do "automated" get/put reg procedures in
> the previous patch we can now scan for availability. Two functions are
> created:
> 
> - kvm_riscv_read_csr_cfg_legacy() will check if the CSR exists by brute
>   forcing KVM_GET_ONE_REG in each one of them, interpreting an EINVAL
>   return as indication that the CSR isn't available. This will be use in
>   absence of KVM_GET_REG_LIST;
> 
> - kvm_riscv_read_csr_cfg() will use the existing result of get_reg_list
>   to check if the CSRs ids are present.
> 
> kvm_riscv_init_multiext_cfg() is now kvm_riscv_init_multiext_csr_cfg()
> to reflect that the function is also dealing with CSRs.
> 
> [1] https://lore.kernel.org/qemu-riscv/CABJz62OfUDHYkQ0T3rGHStQprf1c7_E0qBLbLKhfv=+jb0SYAw@mail.gmail.com/
> 
> Fixes: 4db19d5b21 ("target/riscv/kvm: add missing KVM CSRs")
> Reported-by: Andrea Bolognani <abologna@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/kvm/kvm-cpu.c | 63 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 60 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 99a4f01b15..ec74520872 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -638,6 +638,10 @@ static int kvm_riscv_get_regs_csr(CPUState *cs)
>      for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
>          KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
>  
> +        if (!csr_cfg->supported) {
> +            continue;
> +        }
> +
>          ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
>          if (ret) {
>              return ret;
> @@ -662,6 +666,10 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
>      for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
>          KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
>  
> +        if (!csr_cfg->supported) {
> +            continue;
> +        }
> +
>          if (csr_cfg->prop_size == sizeof(uint32_t)) {
>              reg = kvm_cpu_csr_get_u32(cpu, csr_cfg);
>          } else {
> @@ -1088,6 +1096,32 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
>      }
>  }
>  
> +static void kvm_riscv_read_csr_cfg_legacy(KVMScratchCPU *kvmcpu)
> +{
> +    uint64_t val;
> +    int i, ret;
> +
> +    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
> +        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
> +        struct kvm_one_reg reg;
> +
> +        reg.id = csr_cfg->kvm_reg_id;
> +        reg.addr = (uint64_t)&val;
> +        ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
> +        if (ret != 0) {
> +            if (errno == EINVAL) {
> +                csr_cfg->supported = false;
> +            } else {
> +                error_report("Unable to read KVM CSR %s: %s",
> +                             csr_cfg->name, strerror(errno));
> +                exit(EXIT_FAILURE);
> +            }
> +        } else {
> +            csr_cfg->supported = true;
> +        }
> +    }
> +}
> +
>  static int uint64_cmp(const void *a, const void *b)
>  {
>      uint64_t val1 = *(const uint64_t *)a;
> @@ -1144,7 +1178,27 @@ static void kvm_riscv_read_vlenb(RISCVCPU *cpu, KVMScratchCPU *kvmcpu,
>      }
>  }
>  
> -static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
> +static void kvm_riscv_read_csr_cfg(struct kvm_reg_list *reglist)
> +{
> +    struct kvm_reg_list *reg_search;
> +    uint64_t reg_id;
> +
> +    for (int i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
> +        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
> +
> +        reg_id = csr_cfg->kvm_reg_id;
> +        reg_search = bsearch(&reg_id, reglist->reg, reglist->n,
> +                             sizeof(uint64_t), uint64_cmp);
> +        if (!reg_search) {
> +            continue;
> +        }
> +
> +        csr_cfg->supported = true;
> +    }
> +}
> +
> +static void kvm_riscv_init_multiext_csr_cfg(RISCVCPU *cpu,
> +                                            KVMScratchCPU *kvmcpu)

I'm not sure we want to keep extending the name of this function with each
thing it does (and it does SBI as well, which isn't in the name). Maybe
just shorten it instead to kvm_riscv_init_cfg()?

>  {
>      KVMCPUConfig *multi_ext_cfg;
>      struct kvm_one_reg reg;
> @@ -1161,7 +1215,9 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>       * (EINVAL). Use read_legacy() in this case.
>       */
>      if (errno == EINVAL) {
> -        return kvm_riscv_read_multiext_legacy(cpu, kvmcpu);
> +        kvm_riscv_read_multiext_legacy(cpu, kvmcpu);
> +        kvm_riscv_read_csr_cfg_legacy(kvmcpu);
> +        return;
>      } else if (errno != E2BIG) {
>          /*
>           * E2BIG is an expected error message for the API since we
> @@ -1224,6 +1280,7 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>      }
>  
>      kvm_riscv_check_sbi_dbcn_support(cpu, reglist);
> +    kvm_riscv_read_csr_cfg(reglist);

Shouldn't there be a g_free(reglist) here?

>  }
>  
>  static void riscv_init_kvm_registers(Object *cpu_obj)
> @@ -1237,7 +1294,7 @@ static void riscv_init_kvm_registers(Object *cpu_obj)
>  
>      kvm_riscv_init_machine_ids(cpu, &kvmcpu);
>      kvm_riscv_init_misa_ext_mask(cpu, &kvmcpu);
> -    kvm_riscv_init_multiext_cfg(cpu, &kvmcpu);
> +    kvm_riscv_init_multiext_csr_cfg(cpu, &kvmcpu);
>  
>      kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
>  }
> -- 
> 2.49.0
> 
> 

Thanks,
drew

Re: [PATCH 5/7] target/riscv/kvm: do not read unavailable CSRs

[Daniel Henrique Barboza]

On 4/23/25 12:25 PM, Andrew Jones wrote:
> On Thu, Apr 17, 2025 at 09:48:37AM -0300, Daniel Henrique Barboza wrote:
>> [1] reports that commit 4db19d5b21 broke a KVM guest running kernel 6.6.
>> This happens because the kernel does not know 'senvcfg', making it
>> unable to boot because QEMU is reading/wriiting it without any checks.
>>
>> After converting the CSRs to do "automated" get/put reg procedures in
>> the previous patch we can now scan for availability. Two functions are
>> created:
>>
>> - kvm_riscv_read_csr_cfg_legacy() will check if the CSR exists by brute
>>    forcing KVM_GET_ONE_REG in each one of them, interpreting an EINVAL
>>    return as indication that the CSR isn't available. This will be use in
>>    absence of KVM_GET_REG_LIST;
>>
>> - kvm_riscv_read_csr_cfg() will use the existing result of get_reg_list
>>    to check if the CSRs ids are present.
>>
>> kvm_riscv_init_multiext_cfg() is now kvm_riscv_init_multiext_csr_cfg()
>> to reflect that the function is also dealing with CSRs.
>>
>> [1] https://lore.kernel.org/qemu-riscv/CABJz62OfUDHYkQ0T3rGHStQprf1c7_E0qBLbLKhfv=+jb0SYAw@mail.gmail.com/
>>
>> Fixes: 4db19d5b21 ("target/riscv/kvm: add missing KVM CSRs")
>> Reported-by: Andrea Bolognani <abologna@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/kvm/kvm-cpu.c | 63 ++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 60 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
>> index 99a4f01b15..ec74520872 100644
>> --- a/target/riscv/kvm/kvm-cpu.c
>> +++ b/target/riscv/kvm/kvm-cpu.c
>> @@ -638,6 +638,10 @@ static int kvm_riscv_get_regs_csr(CPUState *cs)
>>       for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
>>           KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
>>   
>> +        if (!csr_cfg->supported) {
>> +            continue;
>> +        }
>> +
>>           ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
>>           if (ret) {
>>               return ret;
>> @@ -662,6 +666,10 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
>>       for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
>>           KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
>>   
>> +        if (!csr_cfg->supported) {
>> +            continue;
>> +        }
>> +
>>           if (csr_cfg->prop_size == sizeof(uint32_t)) {
>>               reg = kvm_cpu_csr_get_u32(cpu, csr_cfg);
>>           } else {
>> @@ -1088,6 +1096,32 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
>>       }
>>   }
>>   
>> +static void kvm_riscv_read_csr_cfg_legacy(KVMScratchCPU *kvmcpu)
>> +{
>> +    uint64_t val;
>> +    int i, ret;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
>> +        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
>> +        struct kvm_one_reg reg;
>> +
>> +        reg.id = csr_cfg->kvm_reg_id;
>> +        reg.addr = (uint64_t)&val;
>> +        ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
>> +        if (ret != 0) {
>> +            if (errno == EINVAL) {
>> +                csr_cfg->supported = false;
>> +            } else {
>> +                error_report("Unable to read KVM CSR %s: %s",
>> +                             csr_cfg->name, strerror(errno));
>> +                exit(EXIT_FAILURE);
>> +            }
>> +        } else {
>> +            csr_cfg->supported = true;
>> +        }
>> +    }
>> +}
>> +
>>   static int uint64_cmp(const void *a, const void *b)
>>   {
>>       uint64_t val1 = *(const uint64_t *)a;
>> @@ -1144,7 +1178,27 @@ static void kvm_riscv_read_vlenb(RISCVCPU *cpu, KVMScratchCPU *kvmcpu,
>>       }
>>   }
>>   
>> -static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>> +static void kvm_riscv_read_csr_cfg(struct kvm_reg_list *reglist)
>> +{
>> +    struct kvm_reg_list *reg_search;
>> +    uint64_t reg_id;
>> +
>> +    for (int i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
>> +        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
>> +
>> +        reg_id = csr_cfg->kvm_reg_id;
>> +        reg_search = bsearch(&reg_id, reglist->reg, reglist->n,
>> +                             sizeof(uint64_t), uint64_cmp);
>> +        if (!reg_search) {
>> +            continue;
>> +        }
>> +
>> +        csr_cfg->supported = true;
>> +    }
>> +}
>> +
>> +static void kvm_riscv_init_multiext_csr_cfg(RISCVCPU *cpu,
>> +                                            KVMScratchCPU *kvmcpu)
> 
> I'm not sure we want to keep extending the name of this function with each
> thing it does (and it does SBI as well, which isn't in the name). Maybe
> just shorten it instead to kvm_riscv_init_cfg()?


Yeah, I'll rename it in v2.

> 
>>   {
>>       KVMCPUConfig *multi_ext_cfg;
>>       struct kvm_one_reg reg;
>> @@ -1161,7 +1215,9 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>>        * (EINVAL). Use read_legacy() in this case.
>>        */
>>       if (errno == EINVAL) {
>> -        return kvm_riscv_read_multiext_legacy(cpu, kvmcpu);
>> +        kvm_riscv_read_multiext_legacy(cpu, kvmcpu);
>> +        kvm_riscv_read_csr_cfg_legacy(kvmcpu);
>> +        return;
>>       } else if (errno != E2BIG) {
>>           /*
>>            * E2BIG is an expected error message for the API since we
>> @@ -1224,6 +1280,7 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>>       }
>>   
>>       kvm_riscv_check_sbi_dbcn_support(cpu, reglist);
>> +    kvm_riscv_read_csr_cfg(reglist);
> 
> Shouldn't there be a g_free(reglist) here?

It should. It seems we're leaking 'reglist'. I'll fix it in v2. Thanks,


Daniel


> 
>>   }
>>   
>>   static void riscv_init_kvm_registers(Object *cpu_obj)
>> @@ -1237,7 +1294,7 @@ static void riscv_init_kvm_registers(Object *cpu_obj)
>>   
>>       kvm_riscv_init_machine_ids(cpu, &kvmcpu);
>>       kvm_riscv_init_misa_ext_mask(cpu, &kvmcpu);
>> -    kvm_riscv_init_multiext_cfg(cpu, &kvmcpu);
>> +    kvm_riscv_init_multiext_csr_cfg(cpu, &kvmcpu);
>>   
>>       kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
>>   }
>> -- 
>> 2.49.0
>>
>>
> 
> Thanks,
> drew

[PATCH 6/7] target/riscv/kvm: add missing KVM CSRs

[Daniel Henrique Barboza]

We're missing scounteren and senvcfg CSRs, both already present in the
KVM UAPI.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/kvm/kvm-cpu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index ec74520872..a91a87b175 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -251,6 +251,11 @@ static KVMCPUConfig kvm_csr_cfgs[] = {
     KVM_CSR_CFG("stval", stval, sizeof(target_ulong), RISCV_CSR_REG(stval)),
     KVM_CSR_CFG("sip", mip, sizeof(uint64_t), RISCV_CSR_REG(sip)),
     KVM_CSR_CFG("satp", satp, sizeof(target_ulong), RISCV_CSR_REG(satp)),
+    KVM_CSR_CFG("scounteren", scounteren, sizeof(uint32_t),
+                RISCV_CSR_REG(scounteren)),
+    KVM_CSR_CFG("senvcfg", senvcfg, sizeof(target_ulong),
+                RISCV_CSR_REG(senvcfg)),
+
 };
 
 static void *kvmconfig_get_env_addr(RISCVCPU *cpu, KVMCPUConfig *csr_cfg)
@@ -696,6 +701,8 @@ static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
     env->stval = 0;
     env->mip = 0;
     env->satp = 0;
+    env->scounteren = 0;
+    env->senvcfg = 0;
 }
 
 static int kvm_riscv_get_regs_fp(CPUState *cs)
-- 
2.49.0

Re: [PATCH 6/7] target/riscv/kvm: add missing KVM CSRs

[Andrew Jones]

On Thu, Apr 17, 2025 at 09:48:38AM -0300, Daniel Henrique Barboza wrote:
> We're missing scounteren and senvcfg CSRs, both already present in the
> KVM UAPI.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

The patch changed enough that it's not an r-b anymore, but it probably
should have been a reported-by in the first place anyway.

> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/kvm/kvm-cpu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index ec74520872..a91a87b175 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -251,6 +251,11 @@ static KVMCPUConfig kvm_csr_cfgs[] = {
>      KVM_CSR_CFG("stval", stval, sizeof(target_ulong), RISCV_CSR_REG(stval)),
>      KVM_CSR_CFG("sip", mip, sizeof(uint64_t), RISCV_CSR_REG(sip)),
>      KVM_CSR_CFG("satp", satp, sizeof(target_ulong), RISCV_CSR_REG(satp)),
> +    KVM_CSR_CFG("scounteren", scounteren, sizeof(uint32_t),
> +                RISCV_CSR_REG(scounteren)),
> +    KVM_CSR_CFG("senvcfg", senvcfg, sizeof(target_ulong),
> +                RISCV_CSR_REG(senvcfg)),
> +

Extra blank here and the sizeof() stuff should go away.

>  };
>  
>  static void *kvmconfig_get_env_addr(RISCVCPU *cpu, KVMCPUConfig *csr_cfg)
> @@ -696,6 +701,8 @@ static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
>      env->stval = 0;
>      env->mip = 0;
>      env->satp = 0;
> +    env->scounteren = 0;
> +    env->senvcfg = 0;
>  }
>  
>  static int kvm_riscv_get_regs_fp(CPUState *cs)
> -- 
> 2.49.0
>

Thanks,
drew

[PATCH 7/7] target/riscv/kvm: reset 'scounteren' with host val

[Daniel Henrique Barboza]

The Linux kernel, up until at least version 6.12, has issues with a KVM
guest setting scounteren to 0 during reset. No error will be thrown
during boot, but trying to use rdtime in the guest (by executing 'ping'
for example) will result in a stack trace and an illegal instruction
error:

/ # ping 8.8.8.8
PING 3.33.130.190 (8.8.8.8): 56 data bytes
[   19.464484] ping[56]: unhandled signal 4 code 0x1 at 0x00007fffae0665f4
[   19.493332] CPU: 0 PID: 56 Comm: ping Not tainted 6.9.0-rc3-dbarboza #7
[   19.523249] Hardware name: riscv-virtio,qemu (DT)
[   19.546641] epc : 00007fffae0665f4 ra : 00000000000c6316 sp : 00007fffc7cfd9f0
[   19.576214]  gp : 0000000000172408 tp : 00000000001767a0 t0 : 0000000000000000
[   19.606719]  t1 : 0000000000000020 t2 : 0000000000000000 s0 : 00007fffc7cfda00
[   19.640938]  s1 : 00007fffc7cfda30 a0 : 0000000000000001 a1 : 00007fffc7cfda30
[   19.676698]  a2 : 0000000000000000 a3 : 00000000000009ee a4 : 00007fffae064000
[   19.721036]  a5 : 0000000000000001 a6 : 0000000000000000 a7 : 00000000001784d0
[   19.765061]  s2 : 00000000001784d0 s3 : 000000000011ca38 s4 : 000000000011d000
[   19.801985]  s5 : 0000000000000001 s6 : 0000000000000000 s7 : 0000000000000000
[   19.841235]  s8 : 0000000000177788 s9 : 0000000000176828 s10: 0000000000000000
[   19.882479]  s11: 00000000001777a8 t3 : ffffffffffffffff t4 : 0000000000172f60
[   19.923651]  t5 : 0000000000000020 t6 : 000000000000000a
[   19.950491] status: 0000000200004020 badaddr: 00000000c01027f3 cause: 0000000000000002
[   19.995864] Code: 431c f613 0017 869b 0007 ea59 000f 0220 435c cfbd (27f3) c010
Illegal instruction
/ #

Reading the host scounteren val and using it during reset, instead of
zeroing it, will prevent this error. It is worth noting that scounteren
is a WARL reg according to the RISC-V ISA spec, and in theory the kernel
should accept a zero val and convert it to something that won't break
the guest.

We can't tell from userspace if the host kernel handles scounteren = 0
during reset accordingly, so to prevent this error we'll assume that it
won't. Read the reg from the host and use it as reset value.

We'll end up repeating code from kvm_riscv_get_regs_csr() to do that.
Create a helper that will get a single CSR and use it in get_regs_csr()
and in kvm_riscv_reset_regs_csr() to avoid code duplication.

Fixes: 4db19d5b21 ("target/riscv/kvm: add missing KVM CSRs")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 73 ++++++++++++++++++++++++++++----------
 1 file changed, 55 insertions(+), 18 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index a91a87b175..918fe51257 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -634,29 +634,40 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
     return ret;
 }
 
-static int kvm_riscv_get_regs_csr(CPUState *cs)
+static int kvm_riscv_get_reg_csr(CPUState *cs,
+                                 KVMCPUConfig *csr_cfg)
 {
     RISCVCPU *cpu = RISCV_CPU(cs);
     uint64_t reg;
-    int i, ret;
+    int ret;
 
-    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
-        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
+    if (!csr_cfg->supported) {
+        return 0;
+    }
 
-        if (!csr_cfg->supported) {
-            continue;
-        }
+    ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    if (csr_cfg->prop_size == sizeof(uint32_t)) {
+        kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
+    } else {
+        kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
+    }
+
+    return 0;
+}
+
+static int kvm_riscv_get_regs_csr(CPUState *cs)
+{
+    int i, ret;
 
-        ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
+    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
+        ret = kvm_riscv_get_reg_csr(cs, &kvm_csr_cfgs[i]);
         if (ret) {
             return ret;
         }
-
-        if (csr_cfg->prop_size == sizeof(uint32_t)) {
-            kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
-        } else {
-            kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
-        }
     }
 
     return 0;
@@ -690,8 +701,11 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
     return 0;
 }
 
-static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
+static void kvm_riscv_reset_regs_csr(CPUState *cs, CPURISCVState *env)
 {
+    uint64_t scounteren_kvm_id = RISCV_CSR_REG(scounteren);
+    int ret;
+
     env->mstatus = 0;
     env->mie = 0;
     env->stvec = 0;
@@ -701,8 +715,30 @@ static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
     env->stval = 0;
     env->mip = 0;
     env->satp = 0;
-    env->scounteren = 0;
     env->senvcfg = 0;
+
+    /*
+     * Some kernels will take issue with env->scounteren = 0
+     * causing problems inside the KVM guest with 'rdtime'.
+     * Read 'scounteren' from the host and use it.
+     */
+    for (int i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
+        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
+
+        if (csr_cfg->kvm_reg_id != scounteren_kvm_id) {
+            continue;
+        }
+
+        if (!csr_cfg->supported) {
+            break;
+        }
+
+        ret = kvm_riscv_get_reg_csr(cs, &kvm_csr_cfgs[i]);
+        if (ret) {
+            error_report("Unable to retrieve scounteren from host ,"
+                         "error %d", ret);
+        }
+    }
 }
 
 static int kvm_riscv_get_regs_fp(CPUState *cs)
@@ -1711,16 +1747,17 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
 void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
 {
     CPURISCVState *env = &cpu->env;
+    CPUState *cs = CPU(cpu);
     int i;
 
     for (i = 0; i < 32; i++) {
         env->gpr[i] = 0;
     }
     env->pc = cpu->env.kernel_addr;
-    env->gpr[10] = kvm_arch_vcpu_id(CPU(cpu)); /* a0 */
+    env->gpr[10] = kvm_arch_vcpu_id(cs);       /* a0 */
     env->gpr[11] = cpu->env.fdt_addr;          /* a1 */
 
-    kvm_riscv_reset_regs_csr(env);
+    kvm_riscv_reset_regs_csr(cs, env);
 }
 
 void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
-- 
2.49.0

Re: [PATCH 7/7] target/riscv/kvm: reset 'scounteren' with host val

[Andrew Jones]

On Thu, Apr 17, 2025 at 09:48:39AM -0300, Daniel Henrique Barboza wrote:
> The Linux kernel, up until at least version 6.12, has issues with a KVM
> guest setting scounteren to 0 during reset. No error will be thrown
> during boot, but trying to use rdtime in the guest (by executing 'ping'
> for example) will result in a stack trace and an illegal instruction
> error:
> 
> / # ping 8.8.8.8
> PING 3.33.130.190 (8.8.8.8): 56 data bytes
> [   19.464484] ping[56]: unhandled signal 4 code 0x1 at 0x00007fffae0665f4
> [   19.493332] CPU: 0 PID: 56 Comm: ping Not tainted 6.9.0-rc3-dbarboza #7
> [   19.523249] Hardware name: riscv-virtio,qemu (DT)
> [   19.546641] epc : 00007fffae0665f4 ra : 00000000000c6316 sp : 00007fffc7cfd9f0
> [   19.576214]  gp : 0000000000172408 tp : 00000000001767a0 t0 : 0000000000000000
> [   19.606719]  t1 : 0000000000000020 t2 : 0000000000000000 s0 : 00007fffc7cfda00
> [   19.640938]  s1 : 00007fffc7cfda30 a0 : 0000000000000001 a1 : 00007fffc7cfda30
> [   19.676698]  a2 : 0000000000000000 a3 : 00000000000009ee a4 : 00007fffae064000
> [   19.721036]  a5 : 0000000000000001 a6 : 0000000000000000 a7 : 00000000001784d0
> [   19.765061]  s2 : 00000000001784d0 s3 : 000000000011ca38 s4 : 000000000011d000
> [   19.801985]  s5 : 0000000000000001 s6 : 0000000000000000 s7 : 0000000000000000
> [   19.841235]  s8 : 0000000000177788 s9 : 0000000000176828 s10: 0000000000000000
> [   19.882479]  s11: 00000000001777a8 t3 : ffffffffffffffff t4 : 0000000000172f60
> [   19.923651]  t5 : 0000000000000020 t6 : 000000000000000a
> [   19.950491] status: 0000000200004020 badaddr: 00000000c01027f3 cause: 0000000000000002
> [   19.995864] Code: 431c f613 0017 869b 0007 ea59 000f 0220 435c cfbd (27f3) c010
> Illegal instruction
> / #
> 
> Reading the host scounteren val and using it during reset, instead of
> zeroing it, will prevent this error. It is worth noting that scounteren
> is a WARL reg according to the RISC-V ISA spec, and in theory the kernel
> should accept a zero val and convert it to something that won't break
> the guest.

0 is legal, so the kernel (KVM) shouldn't change what userspace selects.
Userspace, which includes user policy knowledge, knows best and indeed 0
is the best selection when no other policy is provided. Changing from 0
to whatever KVM has put there is wrong.

> 
> We can't tell from userspace if the host kernel handles scounteren = 0
> during reset accordingly, so to prevent this error we'll assume that it
> won't. Read the reg from the host and use it as reset value.

It's not the host kernel that needs to change how it handles things. It's
the guest kernel. When the guest uses ping, which likely calls gtod, which
uses rdtime, or if the guest uses anything that results in an rdtime use,
then it'll hit this issue if the host doesn't support sscofpmf (which the
QEMU rv64 cpu type doesn't). The reason is because KVM won't expose the
SBI PMU without sscofpmf and then the guest kernel pmu driver won't run,
and currently the only place scounteren is getting set up is in the pmu
driver. The guest kernel should unconditionally set up scounteren to
match what it expects userspace to use (like enable rdtime, since the
guest kernel actually implements gtod in vdso with it)

> 
> We'll end up repeating code from kvm_riscv_get_regs_csr() to do that.
> Create a helper that will get a single CSR and use it in get_regs_csr()
> and in kvm_riscv_reset_regs_csr() to avoid code duplication.
> 
> Fixes: 4db19d5b21 ("target/riscv/kvm: add missing KVM CSRs")

This isn't the right tag since that is already fixed by checking
get-reg-list in a previous patch. This patch is trying to fix a guest
kernel bug, which it shouldn't be doing, at least not without some user
toggle allowing the workaround to be turned on/off.

> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/kvm/kvm-cpu.c | 73 ++++++++++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 18 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index a91a87b175..918fe51257 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -634,29 +634,40 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
>      return ret;
>  }
>  
> -static int kvm_riscv_get_regs_csr(CPUState *cs)
> +static int kvm_riscv_get_reg_csr(CPUState *cs,
> +                                 KVMCPUConfig *csr_cfg)
>  {
>      RISCVCPU *cpu = RISCV_CPU(cs);
>      uint64_t reg;
> -    int i, ret;
> +    int ret;
>  
> -    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
> -        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
> +    if (!csr_cfg->supported) {
> +        return 0;
> +    }
>  
> -        if (!csr_cfg->supported) {
> -            continue;
> -        }
> +    ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    if (csr_cfg->prop_size == sizeof(uint32_t)) {
> +        kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
> +    } else {
> +        kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
> +    }
> +
> +    return 0;
> +}
> +
> +static int kvm_riscv_get_regs_csr(CPUState *cs)
> +{
> +    int i, ret;
>  
> -        ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
> +    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
> +        ret = kvm_riscv_get_reg_csr(cs, &kvm_csr_cfgs[i]);
>          if (ret) {
>              return ret;
>          }
> -
> -        if (csr_cfg->prop_size == sizeof(uint32_t)) {
> -            kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
> -        } else {
> -            kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
> -        }
>      }
>  
>      return 0;
> @@ -690,8 +701,11 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
>      return 0;
>  }
>  
> -static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
> +static void kvm_riscv_reset_regs_csr(CPUState *cs, CPURISCVState *env)
>  {
> +    uint64_t scounteren_kvm_id = RISCV_CSR_REG(scounteren);
> +    int ret;
> +
>      env->mstatus = 0;
>      env->mie = 0;
>      env->stvec = 0;
> @@ -701,8 +715,30 @@ static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
>      env->stval = 0;
>      env->mip = 0;
>      env->satp = 0;
> -    env->scounteren = 0;
>      env->senvcfg = 0;
> +
> +    /*
> +     * Some kernels will take issue with env->scounteren = 0
> +     * causing problems inside the KVM guest with 'rdtime'.
> +     * Read 'scounteren' from the host and use it.
> +     */
> +    for (int i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
> +        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
> +
> +        if (csr_cfg->kvm_reg_id != scounteren_kvm_id) {
> +            continue;
> +        }
> +
> +        if (!csr_cfg->supported) {
> +            break;
> +        }
> +
> +        ret = kvm_riscv_get_reg_csr(cs, &kvm_csr_cfgs[i]);
> +        if (ret) {
> +            error_report("Unable to retrieve scounteren from host ,"
> +                         "error %d", ret);
> +        }
> +    }
>  }
>  
>  static int kvm_riscv_get_regs_fp(CPUState *cs)
> @@ -1711,16 +1747,17 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>  void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
>  {
>      CPURISCVState *env = &cpu->env;
> +    CPUState *cs = CPU(cpu);
>      int i;
>  
>      for (i = 0; i < 32; i++) {
>          env->gpr[i] = 0;
>      }
>      env->pc = cpu->env.kernel_addr;
> -    env->gpr[10] = kvm_arch_vcpu_id(CPU(cpu)); /* a0 */
> +    env->gpr[10] = kvm_arch_vcpu_id(cs);       /* a0 */
>      env->gpr[11] = cpu->env.fdt_addr;          /* a1 */
>  
> -    kvm_riscv_reset_regs_csr(env);
> +    kvm_riscv_reset_regs_csr(cs, env);
>  }
>  
>  void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
> -- 
> 2.49.0
> 
>

I would just drop this patch and make the default 'virt' cpu type 'max',
then nobody will hit the issue. We should also fix the [guest] kernel,
which I'll try to do soon.

Thanks,
drew

Re: [PATCH 7/7] target/riscv/kvm: reset 'scounteren' with host val

[Andrea Bolognani]

On Wed, Apr 23, 2025 at 05:46:16PM +0200, Andrew Jones wrote:
> I would just drop this patch and make the default 'virt' cpu type 'max',
> then nobody will hit the issue.

FWIW virt-manager has recently started doing just that:

  https://github.com/virt-manager/virt-manager/pull/784

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 7/7] target/riscv/kvm: reset 'scounteren' with host val

[Daniel Henrique Barboza]

On 4/23/25 3:06 PM, Andrea Bolognani wrote:
> On Wed, Apr 23, 2025 at 05:46:16PM +0200, Andrew Jones wrote:
>> I would just drop this patch and make the default 'virt' cpu type 'max',
>> then nobody will hit the issue.
> 
> FWIW virt-manager has recently started doing just that:
> 
>    https://github.com/virt-manager/virt-manager/pull/784
> 

We have a patch in the QEMU ML that does that:

"[PATCH 2/2] hw/riscv/virt.c: change default CPU to 'max'"

https://lore.kernel.org/qemu-riscv/20250404152750.332791-3-dbarboza@ventanamicro.com/


Any updates in that patch is appreciated, in particular if it comes from
the tooling side.


Thanks,

Daniel

Re: [PATCH 7/7] target/riscv/kvm: reset 'scounteren' with host val

[Daniel Henrique Barboza]

On 4/23/25 12:46 PM, Andrew Jones wrote:
> On Thu, Apr 17, 2025 at 09:48:39AM -0300, Daniel Henrique Barboza wrote:
>> The Linux kernel, up until at least version 6.12, has issues with a KVM
>> guest setting scounteren to 0 during reset. No error will be thrown
>> during boot, but trying to use rdtime in the guest (by executing 'ping'
>> for example) will result in a stack trace and an illegal instruction
>> error:
>>
>> / # ping 8.8.8.8
>> PING 3.33.130.190 (8.8.8.8): 56 data bytes
>> [   19.464484] ping[56]: unhandled signal 4 code 0x1 at 0x00007fffae0665f4
>> [   19.493332] CPU: 0 PID: 56 Comm: ping Not tainted 6.9.0-rc3-dbarboza #7
>> [   19.523249] Hardware name: riscv-virtio,qemu (DT)
>> [   19.546641] epc : 00007fffae0665f4 ra : 00000000000c6316 sp : 00007fffc7cfd9f0
>> [   19.576214]  gp : 0000000000172408 tp : 00000000001767a0 t0 : 0000000000000000
>> [   19.606719]  t1 : 0000000000000020 t2 : 0000000000000000 s0 : 00007fffc7cfda00
>> [   19.640938]  s1 : 00007fffc7cfda30 a0 : 0000000000000001 a1 : 00007fffc7cfda30
>> [   19.676698]  a2 : 0000000000000000 a3 : 00000000000009ee a4 : 00007fffae064000
>> [   19.721036]  a5 : 0000000000000001 a6 : 0000000000000000 a7 : 00000000001784d0
>> [   19.765061]  s2 : 00000000001784d0 s3 : 000000000011ca38 s4 : 000000000011d000
>> [   19.801985]  s5 : 0000000000000001 s6 : 0000000000000000 s7 : 0000000000000000
>> [   19.841235]  s8 : 0000000000177788 s9 : 0000000000176828 s10: 0000000000000000
>> [   19.882479]  s11: 00000000001777a8 t3 : ffffffffffffffff t4 : 0000000000172f60
>> [   19.923651]  t5 : 0000000000000020 t6 : 000000000000000a
>> [   19.950491] status: 0000000200004020 badaddr: 00000000c01027f3 cause: 0000000000000002
>> [   19.995864] Code: 431c f613 0017 869b 0007 ea59 000f 0220 435c cfbd (27f3) c010
>> Illegal instruction
>> / #
>>
>> Reading the host scounteren val and using it during reset, instead of
>> zeroing it, will prevent this error. It is worth noting that scounteren
>> is a WARL reg according to the RISC-V ISA spec, and in theory the kernel
>> should accept a zero val and convert it to something that won't break
>> the guest.
> 
> 0 is legal, so the kernel (KVM) shouldn't change what userspace selects.
> Userspace, which includes user policy knowledge, knows best and indeed 0
> is the best selection when no other policy is provided. Changing from 0
> to whatever KVM has put there is wrong.
> 
>>
>> We can't tell from userspace if the host kernel handles scounteren = 0
>> during reset accordingly, so to prevent this error we'll assume that it
>> won't. Read the reg from the host and use it as reset value.
> 
> It's not the host kernel that needs to change how it handles things. It's
> the guest kernel. When the guest uses ping, which likely calls gtod, which
> uses rdtime, or if the guest uses anything that results in an rdtime use,
> then it'll hit this issue if the host doesn't support sscofpmf (which the
> QEMU rv64 cpu type doesn't). The reason is because KVM won't expose the
> SBI PMU without sscofpmf and then the guest kernel pmu driver won't run,
> and currently the only place scounteren is getting set up is in the pmu
> driver. The guest kernel should unconditionally set up scounteren to
> match what it expects userspace to use (like enable rdtime, since the
> guest kernel actually implements gtod in vdso with it)
> 
>>
>> We'll end up repeating code from kvm_riscv_get_regs_csr() to do that.
>> Create a helper that will get a single CSR and use it in get_regs_csr()
>> and in kvm_riscv_reset_regs_csr() to avoid code duplication.
>>
>> Fixes: 4db19d5b21 ("target/riscv/kvm: add missing KVM CSRs")
> 
> This isn't the right tag since that is already fixed by checking
> get-reg-list in a previous patch. This patch is trying to fix a guest
> kernel bug, which it shouldn't be doing, at least not without some user
> toggle allowing the workaround to be turned on/off.
> 
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/kvm/kvm-cpu.c | 73 ++++++++++++++++++++++++++++----------
>>   1 file changed, 55 insertions(+), 18 deletions(-)
>>
>> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
>> index a91a87b175..918fe51257 100644
>> --- a/target/riscv/kvm/kvm-cpu.c
>> +++ b/target/riscv/kvm/kvm-cpu.c
>> @@ -634,29 +634,40 @@ static int kvm_riscv_put_regs_core(CPUState *cs)
>>       return ret;
>>   }
>>   
>> -static int kvm_riscv_get_regs_csr(CPUState *cs)
>> +static int kvm_riscv_get_reg_csr(CPUState *cs,
>> +                                 KVMCPUConfig *csr_cfg)
>>   {
>>       RISCVCPU *cpu = RISCV_CPU(cs);
>>       uint64_t reg;
>> -    int i, ret;
>> +    int ret;
>>   
>> -    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
>> -        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
>> +    if (!csr_cfg->supported) {
>> +        return 0;
>> +    }
>>   
>> -        if (!csr_cfg->supported) {
>> -            continue;
>> -        }
>> +    ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
>> +    if (ret) {
>> +        return ret;
>> +    }
>> +
>> +    if (csr_cfg->prop_size == sizeof(uint32_t)) {
>> +        kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
>> +    } else {
>> +        kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int kvm_riscv_get_regs_csr(CPUState *cs)
>> +{
>> +    int i, ret;
>>   
>> -        ret = kvm_get_one_reg(cs, csr_cfg->kvm_reg_id, &reg);
>> +    for (i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
>> +        ret = kvm_riscv_get_reg_csr(cs, &kvm_csr_cfgs[i]);
>>           if (ret) {
>>               return ret;
>>           }
>> -
>> -        if (csr_cfg->prop_size == sizeof(uint32_t)) {
>> -            kvm_cpu_csr_set_u32(cpu, csr_cfg, reg);
>> -        } else {
>> -            kvm_cpu_csr_set_u64(cpu, csr_cfg, reg);
>> -        }
>>       }
>>   
>>       return 0;
>> @@ -690,8 +701,11 @@ static int kvm_riscv_put_regs_csr(CPUState *cs)
>>       return 0;
>>   }
>>   
>> -static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
>> +static void kvm_riscv_reset_regs_csr(CPUState *cs, CPURISCVState *env)
>>   {
>> +    uint64_t scounteren_kvm_id = RISCV_CSR_REG(scounteren);
>> +    int ret;
>> +
>>       env->mstatus = 0;
>>       env->mie = 0;
>>       env->stvec = 0;
>> @@ -701,8 +715,30 @@ static void kvm_riscv_reset_regs_csr(CPURISCVState *env)
>>       env->stval = 0;
>>       env->mip = 0;
>>       env->satp = 0;
>> -    env->scounteren = 0;
>>       env->senvcfg = 0;
>> +
>> +    /*
>> +     * Some kernels will take issue with env->scounteren = 0
>> +     * causing problems inside the KVM guest with 'rdtime'.
>> +     * Read 'scounteren' from the host and use it.
>> +     */
>> +    for (int i = 0; i < ARRAY_SIZE(kvm_csr_cfgs); i++) {
>> +        KVMCPUConfig *csr_cfg = &kvm_csr_cfgs[i];
>> +
>> +        if (csr_cfg->kvm_reg_id != scounteren_kvm_id) {
>> +            continue;
>> +        }
>> +
>> +        if (!csr_cfg->supported) {
>> +            break;
>> +        }
>> +
>> +        ret = kvm_riscv_get_reg_csr(cs, &kvm_csr_cfgs[i]);
>> +        if (ret) {
>> +            error_report("Unable to retrieve scounteren from host ,"
>> +                         "error %d", ret);
>> +        }
>> +    }
>>   }
>>   
>>   static int kvm_riscv_get_regs_fp(CPUState *cs)
>> @@ -1711,16 +1747,17 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>>   void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
>>   {
>>       CPURISCVState *env = &cpu->env;
>> +    CPUState *cs = CPU(cpu);
>>       int i;
>>   
>>       for (i = 0; i < 32; i++) {
>>           env->gpr[i] = 0;
>>       }
>>       env->pc = cpu->env.kernel_addr;
>> -    env->gpr[10] = kvm_arch_vcpu_id(CPU(cpu)); /* a0 */
>> +    env->gpr[10] = kvm_arch_vcpu_id(cs);       /* a0 */
>>       env->gpr[11] = cpu->env.fdt_addr;          /* a1 */
>>   
>> -    kvm_riscv_reset_regs_csr(env);
>> +    kvm_riscv_reset_regs_csr(cs, env);
>>   }
>>   
>>   void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
>> -- 
>> 2.49.0
>>
>>
> 
> I would just drop this patch and make the default 'virt' cpu type 'max',
> then nobody will hit the issue. We should also fix the [guest] kernel,
> which I'll try to do soon.

I'll drop this patch and wait if we can change 'virt' default to 'max'. In
that case we'll leave this issue behind us (at least on QEMU).


Thanks,

Daniel


> 
> Thanks,
> drew