| Message ID | 20230705213955.429895-15-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv, KVM: fixes and enhancements | expand |
On Wed, Jul 05, 2023 at 06:39:49PM -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 MISA extensions, we're using the KVMCPUConfig type to hold > information about the state of each extension. However, multi-letter > extensions 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 ENOENT errors. Any > other error will cause an abort. > > The use of the 'user_set' is similar to what we already do with MISA > extensions: the flag set only if the user is changing the extension > state. > > The 'supported' flag will be used later on to make an exception for > users that are disabling multi-letter extensions that are unknown to > KVM. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.c | 8 +++ > target/riscv/kvm.c | 119 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 127 insertions(+) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 5c8832a030..31e591a938 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -1860,6 +1860,14 @@ static void riscv_cpu_add_user_properties(Object *obj) > riscv_cpu_add_misa_properties(obj); > > for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { > +#ifndef CONFIG_USER_ONLY > + if (kvm_enabled()) { > + /* Check if KVM created the property already */ > + if (object_property_find(obj, prop->name)) { > + continue; > + } > + } > +#endif > qdev_property_add_static(dev, prop); > } > > diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c > index 7afd6024e6..f2545bd560 100644 > --- a/target/riscv/kvm.c > +++ b/target/riscv/kvm.c > @@ -113,6 +113,7 @@ typedef struct KVMCPUConfig { > target_ulong offset; > int kvm_reg_id; > bool user_set; > + bool supported; > } KVMCPUConfig; > > #define KVM_MISA_CFG(_bit, _reg_id) \ > @@ -197,6 +198,81 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs) > } > } > > +#define CPUCFG(_prop) offsetof(struct RISCVCPUConfig, _prop) > + > +#define KVM_EXT_CFG(_name, _prop, _reg_id) \ > + {.name = _name, .offset = CPUCFG(_prop), \ > + .kvm_reg_id = _reg_id} > + > +static KVMCPUConfig kvm_multi_ext_cfgs[] = { > + KVM_EXT_CFG("zicbom", ext_icbom, KVM_RISCV_ISA_EXT_ZICBOM), > + KVM_EXT_CFG("zicboz", ext_icboz, KVM_RISCV_ISA_EXT_ZICBOZ), > + KVM_EXT_CFG("zihintpause", ext_zihintpause, KVM_RISCV_ISA_EXT_ZIHINTPAUSE), > + KVM_EXT_CFG("zbb", ext_zbb, KVM_RISCV_ISA_EXT_ZBB), > + KVM_EXT_CFG("ssaia", ext_ssaia, KVM_RISCV_ISA_EXT_SSAIA), > + KVM_EXT_CFG("sstc", ext_sstc, KVM_RISCV_ISA_EXT_SSTC), > + KVM_EXT_CFG("svinval", ext_svinval, KVM_RISCV_ISA_EXT_SVINVAL), > + KVM_EXT_CFG("svpbmt", ext_svpbmt, KVM_RISCV_ISA_EXT_SVPBMT), > +}; > + > +static void kvm_cpu_cfg_set(RISCVCPU *cpu, KVMCPUConfig *multi_ext, > + uint32_t val) > +{ > + int cpu_cfg_offset = multi_ext->offset; > + bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset; > + > + *ext_enabled = val; > +} > + > +static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu, > + KVMCPUConfig *multi_ext) > +{ > + int cpu_cfg_offset = multi_ext->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) > +{ > + KVMCPUConfig *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; > @@ -215,6 +291,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++) { > + KVMCPUConfig *multi_cfg = &kvm_multi_ext_cfgs[i]; > + > + object_property_add(cpu_obj, multi_cfg->name, "bool", > + NULL, > + kvm_cpu_set_multi_ext_cfg, > + NULL, multi_cfg); > + } > } > > static int kvm_riscv_get_regs_core(CPUState *cs) > @@ -530,6 +615,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++) { > + KVMCPUConfig *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 (errno == ENOENT) { > + /* 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); > + exit(EXIT_FAILURE); TL;DR - We need to change ENOENT back to EINVAL. I realized that I wasn't thinking clearly yesterday when I suggested we should error out for everything but the still-nonexistent ENOENT, which means erroring out even for EINVAL when it means "register not present". That might be fine if we were making this ioctl due to user input, but we're making this ioctl based on what QEMU knows about potential KVM extension support, which means QEMU will no longer work on older KVM which doesn't support everything QEMU knows about, regardless of user input. We're sort of stuck here with either assuming EINVAL is most likely going to be "register not present" and go with that, or we can't do this early probing for extensions at all, since we can't tell the difference between "invalid input" and "register not present". But, there is a solution on the horizon! get-reg-list is coming soon[1]. So, I suggest we do the following: 1. Audit KVM to ensure using EINVAL for get-one-reg's "register not present" is currently safe, and keep our eye on KVM to make sure it remains safe until get-reg-list lands. 2. Once we have get-reg-list then use that as the authority, rather than trying to decide if a register is present or not by errno. We'll still fall back to EINVAL probing when get-reg-list isn't present in KVM, but we'll know it's safe to do so because of (1), i.e. we only fall back when we're on an already audited KVM version. Whether or not we proceed with the EINVAL to ENOENT conversion in KVM no longer currently matters to QEMU with this plan, but we can still do it, as it seems more appropriate and may matter later. [1] https://lore.kernel.org/all/cover.1688010022.git.haibo1.xu@intel.com/ Thanks, drew > + } > + } 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); > @@ -542,6 +660,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.41.0 >
On Thu, Jul 06, 2023 at 10:45:04AM +0200, Andrew Jones wrote: ... > 1. Audit KVM to ensure using EINVAL for get-one-reg's "register not > present" is currently safe, and keep our eye on KVM to make sure > it remains safe until get-reg-list lands. I just did the audit and it looks fine. There's only one way these types of get-one-reg invocations can return EINVAL other than for "register not present" and that's for the encoded reg size not being sizeof(unsigned long). As long as we construct the register ID with the correct size then that can't happen. We currently construct it with kvm_riscv_reg_id() which does the right thing for these invocations. Thanks, drew
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 5c8832a030..31e591a938 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -1860,6 +1860,14 @@ static void riscv_cpu_add_user_properties(Object *obj) riscv_cpu_add_misa_properties(obj); for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { +#ifndef CONFIG_USER_ONLY + if (kvm_enabled()) { + /* Check if KVM created the property already */ + if (object_property_find(obj, prop->name)) { + continue; + } + } +#endif qdev_property_add_static(dev, prop); } diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c index 7afd6024e6..f2545bd560 100644 --- a/target/riscv/kvm.c +++ b/target/riscv/kvm.c @@ -113,6 +113,7 @@ typedef struct KVMCPUConfig { target_ulong offset; int kvm_reg_id; bool user_set; + bool supported; } KVMCPUConfig; #define KVM_MISA_CFG(_bit, _reg_id) \ @@ -197,6 +198,81 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs) } } +#define CPUCFG(_prop) offsetof(struct RISCVCPUConfig, _prop) + +#define KVM_EXT_CFG(_name, _prop, _reg_id) \ + {.name = _name, .offset = CPUCFG(_prop), \ + .kvm_reg_id = _reg_id} + +static KVMCPUConfig kvm_multi_ext_cfgs[] = { + KVM_EXT_CFG("zicbom", ext_icbom, KVM_RISCV_ISA_EXT_ZICBOM), + KVM_EXT_CFG("zicboz", ext_icboz, KVM_RISCV_ISA_EXT_ZICBOZ), + KVM_EXT_CFG("zihintpause", ext_zihintpause, KVM_RISCV_ISA_EXT_ZIHINTPAUSE), + KVM_EXT_CFG("zbb", ext_zbb, KVM_RISCV_ISA_EXT_ZBB), + KVM_EXT_CFG("ssaia", ext_ssaia, KVM_RISCV_ISA_EXT_SSAIA), + KVM_EXT_CFG("sstc", ext_sstc, KVM_RISCV_ISA_EXT_SSTC), + KVM_EXT_CFG("svinval", ext_svinval, KVM_RISCV_ISA_EXT_SVINVAL), + KVM_EXT_CFG("svpbmt", ext_svpbmt, KVM_RISCV_ISA_EXT_SVPBMT), +}; + +static void kvm_cpu_cfg_set(RISCVCPU *cpu, KVMCPUConfig *multi_ext, + uint32_t val) +{ + int cpu_cfg_offset = multi_ext->offset; + bool *ext_enabled = (void *)&cpu->cfg + cpu_cfg_offset; + + *ext_enabled = val; +} + +static uint32_t kvm_cpu_cfg_get(RISCVCPU *cpu, + KVMCPUConfig *multi_ext) +{ + int cpu_cfg_offset = multi_ext->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) +{ + KVMCPUConfig *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; @@ -215,6 +291,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++) { + KVMCPUConfig *multi_cfg = &kvm_multi_ext_cfgs[i]; + + object_property_add(cpu_obj, multi_cfg->name, "bool", + NULL, + kvm_cpu_set_multi_ext_cfg, + NULL, multi_cfg); + } } static int kvm_riscv_get_regs_core(CPUState *cs) @@ -530,6 +615,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++) { + KVMCPUConfig *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 (errno == ENOENT) { + /* 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); + 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); @@ -542,6 +660,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); }
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 MISA extensions, we're using the KVMCPUConfig type to hold information about the state of each extension. However, multi-letter extensions 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 ENOENT errors. Any other error will cause an abort. The use of the 'user_set' is similar to what we already do with MISA extensions: the flag set only if the user is changing the extension state. The 'supported' flag will be used later on to make an exception for users that are disabling multi-letter extensions that are unknown to KVM. Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/cpu.c | 8 +++ target/riscv/kvm.c | 119 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+)