| Message ID | 20240202152154.773253-4-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: named features riscv,isa, 'svade' rework | expand |
On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[]. > > Instead of adding one bool for each named feature that we'll always > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all > named features will point to it. This also means that KVM won't see > these features as always enable, which is our intention. > > If any accelerator adds support to disable one of these features, we'll > have to promote them to regular extensions and allow users to disable it > via command line. > > 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.c | 42 +++++++++++++++++++++++++++++++------- > target/riscv/cpu_cfg.h | 6 ++++++ > target/riscv/tcg/tcg-cpu.c | 2 ++ > 3 files changed, 43 insertions(+), 7 deletions(-) > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
On Sat, Feb 3, 2024 at 1:24 AM Daniel Henrique Barboza <dbarboza@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[]. > > Instead of adding one bool for each named feature that we'll always > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all > named features will point to it. This also means that KVM won't see > these features as always enable, which is our intention. > > If any accelerator adds support to disable one of these features, we'll > have to promote them to regular extensions and allow users to disable it > via command line. > > 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/cpu.c | 42 +++++++++++++++++++++++++++++++------- > target/riscv/cpu_cfg.h | 6 ++++++ > target/riscv/tcg/tcg-cpu.c | 2 ++ > 3 files changed, 43 insertions(+), 7 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 28d3cfa8ce..94843c4f6e 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_always_enabled), > + ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_always_enabled), > + ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_always_enabled), > + ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_always_enabled), > 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_always_enabled), > 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_always_enabled), > ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf), > + ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_always_enabled), > ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc), > + ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_always_enabled), > + ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_always_enabled), > 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), > @@ -1512,6 +1521,11 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +#define ALWAYS_ENABLED_FEATURE(_name) \ > + {.name = _name, \ > + .offset = CPU_CFG_OFFSET(ext_always_enabled), \ > + .enabled = true} > + > /* > * 'Named features' is the name we give to extensions that we > * don't want to expose to users. They are either immutable > @@ -1523,6 +1537,23 @@ 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 > + * in TCG since QEMU RISC-V does not have a cache > + * model. > + */ > + ALWAYS_ENABLED_FEATURE("za64rs"), > + ALWAYS_ENABLED_FEATURE("ziccif"), > + ALWAYS_ENABLED_FEATURE("ziccrse"), > + ALWAYS_ENABLED_FEATURE("ziccamoa"), > + ALWAYS_ENABLED_FEATURE("zicclsm"), > + ALWAYS_ENABLED_FEATURE("ssccptr"), > + > + /* Other named features that TCG always implements */ > + ALWAYS_ENABLED_FEATURE("sstvecd"), > + ALWAYS_ENABLED_FEATURE("sstvala"), > + ALWAYS_ENABLED_FEATURE("sscounterenw"), > + > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -2116,13 +2147,10 @@ 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 'named features' that are cache > + * related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa > + * and Zicclsm. They are always implemented in TCG and > + * doesn't need to be manually enabled by the profile. > */ > static RISCVCPUProfile RVA22U64 = { > .parent = NULL, > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h > index 698f926ab1..c5049ec664 100644 > --- a/target/riscv/cpu_cfg.h > +++ b/target/riscv/cpu_cfg.h > @@ -126,6 +126,12 @@ struct RISCVCPUConfig { > bool ext_svade; > bool ext_zic64b; > > + /* > + * Always 'true' boolean for named features > + * TCG always implement/can't be disabled. > + */ > + bool ext_always_enabled; > + > /* Vendor-specific custom extensions */ > bool ext_xtheadba; > bool ext_xtheadbb; > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index 90861cc065..673097c6e4 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -1347,6 +1347,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs) > RISCVCPU *cpu = RISCV_CPU(cs); > Object *obj = OBJECT(cpu); > > + cpu->cfg.ext_always_enabled = true; > + > misa_ext_user_opts = g_hash_table_new(NULL, g_direct_equal); > multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal); > riscv_cpu_add_user_properties(obj); > -- > 2.43.0 > >
On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[]. > > Instead of adding one bool for each named feature that we'll always > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all > named features will point to it. This also means that KVM won't see > these features as always enable, which is our intention. > > If any accelerator adds support to disable one of these features, we'll > have to promote them to regular extensions and allow users to disable it > via command line. > > After this patch, here's the riscv,isa from a buildroot using the > 'rva22s64' CPU: Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only present in "u" profiles? > # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# I want to raise my frustration with the crock we've been given here by RVI. Any "named feature" that just creates a name for something that already is assumed is completely useless, and DT property that is used to communicate it's presence cannot be used - instead the property needs to be inverted - indicating the absence of that named feature. Without the inversion, software that parses "riscv,isa" cannot make any determination based on the absence of the property - it could be parsing an old DT that does not have the property or it could be parsing the DT of a system that does not support the extension. This is part of why I deprecated `riscv,isa`. It's the same problem as with "zifencei" et al - does a system with `riscv,isa = "rv64imac"` support fence.i? Cheers, Conor. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.c | 42 +++++++++++++++++++++++++++++++------- > target/riscv/cpu_cfg.h | 6 ++++++ > target/riscv/tcg/tcg-cpu.c | 2 ++ > 3 files changed, 43 insertions(+), 7 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 28d3cfa8ce..94843c4f6e 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_always_enabled), > + ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_always_enabled), > + ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_always_enabled), > + ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_always_enabled), > 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_always_enabled), > 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_always_enabled), > ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf), > + ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_always_enabled), > ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc), > + ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_always_enabled), > + ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_always_enabled), > 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), > @@ -1512,6 +1521,11 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +#define ALWAYS_ENABLED_FEATURE(_name) \ > + {.name = _name, \ > + .offset = CPU_CFG_OFFSET(ext_always_enabled), \ > + .enabled = true} > + > /* > * 'Named features' is the name we give to extensions that we > * don't want to expose to users. They are either immutable > @@ -1523,6 +1537,23 @@ 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 > + * in TCG since QEMU RISC-V does not have a cache > + * model. > + */ > + ALWAYS_ENABLED_FEATURE("za64rs"), > + ALWAYS_ENABLED_FEATURE("ziccif"), > + ALWAYS_ENABLED_FEATURE("ziccrse"), > + ALWAYS_ENABLED_FEATURE("ziccamoa"), > + ALWAYS_ENABLED_FEATURE("zicclsm"), > + ALWAYS_ENABLED_FEATURE("ssccptr"), > + > + /* Other named features that TCG always implements */ > + ALWAYS_ENABLED_FEATURE("sstvecd"), > + ALWAYS_ENABLED_FEATURE("sstvala"), > + ALWAYS_ENABLED_FEATURE("sscounterenw"), > + > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -2116,13 +2147,10 @@ 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 'named features' that are cache > + * related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa > + * and Zicclsm. They are always implemented in TCG and > + * doesn't need to be manually enabled by the profile. > */ > static RISCVCPUProfile RVA22U64 = { > .parent = NULL, > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h > index 698f926ab1..c5049ec664 100644 > --- a/target/riscv/cpu_cfg.h > +++ b/target/riscv/cpu_cfg.h > @@ -126,6 +126,12 @@ struct RISCVCPUConfig { > bool ext_svade; > bool ext_zic64b; > > + /* > + * Always 'true' boolean for named features > + * TCG always implement/can't be disabled. > + */ > + bool ext_always_enabled; > + > /* Vendor-specific custom extensions */ > bool ext_xtheadba; > bool ext_xtheadbb; > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index 90861cc065..673097c6e4 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -1347,6 +1347,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs) > RISCVCPU *cpu = RISCV_CPU(cs); > Object *obj = OBJECT(cpu); > > + cpu->cfg.ext_always_enabled = true; > + > misa_ext_user_opts = g_hash_table_new(NULL, g_direct_equal); > multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal); > riscv_cpu_add_user_properties(obj); > -- > 2.43.0 > >
On 2/15/24 10:33, Conor Dooley wrote: > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[]. >> >> Instead of adding one bool for each named feature that we'll always >> implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in >> cpu->cfg. This bool will be set to 'true' in TCG accel init, and all >> named features will point to it. This also means that KVM won't see >> these features as always enable, which is our intention. >> >> If any accelerator adds support to disable one of these features, we'll >> have to promote them to regular extensions and allow users to disable it >> via command line. >> >> After this patch, here's the riscv,isa from a buildroot using the >> 'rva22s64' CPU: > > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only > present in "u" profiles? According to the specs I've read it seems the S profiles includes all extensions from U profiles. For RVA22: "The RVA22S64 mandatory unprivileged extensions include all the mandatory unprivileged extensions in RVA22U64." So rva22s64 will have zicclsm and all other extensions from its U profile too. > >> # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# > > I want to raise my frustration with the crock we've been given here by > RVI. Any "named feature" that just creates a name for something that > already is assumed is completely useless, and DT property that is used > to communicate it's presence cannot be used - instead the property needs > to be inverted - indicating the absence of that named feature. Let's say that I'm not the biggest fan of how these profile extensions are being dealt with in the spec :) the text is vague w.r.t whether zicclsm and others are actual extensions, or a 'named feature'( like we're calling here in QEMU) that is just a glorified way of saying, for example, "zic64b" instead of "all cache blocks have 64 bytes". Thanks, Daniel > > Without the inversion, software that parses "riscv,isa" cannot make any > determination based on the absence of the property - it could be parsing > an old DT that does not have the property or it could be parsing the DT > of a system that does not support the extension. > > This is part of why I deprecated `riscv,isa`. It's the same problem as > with "zifencei" et al - does a system with `riscv,isa = "rv64imac"` > support fence.i? > > Cheers, > Conor. > >> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >> --- >> target/riscv/cpu.c | 42 +++++++++++++++++++++++++++++++------- >> target/riscv/cpu_cfg.h | 6 ++++++ >> target/riscv/tcg/tcg-cpu.c | 2 ++ >> 3 files changed, 43 insertions(+), 7 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 28d3cfa8ce..94843c4f6e 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_always_enabled), >> + ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_always_enabled), >> + ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_always_enabled), >> + ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_always_enabled), >> 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_always_enabled), >> 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_always_enabled), >> ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf), >> + ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_always_enabled), >> ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc), >> + ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_always_enabled), >> + ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_always_enabled), >> 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), >> @@ -1512,6 +1521,11 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = { >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> +#define ALWAYS_ENABLED_FEATURE(_name) \ >> + {.name = _name, \ >> + .offset = CPU_CFG_OFFSET(ext_always_enabled), \ >> + .enabled = true} >> + >> /* >> * 'Named features' is the name we give to extensions that we >> * don't want to expose to users. They are either immutable >> @@ -1523,6 +1537,23 @@ 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 >> + * in TCG since QEMU RISC-V does not have a cache >> + * model. >> + */ >> + ALWAYS_ENABLED_FEATURE("za64rs"), >> + ALWAYS_ENABLED_FEATURE("ziccif"), >> + ALWAYS_ENABLED_FEATURE("ziccrse"), >> + ALWAYS_ENABLED_FEATURE("ziccamoa"), >> + ALWAYS_ENABLED_FEATURE("zicclsm"), >> + ALWAYS_ENABLED_FEATURE("ssccptr"), >> + >> + /* Other named features that TCG always implements */ >> + ALWAYS_ENABLED_FEATURE("sstvecd"), >> + ALWAYS_ENABLED_FEATURE("sstvala"), >> + ALWAYS_ENABLED_FEATURE("sscounterenw"), >> + >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> @@ -2116,13 +2147,10 @@ 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 'named features' that are cache >> + * related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa >> + * and Zicclsm. They are always implemented in TCG and >> + * doesn't need to be manually enabled by the profile. >> */ >> static RISCVCPUProfile RVA22U64 = { >> .parent = NULL, >> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h >> index 698f926ab1..c5049ec664 100644 >> --- a/target/riscv/cpu_cfg.h >> +++ b/target/riscv/cpu_cfg.h >> @@ -126,6 +126,12 @@ struct RISCVCPUConfig { >> bool ext_svade; >> bool ext_zic64b; >> >> + /* >> + * Always 'true' boolean for named features >> + * TCG always implement/can't be disabled. >> + */ >> + bool ext_always_enabled; >> + >> /* Vendor-specific custom extensions */ >> bool ext_xtheadba; >> bool ext_xtheadbb; >> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c >> index 90861cc065..673097c6e4 100644 >> --- a/target/riscv/tcg/tcg-cpu.c >> +++ b/target/riscv/tcg/tcg-cpu.c >> @@ -1347,6 +1347,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs) >> RISCVCPU *cpu = RISCV_CPU(cs); >> Object *obj = OBJECT(cpu); >> >> + cpu->cfg.ext_always_enabled = true; >> + >> misa_ext_user_opts = g_hash_table_new(NULL, g_direct_equal); >> multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal); >> riscv_cpu_add_user_properties(obj); >> -- >> 2.43.0 >> >>
On Thu, Feb 15, 2024 at 01:33:47PM +0000, Conor Dooley wrote: > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[]. > > > > Instead of adding one bool for each named feature that we'll always > > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in > > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all > > named features will point to it. This also means that KVM won't see > > these features as always enable, which is our intention. > > > > If any accelerator adds support to disable one of these features, we'll > > have to promote them to regular extensions and allow users to disable it > > via command line. > > > > After this patch, here's the riscv,isa from a buildroot using the > > 'rva22s64' CPU: > > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only > present in "u" profiles? "s" profiles mandate all the "u" profile mandatory extensions. For example 6.2.2 says """ The RVA22S64 mandatory unprivileged extensions include all the mandatory unprivileged extensions in RVA22U64. """ > > > # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# > > I want to raise my frustration with the crock we've been given here by > RVI. Any "named feature" that just creates a name for something that > already is assumed is completely useless, and DT property that is used > to communicate it's presence cannot be used - instead the property needs > to be inverted - indicating the absence of that named feature. > > Without the inversion, software that parses "riscv,isa" cannot make any > determination based on the absence of the property - it could be parsing > an old DT that does not have the property or it could be parsing the DT > of a system that does not support the extension. I'm guessing any platform which wants to advertise that it's compliant with a profile will update its hardware descriptions to ensure all the profile's mandatory extensions are presented. But, I think I understand your concern. If somebody is parsing the ISA string as way to determine if the platform is compliant with a profile, then they may get a false negative due to the ISA string missing a newly named feature. I'm not sure how much of a problem that will be in practice, though, since testing for profile compliance, just for the sake of it, doesn't seem very useful. Software really only needs to know which extensions are available and if it's an old feature that got newly named, then software likely already has another way of detecting it. > > This is part of why I deprecated `riscv,isa`. It's the same problem as > with "zifencei" et al - does a system with `riscv,isa = "rv64imac"` > support fence.i? Yes, there's a handful of these messy things and the first profiles expose them since they're trying to define them. Fingers crossed that the next profiles won't have to name old features. FWIW, I at least don't see any "This is a new extension name for this feature" notes in the RVA23 profile. Thanks, drew
On Thu, Feb 15, 2024 at 11:13:51AM -0300, Daniel Henrique Barboza wrote: ... > > I want to raise my frustration with the crock we've been given here by > > RVI. Any "named feature" that just creates a name for something that > > already is assumed is completely useless, and DT property that is used > > to communicate it's presence cannot be used - instead the property needs > > to be inverted - indicating the absence of that named feature. > > Let's say that I'm not the biggest fan of how these profile extensions are being > dealt with in the spec :) the text is vague w.r.t whether zicclsm and others > are actual extensions, or a 'named feature'( like we're calling here in QEMU) > The text is vague, I certainly didn't get it at first, but it's been clarified that these "named features" are considered extensions with the given names and those extensions are ratified at the time the profile in which they first appear is ratified. As I said in my other reply, I hope the need to name old features is behind us now that the first profiles are done. > that is just a glorified way of saying, for example, "zic64b" instead of "all > cache blocks have 64 bytes". The note that accompanies "Zic64b" also states that the cache blocks may be larger or smaller than 64 bytes. So, when a platform includes this "Zic64b" extension in its DT it doesn't mean all blocks are 64 bytes, it means they're all compatible with 64 bytes by either using 64-byte sub- blocks (when they're bigger) or by sequencing cache ops across multiple blocks (when they're smaller). So, while we can derive 'zic64b' from a platform which does have all blocks of size 64, some platforms will need to explicitly add it to the ISA string when they know they're compatible, since they'll be putting other block sizes in the block size descriptions. Thanks, drew
On Thu, Feb 15, 2024 at 03:26:18PM +0100, Andrew Jones wrote: > On Thu, Feb 15, 2024 at 01:33:47PM +0000, Conor Dooley wrote: > > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[]. > > > > > > Instead of adding one bool for each named feature that we'll always > > > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in > > > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all > > > named features will point to it. This also means that KVM won't see > > > these features as always enable, which is our intention. > > > > > > If any accelerator adds support to disable one of these features, we'll > > > have to promote them to regular extensions and allow users to disable it > > > via command line. > > > > > > After this patch, here's the riscv,isa from a buildroot using the > > > 'rva22s64' CPU: > > > > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only > > present in "u" profiles? > > "s" profiles mandate all the "u" profile mandatory extensions. For example > 6.2.2 says > > """ > The RVA22S64 mandatory unprivileged extensions include all the mandatory unprivileged > extensions in RVA22U64. > """ Doesn't that rule out emulating misaligned access in s-mode if you want to be profile compliant? > > > # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# > > > > I want to raise my frustration with the crock we've been given here by > > RVI. Any "named feature" that just creates a name for something that > > already is assumed is completely useless, and DT property that is used > > to communicate it's presence cannot be used - instead the property needs > > to be inverted - indicating the absence of that named feature. > > > > Without the inversion, software that parses "riscv,isa" cannot make any > > determination based on the absence of the property - it could be parsing > > an old DT that does not have the property or it could be parsing the DT > > of a system that does not support the extension. > > I'm guessing any platform which wants to advertise that it's compliant > with a profile will update its hardware descriptions to ensure all the > profile's mandatory extensions are presented. But, I think I understand > your concern. If somebody is parsing the ISA string as way to determine > if the platform is compliant with a profile, then they may get a false > negative due to the ISA string missing a newly named feature. Nah, you misunderstand me. I don't care at all about profiles or checking for compliance with one. I'm only interested in how my software can check that some feature is (or is not) supported. This creating a name for something implicit business is not a problem in and of itself, but putting then into "riscv,isa" is a pointless activity as it communicates nothing. > I'm not > sure how much of a problem that will be in practice, though, since testing > for profile compliance, just for the sake of it, doesn't seem very useful. > Software really only needs to know which extensions are available and if > it's an old feature that got newly named, then software likely already > has another way of detecting it. Right. That part is fine, but creating extensions for these things we previously assumed present gives me the impression that creating systems that do not support these features is valid. IFF that does happen, removing the string from "riscv,isa" isn't going to be able to communicate that the feature is unsupported. The commit message here says: > > > If any accelerator adds support to disable one of these features, we'll > > > have to promote them to regular extensions and allow users to disable it > > > via command line. Which is part of what prompted me here, since they cannot be handled in the same way that "regular extensions" are. > > This is part of why I deprecated `riscv,isa`. It's the same problem as > > with "zifencei" et al - does a system with `riscv,isa = "rv64imac"` > > support fence.i? > > Yes, there's a handful of these messy things and the first profiles > expose them since they're trying to define them. Fingers crossed that > the next profiles won't have to name old features. FWIW, I at least > don't see any "This is a new extension name for this feature" notes in > the RVA23 profile. > > Thanks, > drew
On Thu, Feb 15, 2024 at 04:34:32PM +0000, Conor Dooley wrote: > On Thu, Feb 15, 2024 at 03:26:18PM +0100, Andrew Jones wrote: > > On Thu, Feb 15, 2024 at 01:33:47PM +0000, Conor Dooley wrote: > > > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[]. > > > > > > > > Instead of adding one bool for each named feature that we'll always > > > > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in > > > > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all > > > > named features will point to it. This also means that KVM won't see > > > > these features as always enable, which is our intention. > > > > > > > > If any accelerator adds support to disable one of these features, we'll > > > > have to promote them to regular extensions and allow users to disable it > > > > via command line. > > > > > > > > After this patch, here's the riscv,isa from a buildroot using the > > > > 'rva22s64' CPU: > > > > > > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only > > > present in "u" profiles? > > > > "s" profiles mandate all the "u" profile mandatory extensions. For example > > 6.2.2 says > > > > """ > > The RVA22S64 mandatory unprivileged extensions include all the mandatory unprivileged > > extensions in RVA22U64. > > """ > > Doesn't that rule out emulating misaligned access in s-mode if you want > to be profile compliant? That's how I interpret it, but I'll defer to a profile spec author, or at least to somebody more confident of their spec interpretation skills. > > > > > # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# > > > > > > I want to raise my frustration with the crock we've been given here by > > > RVI. Any "named feature" that just creates a name for something that > > > already is assumed is completely useless, and DT property that is used > > > to communicate it's presence cannot be used - instead the property needs > > > to be inverted - indicating the absence of that named feature. > > > > > > Without the inversion, software that parses "riscv,isa" cannot make any > > > determination based on the absence of the property - it could be parsing > > > an old DT that does not have the property or it could be parsing the DT > > > of a system that does not support the extension. > > > > I'm guessing any platform which wants to advertise that it's compliant > > with a profile will update its hardware descriptions to ensure all the > > profile's mandatory extensions are presented. But, I think I understand > > your concern. If somebody is parsing the ISA string as way to determine > > if the platform is compliant with a profile, then they may get a false > > negative due to the ISA string missing a newly named feature. > > Nah, you misunderstand me. I don't care at all about profiles or > checking for compliance with one. I'm only interested in how my software > can check that some feature is (or is not) supported. This creating a name > for something implicit business is not a problem in and of itself, but > putting then into "riscv,isa" is a pointless activity as it communicates > nothing. > > > I'm not > > sure how much of a problem that will be in practice, though, since testing > > for profile compliance, just for the sake of it, doesn't seem very useful. > > Software really only needs to know which extensions are available and if > > it's an old feature that got newly named, then software likely already > > has another way of detecting it. > > Right. That part is fine, but creating extensions for these things we > previously assumed present gives me the impression that creating systems > that do not support these features is valid. IFF that does happen, > removing the string from "riscv,isa" isn't going to be able to > communicate that the feature is unsupported. Ah, now I think I understand the concern. The new names might as well be ignored because the absence of the names in the hardware descriptions is ambiguous. I guess I'd encourage software that has a role in advertising features to use the new names when it has detected the feature or assumes the feature is present (and presumably wouldn't be running if its assumption was wrong). If, for example, Linux puts a new name in /proc/cpuinfo after detecting or assuming the feature's presence, then it no longer matters that the hardware description had it or not from the perspective of the /proc/cpuinfo consumer (assuming they're aware of which kernel version they need). With these types of fixups and enough time, then hopefully most of the software ecosystem will able to live in ignorant bliss. > The commit message here > says: > > > > If any accelerator adds support to disable one of these features, we'll > > > > have to promote them to regular extensions and allow users to disable it > > > > via command line. > > Which is part of what prompted me here, since they cannot be handled in > the same way that "regular extensions" are. > From QEMU's perspective, they can. Linux or whatever software consuming the hardware descriptions may want to distrust the absence of newly named feature extensions and do their own checks, but that's not QEMU's concern. Actually, being able to disable these newly named features allows Linux and other software to test how they behave when the feature goes away. Do they crash fast and clean or do they go off in the weeds? Does their ad hoc detection actually work? Thanks, drew
On Thu, Feb 15, 2024 at 08:11:45PM +0100, Andrew Jones wrote: > On Thu, Feb 15, 2024 at 04:34:32PM +0000, Conor Dooley wrote: > > On Thu, Feb 15, 2024 at 03:26:18PM +0100, Andrew Jones wrote: > > > On Thu, Feb 15, 2024 at 01:33:47PM +0000, Conor Dooley wrote: > > > > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[]. > > > > > > > > > > Instead of adding one bool for each named feature that we'll always > > > > > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in > > > > > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all > > > > > named features will point to it. This also means that KVM won't see > > > > > these features as always enable, which is our intention. > > > > > > > > > > If any accelerator adds support to disable one of these features, we'll > > > > > have to promote them to regular extensions and allow users to disable it > > > > > via command line. > > > > > > > > > > After this patch, here's the riscv,isa from a buildroot using the > > > > > 'rva22s64' CPU: > > > > > > > > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only > > > > present in "u" profiles? > > > > > > "s" profiles mandate all the "u" profile mandatory extensions. For example > > > 6.2.2 says > > > > > > """ > > > The RVA22S64 mandatory unprivileged extensions include all the mandatory unprivileged > > > extensions in RVA22U64. > > > """ > > > > Doesn't that rule out emulating misaligned access in s-mode if you want > > to be profile compliant? > > That's how I interpret it, but I'll defer to a profile spec author, or > at least to somebody more confident of their spec interpretation skills. Hmm, actually it doesn't. Your firmware just needs to _also_ implement it. So your OS kernel could test whether or not the misaligned access performance is beans and then choose to emulate misaligned access itself. Not ideal, but better than I thought. > > > > > # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# > > > > > > > > I want to raise my frustration with the crock we've been given here by > > > > RVI. Any "named feature" that just creates a name for something that > > > > already is assumed is completely useless, and DT property that is used > > > > to communicate it's presence cannot be used - instead the property needs > > > > to be inverted - indicating the absence of that named feature. > > > > > > > > Without the inversion, software that parses "riscv,isa" cannot make any > > > > determination based on the absence of the property - it could be parsing > > > > an old DT that does not have the property or it could be parsing the DT > > > > of a system that does not support the extension. > > > > > > I'm guessing any platform which wants to advertise that it's compliant > > > with a profile will update its hardware descriptions to ensure all the > > > profile's mandatory extensions are presented. But, I think I understand > > > your concern. If somebody is parsing the ISA string as way to determine > > > if the platform is compliant with a profile, then they may get a false > > > negative due to the ISA string missing a newly named feature. > > > > Nah, you misunderstand me. I don't care at all about profiles or > > checking for compliance with one. I'm only interested in how my software > > can check that some feature is (or is not) supported. This creating a name > > for something implicit business is not a problem in and of itself, but > > putting then into "riscv,isa" is a pointless activity as it communicates > > nothing. > > > > > I'm not > > > sure how much of a problem that will be in practice, though, since testing > > > for profile compliance, just for the sake of it, doesn't seem very useful. > > > Software really only needs to know which extensions are available and if > > > it's an old feature that got newly named, then software likely already > > > has another way of detecting it. > > > > Right. That part is fine, but creating extensions for these things we > > previously assumed present gives me the impression that creating systems > > that do not support these features is valid. IFF that does happen, > > removing the string from "riscv,isa" isn't going to be able to > > communicate that the feature is unsupported. > > Ah, now I think I understand the concern. The new names might as well be > ignored because the absence of the names in the hardware descriptions is > ambiguous. Correct. > I guess I'd encourage software that has a role in advertising > features to use the new names when it has detected the feature or assumes > the feature is present (and presumably wouldn't be running if its > assumption was wrong). If, for example, Linux puts a new name in > /proc/cpuinfo after detecting or assuming the feature's presence, then it > no longer matters that the hardware description had it or not from the > perspective of the /proc/cpuinfo consumer (assuming they're aware of which > kernel version they need). With these types of fixups and enough time, > then hopefully most of the software ecosystem will able to live in > ignorant bliss. Yeah, that's effectively what we have to do. I started doing that for zifencei/zicsr in Linux and it should be done for anything else like this going forwards. > > The commit message here > > says: > > > > > If any accelerator adds support to disable one of these features, we'll > > > > > have to promote them to regular extensions and allow users to disable it > > > > > via command line. > > > > Which is part of what prompted me here, since they cannot be handled in > > the same way that "regular extensions" are. > > > > From QEMU's perspective, they can. No they can't. For a "regular extension" you populate the DT with the extension. For these extensions it has to put negated properties in the DT, otherwise it is incorrectly describing the hardware it is emulating. That is handling them differently in my book! If QEMU generates an incorrect DT representation of the hardware it is emulating, that's a QEMU bug. > Linux or whatever software consuming > the hardware descriptions may want to distrust the absence of newly > named feature extensions and do their own checks, but that's not QEMU's > concern. Software should be able to trust that the DT describes the system correctly. I can only speak for Linux here, but validating the DT is not the job of the running kernel - it should be a correct description. > Actually, being able to disable these newly named features allows > Linux and other software to test how they behave when the feature goes > away. That's helpful sure, but it doesn't absolve QEMU of having to correctly generate a DT. Thanks, Conor.
On Fri, Feb 16, 2024 at 6:00 AM Conor Dooley <conor@kernel.org> wrote: > > On Thu, Feb 15, 2024 at 08:11:45PM +0100, Andrew Jones wrote: > > On Thu, Feb 15, 2024 at 04:34:32PM +0000, Conor Dooley wrote: > > > On Thu, Feb 15, 2024 at 03:26:18PM +0100, Andrew Jones wrote: > > > > On Thu, Feb 15, 2024 at 01:33:47PM +0000, Conor Dooley wrote: > > > > > On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[]. > > > > > > > > > > > > Instead of adding one bool for each named feature that we'll always > > > > > > implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in > > > > > > cpu->cfg. This bool will be set to 'true' in TCG accel init, and all > > > > > > named features will point to it. This also means that KVM won't see > > > > > > these features as always enable, which is our intention. > > > > > > > > > > > > If any accelerator adds support to disable one of these features, we'll > > > > > > have to promote them to regular extensions and allow users to disable it > > > > > > via command line. > > > > > > > > > > > > After this patch, here's the riscv,isa from a buildroot using the > > > > > > 'rva22s64' CPU: > > > > > > > > > > Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only > > > > > present in "u" profiles? > > > > > > > > "s" profiles mandate all the "u" profile mandatory extensions. For example > > > > 6.2.2 says > > > > > > > > """ > > > > The RVA22S64 mandatory unprivileged extensions include all the mandatory unprivileged > > > > extensions in RVA22U64. > > > > """ > > > > > > Doesn't that rule out emulating misaligned access in s-mode if you want > > > to be profile compliant? > > > > That's how I interpret it, but I'll defer to a profile spec author, or > > at least to somebody more confident of their spec interpretation skills. > > Hmm, actually it doesn't. Your firmware just needs to _also_ implement > it. So your OS kernel could test whether or not the misaligned access > performance is beans and then choose to emulate misaligned access > itself. Not ideal, but better than I thought. > > > > > > > # 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# > > > > > > > > > > I want to raise my frustration with the crock we've been given here by > > > > > RVI. Any "named feature" that just creates a name for something that > > > > > already is assumed is completely useless, and DT property that is used > > > > > to communicate it's presence cannot be used - instead the property needs > > > > > to be inverted - indicating the absence of that named feature. > > > > > > > > > > Without the inversion, software that parses "riscv,isa" cannot make any > > > > > determination based on the absence of the property - it could be parsing > > > > > an old DT that does not have the property or it could be parsing the DT > > > > > of a system that does not support the extension. > > > > > > > > I'm guessing any platform which wants to advertise that it's compliant > > > > with a profile will update its hardware descriptions to ensure all the > > > > profile's mandatory extensions are presented. But, I think I understand > > > > your concern. If somebody is parsing the ISA string as way to determine > > > > if the platform is compliant with a profile, then they may get a false > > > > negative due to the ISA string missing a newly named feature. > > > > > > Nah, you misunderstand me. I don't care at all about profiles or > > > checking for compliance with one. I'm only interested in how my software > > > can check that some feature is (or is not) supported. This creating a name > > > for something implicit business is not a problem in and of itself, but > > > putting then into "riscv,isa" is a pointless activity as it communicates > > > nothing. > > > > > > > I'm not > > > > sure how much of a problem that will be in practice, though, since testing > > > > for profile compliance, just for the sake of it, doesn't seem very useful. > > > > Software really only needs to know which extensions are available and if > > > > it's an old feature that got newly named, then software likely already > > > > has another way of detecting it. > > > > > > Right. That part is fine, but creating extensions for these things we > > > previously assumed present gives me the impression that creating systems > > > that do not support these features is valid. IFF that does happen, > > > removing the string from "riscv,isa" isn't going to be able to > > > communicate that the feature is unsupported. > > > > Ah, now I think I understand the concern. The new names might as well be > > ignored because the absence of the names in the hardware descriptions is > > ambiguous. > > Correct. > > > I guess I'd encourage software that has a role in advertising > > features to use the new names when it has detected the feature or assumes > > the feature is present (and presumably wouldn't be running if its > > assumption was wrong). If, for example, Linux puts a new name in > > /proc/cpuinfo after detecting or assuming the feature's presence, then it > > no longer matters that the hardware description had it or not from the > > perspective of the /proc/cpuinfo consumer (assuming they're aware of which > > kernel version they need). With these types of fixups and enough time, > > then hopefully most of the software ecosystem will able to live in > > ignorant bliss. > > Yeah, that's effectively what we have to do. I started doing that for > zifencei/zicsr in Linux and it should be done for anything else like > this going forwards. > > > > The commit message here > > > says: > > > > > > If any accelerator adds support to disable one of these features, we'll > > > > > > have to promote them to regular extensions and allow users to disable it > > > > > > via command line. > > > > > > Which is part of what prompted me here, since they cannot be handled in > > > the same way that "regular extensions" are. > > > > > > > From QEMU's perspective, they can. > > No they can't. For a "regular extension" you populate the DT with the > extension. For these extensions it has to put negated properties in the > DT, otherwise it is incorrectly describing the hardware it is emulating. > That is handling them differently in my book! If QEMU generates an > incorrect DT representation of the hardware it is emulating, that's a > QEMU bug. QEMU listing the extensions that it supports seems to me to be the correct approach. It's clunky that the list of "extensions" are constantly changing. There isn't much we can do about that from a QEMU perspective though. Listing the hardware and what it supports is the job of the DT. I see your concern about what happens if the "extensions" are disabled though. Realislity they probably never will be. > > > Linux or whatever software consuming > > the hardware descriptions may want to distrust the absence of newly > > named feature extensions and do their own checks, but that's not QEMU's > > concern. > > Software should be able to trust that the DT describes the system > correctly. I can only speak for Linux here, but validating the DT is not > the job of the running kernel - it should be a correct description. AFAIK the DT is correct. We are describing the hardware within the scope of the DT spec. If a new node exists that describes what the hardware does not support we can update to support that as well. > > > Actually, being able to disable these newly named features allows > > Linux and other software to test how they behave when the feature goes > > away. > > That's helpful sure, but it doesn't absolve QEMU of having to correctly > generate a DT. I'm pretty sure there isn't anything for us to do differently here right? It's just a bad situation that we are trying to support. Alistair > > Thanks, > Conor.
> > No they can't. For a "regular extension" you populate the DT with the > > extension. For these extensions it has to put negated properties in the > > DT, otherwise it is incorrectly describing the hardware it is emulating. > > That is handling them differently in my book! If QEMU generates an > > incorrect DT representation of the hardware it is emulating, that's a > > QEMU bug. > > QEMU listing the extensions that it supports seems to me to be the > correct approach. > > It's clunky that the list of "extensions" are constantly changing. > There isn't much we can do about that from a QEMU perspective though. > > Listing the hardware and what it supports is the job of the DT. > > I see your concern about what happens if the "extensions" are disabled > though. Realislity they probably never will be. Yeah, it's something we can sweep under the rug unless/until someone wants to disable these things. > > > Linux or whatever software consuming > > > the hardware descriptions may want to distrust the absence of newly > > > named feature extensions and do their own checks, but that's not QEMU's > > > concern. > > > > Software should be able to trust that the DT describes the system > > correctly. I can only speak for Linux here, but validating the DT is not > > the job of the running kernel - it should be a correct description. > > AFAIK the DT is correct. We are describing the hardware within the > scope of the DT spec. > > If a new node exists that describes what the hardware does not support > we can update to support that as well. It won't be a new node property, it'll just be negated properties - eg riscv,isa-extensions = ..., "no-zicclsm"; That's what I mean when I say that these will not be able to be treated in the same way as any other extension, but it only applies iff someone wants to disable them. This isn't just a QEMU problem, but QEMU is the bleeding edge of "hardware" support, so it's cropping up here first (or maybe only :)) > > > Actually, being able to disable these newly named features allows > > > Linux and other software to test how they behave when the feature goes > > > away. > > > > That's helpful sure, but it doesn't absolve QEMU of having to correctly > > generate a DT. > > I'm pretty sure there isn't anything for us to do differently here > right? It's just a bad situation that we are trying to support. Until someone wants to turn them off, you can avoid doing anything differently, just like this amazing ascii art I found: _,-\/-,_ \ / \_.._/ _,/ \,_ / \ / \ ,\ ) ( /, (__/ .''. \__) \,_|| / | ||\ | | /|| \| () || () // || || // || || // || || // || /\ -- '' -'-' ^^' )( '^^-- '' -'-' miK (==) `~` Hopefully posting ostriches on the QEMU list isn't grounds for a ban, Conor.
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 28d3cfa8ce..94843c4f6e 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_always_enabled), + ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_always_enabled), + ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_always_enabled), + ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_always_enabled), 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_always_enabled), 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_always_enabled), ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf), + ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_always_enabled), ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc), + ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_always_enabled), + ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_always_enabled), 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), @@ -1512,6 +1521,11 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = { DEFINE_PROP_END_OF_LIST(), }; +#define ALWAYS_ENABLED_FEATURE(_name) \ + {.name = _name, \ + .offset = CPU_CFG_OFFSET(ext_always_enabled), \ + .enabled = true} + /* * 'Named features' is the name we give to extensions that we * don't want to expose to users. They are either immutable @@ -1523,6 +1537,23 @@ 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 + * in TCG since QEMU RISC-V does not have a cache + * model. + */ + ALWAYS_ENABLED_FEATURE("za64rs"), + ALWAYS_ENABLED_FEATURE("ziccif"), + ALWAYS_ENABLED_FEATURE("ziccrse"), + ALWAYS_ENABLED_FEATURE("ziccamoa"), + ALWAYS_ENABLED_FEATURE("zicclsm"), + ALWAYS_ENABLED_FEATURE("ssccptr"), + + /* Other named features that TCG always implements */ + ALWAYS_ENABLED_FEATURE("sstvecd"), + ALWAYS_ENABLED_FEATURE("sstvala"), + ALWAYS_ENABLED_FEATURE("sscounterenw"), + DEFINE_PROP_END_OF_LIST(), }; @@ -2116,13 +2147,10 @@ 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 'named features' that are cache + * related: Za64rs, Zic64b, Ziccif, Ziccrse, Ziccamoa + * and Zicclsm. They are always implemented in TCG and + * doesn't need to be manually enabled by the profile. */ static RISCVCPUProfile RVA22U64 = { .parent = NULL, diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h index 698f926ab1..c5049ec664 100644 --- a/target/riscv/cpu_cfg.h +++ b/target/riscv/cpu_cfg.h @@ -126,6 +126,12 @@ struct RISCVCPUConfig { bool ext_svade; bool ext_zic64b; + /* + * Always 'true' boolean for named features + * TCG always implement/can't be disabled. + */ + bool ext_always_enabled; + /* Vendor-specific custom extensions */ bool ext_xtheadba; bool ext_xtheadbb; diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index 90861cc065..673097c6e4 100644 --- a/target/riscv/tcg/tcg-cpu.c +++ b/target/riscv/tcg/tcg-cpu.c @@ -1347,6 +1347,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs) RISCVCPU *cpu = RISCV_CPU(cs); Object *obj = OBJECT(cpu); + cpu->cfg.ext_always_enabled = true; + misa_ext_user_opts = g_hash_table_new(NULL, g_direct_equal); multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal); riscv_cpu_add_user_properties(obj);
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[]. Instead of adding one bool for each named feature that we'll always implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in cpu->cfg. This bool will be set to 'true' in TCG accel init, and all named features will point to it. This also means that KVM won't see these features as always enable, which is our intention. If any accelerator adds support to disable one of these features, we'll have to promote them to regular extensions and allow users to disable it via command line. 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_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt# Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/cpu.c | 42 +++++++++++++++++++++++++++++++------- target/riscv/cpu_cfg.h | 6 ++++++ target/riscv/tcg/tcg-cpu.c | 2 ++ 3 files changed, 43 insertions(+), 7 deletions(-)