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

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

[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 <sowmini.varadhan@oracle.com>
---
 lib/iommu-common.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/iommu-common.c b/lib/iommu-common.c
index df30632..fd1297d 100644
--- a/lib/iommu-common.c
+++ b/lib/iommu-common.c
@@ -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)) {
-- 
1.7.1

Re: [PATCH] iommu-common: Do not use 64 bit constant 0xffffffffffffffffl for computing align_mask

[Guenter Roeck]

On Sun, Jul 19, 2015 at 02:20:14PM +0200, Sowmini Varadhan wrote:
> 
> 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 <sowmini.varadhan@oracle.com>
> ---
>  lib/iommu-common.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/iommu-common.c b/lib/iommu-common.c
> index df30632..fd1297d 100644
> --- a/lib/iommu-common.c
> +++ b/lib/iommu-common.c
> @@ -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

Re: [PATCH] iommu-common: Do not use 64 bit constant 0xffffffffffffffffl for computing align_mask

[Rasmus Villemoes]

On Sun, Jul 19 2015, Guenter Roeck <linux@roeck-us.net> wrote:

> On Sun, Jul 19, 2015 at 02:20:14PM +0200, Sowmini Varadhan wrote:
>> 
>> 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 <sowmini.varadhan@oracle.com>
>> ---
>>  lib/iommu-common.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/lib/iommu-common.c b/lib/iommu-common.c
>> index df30632..fd1297d 100644
>> --- a/lib/iommu-common.c
>> +++ b/lib/iommu-common.c
>> @@ -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 ?

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

Re: [PATCH] iommu-common: Do not use 64 bit constant 0xffffffffffffffffl for computing align_mask

[Sowmini Varadhan]

On (07/19/15 22:25), Rasmus Villemoes wrote:
> 
> 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

Re: [PATCH] iommu-common: Do not use 64 bit constant 0xffffffffffffffffl for computing align_mask

[Guenter Roeck]

On 07/19/2015 01:25 PM, Rasmus Villemoes wrote:
> On Sun, Jul 19 2015, Guenter Roeck <linux@roeck-us.net> wrote:
>
>> On Sun, Jul 19, 2015 at 02:20:14PM +0200, Sowmini Varadhan wrote:
>>>
>>> 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 <sowmini.varadhan@oracle.com>
>>> ---
>>>   lib/iommu-common.c |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/lib/iommu-common.c b/lib/iommu-common.c
>>> index df30632..fd1297d 100644
>>> --- a/lib/iommu-common.c
>>> +++ b/lib/iommu-common.c
>>> @@ -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 ?
>
> 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

Re: [PATCH] iommu-common: Do not use 64 bit constant 0xffffffffffffffffl for computing align_mask

[Sowmini Varadhan]

On (07/19/15 08:27), Guenter Roeck wrote:
> > -		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 ?

<subsequent example code deleted> 

> 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

Re: [PATCH] iommu-common: Do not use 64 bit constant 0xffffffffffffffffl for computing align_mask

[Rasmus Villemoes]

On Mon, Jul 20 2015, Guenter Roeck <linux@roeck-us.net> wrote:

> 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.

> 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

Re: [PATCH] iommu-common: Do not use 64 bit constant 0xffffffffffffffffl for computing align_mask

[Sowmini Varadhan]

On (07/20/15 19:57), Rasmus Villemoes wrote:
> 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

Re: [PATCH] iommu-common: Do not use 64 bit constant 0xffffffffffffffffl for computing align_mask

[Andrew Morton]

On Mon, 20 Jul 2015 19:57:18 +0200 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On Mon, Jul 20 2015, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > 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?

Re: [PATCH] iommu-common: Do not use 64 bit constant 0xffffffffffffffffl for computing align_mask

[Guenter Roeck]

On 07/20/2015 04:28 PM, Andrew Morton wrote:
> On Mon, 20 Jul 2015 19:57:18 +0200 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
>> On Mon, Jul 20 2015, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>>> 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);
>
-1ul works, at least on x86 (32 and 64 bit).

> 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