diff mbox series

[PULL,09/34] target/riscv: add remaining named features

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

Commit Message

Alistair Francis March 8, 2024, 11:11 a.m. UTC
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(-)

Comments

Clément Chigot March 11, 2024, 1:47 p.m. UTC | #1
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
>
>
Daniel Henrique Barboza March 11, 2024, 2:39 p.m. UTC | #2
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
>>
>>
Daniel Henrique Barboza March 12, 2024, 9:26 a.m. UTC | #3
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 mbox series

Patch

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);