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

[Andrew Jones <ajones@ventanamicro.com>]

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"):

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"?

> 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.

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

Thanks,
drew

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