Received-SPF: pass (google.com: domain of linux-kernel+bounces-135023-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <tencent_D6D1043732E63644A4B1838F4210499BCF05@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: <20240408-dispersal-foster-49c4e269216e@spud>
Date: Mon, 8 Apr 2024 16:21:24 +0800
Cc: =?utf-8?Q?Christoph_M=C3=BCllner?= <christoph.muellner@vrull.eu>,
 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>,
 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>
 <tencent_459BC09115173E0624A4A0F19D2F43704F05@qq.com>
 <CAEg0e7jW=Sfc3tcc0eJwbTx=vb0HM49ZWpda972E-t=Fj1PG+g@mail.gmail.com>
 <20240408-dispersal-foster-49c4e269216e@spud>
To: Conor Dooley <conor@kernel.org>


> On Apr 8, 2024, at 16:10, Conor Dooley <conor@kernel.org> wrote:
>=20
> On Mon, Apr 08, 2024 at 09:55:48AM +0200, Christoph M=C3=BCllner =
wrote:
>> 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@vrull.eu> wrote:
>>>> 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 =
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>
>=20
>>>>>> @@ -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
>>>=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.
>>>=20
>>> 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.
>>=20
>> I wrote that specification. And yes, T-Head reviewed that part.
>> But there is no guarantee for future cores.
>=20
> 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.
>=20
>>> 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
>=20
>> 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.
>=20
> 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=