Messages in this thread | | | Date | Wed, 10 Apr 2024 17:37:06 +0100 | Subject | Re: [PATCH v3 1/5] iommu: Add static iommu_ops->release_domain | From | Robin Murphy <> |
| |
On 2024-04-10 4:26 pm, Jason Gunthorpe wrote: > On Tue, Mar 05, 2024 at 09:33:01AM +0800, Lu Baolu wrote: >> The current device_release callback for individual iommu drivers does the >> following: >> >> 1) Silent IOMMU DMA translation: It detaches any existing domain from the >> device and puts it into a blocking state (some drivers might use the >> identity state). >> 2) Resource release: It releases resources allocated during the >> device_probe callback and restores the device to its pre-probe state. >> >> Step 1 is challenging for individual iommu drivers because each must check >> if a domain is already attached to the device. Additionally, if a deferred >> attach never occurred, the device_release should avoid modifying hardware >> configuration regardless of the reason for its call. >> >> To simplify this process, introduce a static release_domain within the >> iommu_ops structure. It can be either a blocking or identity domain >> depending on the iommu hardware. The iommu core will decide whether to >> attach this domain before the device_release callback, eliminating the >> need for repetitive code in various drivers. >> >> Consequently, the device_release callback can focus solely on the opposite >> operations of device_probe, including releasing all resources allocated >> during that callback. >> >> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com> >> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> Reviewed-by: Kevin Tian <kevin.tian@intel.com> >> --- >> include/linux/iommu.h | 1 + >> drivers/iommu/iommu.c | 19 +++++++++++++++---- >> 2 files changed, 16 insertions(+), 4 deletions(-) > > I looked at all the drivers: > 1) Didn't spend time to guess what ipmmu-vmss is doing > 2) Several drivers are obviously missing the release_domain behavior > and now be trivially fixed > 3) amd, s390, virtio-iommu and arm-smmu should probably support > blocked_domain (I assume that is what their detach does) > 4) arm-smmuv3 can use it too once disable_bypass is removed > 5) Several drivers don't even have release_device functions. > Probably all of them can do their identiy domains too. > > This seems like a pretty reasonable thing to add to this series too: > > diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c > index eb1e62cd499a58..3ddc4b4418a049 100644 > --- a/drivers/iommu/apple-dart.c > +++ b/drivers/iommu/apple-dart.c > @@ -979,6 +979,7 @@ static void apple_dart_get_resv_regions(struct device *dev, > static const struct iommu_ops apple_dart_iommu_ops = { > .identity_domain = &apple_dart_identity_domain, > .blocked_domain = &apple_dart_blocked_domain, > + .release_domain = &apple_dart_blocked_domain, > .domain_alloc_paging = apple_dart_domain_alloc_paging, > .probe_device = apple_dart_probe_device, > .release_device = apple_dart_release_device, > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > index d98c9161948a25..902dc4da44f987 100644 > --- a/drivers/iommu/exynos-iommu.c > +++ b/drivers/iommu/exynos-iommu.c > @@ -1424,8 +1424,6 @@ static void exynos_iommu_release_device(struct device *dev) > struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev); > struct sysmmu_drvdata *data; > > - WARN_ON(exynos_iommu_identity_attach(&exynos_identity_domain, dev)); > - > list_for_each_entry(data, &owner->controllers, owner_node) > device_link_del(data->link); > } > @@ -1471,6 +1469,7 @@ static int exynos_iommu_of_xlate(struct device *dev, > > static const struct iommu_ops exynos_iommu_ops = { > .identity_domain = &exynos_identity_domain, > + .release_domain = &exynos_identity_domain, > .domain_alloc_paging = exynos_iommu_domain_alloc_paging, > .device_group = generic_device_group, > .probe_device = exynos_iommu_probe_device, > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index b8c47f18bc2612..b5693041b18dd4 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -1012,6 +1012,7 @@ static void mtk_iommu_get_resv_regions(struct device *dev, > > static const struct iommu_ops mtk_iommu_ops = { > .identity_domain = &mtk_iommu_identity_domain, > + .release_domain = &mtk_iommu_identity_domain, > .domain_alloc_paging = mtk_iommu_domain_alloc_paging, > .probe_device = mtk_iommu_probe_device, > .release_device = mtk_iommu_release_device, > diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c > index a9fa2a54dc9b39..9e7205af7d7316 100644 > --- a/drivers/iommu/mtk_iommu_v1.c > +++ b/drivers/iommu/mtk_iommu_v1.c > @@ -580,6 +580,7 @@ static int mtk_iommu_v1_hw_init(const struct mtk_iommu_v1_data *data) > > static const struct iommu_ops mtk_iommu_v1_ops = { > .identity_domain = &mtk_iommu_v1_identity_domain, > + .release_domain = &mtk_iommu_v1_identity_domain, > .domain_alloc_paging = mtk_iommu_v1_domain_alloc_paging, > .probe_device = mtk_iommu_v1_probe_device, > .probe_finalize = mtk_iommu_v1_probe_finalize, > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c > index c9528065a59afa..c4c76aaec19e50 100644 > --- a/drivers/iommu/omap-iommu.c > +++ b/drivers/iommu/omap-iommu.c > @@ -1725,6 +1725,7 @@ static void omap_iommu_release_device(struct device *dev) > > static const struct iommu_ops omap_iommu_ops = { > .identity_domain = &omap_iommu_identity_domain, > + .release_domain = &omap_iommu_identity_domain, > .domain_alloc_paging = omap_iommu_domain_alloc_paging, > .probe_device = omap_iommu_probe_device, > .release_device = omap_iommu_release_device, > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index da79d9f4cf6371..e551c5b143bbd3 100644 > --- a/drivers/iommu/rockchip-iommu.c > +++ b/drivers/iommu/rockchip-iommu.c > @@ -1162,6 +1162,7 @@ static int rk_iommu_of_xlate(struct device *dev, > > static const struct iommu_ops rk_iommu_ops = { > .identity_domain = &rk_identity_domain, > + .release_domain = &rk_identity_domain, > .domain_alloc_paging = rk_iommu_domain_alloc_paging, > .probe_device = rk_iommu_probe_device, > .release_device = rk_iommu_release_device, > >> + if (!dev->iommu->attach_deferred && ops->release_domain) >> + ops->release_domain->ops->attach_dev(ops->release_domain, dev); > > We should probably be sensitive to the > dev->iommu->require_direct flag - generally drivers should prefer the > blocked for the release domain, but in case the FW ias asking for > require_direct we need to switch to identity.
At this point do we even need release_domain? It sounds like the logic you want to enforce is going to be trivial to resolve directly in the core code. As typed-in-mail-client pseudocode, roughly:
static void iommu_set_release_domain(struct device *dev) { const struct iommu_ops *ops = dev_iommu_ops(dev); struct iommu_domain *rd;
/* * Static domains are expected not to track any device state, * and thus be tolerant of devices disappearing once "attached" */ if (ops->blocked_domain && !(dev->iommu->require_direct || other_arch_or_platform_reason)) rd = ops->blocked_domain; else if (ops->identity_domain) rd = ops->identity_domain; else /* Hope release_device does the right thing! */ return;
if (!dev->iommu->attach_deferred && rd != dev->iommu_group->domain) __iommu_attach_device(rd, dev); }
..no driver churn necessary. (And frankly I think it's a further bonus to avoid risking any notion of release_domain being implementable as its own distinct special thing)
Thanks, Robin.
> > Also, may as well avoid switching a domain if the group is already > correct and use the common attach function to get the tracing.. So > like this? > > if (!dev->iommu->attach_deferred) { > struct iommu_domain *release_domain = ops->release_domain; > > if (dev->iommu->require_direct && ops->identity_domain) > release_domain = ops->identity_domain; > > if (release_domain && group->domain != release_domain) > WARN_ON(__iommu_attach_device(release_domain, dev)); > } > > Jason
| |