Hi Laurent, Robin On Tue, Aug 29, 2017 at 4:01 PM, Laurent Pinchart <[email protected]> wrote: > Hi Oleksandr, > > Thank you for the patch. > > On Wednesday, 23 August 2017 17:31:42 EEST Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <[email protected]> >> >> 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 <[email protected]> >> CC: Robin Murphy <[email protected]> >> CC: Laurent Pinchart <[email protected]> >> CC: Joerg Roedel <[email protected]> > > Reviewed-by: Laurent Pinchart <[email protected]> > >> --- >> 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) >> { >> /* > > > -- > Regards, > > Laurent Pinchart >
Thank you for the review. Shall I resend patch with added R-bs? -- Regards, Oleksandr Tyshchenko _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
