Hi Magnus,
Thank you for the patch.
On Tuesday 15 March 2016 13:21:55 Magnus Damm wrote:
> From: Magnus Damm <damm+rene...@opensource.se>
>
> Introduce a bitmap for context handing and convert the
> interrupt routine to go 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+rene...@opensource.se>
> ---
>
> 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()
I'm afraid this is still racy. That spinlock belongs to the domain, and we
have multiple domains. You need to add a new lock in the ipmmu_vmsa_device
structure.
> - For test/free use atomic bitops
> - Return IRQ_HANDLED if any of the contexts generated interrupts
>
> drivers/iommu/ipmmu-vmsa.c | 47 +++++++++++++++++++++++++++++------------
> 1 file changed, 35 insertions(+), 12 deletions(-)
>
> --- 0003/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c 2016-03-15 12:42:18.940513000 +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,16 @@
>
> #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;
> + DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> + struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
>
> struct dma_iommu_mapping *mapping;
> };
> @@ -296,6 +301,7 @@ static struct iommu_gather_ops ipmmu_gat
> static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> {
> u64 ttbr;
> + int ret;
>
> /*
> * Allocate the page table operations.
> @@ -325,10 +331,17 @@ 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 = find_first_zero_bit(domain->mmu->ctx, IPMMU_CTX_MAX);
> + if (ret == IPMMU_CTX_MAX) {
> + free_io_pgtable_ops(domain->iop);
> + return -EBUSY;
> + }
> +
> + domain->context_id = ret;
> + domain->mmu->domains[ret] = domain;
> + set_bit(ret, domain->mmu->ctx);
>
> /* TTBR0 */
> ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
> @@ -372,6 +385,8 @@ static int ipmmu_domain_init_context(str
>
> static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
> {
> + clear_bit(domain->context_id, domain->mmu->ctx);
> +
> /*
> * Disable the context. Flush the TLB as required when modifying the
> * context registers.
> @@ -389,10 +404,15 @@ static void ipmmu_domain_destroy_context
> static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
> {
> const u32 err_mask = IMSTR_MHIT | IMSTR_ABORT | IMSTR_PF | IMSTR_TF;
> - struct ipmmu_vmsa_device *mmu = domain->mmu;
> + struct ipmmu_vmsa_device *mmu;
> u32 status;
> u32 iova;
>
> + if (!domain)
> + return IRQ_NONE;
Can this happen, as you test for the corresponding context bit before calling
this function ?
> +
> + mmu = domain->mmu;
> +
> status = ipmmu_ctx_read(domain, IMSTR);
> if (!(status & err_mask))
> return IRQ_NONE;
> @@ -437,16 +457,18 @@ 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;
> -
> - if (!mmu->mapping)
> - return IRQ_NONE;
> + irqreturn_t status = IRQ_NONE;
> + unsigned int i;
>
> - io_domain = mmu->mapping->domain;
> - domain = to_vmsa_domain(io_domain);
> + /* Check interrupts for all active contexts */
Nitpicking, could you add a period at the end of the sentence to match the
existing comment style ?
> + for (i = 0; i < IPMMU_CTX_MAX; i++) {
> + if (!test_bit(i, mmu->ctx))
test_bit() isn't atomic. Let's use explicit locking in every location where
the contexts bitmap is accessed in a racy way.
> + continue;
> + if (ipmmu_domain_irq(mmu->domains[i]) == IRQ_HANDLED)
> + status = IRQ_HANDLED;
> + }
>
> - return ipmmu_domain_irq(domain);
> + return status;
> }
>
> /* ------------------------------------------------------------------------
> @@ -774,6 +796,7 @@ static int ipmmu_probe(struct platform_d
>
> mmu->dev = &pdev->dev;
> mmu->num_utlbs = 32;
> + 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
