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), ®); \
- 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), ®); \
- 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), ®); \
@@ -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, ®);
+ 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, ®);
+ if (ret) {
+ return ret;
+ }
+ }
return 0;
}
--
2.49.0On 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), ®); \
> - 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), ®); \
> - 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), ®); \
> @@ -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, ®);
> + 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, ®);
> + if (ret) {
> + return ret;
> + }
> + }
>
> return 0;
> }
> --
> 2.49.0
>
>
Thanks,
drew
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), ®); \
>> - 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), ®); \
>> - 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), ®); \
>> @@ -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, ®);
>> + 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, ®);
>> + if (ret) {
>> + return ret;
>> + }
>> + }
>>
>> return 0;
>> }
>> --
>> 2.49.0
>>
>>
>
> Thanks,
> drew
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
© 2016 - 2025 Red Hat, Inc.