Re: [PATCH v1] iommu: Skip mapping at address 0x0 if it already exists

[Robin Murphy <robin.murphy@arm.com>]

On 2026-02-24 7:51 pm, Antheas Kapenekakis wrote:
> On Tue, 24 Feb 2026 at 20:43, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>>
>> On Tue, Feb 24, 2026 at 08:33:50PM +0100, Antheas Kapenekakis wrote:
>>> On Tue, 24 Feb 2026 at 20:23, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>>>>
>>>> On Sun, Feb 22, 2026 at 12:50:50AM +0100, Antheas Kapenekakis wrote:
>>>>> Commit 789a5913b29c ("iommu/amd: Use the generic iommu page table")
>>>>> introduces the shared iommu page table for AMD IOMMU. Some bioses
>>>>> contain an identity mapping for address 0x0, which is not parsed
>>>>> properly (e.g., certain Strix Halo devices). This causes the DMA
>>>>> components of the device to fail to initialize (e.g., the NVMe SSD
>>>>> controller), leading to a failed post.
>>>>
>>>> I'm trying to understand the issue here, is it that the old AMD code
>>>> incorrectly succeeded iommu_map() on top of an existing mapping while
>>>> the new code returns -EADDRINUSE?
>>>>
>>>> Then the existing guard for double mapping doesn't work since 0 is an
>>>> ambiguous return?
>>>
>>> Hi Jason,
>>>
>>> It seems like the previous code correctly handled the 0 case, and the
>>> new code does not due to the ambiguous return.
>>
>> I checked the old AMD driver it is definately returning 0 from
>> iommu_iova_to_phys(), so that hasn't changed.
>>
>>>> reasonabe. Just please clean up the commit message to be a bit clearer
>>>> on what caused this regression and add a short comment above the new if:
>>>>
>>>>   0 return from iommu_iova_to_phys() is ambiguous, it could mean a
>>>>   present mapping. EADDRINUSE is reliable when supported, it means the
>>>>   IOVA is already mapped, so ignore it to resolve the ambiguity.
>>>
>>> If you think that this patch is a long term solution, I can drop the
>>> print, the addr == 0 check, add a comment on top, then repost a V2
>>> with an updated description to be more concrete.
>>
>> I think it is a good solution as-is, I just want clarity on what
>> actually regressed here, and I think it is that iommu_map() behavior
>> changed.
> 
> Reverting 789a5913b29c ("iommu/amd: Use the generic iommu page table")
> plus two follow-up fixes also worked.
> 
> Looking through it, the removed amd_iommu_map_pages() /
> amd_iommu_iova_to_phys() in the previous ops might have behaved
> differently.

Yes, the old AMD code would happily replace an existing mapping without
question (but whether it would correctly invalidate TLBs if just
replacing a leaf entry with a different leaf entry at the same level
I'm not entirely sure...)

However, waiting for a mapping to fail (which may scream its own
warning) and then attempting to paper over it seems like an awful bodge.
Why not just do something like this?

Cheers,
Robin.

----->8-----
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 35db51780954..682e95c94236 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1212,8 +1212,11 @@ static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
  
  			if (addr == end)
  				goto map_end;
-
-			phys_addr = iommu_iova_to_phys(domain, addr);
+			/*
+			 * Check for existing translations using an offset, to
+			 * distinguish a mapping of the page at PA 0 from none.
+			 */
+			phys_addr = iommu_iova_to_phys(domain, addr + 1);
  			if (!phys_addr) {
  				map_size += pg_size;
  				continue;