Re: [PATCH v2 5/8] target/riscv/kvm: rewrite kvm_riscv_handle_csr

[Andrew Jones <ajones@ventanamicro.com>]

On Mon, Feb 24, 2025 at 04:24:12PM +0800, Yong-Xuan Wang wrote:
> Rewrite the kvm_riscv_handle_csr() to support additional CSR emulation
> in user space with KVM acceleration. This update reuses the TCG CSR
> emulation function to simplify the implementation and reduce the
> redundant work. Also it introduces two hook functions for certain CSRs.
> Before emulation, the related VS mode context of the CSR can be loaded
> from host in context_load() hook. After the CSR handling, the modified
> VS context is written back in context_put() hook.
> 
> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> ---
>  target/riscv/cpu.h         |  2 --
>  target/riscv/csr.c         | 18 +++-------
>  target/riscv/kvm/kvm-cpu.c | 68 ++++++++++++++++++++++++++++++++------
>  3 files changed, 61 insertions(+), 27 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index df10ff63474b..81b8081d81e0 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -928,8 +928,6 @@ static inline const char *riscv_get_csr_name(int csr_no)
>  }
>  
>  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
> -target_ulong riscv_new_csr_seed(target_ulong new_value,
> -                                target_ulong write_mask);
>  
>  uint8_t satp_mode_max_from_map(uint32_t map);
>  const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit);
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d0068ce98c15..a2830888d010 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -5389,8 +5389,10 @@ static int write_mnstatus(CPURISCVState *env, int csrno, target_ulong val)
>  #endif
>  
>  /* Crypto Extension */
> -target_ulong riscv_new_csr_seed(target_ulong new_value,
> -                                target_ulong write_mask)
> +static RISCVException rmw_seed(CPURISCVState *env, int csrno,
> +                               target_ulong *ret_value,
> +                               target_ulong new_value,
> +                               target_ulong write_mask)
>  {
>      uint16_t random_v;
>      Error *random_e = NULL;
> @@ -5414,18 +5416,6 @@ target_ulong riscv_new_csr_seed(target_ulong new_value,
>          rval = random_v | SEED_OPST_ES16;
>      }
>  
> -    return rval;
> -}
> -
> -static RISCVException rmw_seed(CPURISCVState *env, int csrno,
> -                               target_ulong *ret_value,
> -                               target_ulong new_value,
> -                               target_ulong write_mask)
> -{
> -    target_ulong rval;
> -
> -    rval = riscv_new_csr_seed(new_value, write_mask);
> -
>      if (ret_value) {
>          *ret_value = rval;
>      }
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index d421c7a1b65d..b088b947adae 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1626,26 +1626,72 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
>      return ret;
>  }
>  
> +/* User-space CSR emulation */
> +struct kvm_riscv_emu_csr_data {
> +    target_ulong csr_num;
> +    int (*context_load)(CPUState *cs);
> +    int (*context_put)(CPUState *cs);
> +};
> +
> +struct kvm_riscv_emu_csr_data kvm_riscv_emu_csr_data[] = {
> +    { CSR_SEED, NULL, NULL },

Needing to ensure TCG state is correct, and therefore introducing the
load/put callbacks, makes me wonder if this is a good idea at all. Sharing
code is good, but, other than the TCG state concerns, we also will have to
deal with potentially wanting to compile with CONFIG_TCG=n when we only
want QEMU to be a KVM VMM.

> +};
> +
>  static int kvm_riscv_handle_csr(CPUState *cs, struct kvm_run *run)
>  {
> +    CPURISCVState *env = cpu_env(cs);
>      target_ulong csr_num = run->riscv_csr.csr_num;
>      target_ulong new_value = run->riscv_csr.new_value;
>      target_ulong write_mask = run->riscv_csr.write_mask;
> -    int ret = 0;
> +    struct kvm_riscv_emu_csr_data *emu_csr_data = NULL;
> +    target_ulong ret_value;
> +    RISCVException ret_excp;
> +    int i, ret;
>  
> -    switch (csr_num) {
> -    case CSR_SEED:
> -        run->riscv_csr.ret_value = riscv_new_csr_seed(new_value, write_mask);
> -        break;
> -    default:
> +    for (i = 0; i < ARRAY_SIZE(kvm_riscv_emu_csr_data); i++) {
> +        if (csr_num == kvm_riscv_emu_csr_data[i].csr_num) {
> +            emu_csr_data = &kvm_riscv_emu_csr_data[i];
> +

remove this extra blank line

> +            break;
> +        }
> +    }
> +
> +    if (!emu_csr_data) {
>          qemu_log_mask(LOG_UNIMP,
> -                      "%s: un-handled CSR EXIT for CSR %lx\n",
> -                      __func__, csr_num);
> -        ret = -1;
> -        break;
> +                      "%s: un-handled CSR EXIT for CSR %s\n",
> +                      __func__, riscv_get_csr_name(csr_num));
> +
> +        return -1;
>      }
>  
> -    return ret;
> +    if (emu_csr_data->context_load) {
> +        ret = emu_csr_data->context_load(cs);
> +        if (ret) {
> +            goto handle_failed;
> +        }
> +    }
> +
> +    ret_excp = riscv_csrrw(env, csr_num, &ret_value, new_value, write_mask);
> +    if (ret_excp != RISCV_EXCP_NONE) {
> +        goto handle_failed;
> +    }
> +    run->riscv_csr.ret_value = ret_value;
> +
> +    if (emu_csr_data->context_put) {
> +        ret = emu_csr_data->context_put(cs);
> +        if (ret) {
> +            goto handle_failed;
> +        }
> +    }
> +
> +    return 0;
> +
> +handle_failed:
> +    qemu_log_mask(LOG_UNIMP,
> +                  "%s: failed to handle CSR EXIT for CSR %s\n",
> +                  __func__, riscv_get_csr_name(csr_num));
> +
> +    return -1;
>  }
>  
>  static bool kvm_riscv_handle_debug(CPUState *cs)
> -- 
> 2.17.1
>

So, as I said above, I'm skeptical of this approach, since I'm not sure we
maintain TCG state well enough when using KVM to trust riscv_csrrw() nor
am I sure we can always have such simple load/put functions like the next
patch proposes for AIA registers.

I vote we manage CSRs case-by-case. SEED is already managed and shares
code at a point that is easy to audit, proving there are no TCG state
dependencies in the emulation. AIA should do the same, at least until
we've decided to build a KVM->TCG model that ensures TCG state is always
correct, in which case I don't think the load/put functions would be
necessary here.

(On a related note, Radim Krcmar suggested we consider forwarding more
SBI calls from KVM to QEMU where QEMU could then use OpenSBI's
implementation. That makes sense to me, but we'd need to create that
KVM->TCG model first for it too.)

Thanks,
drew