Messages in this thread Patch in this message | | | Date | Wed, 27 Mar 2024 13:41:17 +0100 | From | Andrew Jones <> | Subject | Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling MAEE errata |
| |
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
> > > > > 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 > >
| |