On Tue, May 30, 2023 at 04:46:20PM -0300, Daniel Henrique Barboza wrote:
> Let's add KVM user properties for the multi-letter extensions that KVM
> currently supports: zicbom, zicboz, zihintpause, zbb, ssaia, sstc,
> svinval and svpbmt.
>
> As with the recently added MISA properties we're also going to add a
> 'user_set' flag in each of them. The flag will be set only if the user
> chose an option that's different from the host and will require extra
> handling from the KVM driver.
>
> However, multi-letter CPUs have more cases to cover than MISA
> extensions, so we're adding an extra 'supported' flag as well. This flag
> will reflect if a given extension is supported by KVM, i.e. KVM knows
> how to handle it. This is determined during KVM extension discovery in
> kvm_riscv_init_multiext_cfg(), where we test for EINVAL errors. Any
> other error different from EINVAL will cause an abort.
I wish that was ENOENT, but I suppose that ship sailed.
>
> The 'supported' flag will then be used later on to give an exception for
> users that are disabling multi-letter extensions that are unknown to
> KVM.
>
> Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
> ---
> target/riscv/kvm.c | 136 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 136 insertions(+)
>
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index bb1dafe263..b4193a10d8 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -202,6 +202,99 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu,
> CPUState *cs)
> }
> }
>
> +typedef struct RISCVCPUMultiExtConfig {
> + const char *name;
No description? I'd prefer we use the same cfg struct for single-letter
and multi-letter extensions. We can use a union to overlap cpu_cfg_offset
and misa_bit.
> + int kvm_reg_id;
> + int cpu_cfg_offset;
> + bool supported;
> + bool user_set;
> +} RISCVCPUMultiExtConfig;
> +
> +#define CPUCFG(_prop) offsetof(struct RISCVCPUConfig, _prop)
> +
> +/*
> + * KVM ISA Multi-letter extensions. We care about the order
> + * since it'll be used to create the ISA string later on.
> + * We follow the same ordering rules of isa_edata_arr[]
> + * from target/riscv/cpu.c.
> + */
> +static RISCVCPUMultiExtConfig kvm_multi_ext_cfgs[] = {
> + {.name = "zicbom", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZICBOM,
> + .cpu_cfg_offset = CPUCFG(ext_icbom)},
> + {.name = "zicboz", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZICBOZ,
> + .cpu_cfg_offset = CPUCFG(ext_icboz)},
> + {.name = "zihintpause", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZIHINTPAUSE,
> + .cpu_cfg_offset = CPUCFG(ext_zihintpause)},
> + {.name = "zbb", .kvm_reg_id = KVM_RISCV_ISA_EXT_ZBB,
> + .cpu_cfg_offset = CPUCFG(ext_zbb)},
> + {.name = "ssaia", .kvm_reg_id = KVM_RISCV_ISA_EXT_SSAIA,
> + .cpu_cfg_offset = CPUCFG(ext_ssaia)},
> + {.name = "sstc", .kvm_reg_id = KVM_RISCV_ISA_EXT_SSTC,
> + .cpu_cfg_offset = CPUCFG(ext_sstc)},
> + {.name = "svinval", .kvm_reg_id = KVM_RISCV_ISA_EXT_SVINVAL,
> + .cpu_cfg_offset = CPUCFG(ext_svinval)},
> + {.name = "svpbmt", .kvm_reg_id = KVM_RISCV_ISA_EXT_SVPBMT,
> + .cpu_cfg_offset = CPUCFG(ext_svpbmt)},
As pointed out in the last patch, it'd be nice to share names (and
descriptions) with TCG.
> +};
> +
> +static void kvm_cpu_cfg_set(RISCVCPU *cpu, RISCVCPUMultiExtConfig *multi_ext,
> + uint32_t val)
> +{
> + int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
> + bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
> +
> + *ext_enabled = val;
> +}
> +
> +static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu,
> + RISCVCPUMultiExtConfig *multi_ext)
> +{
> + int cpu_cfg_offset = multi_ext->cpu_cfg_offset;
> + bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset;
> +
> + return *ext_enabled;
> +}
> +
> +static void kvm_cpu_set_multi_ext_cfg(Object *obj, Visitor *v,
> + const char *name,
> + void *opaque, Error **errp)
> +{
> + RISCVCPUMultiExtConfig *multi_ext_cfg = opaque;
> + RISCVCPU *cpu = RISCV_CPU(obj);
> + bool value, host_val;
> +
> + if (!visit_type_bool(v, name, &value, errp)) {
> + return;
> + }
> +
> + host_val = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
> +
> + /*
> + * Ignore if the user is setting the same value
> + * as the host.
> + */
> + if (value == host_val) {
> + return;
> + }
> +
> + if (!multi_ext_cfg->supported) {
> + /*
> + * Error out if the user is trying to enable an
> + * extension that KVM doesn't support. Ignore
> + * option otherwise.
> + */
> + if (value) {
> + error_setg(errp, "KVM does not support disabling extension %s",
> + multi_ext_cfg->name);
> + }
> +
> + return;
> + }
> +
> + multi_ext_cfg->user_set = true;
> + kvm_cpu_cfg_set(cpu, multi_ext_cfg, value);
> +}
> +
> static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
> {
> int i;
> @@ -216,6 +309,15 @@ static void kvm_riscv_add_cpu_user_properties(Object
> *cpu_obj)
> object_property_set_description(cpu_obj, misa_cfg->name,
> misa_cfg->description);
> }
> +
> + for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
> + RISCVCPUMultiExtConfig *multi_cfg = &kvm_multi_ext_cfgs[i];
> +
> + object_property_add(cpu_obj, multi_cfg->name, "bool",
> + NULL,
You have a getter function, so we might as well set it here, no?
> + kvm_cpu_set_multi_ext_cfg,
> + NULL, multi_cfg);
> + }
> }
>
> static int kvm_riscv_get_regs_core(CPUState *cs)
> @@ -531,6 +633,39 @@ static void kvm_riscv_init_misa_ext_mask(RISCVCPU *cpu,
> env->misa_ext = env->misa_ext_mask;
> }
>
> +static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
> +{
> + CPURISCVState *env = &cpu->env;
> + uint64_t val;
> + int i, ret;
> +
> + for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
> + RISCVCPUMultiExtConfig *multi_ext_cfg = &kvm_multi_ext_cfgs[i];
> + struct kvm_one_reg reg;
> +
> + reg.id = kvm_riscv_reg_id(env, KVM_REG_RISCV_ISA_EXT,
> + multi_ext_cfg->kvm_reg_id);
> + reg.addr = (uint64_t)&val;
> + ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, ®);
> + if (ret != 0) {
> + if (ret == -EINVAL) {
> + /* Silently default to 'false' if KVM does not support it. */
> + multi_ext_cfg->supported = false;
> + val = false;
> + } else {
> + error_report("Unable to read ISA_EXT KVM register %s, "
> + "error %d", multi_ext_cfg->name, ret);
> + kvm_riscv_destroy_scratch_vcpu(kvmcpu);
I don't think we usually bother cleaning up when exiting, we just let exit
do it, but OK.
> + exit(EXIT_FAILURE);
> + }
> + } else {
> + multi_ext_cfg->supported = true;
> + }
> +
> + kvm_cpu_cfg_set(cpu, multi_ext_cfg, val);
> + }
> +}
> +
> void kvm_riscv_init_user_properties(Object *cpu_obj)
> {
> RISCVCPU *cpu = RISCV_CPU(cpu_obj);
> @@ -543,6 +678,7 @@ void kvm_riscv_init_user_properties(Object *cpu_obj)
> kvm_riscv_add_cpu_user_properties(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_destroy_scratch_vcpu(&kvmcpu);
> }
> --
> 2.40.1
>
>
Thanks,
drew
