Re: [PATCH] iommu/amd: Remove dead non-atomic update checking

["Roger Pau Monné" <roger.pau@citrix.com>]

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.