There's only one caller that checks its return value with a WARN_ON_ONCE,
while all other callers don't check the return value at all. Above that,
an undo function should not fail. So, simplify the API to return void by
embedding similar WARN_ONs.
Also for users to pinpoint which condition fails, separate WARN_ON lines,
yet remove the "driver->ops->unpin_pages" check, since it's unreasonable
for callers to unpin on something totally random that wasn't even pinned.
And remove NULL pointer checks for they would trigger oops vs. warnings.
Note that npage is already validated in the vfio core, thus drop the same
check in the type1 code.
Suggested-by: Christoph Hellwig <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Reviewed-by: Kirti Wankhede <[email protected]>
Tested-by: Terrence Xu <[email protected]>
Signed-off-by: Nicolin Chen <[email protected]>
---
.../driver-api/vfio-mediated-device.rst | 2 +-
drivers/gpu/drm/i915/gvt/kvmgt.c | 5 +----
drivers/vfio/vfio.c | 21 +++++++------------
drivers/vfio/vfio.h | 2 +-
drivers/vfio/vfio_iommu_type1.c | 15 ++++++-------
include/linux/vfio.h | 4 ++--
6 files changed, 18 insertions(+), 31 deletions(-)
diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
index d7a512676853..4307421dcaa0 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -263,7 +263,7 @@ driver::
int vfio_pin_pages(struct vfio_device *device, unsigned long *user_pfn,
int npage, int prot, unsigned long *phys_pfn);
- int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
+ void vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
int npage);
These functions call back into the back-end IOMMU module by using the pin_pages
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index e2f6c56ab342..8c67c9aba82d 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -231,18 +231,15 @@ static void intel_gvt_cleanup_vgpu_type_groups(struct intel_gvt *gvt)
static void gvt_unpin_guest_page(struct intel_vgpu *vgpu, unsigned long gfn,
unsigned long size)
{
- struct drm_i915_private *i915 = vgpu->gvt->gt->i915;
int total_pages;
int npage;
- int ret;
total_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
for (npage = 0; npage < total_pages; npage++) {
unsigned long cur_gfn = gfn + npage;
- ret = vfio_unpin_pages(&vgpu->vfio_device, &cur_gfn, 1);
- drm_WARN_ON(&i915->drm, ret != 1);
+ vfio_unpin_pages(&vgpu->vfio_device, &cur_gfn, 1);
}
}
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 100a3d84380c..ad90adbfddc8 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1964,31 +1964,24 @@ EXPORT_SYMBOL(vfio_pin_pages);
* PFNs should not be greater than VFIO_PIN_PAGES_MAX_ENTRIES.
* @npage [in] : count of elements in user_pfn array. This count should not
* be greater than VFIO_PIN_PAGES_MAX_ENTRIES.
- * Return error or number of pages unpinned.
*/
-int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
- int npage)
+void vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
+ int npage)
{
struct vfio_container *container;
struct vfio_iommu_driver *driver;
- int ret;
- if (!user_pfn || !npage || !vfio_assert_device_open(device))
- return -EINVAL;
+ if (WARN_ON(npage <= 0 || npage > VFIO_PIN_PAGES_MAX_ENTRIES))
+ return;
- if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
- return -E2BIG;
+ if (WARN_ON(!vfio_assert_device_open(device)))
+ return;
/* group->container cannot change while a vfio device is open */
container = device->group->container;
driver = container->iommu_driver;
- if (likely(driver && driver->ops->unpin_pages))
- ret = driver->ops->unpin_pages(container->iommu_data, user_pfn,
- npage);
- else
- ret = -ENOTTY;
- return ret;
+ driver->ops->unpin_pages(container->iommu_data, user_pfn, npage);
}
EXPORT_SYMBOL(vfio_unpin_pages);
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index a67130221151..bef4edf58138 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -53,7 +53,7 @@ struct vfio_iommu_driver_ops {
unsigned long *user_pfn,
int npage, int prot,
unsigned long *phys_pfn);
- int (*unpin_pages)(void *iommu_data,
+ void (*unpin_pages)(void *iommu_data,
unsigned long *user_pfn, int npage);
int (*register_notifier)(void *iommu_data,
unsigned long *events,
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index db24062fb343..cfeea4efd625 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -948,20 +948,16 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
return ret;
}
-static int vfio_iommu_type1_unpin_pages(void *iommu_data,
- unsigned long *user_pfn,
- int npage)
+static void vfio_iommu_type1_unpin_pages(void *iommu_data,
+ unsigned long *user_pfn, int npage)
{
struct vfio_iommu *iommu = iommu_data;
bool do_accounting;
int i;
- if (!iommu || !user_pfn || npage <= 0)
- return -EINVAL;
-
/* Supported for v2 version only */
- if (!iommu->v2)
- return -EACCES;
+ if (WARN_ON(!iommu->v2))
+ return;
mutex_lock(&iommu->lock);
@@ -979,7 +975,8 @@ static int vfio_iommu_type1_unpin_pages(void *iommu_data,
}
mutex_unlock(&iommu->lock);
- return i > 0 ? i : -EINVAL;
+
+ WARN_ON(i != npage);
}
static long vfio_sync_unpin(struct vfio_dma *dma, struct vfio_domain *domain,
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 4d26e149db81..5348ef353029 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -159,8 +159,8 @@ bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
int vfio_pin_pages(struct vfio_device *device, unsigned long *user_pfn,
int npage, int prot, unsigned long *phys_pfn);
-int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
- int npage);
+void vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
+ int npage);
int vfio_dma_rw(struct vfio_device *device, dma_addr_t user_iova,
void *data, size_t len, bool write);
--
2.17.1
On 7/8/22 6:44 PM, Nicolin Chen wrote:
> There's only one caller that checks its return value with a WARN_ON_ONCE,
> while all other callers don't check the return value at all. Above that,
> an undo function should not fail. So, simplify the API to return void by
> embedding similar WARN_ONs.
>
> Also for users to pinpoint which condition fails, separate WARN_ON lines,
> yet remove the "driver->ops->unpin_pages" check, since it's unreasonable
> for callers to unpin on something totally random that wasn't even pinned.
> And remove NULL pointer checks for they would trigger oops vs. warnings.
> Note that npage is already validated in the vfio core, thus drop the same
> check in the type1 code.
>
> Suggested-by: Christoph Hellwig <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Reviewed-by: Kirti Wankhede <[email protected]>
> Tested-by: Terrence Xu <[email protected]>
> Signed-off-by: Nicolin Chen <[email protected]>
> ---
> .../driver-api/vfio-mediated-device.rst | 2 +-
> drivers/gpu/drm/i915/gvt/kvmgt.c | 5 +----
> drivers/vfio/vfio.c | 21 +++++++------------
> drivers/vfio/vfio.h | 2 +-
> drivers/vfio/vfio_iommu_type1.c | 15 ++++++-------
> include/linux/vfio.h | 4 ++--
> 6 files changed, 18 insertions(+), 31 deletions(-)
>
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
> index d7a512676853..4307421dcaa0 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -263,7 +263,7 @@ driver::
> int vfio_pin_pages(struct vfio_device *device, unsigned long *user_pfn,
> int npage, int prot, unsigned long *phys_pfn);
>
> - int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> + void vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> int npage);
>
> These functions call back into the back-end IOMMU module by using the pin_pages
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index e2f6c56ab342..8c67c9aba82d 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -231,18 +231,15 @@ static void intel_gvt_cleanup_vgpu_type_groups(struct intel_gvt *gvt)
> static void gvt_unpin_guest_page(struct intel_vgpu *vgpu, unsigned long gfn,
> unsigned long size)
> {
> - struct drm_i915_private *i915 = vgpu->gvt->gt->i915;
> int total_pages;
> int npage;
> - int ret;
>
> total_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
>
> for (npage = 0; npage < total_pages; npage++) {
> unsigned long cur_gfn = gfn + npage;
>
> - ret = vfio_unpin_pages(&vgpu->vfio_device, &cur_gfn, 1);
> - drm_WARN_ON(&i915->drm, ret != 1);
> + vfio_unpin_pages(&vgpu->vfio_device, &cur_gfn, 1);
> }
> }
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 100a3d84380c..ad90adbfddc8 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1964,31 +1964,24 @@ EXPORT_SYMBOL(vfio_pin_pages);
> * PFNs should not be greater than VFIO_PIN_PAGES_MAX_ENTRIES.
> * @npage [in] : count of elements in user_pfn array. This count should not
> * be greater than VFIO_PIN_PAGES_MAX_ENTRIES.
> - * Return error or number of pages unpinned.
> */
> -int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> - int npage)
> +void vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> + int npage)
> {
> struct vfio_container *container;
> struct vfio_iommu_driver *driver;
> - int ret;
>
> - if (!user_pfn || !npage || !vfio_assert_device_open(device))
> - return -EINVAL;
You left out the check for !user_pfn?
> + if (WARN_ON(npage <= 0 || npage > VFIO_PIN_PAGES_MAX_ENTRIES))
> + return;
>
> - if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
> - return -E2BIG;
> + if (WARN_ON(!vfio_assert_device_open(device)))
> + return;
>
> /* group->container cannot change while a vfio device is open */
> container = device->group->container;
> driver = container->iommu_driver;
> - if (likely(driver && driver->ops->unpin_pages))
> - ret = driver->ops->unpin_pages(container->iommu_data, user_pfn,
> - npage);
> - else
> - ret = -ENOTTY;
>
> - return ret;
> + driver->ops->unpin_pages(container->iommu_data, user_pfn, npage);
> }
> EXPORT_SYMBOL(vfio_unpin_pages);
>
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index a67130221151..bef4edf58138 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -53,7 +53,7 @@ struct vfio_iommu_driver_ops {
> unsigned long *user_pfn,
> int npage, int prot,
> unsigned long *phys_pfn);
> - int (*unpin_pages)(void *iommu_data,
> + void (*unpin_pages)(void *iommu_data,
> unsigned long *user_pfn, int npage);
> int (*register_notifier)(void *iommu_data,
> unsigned long *events,
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index db24062fb343..cfeea4efd625 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -948,20 +948,16 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> return ret;
> }
>
> -static int vfio_iommu_type1_unpin_pages(void *iommu_data,
> - unsigned long *user_pfn,
> - int npage)
> +static void vfio_iommu_type1_unpin_pages(void *iommu_data,
> + unsigned long *user_pfn, int npage)
> {
> struct vfio_iommu *iommu = iommu_data;
> bool do_accounting;
> int i;
>
> - if (!iommu || !user_pfn || npage <= 0)
> - return -EINVAL;
Is there a reason the checks above were not checked for WARN_ON?
> -
> /* Supported for v2 version only */
> - if (!iommu->v2)
> - return -EACCES;
> + if (WARN_ON(!iommu->v2))
> + return;
>
> mutex_lock(&iommu->lock);
>
> @@ -979,7 +975,8 @@ static int vfio_iommu_type1_unpin_pages(void *iommu_data,
> }
>
> mutex_unlock(&iommu->lock);
> - return i > 0 ? i : -EINVAL;
> +
> + WARN_ON(i != npage);
> }
>
> static long vfio_sync_unpin(struct vfio_dma *dma, struct vfio_domain *domain,
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 4d26e149db81..5348ef353029 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -159,8 +159,8 @@ bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
>
> int vfio_pin_pages(struct vfio_device *device, unsigned long *user_pfn,
> int npage, int prot, unsigned long *phys_pfn);
> -int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> - int npage);
> +void vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> + int npage);
> int vfio_dma_rw(struct vfio_device *device, dma_addr_t user_iova,
> void *data, size_t len, bool write);
>
On Tue, Jul 12, 2022 at 10:21:14AM -0400, Anthony Krowiak wrote:
> > +void vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> > + int npage)
> > {
> > struct vfio_container *container;
> > struct vfio_iommu_driver *driver;
> > - int ret;
> >
> > - if (!user_pfn || !npage || !vfio_assert_device_open(device))
> > - return -EINVAL;
>
>
> You left out the check for !user_pfn?
Yes. I mentioned in the commit log. And it's in response to Jason's
remark: https://lore.kernel.org/kvm/[email protected]/
Btw, user_pfn is removed in one of the following patches anyway.
> > +static void vfio_iommu_type1_unpin_pages(void *iommu_data,
> > + unsigned long *user_pfn, int npage)
> > {
> > struct vfio_iommu *iommu = iommu_data;
> > bool do_accounting;
> > int i;
> >
> > - if (!iommu || !user_pfn || npage <= 0)
> > - return -EINVAL;
>
>
> Is there a reason the checks above were not checked for WARN_ON?
For pointers, same reason here.
For npage, it's checked in its caller vfio_unpin_pages -- mentioned
in the commit log too. The VFIO core is the only caller and it is
unlikely to change. On the other hand, the plan is to replace this
vfio_iommu_type1_unpin_pages with IOMMUFD implementation.