On Fri, Feb 2, 2024 at 7:44 PM Daniel Henrique Barboza <dbarb...@ventanamicro.com> wrote: > > > > On 2/1/24 23:14, Alistair Francis wrote: > > On Thu, Feb 1, 2024 at 5:15 AM Daniel Henrique Barboza > > <dbarb...@ventanamicro.com> wrote: > >> > >> > >> > >> On 1/29/24 22:10, Alistair Francis wrote: > >>> On Fri, Jan 26, 2024 at 5:54 AM Daniel Henrique Barboza > >>> <dbarb...@ventanamicro.com> wrote: > >>>> > >>>> 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 <dbarb...@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 28d3cfa8ce..1ecd8a57ed 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 698f926ab1..f79fc3dfd1 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; > >>> > >>> Overall this and the previous patch look fine. > >>> > >>> One thing though, why store this information? I feel it could be > >>> confusing having these variables. If a developer sets them to false > >>> nothing actually happens, which is a little misleading > >> > >> These extensions aren't being exposed to users. riscv_cpu_named_features[] > >> isn't > > > > Yep, we should not expose them to users. > > > > I meant developers, as in people reading the C code and compiling QEMU. > > > >> being used to create any CPU user properties. I should've mentioned that in > >> patch 2 ... > >> > >> As for the extra booleans that we'll be setting to 'true', as it is now > >> isa_edata_arr[] stores a string name, priv_ver and a cpu->cfg offset, so > >> everyone that adds a riscv,isa str must also have a valid bool offset in > >> RISCVCPUConfig. Having a bool also allow us to treat them as regular > >> extensions > >> because we can re-use existing code to blindly enable them in profiles like > >> any other profile extension. > > > > Yep, and I think that makes sense. The odd part is that we don't > > actually use these bools. Which I feel is confusing when looking at > > the QEMU code base. Why have a variable that we don't use? > > Let's say that we create an "always_enabled" boolean in cpu->cfg, always set > to > 'true' during riscv_cpu_init(). Then we change riscv_cpu_named_features[] to > work like this: > > MULTI_EXT_CFG_BOOL("svade", always_enabled, true), > MULTI_EXT_CFG_BOOL("zic64b", always_enabled, true), > MULTI_EXT_CFG_BOOL("za64rs", always_enabled, true), > MULTI_EXT_CFG_BOOL("ziccif", always_enabled, true), > (...) > > (I would create another macro for these named features but that's beside the > point) > > If we go this route then we'll need changes in how profiles declare these > internal > extensions, because now we're using CPU_CFG_OFFSET() with a specific boolean > for > each. I would remove them from the profile description, otherwise the code > would > attempt to do stuff with an 'always_enabled' bool, and instead just cite in a > comment that these other profile mandatory extensions are always enabled. > > > Another change on top of my head would be in isa_edata_arr[]. In this case we > would do: > > > >>>> + ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, always_enabled), > >>>> + ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, always_enabled), > >>>> + ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, always_enabled), > >>>> + ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, always_enabled), > > > There might be other changes needed but on top of my head I think this would > work. > Is that what you're referring to?
I would prefer your other offer: > Another idea would be to change the riscv,isa functions to > handle these extensions separately, then we can add them in the array without > a valid cpu->cfg offfset. but this also works. Alistair > > > Thanks, > > Daniel > > > > >> > >> And, in case we need to promote them as regular user extensions, having the > >> booleans in place make it easier to do so. Patch 6 is doing that with > >> 'svade'. > > > > If we plan to do that, I think that also makes sense. But I suspect no > > one has any plant to convert some of these to real configuration > > options. > > > >> > >> > >> Thanks, > >> > >> Daniel > >> > >> > >> We could create a single boolean that is always true in cpu->cfg and use it > >> for these entries. Another idea would be to change the riscv,isa functions > >> to > >> handle these extensions separately, then we can add them in the array > >> without > >> a valid cpu->cfg offfset. > > > > I think these are better ideas. > > > > Alistair > > > >> > >>> > >>> Alistair > >>> > >>>> > >>>> /* Vendor-specific custom extensions */ > >>>> bool ext_xtheadba; > >>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > >>>> index 90861cc065..6d5028cf84 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) > >>>> -- > >>>> 2.43.0 > >>>> > >>>>