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

[Julien Grall <julien@xxxxxxx>]

Hi,

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@xxxxxxxx>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
>   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?
> -            "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.

Cheers,

[1] 
https://lore.kernel.org/linux-arch/20210225135700.1381396-1-yury.norov@xxxxxxxxx/
--
Julien Grall