[PATCH] of/device: add blacklist for iommu dma_ops

[PATCH] of/device: add blacklist for iommu dma_ops

[Rob Clark]

This solves a problem we see with drm/msm, caused by getting
iommu_dma_ops while we attach our own domain and manage it directly at
the iommu API level:

  [0000000000000038] user address but active_mm is swapper
  Internal error: Oops: 96000005 [#1] PREEMPT SMP
  Modules linked in:
  CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
  Hardware name: xxx (DT)
  Workqueue: events deferred_probe_work_func
  pstate: 80c00009 (Nzcv daif +PAN +UAO)
  pc : iommu_dma_map_sg+0x7c/0x2c8
  lr : iommu_dma_map_sg+0x40/0x2c8
  sp : ffffff80095eb4f0
  x29: ffffff80095eb4f0 x28: 0000000000000000
  x27: ffffffc0f9431578 x26: 0000000000000000
  x25: 00000000ffffffff x24: 0000000000000003
  x23: 0000000000000001 x22: ffffffc0fa9ac010
  x21: 0000000000000000 x20: ffffffc0fab40980
  x19: ffffffc0fab40980 x18: 0000000000000003
  x17: 00000000000001c4 x16: 0000000000000007
  x15: 000000000000000e x14: ffffffffffffffff
  x13: ffff000000000000 x12: 0000000000000028
  x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
  x9 : 0000000000000000 x8 : ffffffc0fab409a0
  x7 : 0000000000000000 x6 : 0000000000000002
  x5 : 0000000100000000 x4 : 0000000000000000
  x3 : 0000000000000001 x2 : 0000000000000002
  x1 : ffffffc0f9431578 x0 : 0000000000000000
  Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
  Call trace:
   iommu_dma_map_sg+0x7c/0x2c8
   __iommu_map_sg_attrs+0x70/0x84
   get_pages+0x170/0x1e8
   msm_gem_get_iova+0x8c/0x128
   _msm_gem_kernel_new+0x6c/0xc8
   msm_gem_kernel_new+0x4c/0x58
   dsi_tx_buf_alloc_6g+0x4c/0x8c
   msm_dsi_host_modeset_init+0xc8/0x108
   msm_dsi_modeset_init+0x54/0x18c
   _dpu_kms_drm_obj_init+0x430/0x474
   dpu_kms_hw_init+0x5f8/0x6b4
   msm_drm_bind+0x360/0x6c8
   try_to_bring_up_master.part.7+0x28/0x70
   component_master_add_with_match+0xe8/0x124
   msm_pdev_probe+0x294/0x2b4
   platform_drv_probe+0x58/0xa4
   really_probe+0x150/0x294
   driver_probe_device+0xac/0xe8
   __device_attach_driver+0xa4/0xb4
   bus_for_each_drv+0x98/0xc8
   __device_attach+0xac/0x12c
   device_initial_probe+0x24/0x30
   bus_probe_device+0x38/0x98
   deferred_probe_work_func+0x78/0xa4
   process_one_work+0x24c/0x3dc
   worker_thread+0x280/0x360
   kthread+0x134/0x13c
   ret_from_fork+0x10/0x18
  Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
  ---[ end trace f22dda57f3648e2c ]---
  Kernel panic - not syncing: Fatal exception
  SMP: stopping secondary CPUs
  Kernel Offset: disabled
  CPU features: 0x0,22802a18
  Memory Limit: none

The problem is that when drm/msm does it's own iommu_attach_device(),
now the domain returned by iommu_get_domain_for_dev() is drm/msm's
domain, and it doesn't have domain->iova_cookie.

We kind of avoided this problem prior to sdm845/dpu because the iommu
was attached to the mdp node in dt, which is a child of the toplevel
mdss node (which corresponds to the dev passed in dma_map_sg()).  But
with sdm845, now the iommu is attached at the mdss level so we hit the
iommu_dma_ops in dma_map_sg().

But auto allocating/attaching a domain before the driver is probed was
already a blocking problem for enabling per-context pagetables for the
GPU.  This problem is also now solved with this patch.

Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
Tested-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
This is an alternative/replacement for [1].  What it lacks in elegance
it makes up for in practicality ;-)

[1] https://patchwork.freedesktop.org/patch/264930/

 drivers/of/device.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 5957cd4fa262..15ffee00fb22 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
 	return device_add(&ofdev->dev);
 }
 
+static const struct of_device_id iommu_blacklist[] = {
+	{ .compatible = "qcom,mdp4" },
+	{ .compatible = "qcom,mdss" },
+	{ .compatible = "qcom,sdm845-mdss" },
+	{ .compatible = "qcom,adreno" },
+	{}
+};
+
 /**
  * of_dma_configure - Setup DMA configuration
  * @dev:	Device to apply DMA configuration
@@ -164,6 +172,20 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
 	dev_dbg(dev, "device is%sbehind an iommu\n",
 		iommu ? " " : " not ");
 
+	/*
+	 * There is at least one case where the driver wants to directly
+	 * manage the IOMMU, but if we end up with iommu dma_ops, that
+	 * interferes with the drivers ability to use dma_map_sg() for
+	 * cache operations.  Since we don't currently have a better
+	 * solution, and this code runs before the driver is probed and
+	 * has a chance to intervene, use a simple blacklist to avoid
+	 * ending up with iommu dma_ops:
+	 */
+	if (of_match_device(iommu_blacklist, dev)) {
+		dev_dbg(dev, "skipping iommu hookup\n");
+		iommu = NULL;
+	}
+
 	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
 
 	return 0;
-- 
2.19.2

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Tomasz Figa]

On Sat, Dec 1, 2018 at 8:54 AM Rob Clark <robdclark@gmail.com> wrote:
>
> This solves a problem we see with drm/msm, caused by getting
> iommu_dma_ops while we attach our own domain and manage it directly at
> the iommu API level:
>
>   [0000000000000038] user address but active_mm is swapper
>   Internal error: Oops: 96000005 [#1] PREEMPT SMP
>   Modules linked in:
>   CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
>   Hardware name: xxx (DT)
>   Workqueue: events deferred_probe_work_func
>   pstate: 80c00009 (Nzcv daif +PAN +UAO)
>   pc : iommu_dma_map_sg+0x7c/0x2c8
>   lr : iommu_dma_map_sg+0x40/0x2c8
>   sp : ffffff80095eb4f0
>   x29: ffffff80095eb4f0 x28: 0000000000000000
>   x27: ffffffc0f9431578 x26: 0000000000000000
>   x25: 00000000ffffffff x24: 0000000000000003
>   x23: 0000000000000001 x22: ffffffc0fa9ac010
>   x21: 0000000000000000 x20: ffffffc0fab40980
>   x19: ffffffc0fab40980 x18: 0000000000000003
>   x17: 00000000000001c4 x16: 0000000000000007
>   x15: 000000000000000e x14: ffffffffffffffff
>   x13: ffff000000000000 x12: 0000000000000028
>   x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
>   x9 : 0000000000000000 x8 : ffffffc0fab409a0
>   x7 : 0000000000000000 x6 : 0000000000000002
>   x5 : 0000000100000000 x4 : 0000000000000000
>   x3 : 0000000000000001 x2 : 0000000000000002
>   x1 : ffffffc0f9431578 x0 : 0000000000000000
>   Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
>   Call trace:
>    iommu_dma_map_sg+0x7c/0x2c8
>    __iommu_map_sg_attrs+0x70/0x84
>    get_pages+0x170/0x1e8
>    msm_gem_get_iova+0x8c/0x128
>    _msm_gem_kernel_new+0x6c/0xc8
>    msm_gem_kernel_new+0x4c/0x58
>    dsi_tx_buf_alloc_6g+0x4c/0x8c
>    msm_dsi_host_modeset_init+0xc8/0x108
>    msm_dsi_modeset_init+0x54/0x18c
>    _dpu_kms_drm_obj_init+0x430/0x474
>    dpu_kms_hw_init+0x5f8/0x6b4
>    msm_drm_bind+0x360/0x6c8
>    try_to_bring_up_master.part.7+0x28/0x70
>    component_master_add_with_match+0xe8/0x124
>    msm_pdev_probe+0x294/0x2b4
>    platform_drv_probe+0x58/0xa4
>    really_probe+0x150/0x294
>    driver_probe_device+0xac/0xe8
>    __device_attach_driver+0xa4/0xb4
>    bus_for_each_drv+0x98/0xc8
>    __device_attach+0xac/0x12c
>    device_initial_probe+0x24/0x30
>    bus_probe_device+0x38/0x98
>    deferred_probe_work_func+0x78/0xa4
>    process_one_work+0x24c/0x3dc
>    worker_thread+0x280/0x360
>    kthread+0x134/0x13c
>    ret_from_fork+0x10/0x18
>   Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
>   ---[ end trace f22dda57f3648e2c ]---
>   Kernel panic - not syncing: Fatal exception
>   SMP: stopping secondary CPUs
>   Kernel Offset: disabled
>   CPU features: 0x0,22802a18
>   Memory Limit: none
>
> The problem is that when drm/msm does it's own iommu_attach_device(),
> now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> domain, and it doesn't have domain->iova_cookie.
>
> We kind of avoided this problem prior to sdm845/dpu because the iommu
> was attached to the mdp node in dt, which is a child of the toplevel
> mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> with sdm845, now the iommu is attached at the mdss level so we hit the
> iommu_dma_ops in dma_map_sg().
>
> But auto allocating/attaching a domain before the driver is probed was
> already a blocking problem for enabling per-context pagetables for the
> GPU.  This problem is also now solved with this patch.
>
> Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
> Tested-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> This is an alternative/replacement for [1].  What it lacks in elegance
> it makes up for in practicality ;-)
>
> [1] https://patchwork.freedesktop.org/patch/264930/
>
>  drivers/of/device.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 5957cd4fa262..15ffee00fb22 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
>         return device_add(&ofdev->dev);
>  }
>
> +static const struct of_device_id iommu_blacklist[] = {
> +       { .compatible = "qcom,mdp4" },
> +       { .compatible = "qcom,mdss" },
> +       { .compatible = "qcom,sdm845-mdss" },
> +       { .compatible = "qcom,adreno" },
> +       {}
> +};
> +
>  /**
>   * of_dma_configure - Setup DMA configuration
>   * @dev:       Device to apply DMA configuration
> @@ -164,6 +172,20 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
>         dev_dbg(dev, "device is%sbehind an iommu\n",
>                 iommu ? " " : " not ");
>
> +       /*
> +        * There is at least one case where the driver wants to directly
> +        * manage the IOMMU, but if we end up with iommu dma_ops, that
> +        * interferes with the drivers ability to use dma_map_sg() for
> +        * cache operations.  Since we don't currently have a better
> +        * solution, and this code runs before the driver is probed and
> +        * has a chance to intervene, use a simple blacklist to avoid
> +        * ending up with iommu dma_ops:
> +        */
> +       if (of_match_device(iommu_blacklist, dev)) {
> +               dev_dbg(dev, "skipping iommu hookup\n");
> +               iommu = NULL;
> +       }
> +
>         arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
>
>         return 0;
> --
> 2.19.2
>

+Marek Szyprowski who I believe had a similar problem with Exynos DRM before.

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Marek Szyprowski]

Hi Tomasz,

On 2018-12-03 01:10, Tomasz Figa wrote:
> On Sat, Dec 1, 2018 at 8:54 AM Rob Clark <robdclark@gmail.com> wrote:
>> This solves a problem we see with drm/msm, caused by getting
>> iommu_dma_ops while we attach our own domain and manage it directly at
>> the iommu API level:
>>
>>   [0000000000000038] user address but active_mm is swapper
>>   Internal error: Oops: 96000005 [#1] PREEMPT SMP
>>   Modules linked in:
>>   CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
>>   Hardware name: xxx (DT)
>>   Workqueue: events deferred_probe_work_func
>>   pstate: 80c00009 (Nzcv daif +PAN +UAO)
>>   pc : iommu_dma_map_sg+0x7c/0x2c8
>>   lr : iommu_dma_map_sg+0x40/0x2c8
>>   sp : ffffff80095eb4f0
>>   x29: ffffff80095eb4f0 x28: 0000000000000000
>>   x27: ffffffc0f9431578 x26: 0000000000000000
>>   x25: 00000000ffffffff x24: 0000000000000003
>>   x23: 0000000000000001 x22: ffffffc0fa9ac010
>>   x21: 0000000000000000 x20: ffffffc0fab40980
>>   x19: ffffffc0fab40980 x18: 0000000000000003
>>   x17: 00000000000001c4 x16: 0000000000000007
>>   x15: 000000000000000e x14: ffffffffffffffff
>>   x13: ffff000000000000 x12: 0000000000000028
>>   x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
>>   x9 : 0000000000000000 x8 : ffffffc0fab409a0
>>   x7 : 0000000000000000 x6 : 0000000000000002
>>   x5 : 0000000100000000 x4 : 0000000000000000
>>   x3 : 0000000000000001 x2 : 0000000000000002
>>   x1 : ffffffc0f9431578 x0 : 0000000000000000
>>   Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
>>   Call trace:
>>    iommu_dma_map_sg+0x7c/0x2c8
>>    __iommu_map_sg_attrs+0x70/0x84
>>    get_pages+0x170/0x1e8
>>    msm_gem_get_iova+0x8c/0x128
>>    _msm_gem_kernel_new+0x6c/0xc8
>>    msm_gem_kernel_new+0x4c/0x58
>>    dsi_tx_buf_alloc_6g+0x4c/0x8c
>>    msm_dsi_host_modeset_init+0xc8/0x108
>>    msm_dsi_modeset_init+0x54/0x18c
>>    _dpu_kms_drm_obj_init+0x430/0x474
>>    dpu_kms_hw_init+0x5f8/0x6b4
>>    msm_drm_bind+0x360/0x6c8
>>    try_to_bring_up_master.part.7+0x28/0x70
>>    component_master_add_with_match+0xe8/0x124
>>    msm_pdev_probe+0x294/0x2b4
>>    platform_drv_probe+0x58/0xa4
>>    really_probe+0x150/0x294
>>    driver_probe_device+0xac/0xe8
>>    __device_attach_driver+0xa4/0xb4
>>    bus_for_each_drv+0x98/0xc8
>>    __device_attach+0xac/0x12c
>>    device_initial_probe+0x24/0x30
>>    bus_probe_device+0x38/0x98
>>    deferred_probe_work_func+0x78/0xa4
>>    process_one_work+0x24c/0x3dc
>>    worker_thread+0x280/0x360
>>    kthread+0x134/0x13c
>>    ret_from_fork+0x10/0x18
>>   Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
>>   ---[ end trace f22dda57f3648e2c ]---
>>   Kernel panic - not syncing: Fatal exception
>>   SMP: stopping secondary CPUs
>>   Kernel Offset: disabled
>>   CPU features: 0x0,22802a18
>>   Memory Limit: none
>>
>> The problem is that when drm/msm does it's own iommu_attach_device(),
>> now the domain returned by iommu_get_domain_for_dev() is drm/msm's
>> domain, and it doesn't have domain->iova_cookie.
>>
>> We kind of avoided this problem prior to sdm845/dpu because the iommu
>> was attached to the mdp node in dt, which is a child of the toplevel
>> mdss node (which corresponds to the dev passed in dma_map_sg()).  But
>> with sdm845, now the iommu is attached at the mdss level so we hit the
>> iommu_dma_ops in dma_map_sg().
>>
>> But auto allocating/attaching a domain before the driver is probed was
>> already a blocking problem for enabling per-context pagetables for the
>> GPU.  This problem is also now solved with this patch.
>>
>> Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
>> Tested-by: Douglas Anderson <dianders@chromium.org>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>> This is an alternative/replacement for [1].  What it lacks in elegance
>> it makes up for in practicality ;-)
>>
>> [1] https://patchwork.freedesktop.org/patch/264930/
>>
>>  drivers/of/device.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 5957cd4fa262..15ffee00fb22 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
>>         return device_add(&ofdev->dev);
>>  }
>>
>> +static const struct of_device_id iommu_blacklist[] = {
>> +       { .compatible = "qcom,mdp4" },
>> +       { .compatible = "qcom,mdss" },
>> +       { .compatible = "qcom,sdm845-mdss" },
>> +       { .compatible = "qcom,adreno" },
>> +       {}
>> +};
>> +
>>  /**
>>   * of_dma_configure - Setup DMA configuration
>>   * @dev:       Device to apply DMA configuration
>> @@ -164,6 +172,20 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
>>         dev_dbg(dev, "device is%sbehind an iommu\n",
>>                 iommu ? " " : " not ");
>>
>> +       /*
>> +        * There is at least one case where the driver wants to directly
>> +        * manage the IOMMU, but if we end up with iommu dma_ops, that
>> +        * interferes with the drivers ability to use dma_map_sg() for
>> +        * cache operations.  Since we don't currently have a better
>> +        * solution, and this code runs before the driver is probed and
>> +        * has a chance to intervene, use a simple blacklist to avoid
>> +        * ending up with iommu dma_ops:
>> +        */
>> +       if (of_match_device(iommu_blacklist, dev)) {
>> +               dev_dbg(dev, "skipping iommu hookup\n");
>> +               iommu = NULL;
>> +       }
>> +
>>         arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
>>
>>         return 0;
>> --
>> 2.19.2
>>
> +Marek Szyprowski who I believe had a similar problem with Exynos DRM before.
>
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>


Thanks for adding me. So far Exynos DRM use other workaround, see commit
1feda5eb77fc ("drm/exynos: Use selected dma_dev default iommu domain
instead of a fake one"). Here is the link to the thread with more
comments and background:
https://www.spinics.net/lists/arm-kernel/msg676100.html

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Robin Murphy]

Hi Rob,

On 01/12/2018 16:53, Rob Clark wrote:
> This solves a problem we see with drm/msm, caused by getting
> iommu_dma_ops while we attach our own domain and manage it directly at
> the iommu API level:
> 
>    [0000000000000038] user address but active_mm is swapper
>    Internal error: Oops: 96000005 [#1] PREEMPT SMP
>    Modules linked in:
>    CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
>    Hardware name: xxx (DT)
>    Workqueue: events deferred_probe_work_func
>    pstate: 80c00009 (Nzcv daif +PAN +UAO)
>    pc : iommu_dma_map_sg+0x7c/0x2c8
>    lr : iommu_dma_map_sg+0x40/0x2c8
>    sp : ffffff80095eb4f0
>    x29: ffffff80095eb4f0 x28: 0000000000000000
>    x27: ffffffc0f9431578 x26: 0000000000000000
>    x25: 00000000ffffffff x24: 0000000000000003
>    x23: 0000000000000001 x22: ffffffc0fa9ac010
>    x21: 0000000000000000 x20: ffffffc0fab40980
>    x19: ffffffc0fab40980 x18: 0000000000000003
>    x17: 00000000000001c4 x16: 0000000000000007
>    x15: 000000000000000e x14: ffffffffffffffff
>    x13: ffff000000000000 x12: 0000000000000028
>    x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
>    x9 : 0000000000000000 x8 : ffffffc0fab409a0
>    x7 : 0000000000000000 x6 : 0000000000000002
>    x5 : 0000000100000000 x4 : 0000000000000000
>    x3 : 0000000000000001 x2 : 0000000000000002
>    x1 : ffffffc0f9431578 x0 : 0000000000000000
>    Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
>    Call trace:
>     iommu_dma_map_sg+0x7c/0x2c8
>     __iommu_map_sg_attrs+0x70/0x84
>     get_pages+0x170/0x1e8
>     msm_gem_get_iova+0x8c/0x128
>     _msm_gem_kernel_new+0x6c/0xc8
>     msm_gem_kernel_new+0x4c/0x58
>     dsi_tx_buf_alloc_6g+0x4c/0x8c
>     msm_dsi_host_modeset_init+0xc8/0x108
>     msm_dsi_modeset_init+0x54/0x18c
>     _dpu_kms_drm_obj_init+0x430/0x474
>     dpu_kms_hw_init+0x5f8/0x6b4
>     msm_drm_bind+0x360/0x6c8
>     try_to_bring_up_master.part.7+0x28/0x70
>     component_master_add_with_match+0xe8/0x124
>     msm_pdev_probe+0x294/0x2b4
>     platform_drv_probe+0x58/0xa4
>     really_probe+0x150/0x294
>     driver_probe_device+0xac/0xe8
>     __device_attach_driver+0xa4/0xb4
>     bus_for_each_drv+0x98/0xc8
>     __device_attach+0xac/0x12c
>     device_initial_probe+0x24/0x30
>     bus_probe_device+0x38/0x98
>     deferred_probe_work_func+0x78/0xa4
>     process_one_work+0x24c/0x3dc
>     worker_thread+0x280/0x360
>     kthread+0x134/0x13c
>     ret_from_fork+0x10/0x18
>    Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
>    ---[ end trace f22dda57f3648e2c ]---
>    Kernel panic - not syncing: Fatal exception
>    SMP: stopping secondary CPUs
>    Kernel Offset: disabled
>    CPU features: 0x0,22802a18
>    Memory Limit: none
> 
> The problem is that when drm/msm does it's own iommu_attach_device(),
> now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> domain, and it doesn't have domain->iova_cookie.

Does this crash still happen with 4.20-rc? Because as of 6af588fed391 it 
really shouldn't.

> We kind of avoided this problem prior to sdm845/dpu because the iommu
> was attached to the mdp node in dt, which is a child of the toplevel
> mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> with sdm845, now the iommu is attached at the mdss level so we hit the
> iommu_dma_ops in dma_map_sg().
> 
> But auto allocating/attaching a domain before the driver is probed was
> already a blocking problem for enabling per-context pagetables for the
> GPU.  This problem is also now solved with this patch.

s/solved/worked around/

If you want a guarantee of actually getting a specific hardware context 
allocated for a given domain, there needs to be code in the IOMMU driver 
to understand and honour that. Implicitly depending on whatever happens 
to fall out of current driver behaviour on current systems is not a real 
solution.

> Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure

That's rather misleading, since the crash described above depends on at 
least two other major changes which came long after that commit.

It's not that I don't understand exactly what you want here - just that 
this commit message isn't a coherent justification for that ;)

> Tested-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> This is an alternative/replacement for [1].  What it lacks in elegance
> it makes up for in practicality ;-)
> 
> [1] https://patchwork.freedesktop.org/patch/264930/
> 
>   drivers/of/device.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 5957cd4fa262..15ffee00fb22 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
>   	return device_add(&ofdev->dev);
>   }
>   
> +static const struct of_device_id iommu_blacklist[] = {
> +	{ .compatible = "qcom,mdp4" },
> +	{ .compatible = "qcom,mdss" },
> +	{ .compatible = "qcom,sdm845-mdss" },
> +	{ .compatible = "qcom,adreno" },
> +	{}
> +};
> +
>   /**
>    * of_dma_configure - Setup DMA configuration
>    * @dev:	Device to apply DMA configuration
> @@ -164,6 +172,20 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
>   	dev_dbg(dev, "device is%sbehind an iommu\n",
>   		iommu ? " " : " not ");
>   
> +	/*
> +	 * There is at least one case where the driver wants to directly
> +	 * manage the IOMMU, but if we end up with iommu dma_ops, that
> +	 * interferes with the drivers ability to use dma_map_sg() for
> +	 * cache operations.  Since we don't currently have a better
> +	 * solution, and this code runs before the driver is probed and
> +	 * has a chance to intervene, use a simple blacklist to avoid
> +	 * ending up with iommu dma_ops:
> +	 */
> +	if (of_match_device(iommu_blacklist, dev)) {
> +		dev_dbg(dev, "skipping iommu hookup\n");
> +		iommu = NULL;
> +	}

