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

[Conor Dooley]

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);
>  		}
>  	}
>  }

Attachment: signature.asc
Description: PGP signature