From: Alistair Francis <alistair23@gmail.com>
To: Andrew Jones <ajones@ventanamicro.com>
Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org,
alistair.francis@wdc.com, bmeng@tinylab.org,
liwei1518@gmail.com, zhiwei_liu@linux.alibaba.com,
palmer@rivosinc.com, dbarboza@ventanamicro.com
Subject: Re: [PATCH v2 3/6] target/riscv: add remaining named features
Date: Thu, 15 Feb 2024 14:15:11 +1000 [thread overview]
Message-ID: <CAKmqyKPUDGp9xXi5dvsF_bgrsZTEhcHRR41zCYwAqv4c9q6gMA@mail.gmail.com> (raw)
In-Reply-To: <20240126133101.61344-11-ajones@ventanamicro.com>
On Fri, Jan 26, 2024 at 11:33 PM Andrew Jones <ajones@ventanamicro.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[].
>
> 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>
> Reviewed-by: Andrew Jones <ajones@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 28d3cfa8ce59..1ecd8a57ed02 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 698f926ab1be..f79fc3dfd199 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;
Daniel and I have discussed this a bit before. This doesn't feel
right. We are adding variables that must always be true. To me this
seems confusing to anyone reading the code in the future. I understand
it isn't exposed to users, but it still feels odd.
I guess it's nice that we reuse the same logic and this is in some
ways cleaner than an "always_true", but it just seems clunky.
Especially as this list seems to be growing very quickly.
>
> /* Vendor-specific custom extensions */
> bool ext_xtheadba;
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 90861cc065e5..6d5028cf84d0 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;
Maybe we should assert that these are all true somewhere in the CPU
init/realise/reset. That at least indicates to someone using the code
that these can't actually be disabled.
@Daniel Henrique Barboza that might be a reasonable compromise as I
know I complained to you about the same thing. Unless you have
implemented something really clean and nice :)
Alistair
> +}
> +
> 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
>
>
next prev parent reply other threads:[~2024-02-15 4:15 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-26 13:31 [PATCH v2 0/6] riscv: named features riscv,isa, 'svade' rework Andrew Jones
2024-01-26 13:31 ` [PATCH v2 1/6] target/riscv/tcg: set 'mmu' with 'satp' in cpu_set_profile() Andrew Jones
2024-02-15 3:49 ` Alistair Francis
2024-01-26 13:31 ` [PATCH v2 2/6] target/riscv: add riscv,isa to named features Andrew Jones
2024-02-15 4:06 ` Alistair Francis
2024-01-26 13:31 ` [PATCH v2 3/6] target/riscv: add remaining " Andrew Jones
2024-02-15 4:15 ` Alistair Francis [this message]
2024-01-26 13:31 ` [PATCH v2 4/6] target/riscv: Reset henvcfg to zero Andrew Jones
2024-01-26 13:31 ` [PATCH v2 5/6] target/riscv: Gate hardware A/D PTE bit updating Andrew Jones
2024-01-26 13:31 ` [PATCH v2 6/6] target/riscv: Promote svade to a normal extension Andrew Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKmqyKPUDGp9xXi5dvsF_bgrsZTEhcHRR41zCYwAqv4c9q6gMA@mail.gmail.com \
--to=alistair23@gmail.com \
--cc=ajones@ventanamicro.com \
--cc=alistair.francis@wdc.com \
--cc=bmeng@tinylab.org \
--cc=dbarboza@ventanamicro.com \
--cc=liwei1518@gmail.com \
--cc=palmer@rivosinc.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=zhiwei_liu@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).