Given that a default domain will already have been allocated by the time 
we get here, regardless of whether we pretend of_iommu_configure() did 
nothing, I'm puzzled as to how this change is 'solving' that aspect as 
claimed :/

Is CONFIG_IOMMU_DEFAULT_PASSTHROUGH a sufficient workaround for msm at 
the moment, or do you have other devices which do actually want 
iommu_dma_ops?

Robin.

> +
>   	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
>   
>   	return 0;
> 

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Rob Clark]

On Mon, Dec 3, 2018 at 7:45 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> Hi Rob,
>
> On 01/12/2018 16:53, Rob Clark wrote:
> > This solves a problem we see with drm/msm, caused by getting
> > iommu_dma_ops while we attach our own domain and manage it directly at
> > the iommu API level:
> >
> >    [0000000000000038] user address but active_mm is swapper
> >    Internal error: Oops: 96000005 [#1] PREEMPT SMP
> >    Modules linked in:
> >    CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
> >    Hardware name: xxx (DT)
> >    Workqueue: events deferred_probe_work_func
> >    pstate: 80c00009 (Nzcv daif +PAN +UAO)
> >    pc : iommu_dma_map_sg+0x7c/0x2c8
> >    lr : iommu_dma_map_sg+0x40/0x2c8
> >    sp : ffffff80095eb4f0
> >    x29: ffffff80095eb4f0 x28: 0000000000000000
> >    x27: ffffffc0f9431578 x26: 0000000000000000
> >    x25: 00000000ffffffff x24: 0000000000000003
> >    x23: 0000000000000001 x22: ffffffc0fa9ac010
> >    x21: 0000000000000000 x20: ffffffc0fab40980
> >    x19: ffffffc0fab40980 x18: 0000000000000003
> >    x17: 00000000000001c4 x16: 0000000000000007
> >    x15: 000000000000000e x14: ffffffffffffffff
> >    x13: ffff000000000000 x12: 0000000000000028
> >    x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> >    x9 : 0000000000000000 x8 : ffffffc0fab409a0
> >    x7 : 0000000000000000 x6 : 0000000000000002
> >    x5 : 0000000100000000 x4 : 0000000000000000
> >    x3 : 0000000000000001 x2 : 0000000000000002
> >    x1 : ffffffc0f9431578 x0 : 0000000000000000
> >    Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
> >    Call trace:
> >     iommu_dma_map_sg+0x7c/0x2c8
> >     __iommu_map_sg_attrs+0x70/0x84
> >     get_pages+0x170/0x1e8
> >     msm_gem_get_iova+0x8c/0x128
> >     _msm_gem_kernel_new+0x6c/0xc8
> >     msm_gem_kernel_new+0x4c/0x58
> >     dsi_tx_buf_alloc_6g+0x4c/0x8c
> >     msm_dsi_host_modeset_init+0xc8/0x108
> >     msm_dsi_modeset_init+0x54/0x18c
> >     _dpu_kms_drm_obj_init+0x430/0x474
> >     dpu_kms_hw_init+0x5f8/0x6b4
> >     msm_drm_bind+0x360/0x6c8
> >     try_to_bring_up_master.part.7+0x28/0x70
> >     component_master_add_with_match+0xe8/0x124
> >     msm_pdev_probe+0x294/0x2b4
> >     platform_drv_probe+0x58/0xa4
> >     really_probe+0x150/0x294
> >     driver_probe_device+0xac/0xe8
> >     __device_attach_driver+0xa4/0xb4
> >     bus_for_each_drv+0x98/0xc8
> >     __device_attach+0xac/0x12c
> >     device_initial_probe+0x24/0x30
> >     bus_probe_device+0x38/0x98
> >     deferred_probe_work_func+0x78/0xa4
> >     process_one_work+0x24c/0x3dc
> >     worker_thread+0x280/0x360
> >     kthread+0x134/0x13c
> >     ret_from_fork+0x10/0x18
> >    Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
> >    ---[ end trace f22dda57f3648e2c ]---
> >    Kernel panic - not syncing: Fatal exception
> >    SMP: stopping secondary CPUs
> >    Kernel Offset: disabled
> >    CPU features: 0x0,22802a18
> >    Memory Limit: none
> >
> > The problem is that when drm/msm does it's own iommu_attach_device(),
> > now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> > domain, and it doesn't have domain->iova_cookie.
>
> Does this crash still happen with 4.20-rc? Because as of 6af588fed391 it
> really shouldn't.

for this hw, I'm still on 4.19, although that does look like it would
avoid the issue.

> > We kind of avoided this problem prior to sdm845/dpu because the iommu
> > was attached to the mdp node in dt, which is a child of the toplevel
> > mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> > with sdm845, now the iommu is attached at the mdss level so we hit the
> > iommu_dma_ops in dma_map_sg().
> >
> > But auto allocating/attaching a domain before the driver is probed was
> > already a blocking problem for enabling per-context pagetables for the
> > GPU.  This problem is also now solved with this patch.
>
> s/solved/worked around/
>
> If you want a guarantee of actually getting a specific hardware context
> allocated for a given domain, there needs to be code in the IOMMU driver
> to understand and honour that. Implicitly depending on whatever happens
> to fall out of current driver behaviour on current systems is not a real
> solution.

ok, fair.. but I'll settle for "works" in the absence of better options..

At some level, it would be nice to be able to optionally specify a
context-bank in the iommu bindings.  But not sure how to make that fit
w/ cb allocated per domain.  And I assume I'm the only one who has
this problem?

> > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
>
> That's rather misleading, since the crash described above depends on at
> least two other major changes which came long after that commit.

Fair, when I realized it was the difference in where the iommu
attaches between dpu1 (sdm845) and everything coming before, I should
have removed the tag.

BR,
-R

> It's not that I don't understand exactly what you want here - just that
> this commit message isn't a coherent justification for that ;)
>
> > Tested-by: Douglas Anderson <dianders@chromium.org>
> > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > ---
> > This is an alternative/replacement for [1].  What it lacks in elegance
> > it makes up for in practicality ;-)
> >
> > [1] https://patchwork.freedesktop.org/patch/264930/
> >
> >   drivers/of/device.c | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > index 5957cd4fa262..15ffee00fb22 100644
> > --- a/drivers/of/device.c
> > +++ b/drivers/of/device.c
> > @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> >       return device_add(&ofdev->dev);
> >   }
> >
> > +static const struct of_device_id iommu_blacklist[] = {
> > +     { .compatible = "qcom,mdp4" },
> > +     { .compatible = "qcom,mdss" },
> > +     { .compatible = "qcom,sdm845-mdss" },
> > +     { .compatible = "qcom,adreno" },
> > +     {}
> > +};
> > +
> >   /**
> >    * of_dma_configure - Setup DMA configuration
> >    * @dev:    Device to apply DMA configuration
> > @@ -164,6 +172,20 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
> >       dev_dbg(dev, "device is%sbehind an iommu\n",
> >               iommu ? " " : " not ");
> >
> > +     /*
> > +      * There is at least one case where the driver wants to directly
> > +      * manage the IOMMU, but if we end up with iommu dma_ops, that
> > +      * interferes with the drivers ability to use dma_map_sg() for
> > +      * cache operations.  Since we don't currently have a better
> > +      * solution, and this code runs before the driver is probed and
> > +      * has a chance to intervene, use a simple blacklist to avoid
> > +      * ending up with iommu dma_ops:
> > +      */
> > +     if (of_match_device(iommu_blacklist, dev)) {
> > +             dev_dbg(dev, "skipping iommu hookup\n");
> > +             iommu = NULL;
> > +     }
>
> Given that a default domain will already have been allocated by the time
> we get here, regardless of whether we pretend of_iommu_configure() did
> nothing, I'm puzzled as to how this change is 'solving' that aspect as
> claimed :/
>
> Is CONFIG_IOMMU_DEFAULT_PASSTHROUGH a sufficient workaround for msm at
> the moment, or do you have other devices which do actually want
> iommu_dma_ops?
>
> Robin.
>
> > +
> >       arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
> >
> >       return 0;
> >

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Rob Clark]

On Mon, Dec 3, 2018 at 7:45 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> Hi Rob,
>
> On 01/12/2018 16:53, Rob Clark wrote:
> > This solves a problem we see with drm/msm, caused by getting
> > iommu_dma_ops while we attach our own domain and manage it directly at
> > the iommu API level:
> >
> >    [0000000000000038] user address but active_mm is swapper
> >    Internal error: Oops: 96000005 [#1] PREEMPT SMP
> >    Modules linked in:
> >    CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
> >    Hardware name: xxx (DT)
> >    Workqueue: events deferred_probe_work_func
> >    pstate: 80c00009 (Nzcv daif +PAN +UAO)
> >    pc : iommu_dma_map_sg+0x7c/0x2c8
> >    lr : iommu_dma_map_sg+0x40/0x2c8
> >    sp : ffffff80095eb4f0
> >    x29: ffffff80095eb4f0 x28: 0000000000000000
> >    x27: ffffffc0f9431578 x26: 0000000000000000
> >    x25: 00000000ffffffff x24: 0000000000000003
> >    x23: 0000000000000001 x22: ffffffc0fa9ac010
> >    x21: 0000000000000000 x20: ffffffc0fab40980
> >    x19: ffffffc0fab40980 x18: 0000000000000003
> >    x17: 00000000000001c4 x16: 0000000000000007
> >    x15: 000000000000000e x14: ffffffffffffffff
> >    x13: ffff000000000000 x12: 0000000000000028
> >    x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> >    x9 : 0000000000000000 x8 : ffffffc0fab409a0
> >    x7 : 0000000000000000 x6 : 0000000000000002
> >    x5 : 0000000100000000 x4 : 0000000000000000
> >    x3 : 0000000000000001 x2 : 0000000000000002
> >    x1 : ffffffc0f9431578 x0 : 0000000000000000
> >    Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
> >    Call trace:
> >     iommu_dma_map_sg+0x7c/0x2c8
> >     __iommu_map_sg_attrs+0x70/0x84
> >     get_pages+0x170/0x1e8
> >     msm_gem_get_iova+0x8c/0x128
> >     _msm_gem_kernel_new+0x6c/0xc8
> >     msm_gem_kernel_new+0x4c/0x58
> >     dsi_tx_buf_alloc_6g+0x4c/0x8c
> >     msm_dsi_host_modeset_init+0xc8/0x108
> >     msm_dsi_modeset_init+0x54/0x18c
> >     _dpu_kms_drm_obj_init+0x430/0x474
> >     dpu_kms_hw_init+0x5f8/0x6b4
> >     msm_drm_bind+0x360/0x6c8
> >     try_to_bring_up_master.part.7+0x28/0x70
> >     component_master_add_with_match+0xe8/0x124
> >     msm_pdev_probe+0x294/0x2b4
> >     platform_drv_probe+0x58/0xa4
> >     really_probe+0x150/0x294
> >     driver_probe_device+0xac/0xe8
> >     __device_attach_driver+0xa4/0xb4
> >     bus_for_each_drv+0x98/0xc8
> >     __device_attach+0xac/0x12c
> >     device_initial_probe+0x24/0x30
> >     bus_probe_device+0x38/0x98
> >     deferred_probe_work_func+0x78/0xa4
> >     process_one_work+0x24c/0x3dc
> >     worker_thread+0x280/0x360
> >     kthread+0x134/0x13c
> >     ret_from_fork+0x10/0x18
> >    Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
> >    ---[ end trace f22dda57f3648e2c ]---
> >    Kernel panic - not syncing: Fatal exception
> >    SMP: stopping secondary CPUs
> >    Kernel Offset: disabled
> >    CPU features: 0x0,22802a18
> >    Memory Limit: none
> >
> > The problem is that when drm/msm does it's own iommu_attach_device(),
> > now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> > domain, and it doesn't have domain->iova_cookie.
>
> Does this crash still happen with 4.20-rc? Because as of 6af588fed391 it
> really shouldn't.
>
> > We kind of avoided this problem prior to sdm845/dpu because the iommu
> > was attached to the mdp node in dt, which is a child of the toplevel
> > mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> > with sdm845, now the iommu is attached at the mdss level so we hit the
> > iommu_dma_ops in dma_map_sg().
> >
> > But auto allocating/attaching a domain before the driver is probed was
> > already a blocking problem for enabling per-context pagetables for the
> > GPU.  This problem is also now solved with this patch.
>
> s/solved/worked around/
>
> If you want a guarantee of actually getting a specific hardware context
> allocated for a given domain, there needs to be code in the IOMMU driver
> to understand and honour that. Implicitly depending on whatever happens
> to fall out of current driver behaviour on current systems is not a real
> solution.
>
> > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
>
> That's rather misleading, since the crash described above depends on at
> least two other major changes which came long after that commit.
>
> It's not that I don't understand exactly what you want here - just that
> this commit message isn't a coherent justification for that ;)
>
> > Tested-by: Douglas Anderson <dianders@chromium.org>
> > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > ---
> > This is an alternative/replacement for [1].  What it lacks in elegance
> > it makes up for in practicality ;-)
> >
> > [1] https://patchwork.freedesktop.org/patch/264930/
> >
> >   drivers/of/device.c | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > index 5957cd4fa262..15ffee00fb22 100644
> > --- a/drivers/of/device.c
> > +++ b/drivers/of/device.c
> > @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> >       return device_add(&ofdev->dev);
> >   }
> >
> > +static const struct of_device_id iommu_blacklist[] = {
> > +     { .compatible = "qcom,mdp4" },
> > +     { .compatible = "qcom,mdss" },
> > +     { .compatible = "qcom,sdm845-mdss" },
> > +     { .compatible = "qcom,adreno" },
> > +     {}
> > +};
> > +
> >   /**
> >    * of_dma_configure - Setup DMA configuration
> >    * @dev:    Device to apply DMA configuration
> > @@ -164,6 +172,20 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
> >       dev_dbg(dev, "device is%sbehind an iommu\n",
> >               iommu ? " " : " not ");
> >
> > +     /*
> > +      * There is at least one case where the driver wants to directly
> > +      * manage the IOMMU, but if we end up with iommu dma_ops, that
> > +      * interferes with the drivers ability to use dma_map_sg() for
> > +      * cache operations.  Since we don't currently have a better
> > +      * solution, and this code runs before the driver is probed and
> > +      * has a chance to intervene, use a simple blacklist to avoid
> > +      * ending up with iommu dma_ops:
> > +      */
> > +     if (of_match_device(iommu_blacklist, dev)) {
> > +             dev_dbg(dev, "skipping iommu hookup\n");
> > +             iommu = NULL;
> > +     }
>
> Given that a default domain will already have been allocated by the time
> we get here, regardless of whether we pretend of_iommu_configure() did
> nothing, I'm puzzled as to how this change is 'solving' that aspect as
> claimed :/

Possibly I'm reading this wrong.. I thought it is not created until
arm_iommu_create_mapping()..

but hmm, I guess I was looking at the armv7 code.  Seems to be
different on arm64.. ugg..

> Is CONFIG_IOMMU_DEFAULT_PASSTHROUGH a sufficient workaround for msm at
> the moment, or do you have other devices which do actually want
> iommu_dma_ops?

I think there are at least a few other devices (venus, camera, maybe
some others that are not wired up yet?)

BR,
-R

>
> Robin.
>
> > +
> >       arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
> >
> >       return 0;
> >

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Vivek Gautam]

On Mon, Dec 3, 2018 at 7:56 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Dec 3, 2018 at 7:45 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > Hi Rob,
> >
> > On 01/12/2018 16:53, Rob Clark wrote:
> > > This solves a problem we see with drm/msm, caused by getting
> > > iommu_dma_ops while we attach our own domain and manage it directly at
> > > the iommu API level:
> > >
> > >    [0000000000000038] user address but active_mm is swapper
> > >    Internal error: Oops: 96000005 [#1] PREEMPT SMP
> > >    Modules linked in:
> > >    CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
> > >    Hardware name: xxx (DT)
> > >    Workqueue: events deferred_probe_work_func
> > >    pstate: 80c00009 (Nzcv daif +PAN +UAO)
> > >    pc : iommu_dma_map_sg+0x7c/0x2c8
> > >    lr : iommu_dma_map_sg+0x40/0x2c8
> > >    sp : ffffff80095eb4f0
> > >    x29: ffffff80095eb4f0 x28: 0000000000000000
> > >    x27: ffffffc0f9431578 x26: 0000000000000000
> > >    x25: 00000000ffffffff x24: 0000000000000003
> > >    x23: 0000000000000001 x22: ffffffc0fa9ac010
> > >    x21: 0000000000000000 x20: ffffffc0fab40980
> > >    x19: ffffffc0fab40980 x18: 0000000000000003
> > >    x17: 00000000000001c4 x16: 0000000000000007
> > >    x15: 000000000000000e x14: ffffffffffffffff
> > >    x13: ffff000000000000 x12: 0000000000000028
> > >    x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> > >    x9 : 0000000000000000 x8 : ffffffc0fab409a0
> > >    x7 : 0000000000000000 x6 : 0000000000000002
> > >    x5 : 0000000100000000 x4 : 0000000000000000
> > >    x3 : 0000000000000001 x2 : 0000000000000002
> > >    x1 : ffffffc0f9431578 x0 : 0000000000000000
> > >    Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
> > >    Call trace:
> > >     iommu_dma_map_sg+0x7c/0x2c8
> > >     __iommu_map_sg_attrs+0x70/0x84
> > >     get_pages+0x170/0x1e8
> > >     msm_gem_get_iova+0x8c/0x128
> > >     _msm_gem_kernel_new+0x6c/0xc8
> > >     msm_gem_kernel_new+0x4c/0x58
> > >     dsi_tx_buf_alloc_6g+0x4c/0x8c
> > >     msm_dsi_host_modeset_init+0xc8/0x108
> > >     msm_dsi_modeset_init+0x54/0x18c
> > >     _dpu_kms_drm_obj_init+0x430/0x474
> > >     dpu_kms_hw_init+0x5f8/0x6b4
> > >     msm_drm_bind+0x360/0x6c8
> > >     try_to_bring_up_master.part.7+0x28/0x70
> > >     component_master_add_with_match+0xe8/0x124
> > >     msm_pdev_probe+0x294/0x2b4
> > >     platform_drv_probe+0x58/0xa4
> > >     really_probe+0x150/0x294
> > >     driver_probe_device+0xac/0xe8
> > >     __device_attach_driver+0xa4/0xb4
> > >     bus_for_each_drv+0x98/0xc8
> > >     __device_attach+0xac/0x12c
> > >     device_initial_probe+0x24/0x30
> > >     bus_probe_device+0x38/0x98
> > >     deferred_probe_work_func+0x78/0xa4
> > >     process_one_work+0x24c/0x3dc
> > >     worker_thread+0x280/0x360
> > >     kthread+0x134/0x13c
> > >     ret_from_fork+0x10/0x18
> > >    Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
> > >    ---[ end trace f22dda57f3648e2c ]---
> > >    Kernel panic - not syncing: Fatal exception
> > >    SMP: stopping secondary CPUs
> > >    Kernel Offset: disabled
> > >    CPU features: 0x0,22802a18
> > >    Memory Limit: none
> > >
> > > The problem is that when drm/msm does it's own iommu_attach_device(),
> > > now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> > > domain, and it doesn't have domain->iova_cookie.
> >
> > Does this crash still happen with 4.20-rc? Because as of 6af588fed391 it
> > really shouldn't.
> >
> > > We kind of avoided this problem prior to sdm845/dpu because the iommu
> > > was attached to the mdp node in dt, which is a child of the toplevel
> > > mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> > > with sdm845, now the iommu is attached at the mdss level so we hit the
> > > iommu_dma_ops in dma_map_sg().
> > >
> > > But auto allocating/attaching a domain before the driver is probed was
> > > already a blocking problem for enabling per-context pagetables for the
> > > GPU.  This problem is also now solved with this patch.
> >
> > s/solved/worked around/
> >
> > If you want a guarantee of actually getting a specific hardware context
> > allocated for a given domain, there needs to be code in the IOMMU driver
> > to understand and honour that. Implicitly depending on whatever happens
> > to fall out of current driver behaviour on current systems is not a real
> > solution.
> >
> > > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
> >
> > That's rather misleading, since the crash described above depends on at
> > least two other major changes which came long after that commit.
> >
> > It's not that I don't understand exactly what you want here - just that
> > this commit message isn't a coherent justification for that ;)
> >
> > > Tested-by: Douglas Anderson <dianders@chromium.org>
> > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > > ---
> > > This is an alternative/replacement for [1].  What it lacks in elegance
> > > it makes up for in practicality ;-)
> > >
> > > [1] https://patchwork.freedesktop.org/patch/264930/
> > >
> > >   drivers/of/device.c | 22 ++++++++++++++++++++++
> > >   1 file changed, 22 insertions(+)
> > >
> > > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > > index 5957cd4fa262..15ffee00fb22 100644
> > > --- a/drivers/of/device.c
> > > +++ b/drivers/of/device.c
> > > @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> > >       return device_add(&ofdev->dev);
> > >   }
> > >
> > > +static const struct of_device_id iommu_blacklist[] = {
> > > +     { .compatible = "qcom,mdp4" },
> > > +     { .compatible = "qcom,mdss" },
> > > +     { .compatible = "qcom,sdm845-mdss" },
> > > +     { .compatible = "qcom,adreno" },
> > > +     {}
> > > +};
> > > +
> > >   /**
> > >    * of_dma_configure - Setup DMA configuration
> > >    * @dev:    Device to apply DMA configuration
> > > @@ -164,6 +172,20 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
> > >       dev_dbg(dev, "device is%sbehind an iommu\n",
> > >               iommu ? " " : " not ");
> > >
> > > +     /*
> > > +      * There is at least one case where the driver wants to directly
> > > +      * manage the IOMMU, but if we end up with iommu dma_ops, that
> > > +      * interferes with the drivers ability to use dma_map_sg() for
> > > +      * cache operations.  Since we don't currently have a better
> > > +      * solution, and this code runs before the driver is probed and
> > > +      * has a chance to intervene, use a simple blacklist to avoid
> > > +      * ending up with iommu dma_ops:
> > > +      */
> > > +     if (of_match_device(iommu_blacklist, dev)) {
> > > +             dev_dbg(dev, "skipping iommu hookup\n");
> > > +             iommu = NULL;
> > > +     }
> >
> > Given that a default domain will already have been allocated by the time
> > we get here, regardless of whether we pretend of_iommu_configure() did
> > nothing, I'm puzzled as to how this change is 'solving' that aspect as
> > claimed :/
>
> Possibly I'm reading this wrong.. I thought it is not created until
> arm_iommu_create_mapping()..
>
> but hmm, I guess I was looking at the armv7 code.  Seems to be
> different on arm64.. ugg..
>
> > Is CONFIG_IOMMU_DEFAULT_PASSTHROUGH a sufficient workaround for msm at
> > the moment, or do you have other devices which do actually want
> > iommu_dma_ops?
>
> I think there are at least a few other devices (venus, camera, maybe
> some others that are not wired up yet?)

