Re: [PATCH v10 03/14] xen/bitops: implement fls{l}() in common logic

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.05.2024 15:54, Oleksii Kurochko wrote:
> To avoid the compilation error below, it is needed to update to places
> in common/page_alloc.c where flsl() is used as now flsl() returns unsigned 
> int:
> 
> ./include/xen/kernel.h:18:21: error: comparison of distinct pointer types 
> lacks a cast [-Werror]
>        18 |         (void) (&_x == &_y);            \
>           |                     ^~
>     common/page_alloc.c:1843:34: note: in expansion of macro 'min'
>      1843 |         unsigned int inc_order = min(MAX_ORDER, flsl(e - s) - 1);
> 
> generic_fls{l} was used instead of __builtin_clz{l}(x) as if x is 0,
> the result in undefined.
> 
> The prototype of the per-architecture fls{l}() functions was changed to
> return 'unsigned int' to align with the generic implementation of these
> functions and avoid introducing signed/unsigned mismatches.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
>  The patch is almost independent from Andrew's patch series
>  ( 
> https://lore.kernel.org/xen-devel/20240313172716.2325427-1-andrew.cooper3@xxxxxxxxxx/T/#t)
>  except test_fls() function which IMO can be merged as a separate patch after 
> Andrew's patch
>  will be fully ready.

If there wasn't this dependency (I don't think it's "almost independent"),
I'd be offering R-b with again one nit below.

> --- a/xen/arch/x86/include/asm/bitops.h
> +++ b/xen/arch/x86/include/asm/bitops.h
> @@ -425,20 +425,21 @@ static always_inline unsigned int arch_ffsl(unsigned 
> long x)
>   *
>   * This is defined the same way as ffs.
>   */
> -static inline int flsl(unsigned long x)
> +static always_inline unsigned int arch_flsl(unsigned long x)
>  {
> -    long r;
> +    unsigned long r;
>  
>      asm ( "bsr %1,%0\n\t"
>            "jnz 1f\n\t"
>            "mov $-1,%0\n"
>            "1:" : "=r" (r) : "rm" (x));
> -    return (int)r+1;
> +    return (unsigned int)r+1;

Since you now touch this, you'd better tidy it at the same time:

    return r + 1;

(i.e. style and no need for a cast).

Jan