diff mbox series

[3/7] ACPI/IORT: Handle memory address size limits as limits

Message ID 2ae6199a9cf035c1defd42e48675b827f41cdc95.1701268753.git.robin.murphy@arm.com
State Superseded
Headers show
Series dma-mapping: Clean up arch_setup_dma_ops() | expand

Commit Message

Robin Murphy Nov. 29, 2023, 5:43 p.m. UTC
Return the Root Complex/Named Component memory address size limit as an
inclusive limit value, rather than an exclusive size.  This saves us
having to special-case 64-bit overflow, and simplifies our caller too.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/acpi/arm64/dma.c  |  9 +++------
 drivers/acpi/arm64/iort.c | 18 ++++++++----------
 include/linux/acpi_iort.h |  4 ++--
 3 files changed, 13 insertions(+), 18 deletions(-)

Comments

Will Deacon Dec. 11, 2023, 1:27 p.m. UTC | #1
On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> Return the Root Complex/Named Component memory address size limit as an
> inclusive limit value, rather than an exclusive size.  This saves us
> having to special-case 64-bit overflow, and simplifies our caller too.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/acpi/arm64/dma.c  |  9 +++------
>  drivers/acpi/arm64/iort.c | 18 ++++++++----------
>  include/linux/acpi_iort.h |  4 ++--
>  3 files changed, 13 insertions(+), 18 deletions(-)

[...]

> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 6496ff5a6ba2..eb64d8e17dd1 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>  { return -ENODEV; }
>  #endif
>  
> -static int nc_dma_get_range(struct device *dev, u64 *size)
> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>  {
>  	struct acpi_iort_node *node;
>  	struct acpi_iort_named_component *ncomp;
> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>  		return -EINVAL;
>  	}
>  
> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> -			1ULL<<ncomp->memory_address_limit;
> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;

The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
to drop that? You mention it in the cover letter, so clearly I'm missing
something!

>  
>  	return 0;
>  }
>  
> -static int rc_dma_get_range(struct device *dev, u64 *size)
> +static int rc_dma_get_range(struct device *dev, u64 *limit)
>  {
>  	struct acpi_iort_node *node;
>  	struct acpi_iort_root_complex *rc;
> @@ -1408,8 +1407,7 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
>  		return -EINVAL;
>  	}
>  
> -	*size = rc->memory_address_limit >= 64 ? U64_MAX :
> -			1ULL<<rc->memory_address_limit;
> +	*limit = (1ULL << rc->memory_address_limit) - 1;

Same thing here.

Will
Robin Murphy Dec. 11, 2023, 3:01 p.m. UTC | #2
On 2023-12-11 1:27 pm, Will Deacon wrote:
> On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
>> Return the Root Complex/Named Component memory address size limit as an
>> inclusive limit value, rather than an exclusive size.  This saves us
>> having to special-case 64-bit overflow, and simplifies our caller too.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/acpi/arm64/dma.c  |  9 +++------
>>   drivers/acpi/arm64/iort.c | 18 ++++++++----------
>>   include/linux/acpi_iort.h |  4 ++--
>>   3 files changed, 13 insertions(+), 18 deletions(-)
> 
> [...]
> 
>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> index 6496ff5a6ba2..eb64d8e17dd1 100644
>> --- a/drivers/acpi/arm64/iort.c
>> +++ b/drivers/acpi/arm64/iort.c
>> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>>   { return -ENODEV; }
>>   #endif
>>   
>> -static int nc_dma_get_range(struct device *dev, u64 *size)
>> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>>   {
>>   	struct acpi_iort_node *node;
>>   	struct acpi_iort_named_component *ncomp;
>> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>>   		return -EINVAL;
>>   	}
>>   
>> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
>> -			1ULL<<ncomp->memory_address_limit;
>> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> 
> The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> to drop that? You mention it in the cover letter, so clearly I'm missing
> something!

Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus 
subtracting 1 results in the correct all-bits-set value for an inclusive 
64-bit limit.

Thanks,
Robin.

