Re: [PATCH] xen/lib: introduce generic find next bit operations

[Oleksii <oleksii.kurochko@gmail.com>]

On Fri, 2024-01-26 at 13:20 +0000, Julien Grall wrote:
> Hi,
Hi Julien,

> 
> On 26/01/2024 12:20, Oleksii Kurochko wrote:
> > find-next-bit.c is common for Arm64, PPC and RISCV64,
> > so it is moved to xen/lib.
> > 
> > Suggested-by: Jan Beulich <jbeulich@suse.com>
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >   docs/misra/exclude-list.json                  |   4 -
> >   xen/arch/arm/arm64/lib/Makefile               |   2 +-
> >   xen/arch/arm/include/asm/arm64/bitops.h       |  48 --------
> >   xen/arch/ppc/include/asm/bitops.h             | 115 -------------
> > -----
> >   xen/include/xen/bitops.h                      |  48 ++++++++
> >   xen/lib/Makefile                              |   1 +
> >   .../find_next_bit.c => lib/find-next-bit.c}   |   0
> >   7 files changed, 50 insertions(+), 168 deletions(-)
> >   rename xen/{arch/arm/arm64/lib/find_next_bit.c => lib/find-next-
> > bit.c} (100%)
> > 
> > diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-
> > list.json
> > index 7971d0e70f..7fe02b059d 100644
> > --- a/docs/misra/exclude-list.json
> > +++ b/docs/misra/exclude-list.json
> > @@ -13,10 +13,6 @@
> >               "rel_path": "arch/arm/arm64/insn.c",
> >               "comment": "Imported on Linux, ignore for now"
> >           },
> > -        {
> > -            "rel_path": "arch/arm/arm64/lib/find_next_bit.c",
> 
> Rather than removing the section, I was expecting the rel_path to be 
> updated. Can you explain why you think the exclusion is not
> necessary?
I considered simply updating the path to xen/lib/find-next-bit.c, but
ultimately opted to remove it. This decision was based on the fact that
the line in question checks for a file that no longer exists. If it's
preferable to update the rel_path with xen/lib/find-next-bit.c, I'm
more than willing to make that adjustment.

> 
> > -            "comment": "Imported from Linux, ignore for now"
> > -        },
> >           {
> >               "rel_path": "arch/x86/acpi/boot.c",
> >               "comment": "Imported from Linux, ignore for now"
> > diff --git a/xen/arch/arm/arm64/lib/Makefile
> > b/xen/arch/arm/arm64/lib/Makefile
> > index 1b9c7a95e6..66cfac435a 100644
> > --- a/xen/arch/arm/arm64/lib/Makefile
> > +++ b/xen/arch/arm/arm64/lib/Makefile
> > @@ -1,4 +1,4 @@
> >   obj-y += memcpy.o memcmp.o memmove.o memset.o memchr.o
> >   obj-y += clear_page.o
> > -obj-y += bitops.o find_next_bit.o
> > +obj-y += bitops.o
> >   obj-y += strchr.o strcmp.o strlen.o strncmp.o strnlen.o strrchr.o
> > diff --git a/xen/arch/arm/include/asm/arm64/bitops.h
> > b/xen/arch/arm/include/asm/arm64/bitops.h
> > index d85a49bca4..f9dd066237 100644
> > --- a/xen/arch/arm/include/asm/arm64/bitops.h
> > +++ b/xen/arch/arm/include/asm/arm64/bitops.h
> > @@ -36,57 +36,9 @@ static inline int flsl(unsigned long x)
> >   
> >   /* Based on linux/include/asm-generic/bitops/find.h */
> >   
> > -#ifndef find_next_bit
> > -/**
> > - * find_next_bit - find the next set bit in a memory region
> > - * @addr: The address to base the search on
> > - * @offset: The bitnumber to start searching at
> > - * @size: The bitmap size in bits
> > - */
> > -extern unsigned long find_next_bit(const unsigned long *addr,
> > unsigned long
> > -		size, unsigned long offset);
> > -#endif
> > -
> > -#ifndef find_next_zero_bit
> > -/**
> > - * find_next_zero_bit - find the next cleared bit in a memory
> > region
> > - * @addr: The address to base the search on
> > - * @offset: The bitnumber to start searching at
> > - * @size: The bitmap size in bits
> > - */
> > -extern unsigned long find_next_zero_bit(const unsigned long *addr,
> > unsigned
> > -		long size, unsigned long offset);
> > -#endif
> > -
> > -#ifdef CONFIG_GENERIC_FIND_FIRST_BIT
> > -
> > -/**
> > - * find_first_bit - find the first set bit in a memory region
> > - * @addr: The address to start the search at
> > - * @size: The maximum size to search
> > - *
> > - * Returns the bit number of the first set bit.
> > - */
> > -extern unsigned long find_first_bit(const unsigned long *addr,
> > -				    unsigned long size);
> > -
> > -/**
> > - * find_first_zero_bit - find the first cleared bit in a memory
> > region
> > - * @addr: The address to start the search at
> > - * @size: The maximum size to search
> > - *
> > - * Returns the bit number of the first cleared bit.
> > - */
> > -extern unsigned long find_first_zero_bit(const unsigned long
> > *addr,
> > -					 unsigned long size);
> > -#else /* CONFIG_GENERIC_FIND_FIRST_BIT */
> > -
> >   #define find_first_bit(addr, size) find_next_bit((addr), (size),
> > 0)
> >   #define find_first_zero_bit(addr, size)
> > find_next_zero_bit((addr), (size), 0)
> >   
> > -#endif /* CONFIG_GENERIC_FIND_FIRST_BIT */
> 
> AFAICT, you are changing the behavior for Arm64 without explaining
> why. 
> Before, it was possible to set CONFIG_GENERIC_FIND_FIRST_BIT so the 
> generic version of find_first_*_bit are used. This is not possible 
> anymore with your change.
> 
> Looking at Linux, I see that arm64 is now selecting 
> GENERIC_FIND_FIRST_BIT (see [1]). So I would argue, we should not
> define 
> find_first_bit(). That said, that's probably a separate patch.
> 
> For now, you want to explain why GENERIC_FIND_FIRST_BIT is dropped.
I chose to remove it because I couldn't find any usage or configuration
setting for this in Xen (Arm).

I can add "#ifdef GENERIC_FIND_FIRST_BIT" around find_first_zero_bit()
and find_first_bit() in xen/bitops.h, and according to the link [1], it
should be wrapped with ifdef. Perhaps it would be better to use "#if
defined(GENERIC_FIND_FIRST_BIT) && defined(CONFIG_ARM_64)".

My only concern is that it might seem somewhat inconsistent with the
other find_*_bit() functions added in this patch. Should we be care
about that? I mean that do we need similar config or it would be enough
to add a comment why it is necessary to have ifdef
GENERIC_FIND_FIRST_BIT.

~ Oleksii
> 
> [1] 
> https://lore.kernel.org/linux-arch/20210225135700.1381396-1-yury.norov@gmail.com/
>