Re: [XEN PATCH][for-4.19 v4 1/8] xen/include: add macro ISOLATE_LOW_BIT

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.10.2023 23:44, Stefano Stabellini wrote:
> On Mon, 30 Oct 2023, Jan Beulich wrote:
>> On 27.10.2023 15:34, Nicola Vetrini wrote:
>>> --- a/xen/include/xen/macros.h
>>> +++ b/xen/include/xen/macros.h
>>> @@ -8,8 +8,14 @@
>>>  #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
>>>  #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>>>  
>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
>>> +/*
>>> + * Given an unsigned integer argument, expands to a mask where just the 
>>> least
>>> + * significant nonzero bit of the argument is set, or 0 if no bits are set.
>>> + */
>>> +#define ISOLATE_LOW_BIT(x) ((x) & -(x))
>>
>> Not even considering future Misra changes (which aiui may require that
>> anyway), this generalization of the macro imo demands that its argument
>> now be evaluated only once.
> 
> Fur sure that would be an improvement, but I don't see a trivial way to
> do it and this issue is also present today before the patch.

This was an issue here for MASK_EXTR() and MASK_INSR(), yes, but the new
macro has wider use, and there was no issue elsewhere so far.

> I think it
> would be better to avoid scope-creeping this patch as we are already at
> v4 for something that was expected to be a trivial mechanical change. I
> would rather review the fix as a separate patch, maybe sent by you as
> you probably have a specific implementation in mind?

#define ISOLATE_LOW_BIT(x) ({ \
    typeof(x) x_ = (x); \
    x_ & -x_; \
})

Hard to see the scope creep here. What I would consider scope creep I
specifically didn't even ask for: I'd like this macro to be overridable
by an arch. Specifically (see my earlier naming hint) I'd like to use
x86's BMI insn BLSI in the context of "x86: allow Kconfig control over
psABI level", when ABI v2 or higher is in use.

Jan