Re: [PATCH 7/7] RISC-V: add zbb support to string functions

[Conor Dooley <conor@kernel.org>]

Hey Drew, Christoph,

@Christoph, I did not receive your mail unfortunately as it appears to
be html. I assume you know why that's a problem and had some mailer
issues etc.

also + CC Samuel Ortiz since we discussed this elsewhere...

On Fri, Nov 25, 2022 at 04:28:21PM +0100, Andrew Jones wrote:
> On Fri, Nov 25, 2022 at 12:26:42PM +0100, Christoph Müllner wrote:
> > On Fri, Nov 25, 2022 at 8:49 AM Andrew Jones <ajones@ventanamicro.com>
> > wrote:
> > 
> > > On Fri, Nov 25, 2022 at 12:51:54AM +0100, Heiko Stuebner wrote:
> > > > Am Donnerstag, 24. November 2022, 23:32:58 CET schrieb Conor Dooley:
> > > > > On Thu, Nov 24, 2022 at 11:23:08PM +0100, Heiko Stübner wrote:
> > > ...
> > > > > > > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > > > > > > index bf9dd6764bad..66ff36a57e20 100644
> > > > > > > > --- a/arch/riscv/kernel/cpu.c
> > > > > > > > +++ b/arch/riscv/kernel/cpu.c
> > > > > > > > @@ -166,6 +166,7 @@ static struct riscv_isa_ext_data
> > > isa_ext_arr[] = {
> > > > > > > >       __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(zbb, RISCV_ISA_EXT_ZBB),
> > > > > > > >       __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),
> > > > > > >
> > > > > > > This one I do know that Palmer wants canonically ordered.
> > > > >
> > > > > btw, idk if you noticed but I appear to have picked canonical ordering
> > > > > as today's thing to get confused about a lot.
> > > > >
> > > > > You put zbb after the S extentions - does that meant it is *not* an
> > > > > "Additional Standard Extension" but rather a regular Z one?
> > > >
> > > > This confuses me completely now :-) .
> > > >
> > >
> > > Can we instead post a patch to the spec that changes the order to
> > > alphabetical? The only other option I see is to develop a tool which sorts
> > > extensions and every RISC-V developer keeps it in their back pocket. A
> > > kernel specific tool to check each list we want to keep sorted would be
> > > nice too.
> > >
> > > My preference would be to change the spec to alphabetical order, though,
> > > because the spec isn't explicit[*] enough to write a tool that can handle
> > > all cases. We'll end up needing to have conversations like this one to
> > > write the tool and eventually the tool will be what everyone looks to,
> > > rather than the spec...
> > >
> > > [*] The spec uses words like 'can', 'should', and 'conventional'.
> > >
> > 
> > The unpriv spec is clear about the canonical order (table "Standard ISA
> > extension names"):

So I went reading the isa-manual again for the nth time.. It seems that I
missed the sentence:
Standard machine-level instruction-set extensions are prefixed with the
three letters ``Zxm''. Woops & apologies @Samuel!

However, that table is only clear (as pointed out by Drew) on a
categorical level.

> The caption under table 27.1 does indeed declare the table defines the
> canonical order and that it *must* be used for the name string, but
> almost everywhere else in chapter 27 the word "should" is used to suggest
> how extensions be ordered (only X-extensions say where they must be).
> 
> > 1) Base ISA
> > 2) Standard Unpriv Extension (non alphabetical)
> 
> The 'non alphabetical' part makes this a PITA.
> 
> And section 27.6 says the first letter "conventionally indicates...". I
> suppose we can assume it "must indicate"?

Convention would imply it *should* but not that it must. I think, for
the kernel, we take a stronger view and say that we *will* put them in
the listed order in that table.

> > 3) Standard Supervisor-Level Extensions
> 
> Are the conventions for the first character of S-extensions defined? I've
> seen 'Ss' for "Privileged arch and Supervisor-level extensions", e.g.
> Sscofpmf. 'Sv' for virtual memory (I guess) related extensions, e.g.
> Svinval, and we appear to be using alphabetical order for them.

Again, appears to be a "should". I'd vote for doing everyone a favour
and making it "must" in this case kernel wise.
> 
> > 4) Standard Machine-Level Extensions
> > 5) Non-Standard (Vendor) Extensions
> 
> Anyway, for the relatively easier problem of this kernel list and this
> patch, we could do something with defines like below in order to try and
> keep the order right.
> 
> /*
>  * Each sub-list is sorted alphabetically.
>  */
> #define S_EXTENSIONS                                                    \
>         __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),
> 
> #define Zi_EXTENSIONS                                                   \
>         __RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),             \
>         __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> 
> #define Zb_EXTENSIONS                                                   \
>         __RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZICBOM),                \
> 
> static struct riscv_isa_ext_data isa_ext_arr[] = {
>         Zi_EXTENSIONS
>         Zb_EXTENSIONS
>         S_EXTENSIONS
>         __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX)

The above LGTM (apart from the accident in the zbb entry!)

Thanks for all putting up with my confusion here - although the
lack of clarity appears to be rather widespread hahaha.

Thanks,
Conor.


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv