linux-kernel - Re: [PATCH v13 05/22] vfio iommu: Added pin and unpin callback functions to vfio_iommu_driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <2b57d51c-1012-cf55-bce7-78906285cc0f@nvidia.com>
Date:   Wed, 16 Nov 2016 20:36:46 +0530
From:   Kirti Wankhede <kwankhede@...dia.com>
To:     <alex.williamson@...hat.com>, <pbonzini@...hat.com>,
        <kraxel@...hat.com>, <cjia@...dia.com>, <qemu-devel@...gnu.org>,
        <kvm@...r.kernel.org>, <kevin.tian@...el.com>,
        <jike.song@...el.com>, <bjsdjshi@...ux.vnet.ibm.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v13 05/22] vfio iommu: Added pin and unpin callback
 functions to vfio_iommu_driver_ops



On 11/16/2016 8:33 AM, Dong Jia Shi wrote:
> * Kirti Wankhede <kwankhede@...dia.com> [2016-11-15 20:59:48 +0530]:
> 
> Hi Kirti,
> 
>> Added APIs for pining and unpining set of pages. These call back into
>> backend iommu module to actually pin and unpin pages.
>> 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.
>>
>> Renamed static functions in vfio_type1_iommu.c to resolve conflicts
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@...dia.com>
>> Signed-off-by: Neo Jia <cjia@...dia.com>
>> Change-Id: Ia7417723aaae86bec2959ad9ae6c2915ddd340e0
>> ---
>>  drivers/vfio/vfio.c             | 103 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/vfio/vfio_iommu_type1.c |  20 ++++----
>>  include/linux/vfio.h            |  14 +++++-
>>  3 files changed, 126 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index 2e83bdf007fe..3bf8a01bf67b 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -1799,6 +1799,109 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset)
>>  }
>>  EXPORT_SYMBOL_GPL(vfio_info_cap_shift);
>>
>> +
>> +/*
>> + * 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 to be unpinned. Number of user/guest
>> + *		  PFNs should not be greater than VFIO_PIN_PAGES_MAX_ENTRIES.
> Move the second sentence to the @npage section?
> 
>> + * @npage [in] :count of elements in array.  This count should not be greater
>> + *		than PAGE_SIZE.
> And remove the second sentence here.
> 
>> + * @prot [in] : protection flags
>> + * @phys_pfn[out] : array of host PFNs
> nit:
> I saw three differnt styles here:
>  @xxx [in] :xxxxxxx
>  @xxx [in]: xxxxxxx
>  @xxx[out]: xxxxxxx
> 
> Frankly speeking, I didn't think the [in|out] flags helps much.
> 
>> + * Return error or number of pages pinned.
>> + */
>> +int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, int npage,
>> +		   int prot, unsigned long *phys_pfn)
>> +{
>> +	struct vfio_container *container;
>> +	struct vfio_group *group;
>> +	struct vfio_iommu_driver *driver;
>> +	int ret;
>> +
>> +	if (!dev || !user_pfn || !phys_pfn || !npage)
>> +		return -EINVAL;
>> +
>> +	if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
>> +		return -E2BIG;
>> +
>> +	group = vfio_group_get_from_dev(dev);
>> +	if (IS_ERR(group))
>> +		return PTR_ERR(group);
>> +
>> +	ret = vfio_group_add_container_user(group);
>> +	if (ret)
>> +		goto err_pin_pages;
>> +
>> +	container = group->container;
>> +	down_read(&container->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);
>> +	else
>> +		ret = -ENOTTY;
>> +
>> +	up_read(&container->group_lock);
>> +	vfio_group_try_dissolve_container(group);
>> +
>> +err_pin_pages:
>> +	vfio_group_put(group);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(vfio_pin_pages);
>> +
>> +/*
>> + * Unpin set of host PFNs for local domain only.
>> + * @dev [in] : device
>> + * @user_pfn [in]: array of user/guest PFNs to be unpinned. Number of user/guest
>> + *		  PFNs should not be greater than VFIO_PIN_PAGES_MAX_ENTRIES.
>> + * @npage [in] :count of elements in array.  This count should not be greater
>> + *		than PAGE_SIZE.
> Same nits as above here.
> 
>> + * Return error or number of pages unpinned.
>> + */
> [...]
> 
>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
>> index 0ecae0b1cd34..420cdc928786 100644
>> --- a/include/linux/vfio.h
>> +++ b/include/linux/vfio.h
>> @@ -75,7 +75,11 @@ struct vfio_iommu_driver_ops {
>>  					struct iommu_group *group);
>>  	void		(*detach_group)(void *iommu_data,
>>  					struct iommu_group *group);
>> -
>> +	int		(*pin_pages)(void *iommu_data, unsigned long *user_pfn,
>> +				     int npage, int prot,
>> +				     unsigned long *phys_pfn);
>> +	int		(*unpin_pages)(void *iommu_data,
>> +				       unsigned long *user_pfn, int npage);
>>  };
>>
>>  extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
>> @@ -127,6 +131,14 @@ static inline long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
>>  }
>>  #endif /* CONFIG_EEH */
>>
>> +#define VFIO_PIN_PAGES_MAX_ENTRIES	(PAGE_SIZE/sizeof(unsigned long))
>> +
>> +extern int vfio_pin_pages(struct device *dev, unsigned long *user_pfn,
>> +			  int npage, int prot, unsigned long *phys_pfn);
>> +
>> +extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn,
>> +			    int npage);
>> +
> Move this hunk up to the "External user API" section?
> 

Ok

>>  /*
>>   * IRQfd - generic
>>   */
>> -- 
>> 2.7.0
>>
> 
> The code looks good to me.
> 

Thanks.