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/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; }
--
2.39.2.101.g768bb238c484.dirtyOn 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
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
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
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
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.
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.
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.
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
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(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
© 2016 - 2025 Red Hat, Inc.