1. Patchew
  2. Xen
  3. [PATCH v5 00/23] [PATCH v4 00/30] Enable build of full Xen for RISC-V
  4. View patch

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

Oleksii Kurochko posted 23 patches 1 year, 11 months ago
There is a newer version of this series
[PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions
Posted by Oleksii Kurochko 1 year, 11 months ago 
These functions can be useful for architectures that don't
have corresponding arch-specific instructions.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 Changes in V5:
   - new patch
---
 xen/include/asm-generic/bitops/fls.h  | 18 ++++++++++++++++++
 xen/include/asm-generic/bitops/flsl.h | 10 ++++++++++
 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 */
+#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_ */
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_ */
-- 
2.43.0
Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions
Posted by Andrew Cooper 1 year, 11 months ago 
On 26/02/2024 5:38 pm, 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@gmail.com>
> ---
>  Changes in V5:
>    - new patch
> ---
>  xen/include/asm-generic/bitops/fls.h  | 18 ++++++++++++++++++
>  xen/include/asm-generic/bitops/flsl.h | 10 ++++++++++
>  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 */
> +#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_ */
> 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_ */

Please don't do this.  It's compounding existing problems we have with
bitops, and there's a way to simplify things instead.

If you can wait a couple of days, I'll see about finishing and posting
my prototype demonstrating a simplification across all architectures,
and a reduction of code overall.

~Andrew
Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions
Posted by Oleksii 1 year, 11 months ago 
On Thu, 2024-02-29 at 16:25 +0000, Andrew Cooper wrote:
> On 26/02/2024 5:38 pm, 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@gmail.com>
> > ---
> >  Changes in V5:
> >    - new patch
> > ---
> >  xen/include/asm-generic/bitops/fls.h  | 18 ++++++++++++++++++
> >  xen/include/asm-generic/bitops/flsl.h | 10 ++++++++++
> >  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 */
> > +#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_ */
> > 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_ */
> 
> Please don't do this.  It's compounding existing problems we have
> with
> bitops, and there's a way to simplify things instead.
> 
> If you can wait a couple of days, I'll see about finishing and
> posting
> my prototype demonstrating a simplification across all architectures,
> and a reduction of code overall.
Please add me in CC.

~ Oleksii
Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions
Posted by Jan Beulich 1 year, 11 months ago 
On 26.02.2024 18:38, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/include/asm-generic/bitops/fls.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#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);
> +}

This being an inline function, it requires generic_fls() to be declared.
Yet there's no other header included here. I think these headers would
better be self-contained. Or else (e.g. because of this leading to an
#include cycle) something needs saying somewhere.

The other thing here that worries me is the use of plain int as return
type. Yes, generic_fls() is declared like that, too. But no, the return
value there or here cannot be negative.

Jan
Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions
Posted by Julien Grall 1 year, 11 months ago 
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@gmail.com>
> ---
>   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?

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.

> +#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.

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

-- 
Julien Grall
Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions
Posted by Oleksii 1 year, 11 months ago 
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@gmail.com>
> > ---
> >   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,
>
Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions
Posted by Jan Beulich 1 year, 11 months ago 
On 29.02.2024 14:54, Julien Grall wrote:
> 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@gmail.com>
>> ---
>>   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?

That's in an attempt to follow Linux'es way of having this, aiui. This way
an arch can mix and match header inclusions and private definitions without
any #ifdef-ary.

Jan
Re: [PATCH v5 04/23] xen/asm-generic: introduce generic fls() and flsl() functions
Posted by Julien Grall 1 year, 11 months ago 
Hi Jan,

On 29/02/2024 14:03, Jan Beulich wrote:
> On 29.02.2024 14:54, Julien Grall wrote:
>> 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@gmail.com>
>>> ---
>>>    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?
> 
> That's in an attempt to follow Linux'es way of having this, aiui. This way
> an arch can mix and match header inclusions and private definitions without
> any #ifdef-ary.

Ok. I am not going to oppose it if the goal is to keep the headers in 
sync with Linux.

Although, it might have been useful to mention it in the commit message.

Cheers,

-- 
Julien Grall

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.