diff mbox series

[v1,1/3] RISC-V: clarify ISA string ordering rules in cpu.c

Message ID 20221130234125.2722364-2-conor@kernel.org (mailing list archive)
State Superseded
Headers show
Series Putting some basic order on isa extension lists | expand

Checks

Context Check Description
conchuod/patch_count success Link
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be for-next
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/module_param success Was 0 now: 0
conchuod/build_rv32_defconfig success Build OK
conchuod/build_warn_rv64 success Errors and warnings before: 0 this patch: 0
conchuod/dtb_warn_rv64 success Errors and warnings before: 0 this patch: 0
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 58 lines checked
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Conor Dooley Nov. 30, 2022, 11:41 p.m. UTC 
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(-)

Comments

Andrew Jones Dec. 1, 2022, 8:27 a.m. UTC | #1 
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
Conor Dooley Dec. 1, 2022, 8:48 a.m. UTC | #2 
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.
diff mbox series

Patch

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),