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

On Fri, Apr 12, 2024 at 01:30:08PM +0100, Conor Dooley wrote:

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.

Getting a "consistent meaning" is managed by a vendor. If a vendor supports a vendor extension and puts it in their DT/ACPI table it's up to them to ensure that it works. How does riscv,isa-extensions allow for a consistent meaning?

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.

Sounds good.

- Charlie

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

  } }

}