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