* [PATCH v1 0/3] Putting some basic order on isa extension lists @ 2022-11-30 23:41 Conor Dooley 2022-11-30 23:41 ` [PATCH v1 1/3] RISC-V: clarify ISA string ordering rules in cpu.c Conor Dooley ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Conor Dooley @ 2022-11-30 23:41 UTC (permalink / raw) To: Palmer Dabbelt, linux-riscv Cc: Conor Dooley, ajones, aou, conor, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc From: Conor Dooley <conor.dooley@microchip.com> 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: ajones@ventanamicro.com CC: aou@eecs.berkeley.edu CC: conor@kernel.org CC: conor.dooley@microchip.com CC: corbet@lwn.net CC: guoren@kernel.org CC: heiko@sntech.de CC: palmer@dabbelt.com CC: paul.walmsley@sifive.com CC: linux-kernel@vger.kernel.org CC: linux-riscv@lists.infradead.org CC: linux-doc@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v1 1/3] RISC-V: clarify ISA string ordering rules in cpu.c 2022-11-30 23:41 [PATCH v1 0/3] Putting some basic order on isa extension lists Conor Dooley @ 2022-11-30 23:41 ` Conor Dooley 2022-12-01 8:27 ` Andrew Jones 2022-11-30 23:41 ` [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders Conor Dooley 2022-11-30 23:41 ` [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo Conor Dooley 2 siblings, 1 reply; 18+ messages in thread From: Conor Dooley @ 2022-11-30 23:41 UTC (permalink / raw) To: Palmer Dabbelt, linux-riscv Cc: Conor Dooley, ajones, aou, conor, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc From: Conor Dooley <conor.dooley@microchip.com> While the current list of rules may have been accurate when created it now lacks some clarity in the face of isa-manual updates. Instead of trying to continuously align this rule-set with the one in the specifications, change the role of this comment. This particular comment is important, as the array it "decorates" defines the order in which the ISA string appears to userspace in /proc/cpuinfo. Re-jig and strengthen the wording to provide contributors with a set order in which to add entries & note why this particular struct needs more attention than others. While in the area, add some whitespace and tweak some wording for readability's sake. Suggested-by: Andrew Jones <ajones@ventanamicro.com> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> --- arch/riscv/kernel/cpu.c | 49 ++++++++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c index 852ecccd8920..68b2bd0cc3bc 100644 --- a/arch/riscv/kernel/cpu.c +++ b/arch/riscv/kernel/cpu.c @@ -120,22 +120,45 @@ device_initcall(riscv_cpuinfo_init); .uprop = #UPROP, \ .isa_ext_id = EXTID, \ } + /* - * Here are the ordering rules of extension naming defined by RISC-V - * specification : - * 1. All extensions should be separated from other multi-letter extensions - * by an underscore. - * 2. The first letter following the 'Z' conventionally indicates the most + * The canonical order of ISA extension names in the ISA string is defined in + * chapter 27 of the unprivileged specification. + * + * Ordinarily, for in-kernel data structures, this order is unimportant but + * isa_ext_arr defines the order of the ISA string in /proc/cpuinfo. + * + * The specification uses vague wording, such as should, when it comes to + * ordering so for our purposes the following rules apply: + * + * 1. All multi-letter extensions must be separated from other multi-letter + * extensions by an underscore. + * + * 2. Additional standard extensions (starting with 'Z') must be sorted after + * single-letter extensions and before any higher-privileged extensions. + + * 3. The first letter following the 'Z' conventionally indicates the most * closely related alphabetical extension category, IMAFDQLCBKJTPVH. - * If multiple 'Z' extensions are named, they should be ordered first - * by category, then alphabetically within a category. - * 3. Standard supervisor-level extensions (starts with 'S') should be - * listed after standard unprivileged extensions. If multiple - * supervisor-level extensions are listed, they should be ordered + * If multiple 'Z' extensions are named, they should be ordered first by + * category, then alphabetically within a category. + * + * 3. Standard supervisor-level extensions (starting with 'S') must be listed + * after standard unprivileged extensions. If multiple + * supervisor-level extensions are listed, they must be ordered * alphabetically. - * 4. Non-standard extensions (starts with 'X') must be listed after all - * standard extensions. They must be separated from other multi-letter - * extensions by an underscore. + * + * 4. Standard machine-level extensions (starting with 'Zxm') must be listed + * after any lower-privileged, standard extensions. If multiple + * machine-level extensions are listed, they must be ordered + * alphabetically. + * + * 5. Non-standard extensions (starts with 'X') must be listed after all + * standard extensions. + * + * An example string following the order is: + * rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux + * + * 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(sscofpmf, RISCV_ISA_EXT_SSCOFPMF), -- 2.38.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v1 1/3] RISC-V: clarify ISA string ordering rules in cpu.c 2022-11-30 23:41 ` [PATCH v1 1/3] RISC-V: clarify ISA string ordering rules in cpu.c Conor Dooley @ 2022-12-01 8:27 ` Andrew Jones 2022-12-01 8:48 ` Conor Dooley 0 siblings, 1 reply; 18+ messages in thread From: Andrew Jones @ 2022-12-01 8:27 UTC (permalink / raw) To: Conor Dooley Cc: Palmer Dabbelt, linux-riscv, Conor Dooley, aou, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc On Wed, Nov 30, 2022 at 11:41:24PM +0000, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > While the current list of rules may have been accurate when created > it now lacks some clarity in the face of isa-manual updates. Instead of > trying to continuously align this rule-set with the one in the > specifications, change the role of this comment. > > This particular comment is important, as the array it "decorates" > defines the order in which the ISA string appears to userspace in > /proc/cpuinfo. > > Re-jig and strengthen the wording to provide contributors with a set > order in which to add entries & note why this particular struct needs > more attention than others. > > While in the area, add some whitespace and tweak some wording for > readability's sake. > > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > arch/riscv/kernel/cpu.c | 49 ++++++++++++++++++++++++++++++----------- > 1 file changed, 36 insertions(+), 13 deletions(-) > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c > index 852ecccd8920..68b2bd0cc3bc 100644 > --- a/arch/riscv/kernel/cpu.c > +++ b/arch/riscv/kernel/cpu.c > @@ -120,22 +120,45 @@ device_initcall(riscv_cpuinfo_init); > .uprop = #UPROP, \ > .isa_ext_id = EXTID, \ > } > + > /* > - * Here are the ordering rules of extension naming defined by RISC-V > - * specification : > - * 1. All extensions should be separated from other multi-letter extensions > - * by an underscore. > - * 2. The first letter following the 'Z' conventionally indicates the most > + * The canonical order of ISA extension names in the ISA string is defined in > + * chapter 27 of the unprivileged specification. > + * > + * Ordinarily, for in-kernel data structures, this order is unimportant but > + * isa_ext_arr defines the order of the ISA string in /proc/cpuinfo. > + * > + * The specification uses vague wording, such as should, when it comes to > + * ordering so for our purposes the following rules apply: > + * > + * 1. All multi-letter extensions must be separated from other multi-letter 1. All multi-letter extensions must be separated from other extensions by an underscore. (Because we always lead multi-letter extensions with underscore, even the first one, which follows the single-letter extensions.) > + * extensions by an underscore. > + * > + * 2. Additional standard extensions (starting with 'Z') must be sorted after > + * single-letter extensions and before any higher-privileged extensions. > + > + * 3. The first letter following the 'Z' conventionally indicates the most > * closely related alphabetical extension category, IMAFDQLCBKJTPVH. > - * If multiple 'Z' extensions are named, they should be ordered first > - * by category, then alphabetically within a category. > - * 3. Standard supervisor-level extensions (starts with 'S') should be > - * listed after standard unprivileged extensions. If multiple > - * supervisor-level extensions are listed, they should be ordered > + * If multiple 'Z' extensions are named, they should be ordered first by > + * category, then alphabetically within a category. > + * > + * 3. Standard supervisor-level extensions (starting with 'S') must be listed > + * after standard unprivileged extensions. If multiple > + * supervisor-level extensions are listed, they must be ordered > * alphabetically. > - * 4. Non-standard extensions (starts with 'X') must be listed after all > - * standard extensions. They must be separated from other multi-letter > - * extensions by an underscore. > + * > + * 4. Standard machine-level extensions (starting with 'Zxm') must be listed > + * after any lower-privileged, standard extensions. If multiple > + * machine-level extensions are listed, they must be ordered > + * alphabetically. > + * > + * 5. Non-standard extensions (starts with 'X') must be listed after all > + * standard extensions. ^and alphabetically. > + * > + * An example string following the order is: > + * rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux > + * > + * 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(sscofpmf, RISCV_ISA_EXT_SSCOFPMF), > -- > 2.38.1 > Otherwise, Reviewed-by: Andrew Jones <ajones@ventanamicro.com> Thanks, drew ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 1/3] RISC-V: clarify ISA string ordering rules in cpu.c 2022-12-01 8:27 ` Andrew Jones @ 2022-12-01 8:48 ` Conor Dooley 0 siblings, 0 replies; 18+ messages in thread From: Conor Dooley @ 2022-12-01 8:48 UTC (permalink / raw) To: Andrew Jones Cc: Conor Dooley, Palmer Dabbelt, linux-riscv, aou, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc On Thu, Dec 01, 2022 at 09:27:43AM +0100, Andrew Jones wrote: > On Wed, Nov 30, 2022 at 11:41:24PM +0000, Conor Dooley wrote: > > From: Conor Dooley <conor.dooley@microchip.com> > > > > While the current list of rules may have been accurate when created > > it now lacks some clarity in the face of isa-manual updates. Instead of > > trying to continuously align this rule-set with the one in the > > specifications, change the role of this comment. > > > > This particular comment is important, as the array it "decorates" > > defines the order in which the ISA string appears to userspace in > > /proc/cpuinfo. > > > > Re-jig and strengthen the wording to provide contributors with a set > > order in which to add entries & note why this particular struct needs > > more attention than others. > > > > While in the area, add some whitespace and tweak some wording for > > readability's sake. > > > > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > --- > > arch/riscv/kernel/cpu.c | 49 ++++++++++++++++++++++++++++++----------- > > 1 file changed, 36 insertions(+), 13 deletions(-) > > > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c > > index 852ecccd8920..68b2bd0cc3bc 100644 > > --- a/arch/riscv/kernel/cpu.c > > +++ b/arch/riscv/kernel/cpu.c > > @@ -120,22 +120,45 @@ device_initcall(riscv_cpuinfo_init); > > .uprop = #UPROP, \ > > .isa_ext_id = EXTID, \ > > } > > + > > /* > > - * Here are the ordering rules of extension naming defined by RISC-V > > - * specification : > > - * 1. All extensions should be separated from other multi-letter extensions > > - * by an underscore. > > - * 2. The first letter following the 'Z' conventionally indicates the most > > + * The canonical order of ISA extension names in the ISA string is defined in > > + * chapter 27 of the unprivileged specification. > > + * > > + * Ordinarily, for in-kernel data structures, this order is unimportant but > > + * isa_ext_arr defines the order of the ISA string in /proc/cpuinfo. > > + * > > + * The specification uses vague wording, such as should, when it comes to > > + * ordering so for our purposes the following rules apply: > > + * > > + * 1. All multi-letter extensions must be separated from other multi-letter > > 1. All multi-letter extensions must be separated from other extensions by an > underscore. > > (Because we always lead multi-letter extensions with underscore, even the > first one, which follows the single-letter extensions.) Yah, I need to think as if I am using De Morgan's... The DT ABI requires "should" and permits this. The uAPI is "must"/"will" and always has an _. I'll propagate that change to the docs patch too. > > + * extensions by an underscore. > > + * > > + * 2. Additional standard extensions (starting with 'Z') must be sorted after > > + * single-letter extensions and before any higher-privileged extensions. > > + > > + * 3. The first letter following the 'Z' conventionally indicates the most > > * closely related alphabetical extension category, IMAFDQLCBKJTPVH. > > - * If multiple 'Z' extensions are named, they should be ordered first > > - * by category, then alphabetically within a category. > > - * 3. Standard supervisor-level extensions (starts with 'S') should be > > - * listed after standard unprivileged extensions. If multiple > > - * supervisor-level extensions are listed, they should be ordered > > + * If multiple 'Z' extensions are named, they should be ordered first by > > + * category, then alphabetically within a category. > > + * > > + * 3. Standard supervisor-level extensions (starting with 'S') must be listed > > + * after standard unprivileged extensions. If multiple > > + * supervisor-level extensions are listed, they must be ordered > > * alphabetically. > > - * 4. Non-standard extensions (starts with 'X') must be listed after all > > - * standard extensions. They must be separated from other multi-letter > > - * extensions by an underscore. > > + * > > + * 4. Standard machine-level extensions (starting with 'Zxm') must be listed > > + * after any lower-privileged, standard extensions. If multiple > > + * machine-level extensions are listed, they must be ordered > > + * alphabetically. > > + * > > + * 5. Non-standard extensions (starts with 'X') must be listed after all > > + * standard extensions. > ^and alphabetically. "If multiple non-standard extensions are listed, they must be ordered alphabetically." I'll also propagate this to the doc one, if I have not already. > Otherwise, > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> Cool. I'll give it a bit before respinning, but I think we are at least getting less ambiguous as time goes on.. Thanks, Conor. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders 2022-11-30 23:41 [PATCH v1 0/3] Putting some basic order on isa extension lists Conor Dooley 2022-11-30 23:41 ` [PATCH v1 1/3] RISC-V: clarify ISA string ordering rules in cpu.c Conor Dooley @ 2022-11-30 23:41 ` Conor Dooley 2022-12-01 9:00 ` Andrew Jones 2022-12-01 10:48 ` Heiko Stübner 2022-11-30 23:41 ` [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo Conor Dooley 2 siblings, 2 replies; 18+ messages in thread From: Conor Dooley @ 2022-11-30 23:41 UTC (permalink / raw) To: Palmer Dabbelt, linux-riscv Cc: Conor Dooley, ajones, aou, conor, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc From: Conor Dooley <conor.dooley@microchip.com> 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@microchip.com/ Suggested-by: Andrew Jones <ajones@ventanamicro.com> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> --- 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders 2022-11-30 23:41 ` [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders Conor Dooley @ 2022-12-01 9:00 ` Andrew Jones 2022-12-01 10:47 ` Heiko Stübner 2022-12-01 12:29 ` Conor Dooley 2022-12-01 10:48 ` Heiko Stübner 1 sibling, 2 replies; 18+ messages in thread From: Andrew Jones @ 2022-12-01 9:00 UTC (permalink / raw) To: Conor Dooley Cc: Palmer Dabbelt, linux-riscv, Conor Dooley, aou, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc On Wed, Nov 30, 2022 at 11:41:25PM +0000, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > 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@microchip.com/ > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > 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@kernel.org/ > --- > 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@ventanamicro.com> Thanks, drew ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders 2022-12-01 9:00 ` Andrew Jones @ 2022-12-01 10:47 ` Heiko Stübner 2022-12-01 11:38 ` Andrew Jones 2022-12-01 12:29 ` Conor Dooley 1 sibling, 1 reply; 18+ messages in thread From: Heiko Stübner @ 2022-12-01 10:47 UTC (permalink / raw) To: Conor Dooley, Andrew Jones Cc: Palmer Dabbelt, linux-riscv, Conor Dooley, aou, corbet, guoren, paul.walmsley, linux-kernel, linux-doc 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 <conor.dooley@microchip.com> > > > > 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@microchip.com/ > > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > --- > > 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@kernel.org/ 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders 2022-12-01 10:47 ` Heiko Stübner @ 2022-12-01 11:38 ` Andrew Jones 0 siblings, 0 replies; 18+ messages in thread From: Andrew Jones @ 2022-12-01 11:38 UTC (permalink / raw) To: Heiko Stübner Cc: Conor Dooley, Palmer Dabbelt, linux-riscv, Conor Dooley, aou, corbet, guoren, paul.walmsley, linux-kernel, linux-doc 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 <conor.dooley@microchip.com> > > > > > > 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@microchip.com/ > > > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > > --- > > > 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@kernel.org/ > > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders 2022-12-01 9:00 ` Andrew Jones 2022-12-01 10:47 ` Heiko Stübner @ 2022-12-01 12:29 ` Conor Dooley 2022-12-01 12:37 ` Conor.Dooley 1 sibling, 1 reply; 18+ messages in thread From: Conor Dooley @ 2022-12-01 12:29 UTC (permalink / raw) To: Andrew Jones Cc: Conor Dooley, Palmer Dabbelt, linux-riscv, aou, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc 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 <conor.dooley@microchip.com> > > > > 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@microchip.com/ > > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > --- > > 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders 2022-12-01 12:29 ` Conor Dooley @ 2022-12-01 12:37 ` Conor.Dooley 0 siblings, 0 replies; 18+ messages in thread From: Conor.Dooley @ 2022-12-01 12:37 UTC (permalink / raw) To: ajones Cc: conor, palmer, linux-riscv, aou, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc 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 <conor.dooley@microchip.com> >>> >>> 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@microchip.com/ >>> Suggested-by: Andrew Jones <ajones@ventanamicro.com> >>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >>> --- > >>> 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders 2022-11-30 23:41 ` [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders Conor Dooley 2022-12-01 9:00 ` Andrew Jones @ 2022-12-01 10:48 ` Heiko Stübner 1 sibling, 0 replies; 18+ messages in thread From: Heiko Stübner @ 2022-12-01 10:48 UTC (permalink / raw) To: Palmer Dabbelt, linux-riscv, Conor Dooley Cc: Conor Dooley, ajones, aou, conor, corbet, guoren, paul.walmsley, linux-kernel, linux-doc Am Donnerstag, 1. Dezember 2022, 00:41:25 CET schrieb Conor Dooley: > From: Conor Dooley <conor.dooley@microchip.com> > > 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@microchip.com/ > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> Reviewed-by: Heiko Stuebner <heiko.stuebner@vrull.eu> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo 2022-11-30 23:41 [PATCH v1 0/3] Putting some basic order on isa extension lists Conor Dooley 2022-11-30 23:41 ` [PATCH v1 1/3] RISC-V: clarify ISA string ordering rules in cpu.c Conor Dooley 2022-11-30 23:41 ` [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders Conor Dooley @ 2022-11-30 23:41 ` Conor Dooley 2022-11-30 23:46 ` Conor Dooley ` (2 more replies) 2 siblings, 3 replies; 18+ messages in thread From: Conor Dooley @ 2022-11-30 23:41 UTC (permalink / raw) To: Palmer Dabbelt, linux-riscv Cc: Conor Dooley, ajones, aou, conor, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc From: Conor Dooley <conor.dooley@microchip.com> 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 uAPI. Formalise this as part of the uAPI, by documenting the list of rules we use at this point in time. Signed-off-by: Conor Dooley <conor.dooley@microchip.com> --- I've not "tested" these docs. The NIPA-esque pwbot should go and test it AFAICT. If it doesn't, I'll go add that. --- Documentation/riscv/uabi.rst | 42 ++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst index 21a82cfb6c4d..bc3c8ced644b 100644 --- a/Documentation/riscv/uabi.rst +++ b/Documentation/riscv/uabi.rst @@ -3,4 +3,46 @@ RISC-V Linux User ABI ===================== +Misaligned accesses +------------------- + Misaligned accesses are supported in userspace, but they may perform poorly. + +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", so + "IMAFDQLCBKJTPVH". + +#. All multi-letter extensions will be separated from other multi-letter + extensions by an underscore. + +#. Additional standard extensions (starting with 'Z') will be sorted after + single-letter extensions and before any higher-privileged extensions. + +#. The first letter following the 'Z' conventionally indicates the most + closely related alphabetical extension category, IMAFDQLCBKJTPVH. + If multiple 'Z' extensions are named, they should be ordered first by + category, 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 (starts with 'X') will be listed after all + standard extensions. + +An example string following the order is: + rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux + -- 2.38.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo 2022-11-30 23:41 ` [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo Conor Dooley @ 2022-11-30 23:46 ` Conor Dooley 2022-12-01 3:05 ` Bagas Sanjaya 2022-12-01 9:14 ` Andrew Jones 2 siblings, 0 replies; 18+ messages in thread From: Conor Dooley @ 2022-11-30 23:46 UTC (permalink / raw) To: Palmer Dabbelt, linux-riscv Cc: Conor Dooley, ajones, aou, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc On Wed, Nov 30, 2022 at 11:41:26PM +0000, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > 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 uAPI. > > Formalise this as part of the uAPI, by documenting the list of rules > we use at this point in time. > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > I've not "tested" these docs. The NIPA-esque pwbot should go and > test it AFAICT. If it doesn't, I'll go add that. > --- > Documentation/riscv/uabi.rst | 42 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst > index 21a82cfb6c4d..bc3c8ced644b 100644 > --- a/Documentation/riscv/uabi.rst > +++ b/Documentation/riscv/uabi.rst > @@ -3,4 +3,46 @@ > RISC-V Linux User ABI > ===================== > > +Misaligned accesses > +------------------- > + > Misaligned accesses are supported in userspace, but they may perform poorly. > + > +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", so > + "IMAFDQLCBKJTPVH". > + > +#. All multi-letter extensions will be separated from other multi-letter > + extensions by an underscore. > + > +#. Additional standard extensions (starting with 'Z') will be sorted after > + single-letter extensions and before any higher-privileged extensions. > + > +#. The first letter following the 'Z' conventionally indicates the most > + closely related alphabetical extension category, IMAFDQLCBKJTPVH. > + If multiple 'Z' extensions are named, they should be ordered first by > + category, 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 (starts with 'X') will be listed after all Ehh, it's always the read *after* sending something that I notice the inconsistency. This should be s/starts/starting/ for consistency. > + standard extensions. > + > +An example string following the order is: > + rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux > + > -- > 2.38.1 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo 2022-11-30 23:41 ` [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo Conor Dooley 2022-11-30 23:46 ` Conor Dooley @ 2022-12-01 3:05 ` Bagas Sanjaya 2022-12-01 8:17 ` Conor Dooley 2022-12-01 9:14 ` Andrew Jones 2 siblings, 1 reply; 18+ messages in thread From: Bagas Sanjaya @ 2022-12-01 3:05 UTC (permalink / raw) To: Conor Dooley Cc: Palmer Dabbelt, linux-riscv, Conor Dooley, ajones, aou, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc [-- Attachment #1: Type: text/plain, Size: 1389 bytes --] On Wed, Nov 30, 2022 at 11:41:26PM +0000, Conor Dooley wrote: > +#. Single-letter extensions come first, in "canonical order", so > + "IMAFDQLCBKJTPVH". "..., that is ... ." > +#. The first letter following the 'Z' conventionally indicates the most > + closely related alphabetical extension category, IMAFDQLCBKJTPVH. > + If multiple 'Z' extensions are named, they should be ordered first by > + category, then alphabetically within a category. > + Did you mean "most closely related alphabetical extension category in canonical order"? > +An example string following the order is: > + rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux > + IMO literal code block should be better fit for the example above, rather than definition list: ---- >8 ---- diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst index bc3c8ced644bcf..8005add855dc43 100644 --- a/Documentation/riscv/uabi.rst +++ b/Documentation/riscv/uabi.rst @@ -43,6 +43,7 @@ ordering, so for our purposes the following rules apply: #. Non-standard extensions (starts with 'X') will be listed after all standard extensions. -An example string following the order is: +An example string following the order is:: + rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux Thanks. -- An old man doll... just what I always wanted! - Clara [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo 2022-12-01 3:05 ` Bagas Sanjaya @ 2022-12-01 8:17 ` Conor Dooley 2022-12-02 2:14 ` Bagas Sanjaya 0 siblings, 1 reply; 18+ messages in thread From: Conor Dooley @ 2022-12-01 8:17 UTC (permalink / raw) To: Bagas Sanjaya Cc: Conor Dooley, Palmer Dabbelt, linux-riscv, ajones, aou, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc On Thu, Dec 01, 2022 at 10:05:32AM +0700, Bagas Sanjaya wrote: > On Wed, Nov 30, 2022 at 11:41:26PM +0000, Conor Dooley wrote: > > +#. Single-letter extensions come first, in "canonical order", so > > + "IMAFDQLCBKJTPVH". > > "..., that is ... ." Hmm, that reads strangely to me. s/that/which/. > > > +#. The first letter following the 'Z' conventionally indicates the most > > + closely related alphabetical extension category, IMAFDQLCBKJTPVH. > > + If multiple 'Z' extensions are named, they should be ordered first by > > + category, then alphabetically within a category. > > + > > Did you mean "most closely related alphabetical extension category in > canonical order"? I am not 100% sure what you are suggesting a replacement of here. I think I may reword this as: 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. > > +An example string following the order is: > > + rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux > > + > > IMO literal code block should be better fit for the example above, > rather than definition list: Uh, sure? I'm not sure what impact that has on the output, but I can switch to a pre-formatted block. Thanks, Conor. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo 2022-12-01 8:17 ` Conor Dooley @ 2022-12-02 2:14 ` Bagas Sanjaya 2022-12-02 11:37 ` Conor Dooley 0 siblings, 1 reply; 18+ messages in thread From: Bagas Sanjaya @ 2022-12-02 2:14 UTC (permalink / raw) To: Conor Dooley Cc: Conor Dooley, Palmer Dabbelt, linux-riscv, ajones, aou, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc On 12/1/22 15:17, Conor Dooley wrote: > On Thu, Dec 01, 2022 at 10:05:32AM +0700, Bagas Sanjaya wrote: >> On Wed, Nov 30, 2022 at 11:41:26PM +0000, Conor Dooley wrote: >>> +#. Single-letter extensions come first, in "canonical order", so >>> + "IMAFDQLCBKJTPVH". >> >> "..., that is ... ." > > Hmm, that reads strangely to me. s/that/which/. > OK. >> >>> +#. The first letter following the 'Z' conventionally indicates the most >>> + closely related alphabetical extension category, IMAFDQLCBKJTPVH. >>> + If multiple 'Z' extensions are named, they should be ordered first by >>> + category, then alphabetically within a category. >>> + >> >> Did you mean "most closely related alphabetical extension category in >> canonical order"? > > I am not 100% sure what you are suggesting a replacement of here. I > think I may reword this as: > 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. > That LGTM. >>> +An example string following the order is: >>> + rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux >>> + >> >> IMO literal code block should be better fit for the example above, >> rather than definition list: > > Uh, sure? I'm not sure what impact that has on the output, but I can > switch to a pre-formatted block. > Something like ``foo``? Thanks. -- An old man doll... just what I always wanted! - Clara ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo 2022-12-02 2:14 ` Bagas Sanjaya @ 2022-12-02 11:37 ` Conor Dooley 0 siblings, 0 replies; 18+ messages in thread From: Conor Dooley @ 2022-12-02 11:37 UTC (permalink / raw) To: Bagas Sanjaya Cc: Conor Dooley, Palmer Dabbelt, linux-riscv, ajones, aou, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc [-- Attachment #1: Type: text/plain, Size: 1951 bytes --] On Fri, Dec 02, 2022 at 09:14:08AM +0700, Bagas Sanjaya wrote: > On 12/1/22 15:17, Conor Dooley wrote: > > On Thu, Dec 01, 2022 at 10:05:32AM +0700, Bagas Sanjaya wrote: > >> On Wed, Nov 30, 2022 at 11:41:26PM +0000, Conor Dooley wrote: > >>> +#. Single-letter extensions come first, in "canonical order", so > >>> + "IMAFDQLCBKJTPVH". > >> > >> "..., that is ... ." > > > > Hmm, that reads strangely to me. s/that/which/. > > > > OK. > > >> > >>> +#. The first letter following the 'Z' conventionally indicates the most > >>> + closely related alphabetical extension category, IMAFDQLCBKJTPVH. > >>> + If multiple 'Z' extensions are named, they should be ordered first by > >>> + category, then alphabetically within a category. > >>> + > >> > >> Did you mean "most closely related alphabetical extension category in > >> canonical order"? > > > > I am not 100% sure what you are suggesting a replacement of here. I > > think I may reword this as: > > 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. > > > > That LGTM. > > >>> +An example string following the order is: > >>> + rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux > >>> + > >> > >> IMO literal code block should be better fit for the example above, > >> rather than definition list: > > > > Uh, sure? I'm not sure what impact that has on the output, but I can > > switch to a pre-formatted block. > > > > Something like ``foo``? Not posting a v2 for another few days, but this is what I currently have: https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/tree/Documentation/riscv/uabi.rst?h=riscv-uabi_docs [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo 2022-11-30 23:41 ` [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo Conor Dooley 2022-11-30 23:46 ` Conor Dooley 2022-12-01 3:05 ` Bagas Sanjaya @ 2022-12-01 9:14 ` Andrew Jones 2 siblings, 0 replies; 18+ messages in thread From: Andrew Jones @ 2022-12-01 9:14 UTC (permalink / raw) To: Conor Dooley Cc: Palmer Dabbelt, linux-riscv, Conor Dooley, aou, corbet, guoren, heiko, paul.walmsley, linux-kernel, linux-doc On Wed, Nov 30, 2022 at 11:41:26PM +0000, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > 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 uAPI. uABI > > Formalise this as part of the uAPI, by documenting the list of rules uABI > we use at this point in time. > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > I've not "tested" these docs. The NIPA-esque pwbot should go and > test it AFAICT. If it doesn't, I'll go add that. > --- > Documentation/riscv/uabi.rst | 42 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/Documentation/riscv/uabi.rst b/Documentation/riscv/uabi.rst > index 21a82cfb6c4d..bc3c8ced644b 100644 > --- a/Documentation/riscv/uabi.rst > +++ b/Documentation/riscv/uabi.rst > @@ -3,4 +3,46 @@ > RISC-V Linux User ABI > ===================== > > +Misaligned accesses > +------------------- > + > Misaligned accesses are supported in userspace, but they may perform poorly. > + > +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", so > + "IMAFDQLCBKJTPVH". > + > +#. All multi-letter extensions will be separated from other multi-letter > + extensions by an underscore. > + > +#. Additional standard extensions (starting with 'Z') will be sorted after > + single-letter extensions and before any higher-privileged extensions. > + > +#. The first letter following the 'Z' conventionally indicates the most > + closely related alphabetical extension category, IMAFDQLCBKJTPVH. > + If multiple 'Z' extensions are named, they should be ordered first by > + category, then alphabetically within a category. > + > +#. Standard supervisor-level extensions (starting with 'S') will be listed > + after standard unprivileged extensions. If multiple nit: Seems like a short line, at what character are we required to wrap at in this file? > + 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 (starts with 'X') will be listed after all > + standard extensions. > + > +An example string following the order is: > + rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux > + > -- > 2.38.1 > If this uABI hasn't "shipped" yet, giving us the freedom to discuss it more, then I'd prefer we don't publish this (which looks like "shipping" it) until we're 100% sure that this is the uABI we want. (I feel like if we can still change the order in proc, as the previous patch did, then we haven't yet shipped it.) Thanks, drew ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-12-02 11:38 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-30 23:41 [PATCH v1 0/3] Putting some basic order on isa extension lists Conor Dooley 2022-11-30 23:41 ` [PATCH v1 1/3] RISC-V: clarify ISA string ordering rules in cpu.c Conor Dooley 2022-12-01 8:27 ` Andrew Jones 2022-12-01 8:48 ` Conor Dooley 2022-11-30 23:41 ` [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders Conor Dooley 2022-12-01 9:00 ` Andrew Jones 2022-12-01 10:47 ` Heiko Stübner 2022-12-01 11:38 ` Andrew Jones 2022-12-01 12:29 ` Conor Dooley 2022-12-01 12:37 ` Conor.Dooley 2022-12-01 10:48 ` Heiko Stübner 2022-11-30 23:41 ` [PATCH v1 3/3] Documentation: riscv: add a section about ISA string ordering in /proc/cpuinfo Conor Dooley 2022-11-30 23:46 ` Conor Dooley 2022-12-01 3:05 ` Bagas Sanjaya 2022-12-01 8:17 ` Conor Dooley 2022-12-02 2:14 ` Bagas Sanjaya 2022-12-02 11:37 ` Conor Dooley 2022-12-01 9:14 ` Andrew Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).