Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()
On 06.05.2024 10:16, Oleksii wrote: > On Mon, 2024-05-06 at 08:33 +0200, Jan Beulich wrote: >> On 03.05.2024 19:15, Oleksii wrote: >>> On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: > #include > > +#ifndef arch_check_bitop_size > +#define arch_check_bitop_size(addr) Can this really do nothing? Passing the address of an object smaller than bitop_uint_t will read past the object in the generic__*_bit() functions. >>> It seems RISC-V isn' happy with the following generic definition: >>> extern void __bitop_bad_size(void); >>> >>> /* - Please tidy above here >>> >>> - */ >>> >>> #include >>> >>> #ifndef arch_check_bitop_size >>> >>> #define bitop_bad_size(addr) sizeof(*(addr)) < >>> sizeof(bitop_uint_t) >>> >>> #define arch_check_bitop_size(addr) \ >>> if ( bitop_bad_size(addr) ) __bitop_bad_size(); >>> >>> #endif /* arch_check_bitop_size */ >> >> I'm afraid you've re-discovered something that was also found during >> the >> original Arm porting effort. As nice and logical as it would (seem >> to) be >> to have bitop_uint_t match machine word size, there are places ... >> >>> The following errors occurs. bitop_uint_t for RISC-V is defined as >>> unsigned long for now: >> >> ... where we assume such operations can be done on 32-bit quantities. > Based on RISC-V spec machine word is 32-bit, so then I can just drop > re-definition of bitop_uint_t in riscv/asm/types.h and use the > definition of bitop_uint_t in xen/types.h. > Also it will be needed to update __AMO() macros in /asm/bitops.h > in the following way: >#if BITOP_BITS_PER_WORD == 64 >#define __AMO(op) "amo" #op ".d" >#elif BITOP_BITS_PER_WORD == 32 >#define __AMO(op) "amo" #op ".w" >#else >#error "Unexpected BITS_PER_LONG" >#endif > Note: BITS_PER_LONG was changed to BITOP_BITS_PER_WORD ! > > Only one question remains for me. Given that there are some operations > whichcan be performed on 32-bit quantities, it seems to me that bitop_uint_t > can only be uint32_t. > Am I correct? If yes, do we need to have ability to redefine > bitop_uint_t and BITOP_BITS_PER_WORD in xen/types.h: >#ifndef BITOP_TYPE >#define BITOP_BITS_PER_WORD 32 > >typedef uint32_t bitop_uint_t; >#endif Probably not, right now. Hence me having said "As nice and logical as it would (seem to) be" in the earlier reply. Jan
Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()
On Mon, 2024-05-06 at 08:33 +0200, Jan Beulich wrote: > On 03.05.2024 19:15, Oleksii wrote: > > On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: > > > > #include > > > > > > > > +#ifndef arch_check_bitop_size > > > > +#define arch_check_bitop_size(addr) > > > > > > Can this really do nothing? Passing the address of an object > > > smaller > > > than > > > bitop_uint_t will read past the object in the generic__*_bit() > > > functions. > > It seems RISC-V isn' happy with the following generic definition: > > extern void __bitop_bad_size(void); > > > > /* - Please tidy above here > > > > - */ > > > > #include > > > > #ifndef arch_check_bitop_size > > > > #define bitop_bad_size(addr) sizeof(*(addr)) < > > sizeof(bitop_uint_t) > > > > #define arch_check_bitop_size(addr) \ > > if ( bitop_bad_size(addr) ) __bitop_bad_size(); > > > > #endif /* arch_check_bitop_size */ > > I'm afraid you've re-discovered something that was also found during > the > original Arm porting effort. As nice and logical as it would (seem > to) be > to have bitop_uint_t match machine word size, there are places ... > > > The following errors occurs. bitop_uint_t for RISC-V is defined as > > unsigned long for now: > > ... where we assume such operations can be done on 32-bit quantities. Based on RISC-V spec machine word is 32-bit, so then I can just drop re-definition of bitop_uint_t in riscv/asm/types.h and use the definition of bitop_uint_t in xen/types.h. Also it will be needed to update __AMO() macros in /asm/bitops.h in the following way: #if BITOP_BITS_PER_WORD == 64 #define __AMO(op) "amo" #op ".d" #elif BITOP_BITS_PER_WORD == 32 #define __AMO(op) "amo" #op ".w" #else #error "Unexpected BITS_PER_LONG" #endif Note: BITS_PER_LONG was changed to BITOP_BITS_PER_WORD ! Only one question remains for me. Given that there are some operations whichcan be performed on 32-bit quantities, it seems to me that bitop_uint_t can only be uint32_t. Am I correct? If yes, do we need to have ability to redefine bitop_uint_t and BITOP_BITS_PER_WORD in xen/types.h: #ifndef BITOP_TYPE #define BITOP_BITS_PER_WORD 32 typedef uint32_t bitop_uint_t; #endif ~ Oleksii > > Jan > > > ./common/symbols-dummy.o -o ./.xen-syms.0 > > riscv64-linux-gnu-ld: prelink.o: in function `atomic_sub': > > /build/xen/./arch/riscv/include/asm/atomic.h:152: undefined > > reference > > to `__bitop_bad_size' > > riscv64-linux-gnu-ld: prelink.o: in function > > `evtchn_check_pollers': > > /build/xen/common/event_channel.c:1531: undefined reference to > > `__bitop_bad_size' > > riscv64-linux-gnu-ld: /build/xen/common/event_channel.c:1521: > > undefined > > reference to `__bitop_bad_size' > > riscv64-linux-gnu-ld: prelink.o: in function `evtchn_init': > > /build/xen/common/event_channel.c:1541: undefined reference to > > `__bitop_bad_size' > > riscv64-linux-gnu-ld: prelink.o: in function `_read_lock': > > /build/xen/./include/xen/rwlock.h:94: undefined reference to > > `__bitop_bad_size' > > riscv64-linux-gnu-ld: > > prelink.o:/build/xen/./arch/riscv/include/asm/atomic.h:195: more > > undefined references to `__bitop_bad_size' follow > > riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol > > `__bitop_bad_size' > > isn't defined > > riscv64-linux-gnu-ld: final link failed: bad value > > make[2]: *** [arch/riscv/Makefile:15: xen-syms] Error 1 > > > > ~ Oleksii >
Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()
On 03.05.2024 19:15, Oleksii wrote: > On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: >>> #include >>> >>> +#ifndef arch_check_bitop_size >>> +#define arch_check_bitop_size(addr) >> >> Can this really do nothing? Passing the address of an object smaller >> than >> bitop_uint_t will read past the object in the generic__*_bit() >> functions. > It seems RISC-V isn' happy with the following generic definition: >extern void __bitop_bad_size(void); > >/* - Please tidy above here >- */ > >#include > >#ifndef arch_check_bitop_size > >#define bitop_bad_size(addr) sizeof(*(addr)) < sizeof(bitop_uint_t) > >#define arch_check_bitop_size(addr) \ >if ( bitop_bad_size(addr) ) __bitop_bad_size(); > >#endif /* arch_check_bitop_size */ I'm afraid you've re-discovered something that was also found during the original Arm porting effort. As nice and logical as it would (seem to) be to have bitop_uint_t match machine word size, there are places ... > The following errors occurs. bitop_uint_t for RISC-V is defined as > unsigned long for now: ... where we assume such operations can be done on 32-bit quantities. Jan > ./common/symbols-dummy.o -o ./.xen-syms.0 > riscv64-linux-gnu-ld: prelink.o: in function `atomic_sub': > /build/xen/./arch/riscv/include/asm/atomic.h:152: undefined reference > to `__bitop_bad_size' > riscv64-linux-gnu-ld: prelink.o: in function `evtchn_check_pollers': > /build/xen/common/event_channel.c:1531: undefined reference to > `__bitop_bad_size' > riscv64-linux-gnu-ld: /build/xen/common/event_channel.c:1521: undefined > reference to `__bitop_bad_size' > riscv64-linux-gnu-ld: prelink.o: in function `evtchn_init': > /build/xen/common/event_channel.c:1541: undefined reference to > `__bitop_bad_size' > riscv64-linux-gnu-ld: prelink.o: in function `_read_lock': > /build/xen/./include/xen/rwlock.h:94: undefined reference to > `__bitop_bad_size' > riscv64-linux-gnu-ld: > prelink.o:/build/xen/./arch/riscv/include/asm/atomic.h:195: more > undefined references to `__bitop_bad_size' follow > riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol `__bitop_bad_size' > isn't defined > riscv64-linux-gnu-ld: final link failed: bad value > make[2]: *** [arch/riscv/Makefile:15: xen-syms] Error 1 > > ~ Oleksii
Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()
On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: > > #include > > > > +#ifndef arch_check_bitop_size > > +#define arch_check_bitop_size(addr) > > Can this really do nothing? Passing the address of an object smaller > than > bitop_uint_t will read past the object in the generic__*_bit() > functions. It seems RISC-V isn' happy with the following generic definition: extern void __bitop_bad_size(void); /* - Please tidy above here - */ #include #ifndef arch_check_bitop_size #define bitop_bad_size(addr) sizeof(*(addr)) < sizeof(bitop_uint_t) #define arch_check_bitop_size(addr) \ if ( bitop_bad_size(addr) ) __bitop_bad_size(); #endif /* arch_check_bitop_size */ The following errors occurs. bitop_uint_t for RISC-V is defined as unsigned long for now: ./common/symbols-dummy.o -o ./.xen-syms.0 riscv64-linux-gnu-ld: prelink.o: in function `atomic_sub': /build/xen/./arch/riscv/include/asm/atomic.h:152: undefined reference to `__bitop_bad_size' riscv64-linux-gnu-ld: prelink.o: in function `evtchn_check_pollers': /build/xen/common/event_channel.c:1531: undefined reference to `__bitop_bad_size' riscv64-linux-gnu-ld: /build/xen/common/event_channel.c:1521: undefined reference to `__bitop_bad_size' riscv64-linux-gnu-ld: prelink.o: in function `evtchn_init': /build/xen/common/event_channel.c:1541: undefined reference to `__bitop_bad_size' riscv64-linux-gnu-ld: prelink.o: in function `_read_lock': /build/xen/./include/xen/rwlock.h:94: undefined reference to `__bitop_bad_size' riscv64-linux-gnu-ld: prelink.o:/build/xen/./arch/riscv/include/asm/atomic.h:195: more undefined references to `__bitop_bad_size' follow riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol `__bitop_bad_size' isn't defined riscv64-linux-gnu-ld: final link failed: bad value make[2]: *** [arch/riscv/Makefile:15: xen-syms] Error 1 ~ Oleksii
Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()
On 26.04.2024 10:14, Oleksii wrote: > On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: >>> /* - Please tidy above here - >>> */ >>> >>> #include >>> >>> +#ifndef arch_check_bitop_size >>> +#define arch_check_bitop_size(addr) >> >> Can this really do nothing? Passing the address of an object smaller >> than >> bitop_uint_t will read past the object in the generic__*_bit() >> functions. > Agree, in generic case it would be better to add: > #define arch_check_bitop_size(addr) (sizeof(*(addr)) < > sizeof(bitop_uint_t)) At which point x86 won't need any special casing anymore, I think. Jan
Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()
On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: > > /* - Please tidy above here - > > */ > > > > #include > > > > +#ifndef arch_check_bitop_size > > +#define arch_check_bitop_size(addr) > > Can this really do nothing? Passing the address of an object smaller > than > bitop_uint_t will read past the object in the generic__*_bit() > functions. Agree, in generic case it would be better to add: #define arch_check_bitop_size(addr) (sizeof(*(addr)) < sizeof(bitop_uint_t)) Originally, it was defined as empty becuase majority of supported architectures by Xen don't do this check and I decided to use this definition as generic. Thanks. ~ Oleksii
Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()
On 17.04.2024 12:04, Oleksii Kurochko wrote:
> --- a/xen/arch/ppc/include/asm/page.h
> +++ b/xen/arch/ppc/include/asm/page.h
> @@ -4,7 +4,7 @@
>
> #include
>
> -#include
> +#include
> #include
This wants to move up into the xen/*.h group then, like you have done ...
> --- a/xen/arch/ppc/mm-radix.c
> +++ b/xen/arch/ppc/mm-radix.c
> @@ -1,11 +1,11 @@
> /* SPDX-License-Identifier: GPL-2.0-or-later */
> +#include
> #include
> #include
> #include
> #include
> #include
>
> -#include
> #include
> #include
> #include
.. e.g. here.
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -65,10 +65,137 @@ static inline int generic_flsl(unsigned long x)
> * scope
> */
>
> +#define BITOP_MASK(nr) ((bitop_uint_t)1 << ((nr) % BITOP_BITS_PER_WORD))
> +
> +#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD)
> +
> /* - Please tidy above here - */
>
> #include
>
> +#ifndef arch_check_bitop_size
> +#define arch_check_bitop_size(addr)
Can this really do nothing? Passing the address of an object smaller than
bitop_uint_t will read past the object in the generic__*_bit() functions.
> +#endif
> +
> +/**
> + * generic__test_and_set_bit - Set a bit and return its old value
> + * @nr: Bit to set
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic and can be reordered.
> + * If two examples of this operation race, one can appear to succeed
> + * but actually fail. You must protect multiple accesses with a lock.
> + */
> +static always_inline bool
> +generic__test_and_set_bit(unsigned long nr, volatile void *addr)
> +{
> +bitop_uint_t mask = BITOP_MASK(nr);
> +volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) +
> BITOP_WORD(nr);
The revision log suggests excess parentheses were dropped from such cast
expressions.
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -64,6 +64,11 @@ typedef __u64 __be64;
>
> typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t;
>
> +#ifndef BITOP_TYPE
> +#define BITOP_BITS_PER_WORD 32
> +typedef uint32_t bitop_uint_t;
Personally I find this indentation odd / misleading. For pre-processor
directives the # preferrably remains first on a line (as was iirc
demanded by earlier C standards), followed by one or more blanks if so
desired. File-scope declarations imo should never be indented.
Jan
[PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()
The following generic functions were introduced:
* test_bit
* generic__test_and_set_bit
* generic__test_and_clear_bit
* generic__test_and_change_bit
Also, the patch introduces the following generics which are
used by the functions mentioned above:
* BITOP_BITS_PER_WORD
* BITOP_MASK
* BITOP_WORD
* BITOP_TYPE
These functions and macros can be useful for architectures
that don't have corresponding arch-specific instructions.
Because of that x86 has the following check in the macros test_bit(),
__test_and_set_bit(), __test_and_clear_bit(), __test_and_change_bit():
if ( bitop_bad_size(addr) ) __bitop_bad_size();
It was necessary to make bitop bad size check generic too, so
arch_check_bitop_size() was introduced and defined as empty for other
architectures except x86.
Signed-off-by: Oleksii Kurochko
---
The context ("* Find First Set bit. Bits are labelled from 1." in
xen/bitops.h )
suggests there's a dependency on an uncommitted patch. It happens becuase
the current patch
series is based on Andrew's patch series (
https://lore.kernel.org/xen-devel/20240313172716.2325427-1-andrew.coop...@citrix.com/T/#t
),
but if everything is okay with the current one patch it can be merged
without Andrew's patch series being merged.
---
Changes in V8:
- drop __pure for function which uses volatile.
- drop unnessary () in generic__test_and_change_bit() for addr casting.
- update prototype of generic_test_bit() and test_bit(): now it returns bool
instead of int.
- update generic_test_bit() to use BITOP_MASK().
- Deal with fls{l} changes: it should be in the patch with introduced generic
fls{l}.
- add a footer with explanation of dependency on an uncommitted patch after
Signed-off.
- abstract bitop_size().
- move BITOP_TYPE define to .
---
Changes in V7:
- move everything to xen/bitops.h to follow the same approach for all generic
bit ops.
- put together BITOP_BITS_PER_WORD and bitops_uint_t.
- make BITOP_MASK more generic.
- drop #ifdef ... #endif around BITOP_MASK, BITOP_WORD as they are generic
enough.
- drop "_" for generic__{test_and_set_bit,...}().
- drop " != 0" for functions which return bool.
- add volatile during the cast for generic__{...}().
- update the commit message.
- update arch related code to follow the proposed generic approach.
---
Changes in V6:
- Nothing changed ( only rebase )
---
Changes in V5:
- new patch
---
xen/arch/arm/arm64/livepatch.c| 1 -
xen/arch/arm/include/asm/bitops.h | 67
xen/arch/ppc/include/asm/bitops.h | 52
xen/arch/ppc/include/asm/page.h | 2 +-
xen/arch/ppc/mm-radix.c | 2 +-
xen/arch/x86/include/asm/bitops.h | 30 +++
xen/include/xen/bitops.h | 127 ++
xen/include/xen/types.h | 5 ++
8 files changed, 145 insertions(+), 141 deletions(-)
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index df2cebedde..4bc8ed9be5 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -10,7 +10,6 @@
#include
#include
-#include
#include
#include
#include
diff --git a/xen/arch/arm/include/asm/bitops.h
b/xen/arch/arm/include/asm/bitops.h
index 5104334e48..8e16335e76 100644
--- a/xen/arch/arm/include/asm/bitops.h
+++ b/xen/arch/arm/include/asm/bitops.h
@@ -22,9 +22,6 @@
#define __set_bit(n,p)set_bit(n,p)
#define __clear_bit(n,p) clear_bit(n,p)
-#define BITOP_BITS_PER_WORD 32
-#define BITOP_MASK(nr) (1UL << ((nr) % BITOP_BITS_PER_WORD))
-#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD)
#define BITS_PER_BYTE 8
#define ADDR (*(volatile int *) addr)
@@ -76,70 +73,6 @@ bool test_and_change_bit_timeout(int nr, volatile void *p,
bool clear_mask16_timeout(uint16_t mask, volatile void *p,
unsigned int max_try);
-/**
- * __test_and_set_bit - Set a bit and return its old value
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This operation is non-atomic and can be reordered.
- * If two examples of this operation race, one can appear to succeed
- * but actually fail. You must protect multiple accesses with a lock.
- */
-static inline int __test_and_set_bit(int nr, volatile void *addr)
-{
-unsigned int mask = BITOP_MASK(nr);
-volatile unsigned int *p =
-((volatile unsigned int *)addr) + BITOP_WORD(nr);
-unsigned int old = *p;
-
-*p = old | mask;
-return (old & mask) != 0;
-}
-
-/**
- * __test_and_clear_bit - Clear a bit and return its old value
- * @nr: Bit to clear
- * @addr: Address to count from
- *
- * This operation is non-atomic and can be reordered.
- * If two examples of this operation race, one can appear to succeed
- * but actually fail. You must protect multiple accesses with a lock.
- */
-static inline int __test_and_clear_bit(int nr, volatile void *addr)
-{
-unsigned int mask =