>>   
>>   	return 0;
>>   }
>>   
>> -static int rc_dma_get_range(struct device *dev, u64 *size)
>> +static int rc_dma_get_range(struct device *dev, u64 *limit)
>>   {
>>   	struct acpi_iort_node *node;
>>   	struct acpi_iort_root_complex *rc;
>> @@ -1408,8 +1407,7 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
>>   		return -EINVAL;
>>   	}
>>   
>> -	*size = rc->memory_address_limit >= 64 ? U64_MAX :
>> -			1ULL<<rc->memory_address_limit;
>> +	*limit = (1ULL << rc->memory_address_limit) - 1;
> 
> Same thing here.
> 
> Will
Will Deacon Dec. 11, 2023, 3:30 p.m. UTC | #3
On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> On 2023-12-11 1:27 pm, Will Deacon wrote:
> > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > Return the Root Complex/Named Component memory address size limit as an
> > > inclusive limit value, rather than an exclusive size.  This saves us
> > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > 
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > >   include/linux/acpi_iort.h |  4 ++--
> > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > 
> > [...]
> > 
> > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > --- a/drivers/acpi/arm64/iort.c
> > > +++ b/drivers/acpi/arm64/iort.c
> > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > >   { return -ENODEV; }
> > >   #endif
> > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > >   {
> > >   	struct acpi_iort_node *node;
> > >   	struct acpi_iort_named_component *ncomp;
> > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > >   		return -EINVAL;
> > >   	}
> > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > -			1ULL<<ncomp->memory_address_limit;
> > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > 
> > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > to drop that? You mention it in the cover letter, so clearly I'm missing
> > something!
> 
> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
> subtracting 1 results in the correct all-bits-set value for an inclusive
> 64-bit limit.

Oh, I'd have thought you'd have gotten one of those "left shift count >=
width of type" warnings if you did that.

Will
Jason Gunthorpe Dec. 11, 2023, 3:36 p.m. UTC | #4
On Mon, Dec 11, 2023 at 03:30:24PM +0000, Will Deacon wrote:
> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> > On 2023-12-11 1:27 pm, Will Deacon wrote:
> > > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > > Return the Root Complex/Named Component memory address size limit as an
> > > > inclusive limit value, rather than an exclusive size.  This saves us
> > > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > > 
> > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > > ---
> > > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > > >   include/linux/acpi_iort.h |  4 ++--
> > > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > > 
> > > [...]
> > > 
> > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > > --- a/drivers/acpi/arm64/iort.c
> > > > +++ b/drivers/acpi/arm64/iort.c
> > > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > > >   { return -ENODEV; }
> > > >   #endif
> > > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > > >   {
> > > >   	struct acpi_iort_node *node;
> > > >   	struct acpi_iort_named_component *ncomp;
> > > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > > >   		return -EINVAL;
> > > >   	}
> > > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > > -			1ULL<<ncomp->memory_address_limit;
> > > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > > 
> > > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > > to drop that? You mention it in the cover letter, so clearly I'm missing
> > > something!
> > 
> > Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
> > subtracting 1 results in the correct all-bits-set value for an inclusive
> > 64-bit limit.
> 
> Oh, I'd have thought you'd have gotten one of those "left shift count >=
> width of type" warnings if you did that.

Yes, UBSAN generates warnings for these cases. I'm not sure if it is
actually undefined C behavior or just "suspicious", but such is what
it is..

Jason
Mark Rutland Dec. 11, 2023, 3:37 p.m. UTC | #5
On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> On 2023-12-11 1:27 pm, Will Deacon wrote:
> > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > Return the Root Complex/Named Component memory address size limit as an
> > > inclusive limit value, rather than an exclusive size.  This saves us
> > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > 
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > >   include/linux/acpi_iort.h |  4 ++--
> > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > 
> > [...]
> > 
> > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > --- a/drivers/acpi/arm64/iort.c
> > > +++ b/drivers/acpi/arm64/iort.c
> > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > >   { return -ENODEV; }
> > >   #endif
> > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > >   {
> > >   	struct acpi_iort_node *node;
> > >   	struct acpi_iort_named_component *ncomp;
> > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > >   		return -EINVAL;
> > >   	}
> > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > -			1ULL<<ncomp->memory_address_limit;
> > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > 
> > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > to drop that? You mention it in the cover letter, so clearly I'm missing
> > something!
> 
> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), 

I'm pretty sure that regardless of whether a type is signed, shifting more than
the type's width is undefined behaviour. That causes GCC to scream at compile
time:

