Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] iommu/amd: seqcount write section spans sleepable DTE flush, so readers can livelock on PREEMPT_RT --
commit 7634aee1eec1a71c7f83c233557bd0d61b816f56 Author: Denis V. Lunev <[email protected]> iommu/amd/pgtbl: fix DMA faults on IOVAs mapped while the table grows This commit moves amd_iommu_update_and_flush_device_table() inside the seqcount write section in increase_address_space(). Mappers that call alloc_pte() or fetch_pte() on another CPU then spin until the DTEs point at the new root. This is meant to stop IOVAs from being mapped under the new root before the device uses it. > diff --git a/drivers/iommu/amd/io_pgtable.c b/drivers/iommu/amd/io_pgtable.c > --- a/drivers/iommu/amd/io_pgtable.c > +++ b/drivers/iommu/amd/io_pgtable.c [ ... ] > @@ -131,12 +131,15 @@ static bool increase_address_space(struct > amd_io_pgtable *pgtable, > > *pte = PM_LEVEL_PDE(pgtable->mode, iommu_virt_to_phys(pgtable->root)); > > + /* > + * Keep the seqcount odd until the DTEs point at the new root, or a > + * concurrent alloc_pte() maps an IOVA the device cannot translate yet. > + */ > write_seqcount_begin(&pgtable->seqcount); > pgtable->root = pte; > pgtable->mode += 1; > - write_seqcount_end(&pgtable->seqcount); > - > amd_iommu_update_and_flush_device_table(domain); > + write_seqcount_end(&pgtable->seqcount); [Severity: Medium] Can readers livelock on a PREEMPT_RT build here? pgtable->seqcount is a plain seqcount_t, so write_seqcount_begin() does not disable preemption itself. On non-RT builds, the enclosing spin_lock_irqsave(&domain->lock) disables preemption and IRQs. The odd window therefore runs atomically, and readers on other CPUs spin only for the bounded flush time. On PREEMPT_RT, spinlock_t is a sleepable lock and spin_lock_irqsave() does not disable preemption or IRQs. The writer stays preemptible while the seqcount is odd. Before this patch the odd window covered two stores. Now it also covers amd_iommu_update_and_flush_device_table(): amd_iommu_update_and_flush_device_table() set_dte_entry() /* takes spinlock_t dev_data->dte_lock */ clone_aliases() device_flush_dte() /* sends commands under raw iommu->lock */ domain_flush_complete() /* polls for completion */ The readers cannot sleep: alloc_pte() read_seqcount_begin(&pgtable->seqcount) fetch_pte() read_seqcount_begin(&pgtable->seqcount) If the writer is preempted or blocked on dte_lock, a higher-priority task on the same CPU that maps or unmaps through the same domain spins in read_seqcount_begin() with no priority inheritance. fetch_pte() is also reached from the unmap, iova_to_phys and dirty-read paths. The existing write_seqcount_begin() under spin_lock_irqsave() already conflicts with the seqcount preemption requirement on RT. This patch widens that window from two stores to include blocking operations. Is there a way to publish the new root and mode only after the DTEs are updated, or to otherwise avoid holding the seqcount odd across the DTE flush? > > pte = NULL; > ret = true; -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=1 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
