| Message ID | ff6922206ab5476df907e2a05255663865f07301.1712137031.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
On 03.04.2024 12:19, Oleksii Kurochko wrote: > The patch introduces the following generic functions: > * 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 move the check to generic code and define as 0 > for other architectures as they do not require this check. Hmm, yes, the checks need to be in the outermost wrapper macros. While you're abstracting other stuff to arch_*(), wouldn't it make sense to also abstract this to e.g. arch_check_bitop_size(), with the expansion simply being (effectively) empty in the generic fallback case? > --- a/xen/include/xen/bitops.h > +++ b/xen/include/xen/bitops.h > @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned long x) > * scope > */ > > +#define BITOP_BITS_PER_WORD 32 > +/* typedef uint32_t bitop_uint_t; */ > +#define bitop_uint_t uint32_t So no arch overrides permitted anymore at all? > +#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 <asm/bitops.h> > > +#ifndef bitop_bad_size > +extern void __bitop_bad_size(void); If not switching to arch_check_bitop_size() or alike as suggested above, why exactly does this need duplicating here and in x86? Can't the decl simply move ahead of the #include right above? (Sure, this will then require that nothing needing any of the functions you move here would still include asm/bitops.h; it would need to be xen/bitops.h everywhere.) > +#define bitop_bad_size(addr) 0 > +#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 __pure bool > +generic__test_and_set_bit(unsigned long nr, volatile void *addr) Does __pure actually fit with the use of volatile? The former says multiple accesses may be folded; the latter says they must not be. > +{ > + bitop_uint_t mask = BITOP_MASK(nr); > + volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + BITOP_WORD(nr); Nit: Slightly shorter line possible: volatile bitop_uint_t *p = (volatile bitop_uint_t *)addr + BITOP_WORD(nr); > + bitop_uint_t old = *p; > + > + *p = old | mask; > + return (old & mask); > +} > + > +/** > + * generic__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 always_inline __pure bool > +generic__test_and_clear_bit(bitop_uint_t nr, volatile void *addr) > +{ > + bitop_uint_t mask = BITOP_MASK(nr); > + volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + BITOP_WORD(nr); > + bitop_uint_t old = *p; > + > + *p = old & ~mask; > + return (old & mask); > +} > + > +/* WARNING: non atomic and it can be reordered! */ > +static always_inline __pure bool > +generic__test_and_change_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); > + bitop_uint_t old = *p; > + > + *p = old ^ mask; > + return (old & mask); > +} > +/** > + * generic_test_bit - Determine whether a bit is set > + * @nr: bit number to test > + * @addr: Address to start counting from > + */ > +static always_inline __pure int generic_test_bit(int nr, const volatile void *addr) Further up you use bool; why int here? > +{ > + const volatile bitop_uint_t *p = addr; > + return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1))); And reason not to use BITOP_MASK() here as well (once having switched to bool return type)? > +} > + > +static always_inline __pure bool > +__test_and_set_bit(unsigned long nr, volatile void *addr) > +{ > +#ifndef arch__test_and_set_bit > +#define arch__test_and_set_bit generic__test_and_set_bit > +#endif > + > + return arch__test_and_set_bit(nr, addr); > +} > +#define __test_and_set_bit(nr, addr) ({ \ > + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ > + __test_and_set_bit(nr, addr); \ > +}) > + > +static always_inline __pure bool > +__test_and_clear_bit(bitop_uint_t nr, volatile void *addr) > +{ > +#ifndef arch__test_and_clear_bit > +#define arch__test_and_clear_bit generic__test_and_clear_bit > +#endif > + > + return arch__test_and_clear_bit(nr, addr); > +} > +#define __test_and_clear_bit(nr, addr) ({ \ > + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ > + __test_and_clear_bit(nr, addr); \ > +}) > + > +static always_inline __pure bool > +__test_and_change_bit(unsigned long nr, volatile void *addr) > +{ > +#ifndef arch__test_and_change_bit > +#define arch__test_and_change_bit generic__test_and_change_bit > +#endif > + > + return arch__test_and_change_bit(nr, addr); > +} > +#define __test_and_change_bit(nr, addr) ({ \ > + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ > + __test_and_change_bit(nr, addr); \ > +}) > + > +static always_inline __pure int test_bit(int nr, const volatile void *addr) Further up you use bool; why int here? > +{ > +#ifndef arch_test_bit > +#define arch_test_bit generic_test_bit > +#endif > + > + return arch_test_bit(nr, addr); > +} > +#define test_bit(nr, addr) ({ \ > + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ > + test_bit(nr, addr); \ > +}) From here onwards, ... > +static always_inline __pure int fls(unsigned int x) > +{ > + if (__builtin_constant_p(x)) > + return generic_fls(x); > + > +#ifndef arch_fls > +#define arch_fls generic_fls > +#endif > + > + return arch_fls(x); > +} > + > +static always_inline __pure int flsl(unsigned long x) > +{ > + if (__builtin_constant_p(x)) > + return generic_flsl(x); > + > +#ifndef arch_flsl > +#define arch_flsl generic_flsl > +#endif > + > + return arch_flsl(x); > +} ... does all of this really belong here? Neither title nor description have any hint towards this. > /* > * Find First Set bit. Bits are labelled from 1. > */ This context suggests there's a dependency on an uncommitted patch. Nothing above says so. I guess you have a remark in the cover letter, yet imo that's only partly helpful. Jan
On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote: > On 03.04.2024 12:19, Oleksii Kurochko wrote: > > The patch introduces the following generic functions: > > * 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 move the check to generic code and define as 0 > > for other architectures as they do not require this check. > > Hmm, yes, the checks need to be in the outermost wrapper macros. > While > you're abstracting other stuff to arch_*(), wouldn't it make sense to > also abstract this to e.g. arch_check_bitop_size(), with the > expansion > simply being (effectively) empty in the generic fallback case? Probably it would be better to be consistent and introduce arch_check_bitop_size(). > > > --- a/xen/include/xen/bitops.h > > +++ b/xen/include/xen/bitops.h > > @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned long > > x) > > * scope > > */ > > > > +#define BITOP_BITS_PER_WORD 32 > > +/* typedef uint32_t bitop_uint_t; */ > > +#define bitop_uint_t uint32_t > > So no arch overrides permitted anymore at all? Not really, I agree that it is ugly, but I expected that arch will use undef to override. > > > +#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 <asm/bitops.h> > > > > +#ifndef bitop_bad_size > > +extern void __bitop_bad_size(void); > > If not switching to arch_check_bitop_size() or alike as suggested > above, > why exactly does this need duplicating here and in x86? Can't the > decl > simply move ahead of the #include right above? (Sure, this will then > require that nothing needing any of the functions you move here would > still include asm/bitops.h; it would need to be xen/bitops.h > everywhere.) It could be done in way you suggest, I just wanted to keep changes minimal ( without going and switching asm/bitops.h -> xen/bitops.h ), but we can consider such option. > > > +#define bitop_bad_size(addr) 0 > > +#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 __pure bool > > +generic__test_and_set_bit(unsigned long nr, volatile void *addr) > > Does __pure actually fit with the use of volatile? The former says > multiple > accesses may be folded; the latter says they must not be. Overlooked that, __pure should be dropped. > > > +{ > > + bitop_uint_t mask = BITOP_MASK(nr); > > + volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + > > BITOP_WORD(nr); > > Nit: Slightly shorter line possible: > > volatile bitop_uint_t *p = (volatile bitop_uint_t *)addr + > BITOP_WORD(nr); > > > + bitop_uint_t old = *p; > > + > > + *p = old | mask; > > + return (old & mask); > > +} > > + > > +/** > > + * generic__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 always_inline __pure bool > > +generic__test_and_clear_bit(bitop_uint_t nr, volatile void *addr) > > +{ > > + bitop_uint_t mask = BITOP_MASK(nr); > > + volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + > > BITOP_WORD(nr); > > + bitop_uint_t old = *p; > > + > > + *p = old & ~mask; > > + return (old & mask); > > +} > > + > > +/* WARNING: non atomic and it can be reordered! */ > > +static always_inline __pure bool > > +generic__test_and_change_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); > > + bitop_uint_t old = *p; > > + > > + *p = old ^ mask; > > + return (old & mask); > > +} > > +/** > > + * generic_test_bit - Determine whether a bit is set > > + * @nr: bit number to test > > + * @addr: Address to start counting from > > + */ > > +static always_inline __pure int generic_test_bit(int nr, const > > volatile void *addr) > > Further up you use bool; why int here? No specific reason, I have to update everything to bool. > > > +{ > > + const volatile bitop_uint_t *p = addr; > > + return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - > > 1))); > > And reason not to use BITOP_MASK() here as well (once having switched > to > bool return type)? It seems we can use BITOP_MASK() this implementation was copied from arch specific code. > > > +} > > + > > +static always_inline __pure bool > > +__test_and_set_bit(unsigned long nr, volatile void *addr) > > +{ > > +#ifndef arch__test_and_set_bit > > +#define arch__test_and_set_bit generic__test_and_set_bit > > +#endif > > + > > + return arch__test_and_set_bit(nr, addr); > > +} > > +#define __test_and_set_bit(nr, addr) ({ \ > > + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ > > + __test_and_set_bit(nr, addr); \ > > +}) > > + > > +static always_inline __pure bool > > +__test_and_clear_bit(bitop_uint_t nr, volatile void *addr) > > +{ > > +#ifndef arch__test_and_clear_bit > > +#define arch__test_and_clear_bit generic__test_and_clear_bit > > +#endif > > + > > + return arch__test_and_clear_bit(nr, addr); > > +} > > +#define __test_and_clear_bit(nr, addr) ({ \ > > + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ > > + __test_and_clear_bit(nr, addr); \ > > +}) > > + > > +static always_inline __pure bool > > +__test_and_change_bit(unsigned long nr, volatile void *addr) > > +{ > > +#ifndef arch__test_and_change_bit > > +#define arch__test_and_change_bit generic__test_and_change_bit > > +#endif > > + > > + return arch__test_and_change_bit(nr, addr); > > +} > > +#define __test_and_change_bit(nr, addr) ({ \ > > + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ > > + __test_and_change_bit(nr, addr); \ > > +}) > > + > > +static always_inline __pure int test_bit(int nr, const volatile > > void *addr) > > Further up you use bool; why int here? > > > +{ > > +#ifndef arch_test_bit > > +#define arch_test_bit generic_test_bit > > +#endif > > + > > + return arch_test_bit(nr, addr); > > +} > > +#define test_bit(nr, addr) ({ \ > > + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ > > + test_bit(nr, addr); \ > > +}) > > From here onwards, ... > > > +static always_inline __pure int fls(unsigned int x) > > +{ > > + if (__builtin_constant_p(x)) > > + return generic_fls(x); > > + > > +#ifndef arch_fls > > +#define arch_fls generic_fls > > +#endif > > + > > + return arch_fls(x); > > +} > > + > > +static always_inline __pure int flsl(unsigned long x) > > +{ > > + if (__builtin_constant_p(x)) > > + return generic_flsl(x); > > + > > +#ifndef arch_flsl > > +#define arch_flsl generic_flsl > > +#endif > > + > > + return arch_flsl(x); > > +} > > ... does all of this really belong here? Neither title nor > description have > any hint towards this. No, it should be a part of the [PATCH v7 05/19] xen/bitops: implement fls{l}() in common logic. An issue during rebase. I'll update that. > > > /* > > * Find First Set bit. Bits are labelled from 1. > > */ > > This context suggests there's a dependency on an uncommitted patch. > Nothing > above says so. I guess you have a remark in the cover letter, yet imo > that's > only partly helpful. Is it really a hard dependency? The current patch series really depends on ffs{l}() and that was mentioned in the cover letter ( I'll reword the cover letter to explain why exactly this dependency is needed ), but this patch isn't really depends on Andrew's patch series, where ffs{l}() are introduced. ~ Oleksii
On 04.04.2024 17:45, Oleksii wrote: > On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote: >> On 03.04.2024 12:19, Oleksii Kurochko wrote: >>> --- a/xen/include/xen/bitops.h >>> +++ b/xen/include/xen/bitops.h >>> @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned long >>> x) >>> * scope >>> */ >>> >>> +#define BITOP_BITS_PER_WORD 32 >>> +/* typedef uint32_t bitop_uint_t; */ >>> +#define bitop_uint_t uint32_t >> >> So no arch overrides permitted anymore at all? > Not really, I agree that it is ugly, but I expected that arch will use > undef to override. Which would be fine in principle, just that Misra wants us to avoid #undef-s (iirc). >>> /* >>> * Find First Set bit. Bits are labelled from 1. >>> */ >> >> This context suggests there's a dependency on an uncommitted patch. >> Nothing >> above says so. I guess you have a remark in the cover letter, yet imo >> that's >> only partly helpful. > Is it really a hard dependency? > The current patch series really depends on ffs{l}() and that was > mentioned in the cover letter ( I'll reword the cover letter to explain > why exactly this dependency is needed ), but this patch isn't really > depends on Andrew's patch series, where ffs{l}() are introduced. If anyone acked this patch, and if it otherwise looked independent, it would be a candidate for committing. Just that it won't apply for a non-obvious reason. Jan
On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote: > On 04.04.2024 17:45, Oleksii wrote: > > On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote: > > > On 03.04.2024 12:19, Oleksii Kurochko wrote: > > > > --- a/xen/include/xen/bitops.h > > > > +++ b/xen/include/xen/bitops.h > > > > @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned > > > > long > > > > x) > > > > * scope > > > > */ > > > > > > > > +#define BITOP_BITS_PER_WORD 32 > > > > +/* typedef uint32_t bitop_uint_t; */ > > > > +#define bitop_uint_t uint32_t > > > > > > So no arch overrides permitted anymore at all? > > Not really, I agree that it is ugly, but I expected that arch will > > use > > undef to override. > > Which would be fine in principle, just that Misra wants us to avoid > #undef-s > (iirc). Could you please give me a recommendation how to do that better? The reason why I put this defintions before inclusion of asm/bitops.h as RISC-V specific code uses these definitions inside it, so they should be defined before asm/bitops.h; other option is to define these definitions inside asm/bitops.h for each architecture. > > > > > /* > > > > * Find First Set bit. Bits are labelled from 1. > > > > */ > > > > > > This context suggests there's a dependency on an uncommitted > > > patch. > > > Nothing > > > above says so. I guess you have a remark in the cover letter, yet > > > imo > > > that's > > > only partly helpful. > > Is it really a hard dependency? > > The current patch series really depends on ffs{l}() and that was > > mentioned in the cover letter ( I'll reword the cover letter to > > explain > > why exactly this dependency is needed ), but this patch isn't > > really > > depends on Andrew's patch series, where ffs{l}() are introduced. > > If anyone acked this patch, and if it otherwise looked independent, > it would > be a candidate for committing. Just that it won't apply for a non- > obvious > reason. I didn't think about the it won't apply. In this I have to definitely mention this moment in cover letter. Thanks. ~ Oleksii
On 04.04.2024 18:24, Oleksii wrote: > On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote: >> On 04.04.2024 17:45, Oleksii wrote: >>> On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote: >>>> On 03.04.2024 12:19, Oleksii Kurochko wrote: >>>>> --- a/xen/include/xen/bitops.h >>>>> +++ b/xen/include/xen/bitops.h >>>>> @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned >>>>> long >>>>> x) >>>>> * scope >>>>> */ >>>>> >>>>> +#define BITOP_BITS_PER_WORD 32 >>>>> +/* typedef uint32_t bitop_uint_t; */ >>>>> +#define bitop_uint_t uint32_t >>>> >>>> So no arch overrides permitted anymore at all? >>> Not really, I agree that it is ugly, but I expected that arch will >>> use >>> undef to override. >> >> Which would be fine in principle, just that Misra wants us to avoid >> #undef-s >> (iirc). > Could you please give me a recommendation how to do that better? > > The reason why I put this defintions before inclusion of asm/bitops.h > as RISC-V specific code uses these definitions inside it, so they > should be defined before asm/bitops.h; other option is to define these > definitions inside asm/bitops.h for each architecture. Earlier on you had it that other way already (in a different header, but the principle is the same): Move the generic definitions immediately past inclusion of asm/bitops.h and frame them with #ifndef. Jan
On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote: > On 04.04.2024 18:24, Oleksii wrote: > > On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote: > > > On 04.04.2024 17:45, Oleksii wrote: > > > > On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote: > > > > > On 03.04.2024 12:19, Oleksii Kurochko wrote: > > > > > > --- a/xen/include/xen/bitops.h > > > > > > +++ b/xen/include/xen/bitops.h > > > > > > @@ -65,10 +65,164 @@ static inline int > > > > > > generic_flsl(unsigned > > > > > > long > > > > > > x) > > > > > > * scope > > > > > > */ > > > > > > > > > > > > +#define BITOP_BITS_PER_WORD 32 > > > > > > +/* typedef uint32_t bitop_uint_t; */ > > > > > > +#define bitop_uint_t uint32_t > > > > > > > > > > So no arch overrides permitted anymore at all? > > > > Not really, I agree that it is ugly, but I expected that arch > > > > will > > > > use > > > > undef to override. > > > > > > Which would be fine in principle, just that Misra wants us to > > > avoid > > > #undef-s > > > (iirc). > > Could you please give me a recommendation how to do that better? > > > > The reason why I put this defintions before inclusion of > > asm/bitops.h > > as RISC-V specific code uses these definitions inside it, so they > > should be defined before asm/bitops.h; other option is to define > > these > > definitions inside asm/bitops.h for each architecture. > > Earlier on you had it that other way already (in a different header, > but the principle is the same): Move the generic definitions > immediately > past inclusion of asm/bitops.h and frame them with #ifndef. It can be done in this way: xen/bitops.h: ... #include <asm/bitops.h> #ifndef BITOP_TYPE #define BITOP_BITS_PER_WORD 32 /* typedef uint32_t bitop_uint_t; */ #define bitop_uint_t uint32_t #endif ... But then RISC-V will fail as it is using bitop_uint_t inside asm/bitops.h. So, at least, for RISC-V it will be needed to add asm/bitops.h: #define BITOP_BITS_PER_WORD 32 /* typedef uint32_t bitop_uint_t; */ #define bitop_uint_t uint32_t It seems to me that this breaks the idea of having these macro definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD and bitop_uint_t with the same values as the generic ones. ~ Oleksii > > Jan
On 05.04.2024 09:56, Oleksii wrote: > On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote: >> On 04.04.2024 18:24, Oleksii wrote: >>> On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote: >>>> On 04.04.2024 17:45, Oleksii wrote: >>>>> On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote: >>>>>> On 03.04.2024 12:19, Oleksii Kurochko wrote: >>>>>>> --- a/xen/include/xen/bitops.h >>>>>>> +++ b/xen/include/xen/bitops.h >>>>>>> @@ -65,10 +65,164 @@ static inline int >>>>>>> generic_flsl(unsigned >>>>>>> long >>>>>>> x) >>>>>>> * scope >>>>>>> */ >>>>>>> >>>>>>> +#define BITOP_BITS_PER_WORD 32 >>>>>>> +/* typedef uint32_t bitop_uint_t; */ >>>>>>> +#define bitop_uint_t uint32_t >>>>>> >>>>>> So no arch overrides permitted anymore at all? >>>>> Not really, I agree that it is ugly, but I expected that arch >>>>> will >>>>> use >>>>> undef to override. >>>> >>>> Which would be fine in principle, just that Misra wants us to >>>> avoid >>>> #undef-s >>>> (iirc). >>> Could you please give me a recommendation how to do that better? >>> >>> The reason why I put this defintions before inclusion of >>> asm/bitops.h >>> as RISC-V specific code uses these definitions inside it, so they >>> should be defined before asm/bitops.h; other option is to define >>> these >>> definitions inside asm/bitops.h for each architecture. >> >> Earlier on you had it that other way already (in a different header, >> but the principle is the same): Move the generic definitions >> immediately >> past inclusion of asm/bitops.h and frame them with #ifndef. > It can be done in this way: > xen/bitops.h: > ... > #include <asm/bitops.h> > > #ifndef BITOP_TYPE > #define BITOP_BITS_PER_WORD 32 > /* typedef uint32_t bitop_uint_t; */ > #define bitop_uint_t uint32_t > #endif > ... > > But then RISC-V will fail as it is using bitop_uint_t inside > asm/bitops.h. > So, at least, for RISC-V it will be needed to add asm/bitops.h: > #define BITOP_BITS_PER_WORD 32 > /* typedef uint32_t bitop_uint_t; */ > #define bitop_uint_t uint32_t > > > It seems to me that this breaks the idea of having these macro > definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD and > bitop_uint_t with the same values as the generic ones. I don't follow. Right now patch 7 has #undef BITOP_BITS_PER_WORD #undef bitop_uint_t #define BITOP_BITS_PER_WORD BITS_PER_LONG #define bitop_uint_t unsigned long You'd drop the #undef-s and keep the #define-s. You want to override them both, after all. A problem would arise for _another_ arch wanting to use these (default) types in its asm/bitops.h. Which then could still be solved by having a types-only header. Recall the discussion on the last summit of us meaning to switch to such a model anyway (perhaps it being xen/types/bitops.h and asm/types/bitops.h then), in a broader fashion? IOW for now you could use the simple approach as long as no other arch needs the types in its asm/bitops.h. Later we would introduce the types-only headers, thus catering for possible future uses. Jan
On Fri, 2024-04-05 at 10:05 +0200, Jan Beulich wrote: > On 05.04.2024 09:56, Oleksii wrote: > > On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote: > > > On 04.04.2024 18:24, Oleksii wrote: > > > > On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote: > > > > > On 04.04.2024 17:45, Oleksii wrote: > > > > > > On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote: > > > > > > > On 03.04.2024 12:19, Oleksii Kurochko wrote: > > > > > > > > --- a/xen/include/xen/bitops.h > > > > > > > > +++ b/xen/include/xen/bitops.h > > > > > > > > @@ -65,10 +65,164 @@ static inline int > > > > > > > > generic_flsl(unsigned > > > > > > > > long > > > > > > > > x) > > > > > > > > * scope > > > > > > > > */ > > > > > > > > > > > > > > > > +#define BITOP_BITS_PER_WORD 32 > > > > > > > > +/* typedef uint32_t bitop_uint_t; */ > > > > > > > > +#define bitop_uint_t uint32_t > > > > > > > > > > > > > > So no arch overrides permitted anymore at all? > > > > > > Not really, I agree that it is ugly, but I expected that > > > > > > arch > > > > > > will > > > > > > use > > > > > > undef to override. > > > > > > > > > > Which would be fine in principle, just that Misra wants us to > > > > > avoid > > > > > #undef-s > > > > > (iirc). > > > > Could you please give me a recommendation how to do that > > > > better? > > > > > > > > The reason why I put this defintions before inclusion of > > > > asm/bitops.h > > > > as RISC-V specific code uses these definitions inside it, so > > > > they > > > > should be defined before asm/bitops.h; other option is to > > > > define > > > > these > > > > definitions inside asm/bitops.h for each architecture. > > > > > > Earlier on you had it that other way already (in a different > > > header, > > > but the principle is the same): Move the generic definitions > > > immediately > > > past inclusion of asm/bitops.h and frame them with #ifndef. > > It can be done in this way: > > xen/bitops.h: > > ... > > #include <asm/bitops.h> > > > > #ifndef BITOP_TYPE > > #define BITOP_BITS_PER_WORD 32 > > /* typedef uint32_t bitop_uint_t; */ > > #define bitop_uint_t uint32_t > > #endif > > ... > > > > But then RISC-V will fail as it is using bitop_uint_t inside > > asm/bitops.h. > > So, at least, for RISC-V it will be needed to add asm/bitops.h: > > #define BITOP_BITS_PER_WORD 32 > > /* typedef uint32_t bitop_uint_t; */ > > #define bitop_uint_t uint32_t > > > > > > It seems to me that this breaks the idea of having these macro > > definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD > > and > > bitop_uint_t with the same values as the generic ones. > > I don't follow. Right now patch 7 has > > #undef BITOP_BITS_PER_WORD > #undef bitop_uint_t > > #define BITOP_BITS_PER_WORD BITS_PER_LONG > #define bitop_uint_t unsigned long > > You'd drop the #undef-s and keep the #define-s. You want to override > them > both, after all. > > A problem would arise for _another_ arch wanting to use these > (default) > types in its asm/bitops.h. Which then could still be solved by having > a > types-only header. This problem arise now for Arm and PPC which use BITOP_BITS_PER_WORD inside it. Then it is needed to define BITOP_BITS_PER_WORD=32 in asm/bitops.h for Arm and PPC. If it is okay, then I will happy to follow this approach. > Recall the discussion on the last summit of us meaning > to switch to such a model anyway (perhaps it being xen/types/bitops.h > and > asm/types/bitops.h then), in a broader fashion? IOW for now you could > use > the simple approach as long as no other arch needs the types in its > asm/bitops.h. Later we would introduce the types-only headers, thus > catering for possible future uses. Do we really need asm/types/bitops.h? Can't we just do the following in asm/bitops.h: #ifndef BITOP_TYPE #include <xen/types/bitops.h> #endif ~ Oleksii
On Fri, 2024-04-05 at 13:53 +0200, Oleksii wrote: > On Fri, 2024-04-05 at 10:05 +0200, Jan Beulich wrote: > > On 05.04.2024 09:56, Oleksii wrote: > > > On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote: > > > > On 04.04.2024 18:24, Oleksii wrote: > > > > > On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote: > > > > > > On 04.04.2024 17:45, Oleksii wrote: > > > > > > > On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote: > > > > > > > > On 03.04.2024 12:19, Oleksii Kurochko wrote: > > > > > > > > > --- a/xen/include/xen/bitops.h > > > > > > > > > +++ b/xen/include/xen/bitops.h > > > > > > > > > @@ -65,10 +65,164 @@ static inline int > > > > > > > > > generic_flsl(unsigned > > > > > > > > > long > > > > > > > > > x) > > > > > > > > > * scope > > > > > > > > > */ > > > > > > > > > > > > > > > > > > +#define BITOP_BITS_PER_WORD 32 > > > > > > > > > +/* typedef uint32_t bitop_uint_t; */ > > > > > > > > > +#define bitop_uint_t uint32_t > > > > > > > > > > > > > > > > So no arch overrides permitted anymore at all? > > > > > > > Not really, I agree that it is ugly, but I expected that > > > > > > > arch > > > > > > > will > > > > > > > use > > > > > > > undef to override. > > > > > > > > > > > > Which would be fine in principle, just that Misra wants us > > > > > > to > > > > > > avoid > > > > > > #undef-s > > > > > > (iirc). > > > > > Could you please give me a recommendation how to do that > > > > > better? > > > > > > > > > > The reason why I put this defintions before inclusion of > > > > > asm/bitops.h > > > > > as RISC-V specific code uses these definitions inside it, so > > > > > they > > > > > should be defined before asm/bitops.h; other option is to > > > > > define > > > > > these > > > > > definitions inside asm/bitops.h for each architecture. > > > > > > > > Earlier on you had it that other way already (in a different > > > > header, > > > > but the principle is the same): Move the generic definitions > > > > immediately > > > > past inclusion of asm/bitops.h and frame them with #ifndef. > > > It can be done in this way: > > > xen/bitops.h: > > > ... > > > #include <asm/bitops.h> > > > > > > #ifndef BITOP_TYPE > > > #define BITOP_BITS_PER_WORD 32 > > > /* typedef uint32_t bitop_uint_t; */ > > > #define bitop_uint_t uint32_t > > > #endif > > > ... > > > > > > But then RISC-V will fail as it is using bitop_uint_t inside > > > asm/bitops.h. > > > So, at least, for RISC-V it will be needed to add asm/bitops.h: > > > #define BITOP_BITS_PER_WORD 32 > > > /* typedef uint32_t bitop_uint_t; */ > > > #define bitop_uint_t uint32_t > > > > > > > > > It seems to me that this breaks the idea of having these macro > > > definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD > > > and > > > bitop_uint_t with the same values as the generic ones. > > > > I don't follow. Right now patch 7 has > > > > #undef BITOP_BITS_PER_WORD > > #undef bitop_uint_t > > > > #define BITOP_BITS_PER_WORD BITS_PER_LONG > > #define bitop_uint_t unsigned long > > > > You'd drop the #undef-s and keep the #define-s. You want to > > override > > them > > both, after all. > > > > A problem would arise for _another_ arch wanting to use these > > (default) > > types in its asm/bitops.h. Which then could still be solved by > > having > > a > > types-only header. > This problem arise now for Arm and PPC which use BITOP_BITS_PER_WORD > inside it. Then it is needed to define BITOP_BITS_PER_WORD=32 in > asm/bitops.h for Arm and PPC. If it is okay, then I will happy to > follow this approach. > > > Recall the discussion on the last summit of us meaning > > to switch to such a model anyway (perhaps it being > > xen/types/bitops.h > > and > > asm/types/bitops.h then), in a broader fashion? IOW for now you > > could > > use > > the simple approach as long as no other arch needs the types in its > > asm/bitops.h. Later we would introduce the types-only headers, thus > > catering for possible future uses. > Do we really need asm/types/bitops.h? Can't we just do the following > in > asm/bitops.h: > #ifndef BITOP_TYPE > #include <xen/types/bitops.h> > #endif Or as an options just update <xen/types.h> with after inclusion of <asm/types.h>: #ifndef BITOP_TYPE #define BITOP_BITS_PER_WORD 32 /* typedef uint32_t bitop_uint_t; */ #define bitop_uint_t uint32_t #endif And then just include <xen/types.h> to <<xen/bitops.h>. ~ Oleksii > > ~ Oleksii
On 05.04.2024 13:56, Oleksii wrote: > On Fri, 2024-04-05 at 13:53 +0200, Oleksii wrote: >> On Fri, 2024-04-05 at 10:05 +0200, Jan Beulich wrote: >>> On 05.04.2024 09:56, Oleksii wrote: >>>> On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote: >>>>> On 04.04.2024 18:24, Oleksii wrote: >>>>>> On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote: >>>>>>> On 04.04.2024 17:45, Oleksii wrote: >>>>>>>> On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote: >>>>>>>>> On 03.04.2024 12:19, Oleksii Kurochko wrote: >>>>>>>>>> --- a/xen/include/xen/bitops.h >>>>>>>>>> +++ b/xen/include/xen/bitops.h >>>>>>>>>> @@ -65,10 +65,164 @@ static inline int >>>>>>>>>> generic_flsl(unsigned >>>>>>>>>> long >>>>>>>>>> x) >>>>>>>>>> * scope >>>>>>>>>> */ >>>>>>>>>> >>>>>>>>>> +#define BITOP_BITS_PER_WORD 32 >>>>>>>>>> +/* typedef uint32_t bitop_uint_t; */ >>>>>>>>>> +#define bitop_uint_t uint32_t >>>>>>>>> >>>>>>>>> So no arch overrides permitted anymore at all? >>>>>>>> Not really, I agree that it is ugly, but I expected that >>>>>>>> arch >>>>>>>> will >>>>>>>> use >>>>>>>> undef to override. >>>>>>> >>>>>>> Which would be fine in principle, just that Misra wants us >>>>>>> to >>>>>>> avoid >>>>>>> #undef-s >>>>>>> (iirc). >>>>>> Could you please give me a recommendation how to do that >>>>>> better? >>>>>> >>>>>> The reason why I put this defintions before inclusion of >>>>>> asm/bitops.h >>>>>> as RISC-V specific code uses these definitions inside it, so >>>>>> they >>>>>> should be defined before asm/bitops.h; other option is to >>>>>> define >>>>>> these >>>>>> definitions inside asm/bitops.h for each architecture. >>>>> >>>>> Earlier on you had it that other way already (in a different >>>>> header, >>>>> but the principle is the same): Move the generic definitions >>>>> immediately >>>>> past inclusion of asm/bitops.h and frame them with #ifndef. >>>> It can be done in this way: >>>> xen/bitops.h: >>>> ... >>>> #include <asm/bitops.h> >>>> >>>> #ifndef BITOP_TYPE >>>> #define BITOP_BITS_PER_WORD 32 >>>> /* typedef uint32_t bitop_uint_t; */ >>>> #define bitop_uint_t uint32_t >>>> #endif >>>> ... >>>> >>>> But then RISC-V will fail as it is using bitop_uint_t inside >>>> asm/bitops.h. >>>> So, at least, for RISC-V it will be needed to add asm/bitops.h: >>>> #define BITOP_BITS_PER_WORD 32 >>>> /* typedef uint32_t bitop_uint_t; */ >>>> #define bitop_uint_t uint32_t >>>> >>>> >>>> It seems to me that this breaks the idea of having these macro >>>> definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD >>>> and >>>> bitop_uint_t with the same values as the generic ones. >>> >>> I don't follow. Right now patch 7 has >>> >>> #undef BITOP_BITS_PER_WORD >>> #undef bitop_uint_t >>> >>> #define BITOP_BITS_PER_WORD BITS_PER_LONG >>> #define bitop_uint_t unsigned long >>> >>> You'd drop the #undef-s and keep the #define-s. You want to >>> override >>> them >>> both, after all. >>> >>> A problem would arise for _another_ arch wanting to use these >>> (default) >>> types in its asm/bitops.h. Which then could still be solved by >>> having >>> a >>> types-only header. >> This problem arise now for Arm and PPC which use BITOP_BITS_PER_WORD >> inside it. Then it is needed to define BITOP_BITS_PER_WORD=32 in >> asm/bitops.h for Arm and PPC. If it is okay, then I will happy to >> follow this approach. >> >>> Recall the discussion on the last summit of us meaning >>> to switch to such a model anyway (perhaps it being >>> xen/types/bitops.h >>> and >>> asm/types/bitops.h then), in a broader fashion? IOW for now you >>> could >>> use >>> the simple approach as long as no other arch needs the types in its >>> asm/bitops.h. Later we would introduce the types-only headers, thus >>> catering for possible future uses. >> Do we really need asm/types/bitops.h? Can't we just do the following >> in >> asm/bitops.h: >> #ifndef BITOP_TYPE >> #include <xen/types/bitops.h> >> #endif This might do, yes. > Or as an options just update <xen/types.h> with after inclusion of > <asm/types.h>: > #ifndef BITOP_TYPE > #define BITOP_BITS_PER_WORD 32 > /* typedef uint32_t bitop_uint_t; */ > #define bitop_uint_t uint32_t > #endif > > And then just include <xen/types.h> to <<xen/bitops.h>. That's a (transient) option as well, I guess. Jan
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 <xen/mm.h> #include <xen/vmap.h> -#include <asm/bitops.h> #include <asm/byteorder.h> #include <asm/insn.h> #include <asm/livepatch.h> 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 = 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; -} - -/* WARNING: non atomic and it can be reordered! */ -static inline int __test_and_change_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_bit - Determine whether a bit is set - * @nr: bit number to test - * @addr: Address to start counting from - */ -static inline int test_bit(int nr, const volatile void *addr) -{ - const volatile unsigned int *p = (const volatile unsigned int *)addr; - return 1UL & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD-1))); -} - /* * On ARMv5 and above those functions can be implemented around * the clz instruction for much better code efficiency. diff --git a/xen/arch/ppc/include/asm/bitops.h b/xen/arch/ppc/include/asm/bitops.h index 989d341a44..a17060c7c2 100644 --- a/xen/arch/ppc/include/asm/bitops.h +++ b/xen/arch/ppc/include/asm/bitops.h @@ -15,9 +15,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) (1U << ((nr) % BITOP_BITS_PER_WORD)) -#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD) #define BITS_PER_BYTE 8 /* PPC bit number conversion */ @@ -69,17 +66,6 @@ static inline void clear_bit(int nr, volatile void *addr) clear_bits(BITOP_MASK(nr), (volatile unsigned int *)addr + BITOP_WORD(nr)); } -/** - * test_bit - Determine whether a bit is set - * @nr: bit number to test - * @addr: Address to start counting from - */ -static inline int test_bit(int nr, const volatile void *addr) -{ - const volatile unsigned int *p = addr; - return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1))); -} - static inline unsigned int test_and_clear_bits( unsigned int mask, volatile unsigned int *p) @@ -133,56 +119,6 @@ static inline int test_and_set_bit(unsigned int nr, volatile void *addr) (volatile unsigned int *)addr + BITOP_WORD(nr)) != 0; } -/** - * __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 = 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; -} - -#define flsl(x) generic_flsl(x) -#define fls(x) generic_fls(x) - -/* Based on linux/include/asm-generic/bitops/ffz.h */ -/* - * ffz - find first zero in word. - * @word: The word to search - * - * Undefined if no zero exists, so code should check against ~0UL first. - */ -#define ffz(x) __ffs(~(x)) - /** * hweightN - returns the hamming weight of a N-bit word * @x: the word to weigh diff --git a/xen/arch/ppc/include/asm/page.h b/xen/arch/ppc/include/asm/page.h index 890e285051..482053b023 100644 --- a/xen/arch/ppc/include/asm/page.h +++ b/xen/arch/ppc/include/asm/page.h @@ -4,7 +4,7 @@ #include <xen/types.h> -#include <asm/bitops.h> +#include <xen/bitops.h> #include <asm/byteorder.h> #define PDE_VALID PPC_BIT(0) diff --git a/xen/arch/ppc/mm-radix.c b/xen/arch/ppc/mm-radix.c index daa411a6fa..3cd8c4635a 100644 --- 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 <xen/bitops.h> #include <xen/init.h> #include <xen/kernel.h> #include <xen/mm.h> #include <xen/types.h> #include <xen/lib.h> -#include <asm/bitops.h> #include <asm/byteorder.h> #include <asm/early_printk.h> #include <asm/page.h> diff --git a/xen/arch/x86/include/asm/bitops.h b/xen/arch/x86/include/asm/bitops.h index dd439b69a0..81b43da5db 100644 --- a/xen/arch/x86/include/asm/bitops.h +++ b/xen/arch/x86/include/asm/bitops.h @@ -175,7 +175,7 @@ static inline int test_and_set_bit(int nr, volatile void *addr) }) /** - * __test_and_set_bit - Set a bit and return its old value + * arch__test_and_set_bit - Set a bit and return its old value * @nr: Bit to set * @addr: Address to count from * @@ -183,7 +183,7 @@ static inline int test_and_set_bit(int nr, volatile void *addr) * 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, void *addr) +static inline int arch__test_and_set_bit(int nr, volatile void *addr) { int oldbit; @@ -194,10 +194,7 @@ static inline int __test_and_set_bit(int nr, void *addr) return oldbit; } -#define __test_and_set_bit(nr, addr) ({ \ - if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ - __test_and_set_bit(nr, addr); \ -}) +#define arch__test_and_set_bit arch__test_and_set_bit /** * test_and_clear_bit - Clear a bit and return its old value @@ -224,7 +221,7 @@ static inline int test_and_clear_bit(int nr, volatile void *addr) }) /** - * __test_and_clear_bit - Clear a bit and return its old value + * arch__test_and_clear_bit - Clear a bit and return its old value * @nr: Bit to set * @addr: Address to count from * @@ -232,7 +229,7 @@ static inline int test_and_clear_bit(int nr, volatile void *addr) * 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, void *addr) +static inline int arch__test_and_clear_bit(int nr, volatile void *addr) { int oldbit; @@ -243,13 +240,10 @@ static inline int __test_and_clear_bit(int nr, void *addr) return oldbit; } -#define __test_and_clear_bit(nr, addr) ({ \ - if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ - __test_and_clear_bit(nr, addr); \ -}) +#define arch__test_and_clear_bit arch__test_and_clear_bit /* WARNING: non atomic and it can be reordered! */ -static inline int __test_and_change_bit(int nr, void *addr) +static inline int arch__test_and_change_bit(int nr, volatile void *addr) { int oldbit; @@ -260,10 +254,7 @@ static inline int __test_and_change_bit(int nr, void *addr) return oldbit; } -#define __test_and_change_bit(nr, addr) ({ \ - if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ - __test_and_change_bit(nr, addr); \ -}) +#define arch__test_and_change_bit arch__test_and_change_bit /** * test_and_change_bit - Change a bit and return its new value @@ -307,8 +298,7 @@ static inline int variable_test_bit(int nr, const volatile void *addr) return oldbit; } -#define test_bit(nr, addr) ({ \ - if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ +#define arch_test_bit(nr, addr) ({ \ __builtin_constant_p(nr) ? \ constant_test_bit(nr, addr) : \ variable_test_bit(nr, addr); \ diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h index f14ad0d33a..685c7540cc 100644 --- a/xen/include/xen/bitops.h +++ b/xen/include/xen/bitops.h @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned long x) * scope */ +#define BITOP_BITS_PER_WORD 32 +/* typedef uint32_t bitop_uint_t; */ +#define bitop_uint_t uint32_t + +#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 <asm/bitops.h> +#ifndef bitop_bad_size +extern void __bitop_bad_size(void); +#define bitop_bad_size(addr) 0 +#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 __pure 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); + bitop_uint_t old = *p; + + *p = old | mask; + return (old & mask); +} + +/** + * generic__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 always_inline __pure bool +generic__test_and_clear_bit(bitop_uint_t nr, volatile void *addr) +{ + bitop_uint_t mask = BITOP_MASK(nr); + volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + BITOP_WORD(nr); + bitop_uint_t old = *p; + + *p = old & ~mask; + return (old & mask); +} + +/* WARNING: non atomic and it can be reordered! */ +static always_inline __pure bool +generic__test_and_change_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); + bitop_uint_t old = *p; + + *p = old ^ mask; + return (old & mask); +} +/** + * generic_test_bit - Determine whether a bit is set + * @nr: bit number to test + * @addr: Address to start counting from + */ +static always_inline __pure int generic_test_bit(int nr, const volatile void *addr) +{ + const volatile bitop_uint_t *p = addr; + return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1))); +} + +static always_inline __pure bool +__test_and_set_bit(unsigned long nr, volatile void *addr) +{ +#ifndef arch__test_and_set_bit +#define arch__test_and_set_bit generic__test_and_set_bit +#endif + + return arch__test_and_set_bit(nr, addr); +} +#define __test_and_set_bit(nr, addr) ({ \ + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ + __test_and_set_bit(nr, addr); \ +}) + +static always_inline __pure bool +__test_and_clear_bit(bitop_uint_t nr, volatile void *addr) +{ +#ifndef arch__test_and_clear_bit +#define arch__test_and_clear_bit generic__test_and_clear_bit +#endif + + return arch__test_and_clear_bit(nr, addr); +} +#define __test_and_clear_bit(nr, addr) ({ \ + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ + __test_and_clear_bit(nr, addr); \ +}) + +static always_inline __pure bool +__test_and_change_bit(unsigned long nr, volatile void *addr) +{ +#ifndef arch__test_and_change_bit +#define arch__test_and_change_bit generic__test_and_change_bit +#endif + + return arch__test_and_change_bit(nr, addr); +} +#define __test_and_change_bit(nr, addr) ({ \ + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ + __test_and_change_bit(nr, addr); \ +}) + +static always_inline __pure int test_bit(int nr, const volatile void *addr) +{ +#ifndef arch_test_bit +#define arch_test_bit generic_test_bit +#endif + + return arch_test_bit(nr, addr); +} +#define test_bit(nr, addr) ({ \ + if ( bitop_bad_size(addr) ) __bitop_bad_size(); \ + test_bit(nr, addr); \ +}) + +static always_inline __pure int fls(unsigned int x) +{ + if (__builtin_constant_p(x)) + return generic_fls(x); + +#ifndef arch_fls +#define arch_fls generic_fls +#endif + + return arch_fls(x); +} + +static always_inline __pure int flsl(unsigned long x) +{ + if (__builtin_constant_p(x)) + return generic_flsl(x); + +#ifndef arch_flsl +#define arch_flsl generic_flsl +#endif + + return arch_flsl(x); +} + /* * Find First Set bit. Bits are labelled from 1. */
The patch introduces the following generic functions: * 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 move the check to generic code and define as 0 for other architectures as they do not require this check. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- 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 | 64 ------------- xen/arch/ppc/include/asm/page.h | 2 +- xen/arch/ppc/mm-radix.c | 2 +- xen/arch/x86/include/asm/bitops.h | 28 ++---- xen/include/xen/bitops.h | 154 ++++++++++++++++++++++++++++++ 7 files changed, 165 insertions(+), 153 deletions(-)