ipmmu-vmsa cleanup
Robin Murphy
reviewercover ipmmu-vmsa cleanup
Arnd reports a build warning[1] thanks to me missing ipmmu-vmsa's second set of ops when converting io-pgtable-arm users to the new iommu_iotlb_* callbacks. Rather than just treat the symptom with a point fix, this seemed like a good excuse to clean up the messy #ifdeffery and duplication in the driver that led to the oversight in the first place.
Build-tested successfully for arm64, ARM, and ARM + CONFIG_IOMMU_DMA.
Robin.
[1]:https://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1510423.html
Robin Murphy (4):
iommu/ipmmu-vmsa: Unify domain alloc/free iommu/ipmmu-vmsa: Simplify group allocation iommu/ipmmu-vmsa: Clean up struct ipmmu_vmsa_iommu_priv iommu/ipmmu-vmsa: Unify ipmmu_ops
drivers/iommu/ipmmu-vmsa.c | 251 ++++++++++++++------------------------------- 1 file changed, 78 insertions(+), 173 deletions(-)
Signature · 1 line
2.13.4.dirty
Robin Murphy
reviewerPATCH 1/4 iommu/ipmmu-vmsa: Unify domain alloc/free
We have two implementations for ipmmu_ops->alloc depending on CONFIG_IOMMU_DMA, the difference being whether they accept the IOMMU_DOMAIN_DMA type or not. However, iommu_dma_get_cookie() is guaranteed to return an error when !CONFIG_IOMMU_DMA, so if ipmmu_domain_alloc_dma() was actually checking and handling the return value correctly, it would behave the same as ipmmu_domain_alloc() anyway.
Similarly for freeing; iommu_put_dma_cookie() is robust by design.
--- drivers/iommu/ipmmu-vmsa.c | 65 +++++++++++++++++----------------------------- 1 file changed, 24 insertions(+), 41 deletions(-)
drivers/iommu/ipmmu-vmsa.c +24 -41
@@ -528,6 +528,27 @@ static struct iommu_domain *__ipmmu_domain_alloc(unsigned type)return &domain->io_domain; } +static struct iommu_domain *ipmmu_domain_alloc (unsigned type) +{ +struct iommu_domain *io_domain =NULL ; + +switch (type) { +case IOMMU_DOMAIN_UNMANAGED : + io_domain =__ipmmu_domain_alloc (type); +break ; + +case IOMMU_DOMAIN_DMA : + io_domain =__ipmmu_domain_alloc (type); +if (io_domain &&iommu_get_dma_cookie (io_domain)) { +kfree (io_domain); + io_domain =NULL ; + } +break ; + } + +return io_domain; +} +static void ipmmu_domain_free (struct iommu_domain *io_domain) {struct ipmmu_vmsa_domain *domain =to_vmsa_domain (io_domain);
@@ -536,6 +557,7 @@ static void ipmmu_domain_free(struct iommu_domain *io_domain)* Free the domain resources. We assume that all devices have already * been detached. */ +iommu_put_dma_cookie (io_domain);ipmmu_domain_destroy_context (domain);free_io_pgtable_ops (domain->iop);kfree (domain);
@@ -671,14 +693,6 @@ static int ipmmu_of_xlate(struct device *dev,#if defined(CONFIG_ARM ) && !defined(CONFIG_IOMMU_DMA ) -static struct iommu_domain *ipmmu_domain_alloc (unsigned type) -{ -if (type !=IOMMU_DOMAIN_UNMANAGED ) -return NULL ; - -return __ipmmu_domain_alloc (type); -} -static int ipmmu_add_device (struct device *dev) {struct ipmmu_vmsa_device *mmu =NULL ;
@@ -779,37 +793,6 @@ static const struct iommu_ops ipmmu_ops = {static DEFINE_SPINLOCK (ipmmu_slave_devices_lock);static LIST_HEAD (ipmmu_slave_devices); -static struct iommu_domain *ipmmu_domain_alloc_dma (unsigned type) -{ -struct iommu_domain *io_domain =NULL ; - -switch (type) { -case IOMMU_DOMAIN_UNMANAGED : - io_domain =__ipmmu_domain_alloc (type); -break ; - -case IOMMU_DOMAIN_DMA : - io_domain =__ipmmu_domain_alloc (type); -if (io_domain) -iommu_get_dma_cookie (io_domain); -break ; - } - -return io_domain; -} - -static void ipmmu_domain_free_dma (struct iommu_domain *io_domain) -{ -switch (io_domain->type) { -case IOMMU_DOMAIN_DMA : -iommu_put_dma_cookie (io_domain); -/* fall-through */ -default : -ipmmu_domain_free (io_domain); -break ; - } -} -static int ipmmu_add_device_dma (struct device *dev) {struct iommu_group *group;
@@ -878,8 +861,8 @@ static struct iommu_group *ipmmu_find_group_dma(struct device *dev) }static const struct iommu_ops ipmmu_ops = { - .domain_alloc = ipmmu_domain_alloc_dma, - .domain_free = ipmmu_domain_free_dma, + .domain_alloc = ipmmu_domain_alloc, + .domain_free = ipmmu_domain_free, .attach_dev = ipmmu_attach_device, .detach_dev = ipmmu_detach_device, .map = ipmmu_map,
Signature · 1 line
2.13.4.dirty
Robin Murphy
reviewerPATCH 2/4 iommu/ipmmu-vmsa: Simplify group allocation
In reply to Robin MurphyWe go through quite the merry dance in order to find masters behind the same IPMMU instance, so that we can ensure they are grouped together. None of which is really necessary, since the master's private data already points to the particular IPMMU it is associated with, and that IPMMU instance data is the perfect place to keep track of a per-instance group directly.
--- drivers/iommu/ipmmu-vmsa.c | 53 ++++++++-------------------------------------- 1 file changed, 9 insertions(+), 44 deletions(-)
drivers/iommu/ipmmu-vmsa.c +9 -44
@@ -43,6 +43,7 @@ struct ipmmu_vmsa_device {DECLARE_BITMAP (ctx,IPMMU_CTX_MAX );struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX ]; +struct iommu_group *group;struct dma_iommu_mapping *mapping; };
@@ -59,8 +60,6 @@ struct ipmmu_vmsa_domain {struct ipmmu_vmsa_iommu_priv {struct ipmmu_vmsa_device *mmu; -struct device *dev; -struct list_head list; };static struct ipmmu_vmsa_domain *to_vmsa_domain (struct iommu_domain *dom)
@@ -674,7 +673,6 @@ static int ipmmu_init_platform_device(struct device *dev,return -ENOMEM ; priv->mmu =platform_get_drvdata (ipmmu_pdev); - priv->dev = dev; dev->iommu_fwspec->iommu_priv = priv;return 0 ; }
@@ -790,9 +788,6 @@ static const struct iommu_ops ipmmu_ops = {#ifdef CONFIG_IOMMU_DMA -static DEFINE_SPINLOCK (ipmmu_slave_devices_lock); -static LIST_HEAD (ipmmu_slave_devices); -static int ipmmu_add_device_dma (struct device *dev) {struct iommu_group *group;
@@ -807,55 +802,25 @@ static int ipmmu_add_device_dma(struct device *dev)if (IS_ERR (group))return PTR_ERR (group); -spin_lock (&ipmmu_slave_devices_lock); -list_add (&to_priv (dev)->list, &ipmmu_slave_devices); -spin_unlock (&ipmmu_slave_devices_lock);return 0 ; }static void ipmmu_remove_device_dma (struct device *dev) { -struct ipmmu_vmsa_iommu_priv *priv =to_priv (dev); - -spin_lock (&ipmmu_slave_devices_lock); -list_del (&priv->list); -spin_unlock (&ipmmu_slave_devices_lock); -iommu_group_remove_device (dev); } -static struct device *ipmmu_find_sibling_device (struct device *dev) +static struct iommu_group *ipmmu_find_group (struct device *dev) {struct ipmmu_vmsa_iommu_priv *priv =to_priv (dev); -struct ipmmu_vmsa_iommu_priv *sibling_priv =NULL ; -bool found = false; - -spin_lock (&ipmmu_slave_devices_lock); - -list_for_each_entry (sibling_priv, &ipmmu_slave_devices, list) { -if (priv == sibling_priv) -continue ; -if (sibling_priv->mmu == priv->mmu) { - found = true; -break ; - } - } - -spin_unlock (&ipmmu_slave_devices_lock); - -return found ? sibling_priv->dev :NULL ; -} - -static struct iommu_group *ipmmu_find_group_dma (struct device *dev) -{struct iommu_group *group; -struct device *sibling; - sibling =ipmmu_find_sibling_device (dev); -if (sibling) - group =iommu_group_get (sibling); -if (!sibling ||IS_ERR (group)) - group =generic_device_group (dev); +if (priv->mmu->group) +return iommu_group_ref_get (priv->mmu->group); + + group =iommu_group_alloc (); +if (!IS_ERR (group)) + priv->mmu->group = group;return group; }
@@ -873,7 +838,7 @@ static const struct iommu_ops ipmmu_ops = { .iova_to_phys = ipmmu_iova_to_phys, .add_device = ipmmu_add_device_dma, .remove_device = ipmmu_remove_device_dma, - .device_group = ipmmu_find_group_dma, + .device_group = ipmmu_find_group, .pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K, .of_xlate = ipmmu_of_xlate, };
Signature · 1 line
2.13.4.dirty
Robin Murphy
reviewerPATCH 3/4 iommu/ipmmu-vmsa: Clean up struct ipmmu_vmsa_iommu_priv
In reply to Robin MurphyNow that the IPMMU instance pointer is the only thing remaining in the private data structure, we no longer need the extra level of indirection and can simply stash that directlty in the fwspec.
--- drivers/iommu/ipmmu-vmsa.c | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-)
drivers/iommu/ipmmu-vmsa.c +12 -24
@@ -58,16 +58,12 @@ struct ipmmu_vmsa_domain {spinlock_t lock;/* Protects mappings */ }; -struct ipmmu_vmsa_iommu_priv { -struct ipmmu_vmsa_device *mmu; -}; -static struct ipmmu_vmsa_domain *to_vmsa_domain (struct iommu_domain *dom) { return container_of(dom, struct ipmmu_vmsa_domain, io_domain); } -static struct ipmmu_vmsa_iommu_priv *to_priv (struct device *dev) +static struct ipmmu_vmsa_device *to_ipmmu (struct device *dev) {return dev->iommu_fwspec ? dev->iommu_fwspec->iommu_priv :NULL ; }
@@ -565,15 +561,14 @@ static void ipmmu_domain_free(struct iommu_domain *io_domain)static int ipmmu_attach_device (struct iommu_domain *io_domain,struct device *dev) { -struct ipmmu_vmsa_iommu_priv *priv =to_priv (dev);struct iommu_fwspec *fwspec = dev->iommu_fwspec; -struct ipmmu_vmsa_device *mmu = priv->mmu; +struct ipmmu_vmsa_device *mmu =to_ipmmu (dev);struct ipmmu_vmsa_domain *domain =to_vmsa_domain (io_domain);unsigned long flags;unsigned int i;int ret =0 ; -if (!priv || !priv->mmu) { +if (!mmu) {dev_err (dev,"Cannot attach to IPMMU\n" );return -ENXIO ; }
@@ -662,18 +657,12 @@ static int ipmmu_init_platform_device(struct device *dev, struct of_phandle_args *args) {struct platform_device *ipmmu_pdev; -struct ipmmu_vmsa_iommu_priv *priv; ipmmu_pdev =of_find_device_by_node (args->np);if (!ipmmu_pdev)return -ENODEV ; - priv =kzalloc (sizeof (*priv),GFP_KERNEL ); -if (!priv) -return -ENOMEM ; - - priv->mmu =platform_get_drvdata (ipmmu_pdev); - dev->iommu_fwspec->iommu_priv = priv; + dev->iommu_fwspec->iommu_priv =platform_get_drvdata (ipmmu_pdev);return 0 ; }
@@ -683,7 +672,7 @@ static int ipmmu_of_xlate(struct device *dev,iommu_fwspec_add_ids (dev, spec->args,1 );/* Initialize once - xlate() will call multiple times */ -if (to_priv (dev)) +if (to_ipmmu (dev))return 0 ;return ipmmu_init_platform_device (dev, spec);
@@ -693,14 +682,14 @@ static int ipmmu_of_xlate(struct device *dev,static int ipmmu_add_device (struct device *dev) { -struct ipmmu_vmsa_device *mmu =NULL ; +struct ipmmu_vmsa_device *mmu =to_ipmmu (dev);struct iommu_group *group;int ret;/* * Only let through devices that have been verified in xlate() */ -if (!to_priv (dev)) +if (!mmu)return -ENODEV ;/* Create a device group and add the device to it. */
@@ -729,7 +718,6 @@ static int ipmmu_add_device(struct device *dev)* - Make the mapping size configurable ? We currently use a 2GB mapping * at a 1GB offset to ensure that NULL VAs will fault. */ - mmu =to_priv (dev)->mmu;if (!mmu->mapping) {struct dma_iommu_mapping *mapping;
@@ -795,7 +783,7 @@ static int ipmmu_add_device_dma(struct device *dev)/* * Only let through devices that have been verified in xlate() */ -if (!to_priv (dev)) +if (!to_ipmmu (dev))return -ENODEV ; group =iommu_group_get_for_dev (dev);
@@ -812,15 +800,15 @@ static void ipmmu_remove_device_dma(struct device *dev)static struct iommu_group *ipmmu_find_group (struct device *dev) { -struct ipmmu_vmsa_iommu_priv *priv =to_priv (dev); +struct ipmmu_vmsa_device *mmu =to_ipmmu (dev);struct iommu_group *group; -if (priv->mmu->group) -return iommu_group_ref_get (priv->mmu->group); +if (mmu->group) +return iommu_group_ref_get (mmu->group); group =iommu_group_alloc ();if (!IS_ERR (group)) - priv->mmu->group = group; + mmu->group = group;return group; }
Signature · 1 line
2.13.4.dirty
Robin Murphy
reviewerPATCH 4/4 iommu/ipmmu-vmsa: Unify ipmmu_ops
In reply to Robin MurphyThe remaining difference between the ARM-specific and iommu-dma ops is in the {add,remove}_device implementations, but even those have some overlap and duplication. By stubbing out the few arm_iommu_*() calls, we can get rid of the rest of the inline #ifdeffery to both simplify the code and improve build coverage.
--- drivers/iommu/ipmmu-vmsa.c | 69 +++++++++++++--------------------------------- 1 file changed, 19 insertions(+), 50 deletions(-)
drivers/iommu/ipmmu-vmsa.c +19 -50
@@ -27,6 +27,11 @@#if defined(CONFIG_ARM ) && !defined(CONFIG_IOMMU_DMA )#include <asm/dma-iommu.h> #include <asm/pgalloc.h> +#else +#define arm_iommu_create_mapping (...) NULL +#define arm_iommu_attach_device (...) -ENODEV +#define arm_iommu_release_mapping (...) do {} while (0) +#define arm_iommu_detach_device (...) do {} while (0)#endif #include "io-pgtable.h"
@@ -678,26 +683,17 @@ static int ipmmu_of_xlate(struct device *dev,return ipmmu_init_platform_device (dev, spec); } -#if defined(CONFIG_ARM ) && !defined(CONFIG_IOMMU_DMA ) - -static int ipmmu_add_device (struct device *dev) +static int ipmmu_init_arm_mapping (struct device *dev) {struct ipmmu_vmsa_device *mmu =to_ipmmu (dev);struct iommu_group *group;int ret; -/* -* Only let through devices that have been verified in xlate() -*/ -if (!mmu) -return -ENODEV ; -/* Create a device group and add the device to it. */ group =iommu_group_alloc ();if (IS_ERR (group)) {dev_err (dev,"Failed to allocate IOMMU group\n" ); - ret =PTR_ERR (group); - goto error; +return PTR_ERR (group); } ret =iommu_group_add_device (group, dev);
@@ -705,8 +701,7 @@ static int ipmmu_add_device(struct device *dev)if (ret <0 ) {dev_err (dev,"Failed to add device to IPMMU group\n" ); - group =NULL ; - goto error; +return ret; }/*
@@ -742,41 +737,14 @@ static int ipmmu_add_device(struct device *dev)return 0 ; error: -if (mmu) +iommu_group_remove_device (dev); +if (mmu->mapping)arm_iommu_release_mapping (mmu->mapping); -if (!IS_ERR_OR_NULL (group)) -iommu_group_remove_device (dev); -return ret; } -static void ipmmu_remove_device (struct device *dev) -{ -arm_iommu_detach_device (dev); -iommu_group_remove_device (dev); -} - -static const struct iommu_ops ipmmu_ops = { - .domain_alloc = ipmmu_domain_alloc, - .domain_free = ipmmu_domain_free, - .attach_dev = ipmmu_attach_device, - .detach_dev = ipmmu_detach_device, - .map = ipmmu_map, - .unmap = ipmmu_unmap, - .map_sg = default_iommu_map_sg, - .iova_to_phys = ipmmu_iova_to_phys, - .add_device = ipmmu_add_device, - .remove_device = ipmmu_remove_device, - .pgsize_bitmap =SZ_1G |SZ_2M |SZ_4K , - .of_xlate = ipmmu_of_xlate, -}; - -#endif /* !CONFIG_ARM && CONFIG_IOMMU_DMA */ - -#ifdef CONFIG_IOMMU_DMA - -static int ipmmu_add_device_dma (struct device *dev) +static int ipmmu_add_device (struct device *dev) {struct iommu_group *group;
@@ -786,15 +754,20 @@ static int ipmmu_add_device_dma(struct device *dev)if (!to_ipmmu (dev))return -ENODEV ; +if (IS_ENABLED (CONFIG_ARM ) && !IS_ENABLED (CONFIG_IOMMU_DMA )) +return ipmmu_init_arm_mapping (dev); + group =iommu_group_get_for_dev (dev);if (IS_ERR (group))return PTR_ERR (group); +iommu_group_put (group);return 0 ; } -static void ipmmu_remove_device_dma (struct device *dev) +static void ipmmu_remove_device (struct device *dev) { +arm_iommu_detach_device (dev);iommu_group_remove_device (dev); }
@@ -824,15 +797,13 @@ static const struct iommu_ops ipmmu_ops = { .iotlb_sync = ipmmu_iotlb_sync, .map_sg = default_iommu_map_sg, .iova_to_phys = ipmmu_iova_to_phys, - .add_device = ipmmu_add_device_dma, - .remove_device = ipmmu_remove_device_dma, + .add_device = ipmmu_add_device, + .remove_device = ipmmu_remove_device, .device_group = ipmmu_find_group, .pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K, .of_xlate = ipmmu_of_xlate, }; -#endif /* CONFIG_IOMMU_DMA */ -/* ----------------------------------------------------------------------------- * Probe/remove and init */
@@ -929,9 +900,7 @@ static int ipmmu_remove(struct platform_device *pdev)iommu_device_sysfs_remove (&mmu->iommu);iommu_device_unregister (&mmu->iommu); -#if defined(CONFIG_ARM ) && !defined(CONFIG_IOMMU_DMA )arm_iommu_release_mapping (mmu->mapping); -#endif ipmmu_device_reset (mmu);
Signature · 1 line
2.13.4.dirty
cover ipmmu-vmsa cleanup
In reply to Robin MurphyQuoted 7 lines · Robin Murphy
Arnd reports a build warning[1] thanks to me missing ipmmu-vmsa's second set of ops when converting io-pgtable-arm users to the new iommu_iotlb_* callbacks. Rather than just treat the symptom with a point fix, this seemed like a good excuse to clean up the messy #ifdeffery and duplication in the driver that led to the oversight in the first place.
Build-tested successfully for arm64, ARM, and ARM +
CONFIG_IOMMU_DMA.
Very nice. I looked over the patches and while I only understood half of what you do and why it was different to start with, I very much welcome the end result.
Arnd
Alex Williamson
Appliedcover ipmmu-vmsa cleanup
In reply to Robin MurphyQuoted 12 lines · On Fri, 13 Oct 2017 19:23:38 +0100 Robin Murphy <[email protected]> wrote:
Arnd reports a build warning[1] thanks to me missing ipmmu-vmsa's second set of ops when converting io-pgtable-arm users to the new iommu_iotlb_* callbacks. Rather than just treat the symptom with a point fix, this seemed like a good excuse to clean up the messy #ifdeffery and duplication in the driver that led to the oversight in the first place.
Build-tested successfully for arm64, ARM, and ARM +
CONFIG_IOMMU_DMA.Robin.
[1]:https://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1510423.html
Quoted 9 lines
Robin Murphy (4):
iommu/ipmmu-vmsa: Unify domain alloc/free iommu/ipmmu-vmsa: Simplify group allocation iommu/ipmmu-vmsa: Clean up structipmmu_vmsa_iommu_priviommu/ipmmu-vmsa: Unifyipmmu_opsdrivers/iommu/ipmmu-vmsa.c | 251 ++++++++++++++------------------------------- 1 file changed, 78 insertions(+), 173 deletions(-)
Applied to iommu/ipmmu-vmsa for v4.15. Thanks,
Alex