|   CC      arch/arm64/kernel/setup.o
| arch/arm64/kernel/setup.c: In function 'shift_test':
| arch/arm64/kernel/setup.c:295:20: warning: left shift count >= width of type [-Wshift-count-overflow]
|   295 |         return 1UL << 64;
|       |                    ^~

... and a UBSAN splat:

| ================================================================================
| UBSAN: shift-out-of-bounds in arch/arm64/kernel/setup.c:295:13
| shift exponent 64 is too large for 64-bit type 'long unsigned int'
| CPU: 0 PID: 0 Comm: swapper Not tainted 6.7.0-rc1-00005-g06034455cb74-dirty #3
| Call trace:
|  dump_backtrace+0x90/0xe8
|  show_stack+0x18/0x24
|  dump_stack_lvl+0x48/0x60
|  dump_stack+0x18/0x24
|  __ubsan_handle_shift_out_of_bounds+0x114/0x244
|  shift_test+0x24/0x34
|  setup_arch+0x238/0x68c
|  start_kernel+0x70/0x610
|  __primary_switched+0xbc/0xc4
| ================================================================================

Mark.

> thus
> subtracting 1 results in the correct all-bits-set value for an inclusive
> 64-bit limit.
> 
> Thanks,
> Robin.
> 
> > >   	return 0;
> > >   }
> > > -static int rc_dma_get_range(struct device *dev, u64 *size)
> > > +static int rc_dma_get_range(struct device *dev, u64 *limit)
> > >   {
> > >   	struct acpi_iort_node *node;
> > >   	struct acpi_iort_root_complex *rc;
> > > @@ -1408,8 +1407,7 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
> > >   		return -EINVAL;
> > >   	}
> > > -	*size = rc->memory_address_limit >= 64 ? U64_MAX :
> > > -			1ULL<<rc->memory_address_limit;
> > > +	*limit = (1ULL << rc->memory_address_limit) - 1;
> > 
> > Same thing here.
> > 
> > Will
Robin Murphy Dec. 11, 2023, 3:37 p.m. UTC | #6
On 2023-12-11 3:30 pm, Will Deacon wrote:
> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
>> On 2023-12-11 1:27 pm, Will Deacon wrote:
>>> On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
>>>> Return the Root Complex/Named Component memory address size limit as an
>>>> inclusive limit value, rather than an exclusive size.  This saves us
>>>> having to special-case 64-bit overflow, and simplifies our caller too.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>    drivers/acpi/arm64/dma.c  |  9 +++------
>>>>    drivers/acpi/arm64/iort.c | 18 ++++++++----------
>>>>    include/linux/acpi_iort.h |  4 ++--
>>>>    3 files changed, 13 insertions(+), 18 deletions(-)
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>> index 6496ff5a6ba2..eb64d8e17dd1 100644
>>>> --- a/drivers/acpi/arm64/iort.c
>>>> +++ b/drivers/acpi/arm64/iort.c
>>>> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>>>>    { return -ENODEV; }
>>>>    #endif
>>>> -static int nc_dma_get_range(struct device *dev, u64 *size)
>>>> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>>>>    {
>>>>    	struct acpi_iort_node *node;
>>>>    	struct acpi_iort_named_component *ncomp;
>>>> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>>>>    		return -EINVAL;
>>>>    	}
>>>> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
>>>> -			1ULL<<ncomp->memory_address_limit;
>>>> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
>>>
>>> The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
>>> to drop that? You mention it in the cover letter, so clearly I'm missing
>>> something!
>>
>> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
>> subtracting 1 results in the correct all-bits-set value for an inclusive
>> 64-bit limit.
> 
> Oh, I'd have thought you'd have gotten one of those "left shift count >=
> width of type" warnings if you did that.

Compilers might give such a warning if it was a constant shift whose 
size was visible at compile time, but even then only because compilers 
seem to have a vendetta against us relying on the well-defined 
behaviours of unsigned integer overflow (it's only *signed* shifts which 
are UB if the result is unrepresentable).

