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

[Julien Grall <julien.grall@arm.com>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel