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

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

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