diff mbox series

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

Message ID df7ab5055ef08fa595f913072302770a3f6a5c33.1708962629.git.oleksii.kurochko@gmail.com (mailing list archive)
State Superseded
Headers show
Series Enable build of full Xen for RISC-V | expand

Commit Message

Oleksii Kurochko Feb. 26, 2024, 5:38 p.m. UTC
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

Comments

Julien Grall Feb. 29, 2024, 1:54 p.m. UTC | #1
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,
Jan Beulich Feb. 29, 2024, 2:03 p.m. UTC | #2
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
Julien Grall Feb. 29, 2024, 2:08 p.m. UTC | #3
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,
Jan Beulich Feb. 29, 2024, 3:52 p.m. UTC | #4
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
Oleksii Kurochko Feb. 29, 2024, 4:17 p.m. UTC | #5
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,
>
Andrew Cooper Feb. 29, 2024, 4:25 p.m. UTC | #6
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
Oleksii Kurochko March 1, 2024, 9:15 a.m. UTC | #7
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
diff mbox series

Patch

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_ */