| Message ID | 20230530194623.272652-14-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv, KVM: fixes and enhancements | expand |
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 <dbarboza@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
On 6/7/23 08:48, Andrew Jones wrote: > 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 <dbarboza@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. multi-letter extensions don't have a 'description' field in TCG. Nothing prevents us from adding for KVM though. And yes, I'll create a single struct to handle both MISA and multi-letter extensions. We just need to have different getters/setters for each category. > >> + 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. I believe it's ok to do that for the MISA bits since it's a handful of entries. But I'd rather deal with a little code duplication for multi-letter extensions for now. We have 73 multi-letters extensions in TCG. Adding a description for each one, change how the properties are being created and so on seems too much for this series. > >> +}; >> + >> +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? Yep, might as well set the get() callback in the property. Thanks, Daniel > >> + 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
On Wed, Jun 07, 2023 at 04:59:02PM -0300, Daniel Henrique Barboza wrote: > > > On 6/7/23 08:48, Andrew Jones wrote: > > 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 <dbarboza@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. > > multi-letter extensions don't have a 'description' field in TCG. Nothing > prevents us from adding for KVM though. KVM can wait for descriptions until after adding them to TCG, and then, hopefully, figuring out how to share them with KVM. Thanks, drew
On 6/7/23 08:48, Andrew Jones wrote: > 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 <dbarboza@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? I was going back to do this change then realized that we don't have a get() for this case. We have kvm_cpu_cfg_get(), which is a way of retrieving the value of cpu->cfg.<attr>. It's not the same as a get() impl for object_property_add(). Thanks, Daniel > >> + 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
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; + 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)}, +}; + +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, + 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); + 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); }
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. 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 <dbarboza@ventanamicro.com> --- target/riscv/kvm.c | 136 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+)