For v2+, I added another path with some uapi docs & switched to Drew's
suggested ordering of alphabetically, except in the /proc/cpuinfo array,
as per the discussion today in the pw-sync call. I also added a
sprinkling of comments around which things should be sorted in which
way.
Pasting from the chat on v2, since it's relevant to whether re-ordering
what appears in /proc/cpuinfo is even permitted..
I wrote:
> Drew wrote:
> > 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.
>
> Being a bit hard-nosed about it:
> - the spec has said for years that this order is not correct
>
> - their parser cannot assume any given extension is even present, so the
> index at which the extension starts was only ever going to vary wildly
>
> - to break a parser, it must expect to see extension Abcd before Efgh &
> that order has to change for them
>
> - expecting that a given pair of extensions that appeared one after
> another would always do so is not something we should worry about
> breaking as it was always noted in the comment (and by the specs?)
> that new extensions would be added in alphabetical order (I'd like to
> think that if a clairvoyant wrote a parser and knew that there'd be
> nothing in the gap between the extensions we have now & what may be
> produced they'd also account for this re-ordering...)
>
> - the re-order of sstc is going to land for v6.1 & the addition of sstc
> out of order landed in v6.0, so either that is an issue too or this is
> fine
I'm sending the patchset, so I think it's bordering on obvious that I
think doing this is likely okay & maintaining a "strict" interpretation
of what the spec says is the way to go.
I think the only case we have to worry about breaking uABI stuff is if
this changes behaviour in a way that could not be done by an otherwise
valid change in the ISA string, so bullet 3 above.
I'll leave that determination up to the Higher Powers, but I think it's
going to be very difficult not to accidentally break this order in the
future if we decide that what is currently there, must remain exactly
as-is.
Thanks,
Conor.
Changes since v1:
- strengthened some wording to use "must"
- reworded the bits in both code & doc about what canonical order is
- added some missing "... must be in alphabetical order" sections to
both code & doc
- forced an _ before multi-letter extensions
- there's likely a trivial conflict with drew's addition of an assert
for RISCV_ISA_EXT_ID_MAX.
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
Conor Dooley (3):
RISC-V: clarify ISA string ordering rules in cpu.c
RISC-V: resort all extensions in consistent orders
Documentation: riscv: add a section about ISA string ordering in
/proc/cpuinfo
Documentation/riscv/uabi.rst | 42 +++++++++++++++++++++++++++
arch/riscv/include/asm/hwcap.h | 12 ++++----
arch/riscv/kernel/cpu.c | 53 ++++++++++++++++++++++++----------
arch/riscv/kernel/cpufeature.c | 6 ++--
4 files changed, 91 insertions(+), 22 deletions(-)
--
2.38.1
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/[email protected]/
Suggested-by: Andrew Jones <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
Signed-off-by: Conor Dooley <[email protected]>
---
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,
};
@@ -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)
{
--
2.38.1
The RISC-V specs are permissive in what they allow as the ISA string,
but how we output this to userspace in /proc/cpuinfo is quasi uABI.
Formalise this as part of the uABI, by documenting the list of rules
we use at this point in time.
Signed-off-by: Conor Dooley <[email protected]>
---
Documentation/riscv/uabi.rst | 42 ++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst
index 21a82cfb6c4d..2ebec4c52230 100644
--- a/Documentation/riscv/uabi.rst
+++ b/Documentation/riscv/uabi.rst
@@ -3,4 +3,46 @@
RISC-V Linux User ABI
=====================
+ISA string ordering in /proc/cpuinfo
+------------------------------------
+
+The canonical order of ISA extension names in the ISA string is defined in
+chapter 27 of the unprivileged specification.
+The specification uses vague wording, such as should, when it comes to ordering,
+so for our purposes the following rules apply:
+
+#. Single-letter extensions come first, in canonical order.
+ The canonical order is "IMAFDQLCBKJTPVH".
+
+#. All multi-letter extensions will be separated from other extensions by an
+ underscore.
+
+#. Additional standard extensions (starting with 'Z') will be sorted after
+ single-letter extensions and before any higher-privileged extensions.
+
+#. For additional standard extensions, the first letter following the 'Z'
+ conventionally indicates the most closely related alphabetical
+ extension category. If multiple 'Z' extensions are named, they will be ordered
+ first by category, in canonical order, as listed above, then alphabetically
+ within a category.
+
+#. Standard supervisor-level extensions (starting with 'S') will be listed
+ after standard unprivileged extensions. If multiple supervisor-level
+ extensions are listed, they will be ordered alphabetically.
+
+#. Standard machine-level extensions (starting with 'Zxm') will be listed
+ after any lower-privileged, standard extensions. If multiple machine-level
+ extensions are listed, they will be ordered alphabetically.
+
+#. Non-standard extensions (starting with 'X') will be listed after all standard
+ extensions. If multiple non-standard extensions are listed, they will be
+ ordered alphabetically.
+
+An example string following the order is::
+
+ rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
+
+Misaligned accesses
+-------------------
+
Misaligned accesses are supported in userspace, but they may perform poorly.
--
2.38.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/[email protected]/
> Suggested-by: Andrew Jones <[email protected]>
> Reviewed-by: Andrew Jones <[email protected]>
> Reviewed-by: Heiko Stuebner <[email protected]>
> Signed-off-by: Conor Dooley <[email protected]>
> ---
> 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
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.
Hello:
This series was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <[email protected]>:
On Mon, 5 Dec 2022 14:45:23 +0000 you wrote:
> For v2+, I added another path with some uapi docs & switched to Drew's
> suggested ordering of alphabetically, except in the /proc/cpuinfo array,
> as per the discussion today in the pw-sync call. I also added a
> sprinkling of comments around which things should be sorted in which
> way.
>
> Pasting from the chat on v2, since it's relevant to whether re-ordering
> what appears in /proc/cpuinfo is even permitted..
>
> [...]
Here is the summary with links:
- [v2,1/3] RISC-V: clarify ISA string ordering rules in cpu.c
https://git.kernel.org/riscv/c/99e2266f2460
- [v2,2/3] RISC-V: resort all extensions in consistent orders
https://git.kernel.org/riscv/c/80c200b34ee8
- [v2,3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo
https://git.kernel.org/riscv/c/f07b2b3f9d47
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html