iommu-common: Do not use 64 bit constant 0xffffffffffffffffl for computing align_mask

PATCH Superseded Sowmini Varadhan 4 participants 10 messages Jul 19, 2015 – Jul 21, 2015 Library code

Share & tools

Sowmini Varadhan

author

2015-07-19 12:20 UTC #0 lore raw

PATCH 1/1 iommu-common: Do not use 64 bit constant 0xffffffffffffffffl for computing align_mask

Using a 64 bit constant generates "warning: integer constant is too large for 'long' type" on 32 bit platforms. Instead use ~0l to get the desired effect.

Detected by Andrew Morton who has confirmed that this patch fixes the warning on i386/gcc-4.4.3, i386/gcc-4.4.0 and arm/gcc-4.4.4.

Signed-off-by Sowmini Varadhan <[email protected]>

---
 lib/iommu-common.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

lib/iommu-common.c +1 -1

index df30632..fd1297d 100644
@@ -119,7 +119,7 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
 	unsigned long align_mask = 0;
 
 	if (align_order > 0)
-		align_mask = 0xffffffffffffffffl >> (64 - align_order);
+		align_mask = ~0l >> (64 - align_order);
 
 	/* Sanity check */
 	if (unlikely(npages == 0)) {

Signature · 1 line

1.7.1

Guenter Roeck

2015-07-19 15:27 UTC #1 lore raw

Quoted 18 lines · Sowmini Varadhan

Using a 64 bit constant generates "warning: integer constant is too large for 'long' type" on 32 bit platforms. Instead use ~0l to get the desired effect.
Detected by Andrew Morton who has confirmed that this patch fixes the warning on i386/gcc-4.4.3, i386/gcc-4.4.0 and arm/gcc-4.4.4.
Signed-off-by Sowmini Varadhan <[email protected]>
---
 lib/iommu-common.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
lib/iommu-common.c +0 -0
index df30632..fd1297d 100644

lib/iommu-common.c unsigned long iommu_tbl_range_alloc(struct device *dev, · line 123

Quoted 6 lines

lib/iommu-common.c +1 -1

@@ -119,7 +119,7 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
 	unsigned long align_mask = 0;
 
 	if (align_order > 0)
-		align_mask = 0xffffffffffffffffl >> (64 - align_order);
+		align_mask = ~0l >> (64 - align_order);

Wonder if this just hides the real problem. Unless align_order is very large, the resulting mask on 32 bit systems may be 0. Is this really the idea ?

Guenter

Rasmus Villemoes

2015-07-19 20:25 UTC #2 lore raw

Quoted 21 lines · Guenter Roeck

Quoted 20 lines · Sowmini Varadhan
Using a 64 bit constant generates "warning: integer constant is too large for 'long' type" on 32 bit platforms. Instead use ~0l to get the desired effect.
Detected by Andrew Morton who has confirmed that this patch fixes the warning on i386/gcc-4.4.3, i386/gcc-4.4.0 and arm/gcc-4.4.4.
Signed-off-by Sowmini Varadhan <[email protected]>
---
 lib/iommu-common.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
lib/iommu-common.c +0 -0
index df30632..fd1297d 100644
@@ -119,7 +119,7 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
 	unsigned long align_mask = 0;
 

Quoted 7 lines

lib/iommu-common.c unsigned long iommu_tbl_range_alloc(struct device *dev, · line 123
Quoted 4 lines
lib/iommu-common.c +1 -1
 	if (align_order > 0)
-		align_mask = 0xffffffffffffffffl >> (64 - align_order);
+		align_mask = ~0l >> (64 - align_order);
 
Wonder if this just hides the real problem. Unless align_order is very large, the resulting mask on 32 bit systems may be 0. Is this really the idea ?

Probably not, but that's not what would happen on x86: the shift only depends on the lower 5 or 6 bits - I don't know if other platforms also has that behaviour. So for align_order == 2 and x86_32 we'd effectively end up with a shift of 62 & 31 == 30 (though technically undefined behaviour), and the desired mask of 0x3.

Wouldn't GENMASK(align_order-1, 0) work for all cases (assuming align_order has a sane value)?

Rasmus

Sowmini Varadhan

author

2015-07-19 20:42 UTC #3 lore raw

Quoted 3 lines · Rasmus Villemoes

Wouldn't GENMASK(align_order-1, 0) work for all cases (assuming align_order has a sane value)?

Devices with limits on DMA masks are uncommon, so I'm personally not an expert at all the variations in this space, but I was thinking that this doing

   align_mask = 0xffffffffffffffffull >> (64 - align_order);

would be the compact answer for both 32 and 64 bit cases here?

--Sowmini

Guenter Roeck

2015-07-20 01:58 UTC #4 lore raw

In reply to Rasmus Villemoes

Quoted 31 lines · Rasmus Villemoes

Quoted 21 lines · Guenter Roeck
Quoted 20 lines · Sowmini Varadhan
Using a 64 bit constant generates "warning: integer constant is too large for 'long' type" on 32 bit platforms. Instead use ~0l to get the desired effect.
Detected by Andrew Morton who has confirmed that this patch fixes the warning on i386/gcc-4.4.3, i386/gcc-4.4.0 and arm/gcc-4.4.4.
Signed-off-by Sowmini Varadhan <[email protected]>
---
  lib/iommu-common.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
lib/iommu-common.c +0 -0
index df30632..fd1297d 100644
@@ -119,7 +119,7 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
  	unsigned long align_mask = 0;
 
Quoted 7 lines
lib/iommu-common.c unsigned long iommu_tbl_range_alloc(struct device *dev, · line 123
Quoted 4 lines
lib/iommu-common.c +1 -1
  	if (align_order > 0)
-		align_mask = 0xffffffffffffffffl >> (64 - align_order);
+		align_mask = ~0l >> (64 - align_order);
 
Wonder if this just hides the real problem. Unless align_order is very large, the resulting mask on 32 bit systems may be 0. Is this really the idea ?

Quoted 6 lines

Probably not, but that's not what would happen on x86: the shift only depends on the lower 5 or 6 bits - I don't know if other platforms also has that behaviour. So for align_order == 2 and x86_32 we'd effectively end up with a shift of 62 & 31 == 30 (though technically undefined behaviour), and the desired mask of 0x3.

#include <stdio.h>

main(int argc, char **argv)

{
        unsigned long am1, am2, am3;
        int align_order = 2;

        am1 = 0xffffffffffffffffl >> (64 - align_order);
        am2 = ~0l >> (64 - align_order);
        am3 = ~0ul >> (64 - align_order);

        printf("0x%lx 0x%lx 0x%lx\n", am1, am2, am3);
}

yields an output of

0x3 0xffffffffffffffff 0x3

So either case ~0l appears to be wrong; it should be ~0ul. I don't know if ~0ull makes a difference for some architectures.

Guenter

Sowmini Varadhan

author

2015-07-20 14:44 UTC #9 lore raw

In reply to Guenter Roeck

Quoted 6 lines · Guenter Roeck

lib/iommu-common.c unsigned long iommu_tbl_range_alloc(struct device *dev, · line 122
Quoted 3 lines
lib/iommu-common.c +1 -1
-		align_mask = 0xffffffffffffffffl >> (64 - align_order);
+		align_mask = ~0l >> (64 - align_order);
Wonder if this just hides the real problem. Unless align_order is very large, the resulting mask on 32 bit systems may be 0. Is this really the idea ?

Quoted 2 lines

So either case ~0l appears to be wrong; it should be ~0ul. I don't know if ~0ull makes a difference for some architectures.

I agree about the unsigned part. However, regarding the arch specific twists..

I checked into this.. even though I have a test program on x86_64 that "does the right thing" for both of

  align_mask = ~0ul >> (64 - align_order);
  align_mask = ~0ul >> (BITS_PER_LONG - align_order);

when I compiled with -m32 and without (I tried align_order == 1 and 31 for edge cases), I think there are some gcc/arch specific variations possible based on undefined behavior, so that the second variant is safer.

I'll send out a patch with that version soon.

--Sowmini

Rasmus Villemoes

2015-07-20 17:57 UTC #5 lore raw

In reply to Guenter Roeck

Quoted 1 line · Guenter Roeck

So either case ~0l appears to be wrong; it should be ~0ul.

Yes, right-shifting -1 of any type is probably always wrong, as it will always give -1 again. Probably one should add a smatch/sparse warning for that.

Quoted 1 line

I don't know if ~0ull makes a difference for some architectures.

I highly doubt it. The result is truncated to unsigned long anyway. Assuming align_order always has a value between 0 and BITS_PER_LONG, GENMASK should be exactly what is wanted.

Rasmus

Sowmini Varadhan

author

2015-07-20 19:26 UTC #6 lore raw

Quoted 3 lines · Rasmus Villemoes

I highly doubt it. The result is truncated to unsigned long anyway. Assuming align_order always has a value between 0 and BITS_PER_LONG, GENMASK should be exactly what is wanted.

While GENMASK may do the job, the code is already quite obscure, so I'm going to stick with the minimal delta to get this right, namely

- align_mask = 0xffffffffffffffffl >> (64 - align_order);
+ align_mask = ~0ul >> (BITS_PER_LONG - align_order);

--Sowmini

Andrew Morton

maintainer

2015-07-20 23:28 UTC #7 lore raw

In reply to Rasmus Villemoes

Quoted 6 lines · On Mon, 20 Jul 2015 19:57:18 +0200 Rasmus Villemoes <[email protected]>…

Quoted 1 line · Guenter Roeck
So either case ~0l appears to be wrong; it should be ~0ul.
Yes, right-shifting -1 of any type is probably always wrong, as it will always give -1 again.

Not for unsigned types.

The kernel uses "-1UL" and "-1ULL" quite a lot - it's a convenient way of saying "all ones, regardless of size". Also, assigning plain old "-1" to an unsigned variable will make that variable all-ones regardless of size.

In this case I expect we could do

	align_mask = -1UL >> (64 - align_order);

but I don't know about that 64. Maybe it should be BITS_PER_LONG?

Guenter Roeck

2015-07-21 00:26 UTC #8 lore raw

Quoted 11 lines · Andrew Morton

Quoted 6 lines · On Mon, 20 Jul 2015 19:57:18 +0200 Rasmus Villemoes <[email protected]>…
Quoted 1 line · Guenter Roeck
So either case ~0l appears to be wrong; it should be ~0ul.
Yes, right-shifting -1 of any type is probably always wrong, as it will always give -1 again.
Not for unsigned types.

Quoted 9 lines

The kernel uses "-1UL" and "-1ULL" quite a lot - it's a convenient way of saying "all ones, regardless of size". Also, assigning plain old "-1" to an unsigned variable will make that variable all-ones regardless of size.
In this case I expect we could do
	align_mask = -1UL >> (64 - align_order);

-1ul works, at least on x86 (32 and 64 bit).

Quoted 2 lines

but I don't know about that 64. Maybe it should be BITS_PER_LONG?

I think that is going to be in the next version of the patch.

Guenter