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

[Christoph Müllner]

On Mon, Apr 8, 2024 at 3:58 AM Yangyu Chen <cyy@xxxxxxxxxxxx> 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 <conor.dooley@xxxxxxxxxxxxx>
> > Signed-off-by: Christoph Müllner <christoph.muellner@xxxxxxxx>
> > ---
> >   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/tencent_D1180541B4B31C0371DB634A42681A5BF809@xxxxxx/
>
> 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;
> >   }
> >
> >   /*
>