Yes, for msm, the drm devices do not want the default DMA domain, whereas
the V4L devices - video and camera rely completely on DMA iommu domain
and use the dma mapping apis to manage things.
So enabling CONFIG_IOMMU_DEFAULT_PASSTHROUGH for msm platforms
is not the desired configuration.
Moreover, we can't enable this config with upstream kernels anyways.

Thanks
Vivek

>
> BR,
> -R
>
> >
> > Robin.
> >
> > > +
> > >       arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
> > >
> > >       return 0;
> > >
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Rob Herring]

On Sat, Dec 1, 2018 at 10:54 AM Rob Clark <robdclark@gmail.com> wrote:
>
> This solves a problem we see with drm/msm, caused by getting
> iommu_dma_ops while we attach our own domain and manage it directly at
> the iommu API level:
>
>   [0000000000000038] user address but active_mm is swapper
>   Internal error: Oops: 96000005 [#1] PREEMPT SMP
>   Modules linked in:
>   CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
>   Hardware name: xxx (DT)
>   Workqueue: events deferred_probe_work_func
>   pstate: 80c00009 (Nzcv daif +PAN +UAO)
>   pc : iommu_dma_map_sg+0x7c/0x2c8
>   lr : iommu_dma_map_sg+0x40/0x2c8
>   sp : ffffff80095eb4f0
>   x29: ffffff80095eb4f0 x28: 0000000000000000
>   x27: ffffffc0f9431578 x26: 0000000000000000
>   x25: 00000000ffffffff x24: 0000000000000003
>   x23: 0000000000000001 x22: ffffffc0fa9ac010
>   x21: 0000000000000000 x20: ffffffc0fab40980
>   x19: ffffffc0fab40980 x18: 0000000000000003
>   x17: 00000000000001c4 x16: 0000000000000007
>   x15: 000000000000000e x14: ffffffffffffffff
>   x13: ffff000000000000 x12: 0000000000000028
>   x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
>   x9 : 0000000000000000 x8 : ffffffc0fab409a0
>   x7 : 0000000000000000 x6 : 0000000000000002
>   x5 : 0000000100000000 x4 : 0000000000000000
>   x3 : 0000000000000001 x2 : 0000000000000002
>   x1 : ffffffc0f9431578 x0 : 0000000000000000
>   Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
>   Call trace:
>    iommu_dma_map_sg+0x7c/0x2c8
>    __iommu_map_sg_attrs+0x70/0x84
>    get_pages+0x170/0x1e8
>    msm_gem_get_iova+0x8c/0x128
>    _msm_gem_kernel_new+0x6c/0xc8
>    msm_gem_kernel_new+0x4c/0x58
>    dsi_tx_buf_alloc_6g+0x4c/0x8c
>    msm_dsi_host_modeset_init+0xc8/0x108
>    msm_dsi_modeset_init+0x54/0x18c
>    _dpu_kms_drm_obj_init+0x430/0x474
>    dpu_kms_hw_init+0x5f8/0x6b4
>    msm_drm_bind+0x360/0x6c8
>    try_to_bring_up_master.part.7+0x28/0x70
>    component_master_add_with_match+0xe8/0x124
>    msm_pdev_probe+0x294/0x2b4
>    platform_drv_probe+0x58/0xa4
>    really_probe+0x150/0x294
>    driver_probe_device+0xac/0xe8
>    __device_attach_driver+0xa4/0xb4
>    bus_for_each_drv+0x98/0xc8
>    __device_attach+0xac/0x12c
>    device_initial_probe+0x24/0x30
>    bus_probe_device+0x38/0x98
>    deferred_probe_work_func+0x78/0xa4
>    process_one_work+0x24c/0x3dc
>    worker_thread+0x280/0x360
>    kthread+0x134/0x13c
>    ret_from_fork+0x10/0x18
>   Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
>   ---[ end trace f22dda57f3648e2c ]---
>   Kernel panic - not syncing: Fatal exception
>   SMP: stopping secondary CPUs
>   Kernel Offset: disabled
>   CPU features: 0x0,22802a18
>   Memory Limit: none
>
> The problem is that when drm/msm does it's own iommu_attach_device(),
> now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> domain, and it doesn't have domain->iova_cookie.
>
> We kind of avoided this problem prior to sdm845/dpu because the iommu
> was attached to the mdp node in dt, which is a child of the toplevel
> mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> with sdm845, now the iommu is attached at the mdss level so we hit the
> iommu_dma_ops in dma_map_sg().
>
> But auto allocating/attaching a domain before the driver is probed was
> already a blocking problem for enabling per-context pagetables for the
> GPU.  This problem is also now solved with this patch.
>
> Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
> Tested-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> This is an alternative/replacement for [1].  What it lacks in elegance
> it makes up for in practicality ;-)
>
> [1] https://patchwork.freedesktop.org/patch/264930/
>
>  drivers/of/device.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 5957cd4fa262..15ffee00fb22 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
>         return device_add(&ofdev->dev);
>  }
>
> +static const struct of_device_id iommu_blacklist[] = {
> +       { .compatible = "qcom,mdp4" },
> +       { .compatible = "qcom,mdss" },
> +       { .compatible = "qcom,sdm845-mdss" },
> +       { .compatible = "qcom,adreno" },
> +       {}
> +};

Not completely clear to whether this is still needed or not, but this
really won't scale. Why can't the driver for these devices override
whatever has been setup by default?

Rob

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Rob Clark]

On Tue, Dec 4, 2018 at 2:29 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Sat, Dec 1, 2018 at 10:54 AM Rob Clark <robdclark@gmail.com> wrote:
> >
> > This solves a problem we see with drm/msm, caused by getting
> > iommu_dma_ops while we attach our own domain and manage it directly at
> > the iommu API level:
> >
> >   [0000000000000038] user address but active_mm is swapper
> >   Internal error: Oops: 96000005 [#1] PREEMPT SMP
> >   Modules linked in:
> >   CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
> >   Hardware name: xxx (DT)
> >   Workqueue: events deferred_probe_work_func
> >   pstate: 80c00009 (Nzcv daif +PAN +UAO)
> >   pc : iommu_dma_map_sg+0x7c/0x2c8
> >   lr : iommu_dma_map_sg+0x40/0x2c8
> >   sp : ffffff80095eb4f0
> >   x29: ffffff80095eb4f0 x28: 0000000000000000
> >   x27: ffffffc0f9431578 x26: 0000000000000000
> >   x25: 00000000ffffffff x24: 0000000000000003
> >   x23: 0000000000000001 x22: ffffffc0fa9ac010
> >   x21: 0000000000000000 x20: ffffffc0fab40980
> >   x19: ffffffc0fab40980 x18: 0000000000000003
> >   x17: 00000000000001c4 x16: 0000000000000007
> >   x15: 000000000000000e x14: ffffffffffffffff
> >   x13: ffff000000000000 x12: 0000000000000028
> >   x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> >   x9 : 0000000000000000 x8 : ffffffc0fab409a0
> >   x7 : 0000000000000000 x6 : 0000000000000002
> >   x5 : 0000000100000000 x4 : 0000000000000000
> >   x3 : 0000000000000001 x2 : 0000000000000002
> >   x1 : ffffffc0f9431578 x0 : 0000000000000000
> >   Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
> >   Call trace:
> >    iommu_dma_map_sg+0x7c/0x2c8
> >    __iommu_map_sg_attrs+0x70/0x84
> >    get_pages+0x170/0x1e8
> >    msm_gem_get_iova+0x8c/0x128
> >    _msm_gem_kernel_new+0x6c/0xc8
> >    msm_gem_kernel_new+0x4c/0x58
> >    dsi_tx_buf_alloc_6g+0x4c/0x8c
> >    msm_dsi_host_modeset_init+0xc8/0x108
> >    msm_dsi_modeset_init+0x54/0x18c
> >    _dpu_kms_drm_obj_init+0x430/0x474
> >    dpu_kms_hw_init+0x5f8/0x6b4
> >    msm_drm_bind+0x360/0x6c8
> >    try_to_bring_up_master.part.7+0x28/0x70
> >    component_master_add_with_match+0xe8/0x124
> >    msm_pdev_probe+0x294/0x2b4
> >    platform_drv_probe+0x58/0xa4
> >    really_probe+0x150/0x294
> >    driver_probe_device+0xac/0xe8
> >    __device_attach_driver+0xa4/0xb4
> >    bus_for_each_drv+0x98/0xc8
> >    __device_attach+0xac/0x12c
> >    device_initial_probe+0x24/0x30
> >    bus_probe_device+0x38/0x98
> >    deferred_probe_work_func+0x78/0xa4
> >    process_one_work+0x24c/0x3dc
> >    worker_thread+0x280/0x360
> >    kthread+0x134/0x13c
> >    ret_from_fork+0x10/0x18
> >   Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
> >   ---[ end trace f22dda57f3648e2c ]---
> >   Kernel panic - not syncing: Fatal exception
> >   SMP: stopping secondary CPUs
> >   Kernel Offset: disabled
> >   CPU features: 0x0,22802a18
> >   Memory Limit: none
> >
> > The problem is that when drm/msm does it's own iommu_attach_device(),
> > now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> > domain, and it doesn't have domain->iova_cookie.
> >
> > We kind of avoided this problem prior to sdm845/dpu because the iommu
> > was attached to the mdp node in dt, which is a child of the toplevel
> > mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> > with sdm845, now the iommu is attached at the mdss level so we hit the
> > iommu_dma_ops in dma_map_sg().
> >
> > But auto allocating/attaching a domain before the driver is probed was
> > already a blocking problem for enabling per-context pagetables for the
> > GPU.  This problem is also now solved with this patch.
> >
> > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
> > Tested-by: Douglas Anderson <dianders@chromium.org>
> > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > ---
> > This is an alternative/replacement for [1].  What it lacks in elegance
> > it makes up for in practicality ;-)
> >
> > [1] https://patchwork.freedesktop.org/patch/264930/
> >
> >  drivers/of/device.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > index 5957cd4fa262..15ffee00fb22 100644
> > --- a/drivers/of/device.c
> > +++ b/drivers/of/device.c
> > @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> >         return device_add(&ofdev->dev);
> >  }
> >
> > +static const struct of_device_id iommu_blacklist[] = {
> > +       { .compatible = "qcom,mdp4" },
> > +       { .compatible = "qcom,mdss" },
> > +       { .compatible = "qcom,sdm845-mdss" },
> > +       { .compatible = "qcom,adreno" },
> > +       {}
> > +};
>
> Not completely clear to whether this is still needed or not, but this
> really won't scale. Why can't the driver for these devices override
> whatever has been setup by default?
>

fwiw, at the moment it is not needed, but it will become needed again
to implement per-context pagetables (although I suppose for this we
only need to blacklist qcom,adreno and not also the display nodes).

The reason is that in the current state the core code creates the
first domain before the driver has a chance to intervene and tell it
not to.  And this results that driver ends up using a different
context bank on the iommu than what the firmware expects.

I guess the alternative is to put some property in DT.. but that
doesn't really feel right.  I guess there aren't really many (or any?)
other drivers that have this specific problem, so I don't really
expect it to be a scaling problem.

Yeah, it's a bit ugly, but I'll take a small ugly working hack, over
elegant but non-working any day ;-)... but if someone has a better
idea then I'm all ears.

BR,
-R

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Rob Clark]

On Fri, May 10, 2019 at 7:35 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Dec 4, 2018 at 2:29 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Sat, Dec 1, 2018 at 10:54 AM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > This solves a problem we see with drm/msm, caused by getting
> > > iommu_dma_ops while we attach our own domain and manage it directly at
> > > the iommu API level:
> > >
> > >   [0000000000000038] user address but active_mm is swapper
> > >   Internal error: Oops: 96000005 [#1] PREEMPT SMP
> > >   Modules linked in:
> > >   CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
> > >   Hardware name: xxx (DT)
> > >   Workqueue: events deferred_probe_work_func
> > >   pstate: 80c00009 (Nzcv daif +PAN +UAO)
> > >   pc : iommu_dma_map_sg+0x7c/0x2c8
> > >   lr : iommu_dma_map_sg+0x40/0x2c8
> > >   sp : ffffff80095eb4f0
> > >   x29: ffffff80095eb4f0 x28: 0000000000000000
> > >   x27: ffffffc0f9431578 x26: 0000000000000000
> > >   x25: 00000000ffffffff x24: 0000000000000003
> > >   x23: 0000000000000001 x22: ffffffc0fa9ac010
> > >   x21: 0000000000000000 x20: ffffffc0fab40980
> > >   x19: ffffffc0fab40980 x18: 0000000000000003
> > >   x17: 00000000000001c4 x16: 0000000000000007
> > >   x15: 000000000000000e x14: ffffffffffffffff
> > >   x13: ffff000000000000 x12: 0000000000000028
> > >   x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> > >   x9 : 0000000000000000 x8 : ffffffc0fab409a0
> > >   x7 : 0000000000000000 x6 : 0000000000000002
> > >   x5 : 0000000100000000 x4 : 0000000000000000
> > >   x3 : 0000000000000001 x2 : 0000000000000002
> > >   x1 : ffffffc0f9431578 x0 : 0000000000000000
> > >   Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
> > >   Call trace:
> > >    iommu_dma_map_sg+0x7c/0x2c8
> > >    __iommu_map_sg_attrs+0x70/0x84
> > >    get_pages+0x170/0x1e8
> > >    msm_gem_get_iova+0x8c/0x128
> > >    _msm_gem_kernel_new+0x6c/0xc8
> > >    msm_gem_kernel_new+0x4c/0x58
> > >    dsi_tx_buf_alloc_6g+0x4c/0x8c
> > >    msm_dsi_host_modeset_init+0xc8/0x108
> > >    msm_dsi_modeset_init+0x54/0x18c
> > >    _dpu_kms_drm_obj_init+0x430/0x474
> > >    dpu_kms_hw_init+0x5f8/0x6b4
> > >    msm_drm_bind+0x360/0x6c8
> > >    try_to_bring_up_master.part.7+0x28/0x70
> > >    component_master_add_with_match+0xe8/0x124
> > >    msm_pdev_probe+0x294/0x2b4
> > >    platform_drv_probe+0x58/0xa4
> > >    really_probe+0x150/0x294
> > >    driver_probe_device+0xac/0xe8
> > >    __device_attach_driver+0xa4/0xb4
> > >    bus_for_each_drv+0x98/0xc8
> > >    __device_attach+0xac/0x12c
> > >    device_initial_probe+0x24/0x30
> > >    bus_probe_device+0x38/0x98
> > >    deferred_probe_work_func+0x78/0xa4
> > >    process_one_work+0x24c/0x3dc
> > >    worker_thread+0x280/0x360
> > >    kthread+0x134/0x13c
> > >    ret_from_fork+0x10/0x18
> > >   Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
> > >   ---[ end trace f22dda57f3648e2c ]---
> > >   Kernel panic - not syncing: Fatal exception
> > >   SMP: stopping secondary CPUs
> > >   Kernel Offset: disabled
> > >   CPU features: 0x0,22802a18
> > >   Memory Limit: none
> > >
> > > The problem is that when drm/msm does it's own iommu_attach_device(),
> > > now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> > > domain, and it doesn't have domain->iova_cookie.
> > >
> > > We kind of avoided this problem prior to sdm845/dpu because the iommu
> > > was attached to the mdp node in dt, which is a child of the toplevel
> > > mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> > > with sdm845, now the iommu is attached at the mdss level so we hit the
> > > iommu_dma_ops in dma_map_sg().
> > >
> > > But auto allocating/attaching a domain before the driver is probed was
> > > already a blocking problem for enabling per-context pagetables for the
> > > GPU.  This problem is also now solved with this patch.
> > >
> > > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
> > > Tested-by: Douglas Anderson <dianders@chromium.org>
> > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > > ---
> > > This is an alternative/replacement for [1].  What it lacks in elegance
> > > it makes up for in practicality ;-)
> > >
> > > [1] https://patchwork.freedesktop.org/patch/264930/
> > >
> > >  drivers/of/device.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > > index 5957cd4fa262..15ffee00fb22 100644
> > > --- a/drivers/of/device.c
> > > +++ b/drivers/of/device.c
> > > @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> > >         return device_add(&ofdev->dev);
> > >  }
> > >
> > > +static const struct of_device_id iommu_blacklist[] = {
> > > +       { .compatible = "qcom,mdp4" },
> > > +       { .compatible = "qcom,mdss" },
> > > +       { .compatible = "qcom,sdm845-mdss" },
> > > +       { .compatible = "qcom,adreno" },
> > > +       {}
> > > +};
> >
> > Not completely clear to whether this is still needed or not, but this
> > really won't scale. Why can't the driver for these devices override
> > whatever has been setup by default?
> >
>
> fwiw, at the moment it is not needed, but it will become needed again
> to implement per-context pagetables (although I suppose for this we
> only need to blacklist qcom,adreno and not also the display nodes).

So, another case I've come across, on the display side.. I'm working
on handling the case where bootloader enables display (and takes iommu
out of reset).. as soon as DMA domain gets attached we get iommu
faults, because bootloader has already configured display for scanout.
Unfortunately this all happens before actual driver is probed and has
a chance to intervene.

It's rather unfortunate that we tried to be clever rather than just
making drivers call some function to opt-in to the hookup of dma iommu
ops :-(

BR,
-R

>
> The reason is that in the current state the core code creates the
> first domain before the driver has a chance to intervene and tell it
> not to.  And this results that driver ends up using a different
> context bank on the iommu than what the firmware expects.
>
> I guess the alternative is to put some property in DT.. but that
> doesn't really feel right.  I guess there aren't really many (or any?)
> other drivers that have this specific problem, so I don't really
> expect it to be a scaling problem.
>
> Yeah, it's a bit ugly, but I'll take a small ugly working hack, over
> elegant but non-working any day ;-)... but if someone has a better
> idea then I'm all ears.
>
> BR,
> -R

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Tomasz Figa]

