Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] iommu/amd: iommu_completion_wait() can wait on a cmd_sem_val with no queued CWAIT --
commit 17e0a27d76ec180494393cc226f8033b509119b2 Author: Guanghui Feng <[email protected]> ms/iommu/amd: Wait for completion instead of returning early in iommu_completion_wait() This is a backport of an upstream fix. iommu_completion_wait() now takes iommu->lock before testing need_sync. When need_sync is false, it waits on the last allocated sequence number (cmd_sem_val) instead of returning early. > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > index 373596fa07c0..91cf277f1293 100644 > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -1408,11 +1408,23 @@ static int iommu_completion_wait(struct amd_iommu > *iommu) > int ret; > u64 data; > > - if (!iommu->need_sync) > - return 0; > - > raw_spin_lock_irqsave(&iommu->lock, flags); > > + if (!iommu->need_sync) { > + /* > + * No command has been queued since the last completion-wait. > + * A concurrent CPU may have already queued that CWAIT and > + * cleared need_sync; need_sync == false only means a covering > + * CWAIT is queued, not that all prior commands have completed. > + * Wait for the last allocated sequence number so that any > + * command queued before this call (possibly on another CPU) > + * is guaranteed to have completed before returning. > + */ > + data = iommu->cmd_sem_val; [Severity: Low] Can cmd_sem_val here be a number for which no CWAIT was ever queued? get_cmdsem_val() increments cmd_sem_val before anything is queued. In iommu_flush_irt_and_complete() the sequence is: data = get_cmdsem_val(iommu); build_completion_wait(&cmd2, iommu, data); ret = __iommu_queue_command_sync(iommu, &cmd, true); if (ret) goto out_err; If the first __iommu_queue_command_sync() fails with "Command buffer timeout", it returns -EIO before copy_cmd_to_buffer() and before "iommu->need_sync = sync". So need_sync keeps its previous value, which is false if the last queued command was a CWAIT. No CWAIT carrying number N is queued. A later iommu_completion_wait() then sees need_sync == false, takes this new branch, reads data = cmd_sem_val (N), and calls wait_on_sem(iommu, N). *cmd_sem can never reach N, so the wait spins for LOOP_TIMEOUT iterations of udelay(1) and returns -EIO with "Completion-Wait loop timed out". Before the patch the same state returned 0 immediately. Callers such as domain_flush_complete() would now stall for the full timeout while holding domain->lock. The state lasts until another command sets need_sync and a CWAIT with a higher sequence number completes. This needs a command-buffer-full timeout, so the IOMMU is already misbehaving. The failure of the second queue call in iommu_flush_irt_and_complete() is not affected, because the first call has already set need_sync = true. The commit message says "Since need_sync == false implies no sync command was queued after the last CWAIT, that CWAIT is FIFO-ordered after every not-yet-completed command, so waiting for its sequence number guarantees all prior commands ... have completed." Is that accurate, given that cmd_sem_val can be ahead of the last CWAIT that was actually queued? > + raw_spin_unlock_irqrestore(&iommu->lock, flags); > + return wait_on_sem(iommu, data); > + } > + > data = get_cmdsem_val(iommu); > build_completion_wait(&cmd, iommu, data); > -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=3 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
