Received-SPF: pass (google.com: domain of linux-kernel+bounces-134910-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <tencent_459BC09115173E0624A4A0F19D2F43704F05@qq.com>
Content-Type: text/plain;
	charset=utf-8
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.500.171.1.1\))
Subject: Re: [PATCH v3 2/2] riscv: T-Head: Test availability bit before
 enabling MAE errata
From: Yangyu Chen <cyy@cyyself.name>
In-Reply-To: <CAEg0e7h0F_LaeMoS6Co1UjgLM6ML8SXtTfHW5+OQS6yedjdZPQ@mail.gmail.com>
Date: Mon, 8 Apr 2024 15:36:44 +0800
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-Transfer-Encoding: quoted-printable
References: <20240407213236.2121592-3-christoph.muellner@vrull.eu>
 <tencent_CF232CDF85208DF6BFA3076E73CC4E087306@qq.com>
 <CAEg0e7h0F_LaeMoS6Co1UjgLM6ML8SXtTfHW5+OQS6yedjdZPQ@mail.gmail.com>
To: =?utf-8?Q?Christoph_M=C3=BCllner?= <christoph.muellner@vrull.eu>


> On Apr 8, 2024, at 14:00, Christoph M=C3=BCllner =
<christoph.muellner@vrull.eu> wrote:
>=20
> On Mon, Apr 8, 2024 at 3:58=E2=80=AFAM Yangyu Chen <cyy@cyyself.name> =
wrote:
>>=20
>> 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 =
errata
>>> for all T-Head harts, this broke the Linux startup on QEMU =
emulations
>>> of the C906 emulation.
>>>=20
>>> 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.
>>>=20
>>> 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.
>>>=20
>>> 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(-)
>>>=20
>>> 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>
>>>=20
>>> +#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;
>>>=20
>>=20
>> 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.
>>=20
>> I added CC to guoren for more opinions.
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> 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.
>=20
> 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.
>=20
> =46rom 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.MAEE
> * else test if "xtheadmae" is in the DT
>=20
>=20

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/xtheadsxst=
atus.adoc

Thanks,
Yangyu Chen

>=20
>=20
>>=20
>> [1] =
https://occ-intl-prod.oss-ap-southeast-1.aliyuncs.com/resource//1699268369=
347/XuanTie-C908-UserManual.pdf
>> [2] https://gist.github.com/cyyself/b9445f38cc3ba1094924bd41c9086176
>> [3] =
https://lore.kernel.org/linux-riscv/tencent_D1180541B4B31C0371DB634A42681A=
5BF809@qq.com/
>>=20
>> Thanks,
>> Yangyu Chen
>>=20
>>> -     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;
>>>=20
>>> -     return false;
>>> +     if (!(csr_read(CSR_TH_SXSTATUS) & SXSTATUS_MAEE))
>>> +             return false;
>>> +
>>> +     return true;
>>>  }
>>>=20
>>>  /*
>>=20