| Message ID | 20161019233553.10506.72700.sendpatchset@little-apple (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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);
--- 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) +{ + 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); + } + + 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. */ - 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; - if (!mmu->mapping) - return IRQ_NONE; + spin_lock_irqsave(&mmu->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 = 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);