Cheers,
Robin.
Mark Rutland Dec. 11, 2023, 3:39 p.m. UTC | #7
On Mon, Dec 11, 2023 at 03:30:24PM +0000, Will Deacon wrote:
> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> > On 2023-12-11 1:27 pm, Will Deacon wrote:
> > > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > > Return the Root Complex/Named Component memory address size limit as an
> > > > inclusive limit value, rather than an exclusive size.  This saves us
> > > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > > 
> > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > > ---
> > > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > > >   include/linux/acpi_iort.h |  4 ++--
> > > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > > 
> > > [...]
> > > 
> > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > > --- a/drivers/acpi/arm64/iort.c
> > > > +++ b/drivers/acpi/arm64/iort.c
> > > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > > >   { return -ENODEV; }
> > > >   #endif
> > > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > > >   {
> > > >   	struct acpi_iort_node *node;
> > > >   	struct acpi_iort_named_component *ncomp;
> > > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > > >   		return -EINVAL;
> > > >   	}
> > > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > > -			1ULL<<ncomp->memory_address_limit;
> > > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > > 
> > > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > > to drop that? You mention it in the cover letter, so clearly I'm missing
> > > something!
> > 
> > Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
> > subtracting 1 results in the correct all-bits-set value for an inclusive
> > 64-bit limit.
> 
> Oh, I'd have thought you'd have gotten one of those "left shift count >=
> width of type" warnings if you did that.

I think you'll get a UBSAN splat, but here the compiler doesn't know what
'ncomp->memory_address_limit' will be and so doesn't produce a compile-time
warning.

Regardless, it's undefined behaviour.

Mark.
Robin Murphy Dec. 11, 2023, 4:13 p.m. UTC | #8
On 2023-12-11 3:39 pm, Mark Rutland wrote:
> On Mon, Dec 11, 2023 at 03:30:24PM +0000, Will Deacon wrote:
>> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
>>> On 2023-12-11 1:27 pm, Will Deacon wrote:
>>>> On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
>>>>> Return the Root Complex/Named Component memory address size limit as an
>>>>> inclusive limit value, rather than an exclusive size.  This saves us
>>>>> having to special-case 64-bit overflow, and simplifies our caller too.
>>>>>
>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>> ---
>>>>>    drivers/acpi/arm64/dma.c  |  9 +++------
>>>>>    drivers/acpi/arm64/iort.c | 18 ++++++++----------
>>>>>    include/linux/acpi_iort.h |  4 ++--
>>>>>    3 files changed, 13 insertions(+), 18 deletions(-)
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>>> index 6496ff5a6ba2..eb64d8e17dd1 100644
>>>>> --- a/drivers/acpi/arm64/iort.c
>>>>> +++ b/drivers/acpi/arm64/iort.c
>>>>> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>>>>>    { return -ENODEV; }
>>>>>    #endif
>>>>> -static int nc_dma_get_range(struct device *dev, u64 *size)
>>>>> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>>>>>    {
>>>>>    	struct acpi_iort_node *node;
>>>>>    	struct acpi_iort_named_component *ncomp;
>>>>> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>>>>>    		return -EINVAL;
>>>>>    	}
>>>>> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
>>>>> -			1ULL<<ncomp->memory_address_limit;
>>>>> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
>>>>
>>>> The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
>>>> to drop that? You mention it in the cover letter, so clearly I'm missing
>>>> something!
>>>
>>> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
>>> subtracting 1 results in the correct all-bits-set value for an inclusive
>>> 64-bit limit.
>>
>> Oh, I'd have thought you'd have gotten one of those "left shift count >=
>> width of type" warnings if you did that.
> 
> I think you'll get a UBSAN splat, but here the compiler doesn't know what
> 'ncomp->memory_address_limit' will be and so doesn't produce a compile-time
> warning.
> 
> Regardless, it's undefined behaviour.

