Re: [PATCH v2 3/7] iommu/rockchip: support virtual iommu slave device
HI,
On 2016年06月10日 14:22, Tomasz Figa wrote:
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng 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
---
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] [] dump_stack+0xb4/0xf0
[3.095222] [] rk_iommu_map+0x50/0x4b8
[3.095229] [] iommu_map+0xd8/0x2fc
[3.095237] [] iommu_dma_map_page+0xe4/0x114
[3.095245] [] __iommu_map_page+0x68/0xb4
[3.095251] [] rk_iommu_map+0x120/0x4b8
[3.095258] [] iommu_map+0xd8/0x2fc
[3.095265] [] default_iommu_map_sg+0xa4/0xf8
[3.095273] [] iommu_dma_alloc+0x290/0x2e8
[3.095281] [] __iommu_alloc_attrs+0xbc/0x22c
[3.095287] [] 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
Re: [PATCH v2 3/7] iommu/rockchip: support virtual iommu slave device
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng 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
> ---
> 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)?
> +
> + 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.
Best regards,
Tomasz
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 3/7] iommu/rockchip: support virtual iommu slave device
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.
Signed-off-by: Shunqian Zheng
---
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)
+
+ 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;
+ if (RK_IOMMU_IS_VIRTUAL(iommu)) {
+ dev_dbg(dev, "Master with virtual iommu detached from
domain\n");
return;
+ }
spin_lock_irqsave(&rk_domain->iommus_lock, flags);
list_del_init(&iommu->node);
@@ -862,8 +869,6 @@ static void rk_iommu_detach_device(struct iommu_domain
*domain,
devm_free_irq(iommu->dev, iommu->irq, iommu);
- iommu->domain = NULL;
-
dev_dbg(dev, "Detached from iommu domain\n");
}
@@ -1034,6 +1039,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct rk_iommu *iommu;
struct resource *res;
+ int num_res = pdev->num_resources;
int i;
iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
@@ -1043,12 +1049,19 @@ static int rk_iommu_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, iommu);
iommu->dev = dev;
iommu->num_mmu = 0;
- iommu->bases = devm_kzalloc(dev, sizeof(*iommu->bases) * iommu->num_mmu,
+
+ if (!num_res) {
+ iommu->bases = NULL;
+ dev_info(dev, "this is a virtual iommu\n");
+ return 0;
+ }
+
+ iommu->bases = devm_kzalloc(dev, sizeof(*iommu->bases) * num_res,
GFP_KERNEL);
if (!iommu->bases)
return -ENOMEM;
- for (i = 0; i < pdev->num_resources; i++) {
+ for (i = 0; i < num_res; i++) {
res = platform_get_resource(pdev, IORESOURCE_MEM, i);
if (!res)
continue;
--
1.9.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
