Received-SPF: pass (google.com: domain of linux-kernel+bounces-120713-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 27 Mar 2024 11:03:06 +0000
From: Conor Dooley <conor@kernel.org>
To: Christoph =?iso-8859-1?Q?M=FCllner?= <christoph.muellner@vrull.eu>
Cc: 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>,
	=?iso-8859-1?Q?Bj=F6rn_T=F6pel?= <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>,
	Andrew Jones <ajones@ventanamicro.com>
Subject: Re: [PATCH 2/2] riscv: T-Head: Test availability bit before enabling
 MAEE errata
Message-ID: <20240327-imperfect-washbowl-d95e57cef0ef@spud>
References: <20240327103130.3651950-1-christoph.muellner@vrull.eu>
 <20240327103130.3651950-3-christoph.muellner@vrull.eu>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="ztnT3OuJ/y9UCOEL"
Content-Disposition: inline
In-Reply-To: <20240327103130.3651950-3-christoph.muellner@vrull.eu>


--ztnT3OuJ/y9UCOEL
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 27, 2024 at 11:31:30AM +0100, Christoph M=FCllner 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.
>=20
> 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.
>=20

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

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

>=20
> Signed-off-by: Christoph M=FCllner <christoph.muellner@vrull.eu>
> ---
>  arch/riscv/errata/thead/errata.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>=20
> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/e=
rrata.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>
> =20
> +#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;
> =20
> -	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;
> =20
> -	return false;
> +	if (!(csr_read(CSR_TH_MXSTATUS) & MXSTATUS_MAEE))
> +		return false;
> +
> +	return true;
>  }
> =20
>  /*
> --=20
> 2.44.0
>=20

--ztnT3OuJ/y9UCOEL
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZgP86gAKCRB4tDGHoIJi
0syxAP98Co7XAfFsYfnMnzOgsIfjb+XPHSPmuZTmIeOhn+xvZQEAjP7y2N7JwGPC
u0oOkLDxmwBaDDFykp5t1LWG64W/lQ0=
=e/Gp
-----END PGP SIGNATURE-----

--ztnT3OuJ/y9UCOEL--