[v1] s390x/pci: fix DMA slot leak on I/O TLB entry replacement

[PATCH v1] s390x/pci: fix DMA slot leak on I/O TLB entry replacement

Posted by Omar Elghoul 2 weeks, 2 days ago

When s390_pci_update_iotlb() does a mapping update for an IOVA that has
an already-existing TLB entry with different permissions or translated
address, it unmaps then remaps it. However, at the end of the map path,
it unconditionally decrements the available DMA slot counter without a
corresponding increment in the intermediate unmap branch.

This causes the DMA slot count to be decremented on every remapping of
an active IOVA, leading to a permanent DMA slot leak. This remapping
without a prior invalidation and sync is not seen today in well-behaved
guests but is allowed by the architecture. Fix it by only decrementing
available DMA slots when inserting a brand new mapping.

Cc: qemu-stable@nongnu.org
Fixes: 37fa32de70 ("s390x/pci: Honor DMA limits set by vfio")
Signed-off-by: Omar Elghoul <oelghoul@linux.ibm.com>
---
 hw/s390x/s390-pci-inst.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index b7de23c7d2..5840675aba 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -665,6 +665,9 @@ static uint32_t s390_pci_update_iotlb(S390PCIIOMMU *iommu,
             memory_region_notify_iommu(&iommu->iommu_mr, 0, event);
             event.type = IOMMU_NOTIFIER_MAP;
             event.entry.perm = entry->perm;
+        } else {
+            /* only new mappings consume DMA slots */
+            dec_dma_avail(iommu);
         }
 
         cache = g_new(S390IOTLBEntry, 1);
@@ -673,7 +676,6 @@ static uint32_t s390_pci_update_iotlb(S390PCIIOMMU *iommu,
         cache->len = TARGET_PAGE_SIZE;
         cache->perm = entry->perm;
         g_hash_table_replace(iommu->iotlb, &cache->iova, cache);
-        dec_dma_avail(iommu);
     }
 
     /*
-- 
2.55.0

Re: [PATCH v1] s390x/pci: fix DMA slot leak on I/O TLB entry replacement

Posted by Farhan Ali 1 week, 4 days ago

On 9/10/2026 6:24 AM, Omar Elghoul wrote:
> When s390_pci_update_iotlb() does a mapping update for an IOVA that has
> an already-existing TLB entry with different permissions or translated
> address, it unmaps then remaps it. However, at the end of the map path,
> it unconditionally decrements the available DMA slot counter without a
> corresponding increment in the intermediate unmap branch.
>
> This causes the DMA slot count to be decremented on every remapping of
> an active IOVA, leading to a permanent DMA slot leak. This remapping
> without a prior invalidation and sync is not seen today in well-behaved
> guests but is allowed by the architecture. Fix it by only decrementing
> available DMA slots when inserting a brand new mapping.
>
> Cc: qemu-stable@nongnu.org
> Fixes: 37fa32de70 ("s390x/pci: Honor DMA limits set by vfio")
> Signed-off-by: Omar Elghoul <oelghoul@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-inst.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index b7de23c7d2..5840675aba 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -665,6 +665,9 @@ static uint32_t s390_pci_update_iotlb(S390PCIIOMMU *iommu,
>               memory_region_notify_iommu(&iommu->iommu_mr, 0, event);
>               event.type = IOMMU_NOTIFIER_MAP;
>               event.entry.perm = entry->perm;
> +        } else {
> +            /* only new mappings consume DMA slots */
> +            dec_dma_avail(iommu);
>           }
To me the else statement here makes it a little more confusing. I think 
it would be easier to read, if we increment above when do the unmap in 
the intermediate step. That way we associate incrementing the available 
entries with unmap and decrementing it with map operation. But I guess 
this avoids an additional function call for the same result.

Reviewed-by: Farhan Ali <alifm@linux.ibm.com>

