Re: [PATCH 2/9] xen/bitops: Introduce a multiple_bits_set() helper

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.08.2024 01:06, Andrew Cooper wrote:
> This will be used to simplify real logic in the following patch.  Add compile
> and boot time testing as with other bitops.
> 
> Because the expression is so simple, implement it as a function-like macro
> which is generic on the type of it's argument, rather than having multiple
> variants.
> 
> Testing function-like macros needs a minor adjustments to the infrastructure
> in xen/self-tests.h to avoid bracketing the fn parameter.  The utility of this
> outweighs the associated risks.

We may need to mark these two places as deviated.

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one remark/request:

> --- a/xen/common/bitops.c
> +++ b/xen/common/bitops.c
> @@ -84,8 +84,32 @@ static void __init test_fls(void)
>      CHECK(fls64, 0x8000000000000001ULL, 64);
>  }
>  
> +static void __init test_multiple_bits_set(void)
> +{
> +    /*
> +     * multiple_bits_set() is generic on the type of it's parameter, as the
> +     * internal expression is so simple.
> +     */
> +
> +    CHECK(multiple_bits_set, 0, false);
> +    CHECK(multiple_bits_set, 1, false);
> +    CHECK(multiple_bits_set, 2, false);
> +    CHECK(multiple_bits_set, 3, true);
> +
> +    CHECK(multiple_bits_set, 1 | (1UL << (BITS_PER_LONG - 1)), true);

This is really the same as ...

> +#if BITS_PER_LONG > 32
> +    CHECK(multiple_bits_set, 1 | (1UL << 32), true);
> +    CHECK(multiple_bits_set, 1 | (1UL << 63), true);

... this, at least as long as BITS_PER_LONG > 32 in practice means
BITS_PER_LONG == 64. Perhaps not really worth keeping that line?

Jan