Re: [PATCH 3/6] target/riscv: add remaining named features

[Daniel Henrique Barboza]

On 2/1/24 23:14, Alistair Francis wrote:
> On Thu, Feb 1, 2024 at 5:15 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> > On 1/29/24 22:10, Alistair Francis wrote:
> > > On Fri, Jan 26, 2024 at 5:54 AM Daniel Henrique Barboza
> > > <dbarboza@ventanamicro.com> wrote:
> > > > The RVA22U64 and RVA22S64 profiles mandates certain extensions that,
> > > > until now, we were implying that they were available.
> > > >
> > > > We can't do this anymore since named features also has a riscv,isa
> > > > entry.  Let's add them to riscv_cpu_named_features[].
> > > >
> > > > They will also need to be explicitly enabled in both profile
> > > > descriptions. TCG will enable the named features it already implements,
> > > > other accelerators are free to handle it as they like.
> > > >
> > > > 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_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
> > > >
> > > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > > ---
> > > >    target/riscv/cpu.c         | 41 +++++++++++++++++++++++++++++---------
> > > >    target/riscv/cpu_cfg.h     |  9 +++++++++
> > > >    target/riscv/tcg/tcg-cpu.c | 19 +++++++++++++++++-
> > > >    3 files changed, 59 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index 28d3cfa8ce..1ecd8a57ed 100644
> > > > --- a/target/riscv/cpu.c
> > > > +++ b/target/riscv/cpu.c
> > > > @@ -101,6 +101,10 @@ const RISCVIsaExtData isa_edata_arr[] = {
> > > >        ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
> > > >        ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
> > > >        ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
> > > > +    ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, ext_ziccamoa),
> > > > +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_ziccif),
> > > > +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_zicclsm),
> > > > +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_ziccrse),
> > > >        ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
> > > >        ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
> > > >        ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr),
> > > > @@ -109,6 +113,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
> > > >        ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
> > > >        ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
> > > >        ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
> > > > +    ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, ext_za64rs),
> > > >        ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas),
> > > >        ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs),
> > > >        ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa),
> > > > @@ -170,8 +175,12 @@ const RISCVIsaExtData isa_edata_arr[] = {
> > > >        ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
> > > >        ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
> > > >        ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
> > > > +    ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, ext_ssccptr),
> > > >        ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
> > > > +    ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, ext_sscounterenw),
> > > >        ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> > > > +    ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, ext_sstvala),
> > > > +    ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, ext_sstvecd),
> > > >        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),
> > > > @@ -1523,6 +1532,22 @@ 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
> > > > +     * since QEMU RISC-V does not have a cache model.
> > > > +     */
> > > > +    MULTI_EXT_CFG_BOOL("za64rs", ext_za64rs, true),
> > > > +    MULTI_EXT_CFG_BOOL("ziccif", ext_ziccif, true),
> > > > +    MULTI_EXT_CFG_BOOL("ziccrse", ext_ziccrse, true),
> > > > +    MULTI_EXT_CFG_BOOL("ziccamoa", ext_ziccamoa, true),
> > > > +    MULTI_EXT_CFG_BOOL("zicclsm", ext_zicclsm, true),
> > > > +    MULTI_EXT_CFG_BOOL("ssccptr", ext_ssccptr, true),
> > > > +
> > > > +    /* Other named features that QEMU TCG always implements */
> > > > +    MULTI_EXT_CFG_BOOL("sstvecd", ext_sstvecd, true),
> > > > +    MULTI_EXT_CFG_BOOL("sstvala", ext_sstvala, true),
> > > > +    MULTI_EXT_CFG_BOOL("sscounterenw", ext_sscounterenw, true),
> > > > +
> > > >        DEFINE_PROP_END_OF_LIST(),
> > > >    };
> > > >
> > > > @@ -2116,13 +2141,8 @@ 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 cache related extensions: Za64rs,
> > > > + * Ziccif, Ziccrse, Ziccamoa and Zicclsm.
> > > >     */
> > > >    static RISCVCPUProfile RVA22U64 = {
> > > >        .parent = NULL,
> > > > @@ -2139,7 +2159,9 @@ static RISCVCPUProfile RVA22U64 = {
> > > >            CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz),
> > > >
> > > >            /* mandatory named features for this profile */
> > > > -        CPU_CFG_OFFSET(ext_zic64b),
> > > > +        CPU_CFG_OFFSET(ext_za64rs), CPU_CFG_OFFSET(ext_zic64b),
> > > > +        CPU_CFG_OFFSET(ext_ziccif), CPU_CFG_OFFSET(ext_ziccrse),
> > > > +        CPU_CFG_OFFSET(ext_ziccamoa), CPU_CFG_OFFSET(ext_zicclsm),
> > > >
> > > >            RISCV_PROFILE_EXT_LIST_END
> > > >        }
> > > > @@ -2170,7 +2192,8 @@ static RISCVCPUProfile RVA22S64 = {
> > > >            CPU_CFG_OFFSET(ext_svinval),
> > > >
> > > >            /* rva22s64 named features */
> > > > -        CPU_CFG_OFFSET(ext_svade),
> > > > +        CPU_CFG_OFFSET(ext_sstvecd), CPU_CFG_OFFSET(ext_sstvala),
> > > > +        CPU_CFG_OFFSET(ext_sscounterenw), CPU_CFG_OFFSET(ext_svade),
> > > >
> > > >            RISCV_PROFILE_EXT_LIST_END
> > > >        }
> > > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > > > index 698f926ab1..f79fc3dfd1 100644
> > > > --- a/target/riscv/cpu_cfg.h
> > > > +++ b/target/riscv/cpu_cfg.h
> > > > @@ -125,6 +125,15 @@ struct RISCVCPUConfig {
> > > >        /* Named features  */
> > > >        bool ext_svade;
> > > >        bool ext_zic64b;
> > > > +    bool ext_za64rs;
> > > > +    bool ext_ziccif;
> > > > +    bool ext_ziccrse;
> > > > +    bool ext_ziccamoa;
> > > > +    bool ext_zicclsm;
> > > > +    bool ext_ssccptr;
> > > > +    bool ext_sstvecd;
> > > > +    bool ext_sstvala;
> > > > +    bool ext_sscounterenw;
> > >
> > > Overall this and the previous patch look fine.
> > >
> > > One thing though, why store this information? I feel it could be
> > > confusing having these variables. If a developer sets them to false
> > > nothing actually happens, which is a little misleading
> >
> > These extensions aren't being exposed to users. riscv_cpu_named_features[] isn't
>
> Yep, we should not expose them to users.
>
> I meant developers, as in people reading the C code and compiling QEMU.
>
> > being used to create any CPU user properties. I should've mentioned that in
> > patch 2 ...
> >
> > As for the extra booleans that we'll be setting to 'true', as it is now
> > isa_edata_arr[] stores a string name, priv_ver and a cpu->cfg offset, so
> > everyone that adds a riscv,isa str must also have a valid bool offset in
> > RISCVCPUConfig. Having a bool also allow us to treat them as regular extensions
> > because we can re-use existing code to blindly enable them in profiles like
> > any other profile extension.
>
> Yep, and I think that makes sense. The odd part is that we don't
> actually use these bools. Which I feel is confusing when looking at
> the QEMU code base. Why have a variable that we don't use?

Let's say that we create an "always_enabled" boolean in cpu->cfg, always set to
'true' during riscv_cpu_init(). Then we change riscv_cpu_named_features[] to
work like this:

      MULTI_EXT_CFG_BOOL("svade", always_enabled, true),
      MULTI_EXT_CFG_BOOL("zic64b", always_enabled, true),
      MULTI_EXT_CFG_BOOL("za64rs", always_enabled, true),
      MULTI_EXT_CFG_BOOL("ziccif", always_enabled, true),
      (...)

(I would create another macro for these named features but that's beside the 
point)

If we go this route then we'll need changes in how profiles declare these 
internal
extensions, because now we're using CPU_CFG_OFFSET() with a specific boolean for
each. I would remove them from the profile description, otherwise the code would
attempt to do stuff with an 'always_enabled' bool, and instead just cite in a
comment that these other profile mandatory extensions are always enabled.


Another change on top of my head would be in isa_edata_arr[]. In this case we
would do:


> > > > +    ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, always_enabled),
> > > > +    ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, always_enabled),
> > > > +    ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, always_enabled),
> > > > +    ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, always_enabled),


