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

[Heiko Stuebner <heiko@sntech.de>]

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/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > > index b22525290073..ac5555fd9788 100644
> > > > --- a/arch/riscv/include/asm/hwcap.h
> > > > +++ b/arch/riscv/include/asm/hwcap.h
> > > > @@ -59,6 +59,7 @@ enum riscv_isa_ext_id {
> > > >  	RISCV_ISA_EXT_ZIHINTPAUSE,
> > > >  	RISCV_ISA_EXT_SSTC,
> > > >  	RISCV_ISA_EXT_SVINVAL,
> > > > +	RISCV_ISA_EXT_ZBB,
> > > 
> > > With ZIHINTPAUSE before SSTC and SVINIVAL I assume this is not something
> > > we are canonically ordering but I never, ever know which ones we are
> > > allowed to re-order at will.
> > 
> > I guess we could extend the comments with suitable hints,
> > which could help in the future.
> 
> Aye, for the likes of me that will never, ever remember I like the idea!

I'm 100% with you on this. I remember that this came up either with
svpbmt or zicbom in the past, but I still again forgot how the ordering
goes.

> 
> > > >  	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
> > > >  };
> > > 
> > > > 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 :-) .

The list is still too short to see where other extensions
are placed. I guess I need to find that stuff again about
extension ordering.


> > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > > index 026512ca9c4c..f19b9d4e2dca 100644
> > > > --- a/arch/riscv/kernel/cpufeature.c
> > > > +++ b/arch/riscv/kernel/cpufeature.c
> > > > @@ -201,6 +201,7 @@ void __init riscv_fill_hwcap(void)
> > > >  			} else {
> > > >  				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
> > > >  				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> > > > +				SET_ISA_EXT_MAP("zbb", RISCV_ISA_EXT_ZBB);
> > > >  				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);
> > > 
> > > This one looks like it is, sstc aside. Same question as above, can I
> > > reorder this one? I'll send a patch for it if I can...
> > 
> > hmm, I don't see the difference between cpu.c above
> > (..., svpbmt, zbb, zicbom, ...) and here
> > (..., svpbmt, zbb, zicbom, ...)
> 
> sstc appears last here but first in the cpu.c hunk above.
> 
> > > > diff --git a/arch/riscv/lib/strcmp_zbb.S b/arch/riscv/lib/strcmp_zbb.S
> > > > new file mode 100644
> > > > index 000000000000..aff9b941d3ee
> > > > --- /dev/null
> > > > +++ b/arch/riscv/lib/strcmp_zbb.S
> > > > @@ -0,0 +1,91 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +/*
> > > > + * Copyright (c) 2022 VRULL GmbH
> > > > + * Author: Christoph Muellner <christoph.muellner@vrull.eu>
> > > 
> > > Is a Co-developed-by: appropriate then?
> > 
> > I'd think so ... i.e. the assembly is from Christoph, but is originally
> > part of a pending glibc patchset, hence Christoph and me
> > decided on the co-developed thingy :-) .
> 
> Check your patch again, I don't see a Co-developed-by: tag. (That's what
> I was getting at, not the validity of "Author: Christoph...")

Now I remember, I talked with Christoph about that _after_ sending
this series. So my "git log" did show the Co-developed-by
all the time, which then confused me :-)


Heiko



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