| Message ID | 1491434362-30310-2-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 6 Apr 2017, Andre Przywara wrote: > To safely handle 64-bit registers even on 32-bit systems, introduce > a GENMASK_ULL variant (lifted from Linux). > This adds a BITS_PER_LONG_LONG define as well. > Also fix a bug in the comment for the existing GENMASK variant. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/include/asm-arm/config.h | 2 ++ > xen/include/xen/bitops.h | 5 ++++- > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h > index b2edf95..1f730ce 100644 > --- a/xen/include/asm-arm/config.h > +++ b/xen/include/asm-arm/config.h > @@ -19,6 +19,8 @@ > #define BITS_PER_LONG (BYTES_PER_LONG << 3) > #define POINTER_ALIGN BYTES_PER_LONG > > +#define BITS_PER_LONG_LONG (sizeof (long long) * BITS_PER_BYTE) > + > /* xen_ulong_t is always 64 bits */ > #define BITS_PER_XEN_ULONG 64 > > diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h > index bd0883a..9261e06 100644 > --- a/xen/include/xen/bitops.h > +++ b/xen/include/xen/bitops.h > @@ -5,11 +5,14 @@ > /* > * Create a contiguous bitmask starting at bit position @l and ending at > * position @h. For example > - * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000. > + * GENMASK(30, 21) gives us the 32bit vector 0x7fe00000. > */ > #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 > -- > 2.8.2 >
(CC 'The REST' maintainers) Hi, Andre, as I mentioned already a couple of times. You should CC all the appropriate maintainers for the code you are modifying. On 06/04/17 00:25, Stefano Stabellini wrote: > On Thu, 6 Apr 2017, Andre Przywara wrote: >> To safely handle 64-bit registers even on 32-bit systems, introduce >> a GENMASK_ULL variant (lifted from Linux). >> This adds a BITS_PER_LONG_LONG define as well. >> Also fix a bug in the comment for the existing GENMASK variant. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> I'd like some input from Andrew here. I suggested a similar patch a year ago (see [1]) and the final decision was to drop GENMASK_ULL. > >> --- >> xen/include/asm-arm/config.h | 2 ++ >> xen/include/xen/bitops.h | 5 ++++- >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h >> index b2edf95..1f730ce 100644 >> --- a/xen/include/asm-arm/config.h >> +++ b/xen/include/asm-arm/config.h >> @@ -19,6 +19,8 @@ >> #define BITS_PER_LONG (BYTES_PER_LONG << 3) >> #define POINTER_ALIGN BYTES_PER_LONG >> >> +#define BITS_PER_LONG_LONG (sizeof (long long) * BITS_PER_BYTE) >> + >> /* xen_ulong_t is always 64 bits */ >> #define BITS_PER_XEN_ULONG 64 >> >> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h >> index bd0883a..9261e06 100644 >> --- a/xen/include/xen/bitops.h >> +++ b/xen/include/xen/bitops.h >> @@ -5,11 +5,14 @@ >> /* >> * Create a contiguous bitmask starting at bit position @l and ending at >> * position @h. For example >> - * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000. >> + * GENMASK(30, 21) gives us the 32bit vector 0x7fe00000. >> */ >> #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 >> -- >> 2.8.2 >> Cheers, [1] https://patchwork.kernel.org/patch/8824091/
>>> On 06.04.17 at 10:45, <julien.grall@arm.com> wrote: > (CC 'The REST' maintainers) > > Hi, > > Andre, as I mentioned already a couple of times. You should CC all the > appropriate maintainers for the code you are modifying. > > On 06/04/17 00:25, Stefano Stabellini wrote: >> On Thu, 6 Apr 2017, Andre Przywara wrote: >>> To safely handle 64-bit registers even on 32-bit systems, introduce >>> a GENMASK_ULL variant (lifted from Linux). >>> This adds a BITS_PER_LONG_LONG define as well. >>> Also fix a bug in the comment for the existing GENMASK variant. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > I'd like some input from Andrew here. I suggested a similar patch a year > ago (see [1]) and the final decision was to drop GENMASK_ULL. Well, to be honest, rather than asking Andrew (who would likely just re-state his opinion, as I would re-state mine), it should be Andre to make clear why things are different now than they were when the introduction of the macro was first rejected. Jan
Hi, On 06/04/17 10:15, Jan Beulich wrote: >>>> On 06.04.17 at 10:45, <julien.grall@arm.com> wrote: >> (CC 'The REST' maintainers) >> >> Hi, >> >> Andre, as I mentioned already a couple of times. You should CC all the >> appropriate maintainers for the code you are modifying. >> >> On 06/04/17 00:25, Stefano Stabellini wrote: >>> On Thu, 6 Apr 2017, Andre Przywara wrote: >>>> To safely handle 64-bit registers even on 32-bit systems, introduce >>>> a GENMASK_ULL variant (lifted from Linux). >>>> This adds a BITS_PER_LONG_LONG define as well. >>>> Also fix a bug in the comment for the existing GENMASK variant. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >> >> I'd like some input from Andrew here. I suggested a similar patch a year >> ago (see [1]) and the final decision was to drop GENMASK_ULL. > > Well, to be honest, rather than asking Andrew (who would likely > just re-state his opinion, as I would re-state mine), it should be > Andre to make clear why things are different now than they > were when the introduction of the macro was first rejected. So first for the case of GENMASK in general: ARM64 system registers or ARM IP registers (like in the GIC interrupt controller) are often 64-bit, and often have several fields encoded at bit locations *not* nibble aligned, for instance here: GITS_BASER[0-7]: Valid: bit[63], Indirect: bit[62], InnerCache: bits[61:59], Type: bits[58:56], OuterCache: bits[55:53], ... (from section 8.19.1 of ARM IHI 0069C [1]) So generating bitmasks for those fields is both tedious and error-prone, also hard to read because of the required 16 nibbles, compare: 0x3800000000000000 vs. GENMASK(61, 59). I think this might be an ARM specialty, which would explain why nobody else missed it before. For this special GENMASK_ULL version: The GIC interrupt controller is usable from 32-bit ARM (software) as well, so the ~0UL in the normal GENMASK is not sufficient to cover 64 bits here. Now although this particular series is for ARM64 only, Julien wanted to prepare for the case we ever need to port this to ARM32 as well, see: https://lists.xen.org/archives/html/xen-devel/2016-11/msg00080.html So long story short: Technically we don't need GENMASK_ULL (and BIT_ULL) at the moment, but Julien asked for it. So personally I am happy to drop them, if Julien agrees. On the other hand this is just a tiny patch, also lifted from Linux, so I don't see any real danger in including it into Xen (which would also save me to fix up all the _ULL versions in my patches). Cheers, Andre.
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h index b2edf95..1f730ce 100644 --- a/xen/include/asm-arm/config.h +++ b/xen/include/asm-arm/config.h @@ -19,6 +19,8 @@ #define BITS_PER_LONG (BYTES_PER_LONG << 3) #define POINTER_ALIGN BYTES_PER_LONG +#define BITS_PER_LONG_LONG (sizeof (long long) * BITS_PER_BYTE) + /* xen_ulong_t is always 64 bits */ #define BITS_PER_XEN_ULONG 64 diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h index bd0883a..9261e06 100644 --- a/xen/include/xen/bitops.h +++ b/xen/include/xen/bitops.h @@ -5,11 +5,14 @@ /* * Create a contiguous bitmask starting at bit position @l and ending at * position @h. For example - * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000. + * GENMASK(30, 21) gives us the 32bit vector 0x7fe00000. */ #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
To safely handle 64-bit registers even on 32-bit systems, introduce a GENMASK_ULL variant (lifted from Linux). This adds a BITS_PER_LONG_LONG define as well. Also fix a bug in the comment for the existing GENMASK variant. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/include/asm-arm/config.h | 2 ++ xen/include/xen/bitops.h | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-)