Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
From:   "Wang, Xiao W" <xiao.w.wang@intel.com>
To:     Charlie Jenkins <charlie@rivosinc.com>
CC:     "paul.walmsley@sifive.com" <paul.walmsley@sifive.com>,
        "palmer@dabbelt.com" <palmer@dabbelt.com>,
        "aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
        "ardb@kernel.org" <ardb@kernel.org>,
        "anup@brainfault.org" <anup@brainfault.org>,
        "Li, Haicheng" <haicheng.li@intel.com>,
        "ajones@ventanamicro.com" <ajones@ventanamicro.com>,
        "Liu, Yujie" <yujie.liu@intel.com>,
        "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>,
        "linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v4 2/2] riscv: Optimize bitops with Zbb extension
Thread-Topic: [PATCH v4 2/2] riscv: Optimize bitops with Zbb extension
Thread-Index: AQHaCvp2pr7eekB1vU+a3i97SbKEQ7Biy64AgABNtHA=
Date:   Tue, 31 Oct 2023 01:53:55 +0000
Message-ID: <DM8PR11MB5751DE93BE278FF01576E5A5B8A0A@DM8PR11MB5751.namprd11.prod.outlook.com>
References: <20231030063904.2116277-1-xiao.w.wang@intel.com>
 <20231030063904.2116277-3-xiao.w.wang@intel.com> <ZUAT5gKXM+pU6r3w@ghost>
In-Reply-To: <ZUAT5gKXM+pU6r3w@ghost>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?Windows-1252?Q?nehiyFWjJErHhnNOPqDTHhprwHSYAYGbh8vf0ivG63xr5JWOXDDionGZ?=
 =?Windows-1252?Q?d3hAVpvZiipKobikLIsbu2c3DB/sSemD/UgBuey4jTLag0rumvQlbWF7?=
 =?Windows-1252?Q?YzEOiVhHiMT0vfScoRmBun7sSgT/lSSP6MSGHYgIBbnYL9ZStOARMJ7L?=
 =?Windows-1252?Q?ilWjKMsgyA2TWGbqbsr8KgD9wwjNCtBcYkGUNukUn/Pcp5jbCl+oXBKg?=
 =?Windows-1252?Q?0PUBlCGGNmmqHzqkdzJbEuJGL3hteXek4IlSfP3VRfamBhow0GSgfv6k?=
 =?Windows-1252?Q?Q8kUoP5NhGxwkIQa1uo3hhZ+on41ViAy4E0nQOXJ/osPChejo967k9Du?=
 =?Windows-1252?Q?d1O04w9GVEYpyDB3r/sp9P/60o0nOTfYcFYVX9bTnjdWvHFiMgqUls9P?=
 =?Windows-1252?Q?k1nvsBQqlpa9phF2tg8GebFR6OvZD99xDPv0QJleI4Bpjy/YOytQUoWN?=
 =?Windows-1252?Q?BRAojZpgFPhi7SXCXYuupLb6lgXp61Za7r7I4iwgHCdWwKySsXpaWvWk?=
 =?Windows-1252?Q?ewI6W4A7nvngVzQXmYF2tombJ0pn/PVU4hPPgV5yhKPYTCYUlKqRhjbD?=
 =?Windows-1252?Q?iWMIcpStUkBWXf9q/tckTIBTYiBWsVWr0d4DVEZSS8kPkl+99u+ZULXl?=
 =?Windows-1252?Q?ITfLB1tBRBucezd05+i5QwavC6TEtiR64/0xJz6ZqvMH4Xr9RlHm67pf?=
 =?Windows-1252?Q?sAefoe5bKSOQgL89XkKKeFxCIQbsmDmMFHMNecHNFiMZ7vpADkffxUCK?=
 =?Windows-1252?Q?dV/Vx8XGADPlvoL1om3wFQ8DRHY/C2YS493frv9PUSM3SXuIvaV2a6T/?=
 =?Windows-1252?Q?QICg3+o7pJycPehLEB8nbteR63F8KX5LX0XhwW5jpRZmNl9GzpBPSVXD?=
 =?Windows-1252?Q?v//yr++RzS93CYJpTMuabRV1YPrlDDfvIY+tcp1i2kMg99/V8SJzGEfM?=
 =?Windows-1252?Q?xvAJt8RdpH2Xy3z9E+gJRQpFZKCY1l6G2mK88eRniMXx7z/Ckg6Xu8hh?=
 =?Windows-1252?Q?redSNAe15wAJCt9eYB56sXgWf3leDEPAT45whbMf298MncZV7RHetwl0?=
 =?Windows-1252?Q?23iUhl9YZSXZshh4hPGUtbU3I1XMgVdZe8vQKEcBGpv/OEl2Id/+YjkT?=
 =?Windows-1252?Q?HhWz2mBhDyzUD2kVbV300CbwEA7o/VjULaR5dIsht/vEUQA5vhE+YQ5v?=
 =?Windows-1252?Q?CeEYlkFYUPgfHr0vThpbkeiigjvesXYRSR6phiLEoJRN+Vx5NNHqXuD7?=
 =?Windows-1252?Q?v6mbyijIPOA+B81fFDg0BExExLD4ixAvMyvIpB4mgmfmLCUFHsx+pMYx?=
 =?Windows-1252?Q?kKEtfxQ2zKcRaofAR4eNbymi4TwkXBysACT7uqxKLi1jCKX1qwXm/EV8?=
 =?Windows-1252?Q?IFtJzUSoyCsdLbxHtvcZADQomkIbwCxUu0Nulltn2qkZsGmbaiUClY/h?=
 =?Windows-1252?Q?8CXTXe/DjuHKBVatUXTtWUIaHk1YyCHMY4bem/s+WtbwusRunHThRpJQ?=
 =?Windows-1252?Q?IO9l7BthaEZmrv0qgf0Q9pLhTxDhulBcS0OMo9BbMCrtc/cn1GFkyNrw?=
 =?Windows-1252?Q?eEisZwicUQPIsSbmcsTjy6f3jax4S3CCvpP4C8z0CCvI14Yqz1yEKfiC?=
 =?Windows-1252?Q?qalvYODPgh+sEBFUnDcry5EZ6mQJjiy7SjTgDmWj7V1lwPiQ18W28fKE?=
 =?Windows-1252?Q?m1Q5e2iXg2cz+XX6KDoSfaLhbx5M4HB8?=
Content-Type: text/plain; charset="Windows-1252"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM8PR11MB5751.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: aa6bedf6-0ab5-4c80-3b8f-08dbd9b43d85
X-MS-Exchange-CrossTenant-originalarrivaltime: 31 Oct 2023 01:53:55.2375
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 9mAqZPlopUC087ph5ma/maeENBUTsnsBM/+aOM56YbYUL/QBg/18YATs7rvjPdfmCPKdnHrZSlYoasgAAeCRAQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5461
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-1.3 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Mon, 30 Oct 2023 18:54:28 -0700 (PDT)


> -----Original Message-----
> From: Charlie Jenkins <charlie@rivosinc.com>
> Sent: Tuesday, October 31, 2023 4:37 AM
> To: Wang, Xiao W <xiao.w.wang@intel.com>
> Cc: paul.walmsley@sifive.com; palmer@dabbelt.com;
> aou@eecs.berkeley.edu; ardb@kernel.org; anup@brainfault.org; Li, Haicheng
> <haicheng.li@intel.com>; ajones@ventanamicro.com; Liu, Yujie
> <yujie.liu@intel.com>; linux-riscv@lists.infradead.org; linux-
> efi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v4 2/2] riscv: Optimize bitops with Zbb extension
>=20
> On Mon, Oct 30, 2023 at 02:39:04PM +0800, Xiao Wang wrote:
> > This patch leverages the alternative mechanism to dynamically optimize
> > bitops (including __ffs, __fls, ffs, fls) with Zbb instructions. When
> > Zbb ext is not supported by the runtime CPU, legacy implementation is
> > used. If Zbb is supported, then the optimized variants will be selected
> > via alternative patching.
> >
> > The legacy bitops support is taken from the generic C implementation as
> > fallback.
> >
> > If the parameter is a build-time constant, we leverage compiler builtin=
 to
