On Thu, 2022-07-28 at 15:50 -0400, Matthew Rosato wrote: > On 7/28/22 12:05 PM, Eric Farman wrote: > > As pointed out with the simplification of the > > VFIO_IOMMU_NOTIFY_DMA_UNMAP notifier [1], the length > > parameter was never used to check against the pinned > > pages. > > > > Let's correct that, and see if a page is within the > > affected range instead of simply the first page of > > the range. > > > > [1] > > https://lore.kernel.org/kvm/20220720170457.39cda0d0.alex.williamson@xxxxxxxxxx/ > > > > Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx> > > Reviewed-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx> > > With one nit comment, feel free to change or not... > > > --- > > drivers/s390/cio/vfio_ccw_cp.c | 14 ++++++++++---- > > drivers/s390/cio/vfio_ccw_cp.h | 2 +- > > drivers/s390/cio/vfio_ccw_ops.c | 2 +- > > 3 files changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c > > b/drivers/s390/cio/vfio_ccw_cp.c > > index 8963f452f963..6202f1e3e792 100644 > > --- a/drivers/s390/cio/vfio_ccw_cp.c > > +++ b/drivers/s390/cio/vfio_ccw_cp.c > > @@ -170,12 +170,17 @@ static void page_array_unpin_free(struct > > page_array *pa, struct vfio_device *vde > > kfree(pa->pa_iova); > > } > > > > -static bool page_array_iova_pinned(struct page_array *pa, unsigned > > long iova) > > +static bool page_array_iova_pinned(struct page_array *pa, unsigned > > long iova, > > + unsigned long length) > > { > > + unsigned long iova_pfn_start = iova >> PAGE_SHIFT; > > + unsigned long iova_pfn_end = (iova + length - 1) >> PAGE_SHIFT; > > + unsigned long pfn; > > ^ why do we switch to unsigned longs here when the callers use u64 > for > length (and iova)? Maybe just stick with u64s? > > I thought maybe it was something introduced by Nicolin's series but > it > looks like the old pfn_array_iova_pinned did the same thing. Heh, been that way since the first vfio-ccw commit. Other places that use iova (CCW fetching, etc.) carry the u64 label, so this does jump out now that you mention it. I'll send a quick v3 because... > > > int i; > > > > for (i = 0; i < pa->pa_nr; i++) > > - if (pa->pa_iova[i] == iova) > > + pfn = pa->pa_iova[i] >> PAGE_SHIFT; > > + if (pfn >= iova_pfn_start && pfn <= iova_pfn_end) > > return true; ...this loop needs to be bracketed, and got lost in my sync between systems. > > > > return false; > > @@ -899,11 +904,12 @@ void cp_update_scsw(struct channel_program > > *cp, union scsw *scsw) > > * cp_iova_pinned() - check if an iova is pinned for a ccw chain. > > * @cp: channel_program on which to perform the operation > > * @iova: the iova to check > > + * @length: the length to check from @iova > > * > > * If the @iova is currently pinned for the ccw chain, return > > true; > > * else return false. > > */ > > -bool cp_iova_pinned(struct channel_program *cp, u64 iova) > > +bool cp_iova_pinned(struct channel_program *cp, u64 iova, u64 > > length) > > { > > struct ccwchain *chain; > > int i; > > @@ -913,7 +919,7 @@ bool cp_iova_pinned(struct channel_program *cp, > > u64 iova) > > > > list_for_each_entry(chain, &cp->ccwchain_list, next) { > > for (i = 0; i < chain->ch_len; i++) > > - if (page_array_iova_pinned(chain->ch_pa + i, > > iova)) > > + if (page_array_iova_pinned(chain->ch_pa + i, > > iova, length)) > > return true; > > } > > > > diff --git a/drivers/s390/cio/vfio_ccw_cp.h > > b/drivers/s390/cio/vfio_ccw_cp.h > > index 3194d887e08e..54d26e242533 100644 > > --- a/drivers/s390/cio/vfio_ccw_cp.h > > +++ b/drivers/s390/cio/vfio_ccw_cp.h > > @@ -46,6 +46,6 @@ void cp_free(struct channel_program *cp); > > int cp_prefetch(struct channel_program *cp); > > union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 > > lpm); > > void cp_update_scsw(struct channel_program *cp, union scsw > > *scsw); > > -bool cp_iova_pinned(struct channel_program *cp, u64 iova); > > +bool cp_iova_pinned(struct channel_program *cp, u64 iova, u64 > > length); > > > > #endif > > diff --git a/drivers/s390/cio/vfio_ccw_ops.c > > b/drivers/s390/cio/vfio_ccw_ops.c > > index 0047fd88f938..3f67fa103c7f 100644 > > --- a/drivers/s390/cio/vfio_ccw_ops.c > > +++ b/drivers/s390/cio/vfio_ccw_ops.c > > @@ -39,7 +39,7 @@ static void vfio_ccw_dma_unmap(struct vfio_device > > *vdev, u64 iova, u64 length) > > container_of(vdev, struct vfio_ccw_private, vdev); > > > > /* Drivers MUST unpin pages in response to an invalidation. */ > > - if (!cp_iova_pinned(&private->cp, iova)) > > + if (!cp_iova_pinned(&private->cp, iova, length)) > > return; > > > > vfio_ccw_mdev_reset(private);