There might be other changes needed but on top of my head I think this would 
work.
Is that what you're referring to?


Thanks,

Daniel

> > And, in case we need to promote them as regular user extensions, having the
> > booleans in place make it easier to do so. Patch 6 is doing that with 'svade'.
>
> If we plan to do that, I think that also makes sense. But I suspect no
> one has any plant to convert some of these to real configuration
> options.
>
> > Thanks,
> >
> > Daniel
> >
> >
> > We could create a single boolean that is always true in cpu->cfg and use it
> > for these entries. Another idea would be to change the riscv,isa functions to
> > handle these extensions separately, then we can add them in the array without
> > a valid cpu->cfg offfset.
>
> I think these are better ideas.
>
> Alistair
>
> > > Alistair
> > >
> > > >        /* Vendor-specific custom extensions */
> > > >        bool ext_xtheadba;
> > > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > > > index 90861cc065..6d5028cf84 100644
> > > > --- a/target/riscv/tcg/tcg-cpu.c
> > > > +++ b/target/riscv/tcg/tcg-cpu.c
> > > > @@ -206,7 +206,8 @@ static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, 
> > > > uint32_t feat_offset)
> > > >            cpu->cfg.ext_svadu = false;
> > > >            break;
> > > >        default:
> > > > -        g_assert_not_reached();
> > > > +        /* Named feature already enabled in riscv_tcg_cpu_instance_init */
> > > > +        return;
> > > >        }
> > > >    }
> > > >
> > > > @@ -1342,6 +1343,20 @@ static bool riscv_cpu_has_max_extensions(Object *cpu_obj)
> > > >        return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL;
> > > >    }
> > > >
> > > > +/* Named features that TCG always implements */
> > > > +static void riscv_tcg_cpu_enable_named_feats(RISCVCPU *cpu)
> > > > +{
> > > > +    cpu->cfg.ext_za64rs = true;
> > > > +    cpu->cfg.ext_ziccif = true;
> > > > +    cpu->cfg.ext_ziccrse = true;
> > > > +    cpu->cfg.ext_ziccamoa = true;
> > > > +    cpu->cfg.ext_zicclsm = true;
> > > > +    cpu->cfg.ext_ssccptr = true;
> > > > +    cpu->cfg.ext_sstvecd = true;
> > > > +    cpu->cfg.ext_sstvala = true;
> > > > +    cpu->cfg.ext_sscounterenw = true;
> > > > +}
> > > > +
> > > >    static void riscv_tcg_cpu_instance_init(CPUState *cs)
> > > >    {
> > > >        RISCVCPU *cpu = RISCV_CPU(cs);
> > > > @@ -1354,6 +1369,8 @@ static void riscv_tcg_cpu_instance_init(CPUState *cs)
> > > >        if (riscv_cpu_has_max_extensions(obj)) {
> > > >            riscv_init_max_cpu_extensions(obj);
> > > >        }
> > > > +
> > > > +    riscv_tcg_cpu_enable_named_feats(cpu);
> > > >    }
> > > >
> > > >    static void riscv_tcg_cpu_init_ops(AccelCPUClass *accel_cpu, CPUClass *cc)
> > > > --
> > > > 2.43.0