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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.08.2020 15:42, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> 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
> a stack variable and generally makes the call sites tidier.

I appreciate the intent of tidier code, but I wonder whether this
flushing doesn't go a little too far: There's a need to flush in
general when multiple pages were to be (un)mapped, and there was
at least partial success. Hence e.g. in the order == 0 case I
don't see why any flushing would be needed. Granted errors aren't
commonly expected, but anyway.

> --- 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) )

Both here and in the unmap path, did you get the return value
of iommu_iotlb_flush_all() the wrong way round (i.e. isn't there
a missing ! )?

Jan