| Message ID | 0-v6-e8114faedade+425-iommu_all_defdom_jgg@nvidia.com |
|---|---|
| Headers | show |
| Series | iommu: Make default_domain's mandatory | expand |
On 2023/8/3 8:07, Jason Gunthorpe wrote: > [ It would be good to get this in linux-next, we have some good test > coverage on the ARM side already, thanks! ] > > It has been a long time coming, this series completes the default_domain > transition and makes it so that the core IOMMU code will always have a > non-NULL default_domain for every driver on every > platform. set_platform_dma_ops() turned out to be a bad idea, and so > completely remove it. > > This is achieved by changing each driver to either: > > 1 - Convert the existing (or deleted) ops->detach_dev() into an > op->attach_dev() of an IDENTITY domain. > > This is based on the theory that the ARM32 HW is able to function when > the iommu is turned off and so the turned off state is an IDENTITY > translation. > > 2 - Use a new PLATFORM domain type. This is a hack to accommodate drivers > that we don't really know WTF they do. S390 is legitimately using this > to switch to it's platform dma_ops implementation, which is where the > name comes from. > > 3 - Do #1 and force the default domain to be IDENTITY, this corrects > the tegra-smmu case where even an ARM64 system would have a NULL > default_domain. > > Using this we can apply the rules: > > a) ARM_DMA_USE_IOMMU mode always uses either the driver's > ops->default_domain, ops->def_domain_type(), or an IDENTITY domain. > All ARM32 drivers provide one of these three options. > > b) dma-iommu.c mode uses either the driver's ops->default_domain, > ops->def_domain_type or the usual DMA API policy logic based on the > command line/etc to pick IDENTITY/DMA domain types > > c) All other arch's (PPC/S390) use ops->default_domain always. > > See the patch "Require a default_domain for all iommu drivers" for a > per-driver breakdown. > > The conversion broadly teaches a bunch of ARM32 drivers that they can do > IDENTITY domains. There is some educated guessing involved that these are > actual IDENTITY domains. If this turns out to be wrong the driver can be > trivially changed to use a BLOCKING domain type instead. Further, the > domain type only matters for drivers using ARM64's dma-iommu.c mode as it > will select IDENTITY based on the command line and expect IDENTITY to > work. For ARM32 and other arch cases it is purely documentation. > > Finally, based on all the analysis in this series, we can purge > IOMMU_DOMAIN_UNMANAGED/DMA constants from most of the drivers. This > greatly simplifies understanding the driver contract to the core > code. IOMMU drivers should not be involved in policy for how the DMA API > works, that should be a core core decision. > > The main gain from this work is to remove alot of ARM_DMA_USE_IOMMU > specific code and behaviors from drivers. All that remains in iommu > drivers after this series is the calls to arm_iommu_create_mapping(). > > This is a step toward removing ARM_DMA_USE_IOMMU. > > The IDENTITY domains added to the ARM64 supporting drivers can be tested > by booting in ARM64 mode and enabling CONFIG_IOMMU_DEFAULT_PASSTHROUGH. If > the system still boots then most likely the implementation is an IDENTITY > domain. If not we can trivially change it to BLOCKING or at worst PLATFORM > if there is no detail what is going on in the HW. > > I think this is pretty safe for the ARM32 drivers as they don't really > change, the code that was in detach_dev continues to be called in the same > places it was called before. > > This is on github:https://github.com/jgunthorpe/linux/commits/iommu_all_defdom It seems that after this series, all ARM iommu drivers are able to support the IDENTITY default domain, hence perhaps we can remove below code? If I remember it correctly, the background of this part of code is that some arm drivers didn't support IDENTITY domain, so fall back to DMA domain if IDENTITY domain allocation fails. diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index ddbba3ffbfbd..ee1fa63f0612 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1798,7 +1798,6 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) list_first_entry(&group->devices, struct group_device, list) ->dev; const struct iommu_ops *ops = dev_iommu_ops(dev); - struct iommu_domain *dom; lockdep_assert_held(&group->mutex); @@ -1817,20 +1816,7 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) return __iommu_group_alloc_default_domain(group, req_type); /* The driver gave no guidance on what type to use, try the default */ - dom = __iommu_group_alloc_default_domain(group, iommu_def_domain_type); - if (dom) - return dom; - - /* Otherwise IDENTITY and DMA_FQ defaults will try DMA */ - if (iommu_def_domain_type == IOMMU_DOMAIN_DMA) - return NULL; - dom = __iommu_group_alloc_default_domain(group, IOMMU_DOMAIN_DMA); - if (!dom) - return NULL; - - pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA", - iommu_def_domain_type, group->name); - return dom; + return __iommu_group_alloc_default_domain(group, iommu_def_domain_type); } struct iommu_domain *iommu_group_default_domain(struct iommu_group *group) Best regards, baolu
On Mon, Aug 14, 2023 at 04:43:23PM +0800, Baolu Lu wrote: > > This is on github:https://github.com/jgunthorpe/linux/commits/iommu_all_defdom > > It seems that after this series, all ARM iommu drivers are able to > support the IDENTITY default domain, hence perhaps we can remove below > code? Yes, but this code is still used > If I remember it correctly, the background of this part of code is > that some arm drivers didn't support IDENTITY domain, so fall back to > DMA domain if IDENTITY domain allocation fails. Not quite.. if (req_type) return __iommu_group_alloc_default_domain(group, req_type); req_type == 0 can still happen because it depends on what def_domain_type returns, which is still 0 in alot of cases /* The driver gave no guidance on what type to use, try the default */ dom = __iommu_group_alloc_default_domain(group, iommu_def_domain_type); if (dom) return dom; So we try the default which might be IDENTITY/DMA/DMA_FQ - still have to do this. /* Otherwise IDENTITY and DMA_FQ defaults will try DMA */ if (iommu_def_domain_type == IOMMU_DOMAIN_DMA) return NULL; dom = __iommu_group_alloc_default_domain(group, IOMMU_DOMAIN_DMA); if (!dom) return NULL; pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA", iommu_def_domain_type, group->name); And this hunk is primarily a fallback in case the DMA_FQ didn't work. Then we try normal DMA. That it also protected against not implementing IDENTITY is a side effect, so I think we have to keep all of this still. Jason
On Wed, Aug 02, 2023 at 09:08:02PM -0300, Jason Gunthorpe wrote: > I've avoided doing this because there is no way to make this happen > without an intrusion into the core code. Up till now this has avoided > needing the core code's probe path with some hackery - but now that > default domains are becoming mandatory it is unavoidable. The core probe > path must be run to set the default_domain, only it can do it. Without > a default domain iommufd can't use the group. > > Make it so that iommufd selftest can create a real iommu driver and bind > it only to is own private bus. Add iommu_device_register_bus() as a core > code helper to make this possible. It simply sets the right pointers and > registers the notifier block. The mock driver then works like any normal > driver should, with probe triggered by the bus ops > > When the bus->iommu_ops stuff is fully unwound we can probably do better > here and remove this special case. > > Remove set_platform_dma_ops from selftest and make it use a BLOCKED > default domain. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu-priv.h | 16 +++ > drivers/iommu/iommu.c | 43 +++++++ > drivers/iommu/iommufd/iommufd_private.h | 5 +- > drivers/iommu/iommufd/main.c | 8 +- > drivers/iommu/iommufd/selftest.c | 149 +++++++++++++----------- > 5 files changed, 152 insertions(+), 69 deletions(-) > create mode 100644 drivers/iommu/iommu-priv.h Since this series will miss this kernel again I've taken this patch into the iommufd tree to fix the broken selftest. It needed two edits to make it work out of the context of this series The core code still requires empty free functions: +@@ drivers/iommu/iommufd/selftest.c: static struct iommu_domain *mock_domain_alloc(unsigned int iommu_domain_type) + if (iommu_domain_type == IOMMU_DOMAIN_BLOCKED) + return &mock_blocking_domain; --static void mock_domain_blocking_free(struct iommu_domain *domain) --{ --} -- - static int mock_domain_nop_attach(struct iommu_domain *domain, - struct device *dev) - { +@@ drivers/iommu/iommufd/selftest.c: static void mock_domain_set_plaform_dma_ops(struct device *dev) + */ } - static const struct iommu_domain_ops mock_blocking_ops = { -- .free = mock_domain_blocking_free, - .attach_dev = mock_domain_nop_attach, - }; - And we can't use default_domain so rely on a NULL default domain and set_platform_dma_ops: -@@ drivers/iommu/iommufd/selftest.c: static int mock_domain_nop_attach(struct iommu_domain *domain, +- if (WARN_ON(iommu_domain_type != IOMMU_DOMAIN_UNMANAGED)) ++ if (iommu_domain_type != IOMMU_DOMAIN_UNMANAGED) + return NULL; + + mock = kzalloc(sizeof(*mock), GFP_KERNEL); -@@ drivers/iommu/iommufd/selftest.c: static bool mock_domain_capable(struct device *dev, enum iommu_cap cap) - return cap == IOMMU_CAP_CACHE_COHERENCY; - } - --static void mock_domain_set_plaform_dma_ops(struct device *dev) +static struct iommu_device mock_iommu_device = { +}; + +static struct iommu_device *mock_probe_device(struct device *dev) - { -- /* -- * mock doesn't setup default domains because we can't hook into the -- * normal probe path -- */ ++{ + return &mock_iommu_device; - } - ++} ++ static const struct iommu_ops mock_ops = { -+ /* -+ * IOMMU_DOMAIN_BLOCKED cannot be returned from def_domain_type() -+ * because it is zero. -+ */ -+ .default_domain = &mock_blocking_domain, .owner = THIS_MODULE, .pgsize_bitmap = MOCK_IO_PAGE_SIZE, .domain_alloc = mock_domain_alloc, .capable = mock_domain_capable, -- .set_platform_dma_ops = mock_domain_set_plaform_dma_ops, + .set_platform_dma_ops = mock_domain_set_plaform_dma_ops, + .device_group = generic_device_group, + .probe_device = mock_probe_device, .default_domain_ops = Otherwise it works the same and solves the same problem. There is a small merge conflict with the fixes in Joerg's tree around the bus_iommu_probe Thanks, Jason
On 2023/8/15 1:30, Jason Gunthorpe wrote: > On Mon, Aug 14, 2023 at 04:43:23PM +0800, Baolu Lu wrote: > >>> This is on github:https://github.com/jgunthorpe/linux/commits/iommu_all_defdom >> It seems that after this series, all ARM iommu drivers are able to >> support the IDENTITY default domain, hence perhaps we can remove below >> code? > Yes, but this code is still used > >> If I remember it correctly, the background of this part of code is >> that some arm drivers didn't support IDENTITY domain, so fall back to >> DMA domain if IDENTITY domain allocation fails. > Not quite.. > > if (req_type) > return __iommu_group_alloc_default_domain(group, req_type); > > req_type == 0 can still happen because it depends on what > def_domain_type returns, which is still 0 in alot of cases > > /* The driver gave no guidance on what type to use, try the default */ > dom = __iommu_group_alloc_default_domain(group, iommu_def_domain_type); > if (dom) > return dom; > > So we try the default which might be IDENTITY/DMA/DMA_FQ - still have > to do this. > > /* Otherwise IDENTITY and DMA_FQ defaults will try DMA */ > if (iommu_def_domain_type == IOMMU_DOMAIN_DMA) > return NULL; > dom = __iommu_group_alloc_default_domain(group, IOMMU_DOMAIN_DMA); > if (!dom) > return NULL; > > pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA", > iommu_def_domain_type, group->name); > > And this hunk is primarily a fallback in case the DMA_FQ didn't > work. Then we try normal DMA. > > That it also protected against not implementing IDENTITY is a side > effect, so I think we have to keep all of this still. Okay, fair enough. Thanks for the explanation. Best regards, baolu
[ It would be good to get this in linux-next, we have some good test coverage on the ARM side already, thanks! ] It has been a long time coming, this series completes the default_domain transition and makes it so that the core IOMMU code will always have a non-NULL default_domain for every driver on every platform. set_platform_dma_ops() turned out to be a bad idea, and so completely remove it. This is achieved by changing each driver to either: 1 - Convert the existing (or deleted) ops->detach_dev() into an op->attach_dev() of an IDENTITY domain. This is based on the theory that the ARM32 HW is able to function when the iommu is turned off and so the turned off state is an IDENTITY translation. 2 - Use a new PLATFORM domain type. This is a hack to accommodate drivers that we don't really know WTF they do. S390 is legitimately using this to switch to it's platform dma_ops implementation, which is where the name comes from. 3 - Do #1 and force the default domain to be IDENTITY, this corrects the tegra-smmu case where even an ARM64 system would have a NULL default_domain. Using this we can apply the rules: a) ARM_DMA_USE_IOMMU mode always uses either the driver's ops->default_domain, ops->def_domain_type(), or an IDENTITY domain. All ARM32 drivers provide one of these three options. b) dma-iommu.c mode uses either the driver's ops->default_domain, ops->def_domain_type or the usual DMA API policy logic based on the command line/etc to pick IDENTITY/DMA domain types c) All other arch's (PPC/S390) use ops->default_domain always. See the patch "Require a default_domain for all iommu drivers" for a per-driver breakdown. The conversion broadly teaches a bunch of ARM32 drivers that they can do IDENTITY domains. There is some educated guessing involved that these are actual IDENTITY domains. If this turns out to be wrong the driver can be trivially changed to use a BLOCKING domain type instead. Further, the domain type only matters for drivers using ARM64's dma-iommu.c mode as it will select IDENTITY based on the command line and expect IDENTITY to work. For ARM32 and other arch cases it is purely documentation. Finally, based on all the analysis in this series, we can purge IOMMU_DOMAIN_UNMANAGED/DMA constants from most of the drivers. This greatly simplifies understanding the driver contract to the core code. IOMMU drivers should not be involved in policy for how the DMA API works, that should be a core core decision. The main gain from this work is to remove alot of ARM_DMA_USE_IOMMU specific code and behaviors from drivers. All that remains in iommu drivers after this series is the calls to arm_iommu_create_mapping(). This is a step toward removing ARM_DMA_USE_IOMMU. The IDENTITY domains added to the ARM64 supporting drivers can be tested by booting in ARM64 mode and enabling CONFIG_IOMMU_DEFAULT_PASSTHROUGH. If the system still boots then most likely the implementation is an IDENTITY domain. If not we can trivially change it to BLOCKING or at worst PLATFORM if there is no detail what is going on in the HW. I think this is pretty safe for the ARM32 drivers as they don't really change, the code that was in detach_dev continues to be called in the same places it was called before. This is on github: https://github.com/jgunthorpe/linux/commits/iommu_all_defdom v6: - Rebase on v6.5-rc1/Joerg's tree - Fix the iommufd self test missing the iommu_device_sysfs_add() - Update typo in msm commit message v5: https://lore.kernel.org/r/0-v5-d0a204c678c7+3d16a-iommu_all_defdom_jgg@nvidia.com - Rebase on v6.5-rc1/Joerg's tree - Fix Dan's remark about 'gdev uninitialized' in patch 9 v4: https://lore.kernel.org/r/0-v4-874277bde66e+1a9f6-iommu_all_defdom_jgg@nvidia.com - Fix rebasing typo missing ops->alloc_domain_paging check - Rebase on latest Joerg tree v3: https://lore.kernel.org/r/0-v3-89830a6c7841+43d-iommu_all_defdom_jgg@nvidia.com - FSL is back to a PLATFORM domain, with some fixing so it attach only does something when leaving an UNMANAGED domain like it always was - Rebase on Joerg's tree, adjust for "alloc_type" change - Change the ARM32 untrusted check to a WARN_ON since no ARM32 system can currently set trusted v2: https://lore.kernel.org/r/0-v2-8d1dc464eac9+10f-iommu_all_defdom_jgg@nvidia.com - FSL is an IDENTITY domain - Delete terga-gart instead of trying to carry it - Use the policy determination from iommu_get_default_domain_type() to drive the arm_iommu mode - Reorganize and introduce new patches to do the above: * Split the ops->identity_domain to an independent earlier patch * Remove the UNMANAGED return from def_domain_type in mtk_v1 earlier so the new iommu_get_default_domain_type() can work * Make the driver's def_domain_type have higher policy priority than untrusted * Merge the set_platfom_dma_ops hunk from mtk_v1 along with rockchip into the patch that forced IDENTITY on ARM32 - Revise sun50i to be cleaner and have a non-NULL internal domain - Reword logging in exynos - Remove the gdev from the group alloc path, instead add a new function __iommu_group_domain_alloc() that takes in the group and uses the first device. Split this to its own patch - New patch to make iommufd's mock selftest into a real driver - New patch to fix power's partial iommu driver v1: https://lore.kernel.org/r/0-v1-21cc72fcfb22+a7a-iommu_all_defdom_jgg@nvidia.com Cc: Steven Price <steven.price@arm.com> Cc: Marek Szyprowski <m.szyprowski@samsung.com> Cc: Nicolin Chen <nicolinc@nvidia.com> Cc: Lu Baolu <baolu.lu@linux.intel.com> Cc: Kevin Tian <kevin.tian@intel.com> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Jason Gunthorpe (25): iommu: Add iommu_ops->identity_domain iommu: Add IOMMU_DOMAIN_PLATFORM powerpc/iommu: Setup a default domain and remove set_platform_dma_ops iommu: Add IOMMU_DOMAIN_PLATFORM for S390 iommu/fsl_pamu: Implement a PLATFORM domain iommu/tegra-gart: Remove tegra-gart iommu/mtk_iommu_v1: Implement an IDENTITY domain iommu: Reorganize iommu_get_default_domain_type() to respect def_domain_type() iommu: Allow an IDENTITY domain as the default_domain in ARM32 iommu/exynos: Implement an IDENTITY domain iommu/tegra-smmu: Implement an IDENTITY domain iommu/tegra-smmu: Support DMA domains in tegra iommu/omap: Implement an IDENTITY domain iommu/msm: Implement an IDENTITY domain iommufd/selftest: Make the mock iommu driver into a real driver iommu: Remove ops->set_platform_dma_ops() iommu/qcom_iommu: Add an IOMMU_IDENTITIY_DOMAIN iommu/ipmmu: Add an IOMMU_IDENTITIY_DOMAIN iommu/mtk_iommu: Add an IOMMU_IDENTITIY_DOMAIN iommu/sun50i: Add an IOMMU_IDENTITIY_DOMAIN iommu: Require a default_domain for all iommu drivers iommu: Add __iommu_group_domain_alloc() iommu: Add ops->domain_alloc_paging() iommu: Convert simple drivers with DOMAIN_DMA to domain_alloc_paging() iommu: Convert remaining simple drivers to domain_alloc_paging() arch/arm/configs/multi_v7_defconfig | 1 - arch/arm/configs/tegra_defconfig | 1 - arch/powerpc/kernel/iommu.c | 38 ++- drivers/iommu/Kconfig | 11 - drivers/iommu/Makefile | 1 - drivers/iommu/arm/arm-smmu/qcom_iommu.c | 45 ++- drivers/iommu/exynos-iommu.c | 73 +++-- drivers/iommu/fsl_pamu_domain.c | 41 ++- drivers/iommu/iommu-priv.h | 16 + drivers/iommu/iommu.c | 248 ++++++++++------ drivers/iommu/iommufd/iommufd_private.h | 5 +- drivers/iommu/iommufd/main.c | 8 +- drivers/iommu/iommufd/selftest.c | 149 +++++----- drivers/iommu/ipmmu-vmsa.c | 50 +++- drivers/iommu/msm_iommu.c | 30 +- drivers/iommu/mtk_iommu.c | 30 +- drivers/iommu/mtk_iommu_v1.c | 28 +- drivers/iommu/omap-iommu.c | 28 +- drivers/iommu/rockchip-iommu.c | 26 +- drivers/iommu/s390-iommu.c | 28 +- drivers/iommu/sprd-iommu.c | 7 +- drivers/iommu/sun50i-iommu.c | 35 ++- drivers/iommu/tegra-gart.c | 371 ------------------------ drivers/iommu/tegra-smmu.c | 50 +++- drivers/memory/tegra/mc.c | 34 --- drivers/memory/tegra/tegra20.c | 28 -- include/linux/iommu.h | 16 +- include/soc/tegra/mc.h | 26 -- 28 files changed, 622 insertions(+), 802 deletions(-) create mode 100644 drivers/iommu/iommu-priv.h delete mode 100644 drivers/iommu/tegra-gart.c base-commit: a5003e75a1714857c01317d04982eef81331fe2f