[PATCH] drivers: iommu: Use of add_device()/remove_device() outside of IOMMU groups

[PATCH] drivers: iommu: Use of add_device()/remove_device() outside of IOMMU groups

[Damian Hobson-Garcia]

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.

Signed-off-by: Damian Hobson-Garcia <dhobsong-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
---
 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
-- 
1.7.5.4

Re: [PATCH] drivers: iommu: Use of add_device()/remove_device() outside of IOMMU groups

[Alex Williamson]

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

> Signed-off-by: Damian Hobson-Garcia <dhobsong-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
> ---
>  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

Re: [RFC] drivers: iommu: Use of add_device()/remove_device() outside of IOMMU groups

[Damian Hobson-Garcia]

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.

Thanks,
Damian

>
>> Signed-off-by: Damian Hobson-Garcia <dhobsong-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
>> ---
>>   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
>
>
>


-- 
Damian Hobson-Garcia
IGEL Co.,Ltd
http://www.igel.co.jp

RE: [RFC] drivers: iommu: Use of add_device()/remove_device() outside of IOMMU groups

[Sethi Varun-B16395]

> -----Original Message-----
> From: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org [mailto:iommu-
> bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org] On Behalf Of Damian Hobson-Garcia
> Sent: Thursday, January 17, 2013 8:51 AM
> To: Alex Williamson
> Cc: hdk-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org;
> dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org; laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org
> Subject: Re: [RFC] drivers: iommu: Use of add_device()/remove_device()
> outside of IOMMU groups
> 
> 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/00495
> >> 4.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.
> 
Do you intend to use vfio? That's the specific objective of creating iommu groups.

-Varun

Re: [RFC] drivers: iommu: Use of add_device()/remove_device() outside of IOMMU groups

[Alex Williamson]

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 <dhobsong-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
> >> ---
> >>   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
> >
> >
> >
> 
> 

Re: [RFC] drivers: iommu: Use of add_device()/remove_device() outside of IOMMU groups

[Damian Hobson-Garcia]

Hi Alex,
On 2013/01/18 6:21, Alex Williamson wrote:
> 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,
>

Our iommu is used for translation.  We plan on using vfio at some point, 
though how well that fits with the current IOMMU ARM implementation 
remains to be seen. So considering your and Varun's comments, it looks 
like we want to be implementing groups after all, though generalizing 
the add/remove still seems to me like a good idea.

Thanks,
Damian
> Alex
>
>>>> Signed-off-by: Damian Hobson-Garcia <dhobsong-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
>>>> ---
>>>>    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
>>>
>>>
>>>
>>
>>
>
>
>


-- 
Damian Hobson-Garcia
IGEL Co.,Ltd
http://www.igel.co.jp