| Message ID | 20220706062759.24946-2-nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update vfio_pin/unpin_pages API | expand |
> +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_ONCE(!user_pfn || !npage || !vfio_assert_device_open(device))) This adds an overly long line. Note that I think in general it is better to have an individual WARN_ON per condition anyway, as that allows to directly pinpoint what went wrong when it triggers. > + if (WARN_ON_ONCE(unlikely(!driver || !driver->ops->unpin_pages))) > + return; I'd just skip this check an let it crash. If someone calls unpin on something totally random that wasn't even pinned we don't need to handle that gracefully. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Jul 05, 2022 at 11:54:50PM -0700, Christoph Hellwig 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; > > + if (WARN_ON_ONCE(!user_pfn || !npage || !vfio_assert_device_open(device))) > > This adds an overly long line. Note that I think in general it is > better to have an individual WARN_ON per condition anyway, as that > allows to directly pinpoint what went wrong when it triggers. Following patches are touching this line too. And it'll be shrunk to a shorter line eventually by the end of PATCH-9. Yet, I can separate them as you pointed out. > > + if (WARN_ON_ONCE(unlikely(!driver || !driver->ops->unpin_pages))) > > + return; > > I'd just skip this check an let it crash. If someone calls unpin > on something totally random that wasn't even pinned we don't need to > handle that gracefully. Makes sense. I can drop that in next version. > Reviewed-by: Christoph Hellwig <hch@lst.de> Will add to v3. Thanks for the review!
On Tue, Jul 05, 2022 at 11:27:51PM -0700, Nicolin Chen wrote: > There's only one caller that checks its return value with a WARN_ON_ONCE, > while all other callers do not check return value at all. So simplify the > API to return void by embedding similar WARN_ON_ONCEs. > > Suggested-by: Christoph Hellwig <hch@infradead.org> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > .../driver-api/vfio-mediated-device.rst | 2 +- > drivers/gpu/drm/i915/gvt/kvmgt.c | 5 +--- > drivers/vfio/vfio.c | 24 ++++++++----------- > drivers/vfio/vfio.h | 2 +- > drivers/vfio/vfio_iommu_type1.c | 16 ++++++------- > include/linux/vfio.h | 4 ++-- > 6 files changed, 23 insertions(+), 30 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com> On 7/6/2022 11:57 AM, Nicolin Chen wrote: > There's only one caller that checks its return value with a WARN_ON_ONCE, > while all other callers do not check return value at all. So simplify the > API to return void by embedding similar WARN_ON_ONCEs. > > Suggested-by: Christoph Hellwig <hch@infradead.org> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > .../driver-api/vfio-mediated-device.rst | 2 +- > drivers/gpu/drm/i915/gvt/kvmgt.c | 5 +--- > drivers/vfio/vfio.c | 24 ++++++++----------- > drivers/vfio/vfio.h | 2 +- > drivers/vfio/vfio_iommu_type1.c | 16 ++++++------- > include/linux/vfio.h | 4 ++-- > 6 files changed, 23 insertions(+), 30 deletions(-) > > diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst > index 1c57815619fd..b0fdf76b339a 100644 > --- a/Documentation/driver-api/vfio-mediated-device.rst > +++ b/Documentation/driver-api/vfio-mediated-device.rst > @@ -265,7 +265,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 61e71c1154be..01f45ec70a3d 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -1959,31 +1959,27 @@ 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_ONCE(!user_pfn || !npage || !vfio_assert_device_open(device))) > + return; > > - if (npage > VFIO_PIN_PAGES_MAX_ENTRIES) > - return -E2BIG; > + if (WARN_ON_ONCE(npage > VFIO_PIN_PAGES_MAX_ENTRIES)) > + 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; > + if (WARN_ON_ONCE(unlikely(!driver || !driver->ops->unpin_pages))) > + return; > + > + 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 c13b9290e357..08613edaf722 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -948,20 +948,19 @@ 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; > + if (WARN_ON_ONCE(!iommu || !user_pfn || npage <= 0)) > + return; > > /* Supported for v2 version only */ > - if (!iommu->v2) > - return -EACCES; > + if (WARN_ON_ONCE(!iommu->v2)) > + return; > > mutex_lock(&iommu->lock); > > @@ -979,7 +978,8 @@ static int vfio_iommu_type1_unpin_pages(void *iommu_data, > } > > mutex_unlock(&iommu->lock); > - return i > 0 ? i : -EINVAL; > + > + WARN_ON_ONCE(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 49580fa2073a..d0844ecdc961 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -149,8 +149,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); >
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Wednesday, July 6, 2022 2:28 PM > > There's only one caller that checks its return value with a WARN_ON_ONCE, > while all other callers do not check return value at all. So simplify the > API to return void by embedding similar WARN_ON_ONCEs. While this change keeps the similar effect as before it leads to different policy for same type of errors between pin and unpin paths: e.g. vfio_unpin_pages(): if (WARN_ON_ONCE(!user_pfn || !npage || !vfio_assert_device_open(device))) return; vfio_pin_pages(): if (!user_pfn || !phys_pfn || !npage || !vfio_assert_device_open(device)) return -EINVAL; It sounds a bit weird when reading related code...
On Thu, Jul 07, 2022 at 08:42:28AM +0000, Tian, Kevin wrote: > External email: Use caution opening links or attachments > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > Sent: Wednesday, July 6, 2022 2:28 PM > > > > There's only one caller that checks its return value with a WARN_ON_ONCE, > > while all other callers do not check return value at all. So simplify the > > API to return void by embedding similar WARN_ON_ONCEs. > > While this change keeps the similar effect as before it leads to different > policy for same type of errors between pin and unpin paths: I think it's because of the policy that an undo function should not fail. Meanwhile, indulging faulty inputs isn't good either. > e.g. > > vfio_unpin_pages(): > if (WARN_ON_ONCE(!user_pfn || !npage || !vfio_assert_device_open(device))) > return; > > vfio_pin_pages(): > if (!user_pfn || !phys_pfn || !npage || > !vfio_assert_device_open(device)) > return -EINVAL; > > It sounds a bit weird when reading related code... Any better way to handle this?
On Thu, Jul 07, 2022 at 10:12:41AM -0700, Nicolin Chen wrote: > On Thu, Jul 07, 2022 at 08:42:28AM +0000, Tian, Kevin wrote: > > External email: Use caution opening links or attachments > > > > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > Sent: Wednesday, July 6, 2022 2:28 PM > > > > > > There's only one caller that checks its return value with a WARN_ON_ONCE, > > > while all other callers do not check return value at all. So simplify the > > > API to return void by embedding similar WARN_ON_ONCEs. > > > > While this change keeps the similar effect as before it leads to different > > policy for same type of errors between pin and unpin paths: > > I think it's because of the policy that an undo function should not > fail. Meanwhile, indulging faulty inputs isn't good either. > > > e.g. > > > > vfio_unpin_pages(): > > if (WARN_ON_ONCE(!user_pfn || !npage || !vfio_assert_device_open(device))) > > return; > > > > vfio_pin_pages(): > > if (!user_pfn || !phys_pfn || !npage || > > !vfio_assert_device_open(device)) > > return -EINVAL; > > > > It sounds a bit weird when reading related code... > > Any better way to handle this? They should all be WARN_ON's, that is the standard pattern to assert that function arguments must be correctly formed. I would also drop the tests that obviously will oops on their on anyone, like NULL pointer checks. This is a semi-performance path. Jason
On Thu, Jul 07, 2022 at 04:22:10PM -0300, Jason Gunthorpe wrote: > On Thu, Jul 07, 2022 at 10:12:41AM -0700, Nicolin Chen wrote: > > On Thu, Jul 07, 2022 at 08:42:28AM +0000, Tian, Kevin wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > > Sent: Wednesday, July 6, 2022 2:28 PM > > > > > > > > There's only one caller that checks its return value with a WARN_ON_ONCE, > > > > while all other callers do not check return value at all. So simplify the > > > > API to return void by embedding similar WARN_ON_ONCEs. > > > > > > While this change keeps the similar effect as before it leads to different > > > policy for same type of errors between pin and unpin paths: > > > > I think it's because of the policy that an undo function should not > > fail. Meanwhile, indulging faulty inputs isn't good either. > > > > > e.g. > > > > > > vfio_unpin_pages(): > > > if (WARN_ON_ONCE(!user_pfn || !npage || !vfio_assert_device_open(device))) > > > return; > > > > > > vfio_pin_pages(): > > > if (!user_pfn || !phys_pfn || !npage || > > > !vfio_assert_device_open(device)) > > > return -EINVAL; > > > > > > It sounds a bit weird when reading related code... > > > > Any better way to handle this? > > They should all be WARN_ON's, that is the standard pattern to assert > that function arguments must be correctly formed. OK. I can change that. I assume that, not confined to arguments, we might want to have a WARN_ON for the return value check also. > I would also drop the tests that obviously will oops on their on > anyone, like NULL pointer checks. This is a semi-performance path. OK. I will simply remove those NULL pointer checks. Actually, that !user_pfn check is gone anyway in the following patch, as user_pfn is replaced with iova.
diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst index 1c57815619fd..b0fdf76b339a 100644 --- a/Documentation/driver-api/vfio-mediated-device.rst +++ b/Documentation/driver-api/vfio-mediated-device.rst @@ -265,7 +265,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 61e71c1154be..01f45ec70a3d 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -1959,31 +1959,27 @@ 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_ONCE(!user_pfn || !npage || !vfio_assert_device_open(device))) + return; - if (npage > VFIO_PIN_PAGES_MAX_ENTRIES) - return -E2BIG; + if (WARN_ON_ONCE(npage > VFIO_PIN_PAGES_MAX_ENTRIES)) + 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; + if (WARN_ON_ONCE(unlikely(!driver || !driver->ops->unpin_pages))) + return; + + 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 c13b9290e357..08613edaf722 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -948,20 +948,19 @@ 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; + if (WARN_ON_ONCE(!iommu || !user_pfn || npage <= 0)) + return; /* Supported for v2 version only */ - if (!iommu->v2) - return -EACCES; + if (WARN_ON_ONCE(!iommu->v2)) + return; mutex_lock(&iommu->lock); @@ -979,7 +978,8 @@ static int vfio_iommu_type1_unpin_pages(void *iommu_data, } mutex_unlock(&iommu->lock); - return i > 0 ? i : -EINVAL; + + WARN_ON_ONCE(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 49580fa2073a..d0844ecdc961 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -149,8 +149,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);
There's only one caller that checks its return value with a WARN_ON_ONCE, while all other callers do not check return value at all. So simplify the API to return void by embedding similar WARN_ON_ONCEs. Suggested-by: Christoph Hellwig <hch@infradead.org> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- .../driver-api/vfio-mediated-device.rst | 2 +- drivers/gpu/drm/i915/gvt/kvmgt.c | 5 +--- drivers/vfio/vfio.c | 24 ++++++++----------- drivers/vfio/vfio.h | 2 +- drivers/vfio/vfio_iommu_type1.c | 16 ++++++------- include/linux/vfio.h | 4 ++-- 6 files changed, 23 insertions(+), 30 deletions(-)