diff mbox series

[v2,2/2] xen: Change parameter of generic_{fls,ffs}() to unsigned int

Message ID 20230905073135.12360-3-michal.orzel@amd.com (mailing list archive)
State New, archived
Headers show
Series xen: fixes for UBSAN findings with SMMUv3 enabled | expand

Commit Message

Michal Orzel Sept. 5, 2023, 7:31 a.m. UTC
When running with SMMUv3 and UBSAN enabled on arm64, there are a lot of
warnings printed related to shifting into sign bit in generic_fls()
as it takes parameter of type int.

Example:
(XEN) UBSAN: Undefined behaviour in ./include/xen/bitops.h:69:11
(XEN) left shift of 134217728 by 4 places cannot be represented in type 'int'

It does not make a lot of sense to ask for the last set bit of a negative
value. We don't have a direct user of this helper and all the wrappers
pass value of type unsigned {int,long}.

Linux did the same as part of commit:
3fc2579e6f16 ("fls: change parameter to unsigned int")

To keep consistency between the helpers, take the opportunity to:
 - replace __inline__ with inline,
 - modify generic_ffs() to take parameter of type unsigned int as well
   (currently no user and the only wrapper generic_ffsl() passes unsigned
   long).

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v2:
 - as requested by Jan, s/__inline__/inline/ and modify generic_ffs()
   for consistency

It looks like generic_fls() is only used by Arm and invoked only if
the arguement passed is a compile time constant. This is true for SMMUv3
which makes use of ffs64() in FIELD_{PREP,GET} macros. generic_ffs() has no
user at the moment.

Note for the future:
The return type could be unsigned as well. However, looking at all variations
of the helpers to find first set/clear in the codebase, returning int is the
de-facto standard. Changing the return type of the title helpers would result
in inconsistency or require to do modifications in other places which is not
something I want to do at this stage of release.
---
 xen/include/xen/bitops.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich Sept. 5, 2023, 7:40 a.m. UTC | #1
On 05.09.2023 09:31, Michal Orzel wrote:
> When running with SMMUv3 and UBSAN enabled on arm64, there are a lot of
> warnings printed related to shifting into sign bit in generic_fls()
> as it takes parameter of type int.
> 
> Example:
> (XEN) UBSAN: Undefined behaviour in ./include/xen/bitops.h:69:11
> (XEN) left shift of 134217728 by 4 places cannot be represented in type 'int'
> 
> It does not make a lot of sense to ask for the last set bit of a negative
> value. We don't have a direct user of this helper and all the wrappers
> pass value of type unsigned {int,long}.
> 
> Linux did the same as part of commit:
> 3fc2579e6f16 ("fls: change parameter to unsigned int")
> 
> To keep consistency between the helpers, take the opportunity to:
>  - replace __inline__ with inline,
>  - modify generic_ffs() to take parameter of type unsigned int as well
>    (currently no user and the only wrapper generic_ffsl() passes unsigned
>    long).
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
Henry Wang Sept. 5, 2023, 7:42 a.m. UTC | #2
Hi Michal,

> On Sep 5, 2023, at 15:31, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> When running with SMMUv3 and UBSAN enabled on arm64, there are a lot of
> warnings printed related to shifting into sign bit in generic_fls()
> as it takes parameter of type int.
> 
> Example:
> (XEN) UBSAN: Undefined behaviour in ./include/xen/bitops.h:69:11
> (XEN) left shift of 134217728 by 4 places cannot be represented in type 'int'
> 
> It does not make a lot of sense to ask for the last set bit of a negative
> value. We don't have a direct user of this helper and all the wrappers
> pass value of type unsigned {int,long}.
> 
> Linux did the same as part of commit:
> 3fc2579e6f16 ("fls: change parameter to unsigned int")
> 
> To keep consistency between the helpers, take the opportunity to:
> - replace __inline__ with inline,
> - modify generic_ffs() to take parameter of type unsigned int as well
>   (currently no user and the only wrapper generic_ffsl() passes unsigned
>   long).
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry
diff mbox series

Patch

diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index 654f525fb437..edd6817d5356 100644
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -18,7 +18,7 @@ 
  * differs in spirit from the above ffz (man ffs).
  */
 
-static inline int generic_ffs(int x)
+static inline int generic_ffs(unsigned int x)
 {
     int r = 1;
 
@@ -51,7 +51,7 @@  static inline int generic_ffs(int x)
  * fls: find last bit set.
  */
 
-static __inline__ int generic_fls(int x)
+static inline int generic_fls(unsigned int x)
 {
     int r = 32;