[PATCH v2 5/8] iommu/io-pgtable-arm: Support lockless operation

[linu.cherian@cavium.com (Linu Cherian)]

Robin,
Was trying to understand the new changes. Had few questions on 
arm_lpae_install_table. 

On Thu Jun 22, 2017 at 04:53:54PM +0100, Robin Murphy wrote:
> For parallel I/O with multiple concurrent threads servicing the same
> device (or devices, if several share a domain), serialising page table
> updates becomes a massive bottleneck. On reflection, though, we don't
> strictly need to do that - for valid IOMMU API usage, there are in fact
> only two races that we need to guard against: multiple map requests for
> different blocks within the same region, when the intermediate-level
> table for that region does not yet exist; and multiple unmaps of
> different parts of the same block entry. Both of those are fairly easily
> solved by using a cmpxchg to install the new table, such that if we then
> find that someone else's table got there first, we can simply free ours
> and continue.
> 
> Make the requisite changes such that we can withstand being called
> without the caller maintaining a lock. In theory, this opens up a few
> corners in which wildly misbehaving callers making nonsensical
> overlapping requests might lead to crashes instead of just unpredictable
> results, but correct code really does not deserve to pay a significant
> performance cost for the sake of masking bugs in theoretical broken code.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v2:
>  - Fix barriers in install_table
>  - Make a few more PTE accesses with {READ,WRITE}_ONCE() just in case
>  - Minor cosmetics
> 
>  drivers/iommu/io-pgtable-arm.c | 72 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 57 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 6334f51912ea..52700fa958c2 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -20,6 +20,7 @@
>  
>  #define pr_fmt(fmt)	"arm-lpae io-pgtable: " fmt
>  
> +#include <linux/atomic.h>
>  #include <linux/iommu.h>
>  #include <linux/kernel.h>
>  #include <linux/sizes.h>
> @@ -99,6 +100,8 @@
>  #define ARM_LPAE_PTE_ATTR_HI_MASK	(((arm_lpae_iopte)6) << 52)
>  #define ARM_LPAE_PTE_ATTR_MASK		(ARM_LPAE_PTE_ATTR_LO_MASK |	\
>  					 ARM_LPAE_PTE_ATTR_HI_MASK)
> +/* Software bit for solving coherency races */
> +#define ARM_LPAE_PTE_SW_SYNC		(((arm_lpae_iopte)1) << 55)
>  
>  /* Stage-1 PTE */
>  #define ARM_LPAE_PTE_AP_UNPRIV		(((arm_lpae_iopte)1) << 6)
> @@ -249,15 +252,20 @@ static void __arm_lpae_free_pages(void *pages, size_t size,
>  	free_pages_exact(pages, size);
>  }
>  
> +static void __arm_lpae_sync_pte(arm_lpae_iopte *ptep,
> +				struct io_pgtable_cfg *cfg)
> +{
> +	dma_sync_single_for_device(cfg->iommu_dev, __arm_lpae_dma_addr(ptep),
> +				   sizeof(*ptep), DMA_TO_DEVICE);
> +}
> +
>  static void __arm_lpae_set_pte(arm_lpae_iopte *ptep, arm_lpae_iopte pte,
>  			       struct io_pgtable_cfg *cfg)
>  {
>  	*ptep = pte;
>  
>  	if (!(cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA))
> -		dma_sync_single_for_device(cfg->iommu_dev,
> -					   __arm_lpae_dma_addr(ptep),
> -					   sizeof(pte), DMA_TO_DEVICE);
> +		__arm_lpae_sync_pte(ptep, cfg);
>  }
>  
>  static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
> @@ -314,16 +322,30 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
>  
>  static arm_lpae_iopte arm_lpae_install_table(arm_lpae_iopte *table,
>  					     arm_lpae_iopte *ptep,
> +					     arm_lpae_iopte curr,
>  					     struct io_pgtable_cfg *cfg)
>  {
> -	arm_lpae_iopte new;
> +	arm_lpae_iopte old, new;
>  
>  	new = __pa(table) | ARM_LPAE_PTE_TYPE_TABLE;
>  	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
>  		new |= ARM_LPAE_PTE_NSTABLE;
>  
> -	__arm_lpae_set_pte(ptep, new, cfg);
> -	return new;
> +	/* Ensure the table itself is visible before its PTE can be */
> +	wmb();

Could you please give more hints on why this is required.


> +
> +	old = cmpxchg64_relaxed(ptep, curr, new);
> +
> +	if ((cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA) ||
> +	    (old & ARM_LPAE_PTE_SW_SYNC))
> +		return old;
> +
> +	/* Even if it's not ours, there's no point waiting; just kick it */
> +	__arm_lpae_sync_pte(ptep, cfg);
> +	if (old == curr)
> +		WRITE_ONCE(*ptep, new | ARM_LPAE_PTE_SW_SYNC);

How is it ensured that the cache operations are completed before we flag them with
	ARM_LPAE_PTE_SW_SYNC. The previous version had a wmb() after the sync operation.


> +
> +	return old;
>  }
>


Thanks.  

-- 
Linu cherian