Re: [PATCH v2 3/7] iommu/rockchip: support virtual iommu slave device

[Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>]

HI,

On 2016年06月10日 14:22, Tomasz Figa wrote:
> Hi,
>
> On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng <zhengsq@rock-chips.com> wrote:
>> An virtual master device like DRM need to attach to iommu
>> domain to share the domain with VOP(the one with actual
>> iommu slave). We currently check the group is NULL to indicate
>> a virtual master, which is not true since we decide to use
>> the common iommu api to attach device in DRM.
>>
>> With this patch, we can probe a virtual iommu device and
>> allow the DRM attaching to it. The virtual iommu is needed also
>> because we want convert to use DMA API for map/unmap, cache flush,
>> so that DRM buffer alloc still work even VOP is disabled.
> I'm not really convinced that this is a good idea. This will require
> creating fake devices in the system and generally looks really hacky.
> Please see my alternative proposal inline.
>
>> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
>> ---
>>   drivers/iommu/rockchip-iommu.c | 37 +++++++++++++++++++++++++------------
>>   1 file changed, 25 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
>> index 3c16ec3..d6c3051 100644
>> --- a/drivers/iommu/rockchip-iommu.c
>> +++ b/drivers/iommu/rockchip-iommu.c
>> @@ -75,6 +75,11 @@
>>
>>   #define IOMMU_REG_POLL_COUNT_FAST 1000
>>
>> +/* A virtual iommu in device-tree registered without reg or
>> + * interrupts, so the num_mmu is zero.
>> + */
>> +#define RK_IOMMU_IS_VIRTUAL(iommu) (iommu->num_mmu == 0)
>> +
>>   struct rk_iommu_domain {
>>          struct list_head iommus;
>>          u32 *dt; /* page directory table */
>> @@ -789,13 +794,13 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
>>          int ret, i;
>>          phys_addr_t dte_addr;
>>
>> -       /*
>> -        * Allow 'virtual devices' (e.g., drm) to attach to domain.
>> -        * Such a device does not belong to an iommu group.
>> -        */
>>          iommu = rk_iommu_from_dev(dev);
>> -       if (!iommu)
> Could we instead allocate such virtual rk_iommu struct here (dev could
> be used as iommu->dev for logging purposes and a fake group could be
> allocated too)?
Thanks for your advice.
The "iommu->dev" not only for logging, but mainly for dma_*().
If allocating virtual rk_iommu struct here and set iommu->dev to dev of DRM,
dma_map_single() (or dma_alloc_coherent() as you suggested in patch 6/7) 
may not
act as expected.
Because when DRM attached to iommu, the dma_ops of DRM is the 
rk_iommu_ops, so
dma_map_single() will get endless loop to call rk_iommu_map() like the 
stack shows:

[    3.095215] [<ffffffc0004bc374>] dump_stack+0xb4/0xf0
[    3.095222] [<ffffffc0005a6048>] rk_iommu_map+0x50/0x4b8
[    3.095229] [<ffffffc0005a130c>] iommu_map+0xd8/0x2fc
[    3.095237] [<ffffffc0005a3634>] iommu_dma_map_page+0xe4/0x114
[    3.095245] [<ffffffc0005a4258>] __iommu_map_page+0x68/0xb4
[    3.095251] [<ffffffc0005a6118>] rk_iommu_map+0x120/0x4b8
[    3.095258] [<ffffffc0005a130c>] iommu_map+0xd8/0x2fc
[    3.095265] [<ffffffc0005a15d4>] default_iommu_map_sg+0xa4/0xf8
[    3.095273] [<ffffffc0005a3474>] iommu_dma_alloc+0x290/0x2e8
[    3.095281] [<ffffffc0005a3cbc>] __iommu_alloc_attrs+0xbc/0x22c
[    3.095287] [<ffffffc0005e0794>] rockchip_gem_create_object+0x118/0x168

But, if the virtual iommu registered in device-tree, the iommu->dev is 
set to platform dma_ops,
that is swiotlb_dma_ops for arm64, which can avoid the issue above.

This is why the virtual iommu added in device-tree. Yes, it's hacky, but 
I don't have
another idea to solve the problem.

>
>> +
>> +       iommu->domain = domain;
>> +       if (RK_IOMMU_IS_VIRTUAL(iommu)) {
>> +               dev_dbg(dev, "Attach virtual device to iommu domain\n");
>>                  return 0;
>> +       }
>>
>>          ret = rk_iommu_enable_stall(iommu);
>>          if (ret)
>> @@ -805,7 +810,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
>>          if (ret)
>>                  return ret;
>>
>> -       iommu->domain = domain;
>>
>>          ret = devm_request_irq(iommu->dev, iommu->irq, rk_iommu_irq,
>>                                 IRQF_SHARED, dev_name(dev), iommu);
>> @@ -842,10 +846,13 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
>>          unsigned long flags;
>>          int i;
>>
>> -       /* Allow 'virtual devices' (eg drm) to detach from domain */
>>          iommu = rk_iommu_from_dev(dev);
>> -       if (!iommu)
>> +
>> +       iommu->domain = NULL;
> I don't think it's a good idea to set the domain to NULL before
> disabling the real IOMMU. It might still trigger an interrupt at this
> point and things won't behave correctly. I guess the original line
> could be left as is and simply same assignment added under the if
> below.
Correct, for the virtual one, it's safe to set "iommu->domain = NULL",
while for the real iommu, better to keep as it's before.

Thank you very much,
Shunqian
>
> Best regards,
> Tomasz
>
>
>


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu