Hi, Laurent On Wed, Aug 23, 2017 at 4:46 PM, Laurent Pinchart <[email protected]> wrote: > Hi Oleksandr, > > On Wednesday, 23 August 2017 14:58:47 EEST Oleksandr Tyshchenko wrote: >> On Wed, Aug 23, 2017 at 1:05 PM, Robin Murphy wrote: >> > On 23/08/17 10:36, Oleksandr Tyshchenko wrote: >> >> On Wed, Aug 23, 2017 at 12:25 AM, Laurent Pinchart wrote: >> >>> On Monday, 21 August 2017 15:40:41 EEST Oleksandr Tyshchenko wrote: >> >>>> From: Oleksandr Tyshchenko <[email protected]> >> >>>> >> >>>> In ipmmu_domain_init_context() we are trying to allocate context and >> >>>> if allocation 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". >> >>>> >> >>>> So, set context_id to non-existent value (IPMMU_CTX_MAX) before calling >> >>>> free_io_pgtable_ops() and check it for a valid value (< IPMMU_CTX_MAX) >> >>>> before calling ipmmu_tlb_invalidate(). >> >>>> >> >>>> Signed-off-by: Oleksandr Tyshchenko <[email protected]> >> >>>> --- >> >>>> >> >>>> drivers/iommu/ipmmu-vmsa.c | 4 ++++ >> >>>> 1 file changed, 4 insertions(+) >> >>>> >> >>>> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c >> >>>> index 2a38aa1..5b226c0 100644 >> >>>> --- a/drivers/iommu/ipmmu-vmsa.c >> >>>> +++ b/drivers/iommu/ipmmu-vmsa.c >> >>>> @@ -303,6 +303,9 @@ static void ipmmu_tlb_flush_all(void *cookie) >> >>>> { >> >>>> struct ipmmu_vmsa_domain *domain = cookie; >> >>>> >> >>>> + if (domain->context_id >= IPMMU_CTX_MAX) >> >>>> + return; >> >>>> + >> >>>> ipmmu_tlb_invalidate(domain); >> >>>> } >> >>>> >> >>>> @@ -380,6 +383,7 @@ static int ipmmu_domain_init_context(struct >> >>>> ipmmu_vmsa_domain *domain) >> >>>> */ >> >>>> ret = ipmmu_domain_allocate_context(domain->mmu, domain); >> >>>> if (ret == IPMMU_CTX_MAX) { >> >>>> + domain->context_id = IPMMU_CTX_MAX; >> >>> >> >>> Wouldn't it make more sense to allocate the pgtable ops after >> >>> initializing the context (moving the ipmmu_domain_allocate_context() >> >>> call to the very end of the function) ? That way we would be less >> >>> dependent on changes to pgtable ops init/cleanup code that could >> >>> require the context to be set up. >> >> >> >> Why not. But, not sure about the very end of the function. Since for >> >> writing some HW registers down the function (IMTTLBR0/IMTTUBR0, >> >> IMMAIR0) we need to have what pgtable ops sets up for us (ttbr, mair). >> >> What about to just swap alloc_io_pgtable_ops() and >> >> ipmmu_domain_allocate_context()? >> > >> > This looks a lot more reasonable - reserving a free context is both >> > quicker and more likely to fail (due to limited hardware resources) than >> > setting up a pagetable, so it makes a lot of sense to do that before >> > anything else. >> >> Agree. > > That looks good to me too. In general I prefer initializing everything needed > by the error path before calling anything that could trigger that error path, > instead of initializing variables to magic values that mean part of the > cleanup should be skipped. Make sense.
> > Will you send a v2 ? Yes. > > -- > Regards, > > Laurent Pinchart > -- Regards, Oleksandr Tyshchenko _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
