lkml.org 
[lkml]   [2023]   [Nov]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] iommu: Don't reserve IOVA when address and size are zero
From

On 11/23/2023 4:43 PM, Robin Murphy wrote:
> External email: Use caution opening links or attachments
>
>
> On 2023-11-23 6:12 am, Ashish Mhetre wrote:
>> When the bootloader/firmware doesn't setup the framebuffers, their
>> address and size are zero in "iommu-addresses" property. If we intend to
>> use display driver in kernel without framebuffer then it's causing
>> the display IOMMU mappings to fail as IOVA is reserved with size and
>> address as zero.
>
> Can you clarify the problem there? Looking at the code in
> iova_reserve_iommu_regions() I'm guessing it's that "region->start +
> region->length - 1" underflows so reserve_iova() actually ends up
> reserving the entire valid IOVA space?

Yes, that's the problem which lead to dma_map call failures from
display driver. I don't have the logs handy to pin-point the exact
function which failed as this issue was seen before few months.

>
>> An ideal solution would be firmware removing the "iommu-addresses"
>> property and corresponding "memory-region" if display is not present.
>> But the kernel should be able to handle this by checking for size and
>> address of IOVA and skipping the IOVA reservation if both are 0.
>
> Surely it doesn't make sense to reserve a 0-length region at *any* base
> address? The symptom above wouldn't be quite the same if the base was
> nonzero, but corrupting the rbtree with an entry where pfn_hi < pfn_lo
> would definitely not be good either.
>
Agreed, we should restrict reservation for 0-length region at any base.
I will update the condition in next version.


>> Fixes: a5bf3cfce8cb ("iommu: Implement of_iommu_get_resv_regions()")
>> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
>> ---
>>   drivers/iommu/of_iommu.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>> index 157b286e36bf..150ef65d357a 100644
>> --- a/drivers/iommu/of_iommu.c
>> +++ b/drivers/iommu/of_iommu.c
>> @@ -255,6 +255,10 @@ void of_iommu_get_resv_regions(struct device
>> *dev, struct list_head *list)
>>                               size_t length;
>>
>>                               maps = of_translate_dma_region(np,
>> maps, &iova, &length);
>> +                             if (iova == 0 && length == 0) {
>> +                                     dev_dbg(dev, "Skipping IOVA
>> reservation as address and size are zero\n");
>
> FWIW I'd be inclined to log a visible warning that firmware is giving us
> nonsense.
>
Okay, I'll replace dev_dbg() with dev_warn() in next version.


> Thanks,
> Robin.
>
>> + continue;
>> +                             }
>>                               type = iommu_resv_region_get_type(dev,
>> &phys, iova, length);
>>
>>                               region = iommu_alloc_resv_region(iova,
>> length, prot, type,

\
 
 \ /
  Last update: 2023-11-28 09:13    [W:0.046 / U:0.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site