[GIT PULL] iommu/arm-smmu: fixes for 3.14

[GIT PULL] iommu/arm-smmu: fixes for 3.14

[Will Deacon]

Hi Joerg,

Please pull the following arm-smmu fixes for 3.14.

The brief summary is:

  - Andreas Herrmann took the driver for a run with a real SATA
    controller, which caused the new mutex-based locking to explode
    since we require mappings in atomic context

  - Yifan fixed an issue with the page table creation, which then caused
    breakages with the way in which we flush descriptors out to the
    table walker

  - I ran the driver on a system where the SMMU is hooked into a
    coherent interconnect for table walks, and noticed a shareability
    mismatch between the CPU and the SMMU

These issues are all fixed here and have been tested on both arm and
arm64 based systems.

Cheers,

Will

--->8

The following changes since commit b28a960c42fcd9cfc987441fa6d1c1a471f0f9ed:

  Linux 3.14-rc2 (2014-02-09 18:15:47 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git for-joerg/arm-smmu/fixes

for you to fetch changes up to d123cf82d339c5cc4ffe2a481e0caa23a501d4ac:

  iommu/arm-smmu: fix compilation issue when !CONFIG_ARM_AMBA (2014-02-10 17:02:27 +0000)

----------------------------------------------------------------
Will Deacon (4):
      iommu/arm-smmu: really fix page table locking
      iommu/arm-smmu: fix table flushing during initial allocations
      iommu/arm-smmu: set CBARn.BPSHCFG to NSH for s1-s2-bypass contexts
      iommu/arm-smmu: fix compilation issue when !CONFIG_ARM_AMBA

Yifan Zhang (1):
      iommu/arm-smmu: fix pud/pmd entry fill sequence

 drivers/iommu/arm-smmu.c | 103 ++++++++++++++++++++++++++++-------------------
 1 file changed, 61 insertions(+), 42 deletions(-)

Re: [GIT PULL] iommu/arm-smmu: fixes for 3.14

[Joerg Roedel]

On Mon, Feb 10, 2014 at 06:32:32PM +0000, Will Deacon wrote:
> The following changes since commit b28a960c42fcd9cfc987441fa6d1c1a471f0f9ed:
> 
>   Linux 3.14-rc2 (2014-02-09 18:15:47 -0800)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git for-joerg/arm-smmu/fixes
> 
> for you to fetch changes up to d123cf82d339c5cc4ffe2a481e0caa23a501d4ac:
> 
>   iommu/arm-smmu: fix compilation issue when !CONFIG_ARM_AMBA (2014-02-10 17:02:27 +0000)
> 
> ----------------------------------------------------------------
> Will Deacon (4):
>       iommu/arm-smmu: really fix page table locking
>       iommu/arm-smmu: fix table flushing during initial allocations
>       iommu/arm-smmu: set CBARn.BPSHCFG to NSH for s1-s2-bypass contexts
>       iommu/arm-smmu: fix compilation issue when !CONFIG_ARM_AMBA
> 
> Yifan Zhang (1):
>       iommu/arm-smmu: fix pud/pmd entry fill sequence
> 
>  drivers/iommu/arm-smmu.c | 103 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 61 insertions(+), 42 deletions(-)

Pulled, thanks Will.

Re: [GIT PULL] iommu/arm-smmu: fixes for 3.14

[Joerg Roedel]

On Mon, Feb 10, 2014 at 06:32:32PM +0000, Will Deacon wrote:
>   - Andreas Herrmann took the driver for a run with a real SATA
>     controller, which caused the new mutex-based locking to explode
>     since we require mappings in atomic context

I just did quick review of the changes. Was the SATA controller using
the IOMMU through the DMA-API? In this case you would need the IRQ-safe
spinlocks to avoid dead-locks because the DMA-API can be used from
IRQ-context.


	Joerg

Re: [GIT PULL] iommu/arm-smmu: fixes for 3.14

[Will Deacon]

Hi Joerg,

On Tue, Feb 18, 2014 at 05:50:27PM +0000, Joerg Roedel wrote:
> On Mon, Feb 10, 2014 at 06:32:32PM +0000, Will Deacon wrote:
> >   - Andreas Herrmann took the driver for a run with a real SATA
> >     controller, which caused the new mutex-based locking to explode
> >     since we require mappings in atomic context
> 
> I just did quick review of the changes. Was the SATA controller using
> the IOMMU through the DMA-API? In this case you would need the IRQ-safe
> spinlocks to avoid dead-locks because the DMA-API can be used from
> IRQ-context.

Unfortunately, I don't know and since Calxeda has ceased to be, Andreas is
no longer working on this (I suspect his email address also bounces).

I can send an additional patch using spin_*_irq{save,restore} for the
domain->lock if you like?

Will

Re: [GIT PULL] iommu/arm-smmu: fixes for 3.14

[Will Deacon]

On Tue, Feb 18, 2014 at 06:36:26PM +0000, Will Deacon wrote:
> On Tue, Feb 18, 2014 at 05:50:27PM +0000, Joerg Roedel wrote:
> > I just did quick review of the changes. Was the SATA controller using
> > the IOMMU through the DMA-API? In this case you would need the IRQ-safe
> > spinlocks to avoid dead-locks because the DMA-API can be used from
> > IRQ-context.
> 
> Unfortunately, I don't know and since Calxeda has ceased to be, Andreas is
> no longer working on this (I suspect his email address also bounces).
> 
> I can send an additional patch using spin_*_irq{save,restore} for the
> domain->lock if you like?

FWIW, here's a diff you could apply as a fixup (or I can send a new pull
request if you prefer). It's slightly messy because I had to rename a
parameter in the page table functions (s/flag/prot/).

What do you reckon?

Will

--->8

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 489cbe4c26aa..76ad84637a0f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1253,7 +1253,7 @@ static bool arm_smmu_pte_is_contiguous_range(unsigned long addr,
 
 static int arm_smmu_alloc_init_pte(struct arm_smmu_device *smmu, pmd_t *pmd,
 				   unsigned long addr, unsigned long end,
-				   unsigned long pfn, int flags, int stage)
+				   unsigned long pfn, int prot, int stage)
 {
 	pte_t *pte, *start;
 	pteval_t pteval = ARM_SMMU_PTE_PAGE | ARM_SMMU_PTE_AF | ARM_SMMU_PTE_XN;
@@ -1275,28 +1275,28 @@ static int arm_smmu_alloc_init_pte(struct arm_smmu_device *smmu, pmd_t *pmd,
 
 	if (stage == 1) {
 		pteval |= ARM_SMMU_PTE_AP_UNPRIV | ARM_SMMU_PTE_nG;
-		if (!(flags & IOMMU_WRITE) && (flags & IOMMU_READ))
+		if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
 			pteval |= ARM_SMMU_PTE_AP_RDONLY;
 
-		if (flags & IOMMU_CACHE)
+		if (prot & IOMMU_CACHE)
 			pteval |= (MAIR_ATTR_IDX_CACHE <<
 				   ARM_SMMU_PTE_ATTRINDX_SHIFT);
 	} else {
 		pteval |= ARM_SMMU_PTE_HAP_FAULT;
-		if (flags & IOMMU_READ)
+		if (prot & IOMMU_READ)
 			pteval |= ARM_SMMU_PTE_HAP_READ;
-		if (flags & IOMMU_WRITE)
+		if (prot & IOMMU_WRITE)
 			pteval |= ARM_SMMU_PTE_HAP_WRITE;
-		if (flags & IOMMU_CACHE)
+		if (prot & IOMMU_CACHE)
 			pteval |= ARM_SMMU_PTE_MEMATTR_OIWB;
 		else
 			pteval |= ARM_SMMU_PTE_MEMATTR_NC;
 	}
 
 	/* If no access, create a faulting entry to avoid TLB fills */
-	if (flags & IOMMU_EXEC)
+	if (prot & IOMMU_EXEC)
 		pteval &= ~ARM_SMMU_PTE_XN;
-	else if (!(flags & (IOMMU_READ | IOMMU_WRITE)))
+	else if (!(prot & (IOMMU_READ | IOMMU_WRITE)))
 		pteval &= ~ARM_SMMU_PTE_PAGE;
 
 	pteval |= ARM_SMMU_PTE_SH_IS;
@@ -1358,7 +1358,7 @@ static int arm_smmu_alloc_init_pte(struct arm_smmu_device *smmu, pmd_t *pmd,
 
 static int arm_smmu_alloc_init_pmd(struct arm_smmu_device *smmu, pud_t *pud,
 				   unsigned long addr, unsigned long end,
-				   phys_addr_t phys, int flags, int stage)
+				   phys_addr_t phys, int prot, int stage)
 {
 	int ret;
 	pmd_t *pmd;
@@ -1382,7 +1382,7 @@ static int arm_smmu_alloc_init_pmd(struct arm_smmu_device *smmu, pud_t *pud,
 	do {
 		next = pmd_addr_end(addr, end);
 		ret = arm_smmu_alloc_init_pte(smmu, pmd, addr, end, pfn,
-					      flags, stage);
+					      prot, stage);
 		phys += next - addr;
 	} while (pmd++, addr = next, addr < end);
 
@@ -1391,7 +1391,7 @@ static int arm_smmu_alloc_init_pmd(struct arm_smmu_device *smmu, pud_t *pud,
 
 static int arm_smmu_alloc_init_pud(struct arm_smmu_device *smmu, pgd_t *pgd,
 				   unsigned long addr, unsigned long end,
-				   phys_addr_t phys, int flags, int stage)
+				   phys_addr_t phys, int prot, int stage)
 {
 	int ret = 0;
 	pud_t *pud;
@@ -1415,7 +1415,7 @@ static int arm_smmu_alloc_init_pud(struct arm_smmu_device *smmu, pgd_t *pgd,
 	do {
 		next = pud_addr_end(addr, end);
 		ret = arm_smmu_alloc_init_pmd(smmu, pud, addr, next, phys,
-					      flags, stage);
+					      prot, stage);
 		phys += next - addr;
 	} while (pud++, addr = next, addr < end);
 
@@ -1424,10 +1424,10 @@ static int arm_smmu_alloc_init_pud(struct arm_smmu_device *smmu, pgd_t *pgd,
 
 static int arm_smmu_handle_mapping(struct arm_smmu_domain *smmu_domain,
 				   unsigned long iova, phys_addr_t paddr,
-				   size_t size, int flags)
+				   size_t size, int prot)
 {
 	int ret, stage;
-	unsigned long end;
+	unsigned long end, flags;
 	phys_addr_t input_mask, output_mask;
 	struct arm_smmu_cfg *root_cfg = &smmu_domain->root_cfg;
 	pgd_t *pgd = root_cfg->pgd;
@@ -1454,14 +1454,14 @@ static int arm_smmu_handle_mapping(struct arm_smmu_domain *smmu_domain,
 	if (paddr & ~output_mask)
 		return -ERANGE;
 
-	spin_lock(&smmu_domain->lock);
+	spin_lock_irqsave(&smmu_domain->lock, flags);
 	pgd += pgd_index(iova);
 	end = iova + size;
 	do {
 		unsigned long next = pgd_addr_end(iova, end);
 
 		ret = arm_smmu_alloc_init_pud(smmu, pgd, iova, next, paddr,
-					      flags, stage);
+					      prot, stage);
 		if (ret)
 			goto out_unlock;
 
@@ -1470,7 +1470,7 @@ static int arm_smmu_handle_mapping(struct arm_smmu_domain *smmu_domain,
 	} while (pgd++, iova != end);
 
 out_unlock:
-	spin_unlock(&smmu_domain->lock);
+	spin_unlock_irqrestore(&smmu_domain->lock, flags);
 
 	return ret;
 }

Re: [GIT PULL] iommu/arm-smmu: fixes for 3.14

[Joerg Roedel]

On Tue, Feb 18, 2014 at 07:21:37PM +0000, Will Deacon wrote:
> FWIW, here's a diff you could apply as a fixup (or I can send a new pull
> request if you prefer). It's slightly messy because I had to rename a
> parameter in the page table functions (s/flag/prot/).
> 
> What do you reckon?

Hmm, am I mistaken or do you only convert the mapping path? There is
another use of the smmu_domain->lock that has to be converted. See my
fix attached. Can the arm_smmu_devices_lock also be used in a DMA-API
path?
I also think that the parameter renaming should not be done in a fixup
patch, if you want it to be converted please include a renaming patch in
your updates for the next merge window.


	Joerg


>From e848d16b3129cc7234089d6917a0568b1b5f4acc Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Date: Thu, 20 Feb 2014 12:19:08 +0100
Subject: [PATCH] arm/smmu: Use irqsafe spinlock for domain lock

As the lock might be used through DMA-API which is allowed
in interrupt context.

Signed-off-by: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
---
 drivers/iommu/arm-smmu.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 6fe7922..1d9ab39 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1159,6 +1159,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	struct arm_smmu_domain *smmu_domain = domain->priv;
 	struct arm_smmu_device *device_smmu = dev->archdata.iommu;
 	struct arm_smmu_master *master;
+	unsigned long flags;
 
 	if (!device_smmu) {
 		dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
@@ -1169,7 +1170,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	 * Sanity check the domain. We don't currently support domains
 	 * that cross between different SMMU chains.
 	 */
-	spin_lock(&smmu_domain->lock);
+	spin_lock_irqsave(&smmu_domain->lock, flags);
 	if (!smmu_domain->leaf_smmu) {
 		/* Now that we have a master, we can finalise the domain */
 		ret = arm_smmu_init_domain_context(domain, dev);
@@ -1184,7 +1185,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 			dev_name(device_smmu->dev));
 		goto err_unlock;
 	}
-	spin_unlock(&smmu_domain->lock);
+	spin_unlock_irqrestore(&smmu_domain->lock, flags);
 
 	/* Looks ok, so add the device to the domain */
 	master = find_smmu_master(smmu_domain->leaf_smmu, dev->of_node);
@@ -1194,7 +1195,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return arm_smmu_domain_add_master(smmu_domain, master);
 
 err_unlock:
-	spin_unlock(&smmu_domain->lock);
+	spin_unlock_irqrestore(&smmu_domain->lock, flags);
 	return ret;
 }
 
@@ -1396,6 +1397,7 @@ static int arm_smmu_handle_mapping(struct arm_smmu_domain *smmu_domain,
 	struct arm_smmu_cfg *root_cfg = &smmu_domain->root_cfg;
 	pgd_t *pgd = root_cfg->pgd;
 	struct arm_smmu_device *smmu = root_cfg->smmu;
+	unsigned long irqflags;
 
 	if (root_cfg->cbar == CBAR_TYPE_S2_TRANS) {
 		stage = 2;
@@ -1418,7 +1420,7 @@ static int arm_smmu_handle_mapping(struct arm_smmu_domain *smmu_domain,
 	if (paddr & ~output_mask)
 		return -ERANGE;
 
-	spin_lock(&smmu_domain->lock);
+	spin_lock_irqsave(&smmu_domain->lock, irqflags);
 	pgd += pgd_index(iova);
 	end = iova + size;
 	do {
@@ -1434,7 +1436,7 @@ static int arm_smmu_handle_mapping(struct arm_smmu_domain *smmu_domain,
 	} while (pgd++, iova != end);
 
 out_unlock:
-	spin_unlock(&smmu_domain->lock);
+	spin_unlock_irqrestore(&smmu_domain->lock, irqflags);
 
 	return ret;
 }
-- 
1.7.9.5

Re: [GIT PULL] iommu/arm-smmu: fixes for 3.14

[Will Deacon]

Hi Joerg,

On Thu, Feb 20, 2014 at 11:29:19AM +0000, Joerg Roedel wrote:
> On Tue, Feb 18, 2014 at 07:21:37PM +0000, Will Deacon wrote:
> > FWIW, here's a diff you could apply as a fixup (or I can send a new pull
> > request if you prefer). It's slightly messy because I had to rename a
> > parameter in the page table functions (s/flag/prot/).
> > 
> > What do you reckon?
> 
> Hmm, am I mistaken or do you only convert the mapping path? There is
> another use of the smmu_domain->lock that has to be converted. See my
> fix attached. 

Good catch, they need updating too.

> Can the arm_smmu_devices_lock also be used in a DMA-API path?

I don't think so -- that lock is only taken when:

  - Probing the SMMU
  - Attaching a device to a domain (->attach_dev)
  - Adding or removing a device to/from the SMMU (->{add,remove}_device)

> I also think that the parameter renaming should not be done in a fixup
> patch, if you want it to be converted please include a renaming patch in
> your updates for the next merge window.

Sure, that's just a cosmetic thing anyway.
For your patch below,

  Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>

Cheers for sorting this out,

Will

> From e848d16b3129cc7234089d6917a0568b1b5f4acc Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> Date: Thu, 20 Feb 2014 12:19:08 +0100
> Subject: [PATCH] arm/smmu: Use irqsafe spinlock for domain lock
> 
> As the lock might be used through DMA-API which is allowed
> in interrupt context.
> 
> Signed-off-by: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 6fe7922..1d9ab39 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1159,6 +1159,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	struct arm_smmu_domain *smmu_domain = domain->priv;
>  	struct arm_smmu_device *device_smmu = dev->archdata.iommu;
>  	struct arm_smmu_master *master;
> +	unsigned long flags;
>  
>  	if (!device_smmu) {
>  		dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
> @@ -1169,7 +1170,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	 * Sanity check the domain. We don't currently support domains
>  	 * that cross between different SMMU chains.
>  	 */
> -	spin_lock(&smmu_domain->lock);
> +	spin_lock_irqsave(&smmu_domain->lock, flags);
>  	if (!smmu_domain->leaf_smmu) {
>  		/* Now that we have a master, we can finalise the domain */
>  		ret = arm_smmu_init_domain_context(domain, dev);
> @@ -1184,7 +1185,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  			dev_name(device_smmu->dev));
>  		goto err_unlock;
>  	}
> -	spin_unlock(&smmu_domain->lock);
> +	spin_unlock_irqrestore(&smmu_domain->lock, flags);
>  
>  	/* Looks ok, so add the device to the domain */
>  	master = find_smmu_master(smmu_domain->leaf_smmu, dev->of_node);
> @@ -1194,7 +1195,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	return arm_smmu_domain_add_master(smmu_domain, master);
>  
>  err_unlock:
> -	spin_unlock(&smmu_domain->lock);
> +	spin_unlock_irqrestore(&smmu_domain->lock, flags);
>  	return ret;
>  }
>  
> @@ -1396,6 +1397,7 @@ static int arm_smmu_handle_mapping(struct arm_smmu_domain *smmu_domain,
>  	struct arm_smmu_cfg *root_cfg = &smmu_domain->root_cfg;
>  	pgd_t *pgd = root_cfg->pgd;
>  	struct arm_smmu_device *smmu = root_cfg->smmu;
> +	unsigned long irqflags;
>  
>  	if (root_cfg->cbar == CBAR_TYPE_S2_TRANS) {
>  		stage = 2;
> @@ -1418,7 +1420,7 @@ static int arm_smmu_handle_mapping(struct arm_smmu_domain *smmu_domain,
>  	if (paddr & ~output_mask)
>  		return -ERANGE;
>  
> -	spin_lock(&smmu_domain->lock);
> +	spin_lock_irqsave(&smmu_domain->lock, irqflags);
>  	pgd += pgd_index(iova);
>  	end = iova + size;
>  	do {
> @@ -1434,7 +1436,7 @@ static int arm_smmu_handle_mapping(struct arm_smmu_domain *smmu_domain,
>  	} while (pgd++, iova != end);
>  
>  out_unlock:
> -	spin_unlock(&smmu_domain->lock);
> +	spin_unlock_irqrestore(&smmu_domain->lock, irqflags);
>  
>  	return ret;
>  }
> -- 
> 1.7.9.5
> 
> 
> 

Re: [GIT PULL] iommu/arm-smmu: fixes for 3.14

[Joerg Roedel]

Hi Will,

On Thu, Feb 20, 2014 at 11:51:27AM +0000, Will Deacon wrote:
> > Can the arm_smmu_devices_lock also be used in a DMA-API path?
> 
> I don't think so -- that lock is only taken when:
> 
>   - Probing the SMMU
>   - Attaching a device to a domain (->attach_dev)
>   - Adding or removing a device to/from the SMMU (->{add,remove}_device)

Okay, fine then.

> 
> > I also think that the parameter renaming should not be done in a fixup
> > patch, if you want it to be converted please include a renaming patch in
> > your updates for the next merge window.
> 
> Sure, that's just a cosmetic thing anyway.
> For your patch below,
> 
>   Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>

Thanks, I queued the patch with your Ack ontop of the fixes and will
send them out to Linus after a day in next or so.


Cheers,

	Joerg