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 XTheadMae 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 "MAE"/"XTheadMae" and only enables
it if the th.sxstatus.MAEE bit is set.
The th.sxstatus CSR is documented here:
https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadsxstatus.adoc
XTheadMae is documented here:
https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadmae.adoc
The QEMU patch to emulate th.sxstatus with the MAEE bit not set is here:
https://lore.kernel.org/all/[email protected]/
After applying the referenced QEMU patch, this patchset allows to
successfully boot a C906 QEMU system emulation ("-cpu thead-c906").
Changes in v3:
* Rename to MAE instead of MAEE
Changes in v2:
* use th.sxstatus.MAEE instead of th.mxstatus.MAEE
Christoph Müllner (2):
riscv: thead: Rename T-Head PBMT to MAE
riscv: T-Head: Test availability bit before enabling MAE errata
arch/riscv/Kconfig.errata | 8 ++++----
arch/riscv/errata/thead/errata.c | 24 +++++++++++++++---------
arch/riscv/include/asm/errata_list.h | 20 ++++++++++----------
3 files changed, 29 insertions(+), 23 deletions(-)
--
2.44.0
T-Head's vendor extension to set page attributes has the name
MAE (memory attribute extension).
Let's rename it, so it is clear what this referes to.
Link: https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadmae.adoc
Reviewed-by: Conor Dooley <[email protected]>
Signed-off-by: Christoph Müllner <[email protected]>
---
arch/riscv/Kconfig.errata | 8 ++++----
arch/riscv/errata/thead/errata.c | 10 +++++-----
arch/riscv/include/asm/errata_list.h | 20 ++++++++++----------
3 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata
index 910ba8837add..2acc7d876e1f 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_MAE
+ bool "Apply T-Head's memory attribute extension (XTheadMae) errata"
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 attribute extension errata to handle the
+ non-standard PTE utilization on T-Head SoCs (XTheadMae).
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..6e7ee1f16bee 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,
- unsigned long arch_id, unsigned long impid)
+static bool errata_probe_mae(unsigned int stage,
+ unsigned long arch_id, unsigned long impid)
{
- if (!IS_ENABLED(CONFIG_ERRATA_THEAD_PBMT))
+ if (!IS_ENABLED(CONFIG_ERRATA_THEAD_MAE))
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_mae(stage, archid, impid))
+ cpu_req_errata |= BIT(ERRATA_THEAD_MAE);
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 1f2dbfb8a8bf..efd851e1b483 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_MAE 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_MAE_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_MAE, CONFIG_ERRATA_THEAD_MAE) \
: "=r"(_val) \
: "I"(prot##_SVPBMT >> ALT_SVPBMT_SHIFT), \
- "I"(prot##_THEAD >> ALT_THEAD_PBMT_SHIFT), \
+ "I"(prot##_THEAD >> ALT_THEAD_MAE_SHIFT), \
"I"(ALT_SVPBMT_SHIFT), \
- "I"(ALT_THEAD_PBMT_SHIFT))
+ "I"(ALT_THEAD_MAE_SHIFT))
-#ifdef CONFIG_ERRATA_THEAD_PBMT
+#ifdef CONFIG_ERRATA_THEAD_MAE
/*
* 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_MAE, CONFIG_ERRATA_THEAD_MAE) \
: "+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_MAE_SHIFT), \
+ "I"(_PAGE_PMA_THEAD >> ALT_THEAD_MAE_SHIFT), \
+ "I"(ALT_THEAD_MAE_SHIFT) \
: "t3")
#else
#define ALT_THEAD_PMA(_val)
--
2.44.0
T-Head's memory attribute extension (XTheadMae) (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 XTheadMae uses reserved bits in PTEs and Linux applies the MAE 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 MAE-enable bit
in the th.sxstatus CSR. This CSR is available in HW and can be
emulated in QEMU.
This patch also makes the XTheadMae probing mechanism reliable, because
a test for the right combination of mvendorid, marchid, and mimpid
is not sufficient to enable MAE.
Reviewed-by: Conor Dooley <[email protected]>
Signed-off-by: Christoph Müllner <[email protected]>
---
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 6e7ee1f16bee..bf6a0a6318ee 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_SXSTATUS 0x5c0
+#define SXSTATUS_MAEE _AC(0x200000, UL)
+
static bool errata_probe_mae(unsigned int stage,
unsigned long arch_id, unsigned long impid)
{
@@ -28,11 +31,14 @@ static bool errata_probe_mae(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_SXSTATUS) & SXSTATUS_MAEE))
+ return false;
+
+ return true;
}
/*
--
2.44.0
On 2024/4/8 05:32, Christoph Müllner wrote:
> T-Head's memory attribute extension (XTheadMae) (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 XTheadMae uses reserved bits in PTEs and Linux applies the MAE 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 MAE-enable bit
> in the th.sxstatus CSR. This CSR is available in HW and can be
> emulated in QEMU.
>
> This patch also makes the XTheadMae probing mechanism reliable, because
> a test for the right combination of mvendorid, marchid, and mimpid
> is not sufficient to enable MAE.
>
> Reviewed-by: Conor Dooley <[email protected]>
> Signed-off-by: Christoph Müllner <[email protected]>
> ---
> 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 6e7ee1f16bee..bf6a0a6318ee 100644
> --- a/arch/riscv/errata/thead/errata.c
> +++ b/arch/riscv/errata/thead/errata.c
> @@ -19,6 +19,9 @@
> #include <asm/patch.h>f
> #include <asm/vendorid_list.h>
>
> +#define CSR_TH_SXSTATUS 0x5c0
> +#define SXSTATUS_MAEE _AC(0x200000, UL)
> +
> static bool errata_probe_mae(unsigned int stage,
> unsigned long arch_id, unsigned long impid)
> {
> @@ -28,11 +31,14 @@ static bool errata_probe_mae(unsigned int stage,
> if (arch_id != 0 || impid != 0)
> return false;
>
I would raise a little concern about keeping this "if" statement for
arch_id and imp_id after we have probed it in this CSR way. I would like to
remove it and move the CSR probe earlier than RISCV_ALTERNATIVES.
I added CC to guoren for more opinions.
Even T-Head C908 comes in 2023, which supports RVV 1.0 and also keeps MAEE.
But it also supports Svpbmt, and we can perform the switch by clearing the
MAEE bit in CSR_TH_MXSTATUS in M-Mode Software.
Moreover, T-Head MAEE may not be removed in the future of T-Head CPUs. We
can see something from the T-Head C908 User Manual that adds a Security bit
to MAEE. So, it might used in their own TEE implementation and will not be
removed.
However, C908 has arch_id and impid, which are not equal to zero. You can
see it from the C908 boot log [2] from my patch to support K230 [3]. So, if
we have probed MAEE using CSR, you confirmed that this bit will also be set
in the C906 core. We can only probe it this way and no longer use arch_id
and imp_id. And if the arch_id and imp_id probes get removed, we should
also move the csr probe earlier than riscv alternatives.
[1] https://occ-intl-prod.oss-ap-southeast-1.aliyuncs.com/resource//1699268369347/XuanTie-C908-UserManual.pdf
[2] https://gist.github.com/cyyself/b9445f38cc3ba1094924bd41c9086176
[3] https://lore.kernel.org/linux-riscv/[email protected]/
Thanks,
Yangyu Chen
> - 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_SXSTATUS) & SXSTATUS_MAEE))
> + return false;
> +
> + return true;
> }
>
> /*
On Mon, Apr 8, 2024 at 3:58 AM Yangyu Chen <[email protected]> wrote:
>
> On 2024/4/8 05:32, Christoph Müllner wrote:
> > T-Head's memory attribute extension (XTheadMae) (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 XTheadMae uses reserved bits in PTEs and Linux applies the MAE 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 MAE-enable bit
> > in the th.sxstatus CSR. This CSR is available in HW and can be
> > emulated in QEMU.
> >
> > This patch also makes the XTheadMae probing mechanism reliable, because
> > a test for the right combination of mvendorid, marchid, and mimpid
> > is not sufficient to enable MAE.
> >
> > Reviewed-by: Conor Dooley <[email protected]>
> > Signed-off-by: Christoph Müllner <[email protected]>
> > ---
> > 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 6e7ee1f16bee..bf6a0a6318ee 100644
> > --- a/arch/riscv/errata/thead/errata.c
> > +++ b/arch/riscv/errata/thead/errata.c
> > @@ -19,6 +19,9 @@
> > #include <asm/patch.h>f
> > #include <asm/vendorid_list.h>
> >
> > +#define CSR_TH_SXSTATUS 0x5c0
> > +#define SXSTATUS_MAEE _AC(0x200000, UL)
> > +
> > static bool errata_probe_mae(unsigned int stage,
> > unsigned long arch_id, unsigned long impid)
> > {
> > @@ -28,11 +31,14 @@ static bool errata_probe_mae(unsigned int stage,
> > if (arch_id != 0 || impid != 0)
> > return false;
> >
>
> I would raise a little concern about keeping this "if" statement for
> arch_id and imp_id after we have probed it in this CSR way. I would like to
> remove it and move the CSR probe earlier than RISCV_ALTERNATIVES.
>
> I added CC to guoren for more opinions.
>
> Even T-Head C908 comes in 2023, which supports RVV 1.0 and also keeps MAEE.
> But it also supports Svpbmt, and we can perform the switch by clearing the
> MAEE bit in CSR_TH_MXSTATUS in M-Mode Software.
>
> Moreover, T-Head MAEE may not be removed in the future of T-Head CPUs. We
> can see something from the T-Head C908 User Manual that adds a Security bit
> to MAEE. So, it might used in their own TEE implementation and will not be
> removed.
>
> However, C908 has arch_id and impid, which are not equal to zero. You can
> see it from the C908 boot log [2] from my patch to support K230 [3]. So, if
> we have probed MAEE using CSR, you confirmed that this bit will also be set
> in the C906 core. We can only probe it this way and no longer use arch_id
> and imp_id. And if the arch_id and imp_id probes get removed, we should
> also move the csr probe earlier than riscv alternatives.
We keep the probing via arch_id==0&&impid==0 because we had that
already in the kernel and don't want to break existing functionality.
From the discussions that we had about the v1 and v2 of this series,
my impression is that we should use DT properties instead of probing
arch_id and impid. So, if C908 support is needed, this should probably
be introduced using DT properties. The logic would then be:
* if arch_id == 0 and impid == 0 then decide based on th.sxstatus.MAEE
* else test if "xtheadmae" is in the DT
>
> [1] https://occ-intl-prod.oss-ap-southeast-1.aliyuncs.com/resource//1699268369347/XuanTie-C908-UserManual.pdf
> [2] https://gist.github.com/cyyself/b9445f38cc3ba1094924bd41c9086176
> [3] https://lore.kernel.org/linux-riscv/[email protected]/
>
> Thanks,
> Yangyu Chen
>
> > - 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_SXSTATUS) & SXSTATUS_MAEE))
> > + return false;
> > +
> > + return true;
> > }
> >
> > /*
>
On Mon, Apr 8, 2024 at 9:37 AM Yangyu Chen <[email protected]> wrote:
>
>
>
> > On Apr 8, 2024, at 14:00, Christoph Müllner <[email protected]> wrote:
> >
> > On Mon, Apr 8, 2024 at 3:58 AM Yangyu Chen <[email protected]> wrote:
> >>
> >> On 2024/4/8 05:32, Christoph Müllner wrote:
> >>> T-Head's memory attribute extension (XTheadMae) (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 XTheadMae uses reserved bits in PTEs and Linux applies the MAE 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 MAE-enable bit
> >>> in the th.sxstatus CSR. This CSR is available in HW and can be
> >>> emulated in QEMU.
> >>>
> >>> This patch also makes the XTheadMae probing mechanism reliable, because
> >>> a test for the right combination of mvendorid, marchid, and mimpid
> >>> is not sufficient to enable MAE.
> >>>
> >>> Reviewed-by: Conor Dooley <[email protected]>
> >>> Signed-off-by: Christoph Müllner <[email protected]>
> >>> ---
> >>> 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 6e7ee1f16bee..bf6a0a6318ee 100644
> >>> --- a/arch/riscv/errata/thead/errata.c
> >>> +++ b/arch/riscv/errata/thead/errata.c
> >>> @@ -19,6 +19,9 @@
> >>> #include <asm/patch.h>f
> >>> #include <asm/vendorid_list.h>
> >>>
> >>> +#define CSR_TH_SXSTATUS 0x5c0
> >>> +#define SXSTATUS_MAEE _AC(0x200000, UL)
> >>> +
> >>> static bool errata_probe_mae(unsigned int stage,
> >>> unsigned long arch_id, unsigned long impid)
> >>> {
> >>> @@ -28,11 +31,14 @@ static bool errata_probe_mae(unsigned int stage,
> >>> if (arch_id != 0 || impid != 0)
> >>> return false;
> >>>
> >>
> >> I would raise a little concern about keeping this "if" statement for
> >> arch_id and imp_id after we have probed it in this CSR way. I would like to
> >> remove it and move the CSR probe earlier than RISCV_ALTERNATIVES.
> >>
> >> I added CC to guoren for more opinions.
> >>
> >> Even T-Head C908 comes in 2023, which supports RVV 1.0 and also keeps MAEE.
> >> But it also supports Svpbmt, and we can perform the switch by clearing the
> >> MAEE bit in CSR_TH_MXSTATUS in M-Mode Software.
> >>
> >> Moreover, T-Head MAEE may not be removed in the future of T-Head CPUs. We
> >> can see something from the T-Head C908 User Manual that adds a Security bit
> >> to MAEE. So, it might used in their own TEE implementation and will not be
> >> removed.
> >>
> >> However, C908 has arch_id and impid, which are not equal to zero. You can
> >> see it from the C908 boot log [2] from my patch to support K230 [3]. So, if
> >> we have probed MAEE using CSR, you confirmed that this bit will also be set
> >> in the C906 core. We can only probe it this way and no longer use arch_id
> >> and imp_id. And if the arch_id and imp_id probes get removed, we should
> >> also move the csr probe earlier than riscv alternatives.
> >
> > We keep the probing via arch_id==0&&impid==0 because we had that
> > already in the kernel and don't want to break existing functionality.
> >
> > From the discussions that we had about the v1 and v2 of this series,
> > my impression is that we should use DT properties instead of probing
> > arch_id and impid. So, if C908 support is needed, this should probably
> > be introduced using DT properties. The logic would then be:
> > * if arch_id == 0 and impid == 0 then decide based on th.sxstatus.MAEE
> > * else test if "xtheadmae" is in the DT
> >
> >
>
> I know about it, and Conor also mentioned adding this property to DT a few
> months ago. I agree with this at that time. But for now, you have found the
> T-Head document description about this, and we can probe MAE using CSR even
> on C906. I think only probing in CSR will be a better way to do that. It
> can maintain compatibility with some early cores, such as C906. And will
> also support some new cores with non-zero arch_id and impl_id but may have
> MAE enabled, such as C908.
>
> For future concerns, T-Head said from their documentation that
> "Availability: The th.sxstatus CSR is available on all systems whose
> mvendorid CSR holds a value of 0x5B7." [1] and this extension is frozen and
> stable. I think it's okay to have MAE errara for some cpus whose impl_id
> and arch_id are non-zero. And T-Head may have used this for their TEE, so
> it might never be removed.
I wrote that specification. And yes, T-Head reviewed that part.
But there is no guarantee for future cores.
The question is: why should the kernel have to care about that?
This can all be addressed (hidden) in FW, where core-specific
routines can test the required bits in vendor CSRs and set DT properties
that match the core's configuration.
> Since it might never be removed, if some vendor uses it and makes it hard
> to run the mainline kernel since it requires setting CSR in M-Mode software
> or changing the DT, they may be hard to change for some security reasons
> for TEE, I think it's not right.
>
> I'm also waiting for Conor's opinion about this.
>
> [1] https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadsxstatus.adoc
>
> Thanks,
> Yangyu Chen
>
> >
> >
> >>
> >> [1] https://occ-intl-prod.oss-ap-southeast-1.aliyuncs.com/resource//1699268369347/XuanTie-C908-UserManual.pdf
> >> [2] https://gist.github.com/cyyself/b9445f38cc3ba1094924bd41c9086176
> >> [3] https://lore.kernel.org/linux-riscv/[email protected]/
> >>
> >> Thanks,
> >> Yangyu Chen
> >>
> >>> - 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_SXSTATUS) & SXSTATUS_MAEE))
> >>> + return false;
> >>> +
> >>> + return true;
> >>> }
> >>>
> >>> /*
> >>
>
On Mon, Apr 08, 2024 at 09:55:48AM +0200, Christoph Müllner wrote:
> On Mon, Apr 8, 2024 at 9:37 AM Yangyu Chen <[email protected]> wrote:
> > > On Apr 8, 2024, at 14:00, Christoph Müllner <[email protected]> wrote:
> > > On Mon, Apr 8, 2024 at 3:58 AM Yangyu Chen <[email protected]> wrote:
> > >> On 2024/4/8 05:32, Christoph Müllner wrote:
> > >>> T-Head's memory attribute extension (XTheadMae) (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 XTheadMae uses reserved bits in PTEs and Linux applies the MAE 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 MAE-enable bit
> > >>> in the th.sxstatus CSR. This CSR is available in HW and can be
> > >>> emulated in QEMU.
> > >>>
> > >>> This patch also makes the XTheadMae probing mechanism reliable, because
> > >>> a test for the right combination of mvendorid, marchid, and mimpid
> > >>> is not sufficient to enable MAE.
> > >>>
> > >>> Reviewed-by: Conor Dooley <[email protected]>
> > >>> Signed-off-by: Christoph Müllner <[email protected]>
> > >>> @@ -28,11 +31,14 @@ static bool errata_probe_mae(unsigned int stage,
> > >>> if (arch_id != 0 || impid != 0)
> > >>> return false;
> > >>>
> > >>
> > >> I would raise a little concern about keeping this "if" statement for
> > >> arch_id and imp_id after we have probed it in this CSR way. I would like to
> > >> remove it and move the CSR probe earlier than RISCV_ALTERNATIVES.
> > >>
> > >> I added CC to guoren for more opinions.
> > >>
> > >> Even T-Head C908 comes in 2023, which supports RVV 1.0 and also keeps MAEE.
> > >> But it also supports Svpbmt, and we can perform the switch by clearing the
> > >> MAEE bit in CSR_TH_MXSTATUS in M-Mode Software.
> > >>
> > >> Moreover, T-Head MAEE may not be removed in the future of T-Head CPUs. We
> > >> can see something from the T-Head C908 User Manual that adds a Security bit
> > >> to MAEE. So, it might used in their own TEE implementation and will not be
> > >> removed.
> > >>
> > >> However, C908 has arch_id and impid, which are not equal to zero. You can
> > >> see it from the C908 boot log [2] from my patch to support K230 [3]. So, if
> > >> we have probed MAEE using CSR, you confirmed that this bit will also be set
> > >> in the C906 core. We can only probe it this way and no longer use arch_id
> > >> and imp_id. And if the arch_id and imp_id probes get removed, we should
> > >> also move the csr probe earlier than riscv alternatives.
> > >
> > > We keep the probing via arch_id==0&&impid==0 because we had that
> > > already in the kernel and don't want to break existing functionality.
> > >
> > > From the discussions that we had about the v1 and v2 of this series,
> > > my impression is that we should use DT properties instead of probing
> > > arch_id and impid. So, if C908 support is needed, this should probably
> > > be introduced using DT properties. The logic would then be:
> > > * if arch_id == 0 and impid == 0 then decide based on th.sxstatus.MAEE
> > > * else test if "xtheadmae" is in the DT
> > >
> > >
> >
> > I know about it, and Conor also mentioned adding this property to DT a few
> > months ago. I agree with this at that time. But for now, you have found the
> > T-Head document description about this, and we can probe MAE using CSR even
> > on C906. I think only probing in CSR will be a better way to do that. It
> > can maintain compatibility with some early cores, such as C906. And will
> > also support some new cores with non-zero arch_id and impl_id but may have
> > MAE enabled, such as C908.
> >
> > For future concerns, T-Head said from their documentation that
> > "Availability: The th.sxstatus CSR is available on all systems whose
> > mvendorid CSR holds a value of 0x5B7." [1] and this extension is frozen and
> > stable. I think it's okay to have MAE errara for some cpus whose impl_id
> > and arch_id are non-zero. And T-Head may have used this for their TEE, so
> > it might never be removed.
>
> I wrote that specification. And yes, T-Head reviewed that part.
> But there is no guarantee for future cores.
Yeah, that is what I assumed. Unless its a 100% certainty that this bit
will always have this meaning, we can't unconditionally assume that it
does.
> > Since it might never be removed, if some vendor uses it and makes it hard
> > to run the mainline kernel since it requires setting CSR in M-Mode software
> > or changing the DT, they may be hard to change for some security reasons
> > for TEE, I think it's not right
> The question is: why should the kernel have to care about that?
> This can all be addressed (hidden) in FW, where core-specific
> routines can test the required bits in vendor CSRs and set DT properties
> that match the core's configuration.
I'm also not super inclined to care about it requiring changes in
firmware, because the last time I checked T-Head's SDK (and therefore
the vendors') use a version of OpenSBI that cannot even run mainline and
needs to be updated to begin with.
> On Apr 8, 2024, at 16:10, Conor Dooley <[email protected]> wrote:
>
> On Mon, Apr 08, 2024 at 09:55:48AM +0200, Christoph Müllner wrote:
>> On Mon, Apr 8, 2024 at 9:37 AM Yangyu Chen <[email protected]> wrote:
>>>> On Apr 8, 2024, at 14:00, Christoph Müllner <[email protected]> wrote:
>>>> On Mon, Apr 8, 2024 at 3:58 AM Yangyu Chen <[email protected]> wrote:
>>>>> On 2024/4/8 05:32, Christoph Müllner wrote:
>>>>>> T-Head's memory attribute extension (XTheadMae) (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 XTheadMae uses reserved bits in PTEs and Linux applies the MAE 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 MAE-enable bit
>>>>>> in the th.sxstatus CSR. This CSR is available in HW and can be
>>>>>> emulated in QEMU.
>>>>>>
>>>>>> This patch also makes the XTheadMae probing mechanism reliable, because
>>>>>> a test for the right combination of mvendorid, marchid, and mimpid
>>>>>> is not sufficient to enable MAE.
>>>>>>
>>>>>> Reviewed-by: Conor Dooley <[email protected]>
>>>>>> Signed-off-by: Christoph Müllner <[email protected]>
>
>>>>>> @@ -28,11 +31,14 @@ static bool errata_probe_mae(unsigned int stage,
>>>>>> if (arch_id != 0 || impid != 0)
>>>>>> return false;
>>>>>>
>>>>>
>>>>> I would raise a little concern about keeping this "if" statement for
>>>>> arch_id and imp_id after we have probed it in this CSR way. I would like to
>>>>> remove it and move the CSR probe earlier than RISCV_ALTERNATIVES.
>>>>>
>>>>> I added CC to guoren for more opinions.
>>>>>
>>>>> Even T-Head C908 comes in 2023, which supports RVV 1.0 and also keeps MAEE.
>>>>> But it also supports Svpbmt, and we can perform the switch by clearing the
>>>>> MAEE bit in CSR_TH_MXSTATUS in M-Mode Software.
>>>>>
>>>>> Moreover, T-Head MAEE may not be removed in the future of T-Head CPUs. We
>>>>> can see something from the T-Head C908 User Manual that adds a Security bit
>>>>> to MAEE. So, it might used in their own TEE implementation and will not be
>>>>> removed.
>>>>>
>>>>> However, C908 has arch_id and impid, which are not equal to zero. You can
>>>>> see it from the C908 boot log [2] from my patch to support K230 [3]. So, if
>>>>> we have probed MAEE using CSR, you confirmed that this bit will also be set
>>>>> in the C906 core. We can only probe it this way and no longer use arch_id
>>>>> and imp_id. And if the arch_id and imp_id probes get removed, we should
>>>>> also move the csr probe earlier than riscv alternatives.
>>>>
>>>> We keep the probing via arch_id==0&&impid==0 because we had that
>>>> already in the kernel and don't want to break existing functionality.
>>>>
>>>> From the discussions that we had about the v1 and v2 of this series,
>>>> my impression is that we should use DT properties instead of probing
>>>> arch_id and impid. So, if C908 support is needed, this should probably
>>>> be introduced using DT properties. The logic would then be:
>>>> * if arch_id == 0 and impid == 0 then decide based on th.sxstatus.MAEE
>>>> * else test if "xtheadmae" is in the DT
>>>>
>>>>
>>>
>>> I know about it, and Conor also mentioned adding this property to DT a few
>>> months ago. I agree with this at that time. But for now, you have found the
>>> T-Head document description about this, and we can probe MAE using CSR even
>>> on C906. I think only probing in CSR will be a better way to do that. It
>>> can maintain compatibility with some early cores, such as C906. And will
>>> also support some new cores with non-zero arch_id and impl_id but may have
>>> MAE enabled, such as C908.
>>>
>>> For future concerns, T-Head said from their documentation that
>>> "Availability: The th.sxstatus CSR is available on all systems whose
>>> mvendorid CSR holds a value of 0x5B7." [1] and this extension is frozen and
>>> stable. I think it's okay to have MAE errara for some cpus whose impl_id
>>> and arch_id are non-zero. And T-Head may have used this for their TEE, so
>>> it might never be removed.
>>
>> I wrote that specification. And yes, T-Head reviewed that part.
>> But there is no guarantee for future cores.
>
> Yeah, that is what I assumed. Unless its a 100% certainty that this bit
> will always have this meaning, we can't unconditionally assume that it
> does.
>
>>> Since it might never be removed, if some vendor uses it and makes it hard
>>> to run the mainline kernel since it requires setting CSR in M-Mode software
>>> or changing the DT, they may be hard to change for some security reasons
>>> for TEE, I think it's not right
>
>> The question is: why should the kernel have to care about that?
>> This can all be addressed (hidden) in FW, where core-specific
>> routines can test the required bits in vendor CSRs and set DT properties
>> that match the core's configuration.
>
> I'm also not super inclined to care about it requiring changes in
> firmware, because the last time I checked T-Head's SDK (and therefore
> the vendors') use a version of OpenSBI that cannot even run mainline and
> needs to be updated to begin with.
So the solution might be to have some property like `xtheadmae` and test
th.sxstatus whether it has MAEE bit set when we have this in ISA string in
the DT rather than have MAE enabled for sure if `xtheadmae` exists as
discussed before. This will require changing the DT. However, since the
C908, the first core released by T-Head that supports MAE with non-zero
arch_id and imp_id hasn't merged to mainline yet. It's time to add this
dt-binding and some code to probe it. I can have it tested on K230
recently. Whatever, this patch can go first.
Thanks,
Yangyu Chen
On Mon, Apr 08, 2024 at 04:21:24PM +0800, Yangyu Chen wrote:
>
> So the solution might be to have some property like `xtheadmae` and test
> th.sxstatus whether it has MAEE bit set when we have this in ISA string in
> the DT rather than have MAE enabled for sure if `xtheadmae` exists as
> discussed before. This will require changing the DT.
Yeah, I think I don't mind what you propose, as long as we define in the
binding that "xtheadmae" means the hardware supports it ***AND*** the bit
in the CSR. That's easier on firmware and taps into the existing
support in the kernel. I think it's more consistent with how we handle
the standard extensions to do it that way than to assume "xtheadmae"
being present means that it is enabled.
> However, since the
> C908, the first core released by T-Head that supports MAE with non-zero
> arch_id and imp_id hasn't merged to mainline yet. It's time to add this
> dt-binding and some code to probe it. I can have it tested on K230
> recently. Whatever, this patch can go first.
I don't think adding something like this should block merging the
initial c908 support though, can easily be follow-up work. I'll apply
the k230 stuff Wednesday if nothing has come in on it by then.
Cheers,
Conor.
> On Apr 8, 2024, at 14:00, Christoph Müllner <[email protected]> wrote:
>
> On Mon, Apr 8, 2024 at 3:58 AM Yangyu Chen <[email protected]> wrote:
>>
>> On 2024/4/8 05:32, Christoph Müllner wrote:
>>> T-Head's memory attribute extension (XTheadMae) (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 XTheadMae uses reserved bits in PTEs and Linux applies the MAE 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 MAE-enable bit
>>> in the th.sxstatus CSR. This CSR is available in HW and can be
>>> emulated in QEMU.
>>>
>>> This patch also makes the XTheadMae probing mechanism reliable, because
>>> a test for the right combination of mvendorid, marchid, and mimpid
>>> is not sufficient to enable MAE.
>>>
>>> Reviewed-by: Conor Dooley <[email protected]>
>>> Signed-off-by: Christoph Müllner <[email protected]>
>>> ---
>>> 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 6e7ee1f16bee..bf6a0a6318ee 100644
>>> --- a/arch/riscv/errata/thead/errata.c
>>> +++ b/arch/riscv/errata/thead/errata.c
>>> @@ -19,6 +19,9 @@
>>> #include <asm/patch.h>f
>>> #include <asm/vendorid_list.h>
>>>
>>> +#define CSR_TH_SXSTATUS 0x5c0
>>> +#define SXSTATUS_MAEE _AC(0x200000, UL)
>>> +
>>> static bool errata_probe_mae(unsigned int stage,
>>> unsigned long arch_id, unsigned long impid)
>>> {
>>> @@ -28,11 +31,14 @@ static bool errata_probe_mae(unsigned int stage,
>>> if (arch_id != 0 || impid != 0)
>>> return false;
>>>
>>
>> I would raise a little concern about keeping this "if" statement for
>> arch_id and imp_id after we have probed it in this CSR way. I would like to
>> remove it and move the CSR probe earlier than RISCV_ALTERNATIVES.
>>
>> I added CC to guoren for more opinions.
>>
>> Even T-Head C908 comes in 2023, which supports RVV 1.0 and also keeps MAEE.
>> But it also supports Svpbmt, and we can perform the switch by clearing the
>> MAEE bit in CSR_TH_MXSTATUS in M-Mode Software.
>>
>> Moreover, T-Head MAEE may not be removed in the future of T-Head CPUs. We
>> can see something from the T-Head C908 User Manual that adds a Security bit
>> to MAEE. So, it might used in their own TEE implementation and will not be
>> removed.
>>
>> However, C908 has arch_id and impid, which are not equal to zero. You can
>> see it from the C908 boot log [2] from my patch to support K230 [3]. So, if
>> we have probed MAEE using CSR, you confirmed that this bit will also be set
>> in the C906 core. We can only probe it this way and no longer use arch_id
>> and imp_id. And if the arch_id and imp_id probes get removed, we should
>> also move the csr probe earlier than riscv alternatives.
>
> We keep the probing via arch_id==0&&impid==0 because we had that
> already in the kernel and don't want to break existing functionality.
>
> From the discussions that we had about the v1 and v2 of this series,
> my impression is that we should use DT properties instead of probing
> arch_id and impid. So, if C908 support is needed, this should probably
> be introduced using DT properties. The logic would then be:
> * if arch_id == 0 and impid == 0 then decide based on th.sxstatus.MAEE
> * else test if "xtheadmae" is in the DT
>
>
I know about it, and Conor also mentioned adding this property to DT a few
months ago. I agree with this at that time. But for now, you have found the
T-Head document description about this, and we can probe MAE using CSR even
on C906. I think only probing in CSR will be a better way to do that. It
can maintain compatibility with some early cores, such as C906. And will
also support some new cores with non-zero arch_id and impl_id but may have
MAE enabled, such as C908.
For future concerns, T-Head said from their documentation that
"Availability: The th.sxstatus CSR is available on all systems whose
mvendorid CSR holds a value of 0x5B7." [1] and this extension is frozen and
stable. I think it's okay to have MAE errara for some cpus whose impl_id
and arch_id are non-zero. And T-Head may have used this for their TEE, so
it might never be removed.
Since it might never be removed, if some vendor uses it and makes it hard
to run the mainline kernel since it requires setting CSR in M-Mode software
or changing the DT, they may be hard to change for some security reasons
for TEE, I think it's not right.
I'm also waiting for Conor's opinion about this.
[1] https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadsxstatus.adoc
Thanks,
Yangyu Chen
>
>
>>
>> [1] https://occ-intl-prod.oss-ap-southeast-1.aliyuncs.com/resource//1699268369347/XuanTie-C908-UserManual.pdf
>> [2] https://gist.github.com/cyyself/b9445f38cc3ba1094924bd41c9086176
>> [3] https://lore.kernel.org/linux-riscv/[email protected]/
>>
>> Thanks,
>> Yangyu Chen
>>
>>> - 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_SXSTATUS) & SXSTATUS_MAEE))
>>> + return false;
>>> +
>>> + return true;
>>> }
>>>
>>> /*
>>
Hello:
This series was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <[email protected]>:
On Sun, 7 Apr 2024 23:32:34 +0200 you wrote:
> 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 XTheadMae 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
>
> [...]
Here is the summary with links:
- [v3,1/2] riscv: thead: Rename T-Head PBMT to MAE
https://git.kernel.org/riscv/c/6179d4a21300
- [v3,2/2] riscv: T-Head: Test availability bit before enabling MAE errata
https://git.kernel.org/riscv/c/65b71cc35cc6
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html