Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Julien,

On Thu, 2024-02-29 at 13:54 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 26/02/2024 17:38, Oleksii Kurochko wrote:
> > These functions can be useful for architectures that don't
> > have corresponding arch-specific instructions.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> >   Changes in V5:
> >     - new patch
> > ---
> >   xen/include/asm-generic/bitops/fls.h  | 18 ++++++++++++++++++
> >   xen/include/asm-generic/bitops/flsl.h | 10 ++++++++++
> 
> One header per function seems a little bit excessive to me. Do you
> have 
> any pointer where this request is coming from?
The goal was to be in sync with Linux kernel as Jan mentioned.
I will update the commit message as you suggested in one of replies.

> 
> Why not using the pattern.
> 
> In arch implementation:
> 
> #define fls
> static inline ...
> 
> In the generic header (asm-generic or xen/):
> 
> #ifndef fls
> static inline ...
> #endif
> 
> >   2 files changed, 28 insertions(+)
> >   create mode 100644 xen/include/asm-generic/bitops/fls.h
> >   create mode 100644 xen/include/asm-generic/bitops/flsl.h
> > 
> > diff --git a/xen/include/asm-generic/bitops/fls.h
> > b/xen/include/asm-generic/bitops/fls.h
> > new file mode 100644
> > index 0000000000..369a4c790c
> > --- /dev/null
> > +++ b/xen/include/asm-generic/bitops/fls.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> 
> You should use GPL-2.0-only.
Sure, I'll update the license here and in other files. I automatically
copied this SPDX from Linux kernel.

> 
> > +#ifndef _ASM_GENERIC_BITOPS_FLS_H_
> > +#define _ASM_GENERIC_BITOPS_FLS_H_
> > +
> > +/**
> > + * fls - find last (most-significant) bit set
> > + * @x: the word to search
> > + *
> > + * This is defined the same way as ffs.
> > + * Note fls(0) = 0, fls(1) = 1, fls(0x80000000) = 32.
> > + */
> > +
> > +static inline int fls(unsigned int x)
> > +{
> > +    return generic_fls(x);
> > +}
> > +
> > +#endif /* _ASM_GENERIC_BITOPS_FLS_H_ */
> 
> Missing emacs magic. I am probably not going to repeat this remark
> and 
> the one above again. So please have a look.
Sure, I'll update files with emacs magic.

~ Oleksii
> 
> > diff --git a/xen/include/asm-generic/bitops/flsl.h
> > b/xen/include/asm-generic/bitops/flsl.h
> > new file mode 100644
> > index 0000000000..d0a2e9c729
> > --- /dev/null
> > +++ b/xen/include/asm-generic/bitops/flsl.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_GENERIC_BITOPS_FLSL_H_
> > +#define _ASM_GENERIC_BITOPS_FLSL_H_
> > +
> > +static inline int flsl(unsigned long x)
> > +{
> > +    return generic_flsl(x);
> > +}
> > +
> > +#endif /* _ASM_GENERIC_BITOPS_FLSL_H_ */
> 
> Cheers,
>