From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oleksandr Tyshchenko Subject: Re: [PATCH v2] iommu/ipmmu-vmsa: Rereserving a free context before setting up a pagetable Date: Mon, 28 Aug 2017 16:23:42 +0300 Message-ID: References: <1503498702-27996-1-git-send-email-olekstysh@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1503498702-27996-1-git-send-email-olekstysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Cc: laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org, geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org, damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org, Will Deacon , Oleksandr Tyshchenko , Laurent Pinchart List-Id: iommu@lists.linux-foundation.org Hi, all. Any comments? On Wed, Aug 23, 2017 at 5:31 PM, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko > > Reserving a free context is both quicker and more likely to fail > (due to limited hardware resources) than setting up a pagetable. > What is more the pagetable init/cleanup code could require > the context to be set up. > > Signed-off-by: Oleksandr Tyshchenko > CC: Robin Murphy > CC: Laurent Pinchart > CC: Joerg Roedel > > --- > This patch fixes a bug during rollback logic: > In ipmmu_domain_init_context() we are trying to find an unused context > and if operation fails we will call free_io_pgtable_ops(), > but "domain->context_id" hasn't been initialized yet (likely it is 0 > because of kzalloc). Having the following call stack: > free_io_pgtable_ops() -> io_pgtable_tlb_flush_all() -> > ipmmu_tlb_flush_all() -> ipmmu_tlb_invalidate() > we will get a mistaken cache flush for a context pointed by > uninitialized "domain->context_id". > > > v1 here: > https://lists.linuxfoundation.org/pipermail/iommu/2017-August/023857.html > --- > drivers/iommu/ipmmu-vmsa.c | 42 +++++++++++++++++++++--------------------- > 1 file changed, 21 insertions(+), 21 deletions(-) > > diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c > index 2a38aa1..382f387 100644 > --- a/drivers/iommu/ipmmu-vmsa.c > +++ b/drivers/iommu/ipmmu-vmsa.c > @@ -341,6 +341,19 @@ static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu, > return ret; > } > > +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 int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain) > { > u64 ttbr; > @@ -370,22 +383,22 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain) > */ > domain->cfg.iommu_dev = domain->mmu->dev; > > - domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg, > - domain); > - if (!domain->iop) > - return -EINVAL; > - > /* > * Find an unused context. > */ > ret = ipmmu_domain_allocate_context(domain->mmu, domain); > - if (ret == IPMMU_CTX_MAX) { > - free_io_pgtable_ops(domain->iop); > + if (ret == IPMMU_CTX_MAX) > return -EBUSY; > - } > > domain->context_id = ret; > > + domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg, > + domain); > + if (!domain->iop) { > + ipmmu_domain_free_context(domain->mmu, domain->context_id); > + return -EINVAL; > + } > + > /* TTBR0 */ > ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0]; > ipmmu_ctx_write(domain, IMTTLBR0, ttbr); > @@ -426,19 +439,6 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain) > 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) > { > /* > -- > 2.7.4 > -- Regards, Oleksandr Tyshchenko