Re: [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders

[Andrew Jones]

On Wed, Nov 30, 2022 at 11:41:25PM +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> 
> 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/20221129144742.2935581-2-conor.dooley@xxxxxxxxxxxxx/
> Suggested-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> ---
> I could not decide between adding an alphabetical comment to each
> alphabetical site or not. I did it anyway. Scream if you hate it!
> 
> I also moved a static branch thingy in this version, but that should not
> matter, right? riightt?

riiighttt. And it goes away with [1] anyway.

[1] https://lore.kernel.org/all/20221006070818.3616-1-jszhang@xxxxxxxxxx/

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

Unrelated to this patch, but every time I look at this enum I want to post
the diff below, but I haven't bothered, because this enum also goes away
with [1].

@@ -59,8 +59,9 @@ enum riscv_isa_ext_id {
        RISCV_ISA_EXT_ZIHINTPAUSE,
        RISCV_ISA_EXT_SSTC,
        RISCV_ISA_EXT_SVINVAL,
-       RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
+       RISCV_ISA_EXT_ID_MAX
 };
+static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);

 /*
  * This enum represents the logical ID for each RISC-V ISA extension static

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

And every time I look at this switch I want to delete the return line above...

>  	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 68b2bd0cc3bc..686d41b14206 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -161,12 +161,12 @@ device_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),
>  };

Technically we should have leave these in the wrong order if we want to be
strict about the ISA string published to userspace, but I'm in favor of
changing this array as necessary and hoping we teach userspace to use
flexible parsers. Actually, IMO, we shouldn't teach userspace to parse
this at all. We should instead create sysfs nodes:

 .../isa/zicbom
 .../isa/zihintpause
 .../isa/sscofpmf

and teach userspace to list .../isa/ to learn about extensions. That would
also allow us to publish extension version numbers which we are not
current doing with the proc isa string.

 .../isa/zicbom/major
 .../isa/zicbom/minor

and we could add other properties if necessary too, e.g.

 .../isa/zicbom/block_size

>  
> 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)
>  {
> -- 
> 2.38.1
> 

I realize that I have a suggested-by tag in the commit message, but I
don't really have a strong opinion on how we order extensions where the
order doesn't matter. A consistent policy of alphabetical or always at
the bottom both work for me. I personally prefer alphabetical when
reading the lists, but I realize we'll eventually merge stuff out of
order and then that'll generate some churn to reorder (but hopefully not
too frequently).

My biggest concern is how much we need to care about the order of the
string in proc and whether or not we're allowed to fix its order like
we're doing with this patch. I hope we can, and I vote we do.

Anyway, none of my comments apply directly to this patch, so

Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>

Thanks,
drew