Received-SPF: pass (google.com: domain of linux-kernel+bounces-134974-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240407213236.2121592-3-christoph.muellner@vrull.eu>
 <tencent_CF232CDF85208DF6BFA3076E73CC4E087306@qq.com> <CAEg0e7h0F_LaeMoS6Co1UjgLM6ML8SXtTfHW5+OQS6yedjdZPQ@mail.gmail.com>
 <tencent_459BC09115173E0624A4A0F19D2F43704F05@qq.com>
In-Reply-To: <tencent_459BC09115173E0624A4A0F19D2F43704F05@qq.com>
From: =?UTF-8?Q?Christoph_M=C3=BCllner?= <christoph.muellner@vrull.eu>
Date: Mon, 8 Apr 2024 09:55:48 +0200
Message-ID: <CAEg0e7jW=Sfc3tcc0eJwbTx=vb0HM49ZWpda972E-t=Fj1PG+g@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, 
	Albert Ou <aou@eecs.berkeley.edu>, bjorn@kernel.org, 
	Conor Dooley <conor.dooley@microchip.com>, Conor Dooley <conor@kernel.org>, 
	cooper.qu@linux.alibaba.com, dbarboza@ventanamicro.com, 
	Qingfang Deng <dqfext@gmail.com>, eric.huang@linux.alibaba.com, heiko@sntech.de, 
	linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, 
	Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, philipp.tomsich@vrull.eu, 
	samuel.holland@sifive.com, zhiwei_liu@linux.alibaba.com, 
	Guo Ren <guoren@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 8, 2024 at 9:37=E2=80=AFAM Yangyu Chen <cyy@cyyself.name> wrote=
:
>
>
>
> > On Apr 8, 2024, at 14:00, Christoph M=C3=BCllner <christoph.muellner@vr=
ull.eu> wrote:
> >
> > On Mon, Apr 8, 2024 at 3:58=E2=80=AFAM Yangyu Chen <cyy@cyyself.name> w=
rote:
> >>
> >> 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 writ=
e
> >>> reserved bits in PTEs.
> >>> As XTheadMae uses reserved bits in PTEs and Linux applies the MAE err=
ata
> >>> 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 b=
it
> >>> 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, becau=
se
> >>> 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/the=
ad/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 li=
ke 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 Securit=
y bit
> >> to MAEE. So, it might used in their own TEE implementation and will no=
t 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]. S=
o, if
> >> we have probed MAEE using CSR, you confirmed that this bit will also b=
e 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 shoul=
d
> >> also move the csr probe earlier than riscv alternatives.
> >
> > We keep the probing via arch_id=3D=3D0&&impid=3D=3D0 because we had tha=
t
> > 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.sxstat=
us.MAEE
> > * else test if "xtheadmae" is in the DT
> >
> >
>
> I know about it, and Conor also mentioned adding this property to DT a fe=
w
> months ago. I agree with this at that time. But for now, you have found t=
he
> T-Head document description about this, and we can probe MAE using CSR ev=
en
> 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 hav=
e
> 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 a=
nd
> 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 softwa=
re
> 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/xthea=
dsxstatus.adoc
>
> Thanks,
> Yangyu Chen
>
> >
> >
> >>
> >> [1] https://occ-intl-prod.oss-ap-southeast-1.aliyuncs.com/resource//16=
99268369347/XuanTie-C908-UserManual.pdf
> >> [2] https://gist.github.com/cyyself/b9445f38cc3ba1094924bd41c9086176
> >> [3] https://lore.kernel.org/linux-riscv/tencent_D1180541B4B31C0371DB63=
4A42681A5BF809@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;
> >>>  }
> >>>
> >>>  /*
> >>
>