Re: [PATCH 11/16] target/riscv: add KVM specific MISA properties

From: Andrew Jones <ajones@ventanamicro.com>
To: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org,
	alistair.francis@wdc.com,  bmeng@tinylab.org,
	liweiwei@iscas.ac.cn, zhiwei_liu@linux.alibaba.com,
	 palmer@rivosinc.com
Subject: Re: [PATCH 11/16] target/riscv: add KVM specific MISA properties
Date: Wed, 7 Jun 2023 13:33:59 +0200	[thread overview]
Message-ID: <20230606-0e7faaa57483deff11d595c2@orel> (raw)
In-Reply-To: <20230530194623.272652-12-dbarboza@ventanamicro.com>

On Tue, May 30, 2023 at 04:46:18PM -0300, Daniel Henrique Barboza wrote:
> Using all TCG user properties in KVM is tricky. First because KVM
> supports only a small subset of what TCG provides, so most of the
> cpu->cfg flags do nothing for KVM.
> 
> Second, and more important, we don't have a way of telling if any given
> value is an user input or not. For TCG this has a small impact since we
> just validating everything and error out if needed. But for KVM it would
> be good to know if a given value was set by the user or if it's a value
> already provided by KVM. Otherwise we don't know how to handle failed
> kvm_set_one_regs() when writing the configurations back.
> 
> These characteristics make it overly complicated to use the same user
> facing flags for both KVM and TCG. A simpler approach is to create KVM
> specific properties that have specialized logic, forking KVM and TCG use
> cases for those cases only. Fully separating KVM/TCG properties is
> unneeded at this point - in fact we want the user experience to be as
> equal as possible, regardless of the acceleration chosen.
> 
> We'll start this fork with the MISA properties, adding the MISA bits
> that the KVM driver currently supports. The KVM version of
> RISCVCPUMisaExtConfig and kvm_misa_ext_cfgs[] are inspired by the
> existing RISCVCPUMisaExtConfig and misa_ext_cfgs[] from
> target/riscv/cpu.c. For KVM  we're adding an extra oomph in
> RISCVCPUMisaExtConfig with the 'user_set' boolean. This flag will be set
> when the user set an option that's different than what is already
> configured in the host, requiring KVM intervention to write the regs
> back during kvm_arch_init_vcpu().
> 
> There is no need to duplicate more code than necessary, so we're going
> to use the existing kvm_riscv_init_user_properties() to add the KVM
> specific properties. Any code that is adding a TCG user prop is then
> changed slightly to verify first if there's a KVM prop with the same
> name already added.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 10 ++++++
>  target/riscv/kvm.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 749d8bf5eb..3c348049a3 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1587,6 +1587,11 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>      for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
>          const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
>  
> +        /* Check if KVM didn't create the property already */
> +        if (object_property_find(cpu_obj, misa_cfg->name)) {
> +            continue;
> +        }
> +
>          object_property_add(cpu_obj, misa_cfg->name, "bool",
>                              cpu_get_misa_ext_cfg,
>                              cpu_set_misa_ext_cfg,
> @@ -1710,6 +1715,11 @@ static void riscv_cpu_add_user_properties(Object *obj)
>      riscv_cpu_add_misa_properties(obj);
>  
>      for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
> +        /* Check if KVM didn't create the property already */
> +        if (object_property_find(obj, prop->name)) {
> +            continue;
> +        }
> +
>          qdev_property_add_static(dev, prop);
>      }
>  
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 4d0808cb9a..6afd56cda5 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -22,8 +22,10 @@
>  #include <linux/kvm.h>
>  
>  #include "qemu/timer.h"
> +#include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "qemu/main-loop.h"
> +#include "qapi/visitor.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/kvm_int.h"
> @@ -105,6 +107,81 @@ static uint64_t kvm_riscv_reg_id(CPURISCVState *env, uint64_t type,
>          } \
>      } while (0)
>  
> +typedef struct RISCVCPUMisaExtConfig {

I'd give this a name with KVM in it.

> +    const char *name;
> +    const char *description;
> +    target_ulong misa_bit;
> +    int kvm_reg_id;
> +    bool user_set;
> +} RISCVCPUMisaExtConfig;
> +
> +/* KVM ISA extensions */
> +static RISCVCPUMisaExtConfig kvm_misa_ext_cfgs[] = {
> +    {.name = "a", .description = "Atomic instructions",
> +     .misa_bit = RVA, .kvm_reg_id = KVM_RISCV_ISA_EXT_A},
> +    {.name = "c", .description = "Compressed instructions",
> +     .misa_bit = RVC, .kvm_reg_id = KVM_RISCV_ISA_EXT_C},
> +    {.name = "d", .description = "Double-precision float point",
> +     .misa_bit = RVD, .kvm_reg_id = KVM_RISCV_ISA_EXT_D},
> +    {.name = "f", .description = "Single-precision float point",
> +     .misa_bit = RVF, .kvm_reg_id = KVM_RISCV_ISA_EXT_F},
> +    {.name = "h", .description = "Hypervisor",
> +     .misa_bit = RVH, .kvm_reg_id = KVM_RISCV_ISA_EXT_H},
> +    {.name = "i", .description = "Base integer instruction set",
> +     .misa_bit = RVI, .kvm_reg_id = KVM_RISCV_ISA_EXT_I},
> +    {.name = "m", .description = "Integer multiplication and division",
> +     .misa_bit = RVM, .kvm_reg_id = KVM_RISCV_ISA_EXT_M},
> +};