On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Fri, May 10, 2019 at 7:35 AM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Tue, Dec 4, 2018 at 2:29 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Sat, Dec 1, 2018 at 10:54 AM Rob Clark <robdclark@gmail.com> wrote:
> > > >
> > > > This solves a problem we see with drm/msm, caused by getting
> > > > iommu_dma_ops while we attach our own domain and manage it directly at
> > > > the iommu API level:
> > > >
> > > >   [0000000000000038] user address but active_mm is swapper
> > > >   Internal error: Oops: 96000005 [#1] PREEMPT SMP
> > > >   Modules linked in:
> > > >   CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
> > > >   Hardware name: xxx (DT)
> > > >   Workqueue: events deferred_probe_work_func
> > > >   pstate: 80c00009 (Nzcv daif +PAN +UAO)
> > > >   pc : iommu_dma_map_sg+0x7c/0x2c8
> > > >   lr : iommu_dma_map_sg+0x40/0x2c8
> > > >   sp : ffffff80095eb4f0
> > > >   x29: ffffff80095eb4f0 x28: 0000000000000000
> > > >   x27: ffffffc0f9431578 x26: 0000000000000000
> > > >   x25: 00000000ffffffff x24: 0000000000000003
> > > >   x23: 0000000000000001 x22: ffffffc0fa9ac010
> > > >   x21: 0000000000000000 x20: ffffffc0fab40980
> > > >   x19: ffffffc0fab40980 x18: 0000000000000003
> > > >   x17: 00000000000001c4 x16: 0000000000000007
> > > >   x15: 000000000000000e x14: ffffffffffffffff
> > > >   x13: ffff000000000000 x12: 0000000000000028
> > > >   x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> > > >   x9 : 0000000000000000 x8 : ffffffc0fab409a0
> > > >   x7 : 0000000000000000 x6 : 0000000000000002
> > > >   x5 : 0000000100000000 x4 : 0000000000000000
> > > >   x3 : 0000000000000001 x2 : 0000000000000002
> > > >   x1 : ffffffc0f9431578 x0 : 0000000000000000
> > > >   Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
> > > >   Call trace:
> > > >    iommu_dma_map_sg+0x7c/0x2c8
> > > >    __iommu_map_sg_attrs+0x70/0x84
> > > >    get_pages+0x170/0x1e8
> > > >    msm_gem_get_iova+0x8c/0x128
> > > >    _msm_gem_kernel_new+0x6c/0xc8
> > > >    msm_gem_kernel_new+0x4c/0x58
> > > >    dsi_tx_buf_alloc_6g+0x4c/0x8c
> > > >    msm_dsi_host_modeset_init+0xc8/0x108
> > > >    msm_dsi_modeset_init+0x54/0x18c
> > > >    _dpu_kms_drm_obj_init+0x430/0x474
> > > >    dpu_kms_hw_init+0x5f8/0x6b4
> > > >    msm_drm_bind+0x360/0x6c8
> > > >    try_to_bring_up_master.part.7+0x28/0x70
> > > >    component_master_add_with_match+0xe8/0x124
> > > >    msm_pdev_probe+0x294/0x2b4
> > > >    platform_drv_probe+0x58/0xa4
> > > >    really_probe+0x150/0x294
> > > >    driver_probe_device+0xac/0xe8
> > > >    __device_attach_driver+0xa4/0xb4
> > > >    bus_for_each_drv+0x98/0xc8
> > > >    __device_attach+0xac/0x12c
> > > >    device_initial_probe+0x24/0x30
> > > >    bus_probe_device+0x38/0x98
> > > >    deferred_probe_work_func+0x78/0xa4
> > > >    process_one_work+0x24c/0x3dc
> > > >    worker_thread+0x280/0x360
> > > >    kthread+0x134/0x13c
> > > >    ret_from_fork+0x10/0x18
> > > >   Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
> > > >   ---[ end trace f22dda57f3648e2c ]---
> > > >   Kernel panic - not syncing: Fatal exception
> > > >   SMP: stopping secondary CPUs
> > > >   Kernel Offset: disabled
> > > >   CPU features: 0x0,22802a18
> > > >   Memory Limit: none
> > > >
> > > > The problem is that when drm/msm does it's own iommu_attach_device(),
> > > > now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> > > > domain, and it doesn't have domain->iova_cookie.
> > > >
> > > > We kind of avoided this problem prior to sdm845/dpu because the iommu
> > > > was attached to the mdp node in dt, which is a child of the toplevel
> > > > mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> > > > with sdm845, now the iommu is attached at the mdss level so we hit the
> > > > iommu_dma_ops in dma_map_sg().
> > > >
> > > > But auto allocating/attaching a domain before the driver is probed was
> > > > already a blocking problem for enabling per-context pagetables for the
> > > > GPU.  This problem is also now solved with this patch.
> > > >
> > > > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
> > > > Tested-by: Douglas Anderson <dianders@chromium.org>
> > > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > > > ---
> > > > This is an alternative/replacement for [1].  What it lacks in elegance
> > > > it makes up for in practicality ;-)
> > > >
> > > > [1] https://patchwork.freedesktop.org/patch/264930/
> > > >
> > > >  drivers/of/device.c | 22 ++++++++++++++++++++++
> > > >  1 file changed, 22 insertions(+)
> > > >
> > > > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > > > index 5957cd4fa262..15ffee00fb22 100644
> > > > --- a/drivers/of/device.c
> > > > +++ b/drivers/of/device.c
> > > > @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> > > >         return device_add(&ofdev->dev);
> > > >  }
> > > >
> > > > +static const struct of_device_id iommu_blacklist[] = {
> > > > +       { .compatible = "qcom,mdp4" },
> > > > +       { .compatible = "qcom,mdss" },
> > > > +       { .compatible = "qcom,sdm845-mdss" },
> > > > +       { .compatible = "qcom,adreno" },
> > > > +       {}
> > > > +};
> > >
> > > Not completely clear to whether this is still needed or not, but this
> > > really won't scale. Why can't the driver for these devices override
> > > whatever has been setup by default?
> > >
> >
> > fwiw, at the moment it is not needed, but it will become needed again
> > to implement per-context pagetables (although I suppose for this we
> > only need to blacklist qcom,adreno and not also the display nodes).
>
> So, another case I've come across, on the display side.. I'm working
> on handling the case where bootloader enables display (and takes iommu
> out of reset).. as soon as DMA domain gets attached we get iommu
> faults, because bootloader has already configured display for scanout.
> Unfortunately this all happens before actual driver is probed and has
> a chance to intervene.
>
> It's rather unfortunate that we tried to be clever rather than just
> making drivers call some function to opt-in to the hookup of dma iommu
> ops :-(

I think it still works for the 90% of cases and if 10% needs some
explicit work in the drivers, that's better than requiring 100% of the
drivers to do things manually.

Adding Marek who had the same problem on Exynos.

Best regards,
Tomasz

>
> BR,
> -R
>
> >
> > The reason is that in the current state the core code creates the
> > first domain before the driver has a chance to intervene and tell it
> > not to.  And this results that driver ends up using a different
> > context bank on the iommu than what the firmware expects.
> >
> > I guess the alternative is to put some property in DT.. but that
> > doesn't really feel right.  I guess there aren't really many (or any?)
> > other drivers that have this specific problem, so I don't really
> > expect it to be a scaling problem.
> >
> > Yeah, it's a bit ugly, but I'll take a small ugly working hack, over
> > elegant but non-working any day ;-)... but if someone has a better
> > idea then I'm all ears.
> >
> > BR,
> > -R

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Vivek Gautam]

On 6/3/2019 11:50 AM, Tomasz Figa wrote:
> On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
>> On Fri, May 10, 2019 at 7:35 AM Rob Clark <robdclark@gmail.com> wrote:
>>> On Tue, Dec 4, 2018 at 2:29 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>> On Sat, Dec 1, 2018 at 10:54 AM Rob Clark <robdclark@gmail.com> wrote:
>>>>> This solves a problem we see with drm/msm, caused by getting
>>>>> iommu_dma_ops while we attach our own domain and manage it directly at
>>>>> the iommu API level:
>>>>>
>>>>>    [0000000000000038] user address but active_mm is swapper
>>>>>    Internal error: Oops: 96000005 [#1] PREEMPT SMP
>>>>>    Modules linked in:
>>>>>    CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
>>>>>    Hardware name: xxx (DT)
>>>>>    Workqueue: events deferred_probe_work_func
>>>>>    pstate: 80c00009 (Nzcv daif +PAN +UAO)
>>>>>    pc : iommu_dma_map_sg+0x7c/0x2c8
>>>>>    lr : iommu_dma_map_sg+0x40/0x2c8
>>>>>    sp : ffffff80095eb4f0
>>>>>    x29: ffffff80095eb4f0 x28: 0000000000000000
>>>>>    x27: ffffffc0f9431578 x26: 0000000000000000
>>>>>    x25: 00000000ffffffff x24: 0000000000000003
>>>>>    x23: 0000000000000001 x22: ffffffc0fa9ac010
>>>>>    x21: 0000000000000000 x20: ffffffc0fab40980
>>>>>    x19: ffffffc0fab40980 x18: 0000000000000003
>>>>>    x17: 00000000000001c4 x16: 0000000000000007
>>>>>    x15: 000000000000000e x14: ffffffffffffffff
>>>>>    x13: ffff000000000000 x12: 0000000000000028
>>>>>    x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
>>>>>    x9 : 0000000000000000 x8 : ffffffc0fab409a0
>>>>>    x7 : 0000000000000000 x6 : 0000000000000002
>>>>>    x5 : 0000000100000000 x4 : 0000000000000000
>>>>>    x3 : 0000000000000001 x2 : 0000000000000002
>>>>>    x1 : ffffffc0f9431578 x0 : 0000000000000000
>>>>>    Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
>>>>>    Call trace:
>>>>>     iommu_dma_map_sg+0x7c/0x2c8
>>>>>     __iommu_map_sg_attrs+0x70/0x84
>>>>>     get_pages+0x170/0x1e8
>>>>>     msm_gem_get_iova+0x8c/0x128
>>>>>     _msm_gem_kernel_new+0x6c/0xc8
>>>>>     msm_gem_kernel_new+0x4c/0x58
>>>>>     dsi_tx_buf_alloc_6g+0x4c/0x8c
>>>>>     msm_dsi_host_modeset_init+0xc8/0x108
>>>>>     msm_dsi_modeset_init+0x54/0x18c
>>>>>     _dpu_kms_drm_obj_init+0x430/0x474
>>>>>     dpu_kms_hw_init+0x5f8/0x6b4
>>>>>     msm_drm_bind+0x360/0x6c8
>>>>>     try_to_bring_up_master.part.7+0x28/0x70
>>>>>     component_master_add_with_match+0xe8/0x124
>>>>>     msm_pdev_probe+0x294/0x2b4
>>>>>     platform_drv_probe+0x58/0xa4
>>>>>     really_probe+0x150/0x294
>>>>>     driver_probe_device+0xac/0xe8
>>>>>     __device_attach_driver+0xa4/0xb4
>>>>>     bus_for_each_drv+0x98/0xc8
>>>>>     __device_attach+0xac/0x12c
>>>>>     device_initial_probe+0x24/0x30
>>>>>     bus_probe_device+0x38/0x98
>>>>>     deferred_probe_work_func+0x78/0xa4
>>>>>     process_one_work+0x24c/0x3dc
>>>>>     worker_thread+0x280/0x360
>>>>>     kthread+0x134/0x13c
>>>>>     ret_from_fork+0x10/0x18
>>>>>    Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
>>>>>    ---[ end trace f22dda57f3648e2c ]---
>>>>>    Kernel panic - not syncing: Fatal exception
>>>>>    SMP: stopping secondary CPUs
>>>>>    Kernel Offset: disabled
>>>>>    CPU features: 0x0,22802a18
>>>>>    Memory Limit: none
>>>>>
>>>>> The problem is that when drm/msm does it's own iommu_attach_device(),
>>>>> now the domain returned by iommu_get_domain_for_dev() is drm/msm's
>>>>> domain, and it doesn't have domain->iova_cookie.
>>>>>
>>>>> We kind of avoided this problem prior to sdm845/dpu because the iommu
>>>>> was attached to the mdp node in dt, which is a child of the toplevel
>>>>> mdss node (which corresponds to the dev passed in dma_map_sg()).  But
>>>>> with sdm845, now the iommu is attached at the mdss level so we hit the
>>>>> iommu_dma_ops in dma_map_sg().
>>>>>
>>>>> But auto allocating/attaching a domain before the driver is probed was
>>>>> already a blocking problem for enabling per-context pagetables for the
>>>>> GPU.  This problem is also now solved with this patch.
>>>>>
>>>>> Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
>>>>> Tested-by: Douglas Anderson <dianders@chromium.org>
>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>> ---
>>>>> This is an alternative/replacement for [1].  What it lacks in elegance
>>>>> it makes up for in practicality ;-)
>>>>>
>>>>> [1] https://patchwork.freedesktop.org/patch/264930/
>>>>>
>>>>>   drivers/of/device.c | 22 ++++++++++++++++++++++
>>>>>   1 file changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>>>> index 5957cd4fa262..15ffee00fb22 100644
>>>>> --- a/drivers/of/device.c
>>>>> +++ b/drivers/of/device.c
>>>>> @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
>>>>>          return device_add(&ofdev->dev);
>>>>>   }
>>>>>
>>>>> +static const struct of_device_id iommu_blacklist[] = {
>>>>> +       { .compatible = "qcom,mdp4" },
>>>>> +       { .compatible = "qcom,mdss" },
>>>>> +       { .compatible = "qcom,sdm845-mdss" },
>>>>> +       { .compatible = "qcom,adreno" },
>>>>> +       {}
>>>>> +};
>>>> Not completely clear to whether this is still needed or not, but this
>>>> really won't scale. Why can't the driver for these devices override
>>>> whatever has been setup by default?
>>>>
>>> fwiw, at the moment it is not needed, but it will become needed again
>>> to implement per-context pagetables (although I suppose for this we
>>> only need to blacklist qcom,adreno and not also the display nodes).
>> So, another case I've come across, on the display side.. I'm working
>> on handling the case where bootloader enables display (and takes iommu
>> out of reset).. as soon as DMA domain gets attached we get iommu
>> faults, because bootloader has already configured display for scanout.
>> Unfortunately this all happens before actual driver is probed and has
>> a chance to intervene.

Things are bad for MTP sdm845 too where the bootloader sets up iommu to
display splash screen, and when the kernel resets the iommu, the mappings go
for a toss resulting in fatal faults.
Bjorn was working on something recently to address this. Adding him to 
the thread.


Best regards
Vivek

>> It's rather unfortunate that we tried to be clever rather than just
>> making drivers call some function to opt-in to the hookup of dma iommu
>> ops :-(
> I think it still works for the 90% of cases and if 10% needs some
> explicit work in the drivers, that's better than requiring 100% of the
> drivers to do things manually.
>
> Adding Marek who had the same problem on Exynos.
>
> Best regards,
> Tomasz
>
>> BR,
>> -R
>>
>>> The reason is that in the current state the core code creates the
>>> first domain before the driver has a chance to intervene and tell it
>>> not to.  And this results that driver ends up using a different
>>> context bank on the iommu than what the firmware expects.
>>>
>>> I guess the alternative is to put some property in DT.. but that
>>> doesn't really feel right.  I guess there aren't really many (or any?)
>>> other drivers that have this specific problem, so I don't really
>>> expect it to be a scaling problem.
>>>
>>> Yeah, it's a bit ugly, but I'll take a small ugly working hack, over
>>> elegant but non-working any day ;-)... but if someone has a better
>>> idea then I'm all ears.
>>>
>>> BR,
>>> -R

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Rob Clark]

On Mon, Jun 3, 2019 at 12:57 AM Vivek Gautam
<vivek.gautam@codeaurora.org> wrote:
>
>
>
> On 6/3/2019 11:50 AM, Tomasz Figa wrote:
> > On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
> >> On Fri, May 10, 2019 at 7:35 AM Rob Clark <robdclark@gmail.com> wrote:
> >>> On Tue, Dec 4, 2018 at 2:29 PM Rob Herring <robh+dt@kernel.org> wrote:
> >>>> On Sat, Dec 1, 2018 at 10:54 AM Rob Clark <robdclark@gmail.com> wrote:
> >>>>> This solves a problem we see with drm/msm, caused by getting
> >>>>> iommu_dma_ops while we attach our own domain and manage it directly at
> >>>>> the iommu API level:
> >>>>>
> >>>>>    [0000000000000038] user address but active_mm is swapper
> >>>>>    Internal error: Oops: 96000005 [#1] PREEMPT SMP
> >>>>>    Modules linked in:
> >>>>>    CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
> >>>>>    Hardware name: xxx (DT)
> >>>>>    Workqueue: events deferred_probe_work_func
> >>>>>    pstate: 80c00009 (Nzcv daif +PAN +UAO)
> >>>>>    pc : iommu_dma_map_sg+0x7c/0x2c8
> >>>>>    lr : iommu_dma_map_sg+0x40/0x2c8
> >>>>>    sp : ffffff80095eb4f0
> >>>>>    x29: ffffff80095eb4f0 x28: 0000000000000000
> >>>>>    x27: ffffffc0f9431578 x26: 0000000000000000
> >>>>>    x25: 00000000ffffffff x24: 0000000000000003
> >>>>>    x23: 0000000000000001 x22: ffffffc0fa9ac010
> >>>>>    x21: 0000000000000000 x20: ffffffc0fab40980
> >>>>>    x19: ffffffc0fab40980 x18: 0000000000000003
> >>>>>    x17: 00000000000001c4 x16: 0000000000000007
> >>>>>    x15: 000000000000000e x14: ffffffffffffffff
> >>>>>    x13: ffff000000000000 x12: 0000000000000028
> >>>>>    x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> >>>>>    x9 : 0000000000000000 x8 : ffffffc0fab409a0
> >>>>>    x7 : 0000000000000000 x6 : 0000000000000002
> >>>>>    x5 : 0000000100000000 x4 : 0000000000000000
> >>>>>    x3 : 0000000000000001 x2 : 0000000000000002
> >>>>>    x1 : ffffffc0f9431578 x0 : 0000000000000000
> >>>>>    Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
> >>>>>    Call trace:
> >>>>>     iommu_dma_map_sg+0x7c/0x2c8
> >>>>>     __iommu_map_sg_attrs+0x70/0x84
> >>>>>     get_pages+0x170/0x1e8
> >>>>>     msm_gem_get_iova+0x8c/0x128
> >>>>>     _msm_gem_kernel_new+0x6c/0xc8
> >>>>>     msm_gem_kernel_new+0x4c/0x58
> >>>>>     dsi_tx_buf_alloc_6g+0x4c/0x8c
> >>>>>     msm_dsi_host_modeset_init+0xc8/0x108
> >>>>>     msm_dsi_modeset_init+0x54/0x18c
> >>>>>     _dpu_kms_drm_obj_init+0x430/0x474
> >>>>>     dpu_kms_hw_init+0x5f8/0x6b4
> >>>>>     msm_drm_bind+0x360/0x6c8
> >>>>>     try_to_bring_up_master.part.7+0x28/0x70
> >>>>>     component_master_add_with_match+0xe8/0x124
> >>>>>     msm_pdev_probe+0x294/0x2b4
> >>>>>     platform_drv_probe+0x58/0xa4
> >>>>>     really_probe+0x150/0x294
> >>>>>     driver_probe_device+0xac/0xe8
> >>>>>     __device_attach_driver+0xa4/0xb4
> >>>>>     bus_for_each_drv+0x98/0xc8
> >>>>>     __device_attach+0xac/0x12c
> >>>>>     device_initial_probe+0x24/0x30
> >>>>>     bus_probe_device+0x38/0x98
> >>>>>     deferred_probe_work_func+0x78/0xa4
> >>>>>     process_one_work+0x24c/0x3dc
> >>>>>     worker_thread+0x280/0x360
> >>>>>     kthread+0x134/0x13c
> >>>>>     ret_from_fork+0x10/0x18
> >>>>>    Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
> >>>>>    ---[ end trace f22dda57f3648e2c ]---
> >>>>>    Kernel panic - not syncing: Fatal exception
> >>>>>    SMP: stopping secondary CPUs
> >>>>>    Kernel Offset: disabled
> >>>>>    CPU features: 0x0,22802a18
> >>>>>    Memory Limit: none
> >>>>>
> >>>>> The problem is that when drm/msm does it's own iommu_attach_device(),
> >>>>> now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> >>>>> domain, and it doesn't have domain->iova_cookie.
> >>>>>
> >>>>> We kind of avoided this problem prior to sdm845/dpu because the iommu
> >>>>> was attached to the mdp node in dt, which is a child of the toplevel
> >>>>> mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> >>>>> with sdm845, now the iommu is attached at the mdss level so we hit the
> >>>>> iommu_dma_ops in dma_map_sg().
> >>>>>
> >>>>> But auto allocating/attaching a domain before the driver is probed was
> >>>>> already a blocking problem for enabling per-context pagetables for the
> >>>>> GPU.  This problem is also now solved with this patch.
> >>>>>
> >>>>> Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
> >>>>> Tested-by: Douglas Anderson <dianders@chromium.org>
> >>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
> >>>>> ---
> >>>>> This is an alternative/replacement for [1].  What it lacks in elegance
> >>>>> it makes up for in practicality ;-)
> >>>>>
> >>>>> [1] https://patchwork.freedesktop.org/patch/264930/
> >>>>>
> >>>>>   drivers/of/device.c | 22 ++++++++++++++++++++++
> >>>>>   1 file changed, 22 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
> >>>>> index 5957cd4fa262..15ffee00fb22 100644
> >>>>> --- a/drivers/of/device.c
> >>>>> +++ b/drivers/of/device.c
> >>>>> @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> >>>>>          return device_add(&ofdev->dev);
> >>>>>   }
> >>>>>
> >>>>> +static const struct of_device_id iommu_blacklist[] = {
> >>>>> +       { .compatible = "qcom,mdp4" },
> >>>>> +       { .compatible = "qcom,mdss" },
> >>>>> +       { .compatible = "qcom,sdm845-mdss" },
> >>>>> +       { .compatible = "qcom,adreno" },
> >>>>> +       {}
> >>>>> +};
> >>>> Not completely clear to whether this is still needed or not, but this
> >>>> really won't scale. Why can't the driver for these devices override
> >>>> whatever has been setup by default?
> >>>>
> >>> fwiw, at the moment it is not needed, but it will become needed again
> >>> to implement per-context pagetables (although I suppose for this we
> >>> only need to blacklist qcom,adreno and not also the display nodes).
> >> So, another case I've come across, on the display side.. I'm working
> >> on handling the case where bootloader enables display (and takes iommu
> >> out of reset).. as soon as DMA domain gets attached we get iommu
> >> faults, because bootloader has already configured display for scanout.
> >> Unfortunately this all happens before actual driver is probed and has
> >> a chance to intervene.
>
> Things are bad for MTP sdm845 too where the bootloader sets up iommu to
> display splash screen, and when the kernel resets the iommu, the mappings go
> for a toss resulting in fatal faults.
> Bjorn was working on something recently to address this. Adding him to
> the thread.
>

