Re: [PATCH v2 6/9] target/riscv/kvm: do not read unavailable CSRs

[Andrew Jones <ajones@ventanamicro.com>]

On Fri, Apr 25, 2025 at 08:37:02AM -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()
                                       ^ kvm_riscv_init_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 | 62 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 7cbe566e5f..f341085ba1 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -636,6 +636,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 (KVM_REG_SIZE(csr_cfg->kvm_reg_id) == sizeof(uint32_t)) {
>              reg = kvm_cpu_csr_get_u32(cpu, csr_cfg);
>          } else if (KVM_REG_SIZE(csr_cfg->kvm_reg_id) == sizeof(uint64_t)) {
> @@ -1090,6 +1098,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;
> @@ -1146,7 +1180,26 @@ 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_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>  {
>      g_autofree struct kvm_reg_list *reglist = NULL;
>      KVMCPUConfig *multi_ext_cfg;
> @@ -1163,7 +1216,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
> @@ -1226,6 +1281,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)
> @@ -1239,7 +1295,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_cfg(cpu, &kvmcpu);
>  
>      kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
>  }
> -- 
> 2.49.0
>

Otherwise,

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