[Xen-devel,for-4.7,1/2] xen/bitops: Introduce macros to generate mask

Message ID 1460562931-19858-2-git-send-email-julien.grall@arm.com
State New
Headers show

Commit Message

Julien Grall April 13, 2016, 3:55 p.m.
The code has been imported from the header include/linux/bitops.h in
Linux v4.6-rc3.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/xen/bitops.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Julien Grall April 14, 2016, 8:47 a.m. | #1
Hi Andrew,

On 13/04/2016 19:14, Andrew Cooper wrote:
> On 13/04/16 16:55, Julien Grall wrote:
>> The code has been imported from the header include/linux/bitops.h in
>> Linux v4.6-rc3.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Tim Deegan <tim@xen.org>
>> ---
>>   xen/include/xen/bitops.h | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
>> index cb56f24..e1a4d93 100644
>> --- a/xen/include/xen/bitops.h
>> +++ b/xen/include/xen/bitops.h
>> @@ -3,6 +3,17 @@
>>   #include <asm/types.h>
>>
>>   /*
>> + * Create a contiguous bitmask starting at bit position @l and ending at
>> + * position @h. For example
>> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>> + */
>> +#define GENMASK(h, l) \
>> +	(((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>> +
>> +#define GENMASK_ULL(h, l) \
>> +	(((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>
> You should have just a single GENMASK() which works in terms of LL.
>
> Masks must be signed to work correctly when implicitly extended.

May I ask you why? AFAICT, if the mask is signed, it will result to 
duplicate the sign bit to the new bits. This is not the expected 
behavior of this macro.

For instance, the following patch is using this macro to mask RES0 bits 
in the HPFAR_EL2 register. For ARM, RES0 means the bit is currently read 
as zero but the software should not rely on it to preserve forward 
compatibility.

Regards,
Julien Grall April 14, 2016, 8:55 a.m. | #2
Hi Jan,

On 14/04/2016 05:01, Jan Beulich wrote:
>>>> Julien Grall <julien.grall@arm.com> 04/13/16 6:01 PM >>>
>> --- a/xen/include/xen/bitops.h
>> +++ b/xen/include/xen/bitops.h
>> @@ -3,6 +3,17 @@
>   >#include <asm/types.h>
>   >
>   >/*
>> + * Create a contiguous bitmask starting at bit position @l and ending at
>> + * position @h. For example
>> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>> + */
>> +#define GENMASK(h, l) \
>> +    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>> +
>> +#define GENMASK_ULL(h, l) \
>> +    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>
> Irrespective of Linux perhaps considering them useful, I'm not sure they
> are (and ISTR these macros having got proposed before).

This is useful on ARM to generate mask for register. For instance, the 
following patch introduces mask for the register HPFAR_EL2. Only the 
bits [4:39] are usable, the rest are RES0.

For ARM, RES0 means the bit is currently read as zero but the software 
should not rely on it to preserve forward compatibility. So we want to 
mask those bits to avoid breakage with new version of the architecture.

 > Plus - I don't
> think we even have BITS_PER_LONG_LONG anywhere.

Hmmm right, we don't have it. I can drop GENMASK_ULL as I only need 
GENMASK for the moment.
Julien Grall April 14, 2016, 3:08 p.m. | #3
Hi Jan,

On 14/04/16 15:56, Jan Beulich wrote:
>>>> Julien Grall <julien.grall@arm.com> 04/14/16 10:55 AM >>>
>> On 14/04/2016 05:01, Jan Beulich wrote:
>>>>>> Julien Grall <julien.grall@arm.com> 04/13/16 6:01 PM >>>
>>>> --- a/xen/include/xen/bitops.h
>>>> +++ b/xen/include/xen/bitops.h
>>>> @@ -3,6 +3,17 @@
>>>    >#include <asm/types.h>
>>>    >
>>>    >/*
>>>> + * Create a contiguous bitmask starting at bit position @l and ending at
>>>> + * position @h. For example
>>>> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>>>> + */
>>>> +#define GENMASK(h, l) \
>>>> +    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>>>> +
>>>> +#define GENMASK_ULL(h, l) \
>>>> +    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>>>
>>> Irrespective of Linux perhaps considering them useful, I'm not sure they
>>> are (and ISTR these macros having got proposed before).
>>
>> This is useful on ARM to generate mask for register. For instance, the
>> following patch introduces mask for the register HPFAR_EL2. Only the
>> bits [4:39] are usable, the rest are RES0.
>>
>> For ARM, RES0 means the bit is currently read as zero but the software
>> should not rely on it to preserve forward compatibility. So we want to
>> mask those bits to avoid breakage with new version of the architecture.
>
>   All understood and needed on every kind of architecture. Yet what's wrong
> with expressing this is as 0xfffffffff0, as is being done most everywhere else?

It is less intuitive to read and it is easier to make a mistake in the mask.

Regards,
Julien Grall April 20, 2016, 12:35 p.m. | #4
Hi Jan,

On 14/04/16 16:23, Jan Beulich wrote:
>>>> Julien Grall <julien.grall@arm.com> 04/14/16 5:08 PM >>>
>> On 14/04/16 15:56, Jan Beulich wrote:
>>>>>> Julien Grall <julien.grall@arm.com> 04/14/16 10:55 AM >>>
>>>> On 14/04/2016 05:01, Jan Beulich wrote:
>>>>>>>> Julien Grall <julien.grall@arm.com> 04/13/16 6:01 PM >>>
>>>>>> --- a/xen/include/xen/bitops.h
>>>>>> +++ b/xen/include/xen/bitops.h
>>>>>> @@ -3,6 +3,17 @@
>>>>>     >#include <asm/types.h>
>>>>>     >
>>>>>     >/*
>>>>>> + * Create a contiguous bitmask starting at bit position @l and ending at
>>>>>> + * position @h. For example
>>>>>> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>>>>>> + */
>>>>>> +#define GENMASK(h, l) \
>>>>>> +    (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>>>>>> +
>>>>>> +#define GENMASK_ULL(h, l) \
>>>>>> +    (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>>>>>
>>>>> Irrespective of Linux perhaps considering them useful, I'm not sure they
>>>>> are (and ISTR these macros having got proposed before).
>>>>
>>>> This is useful on ARM to generate mask for register. For instance, the
>>>> following patch introduces mask for the register HPFAR_EL2. Only the
>>>> bits [4:39] are usable, the rest are RES0.
>>>>
>>>> For ARM, RES0 means the bit is currently read as zero but the software
>>>> should not rely on it to preserve forward compatibility. So we want to
>>>> mask those bits to avoid breakage with new version of the architecture.
>>>
>>>    All understood and needed on every kind of architecture. Yet what's wrong
>>> with expressing this is as 0xfffffffff0, as is being done most everywhere else?
>>
>> It is less intuitive to read and it is easier to make a mistake in the mask.
>
> Well, I guess that depends on the person reading/writing the respective piece
> of code. To me the macroized form would at least require getting used to.

It is a matter of taste. Is there any reason to not allow different way 
to create a mask?

In the case of ARM, the macro version helps to find quickly if a mask 
matches the ARM specification. It is also less error-prone with 64-bit mask.

If you are concerned to have this macro in the common, I would be fine 
to carry it in asm-arm/.

Regards,
Julien Grall April 22, 2016, 11:33 a.m. | #5
Hi Jan,

On 20/04/16 17:43, Jan Beulich wrote:
>>>> Julien Grall <julien.grall@arm.com> 04/20/16 2:35 PM >>>
>> It is a matter of taste.
>
> Indeed.
>
>> Is there any reason to not allow different way to create a mask?
>
> I dislike it, but not so much to stand in the way to get it in. I.e. I'm not going
> to NAK it, but I'm also not currently planning to ACK it.

Stefano, who is now "REST maintainers", acked this patch few days. I 
guess this could be considered as valid ack.

As this series has been release-acked by Wei and acked by Stefano, can a 
committer apply this series to master?

Regards,
Julien Grall April 22, 2016, 3:33 p.m. | #6
Hi Stefano,

On 22/04/16 12:49, Stefano Stabellini wrote:
> On Fri, 22 Apr 2016, Julien Grall wrote:
>> Hi Jan,
>>
>> On 20/04/16 17:43, Jan Beulich wrote:
>>>>>> Julien Grall <julien.grall@arm.com> 04/20/16 2:35 PM >>>
>>>> It is a matter of taste.
>>>
>>> Indeed.
>>>
>>>> Is there any reason to not allow different way to create a mask?
>>>
>>> I dislike it, but not so much to stand in the way to get it in. I.e. I'm not
>>> going
>>> to NAK it, but I'm also not currently planning to ACK it.
>>
>> Stefano, who is now "REST maintainers", acked this patch few days. I guess
>> this could be considered as valid ack.
>>
>> As this series has been release-acked by Wei and acked by Stefano, can a
>> committer apply this series to master?
>
> Please resubmit, dropping or fixing GENMASK_ULL

Oh right, I forgot about this macro. Can I keep your ack here?

Cheers,

Patch

diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index cb56f24..e1a4d93 100644
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -3,6 +3,17 @@ 
 #include <asm/types.h>
 
 /*
+ * Create a contiguous bitmask starting at bit position @l and ending at
+ * position @h. For example
+ * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
+ */
+#define GENMASK(h, l) \
+	(((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
+
+#define GENMASK_ULL(h, l) \
+	(((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
+
+/*
  * ffs: find first bit set. This is defined the same way as
  * the libc and compiler builtin ffs routines, therefore
  * differs in spirit from the above ffz (man ffs).