[v2,2/3] RISC-V: resort all extensions in consistent orders

Commit Message

Conor Dooley Dec. 5, 2022, 2:45 p.m. UTC

Ordering between each and every list of extensions is wildly
inconsistent. Per discussion on the lists pick the following policy:

- The array defining order in /proc/cpuinfo follows a narrow
  interpretation of the ISA specifications, described in a comment
  immediately presiding it.

- All other lists of extensions are sorted alphabetically.

This will hopefully allow for easier review & future additions, and
reduce conflicts between patchsets as the number of extensions grows.

Link: https://lore.kernel.org/all/[e-mail]/
Suggested-by: Andrew Jones <[e-mail]>
Reviewed-by: Andrew Jones <[e-mail]>
Reviewed-by: Heiko Stuebner <[e-mail]>
Signed-off-by: Conor Dooley <[e-mail]>
---
 arch/riscv/include/asm/hwcap.h | 12 +++++++-----
 arch/riscv/kernel/cpu.c        |  4 ++--
 arch/riscv/kernel/cpufeature.c |  6 ++++--
 3 files changed, 13 insertions(+), 9 deletions(-)

Comments

Andrew Jones Jan. 20, 2023, 1:56 p.m. UTC | #1

On Mon, Dec 05, 2022 at 02:45:25PM +0000, Conor Dooley wrote:
> Ordering between each and every list of extensions is wildly
> inconsistent. Per discussion on the lists pick the following policy:
> 
> - The array defining order in /proc/cpuinfo follows a narrow
>   interpretation of the ISA specifications, described in a comment
>   immediately presiding it.
> 
> - All other lists of extensions are sorted alphabetically.
> 
> This will hopefully allow for easier review & future additions, and
> reduce conflicts between patchsets as the number of extensions grows.
> 
> Link: https://lore.kernel.org/all/[e-mail]/
> Suggested-by: Andrew Jones <[e-mail]>
> Reviewed-by: Andrew Jones <[e-mail]>
> Reviewed-by: Heiko Stuebner <[e-mail]>
> Signed-off-by: Conor Dooley <[e-mail]>
> ---
>  arch/riscv/include/asm/hwcap.h | 12 +++++++-----
>  arch/riscv/kernel/cpu.c        |  4 ++--
>  arch/riscv/kernel/cpufeature.c |  6 ++++--
>  3 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index b22525290073..ce522aad641a 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -51,14 +51,15 @@ extern unsigned long elf_hwcap;
>   * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter
>   * extensions while all the multi-letter extensions should define the next
>   * available logical extension id.
> + * Entries are sorted alphabetically.
>   */
>  enum riscv_isa_ext_id {
>  	RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
> +	RISCV_ISA_EXT_SSTC,
> +	RISCV_ISA_EXT_SVINVAL,
>  	RISCV_ISA_EXT_SVPBMT,
>  	RISCV_ISA_EXT_ZICBOM,
>  	RISCV_ISA_EXT_ZIHINTPAUSE,
> -	RISCV_ISA_EXT_SSTC,
> -	RISCV_ISA_EXT_SVINVAL,
>  	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,

Hi Conor,

I'm digging this back up because I'm basing Zicboz on it.

If we take "riscv: improve boot time isa extensions handling", then this
becomes a bunch of manually enumerated defines

 #define RISCV_ISA_EXT_SSCOFPMF         26
 #define RISCV_ISA_EXT_SVPBMT           27
 #define RISCV_ISA_EXT_ZICBOM           28
 #define RISCV_ISA_EXT_ZIHINTPAUSE      29
 #define RISCV_ISA_EXT_SSTC             30
 #define RISCV_ISA_EXT_SVINVAL          31

Keeping those in alphabetical order would either require manually
reenumerating them or to allow the numbers to be out of order as
we add more extensions. I think I'd prefer we just add new
extensions at the bottom and keep the numbers in order.

Thanks,
drew

Conor Dooley Jan. 20, 2023, 2:16 p.m. UTC | #2

On Fri, Jan 20, 2023 at 02:56:32PM +0100, Andrew Jones wrote:
> Hi Conor,
> 
> I'm digging this back up because I'm basing Zicboz on it.
> 
> If we take "riscv: improve boot time isa extensions handling", then this
> becomes a bunch of manually enumerated defines
> 
>  #define RISCV_ISA_EXT_SSCOFPMF         26
>  #define RISCV_ISA_EXT_SVPBMT           27
>  #define RISCV_ISA_EXT_ZICBOM           28
>  #define RISCV_ISA_EXT_ZIHINTPAUSE      29
>  #define RISCV_ISA_EXT_SSTC             30
>  #define RISCV_ISA_EXT_SVINVAL          31
> 
> Keeping those in alphabetical order would either require manually
> reenumerating them or to allow the numbers to be out of order as
> we add more extensions. I think I'd prefer we just add new
> extensions at the bottom and keep the numbers in order.

Yes. I mentioned that on one of the earlier versions of Jisheng's
patchset - initially I blindly said "alphabetical please".
I quickly realised that that was a really stupid idea as it is would
just be an _invitiation_ for bugs if we did, since names are far more
easily searchable than figuring out the max in the manual enumeration.

Since Jisheng's patchset just deleted what I had resorted, I left this
change as-was. Just need to make sure any comment about ordering also
gets removed when the enum goes away.
I'll keep an eye on for-next to make sure that it does.

TL;DR I agree!

Thanks,
Conor.

Patch

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index b22525290073..ce522aad641a 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -51,14 +51,15 @@  extern unsigned long elf_hwcap;
  * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter
  * extensions while all the multi-letter extensions should define the next
  * available logical extension id.
+ * Entries are sorted alphabetically.
  */
 enum riscv_isa_ext_id {
 	RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
+	RISCV_ISA_EXT_SSTC,
+	RISCV_ISA_EXT_SVINVAL,
 	RISCV_ISA_EXT_SVPBMT,
 	RISCV_ISA_EXT_ZICBOM,
 	RISCV_ISA_EXT_ZIHINTPAUSE,
-	RISCV_ISA_EXT_SSTC,
-	RISCV_ISA_EXT_SVINVAL,
 	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
 };
 
@@ -66,11 +67,12 @@  enum riscv_isa_ext_id {
  * This enum represents the logical ID for each RISC-V ISA extension static
  * keys. We can use static key to optimize code path if some ISA extensions
  * are available.
+ * Entries are sorted alphabetically.
  */
 enum riscv_isa_ext_key {
 	RISCV_ISA_EXT_KEY_FPU,		/* For 'F' and 'D' */
-	RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
 	RISCV_ISA_EXT_KEY_SVINVAL,
+	RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
 	RISCV_ISA_EXT_KEY_MAX,
 };
 
@@ -90,10 +92,10 @@  static __always_inline int riscv_isa_ext2key(int num)
 		return RISCV_ISA_EXT_KEY_FPU;
 	case RISCV_ISA_EXT_d:
 		return RISCV_ISA_EXT_KEY_FPU;
-	case RISCV_ISA_EXT_ZIHINTPAUSE:
-		return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
 	case RISCV_ISA_EXT_SVINVAL:
 		return RISCV_ISA_EXT_KEY_SVINVAL;
+	case RISCV_ISA_EXT_ZIHINTPAUSE:
+		return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
 	default:
 		return -EINVAL;
 	}
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index db8b16ad9342..0bf1c7f663fc 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -185,12 +185,12 @@  arch_initcall(riscv_cpuinfo_init);
  * New entries to this struct should follow the ordering rules described above.
  */
 static struct riscv_isa_ext_data isa_ext_arr[] = {
+	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
+	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
 	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
 	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
 	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
 	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
-	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
-	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
 	__RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
 };
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 694267d1fe81..8a76a6ce70cf 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -199,12 +199,13 @@  void __init riscv_fill_hwcap(void)
 				this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
 				set_bit(*ext - 'a', this_isa);
 			} else {
+				/* sorted alphabetically */
 				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
+				SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
+				SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
 				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
 				SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
 				SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
-				SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
-				SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
 			}
 #undef SET_ISA_EXT_MAP
 		}
@@ -284,6 +285,7 @@  static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage)
  * This code may also be executed before kernel relocation, so we cannot use
  * addresses generated by the address-of operator as they won't be valid in
  * this context.
+ * Tests, unless otherwise required, are to be added in alphabetical order.
  */
 static u32 __init_or_module cpufeature_probe(unsigned int stage)
 {