Re: [PATCH v5 0/5] iommu: Retire detach_dev callback

[Marek Szyprowski]

Hi All,


On 10.01.2023 03:54, Lu Baolu wrote:
> Hi folks,
>
> The iommu core calls the driver's detach_dev domain op callback only when
> a device is finished assigning to user space and
> iommu_group_release_dma_owner() is called to return the device to the
> kernel, where iommu core wants to set the default domain to the device but
> the driver didn't provide one. The code looks like:
>
>          /*
>           * New drivers should support default domains and so the detach_dev() op
>           * will never be called. Otherwise the NULL domain represents some
>           * platform specific behavior.
>           */
>          if (!new_domain) {
>                  if (WARN_ON(!group->domain->ops->detach_dev))
>                          return -EINVAL;
>                  __iommu_group_for_each_dev(group, group->domain,
>                                             iommu_group_do_detach_device);
>                  group->domain = NULL;
>                  return 0;
>          }
>
> In other words, if the iommu driver provides default domains, the
> .detach_dev callback will never be called; Otherwise, the .detach_dev
> callback is actually called to return control back to the platform DMA
> ops, other than detaching the domain from device.
>
> This series cleanups this by:
>
> - If the IOMMU driver provides default domains, remove .detach_dev
>    callback.
> - Adds a new set_platform_dma iommu op. Any IOMMU driver that doesn't
>    provide default domain should implement set_platform_dma callback
>    instead.
> - Retire .detach_dev callback.
>
> This series originates from various discussion in the community. Thanks
> to Jason, Robin and all others for their ideas.

I wonder how to handle the ARM 32bit case, which doesn't use the default 
domain solution. Today, once this patchset has been merged to 
linux-next, I've noticed that it broke booting of ARM 32bit Exynos based 
boards.

The final solution would be to switch ARM 32bit to generic DMA-IOMMU 
glue, but I'm not sure it this will happen soon. I will try to check if 
any kind of quick workaround can be applied to get it working again.


> The whole series is available on github:
> https://protect2.fireeye.com/v1/url?k=ef2183b6-b0bababa-ef2008f9-000babff3563-336828b3433153d2&q=1&e=4ae5dae4-383f-4a82-9449-3f08f0422cb1&u=https%3A%2F%2Fgithub.com%2FLuBaolu%2Fintel-iommu%2Fcommits%2Fiommu-retire-detach_dev-v5
>
> Change log:
> v5:
>   - Merge some patches to make the series cute. No functionality changes.
>   - Check the op directly and WARN_ON the lack of any necessary
>     callbacks. Get rid of the ret and EINVAL.
>
> v4:
>   - https://lore.kernel.org/linux-iommu/20230104125725.271850-1-baolu.lu@xxxxxxxxxxxxxxx/
>   - Drop the patch which renamed .attach_dev to .set_dev. As Robin said,
>     "... I also wouldn't say that "attach" loses its meaning in a context
>     where an equivalent "detach" operation is only ever implicit in
>     reattaching to something else...". If we have a better name in the
>     future, we can do it in other series.
>   - Adjust the patch of "iommu: Add set_platform_dma_ops iommu ops"
>     according to Jason's following suggestion " ... This is a bit ugly,
>     it would be better to make the previous patch call set_platform_dma
>     if it is set instead of detach_dev and then these patches should just
>     rename the driver's fsl_pamu_detach_device to
>     fsl_pamu_set_platform_dma ..."
>   - Add a new patch to remove deferred attach check from
>     __iommu_detach_domain() path. Make it a separate patch as the
>     prerequisite to remove __iommu_detach_device() helper.
>   - Rename set_platform_dma to set_platform_dma_ops to make it more
>     meaningful.
>
> v3:
>   - https://lore.kernel.org/linux-iommu/20221128064648.1934720-1-baolu.lu@xxxxxxxxxxxxxxx/
>   - Setting blocking domain is not conceptually equal to detach_dev.
>     Dropped all blocking domain related staffs in the previous version.
>
> v2:
>   - https://lore.kernel.org/linux-iommu/20220826123014.52709-1-baolu.lu@xxxxxxxxxxxxxxx/
>   - Replace .detach_dev callback with static block domain ops;
>   - Rename .attach_dev to .set_dev.
>
> v1:
>   - https://lore.kernel.org/linux-iommu/20220516015759.2952771-1-baolu.lu@xxxxxxxxxxxxxxx/
>
> Jason Gunthorpe (1):
>    iommu: Remove deferred attach check from __iommu_detach_device()
>
> Lu Baolu (4):
>    iommu: Remove detach_dev callbacks
>    iommu: Add set_platform_dma_ops iommu ops
>    iommu: Add set_platform_dma_ops callbacks
>    iommu: Remove detach_dev callback
>
>   include/linux/iommu.h                   |  8 ++-
>   include/trace/events/iommu.h            |  7 --
>   drivers/iommu/amd/iommu.c               | 26 -------
>   drivers/iommu/apple-dart.c              | 24 -------
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 23 ------
>   drivers/iommu/exynos-iommu.c            |  1 -
>   drivers/iommu/fsl_pamu_domain.c         |  6 +-
>   drivers/iommu/iommu-traces.c            |  1 -
>   drivers/iommu/iommu.c                   | 94 ++++++++++++-------------
>   drivers/iommu/ipmmu-vmsa.c              | 16 -----
>   drivers/iommu/msm_iommu.c               |  6 +-
>   drivers/iommu/mtk_iommu.c               |  9 ---
>   drivers/iommu/mtk_iommu_v1.c            |  4 +-
>   drivers/iommu/omap-iommu.c              |  6 +-
>   drivers/iommu/rockchip-iommu.c          |  1 -
>   drivers/iommu/s390-iommu.c              |  7 +-
>   drivers/iommu/sprd-iommu.c              | 16 -----
>   drivers/iommu/sun50i-iommu.c            |  1 -
>   drivers/iommu/tegra-gart.c              |  6 +-
>   drivers/iommu/tegra-smmu.c              |  5 +-
>   20 files changed, 69 insertions(+), 198 deletions(-)
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland