qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Daniel Henrique Barboza
Subject: Re: [PATCH v3 3/6] target/riscv: add remaining named features
Date: Thu, 15 Feb 2024 11:13:51 -0300
User-agent: Mozilla Thunderbird 


On 2/15/24 10:33, Conor Dooley wrote:

On Fri, Feb 02, 2024 at 12:21:51PM -0300, Daniel Henrique Barboza 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[].

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:


Why does an "rva22s64" cpu have "zicclsm" in it? Isn't zicclsm only
present in "u" profiles?


According to the specs I've read  it seems  the S profiles includes all 
extensions
from U profiles. For RVA22:

"The RVA22S64 mandatory unprivileged extensions include all the mandatory
unprivileged extensions in RVA22U64."

So rva22s64 will have zicclsm and all other extensions from its U profile too.





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


I want to raise my frustration with the crock we've been given here by
RVI. Any "named feature" that just creates a name for something that
already is assumed is completely useless, and DT property that is used
to communicate it's presence cannot be used - instead the property needs
to be inverted - indicating the absence of that named feature.


Let's say that I'm not the biggest fan of how these profile extensions are being
dealt with in the spec :) the text is vague w.r.t whether zicclsm and others
are actual extensions, or a 'named feature'( like we're calling here in QEMU)
that is just a glorified way of saying, for example, "zic64b" instead of "all
cache blocks have 64 bytes".


Thanks,

Daniel



Without the inversion, software that parses "riscv,isa" cannot make any
determination based on the absence of the property - it could be parsing
an old DT that does not have the property or it could be parsing the DT
of a system that does not support the extension.

This is part of why I deprecated `riscv,isa`. It's the same problem as
with "zifencei" et al - does a system with `riscv,isa = "rv64imac"`
support fence.i?

Cheers,
Conor.



Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
  target/riscv/cpu.c         | 42 +++++++++++++++++++++++++++++++-------
  target/riscv/cpu_cfg.h     |  6 ++++++
  target/riscv/tcg/tcg-cpu.c |  2 ++
  3 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 28d3cfa8ce..94843c4f6e 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_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),
@@ -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_always_enabled),
      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_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),
@@ -1512,6 +1521,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
@@ -1523,6 +1537,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(),
  };
@@ -2116,13 +2147,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/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 698f926ab1..c5049ec664 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -126,6 +126,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/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 90861cc065..673097c6e4 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -1347,6 +1347,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.43.0
reply via email to
[Prev in Thread] Current Thread [Next in Thread]