Messages in this thread | | | From | Christoph Müllner <> | Date | Fri, 29 Mar 2024 12:22:51 +0100 | Subject | Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling MAEE errata |
| |
On Thu, Mar 28, 2024 at 4:43 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > Hi Christoph, > > On 28/03/2024 15:18, Christoph Müllner wrote: > > On Wed, Mar 27, 2024 at 1:41 PM Andrew Jones <ajones@ventanamicro.com> wrote: > >> On Wed, Mar 27, 2024 at 11:03:06AM +0000, Conor Dooley wrote: > >>> On Wed, Mar 27, 2024 at 11:31:30AM +0100, Christoph Müllner wrote: > >>>> T-Head's MAEE mechanism (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 MAEE uses reserved bits in PTEs and Linux applies the MAEE 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 MAEE bit > >>>> in TH_MXSTATUS CSR. As the TH_MXSTATUS CSR is only accessible in M-mode > >>>> this patch depends on M-mode firmware that handles this for us > >>>> transparently. > >>>> > >>>> As this patch breaks Linux bootup on all C9xx machines with MAEE, > >>>> which don't have M-mode firmware that handles the access to the > >>>> TH_MXSTATUS CSR, this patch is marked as RFC. > >> Can we wrap the csr access in a _ASM_EXTABLE()? If firmware handles it, > >> then we return true/false based on the value. If firmware doesn't handle > >> it, and we get an illegal instruction exception, then we assume the bit > >> is set, which is the current behavior. > >> > >>> I think this is gonna be unacceptable in its current state given that it > >>> causes problems for every other version of the firmware. Breaking real > >>> systems for the sake of emulation isn't something we can reasonably do. > >>> > >>> To make this sort of change acceptable, you're gonna have to add some way > >>> to differentiate between systems that do and do not support reading this > >>> CSR. I think we either a) need to check the version of the SBI > >>> implementation to see if it hits the threshold for supporting this > >>> feature, or b) add a specific SBI call for this so that we can > >>> differentiate between firmware not supporting the function and the > >> The FWFT SBI extension is being developed as a mechanism for S-mode to ask > >> M-mode things like this, but I think that extension should be used for > >> features that have potential to be changed by S-mode (even if not > >> everything will be changeable on all platforms), whereas anything that's > >> read-only would be better with... > >> > >>> quote-unquote "hardware" not supporting it. I don't really like option a) > >>> as it could grow to several different options (each for a different SBI > >>> implementation) and support for reading the CSR would need to be > >>> unconditional. I have a feeling that I am missing something though, > >>> that'd make it doable without introducing a new call. > >>> > >>> Thanks, > >>> Conor. > >>> > >>> If only we'd made enabling this be controlled by a specific DT property, > >>> then disabling it in QEMU would be as simple as not setting that > >>> property :( > >> ...this, where "DT property" is "ISA extension name". I wonder if we > >> shouldn't start considering the invention of xlinux_vendor_xyz type > >> extension names which firmware could add to the ISA string / array, > >> in order to communicate read-only information like this? > >> > >> Thanks, > >> drew > > Hi Conor and Drew, > > > > Thank you for your hints. > > I fully agree with all your statements and concerns. > > > > Switching from th.mxstatus to th.sxstatus should address all mentioned concerns: > > * no dependency on OpenSBI changes > > * no break of functionality > > * no need for graceful handling of CSR read failures > > * no need to differentiate between HW and emulation (assuming QEMU > > accepts the emulation of th.sxstatus) > > > > Also note that DT handling would be difficult, because we need to probe before > > setting up the page table. > > > We already parse the DT before setting the page table to disable KASLR > and to parse "no4lvl" or "no5lvl" command line parameters. Take a look > at the kernel/pi directory and setup_vm() in mm/init.c.
Ah, I see. So, this can be done with a function similar to get_kaslr_seed() in arch/riscv/kernel/pi/fdt_early.c. And the Makefile will apply the necessary steps to get this working. The downside is that depending on new information in the DT, it will not be backward compatible. So, I don't see a way around probing th.sxstatus.MAEE.
Independent of that, there is work to be done for the T-Head extension discovery in the Linux kernel: * XThead* extensions are not in the DTS * XThead* extensions are not parsed during bootup * XThead* extensions don't trigger optimizations (string ops) or errata (MAEE) * XThead* extensions are not exported via hwprobe However, I think this is independent of addressing the MAEE issue. So, I will send out a V2 with the th.sxstatus.MAEE probing only.
Thanks, Christoph
> > Thanks, > > Alex > > > > > > Thanks! > > > > > >>>> 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 8c8a8a4b0421..dd7bf6c62a35 100644 > >>>> --- a/arch/riscv/errata/thead/errata.c > >>>> +++ b/arch/riscv/errata/thead/errata.c > >>>> @@ -19,6 +19,9 @@ > >>>> #include <asm/patch.h> > >>>> #include <asm/vendorid_list.h> > >>>> > >>>> +#define CSR_TH_MXSTATUS 0x7c0 > >>>> +#define MXSTATUS_MAEE _AC(0x200000, UL) > >>>> + > >>>> static bool errata_probe_maee(unsigned int stage, > >>>> unsigned long arch_id, unsigned long impid) > >>>> { > >>>> @@ -28,11 +31,14 @@ static bool errata_probe_maee(unsigned int stage, > >>>> if (arch_id != 0 || impid != 0) > >>>> return false; > >>>> > >>>> - 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_MXSTATUS) & MXSTATUS_MAEE)) > >>>> + return false; > >>>> + > >>>> + return true; > >>>> } > >>>> > >>>> /* > >>>> -- > >>>> 2.44.0 > >>>> > >> > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv
| |