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=+jb0s...@mail.gmail.com/
>
> Signed-off-by: Daniel Henrique Barboza <[email protected]>
> ---
> 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
