[PATCH 0/2] vfio-ccw fixes for 5.20

[PATCH 0/2] vfio-ccw fixes for 5.20

[Eric Farman]

Matt, Alex, Jason,

Here's two vfio-ccw patches for 5.20, built on Alex' vfio-next
tree (now that Nicolin's series has landed). One addresses the
DMA unmap problem that was identified last week, and another
was something I stumbled across while testing it.

Hopefully no big surprises in here; look forward to hearing
what you think.

Eric Farman (2):
  vfio/ccw: Add length to DMA_UNMAP checks
  vfio/ccw: Remove FSM Close from remove handlers

 drivers/s390/cio/vfio_ccw_cp.c  | 11 +++++++----
 drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
 drivers/s390/cio/vfio_ccw_drv.c |  1 -
 drivers/s390/cio/vfio_ccw_ops.c |  4 +---
 4 files changed, 9 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH 1/2] vfio/ccw: Add length to DMA_UNMAP checks

[Eric Farman]

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@redhat.com/

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c  | 11 +++++++----
 drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
 drivers/s390/cio/vfio_ccw_ops.c |  2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 8963f452f963..f15b5114abd1 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -170,12 +170,14 @@ 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)
 {
 	int i;
 
 	for (i = 0; i < pa->pa_nr; i++)
-		if (pa->pa_iova[i] == iova)
+		if (pa->pa_iova[i] >= iova &&
+		    pa->pa_iova[i] <= iova + length)
 			return true;
 
 	return false;
@@ -899,11 +901,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 +916,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);
-- 
2.34.1

[PATCH 2/2] vfio/ccw: Remove FSM Close from remove handlers

[Eric Farman]

Now that neither vfio_ccw_sch_probe() nor vfio_ccw_mdev_probe()
affect the FSM state, it doesn't make sense for their _remove()
counterparts try to revert things in this way. Since the FSM open
and close are handled alongside MDEV open/close, these are
unnecessary.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 1 -
 drivers/s390/cio/vfio_ccw_ops.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 4804101ccb0f..86d9e428357b 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -241,7 +241,6 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
 
-	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 	mdev_unregister_device(&sch->dev);
 
 	dev_set_drvdata(&sch->dev, NULL);
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 3f67fa103c7f..4a806a2273b5 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -130,8 +130,6 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 
 	vfio_unregister_group_dev(&private->vdev);
 
-	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
-
 	vfio_uninit_group_dev(&private->vdev);
 	atomic_inc(&private->avail);
 }
-- 
2.34.1

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

[Matthew Rosato]

