[PATCH 0/2] RISC-V: Test th.mxstatus.MAEE bit before enabling MAEE

[PATCH 0/2] RISC-V: Test th.mxstatus.MAEE bit before enabling MAEE

[Christoph Müllner]

Currently, the Linux kernel suffers from a boot regression when running
on the c906 QEMU emulation. Details have been reported here by Björn Töpel:
  https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg04766.html

The main issue is, that Linux enables MAEE for CPUs that have a T-Head
mvendorid but QEMU maintainers don't want to emulate a CPU that uses
reserved bits in PTEs. See also the following discussion for more
context:
  https://lists.gnu.org/archive/html/qemu-devel/2024-02/msg00775.html

This series renames "T-Head PBMT" to "MAEE" and only enables it if
the th.mxstatus.MAEE bit is set.

Since th.mxstatus is only accessible in M-mode, this patch depends on
the following OpenSBI series:
  http://lists.infradead.org/pipermail/opensbi/2024-March/006718.html

The th.mxstatus CSR is documented here:
  https://github.com/T-head-Semi/thead-extension-spec/pull/45

The QEMU patch to emulate th.mxstatus with MAEE not set is here:
  https://lists.gnu.org/archive/html/qemu-devel/2024-03/msg06368.html

After applying all referenced patchsets, this patchset allows to
successfully boot a C906 QEMU system emulation ("-cpu thead-c906").

Christoph Müllner (2):
  riscv: thead: Rename T-Head PBMT to MAEE
  riscv: T-Head: Test availability bit before enabling MAEE errata

 arch/riscv/Kconfig.errata            |  8 ++++----
 arch/riscv/errata/thead/errata.c     | 22 ++++++++++++++--------
 arch/riscv/include/asm/errata_list.h | 20 ++++++++++----------
 3 files changed, 28 insertions(+), 22 deletions(-)

-- 
2.44.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/2] riscv: thead: Rename T-Head PBMT to MAEE

[Christoph Müllner]

T-Head's vendor extension to set page attributes has the name
MAEE (MMU address attribute extension).
Let's rename it, so it is clear what this referes to.

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

Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
---
 arch/riscv/Kconfig.errata            |  8 ++++----
 arch/riscv/errata/thead/errata.c     |  8 ++++----
 arch/riscv/include/asm/errata_list.h | 20 ++++++++++----------
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata
index 910ba8837add..2c24bef7e112 100644
--- a/arch/riscv/Kconfig.errata
+++ b/arch/riscv/Kconfig.errata
@@ -82,14 +82,14 @@ config ERRATA_THEAD
 
 	  Otherwise, please say "N" here to avoid unnecessary overhead.
 
-config ERRATA_THEAD_PBMT
-	bool "Apply T-Head memory type errata"
+config ERRATA_THEAD_MAEE
+	bool "Apply T-Head's MMU address attribute (MAEE)"
 	depends on ERRATA_THEAD && 64BIT && MMU
 	select RISCV_ALTERNATIVE_EARLY
 	default y
 	help
-	  This will apply the memory type errata to handle the non-standard
-	  memory type bits in page-table-entries on T-Head SoCs.
+	  This will apply the memory type errata to handle T-Head's MMU address
+	  attribute extension (MAEE).
 
 	  If you don't know what to do here, say "Y".
 
diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
index b1c410bbc1ae..8c8a8a4b0421 100644
--- a/arch/riscv/errata/thead/errata.c
+++ b/arch/riscv/errata/thead/errata.c
@@ -19,10 +19,10 @@
 #include <asm/patch.h>
 #include <asm/vendorid_list.h>
 
-static bool errata_probe_pbmt(unsigned int stage,
+static bool errata_probe_maee(unsigned int stage,
 			      unsigned long arch_id, unsigned long impid)
 {
-	if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PBMT))
+	if (!IS_ENABLED(CONFIG_ERRATA_THEAD_MAEE))
 		return false;
 
 	if (arch_id != 0 || impid != 0)
