Re: [PATCH 8/8] iommu: Move default domain allocation to iommu_group_get_for_dev()
Hi Will, On Thu, Oct 29, 2015 at 06:22:49PM +, Will Deacon wrote: > The call to iommu_group_get_for_dev in arm_smmu_add_device will end up > calling __iommu_attach_device, since group->domain will now be initialised > by the code above. This means the SMMU driver will see an ->attach_dev > call for a device that is part-way through an ->add_device callback and > will be missing the initialisation necessary for us to idenfity the SMMU > instance to which is corresponds. In fact, the iommudata for the group > won't be initialised at all, so the whole thing will fail afaict. > > Note that I haven't actually taken this for a spin, so I could be missing > something. Yeah, I havn't looked at how to convert the ARM-SMMU drivers to default domains yet, so the issue you describe above is totally possible. But there is no way to trigger it yet, because your domain_alloc function can not yet allocate IOMMU_DOMAIN_DMA domains. While converting the issue must be fixed, of course. I tested this patch-set on an AMD Seattle system and it worked fine there. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 8/8] iommu: Move default domain allocation to iommu_group_get_for_dev()
Hi Joerg, Looking at this from an SMMU perspective, I think there's an issue with attaching to the default domain like this. On Wed, Oct 21, 2015 at 11:51:43PM +0200, Joerg Roedel wrote: > From: Joerg Roedel > > Now that the iommu core support for iommu groups is not > pci-centric anymore, we can move default domain allocation > to the bus independent iommu_group_get_for_dev() function. > > Signed-off-by: Joerg Roedel > --- > drivers/iommu/iommu.c | 18 ++ > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index e2b5526..abae363 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -810,14 +810,6 @@ struct iommu_group *pci_device_group(struct device *dev) > if (IS_ERR(group)) > return NULL; > > - /* > - * Try to allocate a default domain - needs support from the > - * IOMMU driver. > - */ > - group->default_domain = __iommu_domain_alloc(pdev->dev.bus, > - IOMMU_DOMAIN_DMA); > - group->domain = group->default_domain; > - > return group; > } > > @@ -849,6 +841,16 @@ struct iommu_group *iommu_group_get_for_dev(struct > device *dev) > if (IS_ERR(group)) > return group; > > + /* > + * Try to allocate a default domain - needs support from the > + * IOMMU driver. > + */ > + if (!group->default_domain) { > + group->default_domain = __iommu_domain_alloc(dev->bus, > + IOMMU_DOMAIN_DMA); > + group->domain = group->default_domain; > + } > + > ret = iommu_group_add_device(group, dev); > if (ret) { > iommu_group_put(group); The call to iommu_group_get_for_dev in arm_smmu_add_device will end up calling __iommu_attach_device, since group->domain will now be initialised by the code above. This means the SMMU driver will see an ->attach_dev call for a device that is part-way through an ->add_device callback and will be missing the initialisation necessary for us to idenfity the SMMU instance to which is corresponds. In fact, the iommudata for the group won't be initialised at all, so the whole thing will fail afaict. Note that I haven't actually taken this for a spin, so I could be missing something. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 8/8] iommu: Move default domain allocation to iommu_group_get_for_dev()
From: Joerg Roedel Now that the iommu core support for iommu groups is not pci-centric anymore, we can move default domain allocation to the bus independent iommu_group_get_for_dev() function. Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 18 ++ 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index e2b5526..abae363 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -810,14 +810,6 @@ struct iommu_group *pci_device_group(struct device *dev) if (IS_ERR(group)) return NULL; - /* -* Try to allocate a default domain - needs support from the -* IOMMU driver. -*/ - group->default_domain = __iommu_domain_alloc(pdev->dev.bus, -IOMMU_DOMAIN_DMA); - group->domain = group->default_domain; - return group; } @@ -849,6 +841,16 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev) if (IS_ERR(group)) return group; + /* +* Try to allocate a default domain - needs support from the +* IOMMU driver. +*/ + if (!group->default_domain) { + group->default_domain = __iommu_domain_alloc(dev->bus, +IOMMU_DOMAIN_DMA); + group->domain = group->default_domain; + } + ret = iommu_group_add_device(group, dev); if (ret) { iommu_group_put(group); -- 1.9.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu