Received-SPF: pass (google.com: domain of linux-kernel+bounces-121294-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 27 Mar 2024 13:41:17 +0100
From: Andrew Jones <ajones@ventanamicro.com>
To: Conor Dooley <conor@kernel.org>
Cc: Christoph =?utf-8?Q?M=C3=BCllner?= <christoph.muellner@vrull.eu>, 
	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>
Subject: Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling
 MAEE errata
Message-ID: <20240327-77a6b64153a68452d0438999@orel>
References: <20240327103130.3651950-1-christoph.muellner@vrull.eu>
 <20240327103130.3651950-3-christoph.muellner@vrull.eu>
 <20240327-imperfect-washbowl-d95e57cef0ef@spud>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240327-imperfect-washbowl-d95e57cef0ef@spud>

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
> >