[PATCH v2 0/2] target/riscv: Support mxstatus CSR for thead-c906

[PATCH v2 0/2] target/riscv: Support mxstatus CSR for thead-c906

[LIU Zhiwei]

This patch set fix the regression on kernel pointed by Björn Töpel in
https://www.mail-archive.com/qemu-devel@nongnu.org/msg1018232.html.

We first add a framework for vendor CSRs in patch 1. After that we add
one thead-c906 CSR mxstatus, which is used for mmu extension xtheadmaee.

thead-c906 uses some flags in pte [60-63] bits. It has history reasons that
SVPBMT didn't exist when thead-c906 came to wotrld. We named this feature as
xtheadmaee[1]. this feature is controlled by an custom CSR named mxstatus,
whose maee field encodes whether enable the pte [60-63] bits.

[1]:https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadmaee.adoc

LIU Zhiwei (2):
  target/riscv: Register vendors CSR
  target/riscv: Support xtheadmaee for thead-c906

 target/riscv/cpu.c         |  9 ++++++
 target/riscv/cpu.h         |  9 ++++++
 target/riscv/cpu_bits.h    |  6 ++++
 target/riscv/cpu_cfg.h     |  4 ++-
 target/riscv/cpu_helper.c  | 25 ++++++++-------
 target/riscv/meson.build   |  1 +
 target/riscv/tcg/tcg-cpu.c | 25 ++++++++++++++-
 target/riscv/tcg/tcg-cpu.h |  1 +
 target/riscv/xthead_csr.c  | 65 ++++++++++++++++++++++++++++++++++++++
 9 files changed, 132 insertions(+), 13 deletions(-)
 create mode 100644 target/riscv/xthead_csr.c

-- 
2.25.1

[PATCH v2 1/2] target/riscv: Register vendors CSR

[LIU Zhiwei]

riscv specification allows custom CSRs in decode area. So we should
register all vendor CSRs in cpu realize stage.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
1) Use int index to quiet the  Werror for "i < 0".
---
 target/riscv/cpu.c         |  3 +++
 target/riscv/tcg/tcg-cpu.c | 18 ++++++++++++++++++
 target/riscv/tcg/tcg-cpu.h |  1 +
 3 files changed, 22 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 8cbfc7e781..2dcbc9ff32 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1113,6 +1113,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (tcg_enabled()) {
+        riscv_tcg_cpu_register_vendor_csr(cpu);
+    }
     riscv_cpu_register_gdb_regs_for_features(cs);
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 994ca1cdf9..559bf373f3 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -871,6 +871,24 @@ static void riscv_cpu_validate_profiles(RISCVCPU *cpu)
     }
 }
 
+void riscv_tcg_cpu_register_vendor_csr(RISCVCPU *cpu)
+{
+    static const struct {
+        bool (*guard_func)(const RISCVCPUConfig *);
+        riscv_csr_operations *csr_ops;
+    } vendors[] = {
+    };
+    for (int i = 0; i < ARRAY_SIZE(vendors); ++i) {
+        if (!vendors[i].guard_func(&cpu->cfg)) {
+            continue;
+        }
+        for (size_t j = 0; j < CSR_TABLE_SIZE &&
+                           vendors[i].csr_ops[j].name; j++) {
+            csr_ops[j] = vendors[i].csr_ops[j];
+        }
+    }
+}
+
 void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
 {
     CPURISCVState *env = &cpu->env;
diff --git a/target/riscv/tcg/tcg-cpu.h b/target/riscv/tcg/tcg-cpu.h
index f7b32417f8..3daaebf4fb 100644
--- a/target/riscv/tcg/tcg-cpu.h
+++ b/target/riscv/tcg/tcg-cpu.h
@@ -25,5 +25,6 @@
 void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
 void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp);
 bool riscv_cpu_tcg_compatible(RISCVCPU *cpu);
+void riscv_tcg_cpu_register_vendor_csr(RISCVCPU *cpu);
 
 #endif
-- 
2.25.1

[PATCH v2 2/2] target/riscv: Support xtheadmaee for thead-c906

[LIU Zhiwei]

This patch set fix the regression on kernel pointed by Björn Töpel in
https://www.mail-archive.com/qemu-devel@nongnu.org/msg1018232.html.

thead-c906 uses some flags in pte [60-63] bits. It has history reasons that
SVPBMT didn't exist when thead-c906 came to wotrld. We named this feature as
xtheadmaee[1]. this feature is controlled by an custom CSR named mxstatus,
whose maee field encodes whether enable the pte [60-63] bits.

[1]:https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadmaee.adoc

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
v1->v2:
1) Remove mxstatus user mode access
2) Add reference documentation to the commit log
---
 target/riscv/cpu.c         |  6 ++++
 target/riscv/cpu.h         |  9 ++++++
 target/riscv/cpu_bits.h    |  6 ++++
 target/riscv/cpu_cfg.h     |  4 ++-
 target/riscv/cpu_helper.c  | 25 ++++++++-------
 target/riscv/meson.build   |  1 +
 target/riscv/tcg/tcg-cpu.c |  7 +++-
 target/riscv/xthead_csr.c  | 65 ++++++++++++++++++++++++++++++++++++++
 8 files changed, 110 insertions(+), 13 deletions(-)
 create mode 100644 target/riscv/xthead_csr.c

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 2dcbc9ff32..bfdbb0539a 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -171,6 +171,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(xtheadmemidx, PRIV_VERSION_1_11_0, ext_xtheadmemidx),
     ISA_EXT_DATA_ENTRY(xtheadmempair, PRIV_VERSION_1_11_0, ext_xtheadmempair),
     ISA_EXT_DATA_ENTRY(xtheadsync, PRIV_VERSION_1_11_0, ext_xtheadsync),
+    ISA_EXT_DATA_ENTRY(xtheadmaee, PRIV_VERSION_1_11_0, ext_xtheadmaee),
     ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, ext_XVentanaCondOps),
 
     DEFINE_PROP_END_OF_LIST(),
@@ -506,6 +507,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
 
     cpu->cfg.mvendorid = THEAD_VENDOR_ID;
 #ifndef CONFIG_USER_ONLY
+    cpu->cfg.ext_xtheadmaee = true;
     set_satp_mode_max_supported(cpu, VM_1_10_SV39);
 #endif
 
@@ -949,6 +951,9 @@ static void riscv_cpu_reset_hold(Object *obj)
     }
 
     pmp_unlock_entries(env);
+    if (riscv_cpu_cfg(env)->ext_xtheadmaee) {
+        env->th_mxstatus |= TH_MXSTATUS_MAEE;
+    }
 #endif
     env->xl = riscv_cpu_mxl(env);
     riscv_cpu_update_mask(env);
@@ -1439,6 +1444,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
     MULTI_EXT_CFG_BOOL("xtheadmemidx", ext_xtheadmemidx, false),
     MULTI_EXT_CFG_BOOL("xtheadmempair", ext_xtheadmempair, false),
     MULTI_EXT_CFG_BOOL("xtheadsync", ext_xtheadsync, false),
