Received-SPF: pass (google.com: domain of linux-kernel+bounces-134821-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240407213236.2121592-3-christoph.muellner@vrull.eu> <tencent_CF232CDF85208DF6BFA3076E73CC4E087306@qq.com>
In-Reply-To: <tencent_CF232CDF85208DF6BFA3076E73CC4E087306@qq.com>
From: =?UTF-8?Q?Christoph_M=C3=BCllner?= <christoph.muellner@vrull.eu>
Date: Mon, 8 Apr 2024 08:00:12 +0200
Message-ID: <CAEg0e7h0F_LaeMoS6Co1UjgLM6ML8SXtTfHW5+OQS6yedjdZPQ@mail.gmail.com>
Subject: Re: [PATCH v3 2/2] riscv: T-Head: Test availability bit before
 enabling MAE errata
To: Yangyu Chen <cyy@cyyself.name>
Cc: ajones@ventanamicro.com, alex@ghiti.fr, alistair.francis@wdc.com, 
	aou@eecs.berkeley.edu, bjorn@kernel.org, conor.dooley@microchip.com, 
	conor@kernel.org, cooper.qu@linux.alibaba.com, dbarboza@ventanamicro.com, 
	dqfext@gmail.com, eric.huang@linux.alibaba.com, heiko@sntech.de, 
	linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, 
	palmer@dabbelt.com, paul.walmsley@sifive.com, philipp.tomsich@vrull.eu, 
	samuel.holland@sifive.com, zhiwei_liu@linux.alibaba.com, guoren@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 8, 2024 at 3:58=E2=80=AFAM Yangyu Chen <cyy@cyyself.name> wrote=
:
>
> On 2024/4/8 05:32, Christoph M=C3=BCllner 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 errat=
a
> > 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@microchip.com>
> > Signed-off-by: Christoph M=C3=BCllner <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 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 !=3D 0 || impid !=3D 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 MAE=
E.
> But it also supports Svpbmt, and we can perform the switch by clearing th=
e
> 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 b=
it
> to MAEE. So, it might used in their own TEE implementation and will not b=
e
> 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 s=
et
> 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=3D=3D0&&impid=3D=3D0 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 =3D=3D 0 and impid =3D=3D 0 then decide based on th.sxstatus.M=
AEE
* else test if "xtheadmae" is in the DT


>
> [1] https://occ-intl-prod.oss-ap-southeast-1.aliyuncs.com/resource//16992=
68369347/XuanTie-C908-UserManual.pdf
> [2] https://gist.github.com/cyyself/b9445f38cc3ba1094924bd41c9086176
> [3] https://lore.kernel.org/linux-riscv/tencent_D1180541B4B31C0371DB634A4=
2681A5BF809@qq.com/
>
> Thanks,
> Yangyu Chen
>
> > -     if (stage =3D=3D RISCV_ALTERNATIVES_EARLY_BOOT ||
> > -         stage =3D=3D RISCV_ALTERNATIVES_MODULE)
> > -             return true;
> > +     if (stage !=3D RISCV_ALTERNATIVES_EARLY_BOOT &&
> > +         stage !=3D RISCV_ALTERNATIVES_MODULE)
> > +             return false;
> >
> > -     return false;
> > +     if (!(csr_read(CSR_TH_SXSTATUS) & SXSTATUS_MAEE))
> > +             return false;
> > +
> > +     return true;
> >   }
> >
> >   /*
>