| Message ID | 20200204173455.22020-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | nvmx: implement support for MSR bitmaps | expand |
On 04.02.2020 18:34, Roger Pau Monne wrote: > Import the functions and it's dependencies. Based on Linux 5.5, commit > id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Thanks for going this route; two remarks / requests: > --- a/xen/common/bitmap.c > +++ b/xen/common/bitmap.c > @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits) > #endif > EXPORT_SYMBOL(__bitmap_weight); > > +void __bitmap_set(unsigned long *map, unsigned int start, int len) > +{ > + unsigned long *p = map + BIT_WORD(start); > + const unsigned int size = start + len; > + int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); > + unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); > + > + while (len - bits_to_set >= 0) { > + *p |= mask_to_set; > + len -= bits_to_set; > + bits_to_set = BITS_PER_LONG; > + mask_to_set = ~0UL; > + p++; > + } > + if (len) { > + mask_to_set &= BITMAP_LAST_WORD_MASK(size); > + *p |= mask_to_set; > + } > +} > +EXPORT_SYMBOL(__bitmap_set); > + > +void __bitmap_clear(unsigned long *map, unsigned int start, int len) > +{ > + unsigned long *p = map + BIT_WORD(start); > + const unsigned int size = start + len; > + int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); > + unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); > + > + while (len - bits_to_clear >= 0) { > + *p &= ~mask_to_clear; > + len -= bits_to_clear; > + bits_to_clear = BITS_PER_LONG; > + mask_to_clear = ~0UL; > + p++; > + } > + if (len) { > + mask_to_clear &= BITMAP_LAST_WORD_MASK(size); > + *p &= ~mask_to_clear; > + } > +} > +EXPORT_SYMBOL(__bitmap_clear); Despite all the other EXPORT_SYMBOL() in this file, personally I would suggest to refrain from adding more. But I'm not going to insist (until such time that they all get cleaned up). > --- a/xen/include/asm-x86/bitops.h > +++ b/xen/include/asm-x86/bitops.h > @@ -480,4 +480,6 @@ static inline int fls(unsigned int x) > #define hweight16(x) generic_hweight16(x) > #define hweight8(x) generic_hweight8(x) > > +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) At first I thought - why for x86 only? Then I noticed Arm has an almost identical #define already. Which in turn made me look at Linux, where that #define lives in a common header. I think you want to move the Arm one. Or wait, no - Arm's isn't even compatible with the implementations of the functions you add. This definitely needs taking care of, perhaps by way of ignoring my request to go this route (as getting too involved). > @@ -227,6 +229,42 @@ static inline int bitmap_weight(const unsigned long *src, int nbits) > return __bitmap_weight(src, nbits); > } > > +#ifdef __LITTLE_ENDIAN > +#define BITMAP_MEM_ALIGNMENT 8 > +#else > +#define BITMAP_MEM_ALIGNMENT (8 * sizeof(unsigned long)) > +#endif For __LITTLE_ENDIAN to be consistently defined (or not), don't you need to include <asm/byteorder.h> here? Jan
On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote: > On 04.02.2020 18:34, Roger Pau Monne wrote: > > Import the functions and it's dependencies. Based on Linux 5.5, commit > > id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks for going this route; two remarks / requests: > > > --- a/xen/common/bitmap.c > > +++ b/xen/common/bitmap.c > > @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits) > > #endif > > EXPORT_SYMBOL(__bitmap_weight); > > > > +void __bitmap_set(unsigned long *map, unsigned int start, int len) > > +{ > > + unsigned long *p = map + BIT_WORD(start); > > + const unsigned int size = start + len; > > + int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); > > + unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); > > + > > + while (len - bits_to_set >= 0) { > > + *p |= mask_to_set; > > + len -= bits_to_set; > > + bits_to_set = BITS_PER_LONG; > > + mask_to_set = ~0UL; > > + p++; > > + } > > + if (len) { > > + mask_to_set &= BITMAP_LAST_WORD_MASK(size); > > + *p |= mask_to_set; > > + } > > +} > > +EXPORT_SYMBOL(__bitmap_set); > > + > > +void __bitmap_clear(unsigned long *map, unsigned int start, int len) > > +{ > > + unsigned long *p = map + BIT_WORD(start); > > + const unsigned int size = start + len; > > + int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); > > + unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); > > + > > + while (len - bits_to_clear >= 0) { > > + *p &= ~mask_to_clear; > > + len -= bits_to_clear; > > + bits_to_clear = BITS_PER_LONG; > > + mask_to_clear = ~0UL; > > + p++; > > + } > > + if (len) { > > + mask_to_clear &= BITMAP_LAST_WORD_MASK(size); > > + *p &= ~mask_to_clear; > > + } > > +} > > +EXPORT_SYMBOL(__bitmap_clear); > > Despite all the other EXPORT_SYMBOL() in this file, personally I > would suggest to refrain from adding more. But I'm not going to > insist (until such time that they all get cleaned up). > > > --- a/xen/include/asm-x86/bitops.h > > +++ b/xen/include/asm-x86/bitops.h > > @@ -480,4 +480,6 @@ static inline int fls(unsigned int x) > > #define hweight16(x) generic_hweight16(x) > > #define hweight8(x) generic_hweight8(x) > > > > +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > > At first I thought - why for x86 only? Then I noticed Arm has an > almost identical #define already. Which in turn made me look at > Linux, where that #define lives in a common header. I think you > want to move the Arm one. Or wait, no - Arm's isn't even > compatible with the implementations of the functions you add. > This definitely needs taking care of, perhaps by way of ignoring > my request to go this route (as getting too involved). Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be used when the bitmap is mapped to an array of 32bit type elements. I could introduce BIT_LONG that would have the same definition on Arm and x86, and then modify the imported functions to use it, but IMO the right solution would be to change the Arm BIT_WORD macro to also use BITS_PER_LONG (and adjust the callers). This seems quite far off, so if you don't mind I would rather have the original v3 2/2 using set_bit: https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00190.html Thanks, Roger.
On 05.02.2020 14:21, Roger Pau Monné wrote: > On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote: >> On 04.02.2020 18:34, Roger Pau Monne wrote: >>> Import the functions and it's dependencies. Based on Linux 5.5, commit >>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> >> Thanks for going this route; two remarks / requests: >> >>> --- a/xen/common/bitmap.c >>> +++ b/xen/common/bitmap.c >>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits) >>> #endif >>> EXPORT_SYMBOL(__bitmap_weight); >>> >>> +void __bitmap_set(unsigned long *map, unsigned int start, int len) >>> +{ >>> + unsigned long *p = map + BIT_WORD(start); >>> + const unsigned int size = start + len; >>> + int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); >>> + unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); >>> + >>> + while (len - bits_to_set >= 0) { >>> + *p |= mask_to_set; >>> + len -= bits_to_set; >>> + bits_to_set = BITS_PER_LONG; >>> + mask_to_set = ~0UL; >>> + p++; >>> + } >>> + if (len) { >>> + mask_to_set &= BITMAP_LAST_WORD_MASK(size); >>> + *p |= mask_to_set; >>> + } >>> +} >>> +EXPORT_SYMBOL(__bitmap_set); >>> + >>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len) >>> +{ >>> + unsigned long *p = map + BIT_WORD(start); >>> + const unsigned int size = start + len; >>> + int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); >>> + unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); >>> + >>> + while (len - bits_to_clear >= 0) { >>> + *p &= ~mask_to_clear; >>> + len -= bits_to_clear; >>> + bits_to_clear = BITS_PER_LONG; >>> + mask_to_clear = ~0UL; >>> + p++; >>> + } >>> + if (len) { >>> + mask_to_clear &= BITMAP_LAST_WORD_MASK(size); >>> + *p &= ~mask_to_clear; >>> + } >>> +} >>> +EXPORT_SYMBOL(__bitmap_clear); >> >> Despite all the other EXPORT_SYMBOL() in this file, personally I >> would suggest to refrain from adding more. But I'm not going to >> insist (until such time that they all get cleaned up). >> >>> --- a/xen/include/asm-x86/bitops.h >>> +++ b/xen/include/asm-x86/bitops.h >>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x) >>> #define hweight16(x) generic_hweight16(x) >>> #define hweight8(x) generic_hweight8(x) >>> >>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) >> >> At first I thought - why for x86 only? Then I noticed Arm has an >> almost identical #define already. Which in turn made me look at >> Linux, where that #define lives in a common header. I think you >> want to move the Arm one. Or wait, no - Arm's isn't even >> compatible with the implementations of the functions you add. >> This definitely needs taking care of, perhaps by way of ignoring >> my request to go this route (as getting too involved). > > Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be > used when the bitmap is mapped to an array of 32bit type elements. > > I could introduce BIT_LONG that would have the same definition on Arm > and x86, and then modify the imported functions to use it, but IMO the > right solution would be to change the Arm BIT_WORD macro to also use > BITS_PER_LONG (and adjust the callers). So do I. Julien, Stefano? > This seems quite far off, so if you don't mind I would rather have the > original v3 2/2 using set_bit: > > https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00190.html As per my previous reply - yes, I'm okay with that, and yes, expecting this I've also kept your patches this way in my to-be-committed folder (pending Kevin's ack for patch 1). Jan
On 05/02/2020 13:27, Jan Beulich wrote: > On 05.02.2020 14:21, Roger Pau Monné wrote: >> On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote: >>> On 04.02.2020 18:34, Roger Pau Monne wrote: >>>> Import the functions and it's dependencies. Based on Linux 5.5, commit >>>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755. >>>> >>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> >>> Thanks for going this route; two remarks / requests: >>> >>>> --- a/xen/common/bitmap.c >>>> +++ b/xen/common/bitmap.c >>>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits) >>>> #endif >>>> EXPORT_SYMBOL(__bitmap_weight); >>>> >>>> +void __bitmap_set(unsigned long *map, unsigned int start, int len) >>>> +{ >>>> + unsigned long *p = map + BIT_WORD(start); >>>> + const unsigned int size = start + len; >>>> + int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); >>>> + unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); >>>> + >>>> + while (len - bits_to_set >= 0) { >>>> + *p |= mask_to_set; >>>> + len -= bits_to_set; >>>> + bits_to_set = BITS_PER_LONG; >>>> + mask_to_set = ~0UL; >>>> + p++; >>>> + } >>>> + if (len) { >>>> + mask_to_set &= BITMAP_LAST_WORD_MASK(size); >>>> + *p |= mask_to_set; >>>> + } >>>> +} >>>> +EXPORT_SYMBOL(__bitmap_set); >>>> + >>>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len) >>>> +{ >>>> + unsigned long *p = map + BIT_WORD(start); >>>> + const unsigned int size = start + len; >>>> + int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); >>>> + unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); >>>> + >>>> + while (len - bits_to_clear >= 0) { >>>> + *p &= ~mask_to_clear; >>>> + len -= bits_to_clear; >>>> + bits_to_clear = BITS_PER_LONG; >>>> + mask_to_clear = ~0UL; >>>> + p++; >>>> + } >>>> + if (len) { >>>> + mask_to_clear &= BITMAP_LAST_WORD_MASK(size); >>>> + *p &= ~mask_to_clear; >>>> + } >>>> +} >>>> +EXPORT_SYMBOL(__bitmap_clear); >>> >>> Despite all the other EXPORT_SYMBOL() in this file, personally I >>> would suggest to refrain from adding more. But I'm not going to >>> insist (until such time that they all get cleaned up). >>> >>>> --- a/xen/include/asm-x86/bitops.h >>>> +++ b/xen/include/asm-x86/bitops.h >>>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x) >>>> #define hweight16(x) generic_hweight16(x) >>>> #define hweight8(x) generic_hweight8(x) >>>> >>>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) >>> >>> At first I thought - why for x86 only? Then I noticed Arm has an >>> almost identical #define already. Which in turn made me look at >>> Linux, where that #define lives in a common header. I think you >>> want to move the Arm one. Or wait, no - Arm's isn't even >>> compatible with the implementations of the functions you add. >>> This definitely needs taking care of, perhaps by way of ignoring >>> my request to go this route (as getting too involved). >> >> Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be >> used when the bitmap is mapped to an array of 32bit type elements. >> >> I could introduce BIT_LONG that would have the same definition on Arm >> and x86, and then modify the imported functions to use it, but IMO the >> right solution would be to change the Arm BIT_WORD macro to also use >> BITS_PER_LONG (and adjust the callers). > > So do I. Julien, Stefano? BIT_WORD used to use BITS_PER_LONG but this was changed in commit: commit cd338e967c598bf747b03dcfd9d8d45dc40bac1a Author: Ian Campbell <ian.campbell@citrix.com> Date: Thu May 8 16:13:55 2014 +0100 xen: arm: bitops take unsigned int Xen bitmaps can be 4 rather than 8 byte aligned, so use the appropriate type. Otherwise the compiler can generate unaligned 8 byte accesses and cause traps. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> On 64-bit Arm, while we allow unaligned access, the atomic operations still enforce alignment. On 32-bit Arm, there are no unaligned access allowed. However, the change of BIT_WORD is not a concern for 32-bit. I haven't check whether we still have places where bitops are used with 4 byte aligned memory. However, as the bitops take a void * in parameter, there are no promise on the alignment. Therefore, we can't rewrite BIT_WORD without addressing the underlying issues. Introducing BIT_LONG is probably the easiest way at the moment. However, our bitops really ought to specify the alignment in parameter to avoid such issues arising. I would be in favor of using unsigned long *. >> This seems quite far off, so if you don't mind I would rather have the >> original v3 2/2 using set_bit: >> >> https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00190.html > > As per my previous reply - yes, I'm okay with that, and yes, > expecting this I've also kept your patches this way in my > to-be-committed folder (pending Kevin's ack for patch 1). > > Jan > Cheers,
On 08.02.2020 15:37, Julien Grall wrote: > > > On 05/02/2020 13:27, Jan Beulich wrote: >> On 05.02.2020 14:21, Roger Pau Monné wrote: >>> On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote: >>>> On 04.02.2020 18:34, Roger Pau Monne wrote: >>>>> Import the functions and it's dependencies. Based on Linux 5.5, commit >>>>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755. >>>>> >>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>>> >>>> Thanks for going this route; two remarks / requests: >>>> >>>>> --- a/xen/common/bitmap.c >>>>> +++ b/xen/common/bitmap.c >>>>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits) >>>>> #endif >>>>> EXPORT_SYMBOL(__bitmap_weight); >>>>> >>>>> +void __bitmap_set(unsigned long *map, unsigned int start, int len) >>>>> +{ >>>>> + unsigned long *p = map + BIT_WORD(start); >>>>> + const unsigned int size = start + len; >>>>> + int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); >>>>> + unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); >>>>> + >>>>> + while (len - bits_to_set >= 0) { >>>>> + *p |= mask_to_set; >>>>> + len -= bits_to_set; >>>>> + bits_to_set = BITS_PER_LONG; >>>>> + mask_to_set = ~0UL; >>>>> + p++; >>>>> + } >>>>> + if (len) { >>>>> + mask_to_set &= BITMAP_LAST_WORD_MASK(size); >>>>> + *p |= mask_to_set; >>>>> + } >>>>> +} >>>>> +EXPORT_SYMBOL(__bitmap_set); >>>>> + >>>>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len) >>>>> +{ >>>>> + unsigned long *p = map + BIT_WORD(start); >>>>> + const unsigned int size = start + len; >>>>> + int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); >>>>> + unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); >>>>> + >>>>> + while (len - bits_to_clear >= 0) { >>>>> + *p &= ~mask_to_clear; >>>>> + len -= bits_to_clear; >>>>> + bits_to_clear = BITS_PER_LONG; >>>>> + mask_to_clear = ~0UL; >>>>> + p++; >>>>> + } >>>>> + if (len) { >>>>> + mask_to_clear &= BITMAP_LAST_WORD_MASK(size); >>>>> + *p &= ~mask_to_clear; >>>>> + } >>>>> +} >>>>> +EXPORT_SYMBOL(__bitmap_clear); >>>> >>>> Despite all the other EXPORT_SYMBOL() in this file, personally I >>>> would suggest to refrain from adding more. But I'm not going to >>>> insist (until such time that they all get cleaned up). >>>> >>>>> --- a/xen/include/asm-x86/bitops.h >>>>> +++ b/xen/include/asm-x86/bitops.h >>>>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x) >>>>> #define hweight16(x) generic_hweight16(x) >>>>> #define hweight8(x) generic_hweight8(x) >>>>> >>>>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) >>>> >>>> At first I thought - why for x86 only? Then I noticed Arm has an >>>> almost identical #define already. Which in turn made me look at >>>> Linux, where that #define lives in a common header. I think you >>>> want to move the Arm one. Or wait, no - Arm's isn't even >>>> compatible with the implementations of the functions you add. >>>> This definitely needs taking care of, perhaps by way of ignoring >>>> my request to go this route (as getting too involved). >>> >>> Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be >>> used when the bitmap is mapped to an array of 32bit type elements. >>> >>> I could introduce BIT_LONG that would have the same definition on Arm >>> and x86, and then modify the imported functions to use it, but IMO the >>> right solution would be to change the Arm BIT_WORD macro to also use >>> BITS_PER_LONG (and adjust the callers). >> >> So do I. Julien, Stefano? > > BIT_WORD used to use BITS_PER_LONG but this was changed in commit: > > commit cd338e967c598bf747b03dcfd9d8d45dc40bac1a > Author: Ian Campbell <ian.campbell@citrix.com> > Date: Thu May 8 16:13:55 2014 +0100 > > xen: arm: bitops take unsigned int > > Xen bitmaps can be 4 rather than 8 byte aligned, so use the > appropriate type. > Otherwise the compiler can generate unaligned 8 byte accesses and > cause traps. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > On 64-bit Arm, while we allow unaligned access, the atomic operations > still enforce alignment. > > On 32-bit Arm, there are no unaligned access allowed. However, the > change of BIT_WORD is not a concern for 32-bit. > > I haven't check whether we still have places where bitops are used with > 4 byte aligned memory. However, as the bitops take a void * in > parameter, there are no promise on the alignment. I'm pretty sure for x86 the 32-bit guest compat code uses such, at the very least. > Therefore, we can't rewrite BIT_WORD without addressing the underlying > issues. Introducing BIT_LONG is probably the easiest way at the moment. Which would make use (continue to) deviate from Linux'es meaning of BIT_WORD(). > However, our bitops really ought to specify the alignment in parameter > to avoid such issues arising. > > I would be in favor of using unsigned long *. I don't think they should, as this complicates uses on non-64-bit quantities. In fact I think bitops would better be permitted also on sub-32-bit values. But anyway - x86 under the hood uses 32-bit memory accesses too, in a number of cases. It's not obvious to me why Arm64 couldn't do so as well, despite BIT_WORD() - for the purposes of generic code - assuming "unsigned long" to be the base "word". Jan
Hi Jan, On 10/02/2020 08:43, Jan Beulich wrote: > On 08.02.2020 15:37, Julien Grall wrote: >> >> >> On 05/02/2020 13:27, Jan Beulich wrote: >>> On 05.02.2020 14:21, Roger Pau Monné wrote: >>>> On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote: >>>>> On 04.02.2020 18:34, Roger Pau Monne wrote: >>>>>> Import the functions and it's dependencies. Based on Linux 5.5, commit >>>>>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755. >>>>>> >>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>>>> >>>>> Thanks for going this route; two remarks / requests: >>>>> >>>>>> --- a/xen/common/bitmap.c >>>>>> +++ b/xen/common/bitmap.c >>>>>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits) >>>>>> #endif >>>>>> EXPORT_SYMBOL(__bitmap_weight); >>>>>> >>>>>> +void __bitmap_set(unsigned long *map, unsigned int start, int len) >>>>>> +{ >>>>>> + unsigned long *p = map + BIT_WORD(start); >>>>>> + const unsigned int size = start + len; >>>>>> + int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); >>>>>> + unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); >>>>>> + >>>>>> + while (len - bits_to_set >= 0) { >>>>>> + *p |= mask_to_set; >>>>>> + len -= bits_to_set; >>>>>> + bits_to_set = BITS_PER_LONG; >>>>>> + mask_to_set = ~0UL; >>>>>> + p++; >>>>>> + } >>>>>> + if (len) { >>>>>> + mask_to_set &= BITMAP_LAST_WORD_MASK(size); >>>>>> + *p |= mask_to_set; >>>>>> + } >>>>>> +} >>>>>> +EXPORT_SYMBOL(__bitmap_set); >>>>>> + >>>>>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len) >>>>>> +{ >>>>>> + unsigned long *p = map + BIT_WORD(start); >>>>>> + const unsigned int size = start + len; >>>>>> + int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); >>>>>> + unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); >>>>>> + >>>>>> + while (len - bits_to_clear >= 0) { >>>>>> + *p &= ~mask_to_clear; >>>>>> + len -= bits_to_clear; >>>>>> + bits_to_clear = BITS_PER_LONG; >>>>>> + mask_to_clear = ~0UL; >>>>>> + p++; >>>>>> + } >>>>>> + if (len) { >>>>>> + mask_to_clear &= BITMAP_LAST_WORD_MASK(size); >>>>>> + *p &= ~mask_to_clear; >>>>>> + } >>>>>> +} >>>>>> +EXPORT_SYMBOL(__bitmap_clear); >>>>> >>>>> Despite all the other EXPORT_SYMBOL() in this file, personally I >>>>> would suggest to refrain from adding more. But I'm not going to >>>>> insist (until such time that they all get cleaned up). >>>>> >>>>>> --- a/xen/include/asm-x86/bitops.h >>>>>> +++ b/xen/include/asm-x86/bitops.h >>>>>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x) >>>>>> #define hweight16(x) generic_hweight16(x) >>>>>> #define hweight8(x) generic_hweight8(x) >>>>>> >>>>>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) >>>>> >>>>> At first I thought - why for x86 only? Then I noticed Arm has an >>>>> almost identical #define already. Which in turn made me look at >>>>> Linux, where that #define lives in a common header. I think you >>>>> want to move the Arm one. Or wait, no - Arm's isn't even >>>>> compatible with the implementations of the functions you add. >>>>> This definitely needs taking care of, perhaps by way of ignoring >>>>> my request to go this route (as getting too involved). >>>> >>>> Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be >>>> used when the bitmap is mapped to an array of 32bit type elements. >>>> >>>> I could introduce BIT_LONG that would have the same definition on Arm >>>> and x86, and then modify the imported functions to use it, but IMO the >>>> right solution would be to change the Arm BIT_WORD macro to also use >>>> BITS_PER_LONG (and adjust the callers). >>> >>> So do I. Julien, Stefano? >> >> BIT_WORD used to use BITS_PER_LONG but this was changed in commit: >> >> commit cd338e967c598bf747b03dcfd9d8d45dc40bac1a >> Author: Ian Campbell <ian.campbell@citrix.com> >> Date: Thu May 8 16:13:55 2014 +0100 >> >> xen: arm: bitops take unsigned int >> >> Xen bitmaps can be 4 rather than 8 byte aligned, so use the >> appropriate type. >> Otherwise the compiler can generate unaligned 8 byte accesses and >> cause traps. >> >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> >> On 64-bit Arm, while we allow unaligned access, the atomic operations >> still enforce alignment. >> >> On 32-bit Arm, there are no unaligned access allowed. However, the >> change of BIT_WORD is not a concern for 32-bit. >> >> I haven't check whether we still have places where bitops are used with >> 4 byte aligned memory. However, as the bitops take a void * in >> parameter, there are no promise on the alignment. > > I'm pretty sure for x86 the 32-bit guest compat code uses such, at > the very least. I have spent some times looking at it and noticed, there are some in the common code (e.g scheduler, IRQ...). > >> Therefore, we can't rewrite BIT_WORD without addressing the underlying >> issues. Introducing BIT_LONG is probably the easiest way at the moment. > > Which would make use (continue to) deviate from Linux'es meaning of > BIT_WORD(). This would not be really the first time we deviate from Linux... We could possibly rename BIT_WORD to something different, but I don't have a good name, hence why I think BIT_LONG is the best solution so far. > >> However, our bitops really ought to specify the alignment in parameter >> to avoid such issues arising. >> >> I would be in favor of using unsigned long *. > > I don't think they should, as this complicates uses on non-64-bit > quantities. In fact I think bitops would better be permitted also > on sub-32-bit values. But anyway - x86 under the hood uses 32-bit > memory accesses too, in a number of cases. It's not obvious to me > why Arm64 couldn't do so as well, despite BIT_WORD() - for the > purposes of generic code - assuming "unsigned long" to be the base > "word". My point is we should avoid ot use void * and implicetly require an alignment (32-bit at the moment). This has resulted to numerous issues in the past on Arm. To be clear, I am not requesting to handle the void *. Anyway, blindly updating BIT_WORD() is going to break Arm. So you either rename to current macro or create a new one. Cheers,
On 10.02.2020 10:20, Julien Grall wrote: > Hi Jan, > > On 10/02/2020 08:43, Jan Beulich wrote: >> On 08.02.2020 15:37, Julien Grall wrote: >>> >>> >>> On 05/02/2020 13:27, Jan Beulich wrote: >>>> On 05.02.2020 14:21, Roger Pau Monné wrote: >>>>> On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote: >>>>>> On 04.02.2020 18:34, Roger Pau Monne wrote: >>>>>>> Import the functions and it's dependencies. Based on Linux 5.5, commit >>>>>>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755. >>>>>>> >>>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>>>>> >>>>>> Thanks for going this route; two remarks / requests: >>>>>> >>>>>>> --- a/xen/common/bitmap.c >>>>>>> +++ b/xen/common/bitmap.c >>>>>>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits) >>>>>>> #endif >>>>>>> EXPORT_SYMBOL(__bitmap_weight); >>>>>>> >>>>>>> +void __bitmap_set(unsigned long *map, unsigned int start, int len) >>>>>>> +{ >>>>>>> + unsigned long *p = map + BIT_WORD(start); >>>>>>> + const unsigned int size = start + len; >>>>>>> + int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); >>>>>>> + unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); >>>>>>> + >>>>>>> + while (len - bits_to_set >= 0) { >>>>>>> + *p |= mask_to_set; >>>>>>> + len -= bits_to_set; >>>>>>> + bits_to_set = BITS_PER_LONG; >>>>>>> + mask_to_set = ~0UL; >>>>>>> + p++; >>>>>>> + } >>>>>>> + if (len) { >>>>>>> + mask_to_set &= BITMAP_LAST_WORD_MASK(size); >>>>>>> + *p |= mask_to_set; >>>>>>> + } >>>>>>> +} >>>>>>> +EXPORT_SYMBOL(__bitmap_set); >>>>>>> + >>>>>>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len) >>>>>>> +{ >>>>>>> + unsigned long *p = map + BIT_WORD(start); >>>>>>> + const unsigned int size = start + len; >>>>>>> + int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); >>>>>>> + unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); >>>>>>> + >>>>>>> + while (len - bits_to_clear >= 0) { >>>>>>> + *p &= ~mask_to_clear; >>>>>>> + len -= bits_to_clear; >>>>>>> + bits_to_clear = BITS_PER_LONG; >>>>>>> + mask_to_clear = ~0UL; >>>>>>> + p++; >>>>>>> + } >>>>>>> + if (len) { >>>>>>> + mask_to_clear &= BITMAP_LAST_WORD_MASK(size); >>>>>>> + *p &= ~mask_to_clear; >>>>>>> + } >>>>>>> +} >>>>>>> +EXPORT_SYMBOL(__bitmap_clear); >>>>>> >>>>>> Despite all the other EXPORT_SYMBOL() in this file, personally I >>>>>> would suggest to refrain from adding more. But I'm not going to >>>>>> insist (until such time that they all get cleaned up). >>>>>> >>>>>>> --- a/xen/include/asm-x86/bitops.h >>>>>>> +++ b/xen/include/asm-x86/bitops.h >>>>>>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x) >>>>>>> #define hweight16(x) generic_hweight16(x) >>>>>>> #define hweight8(x) generic_hweight8(x) >>>>>>> >>>>>>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) >>>>>> >>>>>> At first I thought - why for x86 only? Then I noticed Arm has an >>>>>> almost identical #define already. Which in turn made me look at >>>>>> Linux, where that #define lives in a common header. I think you >>>>>> want to move the Arm one. Or wait, no - Arm's isn't even >>>>>> compatible with the implementations of the functions you add. >>>>>> This definitely needs taking care of, perhaps by way of ignoring >>>>>> my request to go this route (as getting too involved). >>>>> >>>>> Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be >>>>> used when the bitmap is mapped to an array of 32bit type elements. >>>>> >>>>> I could introduce BIT_LONG that would have the same definition on Arm >>>>> and x86, and then modify the imported functions to use it, but IMO the >>>>> right solution would be to change the Arm BIT_WORD macro to also use >>>>> BITS_PER_LONG (and adjust the callers). >>>> >>>> So do I. Julien, Stefano? >>> >>> BIT_WORD used to use BITS_PER_LONG but this was changed in commit: >>> >>> commit cd338e967c598bf747b03dcfd9d8d45dc40bac1a >>> Author: Ian Campbell <ian.campbell@citrix.com> >>> Date: Thu May 8 16:13:55 2014 +0100 >>> >>> xen: arm: bitops take unsigned int >>> >>> Xen bitmaps can be 4 rather than 8 byte aligned, so use the >>> appropriate type. >>> Otherwise the compiler can generate unaligned 8 byte accesses and >>> cause traps. >>> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >>> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>> >>> On 64-bit Arm, while we allow unaligned access, the atomic operations >>> still enforce alignment. >>> >>> On 32-bit Arm, there are no unaligned access allowed. However, the >>> change of BIT_WORD is not a concern for 32-bit. >>> >>> I haven't check whether we still have places where bitops are used with >>> 4 byte aligned memory. However, as the bitops take a void * in >>> parameter, there are no promise on the alignment. >> >> I'm pretty sure for x86 the 32-bit guest compat code uses such, at >> the very least. > > I have spent some times looking at it and noticed, there are some in the > common code (e.g scheduler, IRQ...). > >> >>> Therefore, we can't rewrite BIT_WORD without addressing the underlying >>> issues. Introducing BIT_LONG is probably the easiest way at the moment. >> >> Which would make use (continue to) deviate from Linux'es meaning of >> BIT_WORD(). > > This would not be really the first time we deviate from Linux... Of course. And there've been quite a few cases where I've argued towards deviation. It's just that iirc you're one of those who prefer less deviation, so I've been a little puzzled. >>> However, our bitops really ought to specify the alignment in parameter >>> to avoid such issues arising. >>> >>> I would be in favor of using unsigned long *. >> >> I don't think they should, as this complicates uses on non-64-bit >> quantities. In fact I think bitops would better be permitted also >> on sub-32-bit values. But anyway - x86 under the hood uses 32-bit >> memory accesses too, in a number of cases. It's not obvious to me >> why Arm64 couldn't do so as well, despite BIT_WORD() - for the >> purposes of generic code - assuming "unsigned long" to be the base >> "word". > > My point is we should avoid ot use void * and implicetly require an > alignment (32-bit at the moment). This has resulted to numerous issues > in the past on Arm. See how/why we have bitop_bad_size() on x86. > To be clear, I am not requesting to handle the void *. > > Anyway, blindly updating BIT_WORD() is going to break Arm. So you either > rename to current macro or create a new one. That's understood. The question was rather what direction to go to resolve the issue. Jan
Hi Jan, On 10/02/2020 09:31, Jan Beulich wrote: > On 10.02.2020 10:20, Julien Grall wrote: >> Hi Jan, >> >> On 10/02/2020 08:43, Jan Beulich wrote: >>> On 08.02.2020 15:37, Julien Grall wrote: >>>> >>>> >>>> On 05/02/2020 13:27, Jan Beulich wrote: >>>>> On 05.02.2020 14:21, Roger Pau Monné wrote: >>>>>> On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote: >>>>>>> On 04.02.2020 18:34, Roger Pau Monne wrote: >>>>>>>> Import the functions and it's dependencies. Based on Linux 5.5, commit >>>>>>>> id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755. >>>>>>>> >>>>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>>>>>> >>>>>>> Thanks for going this route; two remarks / requests: >>>>>>> >>>>>>>> --- a/xen/common/bitmap.c >>>>>>>> +++ b/xen/common/bitmap.c >>>>>>>> @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits) >>>>>>>> #endif >>>>>>>> EXPORT_SYMBOL(__bitmap_weight); >>>>>>>> >>>>>>>> +void __bitmap_set(unsigned long *map, unsigned int start, int len) >>>>>>>> +{ >>>>>>>> + unsigned long *p = map + BIT_WORD(start); >>>>>>>> + const unsigned int size = start + len; >>>>>>>> + int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); >>>>>>>> + unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); >>>>>>>> + >>>>>>>> + while (len - bits_to_set >= 0) { >>>>>>>> + *p |= mask_to_set; >>>>>>>> + len -= bits_to_set; >>>>>>>> + bits_to_set = BITS_PER_LONG; >>>>>>>> + mask_to_set = ~0UL; >>>>>>>> + p++; >>>>>>>> + } >>>>>>>> + if (len) { >>>>>>>> + mask_to_set &= BITMAP_LAST_WORD_MASK(size); >>>>>>>> + *p |= mask_to_set; >>>>>>>> + } >>>>>>>> +} >>>>>>>> +EXPORT_SYMBOL(__bitmap_set); >>>>>>>> + >>>>>>>> +void __bitmap_clear(unsigned long *map, unsigned int start, int len) >>>>>>>> +{ >>>>>>>> + unsigned long *p = map + BIT_WORD(start); >>>>>>>> + const unsigned int size = start + len; >>>>>>>> + int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); >>>>>>>> + unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); >>>>>>>> + >>>>>>>> + while (len - bits_to_clear >= 0) { >>>>>>>> + *p &= ~mask_to_clear; >>>>>>>> + len -= bits_to_clear; >>>>>>>> + bits_to_clear = BITS_PER_LONG; >>>>>>>> + mask_to_clear = ~0UL; >>>>>>>> + p++; >>>>>>>> + } >>>>>>>> + if (len) { >>>>>>>> + mask_to_clear &= BITMAP_LAST_WORD_MASK(size); >>>>>>>> + *p &= ~mask_to_clear; >>>>>>>> + } >>>>>>>> +} >>>>>>>> +EXPORT_SYMBOL(__bitmap_clear); >>>>>>> >>>>>>> Despite all the other EXPORT_SYMBOL() in this file, personally I >>>>>>> would suggest to refrain from adding more. But I'm not going to >>>>>>> insist (until such time that they all get cleaned up). >>>>>>> >>>>>>>> --- a/xen/include/asm-x86/bitops.h >>>>>>>> +++ b/xen/include/asm-x86/bitops.h >>>>>>>> @@ -480,4 +480,6 @@ static inline int fls(unsigned int x) >>>>>>>> #define hweight16(x) generic_hweight16(x) >>>>>>>> #define hweight8(x) generic_hweight8(x) >>>>>>>> >>>>>>>> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) >>>>>>> >>>>>>> At first I thought - why for x86 only? Then I noticed Arm has an >>>>>>> almost identical #define already. Which in turn made me look at >>>>>>> Linux, where that #define lives in a common header. I think you >>>>>>> want to move the Arm one. Or wait, no - Arm's isn't even >>>>>>> compatible with the implementations of the functions you add. >>>>>>> This definitely needs taking care of, perhaps by way of ignoring >>>>>>> my request to go this route (as getting too involved). >>>>>> >>>>>> Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be >>>>>> used when the bitmap is mapped to an array of 32bit type elements. >>>>>> >>>>>> I could introduce BIT_LONG that would have the same definition on Arm >>>>>> and x86, and then modify the imported functions to use it, but IMO the >>>>>> right solution would be to change the Arm BIT_WORD macro to also use >>>>>> BITS_PER_LONG (and adjust the callers). >>>>> >>>>> So do I. Julien, Stefano? >>>> >>>> BIT_WORD used to use BITS_PER_LONG but this was changed in commit: >>>> >>>> commit cd338e967c598bf747b03dcfd9d8d45dc40bac1a >>>> Author: Ian Campbell <ian.campbell@citrix.com> >>>> Date: Thu May 8 16:13:55 2014 +0100 >>>> >>>> xen: arm: bitops take unsigned int >>>> >>>> Xen bitmaps can be 4 rather than 8 byte aligned, so use the >>>> appropriate type. >>>> Otherwise the compiler can generate unaligned 8 byte accesses and >>>> cause traps. >>>> >>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >>>> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>>> >>>> On 64-bit Arm, while we allow unaligned access, the atomic operations >>>> still enforce alignment. >>>> >>>> On 32-bit Arm, there are no unaligned access allowed. However, the >>>> change of BIT_WORD is not a concern for 32-bit. >>>> >>>> I haven't check whether we still have places where bitops are used with >>>> 4 byte aligned memory. However, as the bitops take a void * in >>>> parameter, there are no promise on the alignment. >>> >>> I'm pretty sure for x86 the 32-bit guest compat code uses such, at >>> the very least. >> >> I have spent some times looking at it and noticed, there are some in the >> common code (e.g scheduler, IRQ...). >> >>> >>>> Therefore, we can't rewrite BIT_WORD without addressing the underlying >>>> issues. Introducing BIT_LONG is probably the easiest way at the moment. >>> >>> Which would make use (continue to) deviate from Linux'es meaning of >>> BIT_WORD(). >> >> This would not be really the first time we deviate from Linux... > > Of course. And there've been quite a few cases where I've argued > towards deviation. It's just that iirc you're one of those who > prefer less deviation, so I've been a little puzzled. I have been advocating deviation in a few cases ;). > >>>> However, our bitops really ought to specify the alignment in parameter >>>> to avoid such issues arising. >>>> >>>> I would be in favor of using unsigned long *. >>> >>> I don't think they should, as this complicates uses on non-64-bit >>> quantities. In fact I think bitops would better be permitted also >>> on sub-32-bit values. But anyway - x86 under the hood uses 32-bit >>> memory accesses too, in a number of cases. It's not obvious to me >>> why Arm64 couldn't do so as well, despite BIT_WORD() - for the >>> purposes of generic code - assuming "unsigned long" to be the base >>> "word". >> >> My point is we should avoid ot use void * and implicetly require an >> alignment (32-bit at the moment). This has resulted to numerous issues >> in the past on Arm. > > See how/why we have bitop_bad_size() on x86. Ah, I didn't spot the bitop_bad_size(). We could possibly use the same trick on Arm. > >> To be clear, I am not requesting to handle the void *. >> >> Anyway, blindly updating BIT_WORD() is going to break Arm. So you either >> rename to current macro or create a new one. > > That's understood. The question was rather what direction to go to > resolve the issue. Please suggest a new name for BIT_WORD() and we can repurpose it. So far, I have no idea how to rename it. Cheers,
On 10.02.2020 10:45, Julien Grall wrote: > Please suggest a new name for BIT_WORD() and we can repurpose it. So > far, I have no idea how to rename it. _BIT_WORD() if you/we were to accept the name space violation, or BITMAP_WORD()? Jan
On 10/02/2020 10:28, Jan Beulich wrote: > On 10.02.2020 10:45, Julien Grall wrote: >> Please suggest a new name for BIT_WORD() and we can repurpose it. So >> far, I have no idea how to rename it. > > _BIT_WORD() if you/we were to accept the name space violation, or > BITMAP_WORD()? BITMAP_WORD() is misleading as bitmap are using unsigned long. So my preference is _BIT_WORD(). Another alternative would be ATOMIC_WORD(). Cheers,
On 10.02.2020 12:00, Julien Grall wrote: > On 10/02/2020 10:28, Jan Beulich wrote: >> On 10.02.2020 10:45, Julien Grall wrote: >>> Please suggest a new name for BIT_WORD() and we can repurpose it. So >>> far, I have no idea how to rename it. >> >> _BIT_WORD() if you/we were to accept the name space violation, or >> BITMAP_WORD()? > > BITMAP_WORD() is misleading as bitmap are using unsigned long. So my > preference is _BIT_WORD(). > > Another alternative would be ATOMIC_WORD(). Except that there are also non-atomic bitmap operations, I don't really care about the name as long as it's not BIT_WORD() (or anything else that's likely to collide with other stuff). Jan
Hi, On 10/02/2020 11:59, Jan Beulich wrote: > On 10.02.2020 12:00, Julien Grall wrote: >> On 10/02/2020 10:28, Jan Beulich wrote: >>> On 10.02.2020 10:45, Julien Grall wrote: >>>> Please suggest a new name for BIT_WORD() and we can repurpose it. So >>>> far, I have no idea how to rename it. >>> >>> _BIT_WORD() if you/we were to accept the name space violation, or >>> BITMAP_WORD()? >> >> BITMAP_WORD() is misleading as bitmap are using unsigned long. So my >> preference is _BIT_WORD(). >> >> Another alternative would be ATOMIC_WORD(). > > Except that there are also non-atomic bitmap operations, I don't really > care about the name as long as it's not BIT_WORD() (or anything else > that's likely to collide with other stuff. I am afraid we are disagreing on what is colliding with what here. The naming on Arm has been like that for the past few years. While this may not have been the best choice, this is your suggestion colliding with what is existing. I am not entirely fussed about the namespace violation, although I think the name is potentially misleading. Yet, I would be happy to use _BIT_WORD() as this is the best of it so far. While this is code falls under Arm maintainership, I am still happy to consider other naming. But at this point, you should be the one suggesting. Cheers,
On 10.02.2020 13:21, Julien Grall wrote: > Hi, > > On 10/02/2020 11:59, Jan Beulich wrote: >> On 10.02.2020 12:00, Julien Grall wrote: >>> On 10/02/2020 10:28, Jan Beulich wrote: >>>> On 10.02.2020 10:45, Julien Grall wrote: >>>>> Please suggest a new name for BIT_WORD() and we can repurpose it. So >>>>> far, I have no idea how to rename it. >>>> >>>> _BIT_WORD() if you/we were to accept the name space violation, or >>>> BITMAP_WORD()? >>> >>> BITMAP_WORD() is misleading as bitmap are using unsigned long. So my >>> preference is _BIT_WORD(). >>> >>> Another alternative would be ATOMIC_WORD(). >> >> Except that there are also non-atomic bitmap operations, I don't really >> care about the name as long as it's not BIT_WORD() (or anything else >> that's likely to collide with other stuff. > > I am afraid we are disagreing on what is colliding with what here. The > naming on Arm has been like that for the past few years. While this may > not have been the best choice, this is your suggestion colliding with > what is existing. It is a plain import from Linux which has turned out impossible because of the change that was done at some point to Arm code which, I guess, also originally came from Linux. There's no new naming I've been suggesting here at all. > I am not entirely fussed about the namespace violation, although I think > the name is potentially misleading. Yet, I would be happy to use > _BIT_WORD() as this is the best of it so far. > > While this is code falls under Arm maintainership, I am still happy to > consider other naming. But at this point, you should be the one suggesting. BIT_UNIT() or BITOP_UNIT() or BITOP_WORD()? Jan
On 10/02/2020 12:32, Jan Beulich wrote: > On 10.02.2020 13:21, Julien Grall wrote: >> Hi, >> >> On 10/02/2020 11:59, Jan Beulich wrote: >>> On 10.02.2020 12:00, Julien Grall wrote: >>>> On 10/02/2020 10:28, Jan Beulich wrote: >>>>> On 10.02.2020 10:45, Julien Grall wrote: >>>>>> Please suggest a new name for BIT_WORD() and we can repurpose it. So >>>>>> far, I have no idea how to rename it. >>>>> >>>>> _BIT_WORD() if you/we were to accept the name space violation, or >>>>> BITMAP_WORD()? >>>> >>>> BITMAP_WORD() is misleading as bitmap are using unsigned long. So my >>>> preference is _BIT_WORD(). >>>> >>>> Another alternative would be ATOMIC_WORD(). >>> >>> Except that there are also non-atomic bitmap operations, I don't really >>> care about the name as long as it's not BIT_WORD() (or anything else >>> that's likely to collide with other stuff. >> >> I am afraid we are disagreing on what is colliding with what here. The >> naming on Arm has been like that for the past few years. While this may >> not have been the best choice, this is your suggestion colliding with >> what is existing. > > It is a plain import from Linux which has turned out impossible > because of the change that was done at some point to Arm code > which, I guess, also originally came from Linux. There's no new > naming I've been suggesting here at all. We never claimed we would be fully compatible with Linux and I don't think we could every claim it. Particularly, the bitop operations are different given Linux bitops are based on unsigned long. The bitop did indeed came from Linux originally, however we had to adapt them because Linux Armv8 bitop was expecting 8-byte aligned. This does not hold on Xen. > >> I am not entirely fussed about the namespace violation, although I think >> the name is potentially misleading. Yet, I would be happy to use >> _BIT_WORD() as this is the best of it so far. >> >> While this is code falls under Arm maintainership, I am still happy to >> consider other naming. But at this point, you should be the one suggesting. > > BIT_UNIT() or BITOP_UNIT() or BITOP_WORD()? BITOP_WORD(). Cheers,
On Mon, Feb 10, 2020 at 12:54:04PM +0000, Julien Grall wrote: > > > On 10/02/2020 12:32, Jan Beulich wrote: > > On 10.02.2020 13:21, Julien Grall wrote: > > > Hi, > > > > > > On 10/02/2020 11:59, Jan Beulich wrote: > > > > On 10.02.2020 12:00, Julien Grall wrote: > > > > > On 10/02/2020 10:28, Jan Beulich wrote: > > > > > > On 10.02.2020 10:45, Julien Grall wrote: > > > > > > > Please suggest a new name for BIT_WORD() and we can repurpose it. So > > > > > > > far, I have no idea how to rename it. > > > > > > > > > > > > _BIT_WORD() if you/we were to accept the name space violation, or > > > > > > BITMAP_WORD()? > > > > > > > > > > BITMAP_WORD() is misleading as bitmap are using unsigned long. So my > > > > > preference is _BIT_WORD(). > > > > > > > > > > Another alternative would be ATOMIC_WORD(). > > > > > > > > Except that there are also non-atomic bitmap operations, I don't really > > > > care about the name as long as it's not BIT_WORD() (or anything else > > > > that's likely to collide with other stuff. > > > > > > I am afraid we are disagreing on what is colliding with what here. The > > > naming on Arm has been like that for the past few years. While this may > > > not have been the best choice, this is your suggestion colliding with > > > what is existing. > > > > It is a plain import from Linux which has turned out impossible > > because of the change that was done at some point to Arm code > > which, I guess, also originally came from Linux. There's no new > > naming I've been suggesting here at all. > > We never claimed we would be fully compatible with Linux and I don't think > we could every claim it. Particularly, the bitop operations are different > given Linux bitops are based on unsigned long. > > The bitop did indeed came from Linux originally, however we had to adapt > them because Linux Armv8 bitop was expecting 8-byte aligned. This does not > hold on Xen. > > > > > > I am not entirely fussed about the namespace violation, although I think > > > the name is potentially misleading. Yet, I would be happy to use > > > _BIT_WORD() as this is the best of it so far. > > > > > > While this is code falls under Arm maintainership, I am still happy to > > > consider other naming. But at this point, you should be the one suggesting. > > > > BIT_UNIT() or BITOP_UNIT() or BITOP_WORD()? > > BITOP_WORD(). So I assume you would like me to import bitmap_{set/clear} and use it in the x2APIC MSR patch? Thanks, Roger.
On 14.02.2020 11:40, Roger Pau Monné wrote: > On Mon, Feb 10, 2020 at 12:54:04PM +0000, Julien Grall wrote: >> >> >> On 10/02/2020 12:32, Jan Beulich wrote: >>> On 10.02.2020 13:21, Julien Grall wrote: >>>> Hi, >>>> >>>> On 10/02/2020 11:59, Jan Beulich wrote: >>>>> On 10.02.2020 12:00, Julien Grall wrote: >>>>>> On 10/02/2020 10:28, Jan Beulich wrote: >>>>>>> On 10.02.2020 10:45, Julien Grall wrote: >>>>>>>> Please suggest a new name for BIT_WORD() and we can repurpose it. So >>>>>>>> far, I have no idea how to rename it. >>>>>>> >>>>>>> _BIT_WORD() if you/we were to accept the name space violation, or >>>>>>> BITMAP_WORD()? >>>>>> >>>>>> BITMAP_WORD() is misleading as bitmap are using unsigned long. So my >>>>>> preference is _BIT_WORD(). >>>>>> >>>>>> Another alternative would be ATOMIC_WORD(). >>>>> >>>>> Except that there are also non-atomic bitmap operations, I don't really >>>>> care about the name as long as it's not BIT_WORD() (or anything else >>>>> that's likely to collide with other stuff. >>>> >>>> I am afraid we are disagreing on what is colliding with what here. The >>>> naming on Arm has been like that for the past few years. While this may >>>> not have been the best choice, this is your suggestion colliding with >>>> what is existing. >>> >>> It is a plain import from Linux which has turned out impossible >>> because of the change that was done at some point to Arm code >>> which, I guess, also originally came from Linux. There's no new >>> naming I've been suggesting here at all. >> >> We never claimed we would be fully compatible with Linux and I don't think >> we could every claim it. Particularly, the bitop operations are different >> given Linux bitops are based on unsigned long. >> >> The bitop did indeed came from Linux originally, however we had to adapt >> them because Linux Armv8 bitop was expecting 8-byte aligned. This does not >> hold on Xen. >> >>> >>>> I am not entirely fussed about the namespace violation, although I think >>>> the name is potentially misleading. Yet, I would be happy to use >>>> _BIT_WORD() as this is the best of it so far. >>>> >>>> While this is code falls under Arm maintainership, I am still happy to >>>> consider other naming. But at this point, you should be the one suggesting. >>> >>> BIT_UNIT() or BITOP_UNIT() or BITOP_WORD()? >> >> BITOP_WORD(). > > So I assume you would like me to import bitmap_{set/clear} and use it > in the x2APIC MSR patch? Well, it's really up to you. I've put this as an item on my own todo list already, assuming there may be more places where the two functions might turn out useful. Jan
diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c index fd070bee97..f8b243e77e 100644 --- a/xen/common/bitmap.c +++ b/xen/common/bitmap.c @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int bits) #endif EXPORT_SYMBOL(__bitmap_weight); +void __bitmap_set(unsigned long *map, unsigned int start, int len) +{ + unsigned long *p = map + BIT_WORD(start); + const unsigned int size = start + len; + int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG); + unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start); + + while (len - bits_to_set >= 0) { + *p |= mask_to_set; + len -= bits_to_set; + bits_to_set = BITS_PER_LONG; + mask_to_set = ~0UL; + p++; + } + if (len) { + mask_to_set &= BITMAP_LAST_WORD_MASK(size); + *p |= mask_to_set; + } +} +EXPORT_SYMBOL(__bitmap_set); + +void __bitmap_clear(unsigned long *map, unsigned int start, int len) +{ + unsigned long *p = map + BIT_WORD(start); + const unsigned int size = start + len; + int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG); + unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start); + + while (len - bits_to_clear >= 0) { + *p &= ~mask_to_clear; + len -= bits_to_clear; + bits_to_clear = BITS_PER_LONG; + mask_to_clear = ~0UL; + p++; + } + if (len) { + mask_to_clear &= BITMAP_LAST_WORD_MASK(size); + *p &= ~mask_to_clear; + } +} +EXPORT_SYMBOL(__bitmap_clear); /** * bitmap_find_free_region - find a contiguous aligned mem region diff --git a/xen/include/asm-x86/bitops.h b/xen/include/asm-x86/bitops.h index 5a71afbc89..04b1530388 100644 --- a/xen/include/asm-x86/bitops.h +++ b/xen/include/asm-x86/bitops.h @@ -480,4 +480,6 @@ static inline int fls(unsigned int x) #define hweight16(x) generic_hweight16(x) #define hweight8(x) generic_hweight8(x) +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) + #endif /* _X86_BITOPS_H */ diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h index 4e1e690af1..f07d1f6935 100644 --- a/xen/include/xen/bitmap.h +++ b/xen/include/xen/bitmap.h @@ -85,6 +85,8 @@ extern int __bitmap_intersects(const unsigned long *bitmap1, extern int __bitmap_subset(const unsigned long *bitmap1, const unsigned long *bitmap2, int bits); extern int __bitmap_weight(const unsigned long *bitmap, int bits); +extern void __bitmap_set(unsigned long *map, unsigned int start, int len); +extern void __bitmap_clear(unsigned long *map, unsigned int start, int len); extern int bitmap_find_free_region(unsigned long *bitmap, int bits, int order); extern void bitmap_release_region(unsigned long *bitmap, int pos, int order); @@ -227,6 +229,42 @@ static inline int bitmap_weight(const unsigned long *src, int nbits) return __bitmap_weight(src, nbits); } +#ifdef __LITTLE_ENDIAN +#define BITMAP_MEM_ALIGNMENT 8 +#else +#define BITMAP_MEM_ALIGNMENT (8 * sizeof(unsigned long)) +#endif +#define BITMAP_MEM_MASK (BITMAP_MEM_ALIGNMENT - 1) +#define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1))) + +static inline void bitmap_set(unsigned long *map, unsigned int start, + unsigned int nbits) +{ + if (__builtin_constant_p(nbits) && nbits == 1) + __set_bit(start, map); + else if (__builtin_constant_p(start & BITMAP_MEM_MASK) && + IS_ALIGNED(start, BITMAP_MEM_ALIGNMENT) && + __builtin_constant_p(nbits & BITMAP_MEM_MASK) && + IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT)) + memset((char *)map + start / 8, 0xff, nbits / 8); + else + __bitmap_set(map, start, nbits); +} + +static inline void bitmap_clear(unsigned long *map, unsigned int start, + unsigned int nbits) +{ + if (__builtin_constant_p(nbits) && nbits == 1) + __clear_bit(start, map); + else if (__builtin_constant_p(start & BITMAP_MEM_MASK) && + IS_ALIGNED(start, BITMAP_MEM_ALIGNMENT) && + __builtin_constant_p(nbits & BITMAP_MEM_MASK) && + IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT)) + memset((char *)map + start / 8, 0, nbits / 8); + else + __bitmap_clear(map, start, nbits); +} + #undef bitmap_switch #undef bitmap_bytes
Import the functions and it's dependencies. Based on Linux 5.5, commit id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/common/bitmap.c | 41 ++++++++++++++++++++++++++++++++++++ xen/include/asm-x86/bitops.h | 2 ++ xen/include/xen/bitmap.h | 38 +++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+)