| Message ID | 1454679743-18133-25-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 05, 2016 at 01:42:17PM +0000, Andrew Cooper wrote: > The type of the pointer to a bitmap is not interesting; it does not affect the > representation of the block of bits being pointed to. > > Make the libxc functions consistent with those in Xen, so they can work just > as well with 'unsigned int *' based bitmaps. I'm not sure I understand this, the bitmap functions in xen/include/bitmap.h seem to take unsigned long *. Not saying that I object to this patch, just this comment looks wrong. Wei.
On 05/02/16 16:12, Wei Liu wrote: > On Fri, Feb 05, 2016 at 01:42:17PM +0000, Andrew Cooper wrote: >> The type of the pointer to a bitmap is not interesting; it does not affect the >> representation of the block of bits being pointed to. >> >> Make the libxc functions consistent with those in Xen, so they can work just >> as well with 'unsigned int *' based bitmaps. > I'm not sure I understand this, the bitmap functions in > xen/include/bitmap.h seem to take unsigned long *. > > Not saying that I object to this patch, just this comment looks wrong. The lower level bit operations <asm/bitops.h> are all in terms of void*, for both x86 and arm ~Andrew
At 13:42 +0000 on 05 Feb (1454679737), Andrew Cooper wrote: > The type of the pointer to a bitmap is not interesting; it does not affect the > representation of the block of bits being pointed to. It does affect the alignment, though. Is this safe on ARM? Tim.
On Mon, 2016-02-08 at 16:23 +0000, Tim Deegan wrote: > At 13:42 +0000 on 05 Feb (1454679737), Andrew Cooper wrote: > > The type of the pointer to a bitmap is not interesting; it does not > > affect the > > representation of the block of bits being pointed to. > > It does affect the alignment, though. Is this safe on ARM? Good point. These constructs in the patch: + const unsigned long *addr = _addr; Would be broken if _addr were not suitably aligned for an unsigned long. That probably rules out this approach unfortunately. Ian.
On 08/02/16 16:36, Ian Campbell wrote: > On Mon, 2016-02-08 at 16:23 +0000, Tim Deegan wrote: >> At 13:42 +0000 on 05 Feb (1454679737), Andrew Cooper wrote: >>> The type of the pointer to a bitmap is not interesting; it does not >>> affect the >>> representation of the block of bits being pointed to. >> It does affect the alignment, though. Is this safe on ARM? > Good point. These constructs in the patch: > > + const unsigned long *addr = _addr; > > Would be broken if _addr were not suitably aligned for an unsigned long. > > That probably rules out this approach unfortunately. What about reworking libxc bitops in terms of unsigned char? That should cover all alignment issues. ~Andrew
On Wed, 2016-02-10 at 10:07 +0000, Andrew Cooper wrote: > On 08/02/16 16:36, Ian Campbell wrote: > > On Mon, 2016-02-08 at 16:23 +0000, Tim Deegan wrote: > > > At 13:42 +0000 on 05 Feb (1454679737), Andrew Cooper wrote: > > > > The type of the pointer to a bitmap is not interesting; it does not > > > > affect the > > > > representation of the block of bits being pointed to. > > > It does affect the alignment, though. Is this safe on ARM? > > Good point. These constructs in the patch: > > > > + const unsigned long *addr = _addr; > > > > Would be broken if _addr were not suitably aligned for an unsigned > > long. > > > > That probably rules out this approach unfortunately. > > What about reworking libxc bitops in terms of unsigned char? That > should cover all alignment issues. Assuming any asm or calls to __builtin_foo backends were adjusted to suite, that would be ok, would that be compatible with the Xen side though? Or you could make things a bit cleverer to do the const unsigned long *addr = _addr; in a way which also forces the alignment and adjusts nr to compensate (I don't see that 4 bytes of align is going to make addr point to e.g. a different sort of memory to _addr). Ian.
On Wed, Feb 10, 2016 at 10:07:12AM +0000, Andrew Cooper wrote: > On 08/02/16 16:36, Ian Campbell wrote: > > On Mon, 2016-02-08 at 16:23 +0000, Tim Deegan wrote: > >> At 13:42 +0000 on 05 Feb (1454679737), Andrew Cooper wrote: > >>> The type of the pointer to a bitmap is not interesting; it does not > >>> affect the > >>> representation of the block of bits being pointed to. > >> It does affect the alignment, though. Is this safe on ARM? > > Good point. These constructs in the patch: > > > > + const unsigned long *addr = _addr; > > > > Would be broken if _addr were not suitably aligned for an unsigned long. > > > > That probably rules out this approach unfortunately. > > What about reworking libxc bitops in terms of unsigned char? That > should cover all alignment issues. See 3cab67ac83b1d56c3daedd9c4adfed497a114246 "+/* + * xc_bitops.h has macros that do this as well - however they assume that + * the bitmask is word aligned but xc_cpumap_t is only guaranteed to be + * byte aligned and so we need byte versions for architectures which do + * not support misaligned accesses (which is basically everyone + * but x86, although even on x86 it can be inefficient). + */ " > > ~Andrew > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On 10/02/16 10:18, Ian Campbell wrote: > On Wed, 2016-02-10 at 10:07 +0000, Andrew Cooper wrote: >> On 08/02/16 16:36, Ian Campbell wrote: >>> On Mon, 2016-02-08 at 16:23 +0000, Tim Deegan wrote: >>>> At 13:42 +0000 on 05 Feb (1454679737), Andrew Cooper wrote: >>>>> The type of the pointer to a bitmap is not interesting; it does not >>>>> affect the >>>>> representation of the block of bits being pointed to. >>>> It does affect the alignment, though. Is this safe on ARM? >>> Good point. These constructs in the patch: >>> >>> + const unsigned long *addr = _addr; >>> >>> Would be broken if _addr were not suitably aligned for an unsigned >>> long. >>> >>> That probably rules out this approach unfortunately. >> What about reworking libxc bitops in terms of unsigned char? That >> should cover all alignment issues. > Assuming any asm or calls to __builtin_foo backends were adjusted to suite, > that would be ok, would that be compatible with the Xen side though? It is all plain C. What I mean is -static inline int test_bit(int nr, unsigned long *addr) +static inline int test_bit(int nr, const void *_addr) { + const char *addr = _addr; return (BITMAP_ENTRY(nr, addr) >> BITMAP_SHIFT(nr)) & 1; } and changing BITMAP_{ENTRY,SHIFT}() to use char rather than unsigned long. The prototypes still have void *, but char* is used internally which will match the minimum alignment of any object passed. ~Andrew
diff --git a/tools/libxc/xc_bitops.h b/tools/libxc/xc_bitops.h index cd749f4..2a1710f 100644 --- a/tools/libxc/xc_bitops.h +++ b/tools/libxc/xc_bitops.h @@ -26,48 +26,53 @@ static inline unsigned long *bitmap_alloc(int nr_bits) return calloc(1, bitmap_size(nr_bits)); } -static inline void bitmap_set(unsigned long *addr, int nr_bits) +static inline void bitmap_set(void *addr, int nr_bits) { memset(addr, 0xff, bitmap_size(nr_bits)); } -static inline void bitmap_clear(unsigned long *addr, int nr_bits) +static inline void bitmap_clear(void *addr, int nr_bits) { memset(addr, 0, bitmap_size(nr_bits)); } -static inline int test_bit(int nr, unsigned long *addr) +static inline int test_bit(int nr, const void *_addr) { + const unsigned long *addr = _addr; return (BITMAP_ENTRY(nr, addr) >> BITMAP_SHIFT(nr)) & 1; } -static inline void clear_bit(int nr, unsigned long *addr) +static inline void clear_bit(int nr, void *_addr) { + unsigned long *addr = _addr; BITMAP_ENTRY(nr, addr) &= ~(1UL << BITMAP_SHIFT(nr)); } -static inline void set_bit(int nr, unsigned long *addr) +static inline void set_bit(int nr, void *_addr) { + unsigned long *addr = _addr; BITMAP_ENTRY(nr, addr) |= (1UL << BITMAP_SHIFT(nr)); } -static inline int test_and_clear_bit(int nr, unsigned long *addr) +static inline int test_and_clear_bit(int nr, void *addr) { int oldbit = test_bit(nr, addr); clear_bit(nr, addr); return oldbit; } -static inline int test_and_set_bit(int nr, unsigned long *addr) +static inline int test_and_set_bit(int nr, void *addr) { int oldbit = test_bit(nr, addr); set_bit(nr, addr); return oldbit; } -static inline void bitmap_or(unsigned long *dst, const unsigned long *other, +static inline void bitmap_or(void *_dst, const void *_other, int nr_bits) { + unsigned long *dst = _dst; + const unsigned long *other = _other; int i, nr_longs = (bitmap_size(nr_bits) / sizeof(unsigned long)); for ( i = 0; i < nr_longs; ++i ) dst[i] |= other[i];
The type of the pointer to a bitmap is not interesting; it does not affect the representation of the block of bits being pointed to. Make the libxc functions consistent with those in Xen, so they can work just as well with 'unsigned int *' based bitmaps. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Ian Campbell <Ian.Campbell@citrix.com> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> CC: Wei Liu <wei.liu2@citrix.com> New in v2 --- tools/libxc/xc_bitops.h | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)