| Message ID | 1422028288-891-12-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marek, Thank you for the patch. On Friday 23 January 2015 16:51:21 Marek Szyprowski wrote: > The driver doesn't need to do anything important in device add/remove > callbacks, because initialization will be done from device-tree specific > callbacks added later. IOMMU groups created by current code were never > used. IOMMU groups still seem a bit unclear to me. Will Deacon has nicely explained what they represent in http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310816.html. The IOMMU core doesn't make groups mandatory, but requires them in some code paths. For example the coldplug device add function add_iommu_group() called for all devices already registered when bus_set_iommu() is called will try to warn of devices added multiple times with a WARN_ON(dev->iommu_group). Another example is the iommu_bus_notifier() function which will call the remove_device() operation only when dev->iommu_group isn't NULL. I'm thus unsure whether groups should be made mandatory, or whether the IOMMU core should be fixed to make them really optional (or, third option, whether there's something I haven't understood properly). > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/iommu/exynos-iommu.c | 28 ---------------------------- > 1 file changed, 28 deletions(-) > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > index e62cb96..e40e699 100644 > --- a/drivers/iommu/exynos-iommu.c > +++ b/drivers/iommu/exynos-iommu.c > @@ -1055,32 +1055,6 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct > iommu_domain *domain, return phys; > } > > -static int exynos_iommu_add_device(struct device *dev) > -{ > - struct iommu_group *group; > - int ret; > - > - group = iommu_group_get(dev); > - > - if (!group) { > - group = iommu_group_alloc(); > - if (IS_ERR(group)) { > - dev_err(dev, "Failed to allocate IOMMU group\n"); > - return PTR_ERR(group); > - } > - } > - > - ret = iommu_group_add_device(group, dev); > - iommu_group_put(group); > - > - return ret; > -} > - > -static void exynos_iommu_remove_device(struct device *dev) > -{ > - iommu_group_remove_device(dev); > -} > - > static const struct iommu_ops exynos_iommu_ops = { > .domain_init = exynos_iommu_domain_init, > .domain_destroy = exynos_iommu_domain_destroy, > @@ -1090,8 +1064,6 @@ static const struct iommu_ops exynos_iommu_ops = { > .unmap = exynos_iommu_unmap, > .map_sg = default_iommu_map_sg, > .iova_to_phys = exynos_iommu_iova_to_phys, > - .add_device = exynos_iommu_add_device, > - .remove_device = exynos_iommu_remove_device, > .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, > };
Hi Laurent, On Sun, Jan 25, 2015 at 05:38:22PM +0200, Laurent Pinchart wrote: > IOMMU groups still seem a bit unclear to me. Will Deacon has nicely explained > what they represent in > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310816.html. > The IOMMU core doesn't make groups > mandatory, but requires them in some code paths. > > For example the coldplug device add function add_iommu_group() called for all > devices already registered when bus_set_iommu() is called will try to warn of > devices added multiple times with a WARN_ON(dev->iommu_group). Another example > is the iommu_bus_notifier() function which will call the remove_device() > operation only when dev->iommu_group isn't NULL. > > I'm thus unsure whether groups should be made mandatory, or whether the IOMMU > core should be fixed to make them really optional (or, third option, whether > there's something I haven't understood properly). My plan is to make IOMMU groups mandatory. I am currently preparing and RFC patch-set to introduce default-domains (which will be per group). So when all IOMMU drivers are converted to make use of default domains the iommu groups will be mandatory. Joerg
On Mon, Jan 26, 2015 at 11:00:59AM +0000, Joerg Roedel wrote: > On Sun, Jan 25, 2015 at 05:38:22PM +0200, Laurent Pinchart wrote: > > IOMMU groups still seem a bit unclear to me. Will Deacon has nicely explained > > what they represent in > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310816.html. > > The IOMMU core doesn't make groups > > mandatory, but requires them in some code paths. > > > > For example the coldplug device add function add_iommu_group() called for all > > devices already registered when bus_set_iommu() is called will try to warn of > > devices added multiple times with a WARN_ON(dev->iommu_group). Another example > > is the iommu_bus_notifier() function which will call the remove_device() > > operation only when dev->iommu_group isn't NULL. > > > > I'm thus unsure whether groups should be made mandatory, or whether the IOMMU > > core should be fixed to make them really optional (or, third option, whether > > there's something I haven't understood properly). > > My plan is to make IOMMU groups mandatory. I am currently preparing and > RFC patch-set to introduce default-domains (which will be per group). So > when all IOMMU drivers are converted to make use of default domains the > iommu groups will be mandatory. That makes sense to me, too. I think we should also consider extending the generic IOMMU device-tree binding to describe groups so that they can be instantiated by core code, without each driver having to work things out for itself. Will
Hello, On 2015-01-26 12:00, Joerg Roedel wrote: > Hi Laurent, > > On Sun, Jan 25, 2015 at 05:38:22PM +0200, Laurent Pinchart wrote: >> IOMMU groups still seem a bit unclear to me. Will Deacon has nicely explained >> what they represent in >> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310816.html. >> The IOMMU core doesn't make groups >> mandatory, but requires them in some code paths. >> >> For example the coldplug device add function add_iommu_group() called for all >> devices already registered when bus_set_iommu() is called will try to warn of >> devices added multiple times with a WARN_ON(dev->iommu_group). Another example >> is the iommu_bus_notifier() function which will call the remove_device() >> operation only when dev->iommu_group isn't NULL. >> >> I'm thus unsure whether groups should be made mandatory, or whether the IOMMU >> core should be fixed to make them really optional (or, third option, whether >> there's something I haven't understood properly). > My plan is to make IOMMU groups mandatory. I am currently preparing and > RFC patch-set to introduce default-domains (which will be per group). So > when all IOMMU drivers are converted to make use of default domains the > iommu groups will be mandatory. Thanks for the comment, I will implement all that will be needed for it to exynos iommu driver, but I would like to ask if you plan to merge my existing patches for exynos iommu driver to your tree? Best regards
Hi Joerg, On Monday 26 January 2015 12:00:59 Joerg Roedel wrote: > Hi Laurent, > > On Sun, Jan 25, 2015 at 05:38:22PM +0200, Laurent Pinchart wrote: > > IOMMU groups still seem a bit unclear to me. Will Deacon has nicely > > explained what they represent in > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310816 > > .html. The IOMMU core doesn't make groups > > mandatory, but requires them in some code paths. > > > > For example the coldplug device add function add_iommu_group() called for > > all devices already registered when bus_set_iommu() is called will try to > > warn of devices added multiple times with a WARN_ON(dev->iommu_group). > > Another example is the iommu_bus_notifier() function which will call the > > remove_device() operation only when dev->iommu_group isn't NULL. > > > > I'm thus unsure whether groups should be made mandatory, or whether the > > IOMMU core should be fixed to make them really optional (or, third > > option, whether there's something I haven't understood properly). > > My plan is to make IOMMU groups mandatory. I am currently preparing and > RFC patch-set to introduce default-domains (which will be per group). So > when all IOMMU drivers are converted to make use of default domains the > iommu groups will be mandatory. Could the default domain policy be configured by the IOMMU driver ? The ipmmu- vmsa driver supports four domains only, and serves up to 32 masters, with one group per master. A default of one domain per group wouldn't be usable.
Hi Laurent, On Mon, Jan 26, 2015 at 03:03:57PM +0200, Laurent Pinchart wrote: > On Monday 26 January 2015 12:00:59 Joerg Roedel wrote: > > My plan is to make IOMMU groups mandatory. I am currently preparing and > > RFC patch-set to introduce default-domains (which will be per group). So > > when all IOMMU drivers are converted to make use of default domains the > > iommu groups will be mandatory. > > Could the default domain policy be configured by the IOMMU driver ? The ipmmu- > vmsa driver supports four domains only, and serves up to 32 masters, with one > group per master. A default of one domain per group wouldn't be usable. I guess this limitation is a property of the iommu hardware. I currently have no support for this in the patch-set, but I think it is easy (and the best way) to support a model where we have only one default domain for all groups. This is not in the patch set right now, but will be added at some point. Joerg
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index e62cb96..e40e699 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -1055,32 +1055,6 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain, return phys; } -static int exynos_iommu_add_device(struct device *dev) -{ - struct iommu_group *group; - int ret; - - group = iommu_group_get(dev); - - if (!group) { - group = iommu_group_alloc(); - if (IS_ERR(group)) { - dev_err(dev, "Failed to allocate IOMMU group\n"); - return PTR_ERR(group); - } - } - - ret = iommu_group_add_device(group, dev); - iommu_group_put(group); - - return ret; -} - -static void exynos_iommu_remove_device(struct device *dev) -{ - iommu_group_remove_device(dev); -} - static const struct iommu_ops exynos_iommu_ops = { .domain_init = exynos_iommu_domain_init, .domain_destroy = exynos_iommu_domain_destroy, @@ -1090,8 +1064,6 @@ static const struct iommu_ops exynos_iommu_ops = { .unmap = exynos_iommu_unmap, .map_sg = default_iommu_map_sg, .iova_to_phys = exynos_iommu_iova_to_phys, - .add_device = exynos_iommu_add_device, - .remove_device = exynos_iommu_remove_device, .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE, };
The driver doesn't need to do anything important in device add/remove callbacks, because initialization will be done from device-tree specific callbacks added later. IOMMU groups created by current code were never used. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/iommu/exynos-iommu.c | 28 ---------------------------- 1 file changed, 28 deletions(-)