@@ -140,8 +140,8 @@ static u32 thead_errata_probe(unsigned int stage,
 {
 	u32 cpu_req_errata = 0;
 
-	if (errata_probe_pbmt(stage, archid, impid))
-		cpu_req_errata |= BIT(ERRATA_THEAD_PBMT);
+	if (errata_probe_maee(stage, archid, impid))
+		cpu_req_errata |= BIT(ERRATA_THEAD_MAEE);
 
 	errata_probe_cmo(stage, archid, impid);
 
diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
index ea33288f8a25..7c377e137b41 100644
--- a/arch/riscv/include/asm/errata_list.h
+++ b/arch/riscv/include/asm/errata_list.h
@@ -23,7 +23,7 @@
 #endif
 
 #ifdef CONFIG_ERRATA_THEAD
-#define	ERRATA_THEAD_PBMT 0
+#define	ERRATA_THEAD_MAEE 0
 #define	ERRATA_THEAD_PMU 1
 #define	ERRATA_THEAD_NUMBER 2
 #endif
@@ -53,20 +53,20 @@ asm(ALTERNATIVE("sfence.vma %0", "sfence.vma", SIFIVE_VENDOR_ID,	\
  * in the default case.
  */
 #define ALT_SVPBMT_SHIFT 61
-#define ALT_THEAD_PBMT_SHIFT 59
+#define ALT_THEAD_MAEE_SHIFT 59
 #define ALT_SVPBMT(_val, prot)						\
 asm(ALTERNATIVE_2("li %0, 0\t\nnop",					\
 		  "li %0, %1\t\nslli %0,%0,%3", 0,			\
 			RISCV_ISA_EXT_SVPBMT, CONFIG_RISCV_ISA_SVPBMT,	\
 		  "li %0, %2\t\nslli %0,%0,%4", THEAD_VENDOR_ID,	\
-			ERRATA_THEAD_PBMT, CONFIG_ERRATA_THEAD_PBMT)	\
+			ERRATA_THEAD_MAEE, CONFIG_ERRATA_THEAD_MAEE)	\
 		: "=r"(_val)						\
 		: "I"(prot##_SVPBMT >> ALT_SVPBMT_SHIFT),		\
-		  "I"(prot##_THEAD >> ALT_THEAD_PBMT_SHIFT),		\
+		  "I"(prot##_THEAD >> ALT_THEAD_MAEE_SHIFT),		\
 		  "I"(ALT_SVPBMT_SHIFT),				\
-		  "I"(ALT_THEAD_PBMT_SHIFT))
+		  "I"(ALT_THEAD_MAEE_SHIFT))
 
-#ifdef CONFIG_ERRATA_THEAD_PBMT
+#ifdef CONFIG_ERRATA_THEAD_MAEE
 /*
  * IO/NOCACHE memory types are handled together with svpbmt,
  * so on T-Head chips, check if no other memory type is set,
@@ -83,11 +83,11 @@ asm volatile(ALTERNATIVE(						\
 	"slli    t3, t3, %3\n\t"					\
 	"or      %0, %0, t3\n\t"					\
 	"2:",  THEAD_VENDOR_ID,						\
-		ERRATA_THEAD_PBMT, CONFIG_ERRATA_THEAD_PBMT)		\
+		ERRATA_THEAD_MAEE, CONFIG_ERRATA_THEAD_MAEE)		\
 	: "+r"(_val)							\
-	: "I"(_PAGE_MTMASK_THEAD >> ALT_THEAD_PBMT_SHIFT),		\
-	  "I"(_PAGE_PMA_THEAD >> ALT_THEAD_PBMT_SHIFT),			\
-	  "I"(ALT_THEAD_PBMT_SHIFT)					\
+	: "I"(_PAGE_MTMASK_THEAD >> ALT_THEAD_MAEE_SHIFT),		\
+	  "I"(_PAGE_PMA_THEAD >> ALT_THEAD_MAEE_SHIFT),			\
+	  "I"(ALT_THEAD_MAEE_SHIFT)					\
 	: "t3")
 #else
 #define ALT_THEAD_PMA(_val)
-- 
2.44.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/2] riscv: T-Head: Test availability bit before enabling MAEE errata

[Christoph Müllner]

T-Head's MAEE mechanism (non-compatible equivalent of RVI's Svpbmt)
is currently assumed for all T-Head harts. However, QEMU recently
decided to drop acceptance of guests that write reserved bits in PTEs.
As MAEE uses reserved bits in PTEs and Linux applies the MAEE errata
for all T-Head harts, this broke the Linux startup on QEMU emulations
of the C906 emulation.

This patch attempts to address this issue by testing the MAEE bit
in TH_MXSTATUS CSR. As the TH_MXSTATUS CSR is only accessible in M-mode
this patch depends on M-mode firmware that handles this for us
transparently.

As this patch breaks Linux bootup on all C9xx machines with MAEE,
which don't have M-mode firmware that handles the access to the
TH_MXSTATUS CSR, this patch is marked as RFC.

Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
---
 arch/riscv/errata/thead/errata.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
index 8c8a8a4b0421..dd7bf6c62a35 100644
--- a/arch/riscv/errata/thead/errata.c
+++ b/arch/riscv/errata/thead/errata.c
@@ -19,6 +19,9 @@
 #include <asm/patch.h>
 #include <asm/vendorid_list.h>
 
+#define CSR_TH_MXSTATUS		0x7c0
+#define MXSTATUS_MAEE		_AC(0x200000, UL)
+
 static bool errata_probe_maee(unsigned int stage,
 			      unsigned long arch_id, unsigned long impid)
 {
@@ -28,11 +31,14 @@ static bool errata_probe_maee(unsigned int stage,
 	if (arch_id != 0 || impid != 0)
 		return false;
 
-	if (stage == RISCV_ALTERNATIVES_EARLY_BOOT ||
-	    stage == RISCV_ALTERNATIVES_MODULE)
-		return true;
+	if (stage != RISCV_ALTERNATIVES_EARLY_BOOT &&
+	    stage != RISCV_ALTERNATIVES_MODULE)
+		return false;
 
-	return false;
+	if (!(csr_read(CSR_TH_MXSTATUS) & MXSTATUS_MAEE))
+		return false;
+
+	return true;
 }
 
 /*
-- 
2.44.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling MAEE errata

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3245 bytes --]

On Wed, Mar 27, 2024 at 11:31:30AM +0100, Christoph Müllner wrote:
> T-Head's MAEE mechanism (non-compatible equivalent of RVI's Svpbmt)
> is currently assumed for all T-Head harts. However, QEMU recently
> decided to drop acceptance of guests that write reserved bits in PTEs.
> As MAEE uses reserved bits in PTEs and Linux applies the MAEE errata
> for all T-Head harts, this broke the Linux startup on QEMU emulations
> of the C906 emulation.
> 
> This patch attempts to address this issue by testing the MAEE bit
> in TH_MXSTATUS CSR. As the TH_MXSTATUS CSR is only accessible in M-mode
> this patch depends on M-mode firmware that handles this for us
> transparently.
> 

> As this patch breaks Linux bootup on all C9xx machines with MAEE,
> which don't have M-mode firmware that handles the access to the
> TH_MXSTATUS CSR, this patch is marked as RFC.

I think this is gonna be unacceptable in its current state given that it
causes problems for every other version of the firmware. Breaking real
systems for the sake of emulation isn't something we can reasonably do.

To make this sort of change acceptable, you're gonna have to add some way
to differentiate between systems that do and do not support reading this
CSR. I think we either a) need to check the version of the SBI
implementation to see if it hits the threshold for supporting this
feature, or b) add a specific SBI call for this so that we can
differentiate between firmware not supporting the function and the
quote-unquote "hardware" not supporting it. I don't really like option a)
as it could grow to several different options (each for a different SBI
implementation) and support for reading the CSR would need to be
unconditional. I have a feeling that I am missing something though,
that'd make it doable without introducing a new call.

Thanks,
Conor.

If only we'd made enabling this be controlled by a specific DT property,
then disabling it in QEMU would be as simple as not setting that
property :(

> 
> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> ---
>  arch/riscv/errata/thead/errata.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> index 8c8a8a4b0421..dd7bf6c62a35 100644
> --- a/arch/riscv/errata/thead/errata.c
> +++ b/arch/riscv/errata/thead/errata.c
> @@ -19,6 +19,9 @@
>  #include <asm/patch.h>
>  #include <asm/vendorid_list.h>
>  
> +#define CSR_TH_MXSTATUS		0x7c0
> +#define MXSTATUS_MAEE		_AC(0x200000, UL)
> +
>  static bool errata_probe_maee(unsigned int stage,
>  			      unsigned long arch_id, unsigned long impid)
>  {
> @@ -28,11 +31,14 @@ static bool errata_probe_maee(unsigned int stage,
>  	if (arch_id != 0 || impid != 0)
>  		return false;
>  
> -	if (stage == RISCV_ALTERNATIVES_EARLY_BOOT ||
> -	    stage == RISCV_ALTERNATIVES_MODULE)
> -		return true;
> +	if (stage != RISCV_ALTERNATIVES_EARLY_BOOT &&
> +	    stage != RISCV_ALTERNATIVES_MODULE)
> +		return false;
>  
> -	return false;
> +	if (!(csr_read(CSR_TH_MXSTATUS) & MXSTATUS_MAEE))
> +		return false;
> +
> +	return true;
>  }
>  
>  /*
> -- 
> 2.44.0
> 

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling MAEE errata

[Andrew Jones]

On Wed, Mar 27, 2024 at 11:03:06AM +0000, Conor Dooley wrote:
> On Wed, Mar 27, 2024 at 11:31:30AM +0100, Christoph Müllner wrote:
> > T-Head's MAEE mechanism (non-compatible equivalent of RVI's Svpbmt)
> > is currently assumed for all T-Head harts. However, QEMU recently
> > decided to drop acceptance of guests that write reserved bits in PTEs.
> > As MAEE uses reserved bits in PTEs and Linux applies the MAEE errata
> > for all T-Head harts, this broke the Linux startup on QEMU emulations
> > of the C906 emulation.
> > 
> > This patch attempts to address this issue by testing the MAEE bit
> > in TH_MXSTATUS CSR. As the TH_MXSTATUS CSR is only accessible in M-mode
> > this patch depends on M-mode firmware that handles this for us
> > transparently.
> > 
> 
> > As this patch breaks Linux bootup on all C9xx machines with MAEE,
> > which don't have M-mode firmware that handles the access to the
> > TH_MXSTATUS CSR, this patch is marked as RFC.

Can we wrap the csr access in a _ASM_EXTABLE()? If firmware handles it,
then we return true/false based on the value. If firmware doesn't handle
it, and we get an illegal instruction exception, then we assume the bit
is set, which is the current behavior.

> 
> I think this is gonna be unacceptable in its current state given that it
> causes problems for every other version of the firmware. Breaking real
> systems for the sake of emulation isn't something we can reasonably do.
> 
> To make this sort of change acceptable, you're gonna have to add some way
> to differentiate between systems that do and do not support reading this
> CSR. I think we either a) need to check the version of the SBI
> implementation to see if it hits the threshold for supporting this
> feature, or b) add a specific SBI call for this so that we can
> differentiate between firmware not supporting the function and the

The FWFT SBI extension is being developed as a mechanism for S-mode to ask
M-mode things like this, but I think that extension should be used for
features that have potential to be changed by S-mode (even if not
everything will be changeable on all platforms), whereas anything that's
read-only would be better with...

> quote-unquote "hardware" not supporting it. I don't really like option a)
> as it could grow to several different options (each for a different SBI
> implementation) and support for reading the CSR would need to be
> unconditional. I have a feeling that I am missing something though,
> that'd make it doable without introducing a new call.
> 
> Thanks,
> Conor.
> 
> If only we'd made enabling this be controlled by a specific DT property,
> then disabling it in QEMU would be as simple as not setting that
> property :(

...this, where "DT property" is "ISA extension name". I wonder if we
shouldn't start considering the invention of xlinux_vendor_xyz type
extension names which firmware could add to the ISA string / array,
in order to communicate read-only information like this?

Thanks,
drew

> 
> > 
> > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> > ---
> >  arch/riscv/errata/thead/errata.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> > index 8c8a8a4b0421..dd7bf6c62a35 100644
> > --- a/arch/riscv/errata/thead/errata.c
> > +++ b/arch/riscv/errata/thead/errata.c
> > @@ -19,6 +19,9 @@
> >  #include <asm/patch.h>
> >  #include <asm/vendorid_list.h>
> >  
> > +#define CSR_TH_MXSTATUS		0x7c0
> > +#define MXSTATUS_MAEE		_AC(0x200000, UL)
> > +
> >  static bool errata_probe_maee(unsigned int stage,
> >  			      unsigned long arch_id, unsigned long impid)
> >  {
> > @@ -28,11 +31,14 @@ static bool errata_probe_maee(unsigned int stage,
> >  	if (arch_id != 0 || impid != 0)
> >  		return false;
> >  
> > -	if (stage == RISCV_ALTERNATIVES_EARLY_BOOT ||
> > -	    stage == RISCV_ALTERNATIVES_MODULE)
> > -		return true;
> > +	if (stage != RISCV_ALTERNATIVES_EARLY_BOOT &&
> > +	    stage != RISCV_ALTERNATIVES_MODULE)
> > +		return false;
> >  
> > -	return false;
> > +	if (!(csr_read(CSR_TH_MXSTATUS) & MXSTATUS_MAEE))
> > +		return false;
> > +
> > +	return true;
> >  }
> >  
> >  /*
> > -- 
> > 2.44.0
> > 



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling MAEE errata

[Christoph Müllner]

On Wed, Mar 27, 2024 at 1:41 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Wed, Mar 27, 2024 at 11:03:06AM +0000, Conor Dooley wrote:
> > On Wed, Mar 27, 2024 at 11:31:30AM +0100, Christoph Müllner wrote:
> > > T-Head's MAEE mechanism (non-compatible equivalent of RVI's Svpbmt)
> > > is currently assumed for all T-Head harts. However, QEMU recently
> > > decided to drop acceptance of guests that write reserved bits in PTEs.
> > > As MAEE uses reserved bits in PTEs and Linux applies the MAEE errata
> > > for all T-Head harts, this broke the Linux startup on QEMU emulations
> > > of the C906 emulation.
> > >
> > > This patch attempts to address this issue by testing the MAEE bit
> > > in TH_MXSTATUS CSR. As the TH_MXSTATUS CSR is only accessible in M-mode
> > > this patch depends on M-mode firmware that handles this for us
> > > transparently.
> > >
> >
> > > As this patch breaks Linux bootup on all C9xx machines with MAEE,
> > > which don't have M-mode firmware that handles the access to the
> > > TH_MXSTATUS CSR, this patch is marked as RFC.
>
> Can we wrap the csr access in a _ASM_EXTABLE()? If firmware handles it,
> then we return true/false based on the value. If firmware doesn't handle
> it, and we get an illegal instruction exception, then we assume the bit
> is set, which is the current behavior.
>
> >
> > I think this is gonna be unacceptable in its current state given that it
> > causes problems for every other version of the firmware. Breaking real
> > systems for the sake of emulation isn't something we can reasonably do.
> >
> > To make this sort of change acceptable, you're gonna have to add some way
> > to differentiate between systems that do and do not support reading this
> > CSR. I think we either a) need to check the version of the SBI
> > implementation to see if it hits the threshold for supporting this
> > feature, or b) add a specific SBI call for this so that we can
> > differentiate between firmware not supporting the function and the
>
> The FWFT SBI extension is being developed as a mechanism for S-mode to ask
> M-mode things like this, but I think that extension should be used for
> features that have potential to be changed by S-mode (even if not
> everything will be changeable on all platforms), whereas anything that's
> read-only would be better with...
>
> > quote-unquote "hardware" not supporting it. I don't really like option a)
> > as it could grow to several different options (each for a different SBI
> > implementation) and support for reading the CSR would need to be
> > unconditional. I have a feeling that I am missing something though,
> > that'd make it doable without introducing a new call.
> >
> > Thanks,
> > Conor.
> >
> > If only we'd made enabling this be controlled by a specific DT property,
> > then disabling it in QEMU would be as simple as not setting that
> > property :(
>
> ...this, where "DT property" is "ISA extension name". I wonder if we
> shouldn't start considering the invention of xlinux_vendor_xyz type
> extension names which firmware could add to the ISA string / array,
> in order to communicate read-only information like this?
>
> Thanks,
> drew

Hi Conor and Drew,

Thank you for your hints.
I fully agree with all your statements and concerns.

Switching from th.mxstatus to th.sxstatus should address all mentioned concerns:
* no dependency on OpenSBI changes
* no break of functionality
* no need for graceful handling of CSR read failures
* no need to differentiate between HW and emulation (assuming QEMU
accepts the emulation of th.sxstatus)

Also note that DT handling would be difficult, because we need to probe before
setting up the page table.

Thanks!


>
> >
> > >
> > > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> > > ---
> > >  arch/riscv/errata/thead/errata.c | 14 ++++++++++----
> > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> > > index 8c8a8a4b0421..dd7bf6c62a35 100644
> > > --- a/arch/riscv/errata/thead/errata.c
> > > +++ b/arch/riscv/errata/thead/errata.c
> > > @@ -19,6 +19,9 @@
> > >  #include <asm/patch.h>
> > >  #include <asm/vendorid_list.h>
> > >
> > > +#define CSR_TH_MXSTATUS            0x7c0
> > > +#define MXSTATUS_MAEE              _AC(0x200000, UL)
> > > +
> > >  static bool errata_probe_maee(unsigned int stage,
> > >                           unsigned long arch_id, unsigned long impid)
> > >  {
> > > @@ -28,11 +31,14 @@ static bool errata_probe_maee(unsigned int stage,
> > >     if (arch_id != 0 || impid != 0)
> > >             return false;
> > >
> > > -   if (stage == RISCV_ALTERNATIVES_EARLY_BOOT ||
> > > -       stage == RISCV_ALTERNATIVES_MODULE)
> > > -           return true;
> > > +   if (stage != RISCV_ALTERNATIVES_EARLY_BOOT &&
> > > +       stage != RISCV_ALTERNATIVES_MODULE)
> > > +           return false;
> > >
> > > -   return false;
> > > +   if (!(csr_read(CSR_TH_MXSTATUS) & MXSTATUS_MAEE))
> > > +           return false;
> > > +
> > > +   return true;
> > >  }
> > >
> > >  /*
> > > --
> > > 2.44.0
> > >
>
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling MAEE errata

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1206 bytes --]

On Thu, Mar 28, 2024 at 03:18:22PM +0100, Christoph Müllner wrote:

> Switching from th.mxstatus to th.sxstatus should address all mentioned concerns:
> * no dependency on OpenSBI changes
> * no break of functionality
> * no need for graceful handling of CSR read failures
> * no need to differentiate between HW and emulation (assuming QEMU
> accepts the emulation of th.sxstatus)

Yah, th.sxstatus seems ideal here, provided it is accepted by QEMU - but
if they allow th.mxstatus I would hope emulating th.sxstatus would be
okay too.

> Also note that DT handling would be difficult, because we need to probe before
> setting up the page table.

IIRC the kaslr seed is also read from DT prior to calling the early
alternatives stuff, so while it would be a bit more annoying than usual
I do think it is possible. My (naive) hope here though is that we don't
actually have to deal with this scenario though, as things like the c908
support Svpbmt as well as the maee version. For the k230 the plan is
to use both Zicbom and Svpbmt rather than the non-standard T-Head
alternatives:
https://lore.kernel.org/all/tencent_DF5D7CD182AFDA188E0FB80E314A21038D08@qq.com/

Cheers,
Conor.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling MAEE errata

[Alexandre Ghiti]

Hi Christoph,

On 28/03/2024 15:18, Christoph Müllner wrote:
> On Wed, Mar 27, 2024 at 1:41 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>> On Wed, Mar 27, 2024 at 11:03:06AM +0000, Conor Dooley wrote:
>>> On Wed, Mar 27, 2024 at 11:31:30AM +0100, Christoph Müllner wrote:
>>>> T-Head's MAEE mechanism (non-compatible equivalent of RVI's Svpbmt)
>>>> is currently assumed for all T-Head harts. However, QEMU recently
>>>> decided to drop acceptance of guests that write reserved bits in PTEs.
>>>> As MAEE uses reserved bits in PTEs and Linux applies the MAEE errata
>>>> for all T-Head harts, this broke the Linux startup on QEMU emulations
>>>> of the C906 emulation.
>>>>
>>>> This patch attempts to address this issue by testing the MAEE bit
>>>> in TH_MXSTATUS CSR. As the TH_MXSTATUS CSR is only accessible in M-mode
>>>> this patch depends on M-mode firmware that handles this for us
>>>> transparently.
>>>>
>>>> As this patch breaks Linux bootup on all C9xx machines with MAEE,
>>>> which don't have M-mode firmware that handles the access to the
>>>> TH_MXSTATUS CSR, this patch is marked as RFC.
>> Can we wrap the csr access in a _ASM_EXTABLE()? If firmware handles it,
>> then we return true/false based on the value. If firmware doesn't handle
>> it, and we get an illegal instruction exception, then we assume the bit
>> is set, which is the current behavior.
>>
>>> I think this is gonna be unacceptable in its current state given that it
>>> causes problems for every other version of the firmware. Breaking real
>>> systems for the sake of emulation isn't something we can reasonably do.
>>>
>>> To make this sort of change acceptable, you're gonna have to add some way
>>> to differentiate between systems that do and do not support reading this
>>> CSR. I think we either a) need to check the version of the SBI
>>> implementation to see if it hits the threshold for supporting this
>>> feature, or b) add a specific SBI call for this so that we can
>>> differentiate between firmware not supporting the function and the
>> The FWFT SBI extension is being developed as a mechanism for S-mode to ask
>> M-mode things like this, but I think that extension should be used for
>> features that have potential to be changed by S-mode (even if not
>> everything will be changeable on all platforms), whereas anything that's
>> read-only would be better with...
>>
>>> quote-unquote "hardware" not supporting it. I don't really like option a)
>>> as it could grow to several different options (each for a different SBI
>>> implementation) and support for reading the CSR would need to be
>>> unconditional. I have a feeling that I am missing something though,
>>> that'd make it doable without introducing a new call.
>>>
>>> Thanks,
>>> Conor.
>>>
>>> If only we'd made enabling this be controlled by a specific DT property,
>>> then disabling it in QEMU would be as simple as not setting that
>>> property :(
>> ...this, where "DT property" is "ISA extension name". I wonder if we
>> shouldn't start considering the invention of xlinux_vendor_xyz type
>> extension names which firmware could add to the ISA string / array,
>> in order to communicate read-only information like this?
>>
>> Thanks,
>> drew
> Hi Conor and Drew,
>
> Thank you for your hints.
> I fully agree with all your statements and concerns.
>
> Switching from th.mxstatus to th.sxstatus should address all mentioned concerns:
> * no dependency on OpenSBI changes
> * no break of functionality
> * no need for graceful handling of CSR read failures
> * no need to differentiate between HW and emulation (assuming QEMU
> accepts the emulation of th.sxstatus)
>
> Also note that DT handling would be difficult, because we need to probe before
> setting up the page table.


We already parse the DT before setting the page table to disable KASLR 
and to parse "no4lvl" or "no5lvl" command line parameters. Take a look 
at the kernel/pi directory and setup_vm() in mm/init.c.

Thanks,

Alex


>
> Thanks!
>
>
>>>> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
>>>> ---
>>>>   arch/riscv/errata/thead/errata.c | 14 ++++++++++----
>>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
>>>> index 8c8a8a4b0421..dd7bf6c62a35 100644
>>>> --- a/arch/riscv/errata/thead/errata.c
>>>> +++ b/arch/riscv/errata/thead/errata.c
>>>> @@ -19,6 +19,9 @@
>>>>   #include <asm/patch.h>
>>>>   #include <asm/vendorid_list.h>
>>>>
>>>> +#define CSR_TH_MXSTATUS            0x7c0
>>>> +#define MXSTATUS_MAEE              _AC(0x200000, UL)
>>>> +
>>>>   static bool errata_probe_maee(unsigned int stage,
>>>>                            unsigned long arch_id, unsigned long impid)
>>>>   {
>>>> @@ -28,11 +31,14 @@ static bool errata_probe_maee(unsigned int stage,
>>>>      if (arch_id != 0 || impid != 0)
>>>>              return false;
>>>>
>>>> -   if (stage == RISCV_ALTERNATIVES_EARLY_BOOT ||
>>>> -       stage == RISCV_ALTERNATIVES_MODULE)
>>>> -           return true;
>>>> +   if (stage != RISCV_ALTERNATIVES_EARLY_BOOT &&
>>>> +       stage != RISCV_ALTERNATIVES_MODULE)
>>>> +           return false;
>>>>
>>>> -   return false;
>>>> +   if (!(csr_read(CSR_TH_MXSTATUS) & MXSTATUS_MAEE))
>>>> +           return false;
>>>> +
>>>> +   return true;
>>>>   }
>>>>
>>>>   /*
>>>> --
>>>> 2.44.0
>>>>
>>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling MAEE errata

[Christoph Müllner]

On Thu, Mar 28, 2024 at 4:43 PM Alexandre Ghiti <alex@ghiti.fr> wrote:
>
> Hi Christoph,
>
> On 28/03/2024 15:18, Christoph Müllner wrote:
> > On Wed, Mar 27, 2024 at 1:41 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >> On Wed, Mar 27, 2024 at 11:03:06AM +0000, Conor Dooley wrote:
> >>> On Wed, Mar 27, 2024 at 11:31:30AM +0100, Christoph Müllner wrote:
> >>>> T-Head's MAEE mechanism (non-compatible equivalent of RVI's Svpbmt)
> >>>> is currently assumed for all T-Head harts. However, QEMU recently
> >>>> decided to drop acceptance of guests that write reserved bits in PTEs.
> >>>> As MAEE uses reserved bits in PTEs and Linux applies the MAEE errata
> >>>> for all T-Head harts, this broke the Linux startup on QEMU emulations
> >>>> of the C906 emulation.
> >>>>
> >>>> This patch attempts to address this issue by testing the MAEE bit
> >>>> in TH_MXSTATUS CSR. As the TH_MXSTATUS CSR is only accessible in M-mode
> >>>> this patch depends on M-mode firmware that handles this for us
> >>>> transparently.
> >>>>
> >>>> As this patch breaks Linux bootup on all C9xx machines with MAEE,
> >>>> which don't have M-mode firmware that handles the access to the
> >>>> TH_MXSTATUS CSR, this patch is marked as RFC.
> >> Can we wrap the csr access in a _ASM_EXTABLE()? If firmware handles it,
> >> then we return true/false based on the value. If firmware doesn't handle
> >> it, and we get an illegal instruction exception, then we assume the bit
> >> is set, which is the current behavior.
> >>
> >>> I think this is gonna be unacceptable in its current state given that it
> >>> causes problems for every other version of the firmware. Breaking real
> >>> systems for the sake of emulation isn't something we can reasonably do.
> >>>
> >>> To make this sort of change acceptable, you're gonna have to add some way
> >>> to differentiate between systems that do and do not support reading this
> >>> CSR. I think we either a) need to check the version of the SBI
> >>> implementation to see if it hits the threshold for supporting this
> >>> feature, or b) add a specific SBI call for this so that we can
> >>> differentiate between firmware not supporting the function and the
> >> The FWFT SBI extension is being developed as a mechanism for S-mode to ask
> >> M-mode things like this, but I think that extension should be used for
> >> features that have potential to be changed by S-mode (even if not
> >> everything will be changeable on all platforms), whereas anything that's
> >> read-only would be better with...
> >>
> >>> quote-unquote "hardware" not supporting it. I don't really like option a)
> >>> as it could grow to several different options (each for a different SBI
> >>> implementation) and support for reading the CSR would need to be
> >>> unconditional. I have a feeling that I am missing something though,
> >>> that'd make it doable without introducing a new call.
> >>>
> >>> Thanks,
> >>> Conor.
> >>>
> >>> If only we'd made enabling this be controlled by a specific DT property,
> >>> then disabling it in QEMU would be as simple as not setting that
> >>> property :(
> >> ...this, where "DT property" is "ISA extension name". I wonder if we
> >> shouldn't start considering the invention of xlinux_vendor_xyz type
> >> extension names which firmware could add to the ISA string / array,
> >> in order to communicate read-only information like this?
> >>
> >> Thanks,
> >> drew
> > Hi Conor and Drew,
> >
> > Thank you for your hints.
> > I fully agree with all your statements and concerns.
> >
> > Switching from th.mxstatus to th.sxstatus should address all mentioned concerns:
> > * no dependency on OpenSBI changes
> > * no break of functionality
> > * no need for graceful handling of CSR read failures
> > * no need to differentiate between HW and emulation (assuming QEMU
> > accepts the emulation of th.sxstatus)
> >
> > Also note that DT handling would be difficult, because we need to probe before
> > setting up the page table.
>
>
> We already parse the DT before setting the page table to disable KASLR
> and to parse "no4lvl" or "no5lvl" command line parameters. Take a look
> at the kernel/pi directory and setup_vm() in mm/init.c.

Ah, I see. So, this can be done with a function similar to
get_kaslr_seed() in arch/riscv/kernel/pi/fdt_early.c.
And the Makefile will apply the necessary steps to get this working.
The downside is that depending on new information in the DT, it will not be
backward compatible. So, I don't see a way around probing th.sxstatus.MAEE.

Independent of that, there is work to be done for the T-Head extension
discovery in the Linux kernel:
* XThead* extensions are not in the DTS
* XThead* extensions are not parsed during bootup
* XThead* extensions don't trigger optimizations (string ops) or errata (MAEE)
* XThead* extensions are not exported via hwprobe
However, I think this is independent of addressing the MAEE issue.
So, I will send out a V2 with the th.sxstatus.MAEE probing only.

Thanks,
Christoph

>
> Thanks,
>
> Alex
>
>
> >
> > Thanks!
> >
> >
> >>>> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> >>>> ---
> >>>>   arch/riscv/errata/thead/errata.c | 14 ++++++++++----
> >>>>   1 file changed, 10 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> >>>> index 8c8a8a4b0421..dd7bf6c62a35 100644
> >>>> --- a/arch/riscv/errata/thead/errata.c
> >>>> +++ b/arch/riscv/errata/thead/errata.c
> >>>> @@ -19,6 +19,9 @@
> >>>>   #include <asm/patch.h>
> >>>>   #include <asm/vendorid_list.h>
> >>>>
> >>>> +#define CSR_TH_MXSTATUS            0x7c0
> >>>> +#define MXSTATUS_MAEE              _AC(0x200000, UL)
> >>>> +
> >>>>   static bool errata_probe_maee(unsigned int stage,
> >>>>                            unsigned long arch_id, unsigned long impid)
> >>>>   {
> >>>> @@ -28,11 +31,14 @@ static bool errata_probe_maee(unsigned int stage,
> >>>>      if (arch_id != 0 || impid != 0)
> >>>>              return false;
> >>>>
> >>>> -   if (stage == RISCV_ALTERNATIVES_EARLY_BOOT ||
> >>>> -       stage == RISCV_ALTERNATIVES_MODULE)
> >>>> -           return true;
> >>>> +   if (stage != RISCV_ALTERNATIVES_EARLY_BOOT &&
> >>>> +       stage != RISCV_ALTERNATIVES_MODULE)
> >>>> +           return false;
> >>>>
> >>>> -   return false;
> >>>> +   if (!(csr_read(CSR_TH_MXSTATUS) & MXSTATUS_MAEE))
> >>>> +           return false;
> >>>> +
> >>>> +   return true;
> >>>>   }
> >>>>
> >>>>   /*
> >>>> --
> >>>> 2.44.0
> >>>>
> >>
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling MAEE errata

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 5880 bytes --]

On Fri, Mar 29, 2024 at 12:22:51PM +0100, Christoph Müllner wrote:
> On Thu, Mar 28, 2024 at 4:43 PM Alexandre Ghiti <alex@ghiti.fr> wrote:
> >
> > Hi Christoph,
> >
> > On 28/03/2024 15:18, Christoph Müllner wrote:
> > > On Wed, Mar 27, 2024 at 1:41 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> > >> On Wed, Mar 27, 2024 at 11:03:06AM +0000, Conor Dooley wrote:
> > >>> On Wed, Mar 27, 2024 at 11:31:30AM +0100, Christoph Müllner wrote:
> > >>>> T-Head's MAEE mechanism (non-compatible equivalent of RVI's Svpbmt)
> > >>>> is currently assumed for all T-Head harts. However, QEMU recently
> > >>>> decided to drop acceptance of guests that write reserved bits in PTEs.
> > >>>> As MAEE uses reserved bits in PTEs and Linux applies the MAEE errata
> > >>>> for all T-Head harts, this broke the Linux startup on QEMU emulations
> > >>>> of the C906 emulation.
> > >>>>
> > >>>> This patch attempts to address this issue by testing the MAEE bit
> > >>>> in TH_MXSTATUS CSR. As the TH_MXSTATUS CSR is only accessible in M-mode
> > >>>> this patch depends on M-mode firmware that handles this for us
> > >>>> transparently.
> > >>>>
> > >>>> As this patch breaks Linux bootup on all C9xx machines with MAEE,
> > >>>> which don't have M-mode firmware that handles the access to the
> > >>>> TH_MXSTATUS CSR, this patch is marked as RFC.
> > >> Can we wrap the csr access in a _ASM_EXTABLE()? If firmware handles it,
> > >> then we return true/false based on the value. If firmware doesn't handle
> > >> it, and we get an illegal instruction exception, then we assume the bit
> > >> is set, which is the current behavior.
> > >>
> > >>> I think this is gonna be unacceptable in its current state given that it
> > >>> causes problems for every other version of the firmware. Breaking real
> > >>> systems for the sake of emulation isn't something we can reasonably do.
> > >>>
> > >>> To make this sort of change acceptable, you're gonna have to add some way
> > >>> to differentiate between systems that do and do not support reading this
> > >>> CSR. I think we either a) need to check the version of the SBI
> > >>> implementation to see if it hits the threshold for supporting this
> > >>> feature, or b) add a specific SBI call for this so that we can
> > >>> differentiate between firmware not supporting the function and the
> > >> The FWFT SBI extension is being developed as a mechanism for S-mode to ask
> > >> M-mode things like this, but I think that extension should be used for
> > >> features that have potential to be changed by S-mode (even if not
> > >> everything will be changeable on all platforms), whereas anything that's
> > >> read-only would be better with...
> > >>
> > >>> quote-unquote "hardware" not supporting it. I don't really like option a)
> > >>> as it could grow to several different options (each for a different SBI
> > >>> implementation) and support for reading the CSR would need to be
> > >>> unconditional. I have a feeling that I am missing something though,
> > >>> that'd make it doable without introducing a new call.
> > >>>
> > >>> Thanks,
> > >>> Conor.
> > >>>
> > >>> If only we'd made enabling this be controlled by a specific DT property,
> > >>> then disabling it in QEMU would be as simple as not setting that
> > >>> property :(
> > >> ...this, where "DT property" is "ISA extension name". I wonder if we
> > >> shouldn't start considering the invention of xlinux_vendor_xyz type
> > >> extension names which firmware could add to the ISA string / array,
> > >> in order to communicate read-only information like this?
> > >>
> > >> Thanks,
> > >> drew
> > > Hi Conor and Drew,
> > >
> > > Thank you for your hints.
> > > I fully agree with all your statements and concerns.
> > >
> > > Switching from th.mxstatus to th.sxstatus should address all mentioned concerns:
> > > * no dependency on OpenSBI changes
> > > * no break of functionality
> > > * no need for graceful handling of CSR read failures
> > > * no need to differentiate between HW and emulation (assuming QEMU
> > > accepts the emulation of th.sxstatus)
> > >
> > > Also note that DT handling would be difficult, because we need to probe before
> > > setting up the page table.
> >
> >
> > We already parse the DT before setting the page table to disable KASLR
> > and to parse "no4lvl" or "no5lvl" command line parameters. Take a look
> > at the kernel/pi directory and setup_vm() in mm/init.c.
> 
> Ah, I see. So, this can be done with a function similar to
> get_kaslr_seed() in arch/riscv/kernel/pi/fdt_early.c.
> And the Makefile will apply the necessary steps to get this working.
> The downside is that depending on new information in the DT, it will not be
> backward compatible. So, I don't see a way around probing th.sxstatus.MAEE.

Aye, you're right here. My suggestion about using DT was only for if
there were T-Head CPUs that turned up in the future with non-zero values
for marchid or mimpid. Requiring it for the CPUs we're talking about at
the moment (e.g. c906) would, as you pointed out yourself, cause the same
sort of regression that relying on an updated firmware would.
> Independent of that, there is work to be done for the T-Head extension
> discovery in the Linux kernel:
> * XThead* extensions are not in the DTS
> * XThead* extensions are not parsed during bootup
> * XThead* extensions don't trigger optimizations (string ops) or errata (MAEE)
> * XThead* extensions are not exported via hwprobe

> However, I think this is independent of addressing the MAEE issue.
> So, I will send out a V2 with the th.sxstatus.MAEE probing only.

th.sxstatus seems perfect here since it solves the problem you're
looking to deal with in emulation while being not introducing
regressions for real devices.

Thanks,
Conor.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 0/2] RISC-V: Test th.mxstatus.MAEE bit before enabling MAEE

[Qingfang Deng]

Hi Christoph,

You don't need the access to MXSTATUS. There is SXSTATUS which is a
read-only mirror of MXSTATUS in S-mode.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 0/2] RISC-V: Test th.mxstatus.MAEE bit before enabling MAEE

[Christoph Müllner]

On Wed, Mar 27, 2024 at 2:00 PM Qingfang Deng <dqfext@gmail.com> wrote:
>
> Hi Christoph,
>
> You don't need the access to MXSTATUS. There is SXSTATUS which is a
> read-only mirror of MXSTATUS in S-mode.

Thanks for the hint!
I'll change the patchsets accordingly (and drop the OpenSBI patchset).

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv