Re: [PATCH 02/13] iommu/io-pgtable-arm: Remove redundant call to io_pgtable_tlb_sync()

[Robin Murphy <robin.murphy@arm.com>]

On 14/08/2019 18:56, Will Deacon wrote:
> Commit b6b65ca20bc9 ("iommu/io-pgtable-arm: Add support for non-strict
> mode") added an unconditional call to io_pgtable_tlb_sync() immediately
> after the case where we replace a block entry with a table entry during
> an unmap() call. This is redundant, since the IOMMU API will call
> iommu_tlb_sync() on this path and the patch in question mentions this:
> 
>   | To save having to reason about it too much, make sure the invalidation
>   | in arm_lpae_split_blk_unmap() just performs its own unconditional sync
>   | to minimise the window in which we're technically violating the break-
>   | before-make requirement on a live mapping. This might work out redundant
>   | with an outer-level sync for strict unmaps, but we'll never be splitting
>   | blocks on a DMA fastpath anyway.
> 
> However, this sync gets in the way of deferred TLB invalidation for leaf
> entries and is at best a questionable, unproven hack. Remove it.

Hey, that's my questionable, unproven hack! :P

It's not entirely clear to me how this gets in the way though - AFAICS 
the intent of tlb_flush_leaf exactly matches the desired operation here, 
so couldn't these just wait to be converted in patch #8?

In principle the concern is that if the caller splits a block with 
iommu_unmap_fast(), there's no guarantee of seeing an iommu_tlb_sync() 
before returning to the caller, and thus there's the potential to run 
into a TLB conflict on a subsequent access even if the endpoint was 
"good" and didn't make any accesses *during* the unmap call.

Robin.

> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>   drivers/iommu/io-pgtable-arm-v7s.c | 1 -
>   drivers/iommu/io-pgtable-arm.c     | 1 -
>   2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
> index 0fc8dfab2abf..a62733c6a632 100644
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -587,7 +587,6 @@ static size_t arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,
>   	}
>   
>   	io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
> -	io_pgtable_tlb_sync(&data->iop);
>   	return size;
>   }
>   
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 161a7d56264d..0d6633921c1e 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -583,7 +583,6 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
>   		tablep = iopte_deref(pte, data);
>   	} else if (unmap_idx >= 0) {
>   		io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
> -		io_pgtable_tlb_sync(&data->iop);
>   		return size;
>   	}
>   
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu