Messages in this thread | | | Date | Tue, 28 Nov 2023 13:41:57 +0530 | Subject | Re: [PATCH] iommu: Don't reserve IOVA when address and size are zero | From | Ashish Mhetre <> |
| |
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,
| |