| Message ID | 1460562931-19858-2-git-send-email-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
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,
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.
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,
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,
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,
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,
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).
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(+)