> > calculate the result directly, this approach is inspired by x86 bitops
> > implementation.
> >
> > EFI stub runs before the kernel, so alternative mechanism should not be
> > used there, this patch introduces a macro NO_ALTERNATIVE for this
> purpose.
> >
> > Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> > ---
> >  arch/riscv/include/asm/bitops.h       | 255 +++++++++++++++++++++++++-
> >  drivers/bitopstest/Kconfig            |   1 +
> >  drivers/firmware/efi/libstub/Makefile |   2 +-
> >  3 files changed, 254 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/bitops.h
> b/arch/riscv/include/asm/bitops.h
> > index 3540b690944b..ef35c9ebc2ed 100644
> > --- a/arch/riscv/include/asm/bitops.h
> > +++ b/arch/riscv/include/asm/bitops.h
> > @@ -15,13 +15,262 @@
> >  #include <asm/barrier.h>
> >  #include <asm/bitsperlong.h>
> >
> > +#if !defined(CONFIG_RISCV_ISA_ZBB) || defined(NO_ALTERNATIVE)
> >  #include <asm-generic/bitops/__ffs.h>
> > -#include <asm-generic/bitops/ffz.h>
> > -#include <asm-generic/bitops/fls.h>
> >  #include <asm-generic/bitops/__fls.h>
> > +#include <asm-generic/bitops/ffs.h>
> > +#include <asm-generic/bitops/fls.h>
> > +
> > +#else
> > +#include <asm/alternative-macros.h>
> > +#include <asm/hwcap.h>
> > +
> > +#if (BITS_PER_LONG =3D=3D 64)
> > +#define CTZW	"ctzw "
> > +#define CLZW	"clzw "
> > +#elif (BITS_PER_LONG =3D=3D 32)
> > +#define CTZW	"ctz "
> > +#define CLZW	"clz "
> > +#else
> > +#error "Unexpected BITS_PER_LONG"
> > +#endif
> > +
> > +static __always_inline unsigned long variable__ffs(unsigned long word)
> > +{
> > +	int num;
> > +
> > +	asm_volatile_goto(
> > +		ALTERNATIVE("j %l[legacy]", "nop", 0, RISCV_ISA_EXT_ZBB, 1)
> > +		: : : : legacy);
> > +
>=20
> On this and following asm blocks, checkpatch outputs:  "Lines should not
> end with a '('".

I did below check, but I got no warning.
# ./scripts/checkpatch.pl v4-0002-riscv-Optimize-bitops-with-Zbb-extension.=
patch
total: 0 errors, 0 warnings, 280 lines checked
May I know how you do the check?
BTW, I see arch/riscv/include/asm/jump_label.h and arch/riscv/include/asm/c=
pufeature.h have similar code.

>=20
> > +	asm volatile (
> > +		".option push\n"
> > +		".option arch,+zbb\n"
> > +		"ctz %0, %1\n"
> > +		".option pop\n"
> > +		: "=3Dr" (word) : "r" (word) :);
> > +
> > +	return word;
> > +
> > +legacy:
> > +	num =3D 0;
> > +#if BITS_PER_LONG =3D=3D 64
> > +	if ((word & 0xffffffff) =3D=3D 0) {
> > +		num +=3D 32;
> > +		word >>=3D 32;
> > +	}
> > +#endif
> > +	if ((word & 0xffff) =3D=3D 0) {
> > +		num +=3D 16;
> > +		word >>=3D 16;
> > +	}
> > +	if ((word & 0xff) =3D=3D 0) {
> > +		num +=3D 8;
> > +		word >>=3D 8;
> > +	}
> > +	if ((word & 0xf) =3D=3D 0) {
> > +		num +=3D 4;
> > +		word >>=3D 4;
> > +	}
> > +	if ((word & 0x3) =3D=3D 0) {
> > +		num +=3D 2;
> > +		word >>=3D 2;
> > +	}
> > +	if ((word & 0x1) =3D=3D 0)
> > +		num +=3D 1;
> > +	return num;
> > +}
> > +
> > +/**
> > + * __ffs - find first set bit in a long word
> > + * @word: The word to search
> > + *
> > + * Undefined if no set bit exists, so code should check against 0 firs=
t.
> > + */
> > +#define __ffs(word)				\
> > +	(__builtin_constant_p(word) ?		\
> > +	 (unsigned long)__builtin_ctzl(word) :	\
> > +	 variable__ffs(word))
> > +
> > +static __always_inline unsigned long variable__fls(unsigned long word)
> > +{
> > +	int num;
> > +
> > +	asm_volatile_goto(
> > +		ALTERNATIVE("j %l[legacy]", "nop", 0, RISCV_ISA_EXT_ZBB, 1)
> > +		: : : : legacy);
> > +
> > +	asm volatile (
> > +		".option push\n"
> > +		".option arch,+zbb\n"
> > +		"clz %0, %1\n"
> > +		".option pop\n"
> > +		: "=3Dr" (word) : "r" (word) :);
> > +
> > +	return BITS_PER_LONG - 1 - word;
> > +
> > +legacy:
> > +	num =3D BITS_PER_LONG - 1;
> > +#if BITS_PER_LONG =3D=3D 64
> > +	if (!(word & (~0ul << 32))) {
> > +		num -=3D 32;
> > +		word <<=3D 32;
> > +	}
> > +#endif
> > +	if (!(word & (~0ul << (BITS_PER_LONG-16)))) {
> > +		num -=3D 16;
> > +		word <<=3D 16;
> > +	}
> > +	if (!(word & (~0ul << (BITS_PER_LONG-8)))) {
> > +		num -=3D 8;
> > +		word <<=3D 8;
> > +	}
> > +	if (!(word & (~0ul << (BITS_PER_LONG-4)))) {
> > +		num -=3D 4;
> > +		word <<=3D 4;
> > +	}
> > +	if (!(word & (~0ul << (BITS_PER_LONG-2)))) {
> > +		num -=3D 2;
> > +		word <<=3D 2;
> > +	}
> > +	if (!(word & (~0ul << (BITS_PER_LONG-1))))
> > +		num -=3D 1;
> > +	return num;
> > +}
> > +
> > +/**
> > + * __fls - find last set bit in a long word
> > + * @word: the word to search
> > + *
> > + * Undefined if no set bit exists, so code should check against 0 firs=
t.
> > + */
> > +#define __fls(word)							\
> > +	(__builtin_constant_p(word) ?					\
> > +	 (unsigned long)(BITS_PER_LONG - 1 - __builtin_clzl(word)) :	\
> > +	 variable__fls(word))
> > +
> > +static __always_inline int variable_ffs(int x)
> > +{
> > +	int r;
> > +
> > +	if (!x)
> > +		return 0;
> > +
> > +	asm_volatile_goto(
> > +		ALTERNATIVE("j %l[legacy]", "nop", 0, RISCV_ISA_EXT_ZBB, 1)
> > +		: : : : legacy);
> > +
> > +	asm volatile (
> > +		".option push\n"
> > +		".option arch,+zbb\n"
> > +		CTZW "%0, %1\n"
> > +		".option pop\n"
> > +		: "=3Dr" (r) : "r" (x) :);
> > +
> > +	return r + 1;
> > +
> > +legacy:
> > +	r =3D 1;
> > +	if (!(x & 0xffff)) {
> > +		x >>=3D 16;
> > +		r +=3D 16;
> > +	}
> > +	if (!(x & 0xff)) {
> > +		x >>=3D 8;
> > +		r +=3D 8;
> > +	}
> > +	if (!(x & 0xf)) {
> > +		x >>=3D 4;
> > +		r +=3D 4;
> > +	}
> > +	if (!(x & 3)) {
> > +		x >>=3D 2;
> > +		r +=3D 2;
> > +	}
> > +	if (!(x & 1)) {
> > +		x >>=3D 1;
> > +		r +=3D 1;
> > +	}
> > +	return r;
> > +}
> > +
> > +/**
> > + * ffs - find first set bit in a word
> > + * @x: the word to search
> > + *
> > + * This is defined the same way as the libc and compiler builtin ffs r=
outines.
> > + *
> > + * ffs(value) returns 0 if value is 0 or the position of the first set=
 bit if
> > + * value is nonzero. The first (least significant) bit is at position =
1.
> > + */
> > +#define ffs(x) (__builtin_constant_p(x) ? __builtin_ffs(x) : variable_=
ffs(x))
> > +
> > +static __always_inline int variable_fls(unsigned int x)
> > +{
> > +	int r;
> > +
> > +	if (!x)
> > +		return 0;
> > +
> > +	asm_volatile_goto(
> > +		ALTERNATIVE("j %l[legacy]", "nop", 0, RISCV_ISA_EXT_ZBB, 1)
> > +		: : : : legacy);
> > +
> > +	asm volatile (
> > +		".option push\n"
> > +		".option arch,+zbb\n"
> > +		CLZW "%0, %1\n"
> > +		".option pop\n"
> > +		: "=3Dr" (r) : "r" (x) :);
> > +
> > +	return 32 - r;
> > +
> > +legacy:
> > +	r =3D 32;
> > +	if (!(x & 0xffff0000u)) {
> > +		x <<=3D 16;
> > +		r -=3D 16;
> > +	}
> > +	if (!(x & 0xff000000u)) {
> > +		x <<=3D 8;
> > +		r -=3D 8;
> > +	}
> > +	if (!(x & 0xf0000000u)) {
> > +		x <<=3D 4;
> > +		r -=3D 4;
> > +	}
> > +	if (!(x & 0xc0000000u)) {
> > +		x <<=3D 2;
> > +		r -=3D 2;
> > +	}
> > +	if (!(x & 0x80000000u)) {
> > +		x <<=3D 1;
> > +		r -=3D 1;
> > +	}
> > +	return r;
> > +}
> > +
> > +/**
> > + * fls - find last set bit in a word
> > + * @x: the word to search
> > + *
> > + * This is defined in a similar way as ffs, but returns the position o=
f the most
> > + * significant set bit.
> > + *
> > + * fls(value) returns 0 if value is 0 or the position of the last set =
bit if
> > + * value is nonzero. The last (most significant) bit is at position 32=
.
> > + */
> > +#define fls(x)
> 	\
> > +	(__builtin_constant_p(x) ?					\
> > +	 (int)(((x) !=3D 0) ?						\
> > +	  (sizeof(unsigned int) * 8 - __builtin_clz(x)) : 0) :		\
> > +	 variable_fls(x))
> > +
>=20
> Checkpath complains: "Macro argument reuse 'x' - possible side-effects"
>=20