>   
>           cache = g_new(S390IOTLBEntry, 1);
> @@ -673,7 +676,6 @@ static uint32_t s390_pci_update_iotlb(S390PCIIOMMU *iommu,
>           cache->len = TARGET_PAGE_SIZE;
>           cache->perm = entry->perm;
>           g_hash_table_replace(iommu->iotlb, &cache->iova, cache);
> -        dec_dma_avail(iommu);
>       }
>   
>       /*

Re: [PATCH v1] s390x/pci: fix DMA slot leak on I/O TLB entry replacement

Posted by Matthew Rosato 1 week, 2 days ago

On 9/14/26 5:27 PM, Farhan Ali wrote:
> 
> On 9/10/2026 6:24 AM, Omar Elghoul wrote:
>> When s390_pci_update_iotlb() does a mapping update for an IOVA that has
>> an already-existing TLB entry with different permissions or translated
>> address, it unmaps then remaps it. However, at the end of the map path,
>> it unconditionally decrements the available DMA slot counter without a
>> corresponding increment in the intermediate unmap branch.
>>
>> This causes the DMA slot count to be decremented on every remapping of
>> an active IOVA, leading to a permanent DMA slot leak. This remapping
>> without a prior invalidation and sync is not seen today in well-behaved
>> guests but is allowed by the architecture. Fix it by only decrementing
>> available DMA slots when inserting a brand new mapping.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: 37fa32de70 ("s390x/pci: Honor DMA limits set by vfio")
>> Signed-off-by: Omar Elghoul <oelghoul@linux.ibm.com>
>> ---
>>   hw/s390x/s390-pci-inst.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index b7de23c7d2..5840675aba 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -665,6 +665,9 @@ static uint32_t s390_pci_update_iotlb(S390PCIIOMMU
>> *iommu,
>>               memory_region_notify_iommu(&iommu->iommu_mr, 0, event);
>>               event.type = IOMMU_NOTIFIER_MAP;
>>               event.entry.perm = entry->perm;
>> +        } else {
>> +            /* only new mappings consume DMA slots */
>> +            dec_dma_avail(iommu);
>>           }
> To me the else statement here makes it a little more confusing. I think
> it would be easier to read, if we increment above when do the unmap in
> the intermediate step. That way we associate incrementing the available
> entries with unmap and decrementing it with map operation. But I guess
> this avoids an additional function call for the same result.

So first, for the code itself:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

As to the readability vs adding an intermediate increment -- maybe if
you add another small comment it would help with readability?

Something like:

if (cache) {
    /* valid->valid transitions re-use a DMA slot */
    ...
} else {
    /* invalid->valid transitions consume a new DMA slot */
    ...
}

Note I did tweak your wording a little bit.

You could even add a comment in the straight unmap case if you think it
would also be helpful e.g.

/* valid->invalid transitions will release a DMA slot */

I think this makes it very clear when the counter should be going
up/down/neutral.

Thanks,
Matt

Re: [PATCH v1] s390x/pci: fix DMA slot leak on I/O TLB entry replacement

Posted by Omar Elghoul 1 week, 2 days ago

On 9/16/26 9:45 PM, Matthew Rosato wrote:
> On 9/14/26 5:27 PM, Farhan Ali wrote:
>>
>> On 9/10/2026 6:24 AM, Omar Elghoul wrote:
>>> When s390_pci_update_iotlb() does a mapping update for an IOVA that has
>>> an already-existing TLB entry with different permissions or translated
>>> address, it unmaps then remaps it. However, at the end of the map path,
>>> it unconditionally decrements the available DMA slot counter without a
>>> corresponding increment in the intermediate unmap branch.
>>>
>>> This causes the DMA slot count to be decremented on every remapping of
>>> an active IOVA, leading to a permanent DMA slot leak. This remapping
>>> without a prior invalidation and sync is not seen today in well-behaved
>>> guests but is allowed by the architecture. Fix it by only decrementing
>>> available DMA slots when inserting a brand new mapping.
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Fixes: 37fa32de70 ("s390x/pci: Honor DMA limits set by vfio")
>>> Signed-off-by: Omar Elghoul <oelghoul@linux.ibm.com>
>>> ---
>>>    hw/s390x/s390-pci-inst.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>> index b7de23c7d2..5840675aba 100644
>>> --- a/hw/s390x/s390-pci-inst.c
>>> +++ b/hw/s390x/s390-pci-inst.c
>>> @@ -665,6 +665,9 @@ static uint32_t s390_pci_update_iotlb(S390PCIIOMMU
>>> *iommu,
>>>                memory_region_notify_iommu(&iommu->iommu_mr, 0, event);
>>>                event.type = IOMMU_NOTIFIER_MAP;
>>>                event.entry.perm = entry->perm;
>>> +        } else {
>>> +            /* only new mappings consume DMA slots */
>>> +            dec_dma_avail(iommu);
>>>            }
>> To me the else statement here makes it a little more confusing. I think
>> it would be easier to read, if we increment above when do the unmap in
>> the intermediate step. That way we associate incrementing the available
>> entries with unmap and decrementing it with map operation. But I guess
>> this avoids an additional function call for the same result.
> 
> So first, for the code itself:
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> As to the readability vs adding an intermediate increment -- maybe if
> you add another small comment it would help with readability?
> 
> Something like:
> 
> if (cache) {
>      /* valid->valid transitions re-use a DMA slot */
>      ...
> } else {
>      /* invalid->valid transitions consume a new DMA slot */
>      ...
> }

That's a great suggestion, I will post a v2 with more comments.

Thanks