| Message ID | 20240312203214.350980-1-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-9.0] target/riscv: do not enable all named features by default | expand |
On Tue, Mar 12, 2024 at 9:32 PM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > Commit 3b8022269c added the capability of named features/profile > extensions to be added in riscv,isa. To do that we had to assign priv > versions for each one of them in isa_edata_arr[]. But this resulted in a > side-effect: vendor CPUs that aren't running priv_version_latest started > to experience warnings for these profile extensions [1]: > > | $ 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 > > This is benign as far as the CPU behavior is concerned since disabling > both extensions is a no-op (aside from riscv,isa). But the warnings are > unpleasant to deal with, especially because we're sending user warnings > for extensions that users can't enable/disable. > > Instead of enabling all named features all the time, separate them by > priv version. During finalize() time, after we decided which > priv_version the CPU is running, enable/disable all the named extensions > based on the priv spec chosen. This will be enough for a bug fix, but as > a future work we should look into how we can name these extensions in a > way that we don't need an explicit ext_name => priv_ver as we're doing > here. > > The named extensions being added in isa_edata_arr[] that will be > enabled/disabled based solely on priv version can be removed from > riscv_cpu_named_features[]. 'zic64b' is an extension that can be > disabled based on block sizes so it'll retain its own flag and entry. > > [1] https://lists.gnu.org/archive/html/qemu-devel/2024-03/msg02592.html > > Reported-by: Clément Chigot <chigot@adacore.com> > Fixes: 3b8022269c ("target/riscv: add riscv,isa to named features") > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.c | 40 +++++++++----------------------------- > target/riscv/cpu_cfg.h | 8 +++++--- > target/riscv/tcg/tcg-cpu.c | 14 ++++++++++--- > 3 files changed, 25 insertions(+), 37 deletions(-) Thanks for the quick patch ! Tested-by: Clément Chigot <chigot@adacore.com>
On Tue, Mar 12, 2024 at 05:32:14PM -0300, Daniel Henrique Barboza wrote: > Commit 3b8022269c added the capability of named features/profile > extensions to be added in riscv,isa. To do that we had to assign priv > versions for each one of them in isa_edata_arr[]. But this resulted in a > side-effect: vendor CPUs that aren't running priv_version_latest started > to experience warnings for these profile extensions [1]: > > | $ 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 > > This is benign as far as the CPU behavior is concerned since disabling > both extensions is a no-op (aside from riscv,isa). But the warnings are > unpleasant to deal with, especially because we're sending user warnings > for extensions that users can't enable/disable. > > Instead of enabling all named features all the time, separate them by > priv version. During finalize() time, after we decided which > priv_version the CPU is running, enable/disable all the named extensions > based on the priv spec chosen. This will be enough for a bug fix, but as > a future work we should look into how we can name these extensions in a > way that we don't need an explicit ext_name => priv_ver as we're doing > here. > > The named extensions being added in isa_edata_arr[] that will be > enabled/disabled based solely on priv version can be removed from > riscv_cpu_named_features[]. 'zic64b' is an extension that can be > disabled based on block sizes so it'll retain its own flag and entry. > > [1] https://lists.gnu.org/archive/html/qemu-devel/2024-03/msg02592.html > > Reported-by: Clément Chigot <chigot@adacore.com> > Fixes: 3b8022269c ("target/riscv: add riscv,isa to named features") > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.c | 40 +++++++++----------------------------- > target/riscv/cpu_cfg.h | 8 +++++--- > target/riscv/tcg/tcg-cpu.c | 14 ++++++++++--- > 3 files changed, 25 insertions(+), 37 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 5a48d30828..1da5417764 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -102,10 +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(ziccamoa, PRIV_VERSION_1_11_0, has_priv_1_11), > + ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, has_priv_1_11), > + ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, has_priv_1_11), > + ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, has_priv_1_11), > 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), > @@ -114,7 +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(za64rs, PRIV_VERSION_1_12_0, has_priv_1_11), > 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), > @@ -179,12 +179,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(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11), > 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(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12), > 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(sstvala, PRIV_VERSION_1_12_0, has_priv_1_12), > + ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, has_priv_1_12), > 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), > @@ -1575,11 +1575,6 @@ 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 > @@ -1590,23 +1585,6 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = { > const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = { > 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(), > }; > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h > index 2040b90da0..cb750154bd 100644 > --- a/target/riscv/cpu_cfg.h > +++ b/target/riscv/cpu_cfg.h > @@ -130,10 +130,12 @@ struct RISCVCPUConfig { > bool ext_zic64b; > > /* > - * Always 'true' boolean for named features > - * TCG always implement/can't be disabled. > + * Always 'true' booleans for named features > + * TCG always implement/can't be user disabled, > + * based on spec version. > */ > - bool ext_always_enabled; > + bool has_priv_1_12; > + bool has_priv_1_11; > > /* Vendor-specific custom extensions */ > bool ext_xtheadba; > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index ab6db817db..63192ef54f 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -315,9 +315,19 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu) > > static void riscv_cpu_update_named_features(RISCVCPU *cpu) > { > + if (cpu->env.priv_ver >= PRIV_VERSION_1_11_0) { > + cpu->cfg.has_priv_1_11 = true; > + } > + > + if (cpu->env.priv_ver >= PRIV_VERSION_1_12_0) { > + cpu->cfg.has_priv_1_12 = true; > + } > + > + /* zic64b is 1.12 or later */ > cpu->cfg.ext_zic64b = cpu->cfg.cbom_blocksize == 64 && > cpu->cfg.cbop_blocksize == 64 && > - cpu->cfg.cboz_blocksize == 64; > + cpu->cfg.cboz_blocksize == 64 && > + cpu->cfg.has_priv_1_12; > } > > static void riscv_cpu_validate_g(RISCVCPU *cpu) > @@ -1316,8 +1326,6 @@ 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.2 > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
On Wed, Mar 13, 2024 at 6:33 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > Commit 3b8022269c added the capability of named features/profile > extensions to be added in riscv,isa. To do that we had to assign priv > versions for each one of them in isa_edata_arr[]. But this resulted in a > side-effect: vendor CPUs that aren't running priv_version_latest started > to experience warnings for these profile extensions [1]: > > | $ 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 > > This is benign as far as the CPU behavior is concerned since disabling > both extensions is a no-op (aside from riscv,isa). But the warnings are > unpleasant to deal with, especially because we're sending user warnings > for extensions that users can't enable/disable. > > Instead of enabling all named features all the time, separate them by > priv version. During finalize() time, after we decided which > priv_version the CPU is running, enable/disable all the named extensions > based on the priv spec chosen. This will be enough for a bug fix, but as > a future work we should look into how we can name these extensions in a > way that we don't need an explicit ext_name => priv_ver as we're doing > here. > > The named extensions being added in isa_edata_arr[] that will be > enabled/disabled based solely on priv version can be removed from > riscv_cpu_named_features[]. 'zic64b' is an extension that can be > disabled based on block sizes so it'll retain its own flag and entry. > > [1] https://lists.gnu.org/archive/html/qemu-devel/2024-03/msg02592.html > > Reported-by: Clément Chigot <chigot@adacore.com> > Fixes: 3b8022269c ("target/riscv: add riscv,isa to named features") > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/cpu.c | 40 +++++++++----------------------------- > target/riscv/cpu_cfg.h | 8 +++++--- > target/riscv/tcg/tcg-cpu.c | 14 ++++++++++--- > 3 files changed, 25 insertions(+), 37 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 5a48d30828..1da5417764 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -102,10 +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(ziccamoa, PRIV_VERSION_1_11_0, has_priv_1_11), > + ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, has_priv_1_11), > + ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, has_priv_1_11), > + ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, has_priv_1_11), > 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), > @@ -114,7 +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(za64rs, PRIV_VERSION_1_12_0, has_priv_1_11), > 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), > @@ -179,12 +179,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(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11), > 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(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12), > 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(sstvala, PRIV_VERSION_1_12_0, has_priv_1_12), > + ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, has_priv_1_12), > 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), > @@ -1575,11 +1575,6 @@ 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 > @@ -1590,23 +1585,6 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = { > const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = { > 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(), > }; > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h > index 2040b90da0..cb750154bd 100644 > --- a/target/riscv/cpu_cfg.h > +++ b/target/riscv/cpu_cfg.h > @@ -130,10 +130,12 @@ struct RISCVCPUConfig { > bool ext_zic64b; > > /* > - * Always 'true' boolean for named features > - * TCG always implement/can't be disabled. > + * Always 'true' booleans for named features > + * TCG always implement/can't be user disabled, > + * based on spec version. > */ > - bool ext_always_enabled; > + bool has_priv_1_12; > + bool has_priv_1_11; > > /* Vendor-specific custom extensions */ > bool ext_xtheadba; > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index ab6db817db..63192ef54f 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -315,9 +315,19 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu) > > static void riscv_cpu_update_named_features(RISCVCPU *cpu) > { > + if (cpu->env.priv_ver >= PRIV_VERSION_1_11_0) { > + cpu->cfg.has_priv_1_11 = true; > + } > + > + if (cpu->env.priv_ver >= PRIV_VERSION_1_12_0) { > + cpu->cfg.has_priv_1_12 = true; > + } > + > + /* zic64b is 1.12 or later */ > cpu->cfg.ext_zic64b = cpu->cfg.cbom_blocksize == 64 && > cpu->cfg.cbop_blocksize == 64 && > - cpu->cfg.cboz_blocksize == 64; > + cpu->cfg.cboz_blocksize == 64 && > + cpu->cfg.has_priv_1_12; > } > > static void riscv_cpu_validate_g(RISCVCPU *cpu) > @@ -1316,8 +1326,6 @@ 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.2 > >
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 5a48d30828..1da5417764 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -102,10 +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(ziccamoa, PRIV_VERSION_1_11_0, has_priv_1_11), + ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, has_priv_1_11), + ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, has_priv_1_11), + ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, has_priv_1_11), 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), @@ -114,7 +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(za64rs, PRIV_VERSION_1_12_0, has_priv_1_11), 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), @@ -179,12 +179,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(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11), 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(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12), 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(sstvala, PRIV_VERSION_1_12_0, has_priv_1_12), + ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, has_priv_1_12), 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), @@ -1575,11 +1575,6 @@ 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 @@ -1590,23 +1585,6 @@ const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = { const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = { 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(), }; diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h index 2040b90da0..cb750154bd 100644 --- a/target/riscv/cpu_cfg.h +++ b/target/riscv/cpu_cfg.h @@ -130,10 +130,12 @@ struct RISCVCPUConfig { bool ext_zic64b; /* - * Always 'true' boolean for named features - * TCG always implement/can't be disabled. + * Always 'true' booleans for named features + * TCG always implement/can't be user disabled, + * based on spec version. */ - bool ext_always_enabled; + bool has_priv_1_12; + bool has_priv_1_11; /* Vendor-specific custom extensions */ bool ext_xtheadba; diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index ab6db817db..63192ef54f 100644 --- a/target/riscv/tcg/tcg-cpu.c +++ b/target/riscv/tcg/tcg-cpu.c @@ -315,9 +315,19 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu) static void riscv_cpu_update_named_features(RISCVCPU *cpu) { + if (cpu->env.priv_ver >= PRIV_VERSION_1_11_0) { + cpu->cfg.has_priv_1_11 = true; + } + + if (cpu->env.priv_ver >= PRIV_VERSION_1_12_0) { + cpu->cfg.has_priv_1_12 = true; + } + + /* zic64b is 1.12 or later */ cpu->cfg.ext_zic64b = cpu->cfg.cbom_blocksize == 64 && cpu->cfg.cbop_blocksize == 64 && - cpu->cfg.cboz_blocksize == 64; + cpu->cfg.cboz_blocksize == 64 && + cpu->cfg.has_priv_1_12; } static void riscv_cpu_validate_g(RISCVCPU *cpu) @@ -1316,8 +1326,6 @@ 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);
Commit 3b8022269c added the capability of named features/profile extensions to be added in riscv,isa. To do that we had to assign priv versions for each one of them in isa_edata_arr[]. But this resulted in a side-effect: vendor CPUs that aren't running priv_version_latest started to experience warnings for these profile extensions [1]: | $ 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 This is benign as far as the CPU behavior is concerned since disabling both extensions is a no-op (aside from riscv,isa). But the warnings are unpleasant to deal with, especially because we're sending user warnings for extensions that users can't enable/disable. Instead of enabling all named features all the time, separate them by priv version. During finalize() time, after we decided which priv_version the CPU is running, enable/disable all the named extensions based on the priv spec chosen. This will be enough for a bug fix, but as a future work we should look into how we can name these extensions in a way that we don't need an explicit ext_name => priv_ver as we're doing here. The named extensions being added in isa_edata_arr[] that will be enabled/disabled based solely on priv version can be removed from riscv_cpu_named_features[]. 'zic64b' is an extension that can be disabled based on block sizes so it'll retain its own flag and entry. [1] https://lists.gnu.org/archive/html/qemu-devel/2024-03/msg02592.html Reported-by: Clément Chigot <chigot@adacore.com> Fixes: 3b8022269c ("target/riscv: add riscv,isa to named features") Suggested-by: Andrew Jones <ajones@ventanamicro.com> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/cpu.c | 40 +++++++++----------------------------- target/riscv/cpu_cfg.h | 8 +++++--- target/riscv/tcg/tcg-cpu.c | 14 ++++++++++--- 3 files changed, 25 insertions(+), 37 deletions(-)