RE: [PATCH v4 06/14] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail

["Tian, Kevin" <kevin.tian@intel.com>]

> From: Paul Durrant
> Sent: Tuesday, August 4, 2020 9:42 PM
> 
> From: Paul Durrant <pdurrant@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 <pdurrant@amazon.com>
> ---
> Cc: Jan Beulich <jbeulich@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
>