Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] iommu/amd: cmd_sem seq allocated outside lock, signed wait_on_sem() unsafe - [Low] iommu/amd: wait_on_sem() semantics change to signed >= is not in commit message --
commit 1ce3255ad1464c0ff15e279b0d0ff6234294a548 Author: Ankit Soni <[email protected]> ms/iommu/amd: move wait_on_sem() out of spinlock This commit moves the wait_on_sem() busy-wait in iommu_completion_wait() and iommu_flush_irt_and_complete() out of the iommu->lock critical section. It also changes the wait condition in wait_on_sem() from an equality test to a signed comparison. > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c [ ... ] > @@ -1161,7 +1161,12 @@ static int wait_on_sem(struct amd_iommu *iommu, u64 > data) > { > int i = 0; > > - while (*iommu->cmd_sem != data && i < LOOP_TIMEOUT) { > + /* > + * cmd_sem holds a monotonically non-decreasing completion sequence > + * number. > + */ > + while ((__s64)(READ_ONCE(*iommu->cmd_sem) - data) < 0 && > + i < LOOP_TIMEOUT) { > udelay(1); > i += 1; > } [Severity: Low] The commit message says only that the wait is moved out of the spinlock and that wait_on_sem() "only polls the hardware-updated cmd_sem and does not require iommu->lock". Is that a complete description of the change? The patch also changes the wait condition in wait_on_sem() from an exact match to a signed comparison, adds READ_ONCE(), and restructures the error path in iommu_flush_irt_and_complete(). None of this is mentioned. The new comment states "cmd_sem holds a monotonically non-decreasing completion sequence number". Is that guaranteed by this patch alone? The sequence number is still allocated with atomic64_inc_return() before iommu->lock is taken. That invariant only becomes true with the follow-up patch, which allocates the number under the lock. The later patches in the series do not correct this commit message. [ ... ] > @@ -1406,14 +1411,13 @@ static int iommu_completion_wait(struct amd_iommu > *iommu) > raw_spin_lock_irqsave(&iommu->lock, flags); > > ret = __iommu_queue_command_sync(iommu, &cmd, false); > + raw_spin_unlock_irqrestore(&iommu->lock, flags); > + > if (ret) > - goto out_unlock; > + return ret; > > ret = wait_on_sem(iommu, data); > > -out_unlock: > - raw_spin_unlock_irqrestore(&iommu->lock, flags); > - > return ret; > } [Severity: Medium] Can wait_on_sem() return before this CPU's commands have executed? In iommu_completion_wait(), the sequence number is allocated before the lock is taken: data = atomic64_inc_return(&iommu->cmd_sem_val); build_completion_wait(&cmd, iommu, data); raw_spin_lock_irqsave(&iommu->lock, flags); ret = __iommu_queue_command_sync(iommu, &cmd, false); raw_spin_unlock_irqrestore(&iommu->lock, flags); Queue order can therefore differ from sequence order. Consider two CPUs running this concurrently, for example with iommu.strict=1: CPU A: data = 5, then is delayed before taking iommu->lock CPU B: data = 6, takes the lock, queues COMPLETION_WAIT(6), unlocks CPU A: takes the lock, queues its commands and COMPLETION_WAIT(5) The hardware processes the queue in order, so *cmd_sem becomes 6 and later 5. This contradicts the new comment in wait_on_sem() about monotonicity. CPU A then sees 6 and computes (6 - 5) >= 0 in the new signed test, so it stops waiting. Its own invalidations and COMPLETION_WAIT(5) have not executed yet. Before this patch, the lock held across queue and wait, plus the equality test, prevented this early return. CPU B can also poll only after the hardware has written 5, so (5 - 6) < 0. It spins for LOOP_TIMEOUT and then returns -EIO with the "Completion-Wait loop timed out" alert. The equality test had the same weakness in this case, so that part is not new. The follow-up patch "ms/iommu/amd: serialize sequence allocation under concurrent TLB invalidations" moves the allocation under iommu->lock, so this looks like a regression only in this intermediate commit, but it affects bisection. [ ... ] > @@ -3094,13 +3098,18 @@ static void iommu_flush_irt_and_complete(struct > amd_iommu *iommu, u16 devid) > raw_spin_lock_irqsave(&iommu->lock, flags); > ret = __iommu_queue_command_sync(iommu, &cmd, true); > if (ret) > - goto out; > + goto out_err; > ret = __iommu_queue_command_sync(iommu, &cmd2, false); > if (ret) > - goto out; > + goto out_err; > + raw_spin_unlock_irqrestore(&iommu->lock, flags); > + > wait_on_sem(iommu, data); > -out: > + return; > + > +out_err: > raw_spin_unlock_irqrestore(&iommu->lock, flags); > + return; > } [Severity: Medium] Does iommu_flush_irt_and_complete() have the same exposure? Its sequence number is also allocated outside the lock: data = atomic64_inc_return(&iommu->cmd_sem_val); build_completion_wait(&cmd2, iommu, data); raw_spin_lock_irqsave(&iommu->lock, flags); ... raw_spin_unlock_irqrestore(&iommu->lock, flags); wait_on_sem(iommu, data); With the signed comparison in wait_on_sem(), a CPU holding a lower number can stop waiting once another CPU's higher number has been written, before its IRTE invalidation has executed. The return value of wait_on_sem() is also ignored here, so a timeout would not be reported. -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=1 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