I'm not a huge fan of duplicating the name and description strings. Maybe
we should put them in their own array, indexed by misa bit, in order to
share them.

 struct misa_ext_cfg_name {
     const char *name;
     const char *description;
 };

static const struct misa_ext_cfg_name misa_ext_cfg_names[] = {
    [RVA] = { "a", "Atomic instructions", },
    [RVC] = { "c", "Compressed instructions", },
    ...

#define MISA_CFG(_bit, _enabled) \
    {.name = misa_ext_cfg_names[_bit].name, \
     .description = misa_ext_cfg_names[_bit].description, \
     .misa_bit = _bit, .enabled = _enabled}

static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
    MISA_CFG(RVA, true),
    MISA_CFG(RVC, true),
    ...

#define KVM_MISA_CFG(_bit, _reg_id) \
    {.name = misa_ext_cfg_names[_bit].name,
     .description = misa_ext_cfg_names[_bit].description, \
     .misa_bit = _bit, .kvm_reg_id = _reg_id}

static const RISCVCPUKVMMisaExtConfig kvm_misa_ext_cfgs[] = {
    KVM_MISA_CFG(RVA, KVM_RISCV_ISA_EXT_A),
    KVM_MISA_CFG(RVC, KVM_RISCV_ISA_EXT_C),
    ...

> +
> +static void kvm_cpu_set_misa_ext_cfg(Object *obj, Visitor *v,
> +                                     const char *name,
> +                                     void *opaque, Error **errp)
> +{
> +    RISCVCPUMisaExtConfig *misa_ext_cfg = opaque;
> +    target_ulong misa_bit = misa_ext_cfg->misa_bit;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
> +    bool value, host_bit;
> +
> +    if (!visit_type_bool(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    host_bit = env->misa_ext_mask & misa_bit;
> +
> +    if (value == host_bit) {
> +        return;
> +    }
> +
> +    if (!value) {
> +        misa_ext_cfg->user_set = true;
> +        return;
> +    }
> +
> +    /*
> +     * Forbid users to enable extensions that aren't
> +     * available in the hart.
> +     */
> +    error_setg(errp, "Enabling MISA bit '%s' is not allowed: it's not "
> +               "enabled in the host", misa_ext_cfg->name);
> +}
> +
> +static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(kvm_misa_ext_cfgs); i++) {
> +        RISCVCPUMisaExtConfig *misa_cfg = &kvm_misa_ext_cfgs[i];
> +
> +        object_property_add(cpu_obj, misa_cfg->name, "bool",
> +                            NULL,
> +                            kvm_cpu_set_misa_ext_cfg,
> +                            NULL, misa_cfg);
> +        object_property_set_description(cpu_obj, misa_cfg->name,
> +                                        misa_cfg->description);
> +    }
> +}
> +
>  static int kvm_riscv_get_regs_core(CPUState *cs)
>  {
>      int ret = 0;
> @@ -427,6 +504,7 @@ void kvm_riscv_init_user_properties(Object *cpu_obj)
>          return;
>      }
>  
> +    kvm_riscv_add_cpu_user_properties(cpu_obj);
>      kvm_riscv_init_machine_ids(cpu, &kvmcpu);
>      kvm_riscv_init_misa_ext_mask(cpu, &kvmcpu);
>  
> -- 
> 2.40.1
> 
>

Otherwise, LGTM.

Thanks,
drew