| Message ID | 52730e6314210ba4164a9934a720c4fda201447b.1706266854.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/lib: introduce generic find next bit operations | expand |
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@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? > - "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@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/ >
Hi Oleksii, On 26/01/2024 15:58, Oleksii wrote: > 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. That's not quite correct. The file still exists but with a different name. > 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. The problem is by removing the file, you effectively tell Eclair to report any MISRA error. I don't believe this is what we want here. But if the other agrees with you, then this change ought to be explained in the commit message. To me the key is the commit message should contain enough information for the reviewer to understand what/why you are doing. All the changes I pointed out are definitely not just a normal code movement. > >> >>> - "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). Right. My point is the commit message can lead to think this is a simple code movement and there are no change of behavior. It wasn't clear to me whether this was done on purpose or not. It looks like it was and therefore should really be explained in the commit message. > > 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. Note that I didn't ask to keep GENERIC_FIND_FIRST_BIT. I actually pointed out that it can be removed but it should be explained in the commit message. Cheers,
On 26.01.2024 17:08, Julien Grall wrote: > On 26/01/2024 15:58, Oleksii wrote: >> On Fri, 2024-01-26 at 13:20 +0000, Julien Grall wrote: >>> 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. > > That's not quite correct. The file still exists but with a different name. > >> 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. > > The problem is by removing the file, you effectively tell Eclair to > report any MISRA error. I don't believe this is what we want here. > > But if the other agrees with you, then this change ought to be explained > in the commit message. Imo it can only be removed (rather than updated) if the reason for adding has meanwhile gone away. I don't think that's the case, though. > To me the key is the commit message should contain enough information > for the reviewer to understand what/why you are doing. All the changes I > pointed out are definitely not just a normal code movement. >> >>> >>>> - "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). > > Right. My point is the commit message can lead to think this is a simple > code movement and there are no change of behavior. > > It wasn't clear to me whether this was done on purpose or not. It looks > like it was and therefore should really be explained in the commit message. > >> >> 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. > > Note that I didn't ask to keep GENERIC_FIND_FIRST_BIT. I actually > pointed out that it can be removed but it should be explained in the > commit message. +1 And, Oleksii, this isn't the first time that it is being pointed out to you that commit messages need to contain enough information to justify changes made. Jan
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. > > That's not quite correct. The file still exists but with a different > name. > > > 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. > > The problem is by removing the file, you effectively tell Eclair to > report any MISRA error. I don't believe this is what we want here. I don't know how it should work, but when I ran CI's cppcheck and Eclair jobs nothing were broken: https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/1153360853 > > But if the other agrees with you, then this change ought to be > explained > in the commit message. > > To me the key is the commit message should contain enough information > for the reviewer to understand what/why you are doing. All the > changes I > pointed out are definitely not just a normal code movement. I understand your point, I'll do my best next time. > > > > > > > > > - "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). > > Right. My point is the commit message can lead to think this is a > simple > code movement and there are no change of behavior. > > It wasn't clear to me whether this was done on purpose or not. It > looks > like it was and therefore should really be explained in the commit > message. Next time I'll add the explanation to the commit message to make it more clear. > > > > > 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. > > Note that I didn't ask to keep GENERIC_FIND_FIRST_BIT. I actually > pointed out that it can be removed but it should be explained in the > commit message. GENERIC_FIND_FIRST_BIT config was removed in Linux kernel too as all architectures were switched to use find_{first,last}_bit() unconditionally: https://lore.kernel.org/linux-arch/20211005054059.475634-5-yury.norov@gmail.com/ This is not the case of Xen, as some, at least, x86 has arch specific implementation of find_{first,last}_bit(), but it can be another one justification why this config can be removed in Xen. ~ Oleksii
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", - "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 */ - - #endif /* _ARM_ARM64_BITOPS_H */ /* * Local variables: diff --git a/xen/arch/ppc/include/asm/bitops.h b/xen/arch/ppc/include/asm/bitops.h index 5e7f36c21d..5820b9ce7b 100644 --- a/xen/arch/ppc/include/asm/bitops.h +++ b/xen/arch/ppc/include/asm/bitops.h @@ -217,119 +217,4 @@ static always_inline unsigned long __ffs(unsigned long word) */ #define find_first_set_bit(x) (ffsl(x) - 1) -/* - * Find the first set bit in a memory region. - */ -static inline unsigned long find_first_bit(const unsigned long *addr, - unsigned long size) -{ - const unsigned long *p = addr; - unsigned long result = 0; - unsigned long tmp; - - while ( size & ~(BITS_PER_LONG - 1) ) - { - if ( (tmp = *(p++)) ) - goto found; - result += BITS_PER_LONG; - size -= BITS_PER_LONG; - } - if ( !size ) - return result; - - tmp = (*p) & (~0UL >> (BITS_PER_LONG - size)); - if ( tmp == 0UL ) /* Are any bits set? */ - return result + size; /* Nope. */ - found: - return result + __ffs(tmp); -} - -static inline unsigned long find_next_bit(const unsigned long *addr, - unsigned long size, - unsigned long offset) -{ - const unsigned long *p = addr + BITOP_WORD(offset); - unsigned long result = offset & ~(BITS_PER_LONG - 1); - unsigned long tmp; - - if ( offset >= size ) - return size; - size -= result; - offset %= BITS_PER_LONG; - if ( offset ) - { - tmp = *(p++); - tmp &= (~0UL << offset); - if ( size < BITS_PER_LONG ) - goto found_first; - if ( tmp ) - goto found_middle; - size -= BITS_PER_LONG; - result += BITS_PER_LONG; - } - while ( size & ~(BITS_PER_LONG - 1) ) - { - if ( (tmp = *(p++)) ) - goto found_middle; - result += BITS_PER_LONG; - size -= BITS_PER_LONG; - } - if ( !size ) - return result; - tmp = *p; - - found_first: - tmp &= (~0UL >> (BITS_PER_LONG - size)); - if ( tmp == 0UL ) /* Are any bits set? */ - return result + size; /* Nope. */ - found_middle: - return result + __ffs(tmp); -} - -/* - * This implementation of find_{first,next}_zero_bit was stolen from - * Linus' asm-alpha/bitops.h. - */ -static inline unsigned long find_next_zero_bit(const unsigned long *addr, - unsigned long size, - unsigned long offset) -{ - const unsigned long *p = addr + BITOP_WORD(offset); - unsigned long result = offset & ~(BITS_PER_LONG - 1); - unsigned long tmp; - - if ( offset >= size ) - return size; - size -= result; - offset %= BITS_PER_LONG; - if ( offset ) - { - tmp = *(p++); - tmp |= ~0UL >> (BITS_PER_LONG - offset); - if ( size < BITS_PER_LONG ) - goto found_first; - if ( ~tmp ) - goto found_middle; - size -= BITS_PER_LONG; - result += BITS_PER_LONG; - } - while ( size & ~(BITS_PER_LONG - 1) ) - { - if ( ~(tmp = *(p++)) ) - goto found_middle; - result += BITS_PER_LONG; - size -= BITS_PER_LONG; - } - if ( !size ) - return result; - tmp = *p; - - found_first: - tmp |= ~0UL << size; - if ( tmp == ~0UL ) /* Are any bits zero? */ - return result + size; /* Nope. */ - found_middle: - return result + ffz(tmp); -} - #endif /* _ASM_PPC_BITOPS_H */ diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h index a88d45475c..bddd75a473 100644 --- a/xen/include/xen/bitops.h +++ b/xen/include/xen/bitops.h @@ -105,6 +105,54 @@ static inline int generic_flsl(unsigned long x) */ #include <asm/bitops.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 + +#ifndef 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); +#endif + +#ifndef find_first_zero_bit +/** + * 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); +#endif + #if BITS_PER_LONG == 64 # define fls64 flsl # define ffs64 ffsl diff --git a/xen/lib/Makefile b/xen/lib/Makefile index 2d9ebb945f..e63798e1d4 100644 --- a/xen/lib/Makefile +++ b/xen/lib/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_X86) += x86/ lib-y += bsearch.o lib-y += ctors.o lib-y += ctype.o +lib-y += find-next-bit.o lib-y += list-sort.o lib-y += memchr.o lib-y += memchr_inv.o diff --git a/xen/arch/arm/arm64/lib/find_next_bit.c b/xen/lib/find-next-bit.c similarity index 100% rename from xen/arch/arm/arm64/lib/find_next_bit.c rename to xen/lib/find-next-bit.c
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%)