yeah, I was hitting it on the yoga c630 laptop, but it is the identical problem.

We'd worked around it so far with a hack in arm-smmu to temporarily
ioremap the display controller block and disable INTF1, which is
*defn* not a good solution ;-)

BR,
-R

>
> Best regards
> Vivek
>
> >> It's rather unfortunate that we tried to be clever rather than just
> >> making drivers call some function to opt-in to the hookup of dma iommu
> >> ops :-(
> > I think it still works for the 90% of cases and if 10% needs some
> > explicit work in the drivers, that's better than requiring 100% of the
> > drivers to do things manually.
> >
> > Adding Marek who had the same problem on Exynos.
> >
> > Best regards,
> > Tomasz
> >
> >> BR,
> >> -R
> >>
> >>> The reason is that in the current state the core code creates the
> >>> first domain before the driver has a chance to intervene and tell it
> >>> not to.  And this results that driver ends up using a different
> >>> context bank on the iommu than what the firmware expects.
> >>>
> >>> I guess the alternative is to put some property in DT.. but that
> >>> doesn't really feel right.  I guess there aren't really many (or any?)
> >>> other drivers that have this specific problem, so I don't really
> >>> expect it to be a scaling problem.
> >>>
> >>> Yeah, it's a bit ugly, but I'll take a small ugly working hack, over
> >>> elegant but non-working any day ;-)... but if someone has a better
> >>> idea then I'm all ears.
> >>>
> >>> BR,
> >>> -R
>

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Rob Clark]

On Sun, Jun 2, 2019 at 11:25 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Fri, May 10, 2019 at 7:35 AM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Tue, Dec 4, 2018 at 2:29 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Sat, Dec 1, 2018 at 10:54 AM Rob Clark <robdclark@gmail.com> wrote:
> > > > >
> > > > > This solves a problem we see with drm/msm, caused by getting
> > > > > iommu_dma_ops while we attach our own domain and manage it directly at
> > > > > the iommu API level:
> > > > >
> > > > >   [0000000000000038] user address but active_mm is swapper
> > > > >   Internal error: Oops: 96000005 [#1] PREEMPT SMP
> > > > >   Modules linked in:
> > > > >   CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
> > > > >   Hardware name: xxx (DT)
> > > > >   Workqueue: events deferred_probe_work_func
> > > > >   pstate: 80c00009 (Nzcv daif +PAN +UAO)
> > > > >   pc : iommu_dma_map_sg+0x7c/0x2c8
> > > > >   lr : iommu_dma_map_sg+0x40/0x2c8
> > > > >   sp : ffffff80095eb4f0
> > > > >   x29: ffffff80095eb4f0 x28: 0000000000000000
> > > > >   x27: ffffffc0f9431578 x26: 0000000000000000
> > > > >   x25: 00000000ffffffff x24: 0000000000000003
> > > > >   x23: 0000000000000001 x22: ffffffc0fa9ac010
> > > > >   x21: 0000000000000000 x20: ffffffc0fab40980
> > > > >   x19: ffffffc0fab40980 x18: 0000000000000003
> > > > >   x17: 00000000000001c4 x16: 0000000000000007
> > > > >   x15: 000000000000000e x14: ffffffffffffffff
> > > > >   x13: ffff000000000000 x12: 0000000000000028
> > > > >   x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> > > > >   x9 : 0000000000000000 x8 : ffffffc0fab409a0
> > > > >   x7 : 0000000000000000 x6 : 0000000000000002
> > > > >   x5 : 0000000100000000 x4 : 0000000000000000
> > > > >   x3 : 0000000000000001 x2 : 0000000000000002
> > > > >   x1 : ffffffc0f9431578 x0 : 0000000000000000
> > > > >   Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
> > > > >   Call trace:
> > > > >    iommu_dma_map_sg+0x7c/0x2c8
> > > > >    __iommu_map_sg_attrs+0x70/0x84
> > > > >    get_pages+0x170/0x1e8
> > > > >    msm_gem_get_iova+0x8c/0x128
> > > > >    _msm_gem_kernel_new+0x6c/0xc8
> > > > >    msm_gem_kernel_new+0x4c/0x58
> > > > >    dsi_tx_buf_alloc_6g+0x4c/0x8c
> > > > >    msm_dsi_host_modeset_init+0xc8/0x108
> > > > >    msm_dsi_modeset_init+0x54/0x18c
> > > > >    _dpu_kms_drm_obj_init+0x430/0x474
> > > > >    dpu_kms_hw_init+0x5f8/0x6b4
> > > > >    msm_drm_bind+0x360/0x6c8
> > > > >    try_to_bring_up_master.part.7+0x28/0x70
> > > > >    component_master_add_with_match+0xe8/0x124
> > > > >    msm_pdev_probe+0x294/0x2b4
> > > > >    platform_drv_probe+0x58/0xa4
> > > > >    really_probe+0x150/0x294
> > > > >    driver_probe_device+0xac/0xe8
> > > > >    __device_attach_driver+0xa4/0xb4
> > > > >    bus_for_each_drv+0x98/0xc8
> > > > >    __device_attach+0xac/0x12c
> > > > >    device_initial_probe+0x24/0x30
> > > > >    bus_probe_device+0x38/0x98
> > > > >    deferred_probe_work_func+0x78/0xa4
> > > > >    process_one_work+0x24c/0x3dc
> > > > >    worker_thread+0x280/0x360
> > > > >    kthread+0x134/0x13c
> > > > >    ret_from_fork+0x10/0x18
> > > > >   Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
> > > > >   ---[ end trace f22dda57f3648e2c ]---
> > > > >   Kernel panic - not syncing: Fatal exception
> > > > >   SMP: stopping secondary CPUs
> > > > >   Kernel Offset: disabled
> > > > >   CPU features: 0x0,22802a18
> > > > >   Memory Limit: none
> > > > >
> > > > > The problem is that when drm/msm does it's own iommu_attach_device(),
> > > > > now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> > > > > domain, and it doesn't have domain->iova_cookie.
> > > > >
> > > > > We kind of avoided this problem prior to sdm845/dpu because the iommu
> > > > > was attached to the mdp node in dt, which is a child of the toplevel
> > > > > mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> > > > > with sdm845, now the iommu is attached at the mdss level so we hit the
> > > > > iommu_dma_ops in dma_map_sg().
> > > > >
> > > > > But auto allocating/attaching a domain before the driver is probed was
> > > > > already a blocking problem for enabling per-context pagetables for the
> > > > > GPU.  This problem is also now solved with this patch.
> > > > >
> > > > > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
> > > > > Tested-by: Douglas Anderson <dianders@chromium.org>
> > > > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > > > > ---
> > > > > This is an alternative/replacement for [1].  What it lacks in elegance
> > > > > it makes up for in practicality ;-)
> > > > >
> > > > > [1] https://patchwork.freedesktop.org/patch/264930/
> > > > >
> > > > >  drivers/of/device.c | 22 ++++++++++++++++++++++
> > > > >  1 file changed, 22 insertions(+)
> > > > >
> > > > > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > > > > index 5957cd4fa262..15ffee00fb22 100644
> > > > > --- a/drivers/of/device.c
> > > > > +++ b/drivers/of/device.c
> > > > > @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> > > > >         return device_add(&ofdev->dev);
> > > > >  }
> > > > >
> > > > > +static const struct of_device_id iommu_blacklist[] = {
> > > > > +       { .compatible = "qcom,mdp4" },
> > > > > +       { .compatible = "qcom,mdss" },
> > > > > +       { .compatible = "qcom,sdm845-mdss" },
> > > > > +       { .compatible = "qcom,adreno" },
> > > > > +       {}
> > > > > +};
> > > >
> > > > Not completely clear to whether this is still needed or not, but this
> > > > really won't scale. Why can't the driver for these devices override
> > > > whatever has been setup by default?
> > > >
> > >
> > > fwiw, at the moment it is not needed, but it will become needed again
> > > to implement per-context pagetables (although I suppose for this we
> > > only need to blacklist qcom,adreno and not also the display nodes).
> >
> > So, another case I've come across, on the display side.. I'm working
> > on handling the case where bootloader enables display (and takes iommu
> > out of reset).. as soon as DMA domain gets attached we get iommu
> > faults, because bootloader has already configured display for scanout.
> > Unfortunately this all happens before actual driver is probed and has
> > a chance to intervene.
> >
> > It's rather unfortunate that we tried to be clever rather than just
> > making drivers call some function to opt-in to the hookup of dma iommu
> > ops :-(
>
> I think it still works for the 90% of cases and if 10% needs some
> explicit work in the drivers, that's better than requiring 100% of the
> drivers to do things manually.
>
> Adding Marek who had the same problem on Exynos.

I do wonder how many drivers need to iommu_map in their ->probe()?
I'm thinking moving the auto-hookup to after a successful probe(),
with some function a driver could call if they need mapping in probe,
might be a way to eventually get rid of the blacklist.  But I've no
idea how to find the subset of drivers that would be broken without a
dma_setup_iommu_stuff() call in their probe.

BR,
-R

> Best regards,
> Tomasz
>
> >
> > BR,
> > -R
> >
> > >
> > > The reason is that in the current state the core code creates the
> > > first domain before the driver has a chance to intervene and tell it
> > > not to.  And this results that driver ends up using a different
> > > context bank on the iommu than what the firmware expects.
> > >
> > > I guess the alternative is to put some property in DT.. but that
> > > doesn't really feel right.  I guess there aren't really many (or any?)
> > > other drivers that have this specific problem, so I don't really
> > > expect it to be a scaling problem.
> > >
> > > Yeah, it's a bit ugly, but I'll take a small ugly working hack, over
> > > elegant but non-working any day ;-)... but if someone has a better
> > > idea then I'm all ears.
> > >
> > > BR,
> > > -R

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Vivek Gautam]

On Mon, Jun 3, 2019 at 4:14 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Jun 3, 2019 at 12:57 AM Vivek Gautam
> <vivek.gautam@codeaurora.org> wrote:
> >
> >
> >
> > On 6/3/2019 11:50 AM, Tomasz Figa wrote:
> > > On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
> > >> On Fri, May 10, 2019 at 7:35 AM Rob Clark <robdclark@gmail.com> wrote:
> > >>> On Tue, Dec 4, 2018 at 2:29 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >>>> On Sat, Dec 1, 2018 at 10:54 AM Rob Clark <robdclark@gmail.com> wrote:
> > >>>>> This solves a problem we see with drm/msm, caused by getting
> > >>>>> iommu_dma_ops while we attach our own domain and manage it directly at
> > >>>>> the iommu API level:
> > >>>>>
> > >>>>>    [0000000000000038] user address but active_mm is swapper
> > >>>>>    Internal error: Oops: 96000005 [#1] PREEMPT SMP
> > >>>>>    Modules linked in:
> > >>>>>    CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
> > >>>>>    Hardware name: xxx (DT)
> > >>>>>    Workqueue: events deferred_probe_work_func
> > >>>>>    pstate: 80c00009 (Nzcv daif +PAN +UAO)
> > >>>>>    pc : iommu_dma_map_sg+0x7c/0x2c8
> > >>>>>    lr : iommu_dma_map_sg+0x40/0x2c8
> > >>>>>    sp : ffffff80095eb4f0
> > >>>>>    x29: ffffff80095eb4f0 x28: 0000000000000000
> > >>>>>    x27: ffffffc0f9431578 x26: 0000000000000000
> > >>>>>    x25: 00000000ffffffff x24: 0000000000000003
> > >>>>>    x23: 0000000000000001 x22: ffffffc0fa9ac010
> > >>>>>    x21: 0000000000000000 x20: ffffffc0fab40980
> > >>>>>    x19: ffffffc0fab40980 x18: 0000000000000003
> > >>>>>    x17: 00000000000001c4 x16: 0000000000000007
> > >>>>>    x15: 000000000000000e x14: ffffffffffffffff
> > >>>>>    x13: ffff000000000000 x12: 0000000000000028
> > >>>>>    x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> > >>>>>    x9 : 0000000000000000 x8 : ffffffc0fab409a0
> > >>>>>    x7 : 0000000000000000 x6 : 0000000000000002
> > >>>>>    x5 : 0000000100000000 x4 : 0000000000000000
> > >>>>>    x3 : 0000000000000001 x2 : 0000000000000002
> > >>>>>    x1 : ffffffc0f9431578 x0 : 0000000000000000
> > >>>>>    Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
> > >>>>>    Call trace:
> > >>>>>     iommu_dma_map_sg+0x7c/0x2c8
> > >>>>>     __iommu_map_sg_attrs+0x70/0x84
> > >>>>>     get_pages+0x170/0x1e8
> > >>>>>     msm_gem_get_iova+0x8c/0x128
> > >>>>>     _msm_gem_kernel_new+0x6c/0xc8
> > >>>>>     msm_gem_kernel_new+0x4c/0x58
> > >>>>>     dsi_tx_buf_alloc_6g+0x4c/0x8c
> > >>>>>     msm_dsi_host_modeset_init+0xc8/0x108
> > >>>>>     msm_dsi_modeset_init+0x54/0x18c
> > >>>>>     _dpu_kms_drm_obj_init+0x430/0x474
> > >>>>>     dpu_kms_hw_init+0x5f8/0x6b4
> > >>>>>     msm_drm_bind+0x360/0x6c8
> > >>>>>     try_to_bring_up_master.part.7+0x28/0x70
> > >>>>>     component_master_add_with_match+0xe8/0x124
> > >>>>>     msm_pdev_probe+0x294/0x2b4
> > >>>>>     platform_drv_probe+0x58/0xa4
> > >>>>>     really_probe+0x150/0x294
> > >>>>>     driver_probe_device+0xac/0xe8
> > >>>>>     __device_attach_driver+0xa4/0xb4
> > >>>>>     bus_for_each_drv+0x98/0xc8
> > >>>>>     __device_attach+0xac/0x12c
> > >>>>>     device_initial_probe+0x24/0x30
> > >>>>>     bus_probe_device+0x38/0x98
> > >>>>>     deferred_probe_work_func+0x78/0xa4
> > >>>>>     process_one_work+0x24c/0x3dc
> > >>>>>     worker_thread+0x280/0x360
> > >>>>>     kthread+0x134/0x13c
> > >>>>>     ret_from_fork+0x10/0x18
> > >>>>>    Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
> > >>>>>    ---[ end trace f22dda57f3648e2c ]---
> > >>>>>    Kernel panic - not syncing: Fatal exception
> > >>>>>    SMP: stopping secondary CPUs
> > >>>>>    Kernel Offset: disabled
> > >>>>>    CPU features: 0x0,22802a18
> > >>>>>    Memory Limit: none
> > >>>>>
> > >>>>> The problem is that when drm/msm does it's own iommu_attach_device(),
> > >>>>> now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> > >>>>> domain, and it doesn't have domain->iova_cookie.
> > >>>>>
> > >>>>> We kind of avoided this problem prior to sdm845/dpu because the iommu
> > >>>>> was attached to the mdp node in dt, which is a child of the toplevel
> > >>>>> mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> > >>>>> with sdm845, now the iommu is attached at the mdss level so we hit the
> > >>>>> iommu_dma_ops in dma_map_sg().
> > >>>>>
> > >>>>> But auto allocating/attaching a domain before the driver is probed was
> > >>>>> already a blocking problem for enabling per-context pagetables for the
> > >>>>> GPU.  This problem is also now solved with this patch.
> > >>>>>
> > >>>>> Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
> > >>>>> Tested-by: Douglas Anderson <dianders@chromium.org>
> > >>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
> > >>>>> ---
> > >>>>> This is an alternative/replacement for [1].  What it lacks in elegance
> > >>>>> it makes up for in practicality ;-)
> > >>>>>
> > >>>>> [1] https://patchwork.freedesktop.org/patch/264930/
> > >>>>>
> > >>>>>   drivers/of/device.c | 22 ++++++++++++++++++++++
> > >>>>>   1 file changed, 22 insertions(+)
> > >>>>>
> > >>>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
> > >>>>> index 5957cd4fa262..15ffee00fb22 100644
> > >>>>> --- a/drivers/of/device.c
> > >>>>> +++ b/drivers/of/device.c
> > >>>>> @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> > >>>>>          return device_add(&ofdev->dev);
> > >>>>>   }
> > >>>>>
> > >>>>> +static const struct of_device_id iommu_blacklist[] = {
> > >>>>> +       { .compatible = "qcom,mdp4" },
> > >>>>> +       { .compatible = "qcom,mdss" },
> > >>>>> +       { .compatible = "qcom,sdm845-mdss" },
> > >>>>> +       { .compatible = "qcom,adreno" },
> > >>>>> +       {}
> > >>>>> +};
> > >>>> Not completely clear to whether this is still needed or not, but this
> > >>>> really won't scale. Why can't the driver for these devices override
> > >>>> whatever has been setup by default?
> > >>>>
> > >>> fwiw, at the moment it is not needed, but it will become needed again
> > >>> to implement per-context pagetables (although I suppose for this we
> > >>> only need to blacklist qcom,adreno and not also the display nodes).
> > >> So, another case I've come across, on the display side.. I'm working
> > >> on handling the case where bootloader enables display (and takes iommu
> > >> out of reset).. as soon as DMA domain gets attached we get iommu
> > >> faults, because bootloader has already configured display for scanout.
> > >> Unfortunately this all happens before actual driver is probed and has
> > >> a chance to intervene.
> >
> > Things are bad for MTP sdm845 too where the bootloader sets up iommu to
> > display splash screen, and when the kernel resets the iommu, the mappings go
> > for a toss resulting in fatal faults.
> > Bjorn was working on something recently to address this. Adding him to
> > the thread.
> >
>
> yeah, I was hitting it on the yoga c630 laptop, but it is the identical problem.
>
> We'd worked around it so far with a hack in arm-smmu to temporarily
> ioremap the display controller block and disable INTF1, which is
> *defn* not a good solution ;-)

Right, but this doesn't always work for MTP. I believe Bjorn has something
more inline with downstream.

Regards
>
> BR,
> -R
>
> >
> > Best regards
> > Vivek
> >
> > >> It's rather unfortunate that we tried to be clever rather than just
> > >> making drivers call some function to opt-in to the hookup of dma iommu
> > >> ops :-(
> > > I think it still works for the 90% of cases and if 10% needs some
> > > explicit work in the drivers, that's better than requiring 100% of the
> > > drivers to do things manually.
> > >
> > > Adding Marek who had the same problem on Exynos.
> > >
> > > Best regards,
> > > Tomasz
> > >
> > >> BR,
> > >> -R
> > >>
> > >>> The reason is that in the current state the core code creates the
> > >>> first domain before the driver has a chance to intervene and tell it
> > >>> not to.  And this results that driver ends up using a different
> > >>> context bank on the iommu than what the firmware expects.
> > >>>
> > >>> I guess the alternative is to put some property in DT.. but that
> > >>> doesn't really feel right.  I guess there aren't really many (or any?)
> > >>> other drivers that have this specific problem, so I don't really
> > >>> expect it to be a scaling problem.
> > >>>
> > >>> Yeah, it's a bit ugly, but I'll take a small ugly working hack, over
> > >>> elegant but non-working any day ;-)... but if someone has a better
> > >>> idea then I'm all ears.
> > >>>
> > >>> BR,
> > >>> -R
> >
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Robin Murphy]

