From: Conor Dooley <[email protected]>
I don't know for sure that I have not re-ordered something that is
sacrosanct. It seems that all of these are internal use structs, and
should be okay, barring the obvious exception of the, intentionally
re-ordered, isa_ext_arr.
With that caveat out of the way - all I did here was try to make things
consistent so that it'd be easier to point patch submitters at a "do
this order please".
I never know which of these can be moved without breaking stuff - but
they all seem to be internal use stuff since they're not in uapi?
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.
I guess consider this an RFS, with the S being Screaming in the case of
me doing something you abhor :)
Thanks,
Conor.
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
From: Conor Dooley <[email protected]>
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]>
Signed-off-by: Conor Dooley <[email protected]>
---
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?
---
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 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),
};
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
On Wed, Nov 30, 2022 at 11:41:25PM +0000, Conor Dooley wrote:
> From: Conor Dooley <[email protected]>
>
> 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]>
> Signed-off-by: Conor Dooley <[email protected]>
> ---
> 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/[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,
> };
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 <[email protected]>
Thanks,
drew
Am Donnerstag, 1. Dezember 2022, 00:41:25 CET schrieb Conor Dooley:
> From: Conor Dooley <[email protected]>
>
> 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]>
> Signed-off-by: Conor Dooley <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
Am Donnerstag, 1. Dezember 2022, 10:00:41 CET schrieb Andrew Jones:
> On Wed, Nov 30, 2022 at 11:41:25PM +0000, Conor Dooley wrote:
> > From: Conor Dooley <[email protected]>
> >
> > 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]>
> > Signed-off-by: Conor Dooley <[email protected]>
> > ---
> > 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/[email protected]/
I'm not sure what became of that series since mid october, though noting
that tightly coupling the patching to extensions alone might cause issues [2]
which some of the "features" like fast-unaligned access, that are not directly
bound to a isa-extension but to an implementation detail
[2] https://lore.kernel.org/all/1991071.yIU609i1g2@phil/
>
> > ---
> > 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);
that sounds like a very reasonable idea ... what's keeping you? :-)
Heiko
On Thu, Dec 01, 2022 at 11:47:04AM +0100, Heiko St?bner wrote:
> Am Donnerstag, 1. Dezember 2022, 10:00:41 CET schrieb Andrew Jones:
> > On Wed, Nov 30, 2022 at 11:41:25PM +0000, Conor Dooley wrote:
> > > From: Conor Dooley <[email protected]>
> > >
> > > 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]>
> > > Signed-off-by: Conor Dooley <[email protected]>
> > > ---
> > > 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/[email protected]/
>
> I'm not sure what became of that series since mid october, though noting
> that tightly coupling the patching to extensions alone might cause issues [2]
> which some of the "features" like fast-unaligned access, that are not directly
> bound to a isa-extension but to an implementation detail
>
> [2] https://lore.kernel.org/all/1991071.yIU609i1g2@phil/
Jisheng said he'd send a refresh soon. Hopefully your comments will be
taken into consideration. It seems like we need both the concepts of
cpufeatures and extensions. Where many times a cpufeature directly maps
to an extension, but not always. Or, we could shoehorn the non-extension
cpufeatures into the extension framework by calling them "derived
extensions" or something.
>
>
> >
> > > ---
> > > 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);
>
> that sounds like a very reasonable idea ... what's keeping you? :-)
Posted :-)
Thanks,
drew
On Thu, Dec 01, 2022 at 10:00:41AM +0100, Andrew Jones wrote:
> On Wed, Nov 30, 2022 at 11:41:25PM +0000, Conor Dooley wrote:
> > From: Conor Dooley <[email protected]>
> >
> > 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]>
> > Signed-off-by: Conor Dooley <[email protected]>
> > ---
> > 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
Yah, this all kinda ties in with Palmer's RFC set that does the hwcap
stuff. Kinda been holding off on any thoughts on the isa string as a
valuable anything until that sees a proper respin.
> > 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
I did one thing as a "putting it out there" in the responses to another
series and you suggested something different entirely. Ordinarily, I'd
not put review comments in a suggested-by, but figured it was okay this
time.
> 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).
Think I said it at the yoke yesterday, but I don't think that this is
much of a problem. If it gets out of order, we just get someone that's
sending a patchset already to fix things up.
> 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 guess I sent the patches, so my opinion is fairly obvious, but I think
we change it & see if someone complains about an issue that something
other than a re-jig would break.
Thanks,
Conor.
On 01/12/2022 12:29, Conor Dooley wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Thu, Dec 01, 2022 at 10:00:41AM +0100, Andrew Jones wrote:
>> On Wed, Nov 30, 2022 at 11:41:25PM +0000, Conor Dooley wrote:
>>> From: Conor Dooley <[email protected]>
>>>
>>> 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]>
>>> Signed-off-by: Conor Dooley <[email protected]>
>>> ---
>
>>> 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
>
> Yah, this all kinda ties in with Palmer's RFC set that does the hwcap
> stuff. Kinda been holding off on any thoughts on the isa string as a
> valuable anything until that sees a proper respin.
>
>>> 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
>
> I did one thing as a "putting it out there" in the responses to another
> series and you suggested something different entirely. Ordinarily, I'd
> not put review comments in a suggested-by, but figured it was okay this
> time.
>
>> 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).
>
> Think I said it at the yoke yesterday, but I don't think that this is
> much of a problem. If it gets out of order, we just get someone that's
> sending a patchset already to fix things up.
>
>> 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 guess I sent the patches, so my opinion is fairly obvious, but I think
> we change it & see if someone complains about an issue that something
> other than a re-jig would break.
typo: s/would/wouldn't/, that changes the meaning of my comment.
If a valid addition would break their parser, that's not really a
"uAPI breakage". It's only something that this re-order would break
but additions or valid change of the string based on cpu capability
would not that we need to worry about IMO.