Urgh, you're right... I double-checked 6.5.7.4 in the standard but 
managed to miss 6.5.7.3. So yeah, even though "4 << 62" or "2 << 63" are 
well-defined here, "1  << 64" isn't, dang. Thanks, funky old ISAs which 
did weird things for crazy large shifts and have no relevance to this 
code :(

Cheers,
Robin.
diff mbox series

Patch

diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c
index 93d796531af3..b98a149f8d50 100644
--- a/drivers/acpi/arm64/dma.c
+++ b/drivers/acpi/arm64/dma.c
@@ -8,7 +8,6 @@  void acpi_arch_dma_setup(struct device *dev)
 {
 	int ret;
 	u64 end, mask;
-	u64 size = 0;
 	const struct bus_dma_region *map = NULL;
 
 	/*
@@ -23,9 +22,9 @@  void acpi_arch_dma_setup(struct device *dev)
 	}
 
 	if (dev->coherent_dma_mask)
-		size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
+		end = dev->coherent_dma_mask;
 	else
-		size = 1ULL << 32;
+		end = (1ULL << 32) - 1;
 
 	ret = acpi_dma_get_range(dev, &map);
 	if (!ret && map) {
@@ -36,18 +35,16 @@  void acpi_arch_dma_setup(struct device *dev)
 				end = r->dma_start + r->size - 1;
 		}
 
-		size = end + 1;
 		dev->dma_range_map = map;
 	}
 
 	if (ret == -ENODEV)
-		ret = iort_dma_get_ranges(dev, &size);
+		ret = iort_dma_get_ranges(dev, &end);
 	if (!ret) {
 		/*
 		 * Limit coherent and dma mask based on size retrieved from
 		 * firmware.
 		 */
-		end = size - 1;
 		mask = DMA_BIT_MASK(ilog2(end) + 1);
 		dev->bus_dma_limit = end;
 		dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 6496ff5a6ba2..eb64d8e17dd1 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1367,7 +1367,7 @@  int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
 { return -ENODEV; }
 #endif
 
-static int nc_dma_get_range(struct device *dev, u64 *size)
+static int nc_dma_get_range(struct device *dev, u64 *limit)
 {
 	struct acpi_iort_node *node;
 	struct acpi_iort_named_component *ncomp;
@@ -1384,13 +1384,12 @@  static int nc_dma_get_range(struct device *dev, u64 *size)
 		return -EINVAL;
 	}
 
-	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
-			1ULL<<ncomp->memory_address_limit;
+	*limit = (1ULL << ncomp->memory_address_limit) - 1;
 
 	return 0;
 }
 
-static int rc_dma_get_range(struct device *dev, u64 *size)
+static int rc_dma_get_range(struct device *dev, u64 *limit)
 {
 	struct acpi_iort_node *node;
 	struct acpi_iort_root_complex *rc;
@@ -1408,8 +1407,7 @@  static int rc_dma_get_range(struct device *dev, u64 *size)
 		return -EINVAL;
 	}
 
-	*size = rc->memory_address_limit >= 64 ? U64_MAX :
-			1ULL<<rc->memory_address_limit;
+	*limit = (1ULL << rc->memory_address_limit) - 1;
 
 	return 0;
 }
@@ -1417,16 +1415,16 @@  static int rc_dma_get_range(struct device *dev, u64 *size)
 /**
  * iort_dma_get_ranges() - Look up DMA addressing limit for the device
  * @dev: device to lookup
- * @size: DMA range size result pointer
+ * @limit: DMA limit result pointer
  *
  * Return: 0 on success, an error otherwise.
  */
-int iort_dma_get_ranges(struct device *dev, u64 *size)
+int iort_dma_get_ranges(struct device *dev, u64 *limit)
 {
 	if (dev_is_pci(dev))
-		return rc_dma_get_range(dev, size);
+		return rc_dma_get_range(dev, limit);
 	else
-		return nc_dma_get_range(dev, size);
+		return nc_dma_get_range(dev, limit);
 }
 
 static void __init acpi_iort_register_irq(int hwirq, const char *name,
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 1cb65592c95d..d4ed5622cf2b 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -39,7 +39,7 @@  void iort_get_rmr_sids(struct fwnode_handle *iommu_fwnode,
 void iort_put_rmr_sids(struct fwnode_handle *iommu_fwnode,
 		       struct list_head *head);
 /* IOMMU interface */
-int iort_dma_get_ranges(struct device *dev, u64 *size);
+int iort_dma_get_ranges(struct device *dev, u64 *limit);
 int iort_iommu_configure_id(struct device *dev, const u32 *id_in);
 void iort_iommu_get_resv_regions(struct device *dev, struct list_head *head);
 phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
@@ -55,7 +55,7 @@  void iort_get_rmr_sids(struct fwnode_handle *iommu_fwnode, struct list_head *hea
 static inline
 void iort_put_rmr_sids(struct fwnode_handle *iommu_fwnode, struct list_head *head) { }
 /* IOMMU interface */
-static inline int iort_dma_get_ranges(struct device *dev, u64 *size)
+static inline int iort_dma_get_ranges(struct device *dev, u64 *limit)
 { return -ENODEV; }
 static inline int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
 { return -ENODEV; }