On 03/06/2019 11:47, Rob Clark wrote:
> On Sun, Jun 2, 2019 at 11:25 PM Tomasz Figa <tfiga@chromium.org> wrote:
>>
>> On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
>>>
>>> On Fri, May 10, 2019 at 7:35 AM Rob Clark <robdclark@gmail.com> wrote:
>>>>
>>>> On Tue, Dec 4, 2018 at 2:29 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>>>
>>>>> On Sat, Dec 1, 2018 at 10:54 AM Rob Clark <robdclark@gmail.com> wrote:
>>>>>>
>>>>>> This solves a problem we see with drm/msm, caused by getting
>>>>>> iommu_dma_ops while we attach our own domain and manage it directly at
>>>>>> the iommu API level:
>>>>>>
>>>>>>    [0000000000000038] user address but active_mm is swapper
>>>>>>    Internal error: Oops: 96000005 [#1] PREEMPT SMP
>>>>>>    Modules linked in:
>>>>>>    CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
>>>>>>    Hardware name: xxx (DT)
>>>>>>    Workqueue: events deferred_probe_work_func
>>>>>>    pstate: 80c00009 (Nzcv daif +PAN +UAO)
>>>>>>    pc : iommu_dma_map_sg+0x7c/0x2c8
>>>>>>    lr : iommu_dma_map_sg+0x40/0x2c8
>>>>>>    sp : ffffff80095eb4f0
>>>>>>    x29: ffffff80095eb4f0 x28: 0000000000000000
>>>>>>    x27: ffffffc0f9431578 x26: 0000000000000000
>>>>>>    x25: 00000000ffffffff x24: 0000000000000003
>>>>>>    x23: 0000000000000001 x22: ffffffc0fa9ac010
>>>>>>    x21: 0000000000000000 x20: ffffffc0fab40980
>>>>>>    x19: ffffffc0fab40980 x18: 0000000000000003
>>>>>>    x17: 00000000000001c4 x16: 0000000000000007
>>>>>>    x15: 000000000000000e x14: ffffffffffffffff
>>>>>>    x13: ffff000000000000 x12: 0000000000000028
>>>>>>    x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
>>>>>>    x9 : 0000000000000000 x8 : ffffffc0fab409a0
>>>>>>    x7 : 0000000000000000 x6 : 0000000000000002
>>>>>>    x5 : 0000000100000000 x4 : 0000000000000000
>>>>>>    x3 : 0000000000000001 x2 : 0000000000000002
>>>>>>    x1 : ffffffc0f9431578 x0 : 0000000000000000
>>>>>>    Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
>>>>>>    Call trace:
>>>>>>     iommu_dma_map_sg+0x7c/0x2c8
>>>>>>     __iommu_map_sg_attrs+0x70/0x84
>>>>>>     get_pages+0x170/0x1e8
>>>>>>     msm_gem_get_iova+0x8c/0x128
>>>>>>     _msm_gem_kernel_new+0x6c/0xc8
>>>>>>     msm_gem_kernel_new+0x4c/0x58
>>>>>>     dsi_tx_buf_alloc_6g+0x4c/0x8c
>>>>>>     msm_dsi_host_modeset_init+0xc8/0x108
>>>>>>     msm_dsi_modeset_init+0x54/0x18c
>>>>>>     _dpu_kms_drm_obj_init+0x430/0x474
>>>>>>     dpu_kms_hw_init+0x5f8/0x6b4
>>>>>>     msm_drm_bind+0x360/0x6c8
>>>>>>     try_to_bring_up_master.part.7+0x28/0x70
>>>>>>     component_master_add_with_match+0xe8/0x124
>>>>>>     msm_pdev_probe+0x294/0x2b4
>>>>>>     platform_drv_probe+0x58/0xa4
>>>>>>     really_probe+0x150/0x294
>>>>>>     driver_probe_device+0xac/0xe8
>>>>>>     __device_attach_driver+0xa4/0xb4
>>>>>>     bus_for_each_drv+0x98/0xc8
>>>>>>     __device_attach+0xac/0x12c
>>>>>>     device_initial_probe+0x24/0x30
>>>>>>     bus_probe_device+0x38/0x98
>>>>>>     deferred_probe_work_func+0x78/0xa4
>>>>>>     process_one_work+0x24c/0x3dc
>>>>>>     worker_thread+0x280/0x360
>>>>>>     kthread+0x134/0x13c
>>>>>>     ret_from_fork+0x10/0x18
>>>>>>    Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
>>>>>>    ---[ end trace f22dda57f3648e2c ]---
>>>>>>    Kernel panic - not syncing: Fatal exception
>>>>>>    SMP: stopping secondary CPUs
>>>>>>    Kernel Offset: disabled
>>>>>>    CPU features: 0x0,22802a18
>>>>>>    Memory Limit: none
>>>>>>
>>>>>> The problem is that when drm/msm does it's own iommu_attach_device(),
>>>>>> now the domain returned by iommu_get_domain_for_dev() is drm/msm's
>>>>>> domain, and it doesn't have domain->iova_cookie.
>>>>>>
>>>>>> We kind of avoided this problem prior to sdm845/dpu because the iommu
>>>>>> was attached to the mdp node in dt, which is a child of the toplevel
>>>>>> mdss node (which corresponds to the dev passed in dma_map_sg()).  But
>>>>>> with sdm845, now the iommu is attached at the mdss level so we hit the
>>>>>> iommu_dma_ops in dma_map_sg().
>>>>>>
>>>>>> But auto allocating/attaching a domain before the driver is probed was
>>>>>> already a blocking problem for enabling per-context pagetables for the
>>>>>> GPU.  This problem is also now solved with this patch.
>>>>>>
>>>>>> Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
>>>>>> Tested-by: Douglas Anderson <dianders@chromium.org>
>>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>>> ---
>>>>>> This is an alternative/replacement for [1].  What it lacks in elegance
>>>>>> it makes up for in practicality ;-)
>>>>>>
>>>>>> [1] https://patchwork.freedesktop.org/patch/264930/
>>>>>>
>>>>>>   drivers/of/device.c | 22 ++++++++++++++++++++++
>>>>>>   1 file changed, 22 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>>>>> index 5957cd4fa262..15ffee00fb22 100644
>>>>>> --- a/drivers/of/device.c
>>>>>> +++ b/drivers/of/device.c
>>>>>> @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
>>>>>>          return device_add(&ofdev->dev);
>>>>>>   }
>>>>>>
>>>>>> +static const struct of_device_id iommu_blacklist[] = {
>>>>>> +       { .compatible = "qcom,mdp4" },
>>>>>> +       { .compatible = "qcom,mdss" },
>>>>>> +       { .compatible = "qcom,sdm845-mdss" },
>>>>>> +       { .compatible = "qcom,adreno" },
>>>>>> +       {}
>>>>>> +};
>>>>>
>>>>> Not completely clear to whether this is still needed or not, but this
>>>>> really won't scale. Why can't the driver for these devices override
>>>>> whatever has been setup by default?
>>>>>
>>>>
>>>> fwiw, at the moment it is not needed, but it will become needed again
>>>> to implement per-context pagetables (although I suppose for this we
>>>> only need to blacklist qcom,adreno and not also the display nodes).
>>>
>>> So, another case I've come across, on the display side.. I'm working
>>> on handling the case where bootloader enables display (and takes iommu
>>> out of reset).. as soon as DMA domain gets attached we get iommu
>>> faults, because bootloader has already configured display for scanout.
>>> Unfortunately this all happens before actual driver is probed and has
>>> a chance to intervene.
>>>
>>> It's rather unfortunate that we tried to be clever rather than just
>>> making drivers call some function to opt-in to the hookup of dma iommu
>>> ops :-(
>>
>> I think it still works for the 90% of cases and if 10% needs some
>> explicit work in the drivers, that's better than requiring 100% of the
>> drivers to do things manually.

Right, it's not about "being clever", it's about not adding opt-in code 
to the hundreds and hundreds and hundreds of drivers which *might* ever 
find themselves used on a system where they would need the IOMMU's help 
for DMA.

>> Adding Marek who had the same problem on Exynos.
> 
> I do wonder how many drivers need to iommu_map in their ->probe()?
> I'm thinking moving the auto-hookup to after a successful probe(),
> with some function a driver could call if they need mapping in probe,
> might be a way to eventually get rid of the blacklist.  But I've no
> idea how to find the subset of drivers that would be broken without a
> dma_setup_iommu_stuff() call in their probe.

Wouldn't help much. That particular issue is nothing to do with DMA ops 
really, it's about IOMMU initialisation. On something like SMMUv3 there 
is literally no way to turn the thing on without blocking unknown 
traffic - it *has* to have stream table entries programmed before it can 
even allow stuff to bypass.

The answer is either to either pay attention to the "Quiesce all DMA 
capable devices" part of the boot protocol (which has been there since 
pretty much forever), or to come up with some robust way of 
communicating "live" boot-time mappings to IOMMU drivers so that they 
can program themselves appropriately during probe.

Robin.

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Christoph Hellwig]

If you (and a few others actors in the thread) want people to actually
read what you wrote please follow proper mailing list ettiquette.  I've
given up on reading all the recent mails after scrolling through two
pages of full quotes.

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Vivek Gautam]

On Mon, Jun 3, 2019 at 4:47 PM Christoph Hellwig <hch@lst.de> wrote:
>
> If you (and a few others actors in the thread) want people to actually
> read what you wrote please follow proper mailing list ettiquette.  I've
> given up on reading all the recent mails after scrolling through two
> pages of full quotes.

Apologies for not cutting down the quoted text. I will be more careful
next time onwards.

Regards
Vivek

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Rob Clark]

On Mon, Jun 3, 2019 at 4:14 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 03/06/2019 11:47, Rob Clark wrote:
> > On Sun, Jun 2, 2019 at 11:25 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >>
> >> On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
> >>>
> >>> So, another case I've come across, on the display side.. I'm working
> >>> on handling the case where bootloader enables display (and takes iommu
> >>> out of reset).. as soon as DMA domain gets attached we get iommu
> >>> faults, because bootloader has already configured display for scanout.
> >>> Unfortunately this all happens before actual driver is probed and has
> >>> a chance to intervene.
> >>>
> >>> It's rather unfortunate that we tried to be clever rather than just
> >>> making drivers call some function to opt-in to the hookup of dma iommu
> >>> ops :-(
> >>
> >> I think it still works for the 90% of cases and if 10% needs some
> >> explicit work in the drivers, that's better than requiring 100% of the
> >> drivers to do things manually.
>
> Right, it's not about "being clever", it's about not adding opt-in code
> to the hundreds and hundreds and hundreds of drivers which *might* ever
> find themselves used on a system where they would need the IOMMU's help
> for DMA.

Well, I mean, at one point we didn't do the automatic iommu hookup, we
could have just stuck with that and added a helper so drivers could
request the hookup.  Things wouldn't have been any more broken than
before, and when people bring up systems where iommu is required, they
could have added the necessary dma_iommu_configure() call.  But that
is water under the bridge now.

> >> Adding Marek who had the same problem on Exynos.
> >
> > I do wonder how many drivers need to iommu_map in their ->probe()?
> > I'm thinking moving the auto-hookup to after a successful probe(),
> > with some function a driver could call if they need mapping in probe,
> > might be a way to eventually get rid of the blacklist.  But I've no
> > idea how to find the subset of drivers that would be broken without a
> > dma_setup_iommu_stuff() call in their probe.
>
> Wouldn't help much. That particular issue is nothing to do with DMA ops
> really, it's about IOMMU initialisation. On something like SMMUv3 there
> is literally no way to turn the thing on without blocking unknown
> traffic - it *has* to have stream table entries programmed before it can
> even allow stuff to bypass.

fwiw, on these sdm850 laptops (and I think sdm845 boards like mtp and
db845c) the SMMU (v2) is taken out of bypass by the bootloader.  Bjorn
has some patches for arm-smmu to read-back the state at boot.

(Although older devices were booting with display enabled but SMMU in bypass.)

> The answer is either to either pay attention to the "Quiesce all DMA
> capable devices" part of the boot protocol (which has been there since
> pretty much forever), or to come up with some robust way of
> communicating "live" boot-time mappings to IOMMU drivers so that they
> can program themselves appropriately during probe.

Unfortunately display lit up by bootloader is basically ubiquitous.
Every single android phone does it.  All of the windows-arm laptops do
it.  Basically 99.9% of things that have a display do it.  It's a
tough problem to solve involving clks, gdsc, regulators, etc, in
addition to the display driver.. and sadly now smmu.  And devices
where we can make changes to and update the firmware are rather rare.
So there is really no option to ignore this problem.

I guess if we had some early-quirks mechanism like x86 does, we could
mash the display off early in boot.  That would be an easy solution.
Although I'd prefer a proper solution so that android phones aren't
carrying around enormous stacks of hack patches to achieve a smooth
flicker-free boot.

I suppose arm-smmu could realize that the context bank is already
taken out of bypass..  although I'm not entirely sure if we can assume
that the CPU would be able to read back the pagetable setup by the
bootloader.  Maybe Vivek has an idea about that?

BR,
-R

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 14355 bytes --]

On Mon, Jun 03, 2019 at 12:14:27PM +0100, Robin Murphy wrote:
> On 03/06/2019 11:47, Rob Clark wrote:
> > On Sun, Jun 2, 2019 at 11:25 PM Tomasz Figa <tfiga@chromium.org> wrote:
> > > 
> > > On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
> > > > 
> > > > On Fri, May 10, 2019 at 7:35 AM Rob Clark <robdclark@gmail.com> wrote:
> > > > > 
> > > > > On Tue, Dec 4, 2018 at 2:29 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > 
> > > > > > On Sat, Dec 1, 2018 at 10:54 AM Rob Clark <robdclark@gmail.com> wrote:
> > > > > > > 
> > > > > > > This solves a problem we see with drm/msm, caused by getting
> > > > > > > iommu_dma_ops while we attach our own domain and manage it directly at
> > > > > > > the iommu API level:
> > > > > > > 
> > > > > > >    [0000000000000038] user address but active_mm is swapper
> > > > > > >    Internal error: Oops: 96000005 [#1] PREEMPT SMP
> > > > > > >    Modules linked in:
> > > > > > >    CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
> > > > > > >    Hardware name: xxx (DT)
> > > > > > >    Workqueue: events deferred_probe_work_func
> > > > > > >    pstate: 80c00009 (Nzcv daif +PAN +UAO)
> > > > > > >    pc : iommu_dma_map_sg+0x7c/0x2c8
> > > > > > >    lr : iommu_dma_map_sg+0x40/0x2c8
> > > > > > >    sp : ffffff80095eb4f0
> > > > > > >    x29: ffffff80095eb4f0 x28: 0000000000000000
> > > > > > >    x27: ffffffc0f9431578 x26: 0000000000000000
> > > > > > >    x25: 00000000ffffffff x24: 0000000000000003
> > > > > > >    x23: 0000000000000001 x22: ffffffc0fa9ac010
> > > > > > >    x21: 0000000000000000 x20: ffffffc0fab40980
> > > > > > >    x19: ffffffc0fab40980 x18: 0000000000000003
> > > > > > >    x17: 00000000000001c4 x16: 0000000000000007
> > > > > > >    x15: 000000000000000e x14: ffffffffffffffff
> > > > > > >    x13: ffff000000000000 x12: 0000000000000028
> > > > > > >    x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> > > > > > >    x9 : 0000000000000000 x8 : ffffffc0fab409a0
> > > > > > >    x7 : 0000000000000000 x6 : 0000000000000002
> > > > > > >    x5 : 0000000100000000 x4 : 0000000000000000
> > > > > > >    x3 : 0000000000000001 x2 : 0000000000000002
> > > > > > >    x1 : ffffffc0f9431578 x0 : 0000000000000000
> > > > > > >    Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
> > > > > > >    Call trace:
> > > > > > >     iommu_dma_map_sg+0x7c/0x2c8
> > > > > > >     __iommu_map_sg_attrs+0x70/0x84
> > > > > > >     get_pages+0x170/0x1e8
> > > > > > >     msm_gem_get_iova+0x8c/0x128
> > > > > > >     _msm_gem_kernel_new+0x6c/0xc8
> > > > > > >     msm_gem_kernel_new+0x4c/0x58
> > > > > > >     dsi_tx_buf_alloc_6g+0x4c/0x8c
> > > > > > >     msm_dsi_host_modeset_init+0xc8/0x108
> > > > > > >     msm_dsi_modeset_init+0x54/0x18c
> > > > > > >     _dpu_kms_drm_obj_init+0x430/0x474
> > > > > > >     dpu_kms_hw_init+0x5f8/0x6b4
> > > > > > >     msm_drm_bind+0x360/0x6c8
> > > > > > >     try_to_bring_up_master.part.7+0x28/0x70
> > > > > > >     component_master_add_with_match+0xe8/0x124
> > > > > > >     msm_pdev_probe+0x294/0x2b4
> > > > > > >     platform_drv_probe+0x58/0xa4
> > > > > > >     really_probe+0x150/0x294
> > > > > > >     driver_probe_device+0xac/0xe8
> > > > > > >     __device_attach_driver+0xa4/0xb4
> > > > > > >     bus_for_each_drv+0x98/0xc8
> > > > > > >     __device_attach+0xac/0x12c
> > > > > > >     device_initial_probe+0x24/0x30
> > > > > > >     bus_probe_device+0x38/0x98
> > > > > > >     deferred_probe_work_func+0x78/0xa4
> > > > > > >     process_one_work+0x24c/0x3dc
> > > > > > >     worker_thread+0x280/0x360
> > > > > > >     kthread+0x134/0x13c
> > > > > > >     ret_from_fork+0x10/0x18
> > > > > > >    Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
> > > > > > >    ---[ end trace f22dda57f3648e2c ]---
> > > > > > >    Kernel panic - not syncing: Fatal exception
> > > > > > >    SMP: stopping secondary CPUs
> > > > > > >    Kernel Offset: disabled
> > > > > > >    CPU features: 0x0,22802a18
> > > > > > >    Memory Limit: none
> > > > > > > 
> > > > > > > The problem is that when drm/msm does it's own iommu_attach_device(),
> > > > > > > now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> > > > > > > domain, and it doesn't have domain->iova_cookie.
> > > > > > > 
> > > > > > > We kind of avoided this problem prior to sdm845/dpu because the iommu
> > > > > > > was attached to the mdp node in dt, which is a child of the toplevel
> > > > > > > mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> > > > > > > with sdm845, now the iommu is attached at the mdss level so we hit the
> > > > > > > iommu_dma_ops in dma_map_sg().
> > > > > > > 
> > > > > > > But auto allocating/attaching a domain before the driver is probed was
> > > > > > > already a blocking problem for enabling per-context pagetables for the
> > > > > > > GPU.  This problem is also now solved with this patch.
> > > > > > > 
> > > > > > > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
> > > > > > > Tested-by: Douglas Anderson <dianders@chromium.org>
> > > > > > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > > > > > > ---
> > > > > > > This is an alternative/replacement for [1].  What it lacks in elegance
> > > > > > > it makes up for in practicality ;-)
> > > > > > > 
> > > > > > > [1] https://patchwork.freedesktop.org/patch/264930/
> > > > > > > 
> > > > > > >   drivers/of/device.c | 22 ++++++++++++++++++++++
> > > > > > >   1 file changed, 22 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > > > > > > index 5957cd4fa262..15ffee00fb22 100644
> > > > > > > --- a/drivers/of/device.c
> > > > > > > +++ b/drivers/of/device.c
> > > > > > > @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> > > > > > >          return device_add(&ofdev->dev);
> > > > > > >   }
> > > > > > > 
> > > > > > > +static const struct of_device_id iommu_blacklist[] = {
> > > > > > > +       { .compatible = "qcom,mdp4" },
> > > > > > > +       { .compatible = "qcom,mdss" },
> > > > > > > +       { .compatible = "qcom,sdm845-mdss" },
> > > > > > > +       { .compatible = "qcom,adreno" },
> > > > > > > +       {}
> > > > > > > +};
> > > > > > 
> > > > > > Not completely clear to whether this is still needed or not, but this
> > > > > > really won't scale. Why can't the driver for these devices override
> > > > > > whatever has been setup by default?
> > > > > > 
> > > > > 
> > > > > fwiw, at the moment it is not needed, but it will become needed again
> > > > > to implement per-context pagetables (although I suppose for this we
> > > > > only need to blacklist qcom,adreno and not also the display nodes).
> > > > 
> > > > So, another case I've come across, on the display side.. I'm working
> > > > on handling the case where bootloader enables display (and takes iommu
> > > > out of reset).. as soon as DMA domain gets attached we get iommu
> > > > faults, because bootloader has already configured display for scanout.
> > > > Unfortunately this all happens before actual driver is probed and has
> > > > a chance to intervene.
> > > > 
> > > > It's rather unfortunate that we tried to be clever rather than just
> > > > making drivers call some function to opt-in to the hookup of dma iommu
> > > > ops :-(
> > > 
> > > I think it still works for the 90% of cases and if 10% needs some
> > > explicit work in the drivers, that's better than requiring 100% of the
> > > drivers to do things manually.
> 
> Right, it's not about "being clever", it's about not adding opt-in code to
> the hundreds and hundreds and hundreds of drivers which *might* ever find
> themselves used on a system where they would need the IOMMU's help for DMA.
> 
> > > Adding Marek who had the same problem on Exynos.
> > 
> > I do wonder how many drivers need to iommu_map in their ->probe()?
> > I'm thinking moving the auto-hookup to after a successful probe(),
> > with some function a driver could call if they need mapping in probe,
> > might be a way to eventually get rid of the blacklist.  But I've no
> > idea how to find the subset of drivers that would be broken without a
> > dma_setup_iommu_stuff() call in their probe.
> 
> Wouldn't help much. That particular issue is nothing to do with DMA ops
> really, it's about IOMMU initialisation. On something like SMMUv3 there is
> literally no way to turn the thing on without blocking unknown traffic - it
> *has* to have stream table entries programmed before it can even allow stuff
> to bypass.
> 
> The answer is either to either pay attention to the "Quiesce all DMA capable
> devices" part of the boot protocol (which has been there since pretty much
> forever), or to come up with some robust way of communicating "live"
> boot-time mappings to IOMMU drivers so that they can program themselves
> appropriately during probe.

I ran into a similar issue not too long ago and I've been working on
what I think is a correct fix for this problem. Unfortunately I went
down the rabbit hole of trying to get all of the pieces in the
bootloader merged before posting the kernel patches, and that's been
taking a long time. Let me chime in here in the hope that it will be
helpful to others.

The problem that I ran into was pretty much the same thing that Rob
encountered. We have some platforms that will initialize display over
HDMI in an early bootloader to show a splash screen. During boot we
would usually take over display hardware by just resetting it and then
programming it from scratch. Ultimately we want to do seamless handover
so that the kernel can take over the splash and replace it by the
console when that's ready. But I'm getting ahead of myself.

