Re: [PATCH v6 02/07] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Thursday 20 Oct 2016 08:35:53 Magnus Damm wrote:
> From: Magnus Damm 
> 
> Introduce a bitmap for context handing and convert the
> interrupt routine to handle all registered contexts.
> 
> At this point the number of contexts are still limited.
> 
> Also remove the use of the ARM specific mapping variable
> from ipmmu_irq() to allow compile on ARM64.
> 
> Signed-off-by: Magnus Damm 
> Reviewed-by: Joerg Roedel 
> ---
> 
>  Changes since V5:
>  - None
> 
>  Changes since V4:
>  - None
> 
>  Changes since V3:
>  - None
> 
>  Changes since V2: (Thanks again to Laurent!)
>  - Introduce a spinlock together with the bitmap and domain array.
>  - Break out code into separate functions for alloc and free.
>  - Perform free after (instead of before) configuring hardware registers.
>  - Use the spinlock to protect the domain array in the interrupt handler.
> 
>  Changes since V1: (Thanks to Laurent for feedback!)
>  - Use simple find_first_zero()/set_bit()/clear_bit() for context
> management. - For allocation rely on spinlock held when calling
> ipmmu_domain_init_context() - For test/free use atomic bitops
>  - Return IRQ_HANDLED if any of the contexts generated interrupts
> 
>  drivers/iommu/ipmmu-vmsa.c |   76 +++--
>  1 file changed, 66 insertions(+), 10 deletions(-)
> 
> --- 0004/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c   2016-09-20 21:48:23.770607110 +0900
> @@ -8,6 +8,7 @@
>   * the Free Software Foundation; version 2 of the License.
>   */
> 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -26,12 +27,17 @@
> 
>  #include "io-pgtable.h"
> 
> +#define IPMMU_CTX_MAX 1
> +
>  struct ipmmu_vmsa_device {
>   struct device *dev;
>   void __iomem *base;
>   struct list_head list;
> 
>   unsigned int num_utlbs;
> + spinlock_t lock;/* Protects ctx and domains[] 
*/
> + DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> + struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> 
>   struct dma_iommu_mapping *mapping;
>  };
> @@ -293,9 +299,29 @@ static struct iommu_gather_ops ipmmu_gat
>   * Domain/Context Management
>   */
> 
> +static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu,
> +  struct ipmmu_vmsa_domain *domain)

Nitpicking, I'd name this ipmmu_domain_alloc_context() as the driver uses the 
alloc abbreviation already.

> +{
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(>lock, flags);
> +
> + ret = find_first_zero_bit(mmu->ctx, IPMMU_CTX_MAX);
> + if (ret != IPMMU_CTX_MAX) {
> + mmu->domains[ret] = domain;
> + set_bit(ret, mmu->ctx);
> + }

How about returning a negative error code on error instead of IPMMU_CTX_MAX ? 
I think it would make the API clearer, avoiding the need to think about 
special error handling for this function.

Having said that, I find that the init/alloc and destroy/free function names 
don't carry a very clear semantic. Given the size of the alloc and free 
functions, how about inlining them in their single caller ?

> +
> + spin_unlock_irqrestore(>lock, flags);
> +
> + return ret;
> +}
> +
>  static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>  {
>   u64 ttbr;
> + int ret;
> 
>   /*
>* Allocate the page table operations.
> @@ -325,10 +351,15 @@ static int ipmmu_domain_init_context(str
>   return -EINVAL;
> 
>   /*
> -  * TODO: When adding support for multiple contexts, find an unused
> -  * context.
> +  * Find an unused context.
>*/

The comment now holds on one line.

> - domain->context_id = 0;
> + ret = ipmmu_domain_allocate_context(domain->mmu, domain);
> + if (ret == IPMMU_CTX_MAX) {
> + free_io_pgtable_ops(domain->iop);
> + return -EBUSY;
> + }
> +
> + domain->context_id = ret;
> 
>   /* TTBR0 */
>   ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
> @@ -370,6 +401,19 @@ static int ipmmu_domain_init_context(str
>   return 0;
>  }
> 
> +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> +   unsigned int context_id)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(>lock, flags);
> +
> + clear_bit(context_id, mmu->ctx);
> + mmu->domains[context_id] = NULL;
> +
> + spin_unlock_irqrestore(>lock, flags);
> +}
> +
>  static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
>  {
>   /*
> @@ -380,6 +424,7 @@ static void ipmmu_domain_destroy_context
>*/
>   ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH);
>   ipmmu_tlb_sync(domain);
> + ipmmu_domain_free_context(domain->mmu, domain->context_id);
>  }
> 
>  /*
> ---
> -- @@ -437,16 

[PATCH v6 02/07] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context

[Magnus Damm]

From: Magnus Damm 

Introduce a bitmap for context handing and convert the
interrupt routine to handle all registered contexts.

At this point the number of contexts are still limited.

Also remove the use of the ARM specific mapping variable
from ipmmu_irq() to allow compile on ARM64.

Signed-off-by: Magnus Damm 
Reviewed-by: Joerg Roedel 
---

 Changes since V5:
 - None

 Changes since V4:
 - None

 Changes since V3:
 - None

 Changes since V2: (Thanks again to Laurent!)
 - Introduce a spinlock together with the bitmap and domain array.
 - Break out code into separate functions for alloc and free.
 - Perform free after (instead of before) configuring hardware registers.
 - Use the spinlock to protect the domain array in the interrupt handler.

 Changes since V1: (Thanks to Laurent for feedback!)
 - Use simple find_first_zero()/set_bit()/clear_bit() for context management.
 - For allocation rely on spinlock held when calling ipmmu_domain_init_context()
 - For test/free use atomic bitops
 - Return IRQ_HANDLED if any of the contexts generated interrupts

 drivers/iommu/ipmmu-vmsa.c |   76 ++--
 1 file changed, 66 insertions(+), 10 deletions(-)

--- 0004/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:48:23.770607110 +0900
@@ -8,6 +8,7 @@
  * the Free Software Foundation; version 2 of the License.
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -26,12 +27,17 @@
 
 #include "io-pgtable.h"
 
+#define IPMMU_CTX_MAX 1
+
 struct ipmmu_vmsa_device {
struct device *dev;
void __iomem *base;
struct list_head list;
 
unsigned int num_utlbs;
+   spinlock_t lock;/* Protects ctx and domains[] */
+   DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
+   struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
 
