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

Laurent Pinchart
2016-11-11T00:46:29Z

Hi Magnus,

Thank you for the patch.

On Thursday 20 Oct 2016 08:35:53 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> 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 <damm+renesas@opensource.se>
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> ---
>
> 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 <linux/bitmap.h>
> #include <linux/delay.h>
> #include <linux/dma-mapping.h>
> #include <linux/err.h>
> @@ -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(&mmu->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(&mmu->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(&mmu->lock, flags);
> +
> + clear_bit(context_id, mmu->ctx);
> + mmu->domains[context_id] = NULL;
> +
> + spin_unlock_irqrestore(&mmu->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;

Nitpicking again I like to arrange variable declarations by decreasing line
length when there's no reason not to :-)

> - if (!mmu->mapping)
> - return IRQ_NONE;
> + spin_lock_irqsave(&mmu->lock, flags);
> +
> + /*
> + * Check interrupts for all active contexts.
> + */

This comment holds on a single line too.

With all these small comments addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + 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 = to_vmsa_domain(io_domain);
> + spin_unlock_irqrestore(&mmu->lock, flags);
>
> - return ipmmu_domain_irq(domain);
> + return status;
> }
>
> /*
> ---------------------------------------------------------------------------
> -- @@ -774,6 +828,8 @@ static int ipmmu_probe(struct platform_d
>
> mmu->dev = &pdev->dev;
> mmu->num_utlbs = 32;
> + spin_lock_init(&mmu->lock);
> + bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
>
> /* Map I/O memory and request IRQ. */
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

--
Regards,

Laurent Pinchart