Received-SPF: pass (google.com: domain of linux-kernel+bounces-123050-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240327103130.3651950-1-christoph.muellner@vrull.eu>
 <20240327103130.3651950-3-christoph.muellner@vrull.eu> <20240327-imperfect-washbowl-d95e57cef0ef@spud>
 <20240327-77a6b64153a68452d0438999@orel>
In-Reply-To: <20240327-77a6b64153a68452d0438999@orel>
From: =?UTF-8?Q?Christoph_M=C3=BCllner?= <christoph.muellner@vrull.eu>
Date: Thu, 28 Mar 2024 15:18:22 +0100
Message-ID: <CAEg0e7jyGZV3+04HNYzgfHMGYT9wV_c0A=ekpCRi3L-5yjxK=w@mail.gmail.com>
Subject: Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling
 MAEE errata
To: Andrew Jones <ajones@ventanamicro.com>
Cc: Conor Dooley <conor@kernel.org>, linux-riscv@lists.infradead.org, 
	linux-kernel@vger.kernel.org, Palmer Dabbelt <palmer@dabbelt.com>, 
	Paul Walmsley <paul.walmsley@sifive.com>, Albert Ou <aou@eecs.berkeley.edu>, 
	Philipp Tomsich <philipp.tomsich@vrull.eu>, =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>, 
	Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Heiko Stuebner <heiko@sntech.de>, 
	Cooper Qu <cooper.qu@linux.alibaba.com>, Zhiwei Liu <zhiwei_liu@linux.alibaba.com>, 
	Huang Tao <eric.huang@linux.alibaba.com>, Alistair Francis <alistair.francis@wdc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 27, 2024 at 1:41=E2=80=AFPM Andrew Jones <ajones@ventanamicro.c=
om> 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=C3=BCllner 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-mo=
de
> > > 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 i=
t
> > 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 w=
ay
> > to differentiate between systems that do and do not support reading thi=
s
> > 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 as=
k
> 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 conc=
erns:
* 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 bef=
ore
setting up the page table.

Thanks!


>
> >
> > >
> > > Signed-off-by: Christoph M=C3=BCllner <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/the=
ad/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 !=3D 0 || impid !=3D 0)
> > >             return false;
> > >
> > > -   if (stage =3D=3D RISCV_ALTERNATIVES_EARLY_BOOT ||
> > > -       stage =3D=3D RISCV_ALTERNATIVES_MODULE)
> > > -           return true;
> > > +   if (stage !=3D RISCV_ALTERNATIVES_EARLY_BOOT &&
> > > +       stage !=3D RISCV_ALTERNATIVES_MODULE)
> > > +           return false;
> > >
> > > -   return false;
> > > +   if (!(csr_read(CSR_TH_MXSTATUS) & MXSTATUS_MAEE))
> > > +           return false;
> > > +
> > > +   return true;
> > >  }
> > >
> > >  /*
> > > --
> > > 2.44.0
> > >
>
>