Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices

[Kirti Wankhede <kwankhede@nvidia.com>]

On 10/26/2016 12:49 PM, Tian, Kevin wrote:
>> From: Alex Williamson [mailto:alex.williamson@redhat.com]
>> Sent: Monday, October 24, 2016 10:32 AM
>>
>>>>> -static long vfio_unpin_pages(unsigned long pfn, long npage,
>>>>> -			     int prot, bool do_accounting)
>>>>> +static long __vfio_unpin_pages_remote(struct vfio_iommu *iommu,
>>>>> +				      unsigned long pfn, long npage, int prot,
>>>>> +				      bool do_accounting)
>>>>
>>>> Have you noticed that it's kind of confusing that
>>>> __vfio_{un}pin_pages_remote() uses current, which does a
>>>> get_user_pages_fast() while "local" uses a provided task_struct and
>>>> uses get_user_pages_*remote*()?  And also what was effectively local
>>>> (ie. we're pinning for our own use here) is now "remote" and pinning
>>>> for a remote, vendor driver consumer, is now "local".  It's not very
>>>> intuitive.
>>>>
> 
> I questioned this confusing naming in v8 too...
> 

I do tried to address your concerns on v8.

>>>
>>> 'local' in local_domain was suggested to describe the domain for local
>>> page tracking. Earlier suggestions to have 'mdev' or 'noimmu' in this
>>> name were discarded. May be we should revisit what the name should be.
>>> Any suggestion?
>>>
>>> For local_domain, to pin pages, flow is:
>>>
>>> for local_domain
>>>     |- vfio_pin_pages()
>>>         |- vfio_iommu_type1_pin_pages()
>>>             |- __vfio_pin_page_local()
>>>                 |-  vaddr_get_pfn(task->mm)
>>>                     |- get_user_pages_remote()
>>>
>>> __vfio_pin_page_local() --> get_user_pages_remote()
>>
>>
>> In vfio.c we have the concept of an external user, perhaps that could
>> be continued here.  An mdev driver would be an external, or remote
>> pinning.
>>
> 
> I prefer to use remote here. It's aligned with underlying mm operations
> 

Using 'remote' in this case is also confusing since it is already used
in this file. I liked Alex's suggestion to use external and I'll have
those changed in next version of patch set.

Kirti