Re: [PATCH v2 1/3] vfio/ccw: Add length to DMA_UNMAP checks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux