Re: [PATCH v2 1/7] xen: implement byteswap.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.10.2021 12:47, Lin Liu wrote:
> --- /dev/null
> +++ b/xen/include/xen/byteswap.h
> @@ -0,0 +1,93 @@
> +#ifndef _BYTESWAP_H
> +#define _BYTESWAP_H
> +
> +#include <xen/types.h>
> +
> +#if !__has_builtin(__builtin_bswap16)
> +static always_inline uint16_t __builtin_bswap16(uint16_t val)
> +{
> +    return ((val & 0x00FF) << 8) | ((val & 0xFF00) >> 8);
> +}
> +#endif
> +
> +#if !__has_builtin(__builtin_bswap32)
> +static always_inline uint32_t __builtin_bswap32(uint32_t val)
> +{
> +    return ((val & 0x000000FF) << 24) |
> +           ((val & 0x0000FF00) <<  8) |
> +           ((val & 0x00FF0000) >>  8) |
> +           ((val & 0xFF000000) >> 24);
> +}
> +#endif
> +
> +#if !__has_builtin(__builtin_bswap64)
> +static always_inline uint64_t __builtin_bswap64(uint64_t val)
> +{
> +    return ((val & 0x00000000000000FF) << 56) |
> +           ((val & 0x000000000000FF00) << 40) |
> +           ((val & 0x0000000000FF0000) << 24) |
> +           ((val & 0x00000000FF000000) <<  8) |
> +           ((val & 0x000000FF00000000) >>  8) |
> +           ((val & 0x0000FF0000000000) >> 24) |
> +           ((val & 0x00FF000000000000) >> 40) |
> +           ((val & 0xFF00000000000000) >> 56);
> +}
> +#endif
> +
> +#define bswap16(x) __builtin_bswap16(x)
> +#define bswap32(x) __builtin_bswap32(x)
> +#define bswap64(x) __builtin_bswap64(x)
> +
> +#define bswap_ul(x) bswap##BITS_PER_LONG(x)

I don't see how this is supposed to work - the compiler isn't going to
expand BITS_PER_LONG before the token concatenation. You'll need helper
macros to achieve that. Linux has __PASTE(); we may want to "steal" that
(albeit preferably without any leading underscores).

> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__

While you've worked towards abstracting older vs newer compiler
versions, I'm afraid these constants aren't available in gcc 4.1 yet.
They look to have appeared in 4.6.

> +#  ifndef __LITTLE_ENDIAN
> +#    define __LITTLE_ENDIAN 1234
> +#  endif
> +
> +#  ifndef __LITTLE_ENDIAN_BITFIELD
> +#    define __LITTLE_ENDIAN_BITFIELD
> +#  endif

These are definitions which I don't think belong into a header of this
name. They're imo well placed in byteorder/*.h.

Jan