| Message ID | 20240313172716.2325427-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/bitops: Reduce the mess, starting with ffs() | expand |
On 13/03/2024 5:27 pm, Andrew Cooper wrote: > xen/arch/arm/include/asm/bitops.h | 2 +- > xen/arch/ppc/include/asm/bitops.h | 1 - > xen/arch/x86/include/asm/bitops.h | 30 +++++++++++++----------------- > xen/common/bitops.c | 7 +++++++ > xen/include/xen/bitops.h | 12 ++++++++++++ > 5 files changed, 33 insertions(+), 19 deletions(-) > > diff --git a/xen/arch/arm/include/asm/bitops.h b/xen/arch/arm/include/asm/bitops.h > index 09c6064274a7..59ae8ed150b6 100644 > --- a/xen/arch/arm/include/asm/bitops.h > +++ b/xen/arch/arm/include/asm/bitops.h > @@ -158,7 +158,7 @@ static inline int fls(unsigned int x) > > > #define arch_ffs(x) ({ unsigned int __t = (x); fls(ISOLATE_LSB(__t)); }) > -#define ffsl(x) ({ unsigned long __t = (x); flsl(ISOLATE_LSB(__t)); }) > +#define arch_ffsl(x) ({ unsigned long __t = (x); flsl(ISOLATE_LSB(__t)); }) > > /** > * find_first_set_bit - find the first set bit in @word It turns out this change isn't bisectable on ARM, but it is by the end of the series. Reordering patches 6+7 to be ahead of this one resolves the bisection problem. ~Andrew
On 13/03/2024 5:27 pm, Andrew Cooper wrote: > diff --git a/xen/common/bitops.c b/xen/common/bitops.c > index 484df68768ad..eceffe5029d6 100644 > --- a/xen/common/bitops.c > +++ b/xen/common/bitops.c > @@ -40,6 +40,13 @@ static void test_ffs(void) > CHECK(ffs, 0, 0); > CHECK(ffs, 1, 1); > CHECK(ffs, 0x80000000U, 32); > + > + /* unsigned int ffsl(unsigned long) */ > + CHECK(ffsl, 0, 0); > + CHECK(ffsl, 1, 1); > + CHECK(ffsl, 1UL << (BITS_PER_LONG - 1), BITS_PER_LONG); > + if ( BITS_PER_LONG > 32 ) > + CHECK(ffsl, 1UL << 32, 33); > } This if() needs to be an #if to compile on arm32. Otherwise, I've managed to make the series fully bisectable on all architectures. ~Andrew
On 13/03/2024 5:48 pm, Andrew Cooper wrote: > On 13/03/2024 5:27 pm, Andrew Cooper wrote: >> xen/arch/arm/include/asm/bitops.h | 2 +- >> xen/arch/ppc/include/asm/bitops.h | 1 - >> xen/arch/x86/include/asm/bitops.h | 30 +++++++++++++----------------- >> xen/common/bitops.c | 7 +++++++ >> xen/include/xen/bitops.h | 12 ++++++++++++ >> 5 files changed, 33 insertions(+), 19 deletions(-) >> >> diff --git a/xen/arch/arm/include/asm/bitops.h b/xen/arch/arm/include/asm/bitops.h >> index 09c6064274a7..59ae8ed150b6 100644 >> --- a/xen/arch/arm/include/asm/bitops.h >> +++ b/xen/arch/arm/include/asm/bitops.h >> @@ -158,7 +158,7 @@ static inline int fls(unsigned int x) >> >> >> #define arch_ffs(x) ({ unsigned int __t = (x); fls(ISOLATE_LSB(__t)); }) >> -#define ffsl(x) ({ unsigned long __t = (x); flsl(ISOLATE_LSB(__t)); }) >> +#define arch_ffsl(x) ({ unsigned long __t = (x); flsl(ISOLATE_LSB(__t)); }) >> >> /** >> * find_first_set_bit - find the first set bit in @word > It turns out this change isn't bisectable on ARM, but it is by the end > of the series. > > Reordering patches 6+7 to be ahead of this one resolves the bisection > problem. ... but introduces a bisection issue on x86. I'm going to have to split the series up a bit more to do nicely. ~Andrew > > ~Andrew
diff --git a/xen/arch/arm/include/asm/bitops.h b/xen/arch/arm/include/asm/bitops.h index 09c6064274a7..59ae8ed150b6 100644 --- a/xen/arch/arm/include/asm/bitops.h +++ b/xen/arch/arm/include/asm/bitops.h @@ -158,7 +158,7 @@ static inline int fls(unsigned int x) #define arch_ffs(x) ({ unsigned int __t = (x); fls(ISOLATE_LSB(__t)); }) -#define ffsl(x) ({ unsigned long __t = (x); flsl(ISOLATE_LSB(__t)); }) +#define arch_ffsl(x) ({ unsigned long __t = (x); flsl(ISOLATE_LSB(__t)); }) /** * find_first_set_bit - find the first set bit in @word diff --git a/xen/arch/ppc/include/asm/bitops.h b/xen/arch/ppc/include/asm/bitops.h index 635a3b4e3e33..ecec2a826660 100644 --- a/xen/arch/ppc/include/asm/bitops.h +++ b/xen/arch/ppc/include/asm/bitops.h @@ -173,7 +173,6 @@ static inline int __test_and_clear_bit(int nr, volatile void *addr) #define flsl(x) generic_flsl(x) #define fls(x) generic_fls(x) -#define ffsl(x) ({ unsigned long t_ = (x); flsl(t_ & -t_); }) /* Based on linux/include/asm-generic/bitops/ffz.h */ /* diff --git a/xen/arch/x86/include/asm/bitops.h b/xen/arch/x86/include/asm/bitops.h index 2c5b103cbbd9..99342877e32f 100644 --- a/xen/arch/x86/include/asm/bitops.h +++ b/xen/arch/x86/include/asm/bitops.h @@ -413,23 +413,6 @@ static inline unsigned int find_first_set_bit(unsigned long word) return (unsigned int)word; } -/** - * ffs - find first bit set - * @x: the word to search - * - * This is defined the same way as the libc and compiler builtin ffs routines. - */ -static inline int ffsl(unsigned long x) -{ - long r; - - asm ( "bsf %1,%0\n\t" - "jnz 1f\n\t" - "mov $-1,%0\n" - "1:" : "=r" (r) : "rm" (x)); - return (int)r+1; -} - static inline unsigned int arch_ffs(unsigned int x) { int r = -1; @@ -448,6 +431,19 @@ static inline unsigned int arch_ffs(unsigned int x) } #define arch_ffs arch_ffs +static inline unsigned int arch_ffsl(unsigned long x) +{ + long r = -1; + + /* See arch_ffs() for safety discussion. */ + asm ( "bsf %[val], %[res]" + : [res] "+r" (r) + : [val] "rm" (x) ); + + return r + 1; +} +#define arch_ffsl arch_ffsl + /** * fls - find last bit set * @x: the word to search diff --git a/xen/common/bitops.c b/xen/common/bitops.c index 484df68768ad..eceffe5029d6 100644 --- a/xen/common/bitops.c +++ b/xen/common/bitops.c @@ -40,6 +40,13 @@ static void test_ffs(void) CHECK(ffs, 0, 0); CHECK(ffs, 1, 1); CHECK(ffs, 0x80000000U, 32); + + /* unsigned int ffsl(unsigned long) */ + CHECK(ffsl, 0, 0); + CHECK(ffsl, 1, 1); + CHECK(ffsl, 1UL << (BITS_PER_LONG - 1), BITS_PER_LONG); + if ( BITS_PER_LONG > 32 ) + CHECK(ffsl, 1UL << 32, 33); } static int __init cf_check test_bitops(void) diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h index fb3645d9cf87..a37b42342bc5 100644 --- a/xen/include/xen/bitops.h +++ b/xen/include/xen/bitops.h @@ -125,6 +125,18 @@ static always_inline __pure unsigned int ffs(unsigned int x) return arch_ffs(x); } +static always_inline __pure unsigned int ffsl(unsigned long x) +{ + if ( __builtin_constant_p(x) ) + return __builtin_ffsl(x); + +#ifndef arch_ffsl +#define arch_ffsl __builtin_ffsl +#endif + + return arch_ffsl(x); +} + /* --------------------- Please tidy below here --------------------- */ #ifndef find_next_bit
Exactly as per ffs() in the previous patch. Express the upper bound of the testing in terms of BITS_PER_LONG as it varies between architectures. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> CC: Shawn Anastasio <sanastasio@raptorengineering.com> CC: consulting@bugseng.com <consulting@bugseng.com> CC: Simone Ballarin <simone.ballarin@bugseng.com> CC: Federico Serafini <federico.serafini@bugseng.com> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/arch/arm/include/asm/bitops.h | 2 +- xen/arch/ppc/include/asm/bitops.h | 1 - xen/arch/x86/include/asm/bitops.h | 30 +++++++++++++----------------- xen/common/bitops.c | 7 +++++++ xen/include/xen/bitops.h | 12 ++++++++++++ 5 files changed, 33 insertions(+), 19 deletions(-)