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

[Damian Hobson-Garcia <dhobsong-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>]

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