Received-SPF: pass (google.com: domain of linux-kernel+bounces-142653-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 12 Apr 2024 13:30:08 +0100
From: Conor Dooley <conor.dooley@microchip.com>
To: Charlie Jenkins <charlie@rivosinc.com>
CC: Conor Dooley <conor@kernel.org>, Rob Herring <robh@kernel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Paul Walmsley
	<paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou
	<aou@eecs.berkeley.edu>, Guo Ren <guoren@kernel.org>, Conor Dooley
	<conor+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec
	<jernej.skrabec@gmail.com>, Samuel Holland <samuel@sholland.org>, Evan Green
	<evan@rivosinc.com>, =?iso-8859-1?Q?Cl=E9ment_L=E9ger?=
	<cleger@rivosinc.com>, Jonathan Corbet <corbet@lwn.net>, Shuah Khan
	<shuah@kernel.org>, <linux-riscv@lists.infradead.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Palmer Dabbelt
	<palmer@rivosinc.com>, <linux-arm-kernel@lists.infradead.org>,
	<linux-sunxi@lists.linux.dev>, <linux-doc@vger.kernel.org>,
	<linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH 06/19] riscv: Extend cpufeature.c to detect vendor
 extensions
Message-ID: <20240412-sprawl-product-1e1d02e25bca@wendy>
References: <20240411-dev-charlie-support_thead_vector_6_9-v1-0-4af9815ec746@rivosinc.com>
 <20240411-dev-charlie-support_thead_vector_6_9-v1-6-4af9815ec746@rivosinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="/rKIhypokaxpjzdO"
Content-Disposition: inline
In-Reply-To: <20240411-dev-charlie-support_thead_vector_6_9-v1-6-4af9815ec746@rivosinc.com>

--/rKIhypokaxpjzdO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 11, 2024 at 09:11:12PM -0700, Charlie Jenkins wrote:
>  static void __init riscv_parse_isa_string(unsigned long *this_hwcap, str=
uct riscv_isainfo *isainfo,

> -					  unsigned long *isa2hwcap, const char *isa)
> +					struct riscv_isainfo *isavendorinfo, unsigned long vendorid,
> +					unsigned long *isa2hwcap, const char *isa)
>  {
>  	/*
>  	 * For all possible cpus, we have already validated in
> @@ -349,8 +384,30 @@ static void __init riscv_parse_isa_string(unsigned l=
ong *this_hwcap, struct risc
>  		const char *ext =3D isa++;
>  		const char *ext_end =3D isa;
>  		bool ext_long =3D false, ext_err =3D false;
> +		struct riscv_isainfo *selected_isainfo =3D isainfo;
> +		const struct riscv_isa_ext_data *selected_riscv_isa_ext =3D riscv_isa_=
ext;
> +		size_t selected_riscv_isa_ext_count =3D riscv_isa_ext_count;
> +		unsigned int id_offset =3D 0;
> =20
>  		switch (*ext) {
> +		case 'x':
> +		case 'X':

One quick remark is that we should not go and support this stuff via
riscv,isa in my opinion, only allowing it for the riscv,isa-extensions
parsing. We don't have a way to define meanings for vendor extensions in
this way. ACPI also uses this codepath and at the moment the kernel's
docs say we're gonna follow isa string parsing rules in a specific version
of the ISA manual. While that manual provides a format for the string and
meanings for standard extensions, there's nothing in there that allows us
to get consistent meanings for specific vendor extensions, so I think we
should avoid intentionally supporting this here.

I'd probably go as far as to actively skip vendor extensions in
riscv_parse_isa_string() to avoid any potential issues.

> +			bool found;
> +
> +			found =3D get_isa_vendor_ext(vendorid,
> +						   &selected_riscv_isa_ext,
> +						   &selected_riscv_isa_ext_count);
> +			selected_isainfo =3D isavendorinfo;
> +			id_offset =3D RISCV_ISA_VENDOR_EXT_BASE;
> +			if (!found) {
> +				pr_warn("No associated vendor extensions with vendor id: %lx\n",
> +					vendorid);

This should not be a warning, anything we don't understand should be
silently ignored to avoid spamming just because the kernel has not grown
support for it yet.

Thanks,
Conor.

> +				for (; *isa && *isa !=3D '_'; ++isa)
> +					;
> +				ext_err =3D true;
> +				break;
> +			}
> +			fallthrough;
>  		case 's':
>  			/*
>  			 * Workaround for invalid single-letter 's' & 'u' (QEMU).
> @@ -366,8 +423,6 @@ static void __init riscv_parse_isa_string(unsigned lo=
ng *this_hwcap, struct risc
>  			}
>  			fallthrough;
>  		case 'S':
> -		case 'x':
> -		case 'X':
>  		case 'z':
>  		case 'Z':
>  			/*
> @@ -476,8 +531,10 @@ static void __init riscv_parse_isa_string(unsigned l=
ong *this_hwcap, struct risc
>  				set_bit(nr, isainfo->isa);
>  			}
>  		} else {
> -			for (int i =3D 0; i < riscv_isa_ext_count; i++)
> -				match_isa_ext(&riscv_isa_ext[i], ext, ext_end, isainfo);
> +			for (int i =3D 0; i < selected_riscv_isa_ext_count; i++)
> +				match_isa_ext(&selected_riscv_isa_ext[i], ext,
> +					      ext_end, selected_isainfo,
> +					      id_offset);
>  		}
>  	}
>  }

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

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

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZhkpUAAKCRB4tDGHoIJi
0suMAP9hVav0oRL2jmBJm2ACwJ1+CNYfBT488OIk7AWbRjBIigEAtUP/mmReLFMY
g7/EcQ6a/96RixRPX9yYcg23rYMJOQE=
=w9t9
-----END PGP SIGNATURE-----

--/rKIhypokaxpjzdO--