subject:"\[PATCH 2\/2\] target\/riscv\: Support xtheadmaee for thead\-c906"

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

2024-02-03 Thread LIU Zhiwei




On 2024/1/30 19:43, Christoph Müllner wrote:

On Tue, Jan 30, 2024 at 12:12 PM LIU Zhiwei
 wrote:

thead-c906 uses some flags in pte [60-63] bits. It has history reasons that
SVPBMT didn't exist when thead-c906 came to world.

We named this feature as xtheadmaee. this feature is controlled by an custom
CSR named mxstatus, whose maee field encodes whether enable the pte [60-63] 
bits.

The sections "5.2.2.1 Page table structure" and "15.1.7.1 M-mode extension
status register (MXSTATUS)" in document[1] give the detailed information
about its design.

I would prefer if we would not define an extension like XTheadMaee
without a specification.
The linked document defines the bit MAEE in a custom CSR, but the
scope of XTheadMaee
is not clearly defined (the term XTheadMaee is not even part of the PDF).

We have all the XThead* extensions well described here:
   https://github.com/T-head-Semi/thead-extension-spec/tree/master
And it would not be much effort to add XTheadMaee there as well.


Done.  Thanks for the comment. The XTheadMaee specification is here:

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

Thanks,
Zhiwei



For those who don't know the context of this patch, here is the c906
boot regression report from Björn:
   https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg04766.html

BR
Christoph

Christoph


[1]:https://occ-intl-prod.oss-ap-southeast-1.aliyuncs.com/resource//1699265191641/XuanTie-Openc906-UserManual.pdf
Signed-off-by: LIU Zhiwei 
---
  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 |  9 ++
  target/riscv/xthead_csr.c  | 63 ++
  8 files changed, 105 insertions(+), 18 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 <<

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

2024-01-31 Thread Conor Dooley

On Tue, Jan 30, 2024 at 12:43:25PM +0100, Christoph Müllner wrote:
> On Tue, Jan 30, 2024 at 12:12 PM LIU Zhiwei
>  wrote:
> >
> > thead-c906 uses some flags in pte [60-63] bits. It has history reasons that
> > SVPBMT didn't exist when thead-c906 came to world.
> >
> > We named this feature as xtheadmaee. this feature is controlled by an custom
> > CSR named mxstatus, whose maee field encodes whether enable the pte [60-63] 
> > bits.
> >
> > The sections "5.2.2.1 Page table structure" and "15.1.7.1 M-mode extension
> > status register (MXSTATUS)" in document[1] give the detailed information
> > about its design.
> 
> I would prefer if we would not define an extension like XTheadMaee
> without a specification.
> The linked document defines the bit MAEE in a custom CSR, but the
> scope of XTheadMaee
> is not clearly defined (the term XTheadMaee is not even part of the PDF).
> 
> We have all the XThead* extensions well described here:
>   https://github.com/T-head-Semi/thead-extension-spec/tree/master
> And it would not be much effort to add XTheadMaee there as well.

Yeah, I was gonna request exactly this, so glad to see you beat me to
the punch. It would be really great if this was done, particularly if
this xmaee is going to appear in devicetrees and sooner or later is
gonna want to be documented in a binding.

Cheers,
Conor.

> For those who don't know the context of this patch, here is the c906
> boot regression report from Björn:
>   https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg04766.html


signature.asc
Description: PGP signature

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

2024-01-30 Thread LIU Zhiwei




On 2024/1/31 13:07, Richard Henderson wrote:

On 1/30/24 21:11, LIU Zhiwei wrote:

+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 */
+};


This is clearly the wrong data structure for a single entry in the array.


This array should have the same size with csr_ops so that we can 
override custom CSR behavior directly. Besides, It will have other 
entries in the near future.


I see that I missed surround the th_maee_check, read_th_mxstatus, 
write_mxstatus with !CONFIG_USER_ONLY.  But I don't understand why it is 
wrong for a single entry in the array, at least the compiler think it 
has no error.


Thanks,
Zhiwei




r~

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

2024-01-30 Thread Richard Henderson


On 1/30/24 21:11, LIU Zhiwei wrote:

+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 */
+};


This is clearly the wrong data structure for a single entry in the array.


r~

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

2024-01-30 Thread Christoph Müllner

On Tue, Jan 30, 2024 at 12:12 PM LIU Zhiwei
 wrote:
>
> thead-c906 uses some flags in pte [60-63] bits. It has history reasons that
> SVPBMT didn't exist when thead-c906 came to world.
>
> We named this feature as xtheadmaee. this feature is controlled by an custom
> CSR named mxstatus, whose maee field encodes whether enable the pte [60-63] 
> bits.
>
> The sections "5.2.2.1 Page table structure" and "15.1.7.1 M-mode extension
> status register (MXSTATUS)" in document[1] give the detailed information
> about its design.

I would prefer if we would not define an extension like XTheadMaee
without a specification.
The linked document defines the bit MAEE in a custom CSR, but the
scope of XTheadMaee
is not clearly defined (the term XTheadMaee is not even part of the PDF).

We have all the XThead* extensions well described here:
  https://github.com/T-head-Semi/thead-extension-spec/tree/master
And it would not be much effort to add XTheadMaee there as well.

For those who don't know the context of this patch, here is the c906
boot regression report from Björn:
  https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg04766.html

BR
Christoph

Christoph

>
> [1]:https://occ-intl-prod.oss-ap-southeast-1.aliyuncs.com/resource//1699265191641/XuanTie-Openc906-UserManual.pdf
> Signed-off-by: LIU Zhiwei 
> ---
>  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 |  9 ++
>  target/riscv/xthead_csr.c  | 63 ++
>  8 files changed, 105 insertions(+), 18 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

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

2024-01-30 Thread LIU Zhiwei

thead-c906 uses some flags in pte [60-63] bits. It has history reasons that
SVPBMT didn't exist when thead-c906 came to world.

We named this feature as xtheadmaee. this feature is controlled by an custom
CSR named mxstatus, whose maee field encodes whether enable the pte [60-63] 
bits.

The sections "5.2.2.1 Page table structure" and "15.1.7.1 M-mode extension
status register (MXSTATUS)" in document[1] give the detailed information
about its design.

[1]:https://occ-intl-prod.oss-ap-southeast-1.aliyuncs.com/resource//1699265191641/XuanTie-Openc906-UserManual.pdf
Signed-off-by: LIU Zhiwei 
---
 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 |  9 ++
 target/riscv/xthead_csr.c  | 63 ++
 8 files changed, 105 insertions(+), 18 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
---

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

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

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

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

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

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

6 matches

Site Navigation

Mail list logo

Footer information