Re: [Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

2016-10-08 Thread Jike Song 
On 09/30/2016 07:44 PM, Kirti Wankhede wrote:
> On 9/30/2016 8:40 AM, Jike Song wrote:
>> On 09/30/2016 10:58 AM, Jike Song wrote:
>>> On 09/29/2016 11:06 PM, Kirti Wankhede wrote:


 On 9/29/2016 7:47 AM, Jike Song wrote:
> +Guangrong
>
> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:

 ...

>> +static long vfio_iommu_type1_pin_pages(void *iommu_data,
>> +   unsigned long *user_pfn,
>> +   long npage, int prot,
>> +   unsigned long *phys_pfn)
>> +{
>> +struct vfio_iommu *iommu = iommu_data;
>> +struct vfio_domain *domain;
>> +int i, j, ret;
>> +long retpage;
>> +unsigned long remote_vaddr;
>> +unsigned long *pfn = phys_pfn;
>> +struct vfio_dma *dma;
>> +bool do_accounting = false;
>> +
>> +if (!iommu || !user_pfn || !phys_pfn)
>> +return -EINVAL;
>> +
>> +mutex_lock(>lock);
>> +
>> +if (!iommu->local_domain) {
>> +ret = -EINVAL;
>> +goto pin_done;
>> +}
>> +
>> +domain = iommu->local_domain;
>> +
>> +/*
>> + * If iommu capable domain exist in the container then all 
>> pages are
>> + * already pinned and accounted. Accouting should be done if 
>> there is no
>> + * iommu capable domain in the container.
>> + */
>> +do_accounting = !IS_IOMMU_CAPABLE_DOMAIN_IN_CONTAINER(iommu);
>> +
>> +for (i = 0; i < npage; i++) {
>> +struct vfio_pfn *p;
>> +dma_addr_t iova;
>> +
>> +iova = user_pfn[i] << PAGE_SHIFT;
>> +
>> +dma = vfio_find_dma(iommu, iova, 0);
>> +if (!dma) {
>> +ret = -EINVAL;
>> +goto pin_unwind;
>> +}
>> +
>> +remote_vaddr = dma->vaddr + iova - dma->iova;
>> +
>> +retpage = __vfio_pin_pages_local(domain, remote_vaddr, 
>> prot,
>> + [i], 
>> do_accounting);
>
> Hi Kirti,
>
> Here you call __vfio_pin_pages_local() > vaddr_get_pfn() > GUP regardless
> whether the vaddr already pinned or not. That probably means, if the 
> caller 
> calls vfio_pin_pages() with a GPA for multiple times, you get memory 
> leaks.
>
> GUP always increases the page refcnt.
>
> FWIW, I would like to have the pfn_list_lock implemented with key == iova,
> so you can always try to find the PFN for a given iova, and pin it only if
> not found.
>

 I didn't get how there would be a memory leak.

 Right, GUP increases refcnt, so if vfio_pin_pages() is called for
 multiple types for same GPA, refcnt would be incremented. In
 vfio_iommu_type1_pin_pages() pinned pages list is maintained with
 ref_count. If pfn is already in list, ref_count is incremented and same
 is used while unpining pages.

>>>
>>> Let's have a close look at vfio_unpin_pfn:
>>>
>>> static int vfio_unpin_pfn(struct vfio_domain *domain,
>>>   struct vfio_pfn *vpfn, bool do_accounting)
>>> {
>>> __vfio_unpin_pages_for_mdev(domain, vpfn->pfn, vpfn->prot,
>>> do_accounting);
>>>
>>> if (atomic_dec_and_test(>ref_count))
>>> vfio_remove_from_pfn_list(domain, vpfn);
>>>
>>> return 1;
>>> }
>>>
>>> Here you didn't call __vfio_unpin_pages_for_mdev -- thereby put_page -- for
>>> vpfn->ref_count times. If page->_refcount increased by GUP for (N) times, 
>>> here
>>> you only set it back to (N-1).
>>>
> 
> User of vfio_pin_pages() should call vfio_unpin_pages() also,  so here
> we unpin it once. If vfio_pin_pages() is called twice for same page, we
> should get vfio_unpin_pages() twice for same page.
>

If this is the deliberate design, why do you need a 'ref_count'? You can
simply drop the 'ref_count' and blame the caller for pinning/unpinning
different times.

> If users of these APIs don't follow this, then
> vfio_release_domain() -> vfio_local_unpin_all() takes care of unpin,
> decrement ref_count and delete node on (ref_count == 0) for all
> remaining pfn.
>

Here you did pay attention to the "caller doesn't follow this" situation.
However, dealing with 'ref_count' in vfio-iommu is not enough: memory
leaked.

>>
>> What's more, since all pinned {iova, pfni} already saved, it's better to
>> consult it before calling GUP, which will get_page() unconditionally.
>
> pfn is required to unpin page, so we have pfn as key for rbtree.
> vfio_pin_pages() is called with user_pfn or

Re: [Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

2016-09-30 Thread Kirti Wankhede 


On 9/30/2016 8:40 AM, Jike Song wrote:
> On 09/30/2016 10:58 AM, Jike Song wrote:
>> On 09/29/2016 11:06 PM, Kirti Wankhede wrote:
>>>
>>>
>>> On 9/29/2016 7:47 AM, Jike Song wrote:
 +Guangrong

 On 08/25/2016 11:53 AM, Kirti Wankhede wrote:
>>>
>>> ...
>>>
> +static long vfio_iommu_type1_pin_pages(void *iommu_data,
> +unsigned long *user_pfn,
> +long npage, int prot,
> +unsigned long *phys_pfn)
> +{
> + struct vfio_iommu *iommu = iommu_data;
> + struct vfio_domain *domain;
> + int i, j, ret;
> + long retpage;
> + unsigned long remote_vaddr;
> + unsigned long *pfn = phys_pfn;
> + struct vfio_dma *dma;
> + bool do_accounting = false;
> +
> + if (!iommu || !user_pfn || !phys_pfn)
> + return -EINVAL;
> +
> + mutex_lock(>lock);
> +
> + if (!iommu->local_domain) {
> + ret = -EINVAL;
> + goto pin_done;
> + }
> +
> + domain = iommu->local_domain;
> +
> + /*
> +  * If iommu capable domain exist in the container then all pages are
> +  * already pinned and accounted. Accouting should be done if there is no
> +  * iommu capable domain in the container.
> +  */
> + do_accounting = !IS_IOMMU_CAPABLE_DOMAIN_IN_CONTAINER(iommu);
> +
> + for (i = 0; i < npage; i++) {
> + struct vfio_pfn *p;
> + dma_addr_t iova;
> +
> + iova = user_pfn[i] << PAGE_SHIFT;
> +
> + dma = vfio_find_dma(iommu, iova, 0);
> + if (!dma) {
> + ret = -EINVAL;
> + goto pin_unwind;
> + }
> +
> + remote_vaddr = dma->vaddr + iova - dma->iova;
> +
> + retpage = __vfio_pin_pages_local(domain, remote_vaddr, prot,
> +  [i], do_accounting);

 Hi Kirti,

 Here you call __vfio_pin_pages_local() > vaddr_get_pfn() > GUP regardless
 whether the vaddr already pinned or not. That probably means, if the 
 caller 
 calls vfio_pin_pages() with a GPA for multiple times, you get memory leaks.

 GUP always increases the page refcnt.

 FWIW, I would like to have the pfn_list_lock implemented with key == iova,
 so you can always try to find the PFN for a given iova, and pin it only if
 not found.

>>>
>>> I didn't get how there would be a memory leak.
>>>
>>> Right, GUP increases refcnt, so if vfio_pin_pages() is called for
>>> multiple types for same GPA, refcnt would be incremented. In
>>> vfio_iommu_type1_pin_pages() pinned pages list is maintained with
>>> ref_count. If pfn is already in list, ref_count is incremented and same
>>> is used while unpining pages.
>>>
>>
>> Let's have a close look at vfio_unpin_pfn:
>>
>>  static int vfio_unpin_pfn(struct vfio_domain *domain,
>>struct vfio_pfn *vpfn, bool do_accounting)
>>  {
>>  __vfio_unpin_pages_for_mdev(domain, vpfn->pfn, vpfn->prot,
>>  do_accounting);
>>
>>  if (atomic_dec_and_test(>ref_count))
>>  vfio_remove_from_pfn_list(domain, vpfn);
>>
>>  return 1;
>>  }
>>
>> Here you didn't call __vfio_unpin_pages_for_mdev -- thereby put_page -- for
>> vpfn->ref_count times. If page->_refcount increased by GUP for (N) times, 
>> here
>> you only set it back to (N-1).
>>

User of vfio_pin_pages() should call vfio_unpin_pages() also,  so here
we unpin it once. If vfio_pin_pages() is called twice for same page, we
should get vfio_unpin_pages() twice for same page.

If users of these APIs don't follow this, then
vfio_release_domain() -> vfio_local_unpin_all() takes care of unpin,
decrement ref_count and delete node on (ref_count == 0) for all
remaining pfn.

> 
> What's more, since all pinned {iova, pfni} already saved, it's better to
> consult it before calling GUP, which will get_page() unconditionally.

pfn is required to unpin page, so we have pfn as key for rbtree.
vfio_pin_pages() is called with user_pfn or iova, which can't be used to
search in rbtree with iova in optimized way. Raw way would be to goto
each node of rbtree and check iova which would hamper the performance in
if this is called in performance critical path.
So here optimized way is to first pin it, get pfn and check if already
exist in rbtree. If it exist increment ref_count else add it to the rbtree.

Thanks,
Kirti

Re: [Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

2016-09-29 Thread Jike Song 
On 09/30/2016 10:58 AM, Jike Song wrote:
> On 09/29/2016 11:06 PM, Kirti Wankhede wrote:
>>
>>
>> On 9/29/2016 7:47 AM, Jike Song wrote:
>>> +Guangrong
>>>
>>> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:
>>
>> ...
>>
 +static long vfio_iommu_type1_pin_pages(void *iommu_data,
 + unsigned long *user_pfn,
 + long npage, int prot,
 + unsigned long *phys_pfn)
 +{
 +  struct vfio_iommu *iommu = iommu_data;
 +  struct vfio_domain *domain;
 +  int i, j, ret;
 +  long retpage;
 +  unsigned long remote_vaddr;
 +  unsigned long *pfn = phys_pfn;
 +  struct vfio_dma *dma;
 +  bool do_accounting = false;
 +
 +  if (!iommu || !user_pfn || !phys_pfn)
 +  return -EINVAL;
 +
 +  mutex_lock(>lock);
 +
 +  if (!iommu->local_domain) {
 +  ret = -EINVAL;
 +  goto pin_done;
 +  }
 +
 +  domain = iommu->local_domain;
 +
 +  /*
 +   * If iommu capable domain exist in the container then all pages are
 +   * already pinned and accounted. Accouting should be done if there is no
 +   * iommu capable domain in the container.
 +   */
 +  do_accounting = !IS_IOMMU_CAPABLE_DOMAIN_IN_CONTAINER(iommu);
 +
 +  for (i = 0; i < npage; i++) {
 +  struct vfio_pfn *p;
 +  dma_addr_t iova;
 +
 +  iova = user_pfn[i] << PAGE_SHIFT;
 +
 +  dma = vfio_find_dma(iommu, iova, 0);
 +  if (!dma) {
 +  ret = -EINVAL;
 +  goto pin_unwind;
 +  }
 +
 +  remote_vaddr = dma->vaddr + iova - dma->iova;
 +
 +  retpage = __vfio_pin_pages_local(domain, remote_vaddr, prot,
 +   [i], do_accounting);
>>>
>>> Hi Kirti,
>>>
>>> Here you call __vfio_pin_pages_local() > vaddr_get_pfn() > GUP regardless
>>> whether the vaddr already pinned or not. That probably means, if the caller 
>>> calls vfio_pin_pages() with a GPA for multiple times, you get memory leaks.
>>>
>>> GUP always increases the page refcnt.
>>>
>>> FWIW, I would like to have the pfn_list_lock implemented with key == iova,
>>> so you can always try to find the PFN for a given iova, and pin it only if
>>> not found.
>>>
>>
>> I didn't get how there would be a memory leak.
>>
>> Right, GUP increases refcnt, so if vfio_pin_pages() is called for
>> multiple types for same GPA, refcnt would be incremented. In
>> vfio_iommu_type1_pin_pages() pinned pages list is maintained with
>> ref_count. If pfn is already in list, ref_count is incremented and same
>> is used while unpining pages.
>>
> 
> Let's have a close look at vfio_unpin_pfn:
> 
>   static int vfio_unpin_pfn(struct vfio_domain *domain,
> struct vfio_pfn *vpfn, bool do_accounting)
>   {
>   __vfio_unpin_pages_for_mdev(domain, vpfn->pfn, vpfn->prot,
>   do_accounting);
> 
>   if (atomic_dec_and_test(>ref_count))
>   vfio_remove_from_pfn_list(domain, vpfn);
> 
>   return 1;
>   }
> 
> Here you didn't call __vfio_unpin_pages_for_mdev -- thereby put_page -- for
> vpfn->ref_count times. If page->_refcount increased by GUP for (N) times, here
> you only set it back to (N-1).
> 

What's more, since all pinned {iova, pfni} already saved, it's better to
consult it before calling GUP, which will get_page() unconditionally.

--
Thanks,
Jike

Re: [Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

2016-09-29 Thread Jike Song 
On 09/29/2016 11:06 PM, Kirti Wankhede wrote:
> 
> 
> On 9/29/2016 7:47 AM, Jike Song wrote:
>> +Guangrong
>>
>> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:
> 
> ...
> 
>>> +static long vfio_iommu_type1_pin_pages(void *iommu_data,
>>> +  unsigned long *user_pfn,
>>> +  long npage, int prot,
>>> +  unsigned long *phys_pfn)
>>> +{
>>> +   struct vfio_iommu *iommu = iommu_data;
>>> +   struct vfio_domain *domain;
>>> +   int i, j, ret;
>>> +   long retpage;
>>> +   unsigned long remote_vaddr;
>>> +   unsigned long *pfn = phys_pfn;
>>> +   struct vfio_dma *dma;
>>> +   bool do_accounting = false;
>>> +
>>> +   if (!iommu || !user_pfn || !phys_pfn)
>>> +   return -EINVAL;
>>> +
>>> +   mutex_lock(>lock);
>>> +
>>> +   if (!iommu->local_domain) {
>>> +   ret = -EINVAL;
>>> +   goto pin_done;
>>> +   }
>>> +
>>> +   domain = iommu->local_domain;
>>> +
>>> +   /*
>>> +* If iommu capable domain exist in the container then all pages are
>>> +* already pinned and accounted. Accouting should be done if there is no
>>> +* iommu capable domain in the container.
>>> +*/
>>> +   do_accounting = !IS_IOMMU_CAPABLE_DOMAIN_IN_CONTAINER(iommu);
>>> +
>>> +   for (i = 0; i < npage; i++) {
>>> +   struct vfio_pfn *p;
>>> +   dma_addr_t iova;
>>> +
>>> +   iova = user_pfn[i] << PAGE_SHIFT;
>>> +
>>> +   dma = vfio_find_dma(iommu, iova, 0);
>>> +   if (!dma) {
>>> +   ret = -EINVAL;
>>> +   goto pin_unwind;
>>> +   }
>>> +
>>> +   remote_vaddr = dma->vaddr + iova - dma->iova;
>>> +
>>> +   retpage = __vfio_pin_pages_local(domain, remote_vaddr, prot,
>>> +[i], do_accounting);
>>
>> Hi Kirti,
>>
>> Here you call __vfio_pin_pages_local() > vaddr_get_pfn() > GUP regardless
>> whether the vaddr already pinned or not. That probably means, if the caller 
>> calls vfio_pin_pages() with a GPA for multiple times, you get memory leaks.
>>
>> GUP always increases the page refcnt.
>>
>> FWIW, I would like to have the pfn_list_lock implemented with key == iova,
>> so you can always try to find the PFN for a given iova, and pin it only if
>> not found.
>>
> 
> I didn't get how there would be a memory leak.
> 
> Right, GUP increases refcnt, so if vfio_pin_pages() is called for
> multiple types for same GPA, refcnt would be incremented. In
> vfio_iommu_type1_pin_pages() pinned pages list is maintained with
> ref_count. If pfn is already in list, ref_count is incremented and same
> is used while unpining pages.
> 

Let's have a close look at vfio_unpin_pfn:

static int vfio_unpin_pfn(struct vfio_domain *domain,
  struct vfio_pfn *vpfn, bool do_accounting)
{
__vfio_unpin_pages_for_mdev(domain, vpfn->pfn, vpfn->prot,
do_accounting);

if (atomic_dec_and_test(>ref_count))
vfio_remove_from_pfn_list(domain, vpfn);

return 1;
}

Here you didn't call __vfio_unpin_pages_for_mdev -- thereby put_page -- for
vpfn->ref_count times. If page->_refcount increased by GUP for (N) times, here
you only set it back to (N-1).

--
Thanks,
Jike

Re: [Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

2016-09-29 Thread Kirti Wankhede 


On 9/29/2016 7:47 AM, Jike Song wrote:
> +Guangrong
> 
> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:

...

>> +static long vfio_iommu_type1_pin_pages(void *iommu_data,
>> +   unsigned long *user_pfn,
>> +   long npage, int prot,
>> +   unsigned long *phys_pfn)
>> +{
>> +struct vfio_iommu *iommu = iommu_data;
>> +struct vfio_domain *domain;
>> +int i, j, ret;
>> +long retpage;
>> +unsigned long remote_vaddr;
>> +unsigned long *pfn = phys_pfn;
>> +struct vfio_dma *dma;
>> +bool do_accounting = false;
>> +
>> +if (!iommu || !user_pfn || !phys_pfn)
>> +return -EINVAL;
>> +
>> +mutex_lock(>lock);
>> +
>> +if (!iommu->local_domain) {
>> +ret = -EINVAL;
>> +goto pin_done;
>> +}
>> +
>> +domain = iommu->local_domain;
>> +
>> +/*
>> + * If iommu capable domain exist in the container then all pages are
>> + * already pinned and accounted. Accouting should be done if there is no
>> + * iommu capable domain in the container.
>> + */
>> +do_accounting = !IS_IOMMU_CAPABLE_DOMAIN_IN_CONTAINER(iommu);
>> +
>> +for (i = 0; i < npage; i++) {
>> +struct vfio_pfn *p;
>> +dma_addr_t iova;
>> +
>> +iova = user_pfn[i] << PAGE_SHIFT;
>> +
>> +dma = vfio_find_dma(iommu, iova, 0);
>> +if (!dma) {
>> +ret = -EINVAL;
>> +goto pin_unwind;
>> +}
>> +
>> +remote_vaddr = dma->vaddr + iova - dma->iova;
>> +
>> +retpage = __vfio_pin_pages_local(domain, remote_vaddr, prot,
>> + [i], do_accounting);
> 
> Hi Kirti,
> 
> Here you call __vfio_pin_pages_local() > vaddr_get_pfn() > GUP regardless
> whether the vaddr already pinned or not. That probably means, if the caller 
> calls vfio_pin_pages() with a GPA for multiple times, you get memory leaks.
> 
> GUP always increases the page refcnt.
> 
> FWIW, I would like to have the pfn_list_lock implemented with key == iova,
> so you can always try to find the PFN for a given iova, and pin it only if
> not found.
> 

I didn't get how there would be a memory leak.

Right, GUP increases refcnt, so if vfio_pin_pages() is called for
multiple types for same GPA, refcnt would be incremented. In
vfio_iommu_type1_pin_pages() pinned pages list is maintained with
ref_count. If pfn is already in list, ref_count is incremented and same
is used while unpining pages.

Kirti

Re: [Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

2016-09-28 Thread Jike Song 
+Guangrong

On 08/25/2016 11:53 AM, Kirti Wankhede wrote:
> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
> Mediated device only uses IOMMU APIs, the underlying hardware can be
> managed by an IOMMU domain.
> 
> Aim of this change is:
> - To use most of the code of TYPE1 IOMMU driver for mediated devices
> - To support direct assigned device and mediated device in single module
> 
> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
> IOMMU module that supports pining and unpinning pages for mdev devices
> should provide these functions.
> Added APIs for pining and unpining pages to VFIO module. These calls back
> into backend iommu module to actually pin and unpin pages.
> 
> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
> backend module. More details:
> - When iommu_group of mediated devices is attached, task structure is
>   cached which is used later to pin pages and page accounting.
> - It keeps track of pinned pages for mediated domain. This data is used to
>   verify unpinning request and to unpin remaining pages while detaching, if
>   there are any.
> - Used existing mechanism for page accounting. If iommu capable domain
>   exist in the container then all pages are already pinned and accounted.
>   Accouting for mdev device is only done if there is no iommu capable
>   domain in the container.
> 
> Tested by assigning below combinations of devices to a single VM:
> - GPU pass through only
> - vGPU device only
> - One GPU pass through and one vGPU device
> - two GPU pass through
> 
> Signed-off-by: Kirti Wankhede 
> Signed-off-by: Neo Jia 
> Change-Id: I295d6f0f2e0579b8d9882bfd8fd5a4194b97bd9a
> Reviewed-on: http://git-master/r/1175707
> Reviewed-by: Automatic_Commit_Validation_User
> ---
>  drivers/vfio/vfio.c | 117 ++
>  drivers/vfio/vfio_iommu_type1.c | 498 
> 
>  include/linux/vfio.h|  13 +-
>  3 files changed, 580 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 6fd6fa5469de..e3e342861e04 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1782,6 +1782,123 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, 
> size_t offset)
>  }
>  EXPORT_SYMBOL_GPL(vfio_info_cap_shift);
>  
> +static struct vfio_group *vfio_group_from_dev(struct device *dev)
> +{
> + struct vfio_device *device;
> + struct vfio_group *group;
> + int ret;
> +
> + device = vfio_device_get_from_dev(dev);
> + if (!device)
> + return ERR_PTR(-EINVAL);
> +
> + group = device->group;
> + if (!atomic_inc_not_zero(>container_users)) {
> + ret = -EINVAL;
> + goto err_ret;
> + }
> +
> + if (group->noiommu) {
> + atomic_dec(>container_users);
> + ret = -EPERM;
> + goto err_ret;
> + }
> +
> + if (!group->container->iommu_driver ||
> + !vfio_group_viable(group)) {
> + atomic_dec(>container_users);
> + ret = -EINVAL;
> + goto err_ret;
> + }
> +
> + vfio_device_put(device);
> + return group;
> +
> +err_ret:
> + vfio_device_put(device);
> + return ERR_PTR(ret);
> +}
> +
> +/*
> + * Pin a set of guest PFNs and return their associated host PFNs for local
> + * domain only.
> + * @dev [in] : device
> + * @user_pfn [in]: array of user/guest PFNs
> + * @npage [in]: count of array elements
> + * @prot [in] : protection flags
> + * @phys_pfn[out] : array of host PFNs
> + */
> +long vfio_pin_pages(struct device *dev, unsigned long *user_pfn,
> + long npage, int prot, unsigned long *phys_pfn)
> +{
> + struct vfio_container *container;
> + struct vfio_group *group;
> + struct vfio_iommu_driver *driver;
> + ssize_t ret = -EINVAL;
> +
> + if (!dev || !user_pfn || !phys_pfn)
> + return -EINVAL;
> +
> + group = vfio_group_from_dev(dev);
> + if (IS_ERR(group))
> + return PTR_ERR(group);
> +
> + container = group->container;
> + if (IS_ERR(container))
> + return PTR_ERR(container);
> +
> + down_read(>group_lock);
> +
> + driver = container->iommu_driver;
> + if (likely(driver && driver->ops->pin_pages))
> + ret = driver->ops->pin_pages(container->iommu_data, user_pfn,
> +  npage, prot, phys_pfn);
> +
> + up_read(>group_lock);
> + vfio_group_try_dissolve_container(group);
> +
> + return ret;
> +
> +}
> +EXPORT_SYMBOL(vfio_pin_pages);
> +
> +/*
> + * Unpin set of host PFNs for local domain only.
> + * @dev [in] : device
> + * @pfn [in] : array of host PFNs to be unpinned.
> + * @npage [in] :count of elements in array, that is number of pages.
> + */
> +long vfio_unpin_pages(struct device *dev, unsigned long *pfn, long npage)
> +{
> + struct vfio_container

Re: [Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

2016-08-26 Thread Kirti Wankhede 

Oh, that's the last minute change after running checkpatch.pl :(
Thanks for catching that. I'll correct that.

Thanks,
Kirti

On 8/25/2016 12:59 PM, Dong Jia wrote:
> On Thu, 25 Aug 2016 09:23:54 +0530
> Kirti Wankhede  wrote:
> 
>> @@ -769,6 +1090,33 @@ static int vfio_iommu_type1_attach_group(void 
>> *iommu_data,
>>  if (ret)
>>  goto out_free;
>>
>> +if (IS_ENABLED(CONFIF_VFIO_MDEV) && !iommu_present(bus) &&
> s/CONFIF_VFIO_MDEV/CONFIG_VFIO_MDEV/
> 
>> +(bus == _bus_type)) {
>> +if (iommu->local_domain) {
>> +list_add(>next,
>> + >local_domain->group_list);
>> +kfree(domain);
>> +mutex_unlock(>lock);
>> +return 0;
>> +}
>> +
> 
> 
> 
> Dong Jia
>

Re: [Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

2016-08-25 Thread Dong Jia 
On Thu, 25 Aug 2016 09:23:54 +0530
Kirti Wankhede  wrote:

> @@ -769,6 +1090,33 @@ static int vfio_iommu_type1_attach_group(void 
> *iommu_data,
>   if (ret)
>   goto out_free;
> 
> + if (IS_ENABLED(CONFIF_VFIO_MDEV) && !iommu_present(bus) &&
s/CONFIF_VFIO_MDEV/CONFIG_VFIO_MDEV/

> + (bus == _bus_type)) {
> + if (iommu->local_domain) {
> + list_add(>next,
> +  >local_domain->group_list);
> + kfree(domain);
> + mutex_unlock(>lock);
> + return 0;
> + }
> +



Dong Jia

[Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

2016-08-24 Thread Kirti Wankhede 
VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
Mediated device only uses IOMMU APIs, the underlying hardware can be
managed by an IOMMU domain.

Aim of this change is:
- To use most of the code of TYPE1 IOMMU driver for mediated devices
- To support direct assigned device and mediated device in single module

Added two new callback functions to struct vfio_iommu_driver_ops. Backend
IOMMU module that supports pining and unpinning pages for mdev devices
should provide these functions.
Added APIs for pining and unpining pages to VFIO module. These calls back
into backend iommu module to actually pin and unpin pages.

This change adds pin and unpin support for mediated device to TYPE1 IOMMU
backend module. More details:
- When iommu_group of mediated devices is attached, task structure is
  cached which is used later to pin pages and page accounting.
- It keeps track of pinned pages for mediated domain. This data is used to
  verify unpinning request and to unpin remaining pages while detaching, if
  there are any.
- Used existing mechanism for page accounting. If iommu capable domain
  exist in the container then all pages are already pinned and accounted.
  Accouting for mdev device is only done if there is no iommu capable
  domain in the container.

Tested by assigning below combinations of devices to a single VM:
- GPU pass through only
- vGPU device only
- One GPU pass through and one vGPU device
- two GPU pass through

Signed-off-by: Kirti Wankhede 
Signed-off-by: Neo Jia 
Change-Id: I295d6f0f2e0579b8d9882bfd8fd5a4194b97bd9a
Reviewed-on: http://git-master/r/1175707
Reviewed-by: Automatic_Commit_Validation_User
---
 drivers/vfio/vfio.c | 117 ++
 drivers/vfio/vfio_iommu_type1.c | 498 
 include/linux/vfio.h|  13 +-
 3 files changed, 580 insertions(+), 48 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 6fd6fa5469de..e3e342861e04 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1782,6 +1782,123 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, 
size_t offset)
 }
 EXPORT_SYMBOL_GPL(vfio_info_cap_shift);
 
+static struct vfio_group *vfio_group_from_dev(struct device *dev)
+{
+   struct vfio_device *device;
+   struct vfio_group *group;
+   int ret;
+
+   device = vfio_device_get_from_dev(dev);
+   if (!device)
+   return ERR_PTR(-EINVAL);
+
+   group = device->group;
+   if (!atomic_inc_not_zero(>container_users)) {
+   ret = -EINVAL;
+   goto err_ret;
+   }
+
+   if (group->noiommu) {
+   atomic_dec(>container_users);
+   ret = -EPERM;
+   goto err_ret;
+   }
+
+   if (!group->container->iommu_driver ||
+   !vfio_group_viable(group)) {
+   atomic_dec(>container_users);
+   ret = -EINVAL;
+   goto err_ret;
+   }
+
+   vfio_device_put(device);
+   return group;
+
+err_ret:
+   vfio_device_put(device);
+   return ERR_PTR(ret);
+}
+
+/*
+ * Pin a set of guest PFNs and return their associated host PFNs for local
+ * domain only.
+ * @dev [in] : device
+ * @user_pfn [in]: array of user/guest PFNs
+ * @npage [in]: count of array elements
+ * @prot [in] : protection flags
+ * @phys_pfn[out] : array of host PFNs
+ */
+long vfio_pin_pages(struct device *dev, unsigned long *user_pfn,
+   long npage, int prot, unsigned long *phys_pfn)
+{
+   struct vfio_container *container;
+   struct vfio_group *group;
+   struct vfio_iommu_driver *driver;
+   ssize_t ret = -EINVAL;
+
+   if (!dev || !user_pfn || !phys_pfn)
+   return -EINVAL;
+
+   group = vfio_group_from_dev(dev);
+   if (IS_ERR(group))
+   return PTR_ERR(group);
+
+   container = group->container;
+   if (IS_ERR(container))
+   return PTR_ERR(container);
+
+   down_read(>group_lock);
+
+   driver = container->iommu_driver;
+   if (likely(driver && driver->ops->pin_pages))
+   ret = driver->ops->pin_pages(container->iommu_data, user_pfn,
+npage, prot, phys_pfn);
+
+   up_read(>group_lock);
+   vfio_group_try_dissolve_container(group);
+
+   return ret;
+
+}
+EXPORT_SYMBOL(vfio_pin_pages);
+
+/*
+ * Unpin set of host PFNs for local domain only.
+ * @dev [in] : device
+ * @pfn [in] : array of host PFNs to be unpinned.
+ * @npage [in] :count of elements in array, that is number of pages.
+ */
+long vfio_unpin_pages(struct device *dev, unsigned long *pfn, long npage)
+{
+   struct vfio_container *container;
+   struct vfio_group *group;
+   struct vfio_iommu_driver *driver;
+   ssize_t ret = -EINVAL;
+
+   if (!dev || !pfn)
+   return -EINVAL;
+
+   group = vfio_group_from_dev(dev);
+   if (IS_ERR(group))
+