Hi, Robin
On Wed, Aug 23, 2017 at 1:05 PM, Robin Murphy <robin.mur...@arm.com> wrote:
> 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.
Agree.
>
> 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
>>>
>>
>>
>>
>
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
