Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
>      Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel