| Message ID | 20240308111152.2856137-10-alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/34] target/riscv: Update $ra with current $pc in trans_cm_jalt() | expand |
Hi Alistair, Since this series, I'm getting warnings when using a CPU not supporting the latest ISA, such as the SIFIVE_E series. | $ qemu-system-riscv32 -M sifive_e | qemu-system-riscv32: warning: disabling zic64b extension for hart 0x00000000 because privilege spec version does not match | qemu-system-riscv32: warning: disabling ziccamoa extension for hart 0x00000000 because privilege spec version does not match Those are always enabled during the initialization but riscv_cpu_disable_priv_spec_isa_exts is detecting them as unsupported by the CPU and thus disabling them. However, are those extensions different from zicnptr and zihpm extensions ? As they are not enabled by the same mean, I'm wondering about that ? Or do we want to skip their ISA verification as well ? Thanks, Clément On Fri, Mar 8, 2024 at 12:13 PM Alistair Francis <alistair23@gmail.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[]. > > 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> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > Message-ID: <20240215223955.969568-4-dbarboza@ventanamicro.com> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > --- > target/riscv/cpu_cfg.h | 6 ++++++ > target/riscv/cpu.c | 42 +++++++++++++++++++++++++++++++------- > target/riscv/tcg/tcg-cpu.c | 2 ++ > 3 files changed, 43 insertions(+), 7 deletions(-) > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h > index e68a4ddb92..be39870691 100644 > --- a/target/riscv/cpu_cfg.h > +++ b/target/riscv/cpu_cfg.h > @@ -128,6 +128,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/cpu.c b/target/riscv/cpu.c > index f0cd408237..4c4fa79145 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -102,6 +102,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), > @@ -110,6 +114,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(zaamo, PRIV_VERSION_1_12_0, ext_zaamo), > ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas), > ISA_EXT_DATA_ENTRY(zalrsc, PRIV_VERSION_1_12_0, ext_zalrsc), > @@ -173,8 +178,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), > @@ -1567,6 +1576,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 > @@ -1578,6 +1592,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(), > }; > > @@ -2171,13 +2202,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/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index 2307999387..ea763abb31 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -1315,6 +1315,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.44.0 > >
On 3/11/24 10:47, Clément Chigot wrote: > Hi Alistair, > > Since this series, I'm getting warnings when using a CPU not > supporting the latest ISA, such as the SIFIVE_E series. > | $ qemu-system-riscv32 -M sifive_e > | qemu-system-riscv32: warning: disabling zic64b extension for hart > 0x00000000 because privilege spec version does not match > | qemu-system-riscv32: warning: disabling ziccamoa extension for > hart 0x00000000 because privilege spec version does not match > > Those are always enabled during the initialization but > riscv_cpu_disable_priv_spec_isa_exts is detecting them as unsupported > by the CPU and thus disabling them. > However, are those extensions different from zicnptr and zihpm > extensions ? As they are not enabled by the same mean, I'm wondering > about that ? Or do we want to skip their ISA verification as well ? Disabling zihpm and zicntr will impact the CPU because we'll remove timers from the emulation. An exception was important in this case to not change existing behavior. Disabling 'zic64b' and 'ziccamoa' will only impact riscv,isa. We won't change any cache block size when disabling zic64ba and 'ziccamoa' does nothing because it's cache related and we do not implement cache. All this said, I'm less than pleased with these warnings. And they'll be getting worse and worse: priv_spec 1.13 is around the corner, we'll start adding profile extensions that will be marked as 1.13, some of them will be mapped to something that we already support, and then vendor CPUs will be getting more and more warnings because they'll be running an old priv_spec. My suggestion is to not send user warnings when disabling extensions for vendor CPUs. Vendor CPUs don't allow extensions to be enabled, so if an extension is violating priv_spec it's not the user fault. We can add a LOG_GUEST_ERROR when disabling exts but user warnings will only be shown for non-vendor CPUs. Thanks, Daniel > > Thanks, > Clément > > On Fri, Mar 8, 2024 at 12:13 PM Alistair Francis <alistair23@gmail.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[]. >> >> 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> >> Reviewed-by: Andrew Jones <ajones@ventanamicro.com> >> Message-ID: <20240215223955.969568-4-dbarboza@ventanamicro.com> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com> >> --- >> target/riscv/cpu_cfg.h | 6 ++++++ >> target/riscv/cpu.c | 42 +++++++++++++++++++++++++++++++------- >> target/riscv/tcg/tcg-cpu.c | 2 ++ >> 3 files changed, 43 insertions(+), 7 deletions(-) >> >> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h >> index e68a4ddb92..be39870691 100644 >> --- a/target/riscv/cpu_cfg.h >> +++ b/target/riscv/cpu_cfg.h >> @@ -128,6 +128,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/cpu.c b/target/riscv/cpu.c >> index f0cd408237..4c4fa79145 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -102,6 +102,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), >> @@ -110,6 +114,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(zaamo, PRIV_VERSION_1_12_0, ext_zaamo), >> ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas), >> ISA_EXT_DATA_ENTRY(zalrsc, PRIV_VERSION_1_12_0, ext_zalrsc), >> @@ -173,8 +178,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), >> @@ -1567,6 +1576,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 >> @@ -1578,6 +1592,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(), >> }; >> >> @@ -2171,13 +2202,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/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c >> index 2307999387..ea763abb31 100644 >> --- a/target/riscv/tcg/tcg-cpu.c >> +++ b/target/riscv/tcg/tcg-cpu.c >> @@ -1315,6 +1315,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.44.0 >> >>
Hi Clément, Alistair, On 3/11/24 11:39, Daniel Henrique Barboza wrote: > > > On 3/11/24 10:47, Clément Chigot wrote: >> Hi Alistair, >> >> Since this series, I'm getting warnings when using a CPU not >> supporting the latest ISA, such as the SIFIVE_E series. >> | $ qemu-system-riscv32 -M sifive_e >> | qemu-system-riscv32: warning: disabling zic64b extension for hart >> 0x00000000 because privilege spec version does not match >> | qemu-system-riscv32: warning: disabling ziccamoa extension for >> hart 0x00000000 because privilege spec version does not match >> >> Those are always enabled during the initialization but >> riscv_cpu_disable_priv_spec_isa_exts is detecting them as unsupported >> by the CPU and thus disabling them. >> However, are those extensions different from zicnptr and zihpm >> extensions ? As they are not enabled by the same mean, I'm wondering >> about that ? Or do we want to skip their ISA verification as well ? > > Disabling zihpm and zicntr will impact the CPU because we'll remove timers from > the emulation. An exception was important in this case to not change existing > behavior. > > Disabling 'zic64b' and 'ziccamoa' will only impact riscv,isa. We won't change > any cache block size when disabling zic64ba and 'ziccamoa' does nothing > because it's cache related and we do not implement cache. > > All this said, I'm less than pleased with these warnings. And they'll be getting > worse and worse: priv_spec 1.13 is around the corner, we'll start adding profile > extensions that will be marked as 1.13, some of them will be mapped to something > that we already support, and then vendor CPUs will be getting more and more > warnings because they'll be running an old priv_spec. > > My suggestion is to not send user warnings when disabling extensions for vendor > CPUs. Vendor CPUs don't allow extensions to be enabled, so if an extension is > violating priv_spec it's not the user fault. We can add a LOG_GUEST_ERROR when > disabling exts but user warnings will only be shown for non-vendor CPUs. I discussed this issue with Drew and he suggested that the problem is that we're enabling these innate extensions without regard for the actual priv_ver of the CPU being used, and a better fix would be to prevent this from happening in the first place. I'll send a patch with this approach. Thanks, Daniel > > > Thanks, > > Daniel > > > >> >> Thanks, >> Clément >> >> On Fri, Mar 8, 2024 at 12:13 PM Alistair Francis <alistair23@gmail.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[]. >>> >>> 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> >>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com> >>> Message-ID: <20240215223955.969568-4-dbarboza@ventanamicro.com> >>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com> >>> --- >>> target/riscv/cpu_cfg.h | 6 ++++++ >>> target/riscv/cpu.c | 42 +++++++++++++++++++++++++++++++------- >>> target/riscv/tcg/tcg-cpu.c | 2 ++ >>> 3 files changed, 43 insertions(+), 7 deletions(-) >>> >>> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h >>> index e68a4ddb92..be39870691 100644 >>> --- a/target/riscv/cpu_cfg.h >>> +++ b/target/riscv/cpu_cfg.h >>> @@ -128,6 +128,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/cpu.c b/target/riscv/cpu.c >>> index f0cd408237..4c4fa79145 100644 >>> --- a/target/riscv/cpu.c >>> +++ b/target/riscv/cpu.c >>> @@ -102,6 +102,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), >>> @@ -110,6 +114,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(zaamo, PRIV_VERSION_1_12_0, ext_zaamo), >>> ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas), >>> ISA_EXT_DATA_ENTRY(zalrsc, PRIV_VERSION_1_12_0, ext_zalrsc), >>> @@ -173,8 +178,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), >>> @@ -1567,6 +1576,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 >>> @@ -1578,6 +1592,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(), >>> }; >>> >>> @@ -2171,13 +2202,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/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c >>> index 2307999387..ea763abb31 100644 >>> --- a/target/riscv/tcg/tcg-cpu.c >>> +++ b/target/riscv/tcg/tcg-cpu.c >>> @@ -1315,6 +1315,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.44.0 >>> >>>
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h index e68a4ddb92..be39870691 100644 --- a/target/riscv/cpu_cfg.h +++ b/target/riscv/cpu_cfg.h @@ -128,6 +128,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/cpu.c b/target/riscv/cpu.c index f0cd408237..4c4fa79145 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -102,6 +102,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), @@ -110,6 +114,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(zaamo, PRIV_VERSION_1_12_0, ext_zaamo), ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas), ISA_EXT_DATA_ENTRY(zalrsc, PRIV_VERSION_1_12_0, ext_zalrsc), @@ -173,8 +178,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), @@ -1567,6 +1576,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 @@ -1578,6 +1592,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(), }; @@ -2171,13 +2202,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/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index 2307999387..ea763abb31 100644 --- a/target/riscv/tcg/tcg-cpu.c +++ b/target/riscv/tcg/tcg-cpu.c @@ -1315,6 +1315,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);