linux-kernel - Re: [PATCH 06/19] riscv: Extend cpufeature.c to detect vendor extensions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 12 Apr 2024 13:30:08 +0100
From: Conor Dooley <conor.dooley@...rochip.com>
To: Charlie Jenkins <charlie@...osinc.com>
CC: Conor Dooley <conor@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Paul Walmsley
	<paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, Albert Ou
	<aou@...s.berkeley.edu>, Guo Ren <guoren@...nel.org>, Conor Dooley
	<conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec
	<jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, Evan Green
	<evan@...osinc.com>, Clément Léger
	<cleger@...osinc.com>, Jonathan Corbet <corbet@....net>, Shuah Khan
	<shuah@...nel.org>, <linux-riscv@...ts.infradead.org>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Palmer Dabbelt
	<palmer@...osinc.com>, <linux-arm-kernel@...ts.infradead.org>,
	<linux-sunxi@...ts.linux.dev>, <linux-doc@...r.kernel.org>,
	<linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH 06/19] riscv: Extend cpufeature.c to detect vendor
 extensions

On Thu, Apr 11, 2024 at 09:11:12PM -0700, Charlie Jenkins wrote:
>  static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct 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 long *this_hwcap, struct risc
>  		const char *ext = isa++;
>  		const char *ext_end = isa;
>  		bool ext_long = false, ext_err = false;
> +		struct riscv_isainfo *selected_isainfo = isainfo;
> +		const struct riscv_isa_ext_data *selected_riscv_isa_ext = riscv_isa_ext;
> +		size_t selected_riscv_isa_ext_count = riscv_isa_ext_count;
> +		unsigned int id_offset = 0;
>  
>  		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 = get_isa_vendor_ext(vendorid,
> +						   &selected_riscv_isa_ext,
> +						   &selected_riscv_isa_ext_count);
> +			selected_isainfo = isavendorinfo;
> +			id_offset = 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 != '_'; ++isa)
> +					;
> +				ext_err = 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 long *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 long *this_hwcap, struct risc
>  				set_bit(nr, isainfo->isa);
>  			}
>  		} else {
> -			for (int i = 0; i < riscv_isa_ext_count; i++)
> -				match_isa_ext(&riscv_isa_ext[i], ext, ext_end, isainfo);
> +			for (int i = 0; i < selected_riscv_isa_ext_count; i++)
> +				match_isa_ext(&selected_riscv_isa_ext[i], ext,
> +					      ext_end, selected_isainfo,
> +					      id_offset);
>  		}
>  	}
>  }

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)