One of the things I've been trying to do is replace direct IOMMU API
usage in the Tegra DRM driver by using DMA API only, which we need in
order to fix some corner cases we ran into (I've now forgotten most of
the details and I realize that my commit messages aren't all that
helpful...).

Anyway, when I started using the DMA API I was running into a massive
amount of IOMMU faults starting at early boot. It took me a while to
realize that this was because now the IOMMU was enabled before the
driver had a chance to set up the IOMMU domain. I think the only way
that we were getting around that was because we used to have a custom
IOMMU driver on older Tegra device and don't expose DMA API compatible
IOMMU domains. With newer Tegras using the ARM SMMU we don't really have
that option anymore.

So the root cause of this is that the bootloader initialized the display
controller to scan out from a physical address (the bootloader does not
set up the SMMU) and when the kernel attaches the display controller to
the SMMU during early boot, the display controller ends up trying to
fetch those physical addresses through the SMMU where no mapping for
those addresses exists, hence causing these faults.

The solution that I came up with is to use the reserved-memory bindings
along with memory-region references in the display controller nodes. I
have a patch for the Tegra SMMU driver that implements support for
parsing this data (the IOMMU framework has infrastructure to do this, so
I take it that this has come up before) and setting up 1:1 mappings
right before the SMMU is enabled for a device. This works rather well in
my testing. I've been using hard-coded values because the bootloader
does not properly put the reserved memory regions into the DT. I've been
trying to add that as a side-project, but it turned into a bit of a can
of worms.

These are all standard bindings, so I think we could implement something
similar in the ARM SMMU driver. In fact, I was going to do that once I
had sorted this all out for Tegra SMMU, but I'd be happy if anyone can
beat me to it.

I've attached the patch for Tegra SMMU here. I think pretty much the
same thing could be implement for any other drivers since this is based
on standard bindings. Perhaps there could even be helpers. Actually it
looks like one such helpers already exists:

	iommu_dma_get_resv_regions()

I think the generic code looking up the standard memory-region property
could be added to that to expose this functionality more broadly.

One thing to note is that I have this workaround in the Tegra SMMU
driver to bind the IOMMU domain to a specific instance during this early
direct mappings business. But I think that may no longer be necessary
since there seems to have been some work in this area lately.
Specifically I'm looking at 7423e01741dd6a5f1255f589145313f0fb1c8cbe,
which may help. I can investigate, but let me post the patch first to
keep the discussion going.

Here's a short extract of how I'm using this in device tree:

--- >8 ---
diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2371-2180.dts b/arch/arm64/boot/dts/nvidia/tegra210-p2371-2180.dts
index 5a57396b5948..82c4e0c88740 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210-p2371-2180.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra210-p2371-2180.dts
@@ -8,6 +8,16 @@
        model = "NVIDIA Jetson TX1 Developer Kit";
        compatible = "nvidia,p2371-2180", "nvidia,tegra210";

+       reserved-memory {
+               #address-cells = <2>;
+               #size-cells = <2>;
+               ranges;
+
+               framebuffer: framebuffer@92c9f000 {
+                       reg = <0 0x92c9f000 0 0x00800000>;
+               };
+       };
+
        pcie@1003000 {
                status = "okay";

@@ -35,6 +45,14 @@
        };

        host1x@50000000 {
+               dc@54200000 {
+                       memory-region = <&framebuffer>;
+               };
+
+               dc@54240000 {
+                       memory-region = <&framebuffer>;
+               };
+
                dsi@54300000 {
                        status = "okay";

--- >8 ---

Note that these entries should all be generated at runtime by the
bootloader once it has allocated the framebuffer. That's the can of
worms I've been talking about.

Attached is the SMMU driver patch.

Hope that helps,
Thierry

[-- Attachment #1.2: 0001-iommu-tegra-smmu-Add-support-for-reserved-regions.patch --]
[-- Type: text/plain, Size: 7423 bytes --]

From 1c7c4fcc5364e48fc5390ac17dd7e4ca347a6eb4 Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Fri, 15 Feb 2019 15:16:44 +0100
Subject: [PATCH] iommu/tegra-smmu: Add support for reserved regions

The Tegra DRM driver currently uses the IOMMU API explicitly. This means
that it has fine-grained control over when exactly the translation
through the IOMMU is enabled. This currently happens after the driver
probes, so the driver is in a DMA quiesced state when the IOMMU
translation is enabled.

During the transition of the Tegra DRM driver to use the DMA API instead
of the IOMMU API explicitly, it was observed that on certain platforms
the display controllers were still actively fetching from memory. When a
DMA IOMMU domain is created as part of the DMA/IOMMU API setup during
boot, the IOMMU translation for the display controllers can be enabled a
significant amount of time before the driver has had a chance to reset
the hardware into a sane state. This causes the SMMU to detect faults on
the addresses that the display controller is trying to fetch.

To avoid this, and as a byproduct paving the way for seamless transition
of display from the bootloader to the kernel, add support for reserved
regions in the Tegra SMMU driver. This is implemented using the standard
reserved memory device tree bindings, which let us describe regions of
memory which the kernel is forbidden from using for regular allocations.
The Tegra SMMU driver will parse the nodes associated with each device
via the "memory-region" property and return reserved regions that the
IOMMU core will then create direct mappings for prior to attaching the
IOMMU domains to the devices. This ensures that a 1:1 mapping is in
place when IOMMU translation starts and prevents the SMMU from detecting
any faults.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/iommu/tegra-smmu.c | 115 +++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 8e49869812b0..b96444be325d 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -12,6 +12,7 @@
 #include <linux/iommu.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -545,6 +546,38 @@ static void tegra_smmu_set_pde(struct tegra_smmu_as *as, unsigned long iova,
 	struct tegra_smmu *smmu = as->smmu;
 	u32 *pd = page_address(as->pd);
 	unsigned long offset = pd_index * sizeof(*pd);
+	bool unmap = false;
+
+	/*
+	 * XXX Move this outside of this function. Perhaps add a struct
+	 * iommu_domain parameter to ->{get,put}_resv_regions() so that
+	 * the mapping can be done there.
+	 *
+	 * The problem here is that as->smmu is only known once we attach
+	 * the domain to a device (because then we look up the right SMMU
+	 * instance via the dev->archdata.iommu pointer). When the direct
+	 * mappings are created for reserved regions, the domain has not
+	 * been attached to a device yet, so we don't know. We currently
+	 * fix that up in ->apply_resv_regions() because that is the first
+	 * time where we have access to a struct device that will be used
+	 * with the IOMMU domain. However, that's asymmetric and doesn't
+	 * take care of the page directory mapping either, so we need to
+	 * come up with something better.
+	 */
+	if (as->pd_dma == 0) {
+		as->pd_dma = dma_map_page(smmu->dev, as->pd, 0, SMMU_SIZE_PD,
+					  DMA_TO_DEVICE);
+		if (dma_mapping_error(smmu->dev, as->pd_dma))
+			return;
+
+		if (!smmu_dma_addr_valid(smmu, as->pd_dma)) {
+			dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD,
+				       DMA_TO_DEVICE);
+			return;
+		}
+
+		unmap = true;
+	}
 
 	/* Set the page directory entry first */
 	pd[pd_index] = value;
@@ -557,6 +590,12 @@ static void tegra_smmu_set_pde(struct tegra_smmu_as *as, unsigned long iova,
 	smmu_flush_ptc(smmu, as->pd_dma, offset);
 	smmu_flush_tlb_section(smmu, as->id, iova);
 	smmu_flush(smmu);
+
+	if (unmap) {
+		dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD,
+			       DMA_TO_DEVICE);
+		as->pd_dma = 0;
+	}
 }
 
 static u32 *tegra_smmu_pte_offset(struct page *pt_page, unsigned long iova)
@@ -897,6 +936,79 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 	return group;
 }
 
+static void tegra_smmu_get_resv_regions(struct device *dev, struct list_head *list)
+{
+	struct of_phandle_iterator it;
+	int err;
+
+	if (!dev->of_node)
+		return;
+
+	of_for_each_phandle(&it, err, dev->of_node, "memory-region", NULL, 0) {
+		struct iommu_resv_region *region;
+		struct resource res;
+
+		err = of_address_to_resource(it.node, 0, &res);
+		if (err < 0) {
+			dev_err(dev, "failed to parse memory region %pOFn: %d\n",
+				it.node, err);
+			continue;
+		}
+
+		region = iommu_alloc_resv_region(res.start, resource_size(&res),
+						 IOMMU_READ | IOMMU_WRITE,
+						 IOMMU_RESV_DIRECT);
+		if (!region)
+			return;
+
+		dev_dbg(dev, "reserved region %pR\n", &res);
+		list_add_tail(&region->list, list);
+	}
+}
+
+static void tegra_smmu_put_resv_regions(struct device *dev,
+					struct list_head *list)
+{
+	struct iommu_resv_region *entry, *next;
+
+	list_for_each_entry_safe(entry, next, list, list)
+		kfree(entry);
+}
+
+static void tegra_smmu_apply_resv_region(struct device *dev,
+					 struct iommu_domain *domain,
+					 struct iommu_resv_region *region)
+{
+	struct tegra_smmu *smmu = dev->archdata.iommu;
+	struct tegra_smmu_as *as = to_smmu_as(domain);
+
+	/*
+	 * ->attach_dev() may not have been called yet at this point, so the
+	 * address space may not have been associated with an SMMU instance.
+	 * Set up the association here to make sure subsequent code can rely
+	 * on the SMMU instance being known.
+	 *
+	 * Also make sure that the SMMU instance doesn't conflict if an SMMU
+	 * has been associated with the address space already. This can happen
+	 * if a domain is shared between multiple devices.
+	 *
+	 * Note that this is purely theoretic because there are no known SoCs
+	 * with multiple instances of this SMMU.
+	 *
+	 * XXX Deal with this elsewhere. One possibility would be to pass the
+	 * struct iommu_domain that we're operating on to ->get_resv_regions()
+	 * and ->put_resv_regions() so that the connection between it and the
+	 * struct device (in order to find the SMMU instance) can already be
+	 * established at that time. This would be nicely symmetric because a
+	 * ->put_resv_regions() could undo that again so that ->attach_dev()
+	 * could start from a clean slate.
+	 */
+	if (as->smmu && as->smmu != smmu)
+		WARN(1, "conflicting SMMU instances\n");
+
+	as->smmu = smmu;
+}
+
 static int tegra_smmu_of_xlate(struct device *dev,
 			       struct of_phandle_args *args)
 {
@@ -917,6 +1029,9 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.map = tegra_smmu_map,
 	.unmap = tegra_smmu_unmap,
 	.iova_to_phys = tegra_smmu_iova_to_phys,
+	.get_resv_regions = tegra_smmu_get_resv_regions,
+	.put_resv_regions = tegra_smmu_put_resv_regions,
+	.apply_resv_region = tegra_smmu_apply_resv_region,
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
 };
-- 
2.21.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6217 bytes --]

On Mon, Jun 03, 2019 at 06:20:57AM -0700, Rob Clark wrote:
> On Mon, Jun 3, 2019 at 4:14 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > On 03/06/2019 11:47, Rob Clark wrote:
> > > On Sun, Jun 2, 2019 at 11:25 PM Tomasz Figa <tfiga@chromium.org> wrote:
> > >>
> > >> On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
> > >>>
> > >>> So, another case I've come across, on the display side.. I'm working
> > >>> on handling the case where bootloader enables display (and takes iommu
> > >>> out of reset).. as soon as DMA domain gets attached we get iommu
> > >>> faults, because bootloader has already configured display for scanout.
> > >>> Unfortunately this all happens before actual driver is probed and has
> > >>> a chance to intervene.
> > >>>
> > >>> It's rather unfortunate that we tried to be clever rather than just
> > >>> making drivers call some function to opt-in to the hookup of dma iommu
> > >>> ops :-(
> > >>
> > >> I think it still works for the 90% of cases and if 10% needs some
> > >> explicit work in the drivers, that's better than requiring 100% of the
> > >> drivers to do things manually.
> >
> > Right, it's not about "being clever", it's about not adding opt-in code
> > to the hundreds and hundreds and hundreds of drivers which *might* ever
> > find themselves used on a system where they would need the IOMMU's help
> > for DMA.
> 
> Well, I mean, at one point we didn't do the automatic iommu hookup, we
> could have just stuck with that and added a helper so drivers could
> request the hookup.  Things wouldn't have been any more broken than
> before, and when people bring up systems where iommu is required, they
> could have added the necessary dma_iommu_configure() call.  But that
> is water under the bridge now.
> 
> > >> Adding Marek who had the same problem on Exynos.
> > >
> > > I do wonder how many drivers need to iommu_map in their ->probe()?
> > > I'm thinking moving the auto-hookup to after a successful probe(),
> > > with some function a driver could call if they need mapping in probe,
> > > might be a way to eventually get rid of the blacklist.  But I've no
> > > idea how to find the subset of drivers that would be broken without a
> > > dma_setup_iommu_stuff() call in their probe.
> >
> > Wouldn't help much. That particular issue is nothing to do with DMA ops
> > really, it's about IOMMU initialisation. On something like SMMUv3 there
> > is literally no way to turn the thing on without blocking unknown
> > traffic - it *has* to have stream table entries programmed before it can
> > even allow stuff to bypass.
> 
> fwiw, on these sdm850 laptops (and I think sdm845 boards like mtp and
> db845c) the SMMU (v2) is taken out of bypass by the bootloader.  Bjorn
> has some patches for arm-smmu to read-back the state at boot.
> 
> (Although older devices were booting with display enabled but SMMU in bypass.)
> 
> > The answer is either to either pay attention to the "Quiesce all DMA
> > capable devices" part of the boot protocol (which has been there since
> > pretty much forever), or to come up with some robust way of
> > communicating "live" boot-time mappings to IOMMU drivers so that they
> > can program themselves appropriately during probe.
> 
> Unfortunately display lit up by bootloader is basically ubiquitous.
> Every single android phone does it.  All of the windows-arm laptops do
> it.  Basically 99.9% of things that have a display do it.  It's a
> tough problem to solve involving clks, gdsc, regulators, etc, in
> addition to the display driver.. and sadly now smmu.  And devices
> where we can make changes to and update the firmware are rather rare.
> So there is really no option to ignore this problem.

I think this is going to require at least some degree of cooperation
from the bootloader. See my other thread on that. Unfortunately I think
this is an area where everyone has kind of been doing their own thing
even if standard bindings for this have been around for quite a while
(actually 5 years by now). I suspect that most bootloaders that run
today are not that old, but as always downstream doesn't follow closely
where upstream is guiding.

> I guess if we had some early-quirks mechanism like x86 does, we could
> mash the display off early in boot.  That would be an easy solution.
> Although I'd prefer a proper solution so that android phones aren't
> carrying around enormous stacks of hack patches to achieve a smooth
> flicker-free boot.

The proper solution, I think, is for bootloader and kernel to work
together. Unfortunately that means we'll just have to bite the bullet
and get things fixed across the stack. I think this is just the latest
manifestation of the catch-up that upstream has been playing. Only now
that we're starting to enable all of these features upstream are we
running into interoperability issues.

If upstream had been further along we would've caught these issues way
ahead of time and could've influenced the designs of bootloader much
earlier. Now, unless we get all the vendors to go back and modify 5 year
old code that's going to be difficult.

However, I think Robin has a point here: it's clearly documented in the
boot protocol, so technically bootloaders are buggy and we can't always
go and fix things so that buggy bootloaders continue to work. There's
not a whole lot of incentive for anyone to fix the bootloaders if we
keep doing that, ey?

> I suppose arm-smmu could realize that the context bank is already
> taken out of bypass..  although I'm not entirely sure if we can assume
> that the CPU would be able to read back the pagetable setup by the
> bootloader.  Maybe Vivek has an idea about that?

I've been thinking, though, and I'm not sure I see this issue on newer
Tegras that use the ARM SMMU. I suppose this could be because the
bootloader doesn't enable the SMMU, but then again, I should be getting
these faults even so. Interestingly I don't see those on 64-bit ARM, so
perhaps there's something special about that.

Let me reload a bit of context here so I can give you more than just
speculation.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Rob Clark]

On Mon, Jun 3, 2019 at 6:54 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Mon, Jun 03, 2019 at 06:20:57AM -0700, Rob Clark wrote:
> > On Mon, Jun 3, 2019 at 4:14 AM Robin Murphy <robin.murphy@arm.com> wrote:
> > >
> > > On 03/06/2019 11:47, Rob Clark wrote:
> > > > On Sun, Jun 2, 2019 at 11:25 PM Tomasz Figa <tfiga@chromium.org> wrote:
> > > >>
> > > >> On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
> > > >>>
> > > >>> So, another case I've come across, on the display side.. I'm working
> > > >>> on handling the case where bootloader enables display (and takes iommu
> > > >>> out of reset).. as soon as DMA domain gets attached we get iommu
> > > >>> faults, because bootloader has already configured display for scanout.
> > > >>> Unfortunately this all happens before actual driver is probed and has
> > > >>> a chance to intervene.
> > > >>>
> > > >>> It's rather unfortunate that we tried to be clever rather than just
> > > >>> making drivers call some function to opt-in to the hookup of dma iommu
> > > >>> ops :-(
> > > >>
> > > >> I think it still works for the 90% of cases and if 10% needs some
> > > >> explicit work in the drivers, that's better than requiring 100% of the
> > > >> drivers to do things manually.
> > >
> > > Right, it's not about "being clever", it's about not adding opt-in code
> > > to the hundreds and hundreds and hundreds of drivers which *might* ever
> > > find themselves used on a system where they would need the IOMMU's help
> > > for DMA.
> >
> > Well, I mean, at one point we didn't do the automatic iommu hookup, we
> > could have just stuck with that and added a helper so drivers could
> > request the hookup.  Things wouldn't have been any more broken than
> > before, and when people bring up systems where iommu is required, they
> > could have added the necessary dma_iommu_configure() call.  But that
> > is water under the bridge now.
> >
> > > >> Adding Marek who had the same problem on Exynos.
> > > >
> > > > I do wonder how many drivers need to iommu_map in their ->probe()?
> > > > I'm thinking moving the auto-hookup to after a successful probe(),
> > > > with some function a driver could call if they need mapping in probe,
> > > > might be a way to eventually get rid of the blacklist.  But I've no
> > > > idea how to find the subset of drivers that would be broken without a
> > > > dma_setup_iommu_stuff() call in their probe.
> > >
> > > Wouldn't help much. That particular issue is nothing to do with DMA ops
> > > really, it's about IOMMU initialisation. On something like SMMUv3 there
> > > is literally no way to turn the thing on without blocking unknown
> > > traffic - it *has* to have stream table entries programmed before it can
> > > even allow stuff to bypass.
> >
> > fwiw, on these sdm850 laptops (and I think sdm845 boards like mtp and
> > db845c) the SMMU (v2) is taken out of bypass by the bootloader.  Bjorn
> > has some patches for arm-smmu to read-back the state at boot.
> >
> > (Although older devices were booting with display enabled but SMMU in bypass.)
> >
> > > The answer is either to either pay attention to the "Quiesce all DMA
> > > capable devices" part of the boot protocol (which has been there since
> > > pretty much forever), or to come up with some robust way of
> > > communicating "live" boot-time mappings to IOMMU drivers so that they
> > > can program themselves appropriately during probe.
> >
> > Unfortunately display lit up by bootloader is basically ubiquitous.
> > Every single android phone does it.  All of the windows-arm laptops do
> > it.  Basically 99.9% of things that have a display do it.  It's a
> > tough problem to solve involving clks, gdsc, regulators, etc, in
> > addition to the display driver.. and sadly now smmu.  And devices
> > where we can make changes to and update the firmware are rather rare.
> > So there is really no option to ignore this problem.
>
> I think this is going to require at least some degree of cooperation
> from the bootloader. See my other thread on that. Unfortunately I think
> this is an area where everyone has kind of been doing their own thing
> even if standard bindings for this have been around for quite a while
> (actually 5 years by now). I suspect that most bootloaders that run
> today are not that old, but as always downstream doesn't follow closely
> where upstream is guiding.
>
> > I guess if we had some early-quirks mechanism like x86 does, we could
> > mash the display off early in boot.  That would be an easy solution.
> > Although I'd prefer a proper solution so that android phones aren't
> > carrying around enormous stacks of hack patches to achieve a smooth
> > flicker-free boot.
>
> The proper solution, I think, is for bootloader and kernel to work
> together. Unfortunately that means we'll just have to bite the bullet
> and get things fixed across the stack. I think this is just the latest
> manifestation of the catch-up that upstream has been playing. Only now
> that we're starting to enable all of these features upstream are we
> running into interoperability issues.
>
> If upstream had been further along we would've caught these issues way
> ahead of time and could've influenced the designs of bootloader much
> earlier. Now, unless we get all the vendors to go back and modify 5 year
> old code that's going to be difficult.
>
> However, I think Robin has a point here: it's clearly documented in the
> boot protocol, so technically bootloaders are buggy and we can't always
> go and fix things so that buggy bootloaders continue to work. There's
> not a whole lot of incentive for anyone to fix the bootloaders if we
> keep doing that, ey?
>

A couple notes:

1) The odds of getting new bootloaders for 5yr old phones is basically
   none.. and they are typically signed so we couldn't just write our
   own even if we wanted to.

2) The windows arm laptops shipping actually have "real" UEFI+ACPI..
   for now we've been using device-tree to get linux booting on them.
   But I think we are going to need to shift to ACPI eventually.. so
   a dt specific solution isn't super helpful.

   But we do have EFI GOP to get the address of the boot framebuffer,
   and I believe there is a reserved memory region setup for it.
   Not sure how to connect that to the iommu subsys.

