Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling MAEE errata

[Alexandre Ghiti]

Hi Christoph,

On 28/03/2024 15:18, Christoph Müllner wrote:
> On Wed, Mar 27, 2024 at 1:41 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> 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.

Thanks,

Alex


> Thanks!
>
>
> > > > Signed-off-by: Christoph Müllner <christoph.muellner@xxxxxxxx>
> > > > ---
> > > >   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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv