When updating a DTE, amd_iommu_setup_domain_device checks if the update had been
non-atomic (i.e rc > 0) and eventually throws a warning but since [1], rc can
no longer be positive, making this check never taken.

[1] x86/iommu: remove non-CX16 logic from DMA remapping
    https://gitlab.com/xen-project/xen/-/commit/3fc44151d83d3d63320036bcf06634dfbebe1ff3

Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/drivers/passthrough/amd/iommu_map.c     |  4 +---
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 18 ------------------
 2 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index dde393645a..07f405ed63 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -157,9 +157,7 @@ static void set_iommu_ptes_present(unsigned long pt_mfn,
 /*
  * This function returns
  * - -errno for errors,
- * - 0 for a successful update, atomic when necessary
- * - 1 for a successful but non-atomic update, which may need to be warned
- *   about by the caller.
+ * - 0 for a successful update
  */
 int amd_iommu_set_root_page_table(struct amd_iommu_dte *dte,
                                   uint64_t root_ptr, uint16_t domain_id,
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index d00697edb3..409752ffc8 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -225,24 +225,6 @@ static int __must_check amd_iommu_setup_domain_device(
             spin_unlock_irqrestore(&iommu->lock, flags);
             return rc;
         }
-        if ( rc &&
-             domain != pdev->domain &&
-             /*
-              * By non-atomically updating the DTE's domain ID field last,
-              * during a short window in time TLB entries with the old domain
-              * ID but the new page tables may have been inserted.  This could
-              * affect I/O of other devices using this same (old) domain ID.
-              * Such updating therefore is not a problem if this was the only
-              * device associated with the old domain ID.  Diverting I/O of any
-              * of a dying domain's devices to the quarantine page tables is
-              * intended anyway.
-              */
-             !pdev->domain->is_dying &&
-             pdev->domain != dom_io &&
-             (any_pdev_behind_iommu(pdev->domain, pdev, iommu) ||
-              pdev->phantom_stride) )
-            AMD_IOMMU_WARN(" %pp: reassignment may cause %pd data corruption\n",
-                           &PCI_SBDF(pdev->seg, bus, devfn), pdev->domain);
 
         /*
          * Check remaining settings are still in place from an earlier call
-- 
2.49.0



Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

On Thu, May 22, 2025 at 03:44:12PM +0000, Teddy Astie wrote:
> When updating a DTE, amd_iommu_setup_domain_device checks if the update had been
> non-atomic (i.e rc > 0) and eventually throws a warning but since [1], rc can
> no longer be positive, making this check never taken.
> 
> [1] x86/iommu: remove non-CX16 logic from DMA remapping
>     https://gitlab.com/xen-project/xen/-/commit/3fc44151d83d3d63320036bcf06634dfbebe1ff3

I would avoid putting links to commits, and just reference the commit
by hash:

"When updating a DTE, amd_iommu_setup_domain_device() would check if
the update had been non-atomic (i.e rc > 0) and throw a warning if
such non-atomic update could be dangerous.  However since commit
3fc44151d83d, rc can no longer be positive, making this branch
unreachable code.

No functional change intended."

> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
>  xen/drivers/passthrough/amd/iommu_map.c     |  4 +---
>  xen/drivers/passthrough/amd/pci_amd_iommu.c | 18 ------------------
>  2 files changed, 1 insertion(+), 21 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
> index dde393645a..07f405ed63 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -157,9 +157,7 @@ static void set_iommu_ptes_present(unsigned long pt_mfn,
>  /*
>   * This function returns
>   * - -errno for errors,
> - * - 0 for a successful update, atomic when necessary
> - * - 1 for a successful but non-atomic update, which may need to be warned
> - *   about by the caller.
> + * - 0 for a successful update

I think you can remove the comment completely.  Returning -errno or 0
is the expected behavior.  We just add those comments when functions
diverge from the classic -errno/0 return codes.

>   */
>  int amd_iommu_set_root_page_table(struct amd_iommu_dte *dte,
>                                    uint64_t root_ptr, uint16_t domain_id,
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index d00697edb3..409752ffc8 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -225,24 +225,6 @@ static int __must_check amd_iommu_setup_domain_device(
>              spin_unlock_irqrestore(&iommu->lock, flags);
>              return rc;
>          }

You might want to also adjust the previous if condition (out of
context here) so it's if ( rc ) rather than rc < 0.

Thanks, Roger.