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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/08/2024 11:30 am, Jan Beulich wrote:
> 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.

Perhaps, although it would want to be a project-wide deviation.

Eclair was green with this patch in place, so it's not blocking.

>
>> 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?

I suppose not.  I'll drop it.

However, It occurs to me that I do need a test of 0x8000000000000001ULL
mostly for 32bit builds to check that even when the argument is split,
the answer is still accurate.

~Andrew