> From: Paul Durrant
> Sent: Tuesday, August 4, 2020 9:42 PM
>
> From: Paul Durrant <pdurr...@amazon.com>
>
> This patch adds a full I/O TLB flush to the error paths of iommu_map() and
> iommu_unmap().
>
> Without this change callers need constructs such as:
>
> rc = iommu_map/unmap(...)
> err = iommu_flush(...)
> if ( !rc )
> rc = err;
>
> With this change, it can be simplified to:
>
> rc = iommu_map/unmap(...)
> if ( !rc )
> rc = iommu_flush(...)
>
> because, if the map or unmap fails the flush will be unnecessary. This saves
this statement is different from change in iommu_map...
> a stack variable and generally makes the call sites tidier.
>
> Signed-off-by: Paul Durrant <pdurr...@amazon.com>
> ---
> Cc: Jan Beulich <jbeul...@suse.com>
>
> v2:
> - New in v2
> ---
> xen/drivers/passthrough/iommu.c | 28 ++++++++++++----------------
> 1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index 660dc5deb2..e2c0193a09 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -274,6 +274,10 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t
> mfn,
> break;
> }
>
> + /* Something went wrong so flush everything and clear flush flags */
> + if ( unlikely(rc) && iommu_iotlb_flush_all(d, *flush_flags) )
> + flush_flags = 0;
> +
... earlier you said flush is unnecessary if map fails. But here actually you
still need to flush everything so it's just sort of moving error-path flush
within the map function?
Thanks
Kevin
> return rc;
> }
>
> @@ -283,14 +287,8 @@ int iommu_legacy_map(struct domain *d, dfn_t dfn,
> mfn_t mfn,
> unsigned int flush_flags = 0;
> int rc = iommu_map(d, dfn, mfn, page_order, flags, &flush_flags);
>
> - if ( !this_cpu(iommu_dont_flush_iotlb) )
> - {
> - int err = iommu_iotlb_flush(d, dfn, (1u << page_order),
> - flush_flags);
> -
> - if ( !rc )
> - rc = err;
> - }
> + if ( !this_cpu(iommu_dont_flush_iotlb) && !rc )
> + rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags);
>
> return rc;
> }
> @@ -330,6 +328,10 @@ int iommu_unmap(struct domain *d, dfn_t dfn,
> unsigned int page_order,
> }
> }
>
> + /* Something went wrong so flush everything and clear flush flags */
> + if ( unlikely(rc) && iommu_iotlb_flush_all(d, *flush_flags) )
> + flush_flags = 0;
> +
> return rc;
> }
>
> @@ -338,14 +340,8 @@ int iommu_legacy_unmap(struct domain *d, dfn_t
> dfn, unsigned int page_order)
> unsigned int flush_flags = 0;
> int rc = iommu_unmap(d, dfn, page_order, &flush_flags);
>
> - if ( !this_cpu(iommu_dont_flush_iotlb) )
> - {
> - int err = iommu_iotlb_flush(d, dfn, (1u << page_order),
> - flush_flags);
> -
> - if ( !rc )
> - rc = err;
> - }
> + if ( !this_cpu(iommu_dont_flush_iotlb) && ! rc )
> + rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags);
>
> return rc;
> }
> --
> 2.20.1
>
