On 23/08/17 10:36, Oleksandr Tyshchenko wrote:
> Hi, Laurent.
>
> On Wed, Aug 23, 2017 at 12:25 AM, Laurent Pinchart
> <laurent.pinch...@ideasonboard.com> wrote:
>> Hi Oleksandr,
>>
>> Thank you for the patch.
>>
>> On Monday, 21 August 2017 15:40:41 EEST Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
>>>
>>> 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 <oleksandr_tyshche...@epam.com>
>>> ---
>>> 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.
Robin.
> ...
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 2a38aa1..90af1c7 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -370,22 +370,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);
> ...
>
>>
>>> free_io_pgtable_ops(domain->iop);
>>> return -EBUSY;
>>> }
>>
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>
>
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
