Messages in this thread | | | From | Christoph Müllner <> | Date | Mon, 8 Apr 2024 09:55:48 +0200 | Subject | Re: [PATCH v3 2/2] riscv: T-Head: Test availability bit before enabling MAE errata |
| |
On Mon, Apr 8, 2024 at 9:37 AM Yangyu Chen <cyy@cyyself.name> wrote: > > > > > On Apr 8, 2024, at 14:00, Christoph Müllner <christoph.muellner@vrull.eu> wrote: > > > > On Mon, Apr 8, 2024 at 3:58 AM Yangyu Chen <cyy@cyyself.name> 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@microchip.com> > >>> Signed-off-by: Christoph Müllner <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 != 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 > > > > > > 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.
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 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/xtheadsxstatus.adoc > > Thanks, > Yangyu Chen > > > > > > >> > >> [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@qq.com/ > >> > >> 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; > >>> } > >>> > >>> /* > >> >
| |