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

[Conor Dooley <conor@kernel.org>]

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!

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

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

> > > +	/* Main loop for aligned string.  */
> > > +	.p2align 3
> > > +1:
> > > +	REG_L	data1, 0(src1)
> > > +	REG_L	data2, 0(src2)
> > > +	orc.b	data1_orcb, data1
> > > +	bne	data1_orcb, m1, 2f
> > > +	addi	src1, src1, SZREG
> > > +	addi	src2, src2, SZREG
> > > +	beq	data1, data2, 1b
> > > +
> > > +	/* Words don't match, and no null byte in the first
> > > +	 * word. Get bytes in big-endian order and compare.  */
> > > +#ifndef CONFIG_CPU_BIG_ENDIAN
> > 
> > I know this is a lift from the reference implementation in the spec, but
> > do we actually need this ifndef section?
> 
> I don't know, but _if_ big endian support comes in the future,
> having one place less to break also might be helpful? :-)

One less place to have to go and fix it up, but I hope it never comes to
pass! And no harm is being close to the one in the spec...


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