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: [email protected] >> Fixes: 37fa32de70 ("s390x/pci: Honor DMA limits set by vfio") >> Signed-off-by: Omar Elghoul <[email protected]> >> --- >> 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 <[email protected]> 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
