Re: [bootwrapper PATCH v2 02/13] Add bit-field macros

Steven Price
2022-01-17T12:15:09Z

On 14/01/2022 10:56, Mark Rutland wrote:
> Arm architectural documentation typically defines bit-fields as
> `[msb,lsb]` and single-bit fields as `[bit]`. For clarity it would be
> helpful if we could define fields in the same way.
>
> Add helpers so that we can do so, along with helper to extract/insert
> bit-field values.
>
> There should be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
> include/bits.h | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 79 insertions(+)
> create mode 100644 include/bits.h
>
> diff --git a/include/bits.h b/include/bits.h
> new file mode 100644
> index 0000000..0bf2c67
> --- /dev/null
> +++ b/include/bits.h
> @@ -0,0 +1,79 @@
> +/*
> + * include/bits.h - helpers for bit-field definitions.
> + *
> + * Copyright (C) 2021 ARM Limited. All rights reserved.
> + *
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE.txt file.
> + */
> +#ifndef __BITS_H
> +#define __BITS_H
> +
> +#ifdef __ASSEMBLY__
> +#define UL(x) x
> +#define ULL(x) x
> +#else
> +#define UL(x) x##UL
> +#define ULL(x) x##ULL
> +#endif
> +
> +/*
> + * Define a contiguous mask of bits with `msb` as the most significant bit and
> + * `lsb` as the least significant bit. The `msb` value must be greater than or
> + * equal to `lsb`.
> + *
> + * For example:
> + * - BITS(63, 63) is 0x8000000000000000
> + * - BITS(63, 0) is 0xFFFFFFFFFFFFFFFF
> + * - BITS(0, 0) is 0x0000000000000001
> + * - BITS(49, 17) is 0x0001FFFFFFFE0000
> + */
> +#define BITS(msb, lsb) \
> + ((~ULL(0) >> (63 - msb)) & (~ULL(0) << lsb))
> +
> +/*
> + * Define a mask of a single set bit `b`.
> + *
> + * For example:
> + * - BIT(63) is 0x8000000000000000
> + * - BIT(0) is 0x0000000000000001
> + * - BIT(32) is 0x0000000100000000
> + */
> +#define BIT(b) BITS(b, b)
> +
> +/*
> + * Find the least significant set bit in the contiguous set of bits in `mask`.
> + *
> + * For example:
> + * - BITS_LSB(0x0000000000000001) is 0
> + * - BITS_LSB(0x000000000000ff00) is 8
> + * - BITS_LSB(0x8000000000000000) is 63
> + */
> +#define BITS_LSB(mask) (__builtin_ffsll(mask) - 1)
> +
> +/*
> + * Extract a bit-field out of `val` described by the contiguous set of bits in
> + * `mask`.
> + *
> + * For example:
> + * - BITS_EXTRACT(0xABCD, BITS(15, 12)) is 0xA
> + * - BITS_EXTRACT(0xABCD, BITS(11, 8)) is 0xB
> + * - BITS_EXTRACT(0xABCD, BIT(7)) is 0x1
> + */
> +#define BITS_EXTRACT(val, mask) \
> + (((val) & (mask)) >> BITS_LSB(mask))
> +
> +/*
> + * Insert the least significant bits of `val` into the bit-field described by
> + * the contiguous set of bits in `mask`.
> + *
> + * For example:
> + * - BITS_INSERT(BITS(3, 0), 0xA) is 0x000A
> + * - BITS_INSERT(BITS(15, 12), 0xA) is 0xA000
> + * - BITS_INSERT(BIT(15), 0xF) is 0x1000

* - BITS_INSERT(BIT(35), 0x1) is 0x0

... which might be surprising!

of course BITS_INSERT(BIT(35), ULL(0x1)) works, but I feel either the
description above needs to signpost this gotcha, or a cast is needed below.

AFAICT this macro isn't actually used in your series.

Steve

> + *
> + */
> +#define BITS_INSERT(mask, val) \
> + (((val) << BITS_LSB(mask)) & (mask))
> +
> +#endif