Ditto.

> > +#endif /* !defined(CONFIG_RISCV_ISA_ZBB) || defined(NO_ALTERNATIVE)
> */
> > +
> > +#include <asm-generic/bitops/ffz.h>
> >  #include <asm-generic/bitops/fls64.h>
> >  #include <asm-generic/bitops/sched.h>
> > -#include <asm-generic/bitops/ffs.h>
> >
> >  #include <asm-generic/bitops/hweight.h>
> >
> > diff --git a/drivers/bitopstest/Kconfig b/drivers/bitopstest/Kconfig
> > index d0e2af4b801e..6ef6dcd41d49 100644
> > --- a/drivers/bitopstest/Kconfig
> > +++ b/drivers/bitopstest/Kconfig
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  menuconfig BITOPSTEST
> >  	tristate "self test for bitops optimization"
> > +	default y
> >  	help
> >  	  Enable this to test the bitops APIs.
>=20
> Is this a test you wanted to add? The source code isn't included.

Sorry, I mistakenly did a "git add" for my local test. Will drop it.

BRs,
Xiao

>=20
> - Charlie
>=20
> >
> > diff --git a/drivers/firmware/efi/libstub/Makefile
> b/drivers/firmware/efi/libstub/Makefile
> > index a1157c2a7170..d68cacd4e3af 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -28,7 +28,7 @@ cflags-$(CONFIG_ARM)		+=3D -
> DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
> >  				   -DEFI_HAVE_MEMCHR -
> DEFI_HAVE_STRRCHR \
> >  				   -DEFI_HAVE_STRCMP -fno-builtin -fpic \
> >  				   $(call cc-option,-mno-single-pic-base)
> > -cflags-$(CONFIG_RISCV)		+=3D -fpic
> > +cflags-$(CONFIG_RISCV)		+=3D -fpic -DNO_ALTERNATIVE
> >  cflags-$(CONFIG_LOONGARCH)	+=3D -fpie
> >
> >  cflags-$(CONFIG_EFI_PARAMS_FROM_FDT)	+=3D -
> I$(srctree)/scripts/dtc/libfdt
> > --
> > 2.25.1
> >