Re: [PATCH v2 1/8] target/riscv/kvm: rewrite get/set for KVM_REG_RISCV_CSR

[Andrew Jones]

On Mon, Feb 24, 2025 at 04:24:08PM +0800, Yong-Xuan Wang wrote:
> As KVM_REG_RISCV_CSR includes several subtypes of CSR, rewrite the
> related macros and functions to prepare for other subtypes.
> 
> Signed-off-by: Yong-Xuan Wang <yongxuan.w...@sifive.com>
> ---
>  target/riscv/kvm/kvm-cpu.c | 70 +++++++++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 27 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 471fd554b369..ff1211d2fe39 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -111,9 +111,8 @@ static uint64_t kvm_riscv_vector_reg_id(RISCVCPU *cpu,
>      kvm_riscv_reg_id_ulong(env, 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, \
> -                           KVM_REG_RISCV_CSR_REG(name))
> +#define RISCV_CSR_REG(env, idx) \
> +    kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CSR, idx)
>  
>  #define RISCV_CONFIG_REG(env, name) \
>      kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG, \
> @@ -130,17 +129,20 @@ static uint64_t kvm_riscv_vector_reg_id(RISCVCPU *cpu,
>      kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_VECTOR, \
>                             KVM_REG_RISCV_VECTOR_CSR_REG(name))
>  
> -#define KVM_RISCV_GET_CSR(cs, env, csr, reg) \
> +#define RISCV_GENERAL_CSR_REG(name) \
> +    (KVM_REG_RISCV_CSR_GENERAL | KVM_REG_RISCV_CSR_REG(name))
> +
> +#define KVM_RISCV_GET_CSR(cs, env, idx, 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(env, idx), &reg); \
>          if (_ret) { \
>              return _ret; \
>          } \
>      } while (0)
>  
> -#define KVM_RISCV_SET_CSR(cs, env, csr, reg) \
> +#define KVM_RISCV_SET_CSR(cs, env, idx, 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(env, idx), &reg); \
>          if (_ret) { \
>              return _ret; \
>          } \
> @@ -608,36 +610,50 @@ 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_regs_general_csr(CPUState *cs)
>  {
>      CPURISCVState *env = &RISCV_CPU(cs)->env;
>  
> -    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);
> +    KVM_RISCV_GET_CSR(cs, env, RISCV_GENERAL_CSR_REG(sstatus), env->mstatus);
> +    KVM_RISCV_GET_CSR(cs, env, RISCV_GENERAL_CSR_REG(sie), env->mie);
> +    KVM_RISCV_GET_CSR(cs, env, RISCV_GENERAL_CSR_REG(stvec), env->stvec);
> +    KVM_RISCV_GET_CSR(cs, env, RISCV_GENERAL_CSR_REG(sscratch), 
> env->sscratch);
> +    KVM_RISCV_GET_CSR(cs, env, RISCV_GENERAL_CSR_REG(sepc), env->sepc);
> +    KVM_RISCV_GET_CSR(cs, env, RISCV_GENERAL_CSR_REG(scause), env->scause);
> +    KVM_RISCV_GET_CSR(cs, env, RISCV_GENERAL_CSR_REG(stval), env->stval);
> +    KVM_RISCV_GET_CSR(cs, env, RISCV_GENERAL_CSR_REG(sip), env->mip);
> +    KVM_RISCV_GET_CSR(cs, env, RISCV_GENERAL_CSR_REG(satp), env->satp);
>  
>      return 0;
>  }
>  
> -static int kvm_riscv_put_regs_csr(CPUState *cs)
> +static int kvm_riscv_put_regs_general_csr(CPUState *cs)
>  {
>      CPURISCVState *env = &RISCV_CPU(cs)->env;
>  
> -    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);
> +    KVM_RISCV_SET_CSR(cs, env, RISCV_GENERAL_CSR_REG(sstatus), env->mstatus);
> +    KVM_RISCV_SET_CSR(cs, env, RISCV_GENERAL_CSR_REG(sie), env->mie);
> +    KVM_RISCV_SET_CSR(cs, env, RISCV_GENERAL_CSR_REG(stvec), env->stvec);
> +    KVM_RISCV_SET_CSR(cs, env, RISCV_GENERAL_CSR_REG(sscratch), 
> env->sscratch);
> +    KVM_RISCV_SET_CSR(cs, env, RISCV_GENERAL_CSR_REG(sepc), env->sepc);
> +    KVM_RISCV_SET_CSR(cs, env, RISCV_GENERAL_CSR_REG(scause), env->scause);
> +    KVM_RISCV_SET_CSR(cs, env, RISCV_GENERAL_CSR_REG(stval), env->stval);
> +    KVM_RISCV_SET_CSR(cs, env, RISCV_GENERAL_CSR_REG(sip), env->mip);
> +    KVM_RISCV_SET_CSR(cs, env, RISCV_GENERAL_CSR_REG(satp), env->satp);
> +
> +    return 0;
> +}
> +
> +static int kvm_riscv_get_regs_csr(CPUState *cs)
> +{
> +    kvm_riscv_get_regs_general_csr(cs);

KVM_RISCV_GET/SET_CSR() can return errors so the return value should be
checked here and below (just kvm_arch_get/put_registers() checks the
returns values of kvm_riscv_get/put_regs_csr()).

Otherwise,

Reviewed-by: Andrew Jones <ajo...@ventanamicro.com>

> +
> +    return 0;
> +}
> +
> +static int kvm_riscv_put_regs_csr(CPUState *cs)
> +{
> +    kvm_riscv_put_regs_general_csr(cs);
>  
>      return 0;
>  }
> -- 
> 2.17.1
>