On Thu, 2013-01-17 at 12:20 +0900, Damian Hobson-Garcia wrote:
> I mistakenly sent this with the tag [PATCH] when it should have been
> [RFC]. Updated with this reply.
>
> Hi Alex,
> On 2013/01/17 6:47, Alex Williamson wrote:
> > On Tue, 2013-01-15 at 19:10 +0900, Damian Hobson-Garcia wrote:
> >> We've recently been thinking about the best way to attach platform devices
> >> in
> >> an IOMMU in a generic way without having to have every platform device
> >> register itself at probe time. We've seen that at least on the tegra
> >> IOMMU,
> >> using the platform_bus notifier seems like a viable option
> >> (http://lists.linuxfoundation.org/pipermail/iommu/2012-December/004954.html).
> >> We also noticed that
> >> the IOMMU ops add_device() function is called (indirectly) from a similar
> >> bus
> >> notifier and we're thinking that using the bus notifier in the common code
> >> would be better than us rolling our own to do basically the same thing.
> >>
> >> The problem, however, is that add_device() seems to be intended for use
> >> with
> >> IOMMU groups. i.e. After calling add_device() on a device that is should
> >> be
> >> in a group, dev->iommu_group should have some meaningful value.
> >> I think though, that if the IOMMU groups requirement can be removed, it
> >> should
> >> be more versatile.
> >>
> >> I believe that removing the IOMMU groups dependency can be achieved with
> >> the
> >> patch below. Currently the intel-iommu seems to be the only implementation
> >> using these callbacks.
> >
> > What stops you from creating a single iommu_group and adding all device
> > to it? Thanks,
> >
> > Alex
> Thanks for your comment.
>
> Nothing stops me, but putting everything in one group doesn't really add
> any benefit over not using groups at all, does it? It seems like its
> just adding more complexity(i.e code) to the driver without adding any
> additional functionality.
Does your iommu provide any isolation or is it translate-only. As Varun
notes, groups enable vfio, so if there's any possibility that you care
about userspace drivers for your devices or assigning devices to qemu
guests, then groups might be useful. The overhead of groups is really
meant to be very minimal for the iommu driver. That said, I guess I
have no real objection to generalizing the iommu bus notifier for
add/remove. Thanks,
Alex
> >> Signed-off-by: Damian Hobson-Garcia <dhobs...@igel.co.jp>
> >> ---
> >> drivers/iommu/intel-iommu.c | 3 ++-
> >> drivers/iommu/iommu.c | 2 +-
> >> include/linux/iommu.h | 4 ++--
> >> 3 files changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> >> index b9d0911..a763872 100644
> >> --- a/drivers/iommu/intel-iommu.c
> >> +++ b/drivers/iommu/intel-iommu.c
> >> @@ -4217,7 +4217,8 @@ root_bus:
> >>
> >> static void intel_iommu_remove_device(struct device *dev)
> >> {
> >> - iommu_group_remove_device(dev);
> >> + if (dev->iommu_group)
> >> + iommu_group_remove_device(dev);
> >> }
> >>
> >> static struct iommu_ops intel_iommu_ops = {
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index ddbdaca..a6046c2 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -510,7 +510,7 @@ static int iommu_bus_notifier(struct notifier_block
> >> *nb,
> >> if (ops->add_device)
> >> return ops->add_device(dev);
> >> } else if (action == BUS_NOTIFY_DEL_DEVICE) {
> >> - if (ops->remove_device && dev->iommu_group) {
> >> + if (ops->remove_device) {
> >> ops->remove_device(dev);
> >> return 0;
> >> }
> >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> >> index f3b99e1..2715592 100644
> >> --- a/include/linux/iommu.h
> >> +++ b/include/linux/iommu.h
> >> @@ -74,8 +74,8 @@ enum iommu_attr {
> >> * @unmap: unmap a physically contiguous memory region from an iommu
> >> domain
> >> * @iova_to_phys: translate iova to physical address
> >> * @domain_has_cap: domain capabilities query
> >> - * @add_device: add device to iommu grouping
> >> - * @remove_device: remove device from iommu grouping
> >> + * @add_device: add device to iommu grouping or domain
> >> + * @remove_device: remove device from iommu grouping or domain
> >> * @domain_get_attr: Query domain attributes
> >> * @domain_set_attr: Change domain attributes
> >> * @pgsize_bitmap: bitmap of supported page sizes
> >
> >
> >
>
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