struct dma_iommu_mapping *mapping;
 };
@@ -293,9 +299,29 @@ static struct iommu_gather_ops ipmmu_gat
  * Domain/Context Management
  */
 
+static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu,
+struct ipmmu_vmsa_domain *domain)
+{
+   unsigned long flags;
+   int ret;
+
+   spin_lock_irqsave(>lock, flags);
+
+   ret = find_first_zero_bit(mmu->ctx, IPMMU_CTX_MAX);
+   if (ret != IPMMU_CTX_MAX) {
+   mmu->domains[ret] = domain;
+   set_bit(ret, mmu->ctx);
+   }
+
+   spin_unlock_irqrestore(>lock, flags);
+
+   return ret;
+}
+
 static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
 {
u64 ttbr;
+   int ret;
 
/*
 * Allocate the page table operations.
@@ -325,10 +351,15 @@ static int ipmmu_domain_init_context(str
return -EINVAL;
 
/*
-* TODO: When adding support for multiple contexts, find an unused
-* context.
+* Find an unused context.
 */
-   domain->context_id = 0;
+   ret = ipmmu_domain_allocate_context(domain->mmu, domain);
+   if (ret == IPMMU_CTX_MAX) {
+   free_io_pgtable_ops(domain->iop);
+   return -EBUSY;
+   }
+
+   domain->context_id = ret;
 
/* TTBR0 */
ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
@@ -370,6 +401,19 @@ static int ipmmu_domain_init_context(str
return 0;
 }
 
+static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
+ unsigned int context_id)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(>lock, flags);
+
+   clear_bit(context_id, mmu->ctx);
+   mmu->domains[context_id] = NULL;
+
+   spin_unlock_irqrestore(>lock, flags);
+}
+
 static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
 {
/*
@@ -380,6 +424,7 @@ static void ipmmu_domain_destroy_context
 */
ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH);
ipmmu_tlb_sync(domain);
+   ipmmu_domain_free_context(domain->mmu, domain->context_id);
 }
 
 /* 
-
@@ -437,16 +482,25 @@ static irqreturn_t ipmmu_domain_irq(stru
 static irqreturn_t ipmmu_irq(int irq, void *dev)
 {
struct ipmmu_vmsa_device *mmu = dev;
-   struct iommu_domain *io_domain;
-   struct ipmmu_vmsa_domain *domain;
+   irqreturn_t status = IRQ_NONE;
+   unsigned int i;
+   unsigned long flags;
 
-   if (!mmu->mapping)
-   return IRQ_NONE;
+   spin_lock_irqsave(>lock, flags);
+
+   /*
+* Check interrupts for all active contexts.
+*/
+   for (i = 0; i < IPMMU_CTX_MAX; i++) {
+   if (!mmu->domains[i])
+   continue;
+   if (ipmmu_domain_irq(mmu->domains[i]) == IRQ_HANDLED)
+   status = IRQ_HANDLED;
+   }
 
-   io_domain = mmu->mapping->domain;
-   domain =