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

["Oleksii K." <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2024-05-21 at 13:18 +0200, Jan Beulich wrote:
> 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.

Aren't all changes, except those in xen/common/bitops.c, independent? I
could move these changes in xen/common/bitops.c to a separate commit. I
think it is safe to commit them ( an introduction of common logic for
fls{l}() and tests ) separately since the CI tests have passed.

~ Oleksii

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