| Message ID | 20240126133101.61344-11-ajones@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: named features riscv,isa, 'svade' rework | expand |
On Fri, Jan 26, 2024 at 11:33 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > From: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > The RVA22U64 and RVA22S64 profiles mandates certain extensions that, > until now, we were implying that they were available. > > We can't do this anymore since named features also has a riscv,isa > entry. Let's add them to riscv_cpu_named_features[]. > > They will also need to be explicitly enabled in both profile > descriptions. TCG will enable the named features it already implements, > other accelerators are free to handle it as they like. > > After this patch, here's the riscv,isa from a buildroot using the > 'rva22s64' CPU: > > # cat /proc/device-tree/cpus/cpu@0/riscv,isa > rv64imafdc_zic64b_zicbom_zicbop_zicboz_ziccamoa_ziccif_zicclsm_ziccrse_ > zicntr_zicsr_zifencei_zihintpause_zihpm_za64rs_zfhmin_zca_zcd_zba_zbb_ > zbs_zkt_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > --- > target/riscv/cpu.c | 41 +++++++++++++++++++++++++++++--------- > target/riscv/cpu_cfg.h | 9 +++++++++ > target/riscv/tcg/tcg-cpu.c | 19 +++++++++++++++++- > 3 files changed, 59 insertions(+), 10 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 28d3cfa8ce59..1ecd8a57ed02 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -101,6 +101,10 @@ const RISCVIsaExtData isa_edata_arr[] = { > ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom), > ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop), > ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz), > + ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, ext_ziccamoa), > + ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_ziccif), > + ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_zicclsm), > + ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_ziccrse), > ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond), > ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr), > ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr), > @@ -109,6 +113,7 @@ const RISCVIsaExtData isa_edata_arr[] = { > ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause), > ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm), > ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul), > + ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, ext_za64rs), > ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas), > ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs), > ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa), > @@ -170,8 +175,12 @@ const RISCVIsaExtData isa_edata_arr[] = { > ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp), > ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen), > ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia), > + ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, ext_ssccptr), > ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf), > + ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_sscounterenw), > ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc), > + ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_sstvala), > + ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_sstvecd), > ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade), > ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu), > ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval), > @@ -1523,6 +1532,22 @@ const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = { > MULTI_EXT_CFG_BOOL("svade", ext_svade, true), > MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true), > > + /* > + * cache-related extensions that are always enabled > + * since QEMU RISC-V does not have a cache model. > + */ > + MULTI_EXT_CFG_BOOL("za64rs", ext_za64rs, true), > + MULTI_EXT_CFG_BOOL("ziccif", ext_ziccif, true), > + MULTI_EXT_CFG_BOOL("ziccrse", ext_ziccrse, true), > + MULTI_EXT_CFG_BOOL("ziccamoa", ext_ziccamoa, true), > + MULTI_EXT_CFG_BOOL("zicclsm", ext_zicclsm, true), > + MULTI_EXT_CFG_BOOL("ssccptr", ext_ssccptr, true), > + > + /* Other named features that QEMU TCG always implements */ > + MULTI_EXT_CFG_BOOL("sstvecd", ext_sstvecd, true), > + MULTI_EXT_CFG_BOOL("sstvala", ext_sstvala, true), > + MULTI_EXT_CFG_BOOL("sscounterenw", ext_sscounterenw, true), > + > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -2116,13 +2141,8 @@ static const PropertyInfo prop_marchid = { > }; > > /* > - * RVA22U64 defines some 'named features' or 'synthetic extensions' > - * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa > - * and Zicclsm. We do not implement caching in QEMU so we'll consider > - * all these named features as always enabled. > - * > - * There's no riscv,isa update for them (nor for zic64b, despite it > - * having a cfg offset) at this moment. > + * RVA22U64 defines some cache related extensions: Za64rs, > + * Ziccif, Ziccrse, Ziccamoa and Zicclsm. > */ > static RISCVCPUProfile RVA22U64 = { > .parent = NULL, > @@ -2139,7 +2159,9 @@ static RISCVCPUProfile RVA22U64 = { > CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz), > > /* mandatory named features for this profile */ > - CPU_CFG_OFFSET(ext_zic64b), > + CPU_CFG_OFFSET(ext_za64rs), CPU_CFG_OFFSET(ext_zic64b), > + CPU_CFG_OFFSET(ext_ziccif), CPU_CFG_OFFSET(ext_ziccrse), > + CPU_CFG_OFFSET(ext_ziccamoa), CPU_CFG_OFFSET(ext_zicclsm), > > RISCV_PROFILE_EXT_LIST_END > } > @@ -2170,7 +2192,8 @@ static RISCVCPUProfile RVA22S64 = { > CPU_CFG_OFFSET(ext_svinval), > > /* rva22s64 named features */ > - CPU_CFG_OFFSET(ext_svade), > + CPU_CFG_OFFSET(ext_sstvecd), CPU_CFG_OFFSET(ext_sstvala), > + CPU_CFG_OFFSET(ext_sscounterenw), CPU_CFG_OFFSET(ext_svade), > > RISCV_PROFILE_EXT_LIST_END > } > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h > index 698f926ab1be..f79fc3dfd199 100644 > --- a/target/riscv/cpu_cfg.h > +++ b/target/riscv/cpu_cfg.h > @@ -125,6 +125,15 @@ struct RISCVCPUConfig { > /* Named features */ > bool ext_svade; > bool ext_zic64b; > + bool ext_za64rs; > + bool ext_ziccif; > + bool ext_ziccrse; > + bool ext_ziccamoa; > + bool ext_zicclsm; > + bool ext_ssccptr; > + bool ext_sstvecd; > + bool ext_sstvala; > + bool ext_sscounterenw; Daniel and I have discussed this a bit before. This doesn't feel right. We are adding variables that must always be true. To me this seems confusing to anyone reading the code in the future. I understand it isn't exposed to users, but it still feels odd. I guess it's nice that we reuse the same logic and this is in some ways cleaner than an "always_true", but it just seems clunky. Especially as this list seems to be growing very quickly. > > /* Vendor-specific custom extensions */ > bool ext_xtheadba; > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index 90861cc065e5..6d5028cf84d0 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -206,7 +206,8 @@ static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset) > cpu->cfg.ext_svadu = false; > break; > default: > - g_assert_not_reached(); > + /* Named feature already enabled in riscv_tcg_cpu_instance_init */ > + return; > } > } > > @@ -1342,6 +1343,20 @@ static bool riscv_cpu_has_max_extensions(Object *cpu_obj) > return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL; > } > > +/* Named features that TCG always implements */ > +static void riscv_tcg_cpu_enable_named_feats(RISCVCPU *cpu) > +{ > + cpu->cfg.ext_za64rs = true; > + cpu->cfg.ext_ziccif = true; > + cpu->cfg.ext_ziccrse = true; > + cpu->cfg.ext_ziccamoa = true; > + cpu->cfg.ext_zicclsm = true; > + cpu->cfg.ext_ssccptr = true; > + cpu->cfg.ext_sstvecd = true; > + cpu->cfg.ext_sstvala = true; > + cpu->cfg.ext_sscounterenw = true; Maybe we should assert that these are all true somewhere in the CPU init/realise/reset. That at least indicates to someone using the code that these can't actually be disabled. @Daniel Henrique Barboza that might be a reasonable compromise as I know I complained to you about the same thing. Unless you have implemented something really clean and nice :) Alistair > +} > + > static void riscv_tcg_cpu_instance_init(CPUState *cs) > { > RISCVCPU *cpu = RISCV_CPU(cs); > @@ -1354,6 +1369,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs) > if (riscv_cpu_has_max_extensions(obj)) { > riscv_init_max_cpu_extensions(obj); > } > + > + riscv_tcg_cpu_enable_named_feats(cpu); > } > > static void riscv_tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc) > -- > 2.43.0 > >
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 28d3cfa8ce59..1ecd8a57ed02 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -101,6 +101,10 @@ const RISCVIsaExtData isa_edata_arr[] = { ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom), ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop), ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz), + ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, ext_ziccamoa), + ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_ziccif), + ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_zicclsm), + ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_ziccrse), ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond), ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr), ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr), @@ -109,6 +113,7 @@ const RISCVIsaExtData isa_edata_arr[] = { ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause), ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm), ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul), + ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, ext_za64rs), ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas), ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs), ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa), @@ -170,8 +175,12 @@ const RISCVIsaExtData isa_edata_arr[] = { ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp), ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen), ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia), + ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, ext_ssccptr), ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf), + ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_sscounterenw), ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc), + ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_sstvala), + ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_sstvecd), ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade), ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu), ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval), @@ -1523,6 +1532,22 @@ const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = { MULTI_EXT_CFG_BOOL("svade", ext_svade, true), MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true), + /* + * cache-related extensions that are always enabled + * since QEMU RISC-V does not have a cache model. + */ + MULTI_EXT_CFG_BOOL("za64rs", ext_za64rs, true), + MULTI_EXT_CFG_BOOL("ziccif", ext_ziccif, true), + MULTI_EXT_CFG_BOOL("ziccrse", ext_ziccrse, true), + MULTI_EXT_CFG_BOOL("ziccamoa", ext_ziccamoa, true), + MULTI_EXT_CFG_BOOL("zicclsm", ext_zicclsm, true), + MULTI_EXT_CFG_BOOL("ssccptr", ext_ssccptr, true), + + /* Other named features that QEMU TCG always implements */ + MULTI_EXT_CFG_BOOL("sstvecd", ext_sstvecd, true), + MULTI_EXT_CFG_BOOL("sstvala", ext_sstvala, true), + MULTI_EXT_CFG_BOOL("sscounterenw", ext_sscounterenw, true), + DEFINE_PROP_END_OF_LIST(), }; @@ -2116,13 +2141,8 @@ static const PropertyInfo prop_marchid = { }; /* - * RVA22U64 defines some 'named features' or 'synthetic extensions' - * that are cache related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa - * and Zicclsm. We do not implement caching in QEMU so we'll consider - * all these named features as always enabled. - * - * There's no riscv,isa update for them (nor for zic64b, despite it - * having a cfg offset) at this moment. + * RVA22U64 defines some cache related extensions: Za64rs, + * Ziccif, Ziccrse, Ziccamoa and Zicclsm. */ static RISCVCPUProfile RVA22U64 = { .parent = NULL, @@ -2139,7 +2159,9 @@ static RISCVCPUProfile RVA22U64 = { CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz), /* mandatory named features for this profile */ - CPU_CFG_OFFSET(ext_zic64b), + CPU_CFG_OFFSET(ext_za64rs), CPU_CFG_OFFSET(ext_zic64b), + CPU_CFG_OFFSET(ext_ziccif), CPU_CFG_OFFSET(ext_ziccrse), + CPU_CFG_OFFSET(ext_ziccamoa), CPU_CFG_OFFSET(ext_zicclsm), RISCV_PROFILE_EXT_LIST_END } @@ -2170,7 +2192,8 @@ static RISCVCPUProfile RVA22S64 = { CPU_CFG_OFFSET(ext_svinval), /* rva22s64 named features */ - CPU_CFG_OFFSET(ext_svade), + CPU_CFG_OFFSET(ext_sstvecd), CPU_CFG_OFFSET(ext_sstvala), + CPU_CFG_OFFSET(ext_sscounterenw), CPU_CFG_OFFSET(ext_svade), RISCV_PROFILE_EXT_LIST_END } diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h index 698f926ab1be..f79fc3dfd199 100644 --- a/target/riscv/cpu_cfg.h +++ b/target/riscv/cpu_cfg.h @@ -125,6 +125,15 @@ struct RISCVCPUConfig { /* Named features */ bool ext_svade; bool ext_zic64b; + bool ext_za64rs; + bool ext_ziccif; + bool ext_ziccrse; + bool ext_ziccamoa; + bool ext_zicclsm; + bool ext_ssccptr; + bool ext_sstvecd; + bool ext_sstvala; + bool ext_sscounterenw; /* Vendor-specific custom extensions */ bool ext_xtheadba; diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index 90861cc065e5..6d5028cf84d0 100644 --- a/target/riscv/tcg/tcg-cpu.c +++ b/target/riscv/tcg/tcg-cpu.c @@ -206,7 +206,8 @@ static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset) cpu->cfg.ext_svadu = false; break; default: - g_assert_not_reached(); + /* Named feature already enabled in riscv_tcg_cpu_instance_init */ + return; } } @@ -1342,6 +1343,20 @@ static bool riscv_cpu_has_max_extensions(Object *cpu_obj) return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL; } +/* Named features that TCG always implements */ +static void riscv_tcg_cpu_enable_named_feats(RISCVCPU *cpu) +{ + cpu->cfg.ext_za64rs = true; + cpu->cfg.ext_ziccif = true; + cpu->cfg.ext_ziccrse = true; + cpu->cfg.ext_ziccamoa = true; + cpu->cfg.ext_zicclsm = true; + cpu->cfg.ext_ssccptr = true; + cpu->cfg.ext_sstvecd = true; + cpu->cfg.ext_sstvala = true; + cpu->cfg.ext_sscounterenw = true; +} + static void riscv_tcg_cpu_instance_init(CPUState *cs) { RISCVCPU *cpu = RISCV_CPU(cs); @@ -1354,6 +1369,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs) if (riscv_cpu_has_max_extensions(obj)) { riscv_init_max_cpu_extensions(obj); } + + riscv_tcg_cpu_enable_named_feats(cpu); } static void riscv_tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)