Re: [RFC 2/2] RISC-V: resort all extensions in "canonical" order

Andrew Jones
2022-11-29T16:35:29Z

On Tue, Nov 29, 2022 at 02:47:43PM +0000, Conor Dooley wrote:
> Per the comment in cpu.c, re-sort all lists/tables/enums/whatever in
> arch/riscv (apart from KVM) in the current edition of what the isa
> manual considers to be "canonical" order.
>
> None of this is in uapi, so we are free to re-order it? I'm never sure
> when it comes to hwcap...
>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> arch/riscv/include/asm/hwcap.h | 6 +++---
> arch/riscv/kernel/cpu.c | 4 ++--
> arch/riscv/kernel/cpufeature.c | 4 ++--
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index b22525290073..d7d5f27619ee 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -53,12 +53,12 @@ extern unsigned long elf_hwcap;
> * available logical extension id.
> */
> enum riscv_isa_ext_id {
> - RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
> - RISCV_ISA_EXT_SVPBMT,
> - RISCV_ISA_EXT_ZICBOM,
> + RISCV_ISA_EXT_ZICBOM = RISCV_ISA_EXT_BASE,
> RISCV_ISA_EXT_ZIHINTPAUSE,
> + RISCV_ISA_EXT_SSCOFPMF,
> RISCV_ISA_EXT_SSTC,
> RISCV_ISA_EXT_SVINVAL,
> + RISCV_ISA_EXT_SVPBMT,
> RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,

Or,

@@ -48,17 +48,16 @@ extern unsigned long elf_hwcap;
/*
* This enum represent the logical ID for each multi-letter RISC-V ISA extension.
* The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed
- * 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.
+ * RISCV_ISA_EXT_MAX. While the order doesn't matter, we keep it sorted
+ * alphabetically for neatness.
*/
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,
};

> };
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 5e42c92a8456..1d0fa0ebf6a8 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -150,12 +150,12 @@ device_initcall(riscv_cpuinfo_init);
> * extensions by an underscore.
> */
> 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),
> };

Yes, this one should be put in canonical order (I guess).

>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 694267d1fe81..d3df72c4b94f 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -199,12 +199,12 @@ void __init riscv_fill_hwcap(void)
> this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
> set_bit(*ext - 'a', this_isa);
> } else {
> - SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
> - 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("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);

Or,

@@ -199,12 +199,16 @@ void __init riscv_fill_hwcap(void)
this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
set_bit(*ext - 'a', this_isa);
} else {
+ /*
+ * While the order doesn't matter here, we sort
+ * alphabetically for neatness.
+ */
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
}

> }
> #undef SET_ISA_EXT_MAP
> }
> --
> 2.38.1
>

Thanks,
drew