Re: [RFC 1/2] RISC-V: clarify ISA string ordering rules in cpu.c

[Conor Dooley]

On Tue, Nov 29, 2022 at 06:19:51PM +0100, Andrew Jones wrote:
> On Tue, Nov 29, 2022 at 04:54:19PM +0000, Conor Dooley wrote:
> > On Tue, Nov 29, 2022 at 05:12:23PM +0100, Andrew Jones wrote:
> > > On Tue, Nov 29, 2022 at 02:47:42PM +0000, Conor Dooley wrote:
> > > > While the list of rules may have been accurate when created, it now
> > > > lacks some clarity in the face of isa-manual updates. Specifically:
> > > > 
> > > > - there is no mention here of a distinction between regular 'Z'
> > > >   extensions which are "Additional Standard Extensions" and "Zxm"
> > > >   extensions which are "Standard Machine-Level Extensions"
> > > > 
> > > > - there is also no explicit mention of where either should be sorted in
> > > >   the list
> > > > 
> > > > - underscores are only required between two *multi-letter* extensions but
> > > >   the list of rules implies that this is required between a multi-letter
> > > >   extension and any other extension. IOW "rv64imafdzicsr_zifencei" is a
> > > >   valid string
> > > > 
> > > > Attempt to clean up the list of rules, by adding information on the
> > > > above & sprinkling in some white space for readability.
> > > > 
> > > > Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> > > > ---
> > > >  arch/riscv/kernel/cpu.c | 22 +++++++++++++++++-----
> > > >  1 file changed, 17 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > > index 852ecccd8920..5e42c92a8456 100644
> > > > --- a/arch/riscv/kernel/cpu.c
> > > > +++ b/arch/riscv/kernel/cpu.c
> > > > @@ -120,20 +120,32 @@ 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.
> > > > + *
> > > > + * 1. All multi-letter extensions should be separated from other multi-letter
> > > > + *    extensions by an underscore.
> > > > + *
> > > >   * 2. 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.
> > > > + *    'Z' extensions should be sorted after single-letter extensions and before
> > > > + *    any higher-privileged extensions.
> > > > + *    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
> > > >   *    alphabetically.
> > > > - * 4. Non-standard extensions (starts with 'X') must be listed after all
> > > > + *
> > > > + * 4  Standard machine-level extensions (starts with 'Zxm') should be
> > > > + *    listed after any lower-privileged, standard extensions.  If multiple
> > > > + *    machine-level extensions are listed, they should be ordered
> > > > + *    alphabetically.
> > > > + *
> > > > + * 5. 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.
> > > >   */
> > > > -- 
> > > > 2.38.1
> > > >
> > > 
> > > Alternatively, we could change the comment to just point out the spec
> > > chapter and provide an example, e.g.
> > 
> > IDK, maybe add the reference & the example but keep the summary?
> 
> It risks needing to synchronize the comment with the spec.

Heh, see I almost see this as an *advantage* of this approach!

> Also, the
> comment doesn't need to reproduce the flexible specifications, since
> Linux has a single implementation (it always puts an underscore between
> single-letter extensions and the first multi-letter extension, for
> example). So, rather than paraphrase too much of the spec, we could just
> point out Linux's specific implementation (with the help of an example).

You're right here though. I've had my head stuck in the incoming-string
side of things rather than outgoing. I'll wait for the all clear from
Above, but ideally I'll reword this explaining where this info shows up
(and therefore why it needs to be ordered), what elements of the rules
matter here (entirely ordering) and then include your bits from below?

> I don't feel that strongly about it though, so we can keep the text
> the spec paraphrasing too.

> > > /*
> > >  * The canonical order of ISA extension names in the ISA string is defined in
> > >  * chapter 27 of the unprivileged spec. An example string following the
> > >  * order is
> > >  *
> > >  *   rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> > >  *
> > >  * Notice how Z-extensions are first sorted by category using the canonical
> > >  * order of the first letter following the Z. Extension groups are in the
> > >  * order specified in chapter 27. Extensions within each group are sorted
> > >  * alphabetically.
> > >  */