3) The automatic attach of DMA domain is also causing a different
   problem for us on the GPU side, preventing us from supporting per-
   context pagetables (since we end up with a disagreement about
   which context bank is used between arm-smmu and the firmware).

I'm kinda glad that x86 folks were more pragmatic about getting linux
to work on actual hardware, not just restricting things to hw that
looked the way they wanted it too.. at some point in arch/arm64 we are
going to have to decide that reality is a thing.  Ignoring that is
only going to force users and distros to downstream kernels.

BR,
-R

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 8670 bytes --]

On Mon, Jun 03, 2019 at 07:20:14AM -0700, Rob Clark wrote:
> On Mon, Jun 3, 2019 at 6:54 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Mon, Jun 03, 2019 at 06:20:57AM -0700, Rob Clark wrote:
> > > On Mon, Jun 3, 2019 at 4:14 AM Robin Murphy <robin.murphy@arm.com> wrote:
> > > >
> > > > On 03/06/2019 11:47, Rob Clark wrote:
> > > > > On Sun, Jun 2, 2019 at 11:25 PM Tomasz Figa <tfiga@chromium.org> wrote:
> > > > >>
> > > > >> On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
> > > > >>>
> > > > >>> So, another case I've come across, on the display side.. I'm working
> > > > >>> on handling the case where bootloader enables display (and takes iommu
> > > > >>> out of reset).. as soon as DMA domain gets attached we get iommu
> > > > >>> faults, because bootloader has already configured display for scanout.
> > > > >>> Unfortunately this all happens before actual driver is probed and has
> > > > >>> a chance to intervene.
> > > > >>>
> > > > >>> It's rather unfortunate that we tried to be clever rather than just
> > > > >>> making drivers call some function to opt-in to the hookup of dma iommu
> > > > >>> ops :-(
> > > > >>
> > > > >> I think it still works for the 90% of cases and if 10% needs some
> > > > >> explicit work in the drivers, that's better than requiring 100% of the
> > > > >> drivers to do things manually.
> > > >
> > > > Right, it's not about "being clever", it's about not adding opt-in code
> > > > to the hundreds and hundreds and hundreds of drivers which *might* ever
> > > > find themselves used on a system where they would need the IOMMU's help
> > > > for DMA.
> > >
> > > Well, I mean, at one point we didn't do the automatic iommu hookup, we
> > > could have just stuck with that and added a helper so drivers could
> > > request the hookup.  Things wouldn't have been any more broken than
> > > before, and when people bring up systems where iommu is required, they
> > > could have added the necessary dma_iommu_configure() call.  But that
> > > is water under the bridge now.
> > >
> > > > >> Adding Marek who had the same problem on Exynos.
> > > > >
> > > > > I do wonder how many drivers need to iommu_map in their ->probe()?
> > > > > I'm thinking moving the auto-hookup to after a successful probe(),
> > > > > with some function a driver could call if they need mapping in probe,
> > > > > might be a way to eventually get rid of the blacklist.  But I've no
> > > > > idea how to find the subset of drivers that would be broken without a
> > > > > dma_setup_iommu_stuff() call in their probe.
> > > >
> > > > Wouldn't help much. That particular issue is nothing to do with DMA ops
> > > > really, it's about IOMMU initialisation. On something like SMMUv3 there
> > > > is literally no way to turn the thing on without blocking unknown
> > > > traffic - it *has* to have stream table entries programmed before it can
> > > > even allow stuff to bypass.
> > >
> > > fwiw, on these sdm850 laptops (and I think sdm845 boards like mtp and
> > > db845c) the SMMU (v2) is taken out of bypass by the bootloader.  Bjorn
> > > has some patches for arm-smmu to read-back the state at boot.
> > >
> > > (Although older devices were booting with display enabled but SMMU in bypass.)
> > >
> > > > The answer is either to either pay attention to the "Quiesce all DMA
> > > > capable devices" part of the boot protocol (which has been there since
> > > > pretty much forever), or to come up with some robust way of
> > > > communicating "live" boot-time mappings to IOMMU drivers so that they
> > > > can program themselves appropriately during probe.
> > >
> > > Unfortunately display lit up by bootloader is basically ubiquitous.
> > > Every single android phone does it.  All of the windows-arm laptops do
> > > it.  Basically 99.9% of things that have a display do it.  It's a
> > > tough problem to solve involving clks, gdsc, regulators, etc, in
> > > addition to the display driver.. and sadly now smmu.  And devices
> > > where we can make changes to and update the firmware are rather rare.
> > > So there is really no option to ignore this problem.
> >
> > I think this is going to require at least some degree of cooperation
> > from the bootloader. See my other thread on that. Unfortunately I think
> > this is an area where everyone has kind of been doing their own thing
> > even if standard bindings for this have been around for quite a while
> > (actually 5 years by now). I suspect that most bootloaders that run
> > today are not that old, but as always downstream doesn't follow closely
> > where upstream is guiding.
> >
> > > I guess if we had some early-quirks mechanism like x86 does, we could
> > > mash the display off early in boot.  That would be an easy solution.
> > > Although I'd prefer a proper solution so that android phones aren't
> > > carrying around enormous stacks of hack patches to achieve a smooth
> > > flicker-free boot.
> >
> > The proper solution, I think, is for bootloader and kernel to work
> > together. Unfortunately that means we'll just have to bite the bullet
> > and get things fixed across the stack. I think this is just the latest
> > manifestation of the catch-up that upstream has been playing. Only now
> > that we're starting to enable all of these features upstream are we
> > running into interoperability issues.
> >
> > If upstream had been further along we would've caught these issues way
> > ahead of time and could've influenced the designs of bootloader much
> > earlier. Now, unless we get all the vendors to go back and modify 5 year
> > old code that's going to be difficult.
> >
> > However, I think Robin has a point here: it's clearly documented in the
> > boot protocol, so technically bootloaders are buggy and we can't always
> > go and fix things so that buggy bootloaders continue to work. There's
> > not a whole lot of incentive for anyone to fix the bootloaders if we
> > keep doing that, ey?
> >
> 
> A couple notes:
> 
> 1) The odds of getting new bootloaders for 5yr old phones is basically
>    none.. and they are typically signed so we couldn't just write our
>    own even if we wanted to.
> 
> 2) The windows arm laptops shipping actually have "real" UEFI+ACPI..
>    for now we've been using device-tree to get linux booting on them.
>    But I think we are going to need to shift to ACPI eventually.. so
>    a dt specific solution isn't super helpful.
> 
>    But we do have EFI GOP to get the address of the boot framebuffer,
>    and I believe there is a reserved memory region setup for it.
>    Not sure how to connect that to the iommu subsys.

It shouldn't be a problem to hook something else up to the IOMMU
subsystem. Hopefully it's something that people are going to standardize
on.

> 3) The automatic attach of DMA domain is also causing a different
>    problem for us on the GPU side, preventing us from supporting per-
>    context pagetables (since we end up with a disagreement about
>    which context bank is used between arm-smmu and the firmware).

I'm not sure I understand this issue. Is the context bank hard-coded in
the firmware somehow? Or is it possible to rewrite which one is going to
be used at runtime? Do you switch out the actual page tables rather than
the IOMMU domains for context switching?

> I'm kinda glad that x86 folks were more pragmatic about getting linux
> to work on actual hardware, not just restricting things to hw that
> looked the way they wanted it too.. at some point in arch/arm64 we are
> going to have to decide that reality is a thing.  Ignoring that is
> only going to force users and distros to downstream kernels.

You're comparing apples to oranges here. On x86 at least there was some
standardization when Linux started, whereas we still don't really have
that on ARM after so many years of efforts to standardize. I think we
are slowly getting there, but this particular instance shows that we're
not there yet.

Don't get me wrong, I'm not trying to say that we should just ignore
everything that's out there just because it may not be the way we want
it to be. On the other hand if we just take everything as-is and try to
implement workarounds and quirks every step of the way that's going to
also take away a lot of the resources that are already pretty scarce as
it is. I think it needs to be a reasonable compromise.

Also, there doesn't really seem to be a standardizing force in the Linux
on ARM world, so who's going to do that if not the Linux community?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Jordan Crouse]

> It shouldn't be a problem to hook something else up to the IOMMU
> subsystem. Hopefully it's something that people are going to standardize
> on.
> 
> > 3) The automatic attach of DMA domain is also causing a different
> >    problem for us on the GPU side, preventing us from supporting per-
> >    context pagetables (since we end up with a disagreement about
> >    which context bank is used between arm-smmu and the firmware).
> 
> I'm not sure I understand this issue. Is the context bank hard-coded in
> the firmware somehow? Or is it possible to rewrite which one is going to
> be used at runtime? Do you switch out the actual page tables rather than
> the IOMMU domains for context switching?
 
We have a rather long history on this but the tl;dr is that the GPU microcode
switches the pagetables by rewriting TTBR0 on the fly (since this is
arm-smmu-v2 we have no better option) and yes, unfortunately it is hard coded
to use context bank 0. [1] is the current patchset to support all this,
including my own take on avoiding the dma-domain (all the cool kids have one).

Jordan

[1] https://patchwork.freedesktop.org/series/57441/

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] of/device: add blacklist for iommu dma_ops

[Tomasz Figa]

On Mon, Jun 3, 2019 at 7:48 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Sun, Jun 2, 2019 at 11:25 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > On Mon, Jun 3, 2019 at 4:40 AM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Fri, May 10, 2019 at 7:35 AM Rob Clark <robdclark@gmail.com> wrote:
> > > >
> > > > On Tue, Dec 4, 2018 at 2:29 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Sat, Dec 1, 2018 at 10:54 AM Rob Clark <robdclark@gmail.com> wrote:
> > > > > >
> > > > > > This solves a problem we see with drm/msm, caused by getting
> > > > > > iommu_dma_ops while we attach our own domain and manage it directly at
> > > > > > the iommu API level:
> > > > > >
> > > > > >   [0000000000000038] user address but active_mm is swapper
> > > > > >   Internal error: Oops: 96000005 [#1] PREEMPT SMP
> > > > > >   Modules linked in:
> > > > > >   CPU: 7 PID: 70 Comm: kworker/7:1 Tainted: G        W         4.19.3 #90
> > > > > >   Hardware name: xxx (DT)
> > > > > >   Workqueue: events deferred_probe_work_func
> > > > > >   pstate: 80c00009 (Nzcv daif +PAN +UAO)
> > > > > >   pc : iommu_dma_map_sg+0x7c/0x2c8
> > > > > >   lr : iommu_dma_map_sg+0x40/0x2c8
> > > > > >   sp : ffffff80095eb4f0
> > > > > >   x29: ffffff80095eb4f0 x28: 0000000000000000
> > > > > >   x27: ffffffc0f9431578 x26: 0000000000000000
> > > > > >   x25: 00000000ffffffff x24: 0000000000000003
> > > > > >   x23: 0000000000000001 x22: ffffffc0fa9ac010
> > > > > >   x21: 0000000000000000 x20: ffffffc0fab40980
> > > > > >   x19: ffffffc0fab40980 x18: 0000000000000003
> > > > > >   x17: 00000000000001c4 x16: 0000000000000007
> > > > > >   x15: 000000000000000e x14: ffffffffffffffff
> > > > > >   x13: ffff000000000000 x12: 0000000000000028
> > > > > >   x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
> > > > > >   x9 : 0000000000000000 x8 : ffffffc0fab409a0
> > > > > >   x7 : 0000000000000000 x6 : 0000000000000002
> > > > > >   x5 : 0000000100000000 x4 : 0000000000000000
> > > > > >   x3 : 0000000000000001 x2 : 0000000000000002
> > > > > >   x1 : ffffffc0f9431578 x0 : 0000000000000000
> > > > > >   Process kworker/7:1 (pid: 70, stack limit = 0x0000000017d08ffb)
> > > > > >   Call trace:
> > > > > >    iommu_dma_map_sg+0x7c/0x2c8
> > > > > >    __iommu_map_sg_attrs+0x70/0x84
> > > > > >    get_pages+0x170/0x1e8
> > > > > >    msm_gem_get_iova+0x8c/0x128
> > > > > >    _msm_gem_kernel_new+0x6c/0xc8
> > > > > >    msm_gem_kernel_new+0x4c/0x58
> > > > > >    dsi_tx_buf_alloc_6g+0x4c/0x8c
> > > > > >    msm_dsi_host_modeset_init+0xc8/0x108
> > > > > >    msm_dsi_modeset_init+0x54/0x18c
> > > > > >    _dpu_kms_drm_obj_init+0x430/0x474
> > > > > >    dpu_kms_hw_init+0x5f8/0x6b4
> > > > > >    msm_drm_bind+0x360/0x6c8
> > > > > >    try_to_bring_up_master.part.7+0x28/0x70
> > > > > >    component_master_add_with_match+0xe8/0x124
> > > > > >    msm_pdev_probe+0x294/0x2b4
> > > > > >    platform_drv_probe+0x58/0xa4
> > > > > >    really_probe+0x150/0x294
> > > > > >    driver_probe_device+0xac/0xe8
> > > > > >    __device_attach_driver+0xa4/0xb4
> > > > > >    bus_for_each_drv+0x98/0xc8
> > > > > >    __device_attach+0xac/0x12c
> > > > > >    device_initial_probe+0x24/0x30
> > > > > >    bus_probe_device+0x38/0x98
> > > > > >    deferred_probe_work_func+0x78/0xa4
> > > > > >    process_one_work+0x24c/0x3dc
> > > > > >    worker_thread+0x280/0x360
> > > > > >    kthread+0x134/0x13c
> > > > > >    ret_from_fork+0x10/0x18
> > > > > >   Code: d2800004 91000725 6b17039f 5400048a (f9401f40)
> > > > > >   ---[ end trace f22dda57f3648e2c ]---
> > > > > >   Kernel panic - not syncing: Fatal exception
> > > > > >   SMP: stopping secondary CPUs
> > > > > >   Kernel Offset: disabled
> > > > > >   CPU features: 0x0,22802a18
> > > > > >   Memory Limit: none
> > > > > >
> > > > > > The problem is that when drm/msm does it's own iommu_attach_device(),
> > > > > > now the domain returned by iommu_get_domain_for_dev() is drm/msm's
> > > > > > domain, and it doesn't have domain->iova_cookie.
> > > > > >
> > > > > > We kind of avoided this problem prior to sdm845/dpu because the iommu
> > > > > > was attached to the mdp node in dt, which is a child of the toplevel
> > > > > > mdss node (which corresponds to the dev passed in dma_map_sg()).  But
> > > > > > with sdm845, now the iommu is attached at the mdss level so we hit the
> > > > > > iommu_dma_ops in dma_map_sg().
> > > > > >
> > > > > > But auto allocating/attaching a domain before the driver is probed was
> > > > > > already a blocking problem for enabling per-context pagetables for the
> > > > > > GPU.  This problem is also now solved with this patch.
> > > > > >
> > > > > > Fixes: 97890ba9289c dma-mapping: detect and configure IOMMU in of_dma_configure
> > > > > > Tested-by: Douglas Anderson <dianders@chromium.org>
> > > > > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > > > > > ---
> > > > > > This is an alternative/replacement for [1].  What it lacks in elegance
> > > > > > it makes up for in practicality ;-)
> > > > > >
> > > > > > [1] https://patchwork.freedesktop.org/patch/264930/
> > > > > >
> > > > > >  drivers/of/device.c | 22 ++++++++++++++++++++++
> > > > > >  1 file changed, 22 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > > > > > index 5957cd4fa262..15ffee00fb22 100644
> > > > > > --- a/drivers/of/device.c
> > > > > > +++ b/drivers/of/device.c
> > > > > > @@ -72,6 +72,14 @@ int of_device_add(struct platform_device *ofdev)
> > > > > >         return device_add(&ofdev->dev);
> > > > > >  }
> > > > > >
> > > > > > +static const struct of_device_id iommu_blacklist[] = {
> > > > > > +       { .compatible = "qcom,mdp4" },
> > > > > > +       { .compatible = "qcom,mdss" },
> > > > > > +       { .compatible = "qcom,sdm845-mdss" },
> > > > > > +       { .compatible = "qcom,adreno" },
> > > > > > +       {}
> > > > > > +};
> > > > >
> > > > > Not completely clear to whether this is still needed or not, but this
> > > > > really won't scale. Why can't the driver for these devices override
> > > > > whatever has been setup by default?
> > > > >
> > > >
> > > > fwiw, at the moment it is not needed, but it will become needed again
> > > > to implement per-context pagetables (although I suppose for this we
> > > > only need to blacklist qcom,adreno and not also the display nodes).
> > >
> > > So, another case I've come across, on the display side.. I'm working
> > > on handling the case where bootloader enables display (and takes iommu
> > > out of reset).. as soon as DMA domain gets attached we get iommu
> > > faults, because bootloader has already configured display for scanout.
> > > Unfortunately this all happens before actual driver is probed and has
> > > a chance to intervene.
> > >
> > > It's rather unfortunate that we tried to be clever rather than just
> > > making drivers call some function to opt-in to the hookup of dma iommu
> > > ops :-(
> >
> > I think it still works for the 90% of cases and if 10% needs some
> > explicit work in the drivers, that's better than requiring 100% of the
> > drivers to do things manually.
> >
> > Adding Marek who had the same problem on Exynos.
>
> I do wonder how many drivers need to iommu_map in their ->probe()?

Any driver that allocates some internal buffers using DMA API.

Also all V4L2 drivers would need it, because as soon as they call
register_video_device() the userspace can open the video node and do
buffer allocations, which in turn requires the DMA API to be all set.

> I'm thinking moving the auto-hookup to after a successful probe(),
> with some function a driver could call if they need mapping in probe,
> might be a way to eventually get rid of the blacklist.  But I've no
> idea how to find the subset of drivers that would be broken without a
> dma_setup_iommu_stuff() call in their probe.

Most of the drivers that call dma_alloc_() or dma_map_() from probe
(or could have it called asynchronously before the probe returns).

But first of all, I remember Marek already submitted some patches long
ago that extended struct driver with some flag that means that the
driver doesn't want the IOMMU to be attached before probe. Why
wouldn't that work? Sounds like a perfect opt-out solution.

>
> BR,
> -R
>
> > Best regards,
> > Tomasz
> >
> > >
> > > BR,
> > > -R
> > >
> > > >
> > > > The reason is that in the current state the core code creates the
> > > > first domain before the driver has a chance to intervene and tell it
> > > > not to.  And this results that driver ends up using a different
> > > > context bank on the iommu than what the firmware expects.
> > > >
> > > > I guess the alternative is to put some property in DT.. but that
> > > > doesn't really feel right.  I guess there aren't really many (or any?)
> > > > other drivers that have this specific problem, so I don't really
> > > > expect it to be a scaling problem.
> > > >
> > > > Yeah, it's a bit ugly, but I'll take a small ugly working hack, over
> > > > elegant but non-working any day ;-)... but if someone has a better
> > > > idea then I'm all ears.
> > > >
> > > > BR,
> > > > -R

Re: [Freedreno] [PATCH] of/device: add blacklist for iommu dma_ops

[Rob Clark]

On Tue, Jun 4, 2019 at 11:58 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> But first of all, I remember Marek already submitted some patches long
> ago that extended struct driver with some flag that means that the
> driver doesn't want the IOMMU to be attached before probe. Why
> wouldn't that work? Sounds like a perfect opt-out solution.

Actually, yeah.. we should do that.  That is the simplest solution.

BR,
-R

Re: [Freedreno] [PATCH] of/device: add blacklist for iommu dma_ops

[Marek Szyprowski]

Hi Rob,

On 2019-06-05 14:57, Rob Clark wrote:
> On Tue, Jun 4, 2019 at 11:58 PM Tomasz Figa <tfiga@chromium.org> wrote:
>> But first of all, I remember Marek already submitted some patches long
>> ago that extended struct driver with some flag that means that the
>> driver doesn't want the IOMMU to be attached before probe. Why
>> wouldn't that work? Sounds like a perfect opt-out solution.
> Actually, yeah.. we should do that.  That is the simplest solution.

Tomasz has very good memory. It took me a while to find that old patches:

https://patchwork.kernel.org/patch/4677251/
https://patchwork.kernel.org/patch/4677941/
https://patchwork.kernel.org/patch/4677401/

It looks that my idea was a bit ahead of its time ;)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [Freedreno] [PATCH] of/device: add blacklist for iommu dma_ops

[Rob Clark]

On Wed, Jun 5, 2019 at 6:18 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Rob,
>
> On 2019-06-05 14:57, Rob Clark wrote:
> > On Tue, Jun 4, 2019 at 11:58 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >> But first of all, I remember Marek already submitted some patches long
> >> ago that extended struct driver with some flag that means that the
> >> driver doesn't want the IOMMU to be attached before probe. Why
> >> wouldn't that work? Sounds like a perfect opt-out solution.
> > Actually, yeah.. we should do that.  That is the simplest solution.
>
> Tomasz has very good memory. It took me a while to find that old patches:
>
> https://patchwork.kernel.org/patch/4677251/
> https://patchwork.kernel.org/patch/4677941/
> https://patchwork.kernel.org/patch/4677401/
>
> It looks that my idea was a bit ahead of its time ;)
>

if I re-spin this, was their a reason not to just use bitfields, ie:

-    bool suppress_bind_attrs;    /* disables bind/unbind via sysfs */
+    bool suppress_bind_attrs : 1;    /* disables bind/unbind via sysfs */
+    bool has_own_iommu_manager : 1;  /* driver explictly manages IOMMU */

That seems like it would have been a bit less churn and a bit nicer
looking (IMO at least)

BR,
-R