[PATCH v2 0/3] vfio-ccw fixes for 5.20

[PATCH v2 0/3] vfio-ccw fixes for 5.20

[Eric Farman]

Matt, Alex,

Here is an updated series for the DMA UNMAP length problem.
As before, it is built on Alex' vfio-next tree, and contains
two small fixes identified while testing this.

Changelog:
v1->v2:
 - [MR] Rework the boundary checking of page_array_iova_pinned,
   to compare against pfns instead of iovas.
 - [MR] Add r-b to Patch 2 (Thank you!)
 - [EF] (NEW) Noticed a missing return code check in the close routine
v1: https://lore.kernel.org/r/20220726150123.2567761-1-farman@linux.ibm.com/

Eric Farman (3):
  vfio/ccw: Add length to DMA_UNMAP checks
  vfio/ccw: Remove FSM Close from remove handlers
  vfio/ccw: Check return code from subchannel quiesce

 drivers/s390/cio/vfio_ccw_cp.c  | 14 ++++++++++----
 drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
 drivers/s390/cio/vfio_ccw_drv.c |  1 -
 drivers/s390/cio/vfio_ccw_fsm.c |  2 +-
 drivers/s390/cio/vfio_ccw_ops.c |  4 +---
 5 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] 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  | 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;
 	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;
 
 	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);
-- 
2.34.1

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

[Matthew Rosato]

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@redhat.com/
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

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

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.

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

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

[Eric Farman]

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@redhat.com/
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> 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);

[PATCH v2 2/3] 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>
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);
 }
-- 
2.34.1

[PATCH v2 3/3] vfio/ccw: Check return code from subchannel quiesce

[Eric Farman]

If a subchannel is busy when a close is performed, the subchannel
needs to be quiesced and left nice and tidy, so nothing unexpected
(like a solicited interrupt) shows up while in the closed state.
Unfortunately, the return code from this call isn't checked,
so any busy subchannel is treated as a failing one.

Fix that, so that the close on a busy subchannel happens normally.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_fsm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 4b8b623df24f..a59c758869f8 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -407,7 +407,7 @@ static void fsm_close(struct vfio_ccw_private *private,
 
 	ret = cio_disable_subchannel(sch);
 	if (ret == -EBUSY)
-		vfio_ccw_sch_quiesce(sch);
+		ret = vfio_ccw_sch_quiesce(sch);
 	if (ret)
 		goto err_unlock;
 
-- 
2.34.1

Re: [PATCH v2 3/3] vfio/ccw: Check return code from subchannel quiesce

[Matthew Rosato]

On 7/28/22 12:05 PM, Eric Farman wrote:
> If a subchannel is busy when a close is performed, the subchannel
> needs to be quiesced and left nice and tidy, so nothing unexpected
> (like a solicited interrupt) shows up while in the closed state.
> Unfortunately, the return code from this call isn't checked,
> so any busy subchannel is treated as a failing one.
> 
> Fix that, so that the close on a busy subchannel happens normally.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

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

> ---
>   drivers/s390/cio/vfio_ccw_fsm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4b8b623df24f..a59c758869f8 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -407,7 +407,7 @@ static void fsm_close(struct vfio_ccw_private *private,
>   
>   	ret = cio_disable_subchannel(sch);
>   	if (ret == -EBUSY)
> -		vfio_ccw_sch_quiesce(sch);
> +		ret = vfio_ccw_sch_quiesce(sch);
>   	if (ret)
>   		goto err_unlock;
>