+    MULTI_EXT_CFG_BOOL("xtheadmaee", ext_xtheadmaee, false),
     MULTI_EXT_CFG_BOOL("xventanacondops", ext_XVentanaCondOps, false),
 
     DEFINE_PROP_END_OF_LIST(),
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 5f3955c38d..1bacf40355 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -412,6 +412,14 @@ struct CPUArchState {
     target_ulong cur_pmmask;
     target_ulong cur_pmbase;
 
+    union {
+        /* Custom CSR for Xuantie CPU */
+        struct {
+#ifndef CONFIG_USER_ONLY
+            target_ulong th_mxstatus;
+#endif
+        };
+    };
     /* Fields from here on are preserved across CPU reset. */
     QEMUTimer *stimer; /* Internal timer for S-mode interrupt */
     QEMUTimer *vstimer; /* Internal timer for VS-mode interrupt */
@@ -799,6 +807,7 @@ void riscv_add_satp_mode_properties(Object *obj);
 bool riscv_cpu_accelerator_compatible(RISCVCPU *cpu);
 
 /* CSR function table */
+extern riscv_csr_operations th_csr_ops[CSR_TABLE_SIZE];
 extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
 
 extern const bool valid_vm_1_10_32[], valid_vm_1_10_64[];
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index e116f6c252..67ebb1cefe 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -897,4 +897,10 @@ typedef enum RISCVException {
 /* JVT CSR bits */
 #define JVT_MODE                           0x3F
 #define JVT_BASE                           (~0x3F)
+
+/* Xuantie custom CSRs */
+#define CSR_TH_MXSTATUS 0x7c0
+
+#define TH_MXSTATUS_MAEE_SHIFT  21
+#define TH_MXSTATUS_MAEE        (0x1 << TH_MXSTATUS_MAEE_SHIFT)
 #endif
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 780ae6ef17..3735c69fd6 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -136,6 +136,7 @@ struct RISCVCPUConfig {
     bool ext_xtheadmemidx;
     bool ext_xtheadmempair;
     bool ext_xtheadsync;
+    bool ext_xtheadmaee;
     bool ext_XVentanaCondOps;
 
     uint32_t pmu_mask;
@@ -176,7 +177,8 @@ static inline bool has_xthead_p(const RISCVCPUConfig *cfg)
            cfg->ext_xtheadcondmov ||
            cfg->ext_xtheadfmemidx || cfg->ext_xtheadfmv ||
            cfg->ext_xtheadmac || cfg->ext_xtheadmemidx ||
-           cfg->ext_xtheadmempair || cfg->ext_xtheadsync;
+           cfg->ext_xtheadmempair || cfg->ext_xtheadsync ||
+           cfg->ext_xtheadmaee;
 }
 
 #define MATERIALISE_EXT_PREDICATE(ext) \
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index c7cc7eb423..5c1f380276 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -812,6 +812,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
     int napot_bits = 0;
     target_ulong napot_mask;
 
+    bool skip_pte_check = env->th_mxstatus & TH_MXSTATUS_MAEE;
     /*
      * Check if we should use the background registers for the two
      * stage translation. We don't need to check if we actually need
@@ -974,18 +975,19 @@ restart:
         if (riscv_cpu_sxl(env) == MXL_RV32) {
             ppn = pte >> PTE_PPN_SHIFT;
         } else {
-            if (pte & PTE_RESERVED) {
-                return TRANSLATE_FAIL;
-            }
+            if (!skip_pte_check) {
+                if (pte & PTE_RESERVED) {
+                    return TRANSLATE_FAIL;
+                }
 
-            if (!pbmte && (pte & PTE_PBMT)) {
-                return TRANSLATE_FAIL;
-            }
+                if (!pbmte && (pte & PTE_PBMT)) {
+                    return TRANSLATE_FAIL;
+                }
 
-            if (!riscv_cpu_cfg(env)->ext_svnapot && (pte & PTE_N)) {
-                return TRANSLATE_FAIL;
+                if (!riscv_cpu_cfg(env)->ext_svnapot && (pte & PTE_N)) {
+                    return TRANSLATE_FAIL;
+                }
             }
-
             ppn = (pte & (target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT;
         }
 
@@ -998,7 +1000,8 @@ restart:
         }
 
         /* Inner PTE, continue walking */
-        if (pte & (PTE_D | PTE_A | PTE_U | PTE_ATTR)) {
+        if ((pte & (PTE_D | PTE_A | PTE_U)) ||
+            (!skip_pte_check && (pte & PTE_ATTR))) {
             return TRANSLATE_FAIL;
         }
         base = ppn << PGSHIFT;
@@ -1012,7 +1015,7 @@ restart:
         /* Misaligned PPN */
         return TRANSLATE_FAIL;
     }
-    if (!pbmte && (pte & PTE_PBMT)) {
+    if (!skip_pte_check && !pbmte && (pte & PTE_PBMT)) {
         /* Reserved without Svpbmt. */
         return TRANSLATE_FAIL;
     }
diff --git a/target/riscv/meson.build b/target/riscv/meson.build
index a5e0734e7f..d7f675881d 100644
--- a/target/riscv/meson.build
+++ b/target/riscv/meson.build
@@ -12,6 +12,7 @@ riscv_ss.add(files(
   'cpu.c',
   'cpu_helper.c',
   'csr.c',
+  'xthead_csr.c',
   'fpu_helper.c',
   'gdbstub.c',
   'op_helper.c',
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 559bf373f3..4b1184c8ab 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -763,7 +763,6 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.pmu_mask = 0;
         cpu->pmu_avail_ctrs = 0;
     }
-
     /*
      * Disable isa extensions based on priv spec after we
      * validated and set everything we need.
@@ -871,12 +870,18 @@ static void riscv_cpu_validate_profiles(RISCVCPU *cpu)
     }
 }
 
+static inline bool th_csr_p(const RISCVCPUConfig *cfg)
+{
+    return cfg->ext_xtheadmaee;
+}
+
 void riscv_tcg_cpu_register_vendor_csr(RISCVCPU *cpu)
 {
     static const struct {
         bool (*guard_func)(const RISCVCPUConfig *);
         riscv_csr_operations *csr_ops;
     } vendors[] = {
+        { th_csr_p, th_csr_ops },
     };
     for (int i = 0; i < ARRAY_SIZE(vendors); ++i) {
         if (!vendors[i].guard_func(&cpu->cfg)) {
diff --git a/target/riscv/xthead_csr.c b/target/riscv/xthead_csr.c
new file mode 100644
index 0000000000..9f88fa50db
--- /dev/null
+++ b/target/riscv/xthead_csr.c
@@ -0,0 +1,65 @@
+/*
+ * Xuantie implementation for RISC-V Control and Status Registers.
+ *
+ * Copyright (c) 2024 Alibaba Group. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "cpu.h"
+#include "tcg/tcg-cpu.h"
+#include "exec/exec-all.h"
+#include "exec/tb-flush.h"
+#include "qapi/error.h"
+
+#if !defined(CONFIG_USER_ONLY)
+static RISCVException th_maee_check(CPURISCVState *env, int csrno)
+{
+    if (riscv_cpu_cfg(env)->ext_xtheadmaee) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException
+read_th_mxstatus(CPURISCVState *env, int csrno, target_ulong *val)
+{
+    *val = env->th_mxstatus;
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException
+write_th_mxstatus(CPURISCVState *env, int csrno, target_ulong val)
+{
+    uint64_t mxstatus = env->th_mxstatus;
+    uint64_t mask = TH_MXSTATUS_MAEE;
+
+    if ((val ^ mxstatus) & TH_MXSTATUS_MAEE) {
+        tlb_flush(env_cpu(env));
+    }
+
+    mxstatus = (mxstatus & ~mask) | (val & mask);
+    env->th_mxstatus = mxstatus;
+    return RISCV_EXCP_NONE;
+}
+#endif
+
+riscv_csr_operations th_csr_ops[CSR_TABLE_SIZE] = {
+#if !defined(CONFIG_USER_ONLY)
+    [CSR_TH_MXSTATUS]     = { "th_mxstatus", th_maee_check, read_th_mxstatus,
+                                                            write_th_mxstatus},
+#endif /* !CONFIG_USER_ONLY */
+};
-- 
2.25.1

Re: [PATCH v2 2/2] target/riscv: Support xtheadmaee for thead-c906

[Alistair Francis]

On Sun, Feb 4, 2024 at 3:44 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> This patch set fix the regression on kernel pointed by Björn Töpel in
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg1018232.html.
>
> thead-c906 uses some flags in pte [60-63] bits. It has history reasons that
> SVPBMT didn't exist when thead-c906 came to wotrld. We named this feature as
> xtheadmaee[1]. this feature is controlled by an custom CSR named mxstatus,
> whose maee field encodes whether enable the pte [60-63] bits.
>
> [1]:https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadmaee.adoc
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
> v1->v2:
> 1) Remove mxstatus user mode access
> 2) Add reference documentation to the commit log
> ---
>  target/riscv/cpu.c         |  6 ++++
>  target/riscv/cpu.h         |  9 ++++++
>  target/riscv/cpu_bits.h    |  6 ++++
>  target/riscv/cpu_cfg.h     |  4 ++-
>  target/riscv/cpu_helper.c  | 25 ++++++++-------
>  target/riscv/meson.build   |  1 +
>  target/riscv/tcg/tcg-cpu.c |  7 +++-
>  target/riscv/xthead_csr.c  | 65 ++++++++++++++++++++++++++++++++++++++
>  8 files changed, 110 insertions(+), 13 deletions(-)
>  create mode 100644 target/riscv/xthead_csr.c
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 2dcbc9ff32..bfdbb0539a 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -171,6 +171,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(xtheadmemidx, PRIV_VERSION_1_11_0, ext_xtheadmemidx),
>      ISA_EXT_DATA_ENTRY(xtheadmempair, PRIV_VERSION_1_11_0, ext_xtheadmempair),
>      ISA_EXT_DATA_ENTRY(xtheadsync, PRIV_VERSION_1_11_0, ext_xtheadsync),
> +    ISA_EXT_DATA_ENTRY(xtheadmaee, PRIV_VERSION_1_11_0, ext_xtheadmaee),
>      ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, ext_XVentanaCondOps),
>
>      DEFINE_PROP_END_OF_LIST(),
> @@ -506,6 +507,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
>
>      cpu->cfg.mvendorid = THEAD_VENDOR_ID;
>  #ifndef CONFIG_USER_ONLY
> +    cpu->cfg.ext_xtheadmaee = true;
>      set_satp_mode_max_supported(cpu, VM_1_10_SV39);
>  #endif
>
> @@ -949,6 +951,9 @@ static void riscv_cpu_reset_hold(Object *obj)
>      }
>
>      pmp_unlock_entries(env);
> +    if (riscv_cpu_cfg(env)->ext_xtheadmaee) {
> +        env->th_mxstatus |= TH_MXSTATUS_MAEE;
> +    }
>  #endif
>      env->xl = riscv_cpu_mxl(env);
>      riscv_cpu_update_mask(env);
> @@ -1439,6 +1444,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
>      MULTI_EXT_CFG_BOOL("xtheadmemidx", ext_xtheadmemidx, false),
>      MULTI_EXT_CFG_BOOL("xtheadmempair", ext_xtheadmempair, false),
>      MULTI_EXT_CFG_BOOL("xtheadsync", ext_xtheadsync, false),
> +    MULTI_EXT_CFG_BOOL("xtheadmaee", ext_xtheadmaee, false),
>      MULTI_EXT_CFG_BOOL("xventanacondops", ext_XVentanaCondOps, false),
>
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 5f3955c38d..1bacf40355 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -412,6 +412,14 @@ struct CPUArchState {
>      target_ulong cur_pmmask;
>      target_ulong cur_pmbase;
>
> +    union {
> +        /* Custom CSR for Xuantie CPU */
> +        struct {
> +#ifndef CONFIG_USER_ONLY
> +            target_ulong th_mxstatus;
> +#endif
> +        };
> +    };
>      /* Fields from here on are preserved across CPU reset. */
>      QEMUTimer *stimer; /* Internal timer for S-mode interrupt */
>      QEMUTimer *vstimer; /* Internal timer for VS-mode interrupt */
> @@ -799,6 +807,7 @@ void riscv_add_satp_mode_properties(Object *obj);
>  bool riscv_cpu_accelerator_compatible(RISCVCPU *cpu);
>
>  /* CSR function table */
> +extern riscv_csr_operations th_csr_ops[CSR_TABLE_SIZE];
>  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
>
>  extern const bool valid_vm_1_10_32[], valid_vm_1_10_64[];
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index e116f6c252..67ebb1cefe 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -897,4 +897,10 @@ typedef enum RISCVException {
>  /* JVT CSR bits */
>  #define JVT_MODE                           0x3F
>  #define JVT_BASE                           (~0x3F)
> +
> +/* Xuantie custom CSRs */
> +#define CSR_TH_MXSTATUS 0x7c0
> +
> +#define TH_MXSTATUS_MAEE_SHIFT  21
> +#define TH_MXSTATUS_MAEE        (0x1 << TH_MXSTATUS_MAEE_SHIFT)
>  #endif
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 780ae6ef17..3735c69fd6 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -136,6 +136,7 @@ struct RISCVCPUConfig {
>      bool ext_xtheadmemidx;
>      bool ext_xtheadmempair;
>      bool ext_xtheadsync;
> +    bool ext_xtheadmaee;
>      bool ext_XVentanaCondOps;
>
>      uint32_t pmu_mask;
> @@ -176,7 +177,8 @@ static inline bool has_xthead_p(const RISCVCPUConfig *cfg)
>             cfg->ext_xtheadcondmov ||
>             cfg->ext_xtheadfmemidx || cfg->ext_xtheadfmv ||
>             cfg->ext_xtheadmac || cfg->ext_xtheadmemidx ||
> -           cfg->ext_xtheadmempair || cfg->ext_xtheadsync;
> +           cfg->ext_xtheadmempair || cfg->ext_xtheadsync ||
> +           cfg->ext_xtheadmaee;
>  }
>
>  #define MATERIALISE_EXT_PREDICATE(ext) \
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index c7cc7eb423..5c1f380276 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -812,6 +812,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>      int napot_bits = 0;
>      target_ulong napot_mask;
>
> +    bool skip_pte_check = env->th_mxstatus & TH_MXSTATUS_MAEE;
>      /*
>       * Check if we should use the background registers for the two
>       * stage translation. We don't need to check if we actually need
> @@ -974,18 +975,19 @@ restart:
>          if (riscv_cpu_sxl(env) == MXL_RV32) {
>              ppn = pte >> PTE_PPN_SHIFT;
>          } else {
> -            if (pte & PTE_RESERVED) {
> -                return TRANSLATE_FAIL;
> -            }
> +            if (!skip_pte_check) {
> +                if (pte & PTE_RESERVED) {
> +                    return TRANSLATE_FAIL;
> +                }
>
> -            if (!pbmte && (pte & PTE_PBMT)) {
> -                return TRANSLATE_FAIL;
> -            }
> +                if (!pbmte && (pte & PTE_PBMT)) {
> +                    return TRANSLATE_FAIL;
> +                }
>
> -            if (!riscv_cpu_cfg(env)->ext_svnapot && (pte & PTE_N)) {
> -                return TRANSLATE_FAIL;
> +                if (!riscv_cpu_cfg(env)->ext_svnapot && (pte & PTE_N)) {
> +                    return TRANSLATE_FAIL;
> +                }
>              }
> -
>              ppn = (pte & (target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT;

Unfortunately we won't be able to take this upstream. This is core
QEMU RISC-V code that is now being changed against the spec. I think
adding the CSR is fine, but we can't take this core change.

A fix that works for everyone should be supporting the th_mxstatus
CSR, but don't support setting the TH_MXSTATUS_MAEE bit. That way
guests can detect that the bit isn't set and not use the reserved bits
in the PTE. From my understanding the extra PTE bits are related to
cache control in the hardware, which we don't need here

Alistair

>          }
>
> @@ -998,7 +1000,8 @@ restart:
>          }
>
>          /* Inner PTE, continue walking */
> -        if (pte & (PTE_D | PTE_A | PTE_U | PTE_ATTR)) {
> +        if ((pte & (PTE_D | PTE_A | PTE_U)) ||
> +            (!skip_pte_check && (pte & PTE_ATTR))) {
>              return TRANSLATE_FAIL;
>          }
>          base = ppn << PGSHIFT;
> @@ -1012,7 +1015,7 @@ restart:
>          /* Misaligned PPN */
>          return TRANSLATE_FAIL;
>      }
> -    if (!pbmte && (pte & PTE_PBMT)) {
> +    if (!skip_pte_check && !pbmte && (pte & PTE_PBMT)) {
>          /* Reserved without Svpbmt. */
>          return TRANSLATE_FAIL;
>      }
> diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> index a5e0734e7f..d7f675881d 100644
> --- a/target/riscv/meson.build
> +++ b/target/riscv/meson.build
> @@ -12,6 +12,7 @@ riscv_ss.add(files(
>    'cpu.c',
>    'cpu_helper.c',
>    'csr.c',
> +  'xthead_csr.c',
>    'fpu_helper.c',
>    'gdbstub.c',
>    'op_helper.c',
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 559bf373f3..4b1184c8ab 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -763,7 +763,6 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>          cpu->cfg.pmu_mask = 0;
>          cpu->pmu_avail_ctrs = 0;
>      }
> -
>      /*
>       * Disable isa extensions based on priv spec after we
>       * validated and set everything we need.
> @@ -871,12 +870,18 @@ static void riscv_cpu_validate_profiles(RISCVCPU *cpu)
>      }
>  }
>
> +static inline bool th_csr_p(const RISCVCPUConfig *cfg)
> +{
> +    return cfg->ext_xtheadmaee;
> +}
> +
>  void riscv_tcg_cpu_register_vendor_csr(RISCVCPU *cpu)
>  {
>      static const struct {
>          bool (*guard_func)(const RISCVCPUConfig *);
>          riscv_csr_operations *csr_ops;
>      } vendors[] = {
> +        { th_csr_p, th_csr_ops },
>      };
>      for (int i = 0; i < ARRAY_SIZE(vendors); ++i) {
>          if (!vendors[i].guard_func(&cpu->cfg)) {
> diff --git a/target/riscv/xthead_csr.c b/target/riscv/xthead_csr.c
> new file mode 100644
> index 0000000000..9f88fa50db
> --- /dev/null
> +++ b/target/riscv/xthead_csr.c
> @@ -0,0 +1,65 @@
> +/*
> + * Xuantie implementation for RISC-V Control and Status Registers.
> + *
> + * Copyright (c) 2024 Alibaba Group. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "cpu.h"
> +#include "tcg/tcg-cpu.h"
> +#include "exec/exec-all.h"
> +#include "exec/tb-flush.h"
> +#include "qapi/error.h"
> +
> +#if !defined(CONFIG_USER_ONLY)
> +static RISCVException th_maee_check(CPURISCVState *env, int csrno)
> +{
> +    if (riscv_cpu_cfg(env)->ext_xtheadmaee) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException
> +read_th_mxstatus(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> +    *val = env->th_mxstatus;
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException
> +write_th_mxstatus(CPURISCVState *env, int csrno, target_ulong val)
> +{
> +    uint64_t mxstatus = env->th_mxstatus;
> +    uint64_t mask = TH_MXSTATUS_MAEE;
> +
> +    if ((val ^ mxstatus) & TH_MXSTATUS_MAEE) {
> +        tlb_flush(env_cpu(env));
> +    }
> +
> +    mxstatus = (mxstatus & ~mask) | (val & mask);
> +    env->th_mxstatus = mxstatus;
> +    return RISCV_EXCP_NONE;
> +}
> +#endif
> +
> +riscv_csr_operations th_csr_ops[CSR_TABLE_SIZE] = {
> +#if !defined(CONFIG_USER_ONLY)
> +    [CSR_TH_MXSTATUS]     = { "th_mxstatus", th_maee_check, read_th_mxstatus,
> +                                                            write_th_mxstatus},
> +#endif /* !CONFIG_USER_ONLY */
> +};
> --
> 2.25.1
>
>

Re: [PATCH v2 2/2] target/riscv: Support xtheadmaee for thead-c906

[Christoph Müllner]

On Mon, Feb 5, 2024 at 3:42 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Sun, Feb 4, 2024 at 3:44 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
> >
> > This patch set fix the regression on kernel pointed by Björn Töpel in
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg1018232.html.
> >
> > thead-c906 uses some flags in pte [60-63] bits. It has history reasons that
> > SVPBMT didn't exist when thead-c906 came to wotrld. We named this feature as
> > xtheadmaee[1]. this feature is controlled by an custom CSR named mxstatus,
> > whose maee field encodes whether enable the pte [60-63] bits.
> >
> > [1]:https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadmaee.adoc
> >
> > Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> > ---
> > v1->v2:
> > 1) Remove mxstatus user mode access
> > 2) Add reference documentation to the commit log
> > ---
> >  target/riscv/cpu.c         |  6 ++++
> >  target/riscv/cpu.h         |  9 ++++++
> >  target/riscv/cpu_bits.h    |  6 ++++
> >  target/riscv/cpu_cfg.h     |  4 ++-
> >  target/riscv/cpu_helper.c  | 25 ++++++++-------
> >  target/riscv/meson.build   |  1 +
> >  target/riscv/tcg/tcg-cpu.c |  7 +++-
> >  target/riscv/xthead_csr.c  | 65 ++++++++++++++++++++++++++++++++++++++
> >  8 files changed, 110 insertions(+), 13 deletions(-)
> >  create mode 100644 target/riscv/xthead_csr.c
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 2dcbc9ff32..bfdbb0539a 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -171,6 +171,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
> >      ISA_EXT_DATA_ENTRY(xtheadmemidx, PRIV_VERSION_1_11_0, ext_xtheadmemidx),
> >      ISA_EXT_DATA_ENTRY(xtheadmempair, PRIV_VERSION_1_11_0, ext_xtheadmempair),
> >      ISA_EXT_DATA_ENTRY(xtheadsync, PRIV_VERSION_1_11_0, ext_xtheadsync),
> > +    ISA_EXT_DATA_ENTRY(xtheadmaee, PRIV_VERSION_1_11_0, ext_xtheadmaee),
> >      ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, ext_XVentanaCondOps),
> >
> >      DEFINE_PROP_END_OF_LIST(),
> > @@ -506,6 +507,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
> >
> >      cpu->cfg.mvendorid = THEAD_VENDOR_ID;
> >  #ifndef CONFIG_USER_ONLY
> > +    cpu->cfg.ext_xtheadmaee = true;
> >      set_satp_mode_max_supported(cpu, VM_1_10_SV39);
> >  #endif
> >
> > @@ -949,6 +951,9 @@ static void riscv_cpu_reset_hold(Object *obj)
> >      }
> >
> >      pmp_unlock_entries(env);
> > +    if (riscv_cpu_cfg(env)->ext_xtheadmaee) {
> > +        env->th_mxstatus |= TH_MXSTATUS_MAEE;
> > +    }
> >  #endif
> >      env->xl = riscv_cpu_mxl(env);
> >      riscv_cpu_update_mask(env);
> > @@ -1439,6 +1444,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
> >      MULTI_EXT_CFG_BOOL("xtheadmemidx", ext_xtheadmemidx, false),
> >      MULTI_EXT_CFG_BOOL("xtheadmempair", ext_xtheadmempair, false),
> >      MULTI_EXT_CFG_BOOL("xtheadsync", ext_xtheadsync, false),
> > +    MULTI_EXT_CFG_BOOL("xtheadmaee", ext_xtheadmaee, false),
> >      MULTI_EXT_CFG_BOOL("xventanacondops", ext_XVentanaCondOps, false),
> >
> >      DEFINE_PROP_END_OF_LIST(),
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 5f3955c38d..1bacf40355 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -412,6 +412,14 @@ struct CPUArchState {
> >      target_ulong cur_pmmask;
> >      target_ulong cur_pmbase;
> >
> > +    union {
> > +        /* Custom CSR for Xuantie CPU */
> > +        struct {
> > +#ifndef CONFIG_USER_ONLY
> > +            target_ulong th_mxstatus;
> > +#endif
> > +        };
> > +    };
> >      /* Fields from here on are preserved across CPU reset. */
> >      QEMUTimer *stimer; /* Internal timer for S-mode interrupt */
> >      QEMUTimer *vstimer; /* Internal timer for VS-mode interrupt */
> > @@ -799,6 +807,7 @@ void riscv_add_satp_mode_properties(Object *obj);
> >  bool riscv_cpu_accelerator_compatible(RISCVCPU *cpu);
> >
> >  /* CSR function table */
> > +extern riscv_csr_operations th_csr_ops[CSR_TABLE_SIZE];
> >  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> >
> >  extern const bool valid_vm_1_10_32[], valid_vm_1_10_64[];
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index e116f6c252..67ebb1cefe 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -897,4 +897,10 @@ typedef enum RISCVException {
> >  /* JVT CSR bits */
> >  #define JVT_MODE                           0x3F
> >  #define JVT_BASE                           (~0x3F)
> > +
> > +/* Xuantie custom CSRs */
> > +#define CSR_TH_MXSTATUS 0x7c0
> > +
> > +#define TH_MXSTATUS_MAEE_SHIFT  21
> > +#define TH_MXSTATUS_MAEE        (0x1 << TH_MXSTATUS_MAEE_SHIFT)
> >  #endif
> > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > index 780ae6ef17..3735c69fd6 100644
> > --- a/target/riscv/cpu_cfg.h
> > +++ b/target/riscv/cpu_cfg.h
> > @@ -136,6 +136,7 @@ struct RISCVCPUConfig {
> >      bool ext_xtheadmemidx;
> >      bool ext_xtheadmempair;
> >      bool ext_xtheadsync;
> > +    bool ext_xtheadmaee;
> >      bool ext_XVentanaCondOps;
> >
> >      uint32_t pmu_mask;
> > @@ -176,7 +177,8 @@ static inline bool has_xthead_p(const RISCVCPUConfig *cfg)
> >             cfg->ext_xtheadcondmov ||
> >             cfg->ext_xtheadfmemidx || cfg->ext_xtheadfmv ||
> >             cfg->ext_xtheadmac || cfg->ext_xtheadmemidx ||
> > -           cfg->ext_xtheadmempair || cfg->ext_xtheadsync;
> > +           cfg->ext_xtheadmempair || cfg->ext_xtheadsync ||
> > +           cfg->ext_xtheadmaee;
> >  }
> >
> >  #define MATERIALISE_EXT_PREDICATE(ext) \
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index c7cc7eb423..5c1f380276 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -812,6 +812,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
> >      int napot_bits = 0;
> >      target_ulong napot_mask;
> >
> > +    bool skip_pte_check = env->th_mxstatus & TH_MXSTATUS_MAEE;
> >      /*
> >       * Check if we should use the background registers for the two
> >       * stage translation. We don't need to check if we actually need
> > @@ -974,18 +975,19 @@ restart:
> >          if (riscv_cpu_sxl(env) == MXL_RV32) {
> >              ppn = pte >> PTE_PPN_SHIFT;
> >          } else {
> > -            if (pte & PTE_RESERVED) {
> > -                return TRANSLATE_FAIL;
> > -            }
> > +            if (!skip_pte_check) {
> > +                if (pte & PTE_RESERVED) {
> > +                    return TRANSLATE_FAIL;
> > +                }
> >
> > -            if (!pbmte && (pte & PTE_PBMT)) {
> > -                return TRANSLATE_FAIL;
> > -            }
> > +                if (!pbmte && (pte & PTE_PBMT)) {
> > +                    return TRANSLATE_FAIL;
> > +                }
> >
> > -            if (!riscv_cpu_cfg(env)->ext_svnapot && (pte & PTE_N)) {
> > -                return TRANSLATE_FAIL;
> > +                if (!riscv_cpu_cfg(env)->ext_svnapot && (pte & PTE_N)) {
> > +                    return TRANSLATE_FAIL;
> > +                }
> >              }
> > -
> >              ppn = (pte & (target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT;
>
> Unfortunately we won't be able to take this upstream. This is core
> QEMU RISC-V code that is now being changed against the spec. I think
> adding the CSR is fine, but we can't take this core change.
>
> A fix that works for everyone should be supporting the th_mxstatus
> CSR, but don't support setting the TH_MXSTATUS_MAEE bit. That way
> guests can detect that the bit isn't set and not use the reserved bits
> in the PTE. From my understanding the extra PTE bits are related to
> cache control in the hardware, which we don't need here

Sounds good! Let me recap the overall plan:
* QEMU does not emulate MAEE, but signals that MAEE is not available
by setting TH_MXSTATUS_MAEE to 0.
* Consequence: The c906 emulation does not enable any page-base memory
attribute mechanism.
* OpenSBI tests the TH_MXSTATUS_MAEE bit (M-mode only) and provides
that information to user-space (e.g. DTB).
* The current Linux errata code will be enhanced to not assume MAEE
for each core with T-Head vendor ID, but also query the MAEE bit and
ensure it is set.
* Booting on a QEMU c906 will not enable MAEE, but will still boot.

We never had a complete MAEE implementation upstream, because that's not needed.
But until recently we could mess with reserved bits how we want.
Now that QEMU is more restrictive about reserved bits in the PTEs, we
need to address this in the kernel.

The downside is, that the kernel now sees a c906 CPU without MAEE and
such a CPU does not exist.
But that's fine, because this does not require extra code to handle this case.
Also, the additional check for the MAEE bit in the kernel errata
probing code is likely needed anyway for future T-Head CPUs.

BTW, what was the reason for QEMU to prevent setting reserved bits in PTEs?

Thanks,
Christoph

>
> Alistair
>
> >          }
> >
> > @@ -998,7 +1000,8 @@ restart:
> >          }
> >
> >          /* Inner PTE, continue walking */
> > -        if (pte & (PTE_D | PTE_A | PTE_U | PTE_ATTR)) {
> > +        if ((pte & (PTE_D | PTE_A | PTE_U)) ||
> > +            (!skip_pte_check && (pte & PTE_ATTR))) {
> >              return TRANSLATE_FAIL;
> >          }
> >          base = ppn << PGSHIFT;
> > @@ -1012,7 +1015,7 @@ restart:
> >          /* Misaligned PPN */
> >          return TRANSLATE_FAIL;
> >      }
> > -    if (!pbmte && (pte & PTE_PBMT)) {
> > +    if (!skip_pte_check && !pbmte && (pte & PTE_PBMT)) {
> >          /* Reserved without Svpbmt. */
> >          return TRANSLATE_FAIL;
> >      }
> > diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> > index a5e0734e7f..d7f675881d 100644
> > --- a/target/riscv/meson.build
> > +++ b/target/riscv/meson.build
> > @@ -12,6 +12,7 @@ riscv_ss.add(files(
> >    'cpu.c',
> >    'cpu_helper.c',
> >    'csr.c',
> > +  'xthead_csr.c',
> >    'fpu_helper.c',
> >    'gdbstub.c',
> >    'op_helper.c',
> > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > index 559bf373f3..4b1184c8ab 100644
> > --- a/target/riscv/tcg/tcg-cpu.c
> > +++ b/target/riscv/tcg/tcg-cpu.c
> > @@ -763,7 +763,6 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> >          cpu->cfg.pmu_mask = 0;
> >          cpu->pmu_avail_ctrs = 0;
> >      }
> > -
> >      /*
> >       * Disable isa extensions based on priv spec after we
> >       * validated and set everything we need.
> > @@ -871,12 +870,18 @@ static void riscv_cpu_validate_profiles(RISCVCPU *cpu)
> >      }
> >  }
> >
> > +static inline bool th_csr_p(const RISCVCPUConfig *cfg)
> > +{
> > +    return cfg->ext_xtheadmaee;
> > +}
> > +
> >  void riscv_tcg_cpu_register_vendor_csr(RISCVCPU *cpu)
> >  {
> >      static const struct {
> >          bool (*guard_func)(const RISCVCPUConfig *);
> >          riscv_csr_operations *csr_ops;
> >      } vendors[] = {
> > +        { th_csr_p, th_csr_ops },
> >      };
> >      for (int i = 0; i < ARRAY_SIZE(vendors); ++i) {
> >          if (!vendors[i].guard_func(&cpu->cfg)) {
> > diff --git a/target/riscv/xthead_csr.c b/target/riscv/xthead_csr.c
> > new file mode 100644
> > index 0000000000..9f88fa50db
> > --- /dev/null
> > +++ b/target/riscv/xthead_csr.c
> > @@ -0,0 +1,65 @@
> > +/*
> > + * Xuantie implementation for RISC-V Control and Status Registers.
> > + *
> > + * Copyright (c) 2024 Alibaba Group. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2 or later, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along with
> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/log.h"
> > +#include "cpu.h"
> > +#include "tcg/tcg-cpu.h"
> > +#include "exec/exec-all.h"
> > +#include "exec/tb-flush.h"
> > +#include "qapi/error.h"
> > +
> > +#if !defined(CONFIG_USER_ONLY)
> > +static RISCVException th_maee_check(CPURISCVState *env, int csrno)
> > +{
> > +    if (riscv_cpu_cfg(env)->ext_xtheadmaee) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException
> > +read_th_mxstatus(CPURISCVState *env, int csrno, target_ulong *val)
> > +{
> > +    *val = env->th_mxstatus;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException
> > +write_th_mxstatus(CPURISCVState *env, int csrno, target_ulong val)
> > +{
> > +    uint64_t mxstatus = env->th_mxstatus;
> > +    uint64_t mask = TH_MXSTATUS_MAEE;
> > +
> > +    if ((val ^ mxstatus) & TH_MXSTATUS_MAEE) {
> > +        tlb_flush(env_cpu(env));
> > +    }
> > +
> > +    mxstatus = (mxstatus & ~mask) | (val & mask);
> > +    env->th_mxstatus = mxstatus;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +#endif
> > +
> > +riscv_csr_operations th_csr_ops[CSR_TABLE_SIZE] = {
> > +#if !defined(CONFIG_USER_ONLY)
> > +    [CSR_TH_MXSTATUS]     = { "th_mxstatus", th_maee_check, read_th_mxstatus,
> > +                                                            write_th_mxstatus},
> > +#endif /* !CONFIG_USER_ONLY */
> > +};
> > --
> > 2.25.1
> >
> >

Re: [PATCH v2 2/2] target/riscv: Support xtheadmaee for thead-c906

[Alistair Francis]

On Mon, Feb 5, 2024 at 6:37 PM Christoph Müllner
<christoph.muellner@vrull.eu> wrote:
>
> On Mon, Feb 5, 2024 at 3:42 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Sun, Feb 4, 2024 at 3:44 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
> > >
> > > This patch set fix the regression on kernel pointed by Björn Töpel in
> > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg1018232.html.
> > >
> > > thead-c906 uses some flags in pte [60-63] bits. It has history reasons that
> > > SVPBMT didn't exist when thead-c906 came to wotrld. We named this feature as
> > > xtheadmaee[1]. this feature is controlled by an custom CSR named mxstatus,
> > > whose maee field encodes whether enable the pte [60-63] bits.
> > >
> > > [1]:https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadmaee.adoc
> > >
> > > Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> > > ---
> > > v1->v2:
> > > 1) Remove mxstatus user mode access
> > > 2) Add reference documentation to the commit log
> > > ---
> > >  target/riscv/cpu.c         |  6 ++++
> > >  target/riscv/cpu.h         |  9 ++++++
> > >  target/riscv/cpu_bits.h    |  6 ++++
> > >  target/riscv/cpu_cfg.h     |  4 ++-
> > >  target/riscv/cpu_helper.c  | 25 ++++++++-------
> > >  target/riscv/meson.build   |  1 +
> > >  target/riscv/tcg/tcg-cpu.c |  7 +++-
> > >  target/riscv/xthead_csr.c  | 65 ++++++++++++++++++++++++++++++++++++++
> > >  8 files changed, 110 insertions(+), 13 deletions(-)
> > >  create mode 100644 target/riscv/xthead_csr.c
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 2dcbc9ff32..bfdbb0539a 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -171,6 +171,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
> > >      ISA_EXT_DATA_ENTRY(xtheadmemidx, PRIV_VERSION_1_11_0, ext_xtheadmemidx),
> > >      ISA_EXT_DATA_ENTRY(xtheadmempair, PRIV_VERSION_1_11_0, ext_xtheadmempair),
> > >      ISA_EXT_DATA_ENTRY(xtheadsync, PRIV_VERSION_1_11_0, ext_xtheadsync),
> > > +    ISA_EXT_DATA_ENTRY(xtheadmaee, PRIV_VERSION_1_11_0, ext_xtheadmaee),
> > >      ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, ext_XVentanaCondOps),
> > >
> > >      DEFINE_PROP_END_OF_LIST(),
> > > @@ -506,6 +507,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
> > >
> > >      cpu->cfg.mvendorid = THEAD_VENDOR_ID;
> > >  #ifndef CONFIG_USER_ONLY
> > > +    cpu->cfg.ext_xtheadmaee = true;
> > >      set_satp_mode_max_supported(cpu, VM_1_10_SV39);
> > >  #endif
> > >
> > > @@ -949,6 +951,9 @@ static void riscv_cpu_reset_hold(Object *obj)
> > >      }
> > >
> > >      pmp_unlock_entries(env);
> > > +    if (riscv_cpu_cfg(env)->ext_xtheadmaee) {
> > > +        env->th_mxstatus |= TH_MXSTATUS_MAEE;
> > > +    }
> > >  #endif
> > >      env->xl = riscv_cpu_mxl(env);
> > >      riscv_cpu_update_mask(env);
> > > @@ -1439,6 +1444,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
> > >      MULTI_EXT_CFG_BOOL("xtheadmemidx", ext_xtheadmemidx, false),
> > >      MULTI_EXT_CFG_BOOL("xtheadmempair", ext_xtheadmempair, false),
> > >      MULTI_EXT_CFG_BOOL("xtheadsync", ext_xtheadsync, false),
> > > +    MULTI_EXT_CFG_BOOL("xtheadmaee", ext_xtheadmaee, false),
> > >      MULTI_EXT_CFG_BOOL("xventanacondops", ext_XVentanaCondOps, false),
> > >
> > >      DEFINE_PROP_END_OF_LIST(),
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 5f3955c38d..1bacf40355 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -412,6 +412,14 @@ struct CPUArchState {
> > >      target_ulong cur_pmmask;
> > >      target_ulong cur_pmbase;
> > >
> > > +    union {
> > > +        /* Custom CSR for Xuantie CPU */
> > > +        struct {
> > > +#ifndef CONFIG_USER_ONLY
> > > +            target_ulong th_mxstatus;
> > > +#endif
> > > +        };
> > > +    };
> > >      /* Fields from here on are preserved across CPU reset. */
> > >      QEMUTimer *stimer; /* Internal timer for S-mode interrupt */
> > >      QEMUTimer *vstimer; /* Internal timer for VS-mode interrupt */
> > > @@ -799,6 +807,7 @@ void riscv_add_satp_mode_properties(Object *obj);
> > >  bool riscv_cpu_accelerator_compatible(RISCVCPU *cpu);
> > >
> > >  /* CSR function table */
> > > +extern riscv_csr_operations th_csr_ops[CSR_TABLE_SIZE];
> > >  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> > >
> > >  extern const bool valid_vm_1_10_32[], valid_vm_1_10_64[];
> > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > > index e116f6c252..67ebb1cefe 100644
> > > --- a/target/riscv/cpu_bits.h
> > > +++ b/target/riscv/cpu_bits.h
> > > @@ -897,4 +897,10 @@ typedef enum RISCVException {
> > >  /* JVT CSR bits */
> > >  #define JVT_MODE                           0x3F
> > >  #define JVT_BASE                           (~0x3F)
> > > +
> > > +/* Xuantie custom CSRs */
> > > +#define CSR_TH_MXSTATUS 0x7c0
> > > +
> > > +#define TH_MXSTATUS_MAEE_SHIFT  21
> > > +#define TH_MXSTATUS_MAEE        (0x1 << TH_MXSTATUS_MAEE_SHIFT)
> > >  #endif
> > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > > index 780ae6ef17..3735c69fd6 100644
> > > --- a/target/riscv/cpu_cfg.h
> > > +++ b/target/riscv/cpu_cfg.h
> > > @@ -136,6 +136,7 @@ struct RISCVCPUConfig {
> > >      bool ext_xtheadmemidx;
> > >      bool ext_xtheadmempair;
> > >      bool ext_xtheadsync;
> > > +    bool ext_xtheadmaee;
> > >      bool ext_XVentanaCondOps;
> > >
> > >      uint32_t pmu_mask;
> > > @@ -176,7 +177,8 @@ static inline bool has_xthead_p(const RISCVCPUConfig *cfg)
> > >             cfg->ext_xtheadcondmov ||
> > >             cfg->ext_xtheadfmemidx || cfg->ext_xtheadfmv ||
> > >             cfg->ext_xtheadmac || cfg->ext_xtheadmemidx ||
> > > -           cfg->ext_xtheadmempair || cfg->ext_xtheadsync;
> > > +           cfg->ext_xtheadmempair || cfg->ext_xtheadsync ||
> > > +           cfg->ext_xtheadmaee;
> > >  }
> > >
> > >  #define MATERIALISE_EXT_PREDICATE(ext) \
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index c7cc7eb423..5c1f380276 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -812,6 +812,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
> > >      int napot_bits = 0;
> > >      target_ulong napot_mask;
> > >
> > > +    bool skip_pte_check = env->th_mxstatus & TH_MXSTATUS_MAEE;
> > >      /*
> > >       * Check if we should use the background registers for the two
> > >       * stage translation. We don't need to check if we actually need
> > > @@ -974,18 +975,19 @@ restart:
> > >          if (riscv_cpu_sxl(env) == MXL_RV32) {
> > >              ppn = pte >> PTE_PPN_SHIFT;
> > >          } else {
> > > -            if (pte & PTE_RESERVED) {
> > > -                return TRANSLATE_FAIL;
> > > -            }
> > > +            if (!skip_pte_check) {
> > > +                if (pte & PTE_RESERVED) {
> > > +                    return TRANSLATE_FAIL;
> > > +                }
> > >
> > > -            if (!pbmte && (pte & PTE_PBMT)) {
> > > -                return TRANSLATE_FAIL;
> > > -            }
> > > +                if (!pbmte && (pte & PTE_PBMT)) {
> > > +                    return TRANSLATE_FAIL;
> > > +                }
> > >
> > > -            if (!riscv_cpu_cfg(env)->ext_svnapot && (pte & PTE_N)) {
> > > -                return TRANSLATE_FAIL;
> > > +                if (!riscv_cpu_cfg(env)->ext_svnapot && (pte & PTE_N)) {
> > > +                    return TRANSLATE_FAIL;
> > > +                }
> > >              }
> > > -
> > >              ppn = (pte & (target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> >
> > Unfortunately we won't be able to take this upstream. This is core
> > QEMU RISC-V code that is now being changed against the spec. I think
> > adding the CSR is fine, but we can't take this core change.
> >
> > A fix that works for everyone should be supporting the th_mxstatus
> > CSR, but don't support setting the TH_MXSTATUS_MAEE bit. That way
> > guests can detect that the bit isn't set and not use the reserved bits
> > in the PTE. From my understanding the extra PTE bits are related to
> > cache control in the hardware, which we don't need here
>
> Sounds good! Let me recap the overall plan:
> * QEMU does not emulate MAEE, but signals that MAEE is not available
> by setting TH_MXSTATUS_MAEE to 0.

Yep!

> * Consequence: The c906 emulation does not enable any page-base memory
> attribute mechanism.

Exactly

> * OpenSBI tests the TH_MXSTATUS_MAEE bit (M-mode only) and provides
> that information to user-space (e.g. DTB).

To the kernel, but yep!

> * The current Linux errata code will be enhanced to not assume MAEE
> for each core with T-Head vendor ID, but also query the MAEE bit and
> ensure it is set.

I feel like it should already do that :)

> * Booting on a QEMU c906 will not enable MAEE, but will still boot.

That's the hope

>
> We never had a complete MAEE implementation upstream, because that's not needed.
> But until recently we could mess with reserved bits how we want.
> Now that QEMU is more restrictive about reserved bits in the PTEs, we
> need to address this in the kernel.
>
> The downside is, that the kernel now sees a c906 CPU without MAEE and
> such a CPU does not exist.

Yeah, that is the downside. But in reality a CPU could exist that
doesn't allow seeing MAEE, so I don't think it's that insane.

> But that's fine, because this does not require extra code to handle this case.
> Also, the additional check for the MAEE bit in the kernel errata
> probing code is likely needed anyway for future T-Head CPUs.

Exactly

>
> BTW, what was the reason for QEMU to prevent setting reserved bits in PTEs?

I don't have it in front of me, but I thought that is what the spec
said should happen

Alistair

>
> Thanks,
> Christoph
>
> >
> > Alistair
> >
> > >          }
> > >
> > > @@ -998,7 +1000,8 @@ restart:
> > >          }
> > >
> > >          /* Inner PTE, continue walking */
> > > -        if (pte & (PTE_D | PTE_A | PTE_U | PTE_ATTR)) {
> > > +        if ((pte & (PTE_D | PTE_A | PTE_U)) ||
> > > +            (!skip_pte_check && (pte & PTE_ATTR))) {
> > >              return TRANSLATE_FAIL;
> > >          }
> > >          base = ppn << PGSHIFT;
> > > @@ -1012,7 +1015,7 @@ restart:
> > >          /* Misaligned PPN */
> > >          return TRANSLATE_FAIL;
> > >      }
> > > -    if (!pbmte && (pte & PTE_PBMT)) {
> > > +    if (!skip_pte_check && !pbmte && (pte & PTE_PBMT)) {
> > >          /* Reserved without Svpbmt. */
> > >          return TRANSLATE_FAIL;
> > >      }
> > > diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> > > index a5e0734e7f..d7f675881d 100644
> > > --- a/target/riscv/meson.build
> > > +++ b/target/riscv/meson.build
> > > @@ -12,6 +12,7 @@ riscv_ss.add(files(
> > >    'cpu.c',
> > >    'cpu_helper.c',
> > >    'csr.c',
> > > +  'xthead_csr.c',
> > >    'fpu_helper.c',
> > >    'gdbstub.c',
> > >    'op_helper.c',
> > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > > index 559bf373f3..4b1184c8ab 100644
> > > --- a/target/riscv/tcg/tcg-cpu.c
> > > +++ b/target/riscv/tcg/tcg-cpu.c
> > > @@ -763,7 +763,6 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> > >          cpu->cfg.pmu_mask = 0;
> > >          cpu->pmu_avail_ctrs = 0;
> > >      }
> > > -
> > >      /*
> > >       * Disable isa extensions based on priv spec after we
> > >       * validated and set everything we need.
> > > @@ -871,12 +870,18 @@ static void riscv_cpu_validate_profiles(RISCVCPU *cpu)
> > >      }
> > >  }
> > >
> > > +static inline bool th_csr_p(const RISCVCPUConfig *cfg)
> > > +{
> > > +    return cfg->ext_xtheadmaee;
> > > +}
> > > +
> > >  void riscv_tcg_cpu_register_vendor_csr(RISCVCPU *cpu)
> > >  {
> > >      static const struct {
> > >          bool (*guard_func)(const RISCVCPUConfig *);
> > >          riscv_csr_operations *csr_ops;
> > >      } vendors[] = {
> > > +        { th_csr_p, th_csr_ops },
> > >      };
> > >      for (int i = 0; i < ARRAY_SIZE(vendors); ++i) {
> > >          if (!vendors[i].guard_func(&cpu->cfg)) {
> > > diff --git a/target/riscv/xthead_csr.c b/target/riscv/xthead_csr.c
> > > new file mode 100644
> > > index 0000000000..9f88fa50db
> > > --- /dev/null
> > > +++ b/target/riscv/xthead_csr.c
> > > @@ -0,0 +1,65 @@
> > > +/*
> > > + * Xuantie implementation for RISC-V Control and Status Registers.
> > > + *
> > > + * Copyright (c) 2024 Alibaba Group. All rights reserved.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify it
> > > + * under the terms and conditions of the GNU General Public License,
> > > + * version 2 or later, as published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope it will be useful, but WITHOUT
> > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > > + * more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License along with
> > > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qemu/log.h"
> > > +#include "cpu.h"
> > > +#include "tcg/tcg-cpu.h"
> > > +#include "exec/exec-all.h"
> > > +#include "exec/tb-flush.h"
> > > +#include "qapi/error.h"
> > > +
> > > +#if !defined(CONFIG_USER_ONLY)
> > > +static RISCVException th_maee_check(CPURISCVState *env, int csrno)
> > > +{
> > > +    if (riscv_cpu_cfg(env)->ext_xtheadmaee) {
> > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > +    }
> > > +    return RISCV_EXCP_NONE;
> > > +}
> > > +
> > > +static RISCVException
> > > +read_th_mxstatus(CPURISCVState *env, int csrno, target_ulong *val)
> > > +{
> > > +    *val = env->th_mxstatus;
> > > +    return RISCV_EXCP_NONE;
> > > +}
> > > +
> > > +static RISCVException
> > > +write_th_mxstatus(CPURISCVState *env, int csrno, target_ulong val)
> > > +{
> > > +    uint64_t mxstatus = env->th_mxstatus;
> > > +    uint64_t mask = TH_MXSTATUS_MAEE;
> > > +
> > > +    if ((val ^ mxstatus) & TH_MXSTATUS_MAEE) {
> > > +        tlb_flush(env_cpu(env));
> > > +    }
> > > +
> > > +    mxstatus = (mxstatus & ~mask) | (val & mask);
> > > +    env->th_mxstatus = mxstatus;
> > > +    return RISCV_EXCP_NONE;
> > > +}
> > > +#endif
> > > +
> > > +riscv_csr_operations th_csr_ops[CSR_TABLE_SIZE] = {
> > > +#if !defined(CONFIG_USER_ONLY)
> > > +    [CSR_TH_MXSTATUS]     = { "th_mxstatus", th_maee_check, read_th_mxstatus,
> > > +                                                            write_th_mxstatus},
> > > +#endif /* !CONFIG_USER_ONLY */
> > > +};
> > > --
> > > 2.25.1
> > >
> > >

Re: [PATCH v2 2/2] target/riscv: Support xtheadmaee for thead-c906

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3666 bytes --]

Christoph linked here on his submission to Linux of a fix for this, so I
am reviving this to leave a couple comments :)

On Thu, Feb 15, 2024 at 02:24:02PM +1000, Alistair Francis wrote:
> On Mon, Feb 5, 2024 at 6:37 PM Christoph Müllner
> <christoph.muellner@vrull.eu> wrote:
> > On Mon, Feb 5, 2024 at 3:42 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > On Sun, Feb 4, 2024 at 3:44 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:

> > > >              ppn = (pte & (target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> > >
> > > Unfortunately we won't be able to take this upstream. This is core
> > > QEMU RISC-V code that is now being changed against the spec. I think
> > > adding the CSR is fine, but we can't take this core change.
> > >
> > > A fix that works for everyone should be supporting the th_mxstatus
> > > CSR, but don't support setting the TH_MXSTATUS_MAEE bit. That way
> > > guests can detect that the bit isn't set and not use the reserved bits
> > > in the PTE. From my understanding the extra PTE bits are related to
> > > cache control in the hardware, which we don't need here
> >
> > Sounds good! Let me recap the overall plan:
> > * QEMU does not emulate MAEE, but signals that MAEE is not available
> > by setting TH_MXSTATUS_MAEE to 0.
> 
> Yep!
> 
> > * Consequence: The c906 emulation does not enable any page-base memory
> > attribute mechanism.
> 
> Exactly
> 
> > * OpenSBI tests the TH_MXSTATUS_MAEE bit (M-mode only) and provides
> > that information to user-space (e.g. DTB).
> 
> To the kernel, but yep!
> 
> > * The current Linux errata code will be enhanced to not assume MAEE
> > for each core with T-Head vendor ID, but also query the MAEE bit and
> > ensure it is set.
> 
> I feel like it should already do that :)

It doesn't quite do this right now. It only makes the assumption for
CPUs where marchid and mvendorid are zero. The c908, and I think Guo Ren
confirmed it will be the case going forward, sets these to non-zero
values. We should have always required a dt property be set, rather than
using m*id, but we can't go back on that for these devices. Going
forward, if there are more CPUs that want to use this e.g. C908 in MAEE
mode (it can do svpbmt too) I'm gonna require it is explicitly set in
DT.

> > * Booting on a QEMU c906 will not enable MAEE, but will still boot.
> 
> That's the hope
> 
> >
> > We never had a complete MAEE implementation upstream, because that's not needed.
> > But until recently we could mess with reserved bits how we want.
> > Now that QEMU is more restrictive about reserved bits in the PTEs, we
> > need to address this in the kernel.
> >
> > The downside is, that the kernel now sees a c906 CPU without MAEE and
> > such a CPU does not exist.
> 
> Yeah, that is the downside. But in reality a CPU could exist that
> doesn't allow seeing MAEE, so I don't think it's that insane.

Aye, and the case of a new CPU disabling it but not setting m*id will be
a pain.

> > But that's fine, because this does not require extra code to handle this case.
> > Also, the additional check for the MAEE bit in the kernel errata
> > probing code is likely needed anyway for future T-Head CPUs.
> 
> Exactly

I don't really want to have extension detection side channels like this
in Linux if it can be at all avoided, I'd rather get this information from
DT or ACPI. The marchid == mvendorid == 0x0 case has a pretty high chance
of needing some special handling in the future though, so something
targeting those cases when there's some demonstrable problem seems fair
to me.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/2] target/riscv: Support xtheadmaee for thead-c906

[Alistair Francis]

On Wed, Mar 27, 2024 at 9:19 PM Conor Dooley <conor@kernel.org> wrote:
>
> Christoph linked here on his submission to Linux of a fix for this, so I
> am reviving this to leave a couple comments :)
>
> On Thu, Feb 15, 2024 at 02:24:02PM +1000, Alistair Francis wrote:
> > On Mon, Feb 5, 2024 at 6:37 PM Christoph Müllner
> > <christoph.muellner@vrull.eu> wrote:
> > > On Mon, Feb 5, 2024 at 3:42 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > > On Sun, Feb 4, 2024 at 3:44 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> > > > >              ppn = (pte & (target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> > > >
> > > > Unfortunately we won't be able to take this upstream. This is core
> > > > QEMU RISC-V code that is now being changed against the spec. I think
> > > > adding the CSR is fine, but we can't take this core change.
> > > >
> > > > A fix that works for everyone should be supporting the th_mxstatus
> > > > CSR, but don't support setting the TH_MXSTATUS_MAEE bit. That way
> > > > guests can detect that the bit isn't set and not use the reserved bits
> > > > in the PTE. From my understanding the extra PTE bits are related to
> > > > cache control in the hardware, which we don't need here
> > >
> > > Sounds good! Let me recap the overall plan:
> > > * QEMU does not emulate MAEE, but signals that MAEE is not available
> > > by setting TH_MXSTATUS_MAEE to 0.
> >
> > Yep!
> >
> > > * Consequence: The c906 emulation does not enable any page-base memory
> > > attribute mechanism.
> >
> > Exactly
> >
> > > * OpenSBI tests the TH_MXSTATUS_MAEE bit (M-mode only) and provides
> > > that information to user-space (e.g. DTB).
> >
> > To the kernel, but yep!
> >
> > > * The current Linux errata code will be enhanced to not assume MAEE
> > > for each core with T-Head vendor ID, but also query the MAEE bit and
> > > ensure it is set.
> >
> > I feel like it should already do that :)
>
> It doesn't quite do this right now. It only makes the assumption for
> CPUs where marchid and mvendorid are zero. The c908, and I think Guo Ren
> confirmed it will be the case going forward, sets these to non-zero
> values. We should have always required a dt property be set, rather than
> using m*id, but we can't go back on that for these devices. Going
> forward, if there are more CPUs that want to use this e.g. C908 in MAEE
> mode (it can do svpbmt too) I'm gonna require it is explicitly set in
> DT.

A DT node that we don't set also works fine for us

Alistair

Re: [PATCH v2 2/2] target/riscv: Support xtheadmaee for thead-c906

[Christoph Müllner]

On Thu, Mar 28, 2024 at 2:19 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Mar 27, 2024 at 9:19 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > Christoph linked here on his submission to Linux of a fix for this, so I
> > am reviving this to leave a couple comments :)
> >
> > On Thu, Feb 15, 2024 at 02:24:02PM +1000, Alistair Francis wrote:
> > > On Mon, Feb 5, 2024 at 6:37 PM Christoph Müllner
> > > <christoph.muellner@vrull.eu> wrote:
> > > > On Mon, Feb 5, 2024 at 3:42 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > > > On Sun, Feb 4, 2024 at 3:44 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
> >
> > > > > >              ppn = (pte & (target_ulong)PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> > > > >
> > > > > Unfortunately we won't be able to take this upstream. This is core
> > > > > QEMU RISC-V code that is now being changed against the spec. I think
> > > > > adding the CSR is fine, but we can't take this core change.
> > > > >
> > > > > A fix that works for everyone should be supporting the th_mxstatus
> > > > > CSR, but don't support setting the TH_MXSTATUS_MAEE bit. That way
> > > > > guests can detect that the bit isn't set and not use the reserved bits
> > > > > in the PTE. From my understanding the extra PTE bits are related to
> > > > > cache control in the hardware, which we don't need here
> > > >
> > > > Sounds good! Let me recap the overall plan:
> > > > * QEMU does not emulate MAEE, but signals that MAEE is not available
> > > > by setting TH_MXSTATUS_MAEE to 0.
> > >
> > > Yep!
> > >
> > > > * Consequence: The c906 emulation does not enable any page-base memory
> > > > attribute mechanism.
> > >
> > > Exactly
> > >
> > > > * OpenSBI tests the TH_MXSTATUS_MAEE bit (M-mode only) and provides
> > > > that information to user-space (e.g. DTB).
> > >
> > > To the kernel, but yep!
> > >
> > > > * The current Linux errata code will be enhanced to not assume MAEE
> > > > for each core with T-Head vendor ID, but also query the MAEE bit and
> > > > ensure it is set.
> > >
> > > I feel like it should already do that :)
> >
> > It doesn't quite do this right now. It only makes the assumption for
> > CPUs where marchid and mvendorid are zero. The c908, and I think Guo Ren
> > confirmed it will be the case going forward, sets these to non-zero
> > values. We should have always required a dt property be set, rather than
> > using m*id, but we can't go back on that for these devices. Going
> > forward, if there are more CPUs that want to use this e.g. C908 in MAEE
> > mode (it can do svpbmt too) I'm gonna require it is explicitly set in
>
> A DT node that we don't set also works fine for us

I would really like to do that, but given the page table is set up so
early in the boot process
probing via CSR is much easier to realize in the kernel than parsing the DT.
Therefore, I think th.sxstatus emulation + probing is the best way to
move forward.

Thanks,
Christoph