All callers of the bitmap_switch macro (which are all within bitmap.h)
pass an int as first parameter. Do not assign it to an unsigned int
potentially introducing errors.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---

We could also have gone the other way and change all the callers and the
callers' callers to use an unsigned int instead, but I went for the path
of least resistance.

diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
index 3caf92c76d..7caec098d7 100644
--- a/xen/include/xen/bitmap.h
+++ b/xen/include/xen/bitmap.h
@@ -101,7 +101,7 @@ extern int bitmap_allocate_region(unsigned long *bitmap, int pos, int order);
 #define bitmap_bytes(nbits) (BITS_TO_LONGS(nbits) * sizeof(unsigned long))
 
 #define bitmap_switch(nbits, zero, small, large)			  \
-	unsigned int n__ = (nbits);					  \
+	int n__ = (nbits);					  \
 	if (__builtin_constant_p(nbits) && !n__) {			  \
 		zero;							  \
 	} else if (__builtin_constant_p(nbits) && n__ <= BITS_PER_LONG) { \

Hi Stefano,

On 29/09/2023 00:19, Stefano Stabellini wrote:
> All callers of the bitmap_switch macro (which are all within bitmap.h)
> pass an int as first parameter. Do not assign it to an unsigned int
> potentially introducing errors.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
> 
> We could also have gone the other way and change all the callers and the
> callers' callers to use an unsigned int instead, but I went for the path
> of least resistance.

I understand this will solve the issue right now because the callers are 
all passing 'int'. However, all the callers will need to switch to 
'unsigned int' in order to solve violations in their callers.

That unless we decide to use 'int' everywhere, but I think this is a bad 
idea because 'n__' is not supposed to be negative.

Overall, this may be an easy win right now, but this will need to be 
reverted. So, I am not happy to ack it and would in fact be leaning 
towards Nacking it.

Cheers,

-- 
Julien Grall

On Fri, 29 Sep 2023, Julien Grall wrote:
> On 29/09/2023 00:19, Stefano Stabellini wrote:
> > All callers of the bitmap_switch macro (which are all within bitmap.h)
> > pass an int as first parameter. Do not assign it to an unsigned int
> > potentially introducing errors.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > ---
> > 
> > We could also have gone the other way and change all the callers and the
> > callers' callers to use an unsigned int instead, but I went for the path
> > of least resistance.
> 
> I understand this will solve the issue right now because the callers are all
> passing 'int'. However, all the callers will need to switch to 'unsigned int'
> in order to solve violations in their callers.
> 
> That unless we decide to use 'int' everywhere, but I think this is a bad idea
> because 'n__' is not supposed to be negative.
> 
> Overall, this may be an easy win right now, but this will need to be reverted.
> So, I am not happy to ack it and would in fact be leaning towards Nacking it.

I understand this point and I was undecided myself about the approach.
The issue for me is the overwhelming amount of gcc warnings (thankfully
Luca's script helps a lot with it). With so many warning, it is
difficult to draw the line where to stop fixing things to generate a
digestable patch and not having the feeling of unraveling an infinite
ball of yarn. So, worried about having to change hundreds of lines of
code, I submitted the minimal change instead.

In this case though unsigned int is obviously the right type and the
patch below works as well. So I think that's better.

---
[PATCH v2] bitmap: fix nbits signess

To avoid potentially dangerous sign conversions in bitmap_switch, all
the callers of the bitmap_switch macro (which are all within bitmap.h)
should pass an unsigned int as first parameter.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
Changes in v2:
- change nbits instead

diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
index 3caf92c76d..657390e32e 100644
--- a/xen/include/xen/bitmap.h
+++ b/xen/include/xen/bitmap.h
@@ -110,7 +110,7 @@ extern int bitmap_allocate_region(unsigned long *bitmap, int pos, int order);
 		large;							  \
 	}
 
-static inline void bitmap_zero(unsigned long *dst, int nbits)
+static inline void bitmap_zero(unsigned long *dst, unsigned int nbits)
 {
 	bitmap_switch(nbits,,
 		*dst = 0UL,
@@ -134,7 +134,7 @@ static inline void bitmap_fill(unsigned long *dst, int nbits)
 }
 
 static inline void bitmap_copy(unsigned long *dst, const unsigned long *src,
-			int nbits)
+			unsigned int nbits)
 {
 	bitmap_switch(nbits,,
 		*dst = *src,
@@ -142,7 +142,7 @@ static inline void bitmap_copy(unsigned long *dst, const unsigned long *src,
 }
 
 static inline void bitmap_and(unsigned long *dst, const unsigned long *src1,
-			const unsigned long *src2, int nbits)
+			const unsigned long *src2, unsigned int nbits)
 {
 	bitmap_switch(nbits,,
 		*dst = *src1 & *src2,
@@ -150,7 +150,7 @@ static inline void bitmap_and(unsigned long *dst, const unsigned long *src1,
 }
 
 static inline void bitmap_or(unsigned long *dst, const unsigned long *src1,
-			const unsigned long *src2, int nbits)
+			const unsigned long *src2, unsigned int nbits)
 {
 	bitmap_switch(nbits,,
 		*dst = *src1 | *src2,
@@ -158,7 +158,7 @@ static inline void bitmap_or(unsigned long *dst, const unsigned long *src1,
 }
 
 static inline void bitmap_xor(unsigned long *dst, const unsigned long *src1,
-			const unsigned long *src2, int nbits)
+			const unsigned long *src2, unsigned int nbits)
 {
 	bitmap_switch(nbits,,
 		*dst = *src1 ^ *src2,
@@ -166,7 +166,7 @@ static inline void bitmap_xor(unsigned long *dst, const unsigned long *src1,
 }
 
 static inline void bitmap_andnot(unsigned long *dst, const unsigned long *src1,
-			const unsigned long *src2, int nbits)
+			const unsigned long *src2, unsigned int nbits)
 {
 	bitmap_switch(nbits,,
 		*dst = *src1 & ~*src2,
@@ -174,7 +174,7 @@ static inline void bitmap_andnot(unsigned long *dst, const unsigned long *src1,
 }
 
 static inline void bitmap_complement(unsigned long *dst, const unsigned long *src,
-			int nbits)
+			unsigned int nbits)
 {
 	bitmap_switch(nbits,,
 		*dst = ~*src & BITMAP_LAST_WORD_MASK(nbits),
@@ -182,7 +182,7 @@ static inline void bitmap_complement(unsigned long *dst, const unsigned long *sr
 }
 
 static inline int bitmap_equal(const unsigned long *src1,
-			const unsigned long *src2, int nbits)
+			const unsigned long *src2, unsigned int nbits)
 {
 	bitmap_switch(nbits,
 		return -1,
@@ -191,7 +191,7 @@ static inline int bitmap_equal(const unsigned long *src1,
 }
 
 static inline int bitmap_intersects(const unsigned long *src1,
-			const unsigned long *src2, int nbits)
+			const unsigned long *src2, unsigned int nbits)
 {
 	bitmap_switch(nbits,
 		return -1,
@@ -200,7 +200,7 @@ static inline int bitmap_intersects(const unsigned long *src1,
 }
 
 static inline int bitmap_subset(const unsigned long *src1,
-			const unsigned long *src2, int nbits)
+			const unsigned long *src2, unsigned int nbits)
 {
 	bitmap_switch(nbits,
 		return -1,
@@ -208,7 +208,7 @@ static inline int bitmap_subset(const unsigned long *src1,
 		return __bitmap_subset(src1, src2, nbits));
 }
 
-static inline int bitmap_empty(const unsigned long *src, int nbits)
+static inline int bitmap_empty(const unsigned long *src, unsigned int nbits)
 {
 	bitmap_switch(nbits,
 		return -1,
@@ -216,7 +216,7 @@ static inline int bitmap_empty(const unsigned long *src, int nbits)
 		return __bitmap_empty(src, nbits));
 }
 
-static inline int bitmap_full(const unsigned long *src, int nbits)
+static inline int bitmap_full(const unsigned long *src, unsigned int nbits)
 {
 	bitmap_switch(nbits,
 		return -1,

On 29.09.2023 23:13, Stefano Stabellini wrote:
> [PATCH v2] bitmap: fix nbits signess
> 
> To avoid potentially dangerous sign conversions in bitmap_switch, all
> the callers of the bitmap_switch macro (which are all within bitmap.h)
> should pass an unsigned int as first parameter.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

On Fri, Sep 29, 2023 at 02:13:59PM -0700, Stefano Stabellini wrote:
> On Fri, 29 Sep 2023, Julien Grall wrote:
> > On 29/09/2023 00:19, Stefano Stabellini wrote:
> > > All callers of the bitmap_switch macro (which are all within bitmap.h)
> > > pass an int as first parameter. Do not assign it to an unsigned int
> > > potentially introducing errors.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > > ---
> > > 
> > > We could also have gone the other way and change all the callers and the
> > > callers' callers to use an unsigned int instead, but I went for the path
> > > of least resistance.
> > 
> > I understand this will solve the issue right now because the callers are all
> > passing 'int'. However, all the callers will need to switch to 'unsigned int'
> > in order to solve violations in their callers.
> > 
> > That unless we decide to use 'int' everywhere, but I think this is a bad idea
> > because 'n__' is not supposed to be negative.
> > 
> > Overall, this may be an easy win right now, but this will need to be reverted.
> > So, I am not happy to ack it and would in fact be leaning towards Nacking it.
> 
> I understand this point and I was undecided myself about the approach.
> The issue for me is the overwhelming amount of gcc warnings (thankfully
> Luca's script helps a lot with it). With so many warning, it is
> difficult to draw the line where to stop fixing things to generate a
> digestable patch and not having the feeling of unraveling an infinite
> ball of yarn. So, worried about having to change hundreds of lines of
> code, I submitted the minimal change instead.
> 
> In this case though unsigned int is obviously the right type and the
> patch below works as well. So I think that's better.

This looks much better to me.  Did changing the nbits arguments of
bitmap_copy(), bitmap_fill(), and bitmap_weight(), result in the mess
getting worse?  If so then this is a reasonable stopping point, but if
you didn't check I would be tempted to do so (ensure consistency amoung
these functions).


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

On Fri, Sep 29, 2023 at 08:24:37AM +0100, Julien Grall wrote:
> Hi Stefano,
> 
> On 29/09/2023 00:19, Stefano Stabellini wrote:
> > All callers of the bitmap_switch macro (which are all within bitmap.h)
> > pass an int as first parameter. Do not assign it to an unsigned int
> > potentially introducing errors.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > ---
> > 
> > We could also have gone the other way and change all the callers and the
> > callers' callers to use an unsigned int instead, but I went for the path
> > of least resistance.
> 
> I understand this will solve the issue right now because the callers are 
> all passing 'int'. However, all the callers will need to switch to 
> 'unsigned int' in order to solve violations in their callers.
> 
> That unless we decide to use 'int' everywhere, but I think this is a bad 
> idea because 'n__' is not supposed to be negative.
> 
> Overall, this may be an easy win right now, but this will need to be 
> reverted. So, I am not happy to ack it and would in fact be leaning 
> towards Nacking it.

I would have tended to review on the basis situations like this unsigned
is always the way to go.  n__ *wants* to be unsigned since such
structures nearly always have some level of implicit assemption of being
unsigned.

Turns out though the situation is worse since as proposed this also
significantly changes the logic.  The value of `(int)n__ <= BITS_PER_LONG`
may differ from `(unsigned int)n__ <= BITS_PER_LONG`.  I wouldn't expect
these functions to get negative values, but if they did a lurking issue
has been added.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

> On 29 Sep 2023, at 08:24, Julien Grall <julien@xen.org> wrote:
> 
> Hi Stefano,
> 
> On 29/09/2023 00:19, Stefano Stabellini wrote:
>> All callers of the bitmap_switch macro (which are all within bitmap.h)
>> pass an int as first parameter. Do not assign it to an unsigned int
>> potentially introducing errors.
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>> ---
>> We could also have gone the other way and change all the callers and the
>> callers' callers to use an unsigned int instead, but I went for the path
>> of least resistance.
> 
> I understand this will solve the issue right now because the callers are all passing 'int'. However, all the callers will need to switch to 'unsigned int' in order to solve violations in their callers.

I was about to send a patch like this but Stefano beat me sending this one,
anyway I confirm that switching the callers type will solve this violation.

Cheers,
Luca