On 7/26/22 11:01 AM, 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@redhat.com/
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_cp.c  | 11 +++++++----
>   drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
>   drivers/s390/cio/vfio_ccw_ops.c |  2 +-
>   3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 8963f452f963..f15b5114abd1 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -170,12 +170,14 @@ 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)
>   {
>   	int i;
>   
>   	for (i = 0; i < pa->pa_nr; i++)
> -		if (pa->pa_iova[i] == iova)
> +		if (pa->pa_iova[i] >= iova &&
> +		    pa->pa_iova[i] <= iova + length)

For the sake of completeness, I think you want to be checking to make 
sure the end of the page is also within the range, not just the start?

if (pa->pa_iova[i] >= iova &&
     pa->pa_iova[i] + PAGE_SIZE <= iova + length)

Re: [PATCH 2/2] vfio/ccw: Remove FSM Close from remove handlers

[Matthew Rosato]

On 7/26/22 11:01 AM, Eric Farman wrote:
> Now that neither vfio_ccw_sch_probe() nor vfio_ccw_mdev_probe()
> affect the FSM state, it doesn't make sense for their _remove()
> counterparts try to revert things in this way. Since the FSM open
> and close are handled alongside MDEV open/close, these are
> unnecessary.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
>   drivers/s390/cio/vfio_ccw_drv.c | 1 -
>   drivers/s390/cio/vfio_ccw_ops.c | 2 --
>   2 files changed, 3 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 4804101ccb0f..86d9e428357b 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -241,7 +241,6 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
>   {
>   	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>   
> -	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
>   	mdev_unregister_device(&sch->dev);
>   
>   	dev_set_drvdata(&sch->dev, NULL);
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 3f67fa103c7f..4a806a2273b5 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -130,8 +130,6 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
>   
>   	vfio_unregister_group_dev(&private->vdev);
>   
> -	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
> -
>   	vfio_uninit_group_dev(&private->vdev);
>   	atomic_inc(&private->avail);
>   }

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

[Eric Farman]

On Tue, 2022-07-26 at 12:12 -0400, Matthew Rosato wrote:
> On 7/26/22 11:01 AM, 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@redhat.com/
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >   drivers/s390/cio/vfio_ccw_cp.c  | 11 +++++++----
> >   drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
> >   drivers/s390/cio/vfio_ccw_ops.c |  2 +-
> >   3 files changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_cp.c
> > b/drivers/s390/cio/vfio_ccw_cp.c
> > index 8963f452f963..f15b5114abd1 100644
> > --- a/drivers/s390/cio/vfio_ccw_cp.c
> > +++ b/drivers/s390/cio/vfio_ccw_cp.c
> > @@ -170,12 +170,14 @@ 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)
> >   {
> >   	int i;
> >   
> >   	for (i = 0; i < pa->pa_nr; i++)
> > -		if (pa->pa_iova[i] == iova)
> > +		if (pa->pa_iova[i] >= iova &&
> > +		    pa->pa_iova[i] <= iova + length)
> 
> For the sake of completeness, I think you want to be checking to
> make 
> sure the end of the page is also within the range, not just the
> start?
> 
> if (pa->pa_iova[i] >= iova &&
>      pa->pa_iova[i] + PAGE_SIZE <= iova + length)

Well +PAGE_SIZE would iterate to the next page, so that would be
captured on the next iteration of the for(i) loop if the pages were
contiguous (or not applicable, if the pages weren't).

But, since the comment is really about the end of the page (0xfff), I
guess I'm not understanding what that gets us so perhaps you could help
elaborate your question? From my chair, since the pa_iova argument
passed to vfio_pin_pages() pins the whole page, checking the start
address versus the end (or anywhere in between) should still capture
its interaction with an affected range. That is to say, we don't care
about the -whole- page being within the unmap range, but -any- part of
it.

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

[Matthew Rosato]

On 7/27/22 12:45 PM, Eric Farman wrote:
> On Tue, 2022-07-26 at 12:12 -0400, Matthew Rosato wrote:
>> On 7/26/22 11:01 AM, 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@redhat.com/
>>>
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>> ---
>>>    drivers/s390/cio/vfio_ccw_cp.c  | 11 +++++++----
>>>    drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
>>>    drivers/s390/cio/vfio_ccw_ops.c |  2 +-
>>>    3 files changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c
>>> b/drivers/s390/cio/vfio_ccw_cp.c
>>> index 8963f452f963..f15b5114abd1 100644
>>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>>> @@ -170,12 +170,14 @@ 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)
>>>    {
>>>    	int i;
>>>    
>>>    	for (i = 0; i < pa->pa_nr; i++)
>>> -		if (pa->pa_iova[i] == iova)
>>> +		if (pa->pa_iova[i] >= iova &&
>>> +		    pa->pa_iova[i] <= iova + length)
>>
>> For the sake of completeness, I think you want to be checking to
>> make
>> sure the end of the page is also within the range, not just the
>> start?
>>
>> if (pa->pa_iova[i] >= iova &&
>>       pa->pa_iova[i] + PAGE_SIZE <= iova + length)
> 
> Well +PAGE_SIZE would iterate to the next page, so that would be
> captured on the next iteration of the for(i) loop if the pages were
> contiguous (or not applicable, if the pages weren't).

FWIW, the '+ PAGE_SIZE' was to match the '+ length' in your comparison.

If you really only want to only look at the start of the pa_iova being 
within the range, then I think you want 'pa->pa_iova[i] < iova + 
length', not <=.

> 
> But, since the comment is really about the end of the page (0xfff), I
> guess I'm not understanding what that gets us so perhaps you could help
> elaborate your question? From my chair, since the pa_iova argument
> passed to vfio_pin_pages() pins the whole page, checking the start
> address versus the end (or anywhere in between) should still capture
> its interaction with an affected range. That is to say, we don't care
> about the -whole- page being within the unmap range, but -any- part of
> it.
> 

As far as my suggestion to also look at the end of the pa_iova[i] -- 
This was particularly geared at ensuring the entire page fell within the 
range, not just a subset.  But I think you're right, we don't really 
care about that.  On the flip side, do we care if the iova somehow 
starts sometime between pa_iova[i] and pa_iova[i] + PAGE_SIZE - 1?  That 
would still be a subset, though I'm not sure such a thing could happen 
today (e.g. an input 'iova' that is not on a page boundary)..

I wonder if the simplest thing would be to just copy what gvt does and 
convert to pfn as it takes all of this out of the equation and looks 
instead at whether the inputs overlaps at a page granularity (which is 
what we really care about), e.g. something like (untested):

u64 iov_pfn = iova >> PAGE_SHIFT;
u64 end_iov_pfn = iov_pfn + (length / PAGE_SIZE);
u64 pfn;
int i;

for (i = 0; i < pa->pa_nr; i++) {
    pfn = pa->pa_iova[i] >> PAGE_SHIFT;
    if (pfn >= iov_pfn && pfn < end_iov_pfn)
	return true;
}

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

[Eric Farman]

On Wed, 2022-07-27 at 13:16 -0400, Matthew Rosato wrote:
> On 7/27/22 12:45 PM, Eric Farman wrote:
> > On Tue, 2022-07-26 at 12:12 -0400, Matthew Rosato wrote:
> > > On 7/26/22 11:01 AM, 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@redhat.com/
> > > > 
> > > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > > > ---
> > > >    drivers/s390/cio/vfio_ccw_cp.c  | 11 +++++++----
> > > >    drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
> > > >    drivers/s390/cio/vfio_ccw_ops.c |  2 +-
> > > >    3 files changed, 9 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c
> > > > b/drivers/s390/cio/vfio_ccw_cp.c
> > > > index 8963f452f963..f15b5114abd1 100644
> > > > --- a/drivers/s390/cio/vfio_ccw_cp.c
> > > > +++ b/drivers/s390/cio/vfio_ccw_cp.c
> > > > @@ -170,12 +170,14 @@ 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)
> > > >    {
> > > >    	int i;
> > > >    
> > > >    	for (i = 0; i < pa->pa_nr; i++)
> > > > -		if (pa->pa_iova[i] == iova)
> > > > +		if (pa->pa_iova[i] >= iova &&
> > > > +		    pa->pa_iova[i] <= iova + length)
> > > 
> > > For the sake of completeness, I think you want to be checking to
> > > make
> > > sure the end of the page is also within the range, not just the
> > > start?
> > > 
> > > if (pa->pa_iova[i] >= iova &&
> > >       pa->pa_iova[i] + PAGE_SIZE <= iova + length)
> > 
> > Well +PAGE_SIZE would iterate to the next page, so that would be
> > captured on the next iteration of the for(i) loop if the pages were
> > contiguous (or not applicable, if the pages weren't).
> 
> FWIW, the '+ PAGE_SIZE' was to match the '+ length' in your
> comparison.
> 
> If you really only want to only look at the start of the pa_iova
> being 
> within the range, then I think you want 'pa->pa_iova[i] < iova + 
> length', not <=.

Touche.

> 
> > But, since the comment is really about the end of the page (0xfff),
> > I
> > guess I'm not understanding what that gets us so perhaps you could
> > help
> > elaborate your question? From my chair, since the pa_iova argument
> > passed to vfio_pin_pages() pins the whole page, checking the start
> > address versus the end (or anywhere in between) should still
> > capture
> > its interaction with an affected range. That is to say, we don't
> > care
> > about the -whole- page being within the unmap range, but -any- part
> > of
> > it.
> > 
> 
> As far as my suggestion to also look at the end of the pa_iova[i] -- 
> This was particularly geared at ensuring the entire page fell within
> the 
> range, not just a subset.  But I think you're right, we don't really 
> care about that.  On the flip side, do we care if the iova somehow 
> starts sometime between pa_iova[i] and pa_iova[i] + PAGE_SIZE - 1?  

As I was mulling your original reply, I was having the same thought. I
think the answer is it's probably unlikely, but was trying to figure
out how to rope something in for that.

> That 
> would still be a subset, though I'm not sure such a thing could
> happen 
> today (e.g. an input 'iova' that is not on a page boundary)..
> 
> I wonder if the simplest thing would be to just copy what gvt does
> and 
> convert to pfn as it takes all of this out of the equation and looks 
> instead at whether the inputs overlaps at a page granularity (which
> is 
> what we really care about)

Agreed.

> , e.g. something like (untested):

Ha. This is almost exactly what I had in place originally, before I
applied Nicolin's series and the pfns were still available in the
struct that pa points to. I should have left that all in place instead
of cleaning it up this way. Let me get that refit and doublechecked,
and I'll send a v2. Thanks!

> 
> u64 iov_pfn = iova >> PAGE_SHIFT;
> u64 end_iov_pfn = iov_pfn + (length / PAGE_SIZE);
> u64 pfn;
> int i;
> 
> for (i = 0; i < pa->pa_nr; i++) {
>     pfn = pa->pa_iova[i] >> PAGE_SHIFT;
>     if (pfn >= iov_pfn && pfn < end_iov_pfn)
> 	return true;
> }
> 
> 
>