[RFC v2 0/3] fio-ccw fixes for kernel stacktraces

[RFC v2 0/3] fio-ccw fixes for kernel stacktraces

[Farhan Ali]

Hi,

I have recently seen some interesting kernel stacktraces while
testing vfio-ccw mediated devices. These stack traces are seen
when things don't go right, for example when a guest crashes or
dies while still doing some I/O on the mediated device.

I have also come across an issue in the common vfio code as
well which I reported upstream (https://marc.info/?l=kvm&m=155369393523503&w=2)
and submitted a patch separately for it
(https://marc.info/?l=kvm&m=155414703613868&w=2).

I would appreciate any review or feedback on these patches.

Thanks
Farhan

ChangeLog
---------
v1 -> v2
   - Use the return code from cio_cancel_halt_clear to prevent
     waiting unnecessarily (patch 2).

   - No need to reset the device on the remove path (patch 3).

Farhan Ali (3):
  vfio-ccw: Do not call flush_workqueue while holding the spinlock
  vfio-ccw: Prevent quiesce function going into an infinite loop
  vfio-ccw: Release any channel program when releasing/removing vfio-ccw
    mdev

 drivers/s390/cio/vfio_ccw_drv.c | 27 ++++++++++++---------------
 drivers/s390/cio/vfio_ccw_ops.c | 11 ++++++++++-
 2 files changed, 22 insertions(+), 16 deletions(-)

-- 
2.7.4

[RFC v2 1/3] vfio-ccw: Do not call flush_workqueue while holding the spinlock

[Farhan Ali]

Currently we call flush_workqueue while holding the subchannel
spinlock. But flush_workqueue function can go to sleep, so
do not call the function while holding the spinlock.

Fixes the following bug:

[  285.203430] BUG: scheduling while atomic: bash/14193/0x00000002
[  285.203434] INFO: lockdep is turned off.
....
[  285.203485] Preemption disabled at:
[  285.203488] [<000003ff80243e5c>] vfio_ccw_sch_quiesce+0xbc/0x120 [vfio_ccw]
[  285.203496] CPU: 7 PID: 14193 Comm: bash Tainted: G        W
....
[  285.203504] Call Trace:
[  285.203510] ([<0000000000113772>] show_stack+0x82/0xd0)
[  285.203514]  [<0000000000b7a102>] dump_stack+0x92/0xd0
[  285.203518]  [<000000000017b8be>] __schedule_bug+0xde/0xf8
[  285.203524]  [<0000000000b95b5a>] __schedule+0x7a/0xc38
[  285.203528]  [<0000000000b9678a>] schedule+0x72/0xb0
[  285.203533]  [<0000000000b9bfbc>] schedule_timeout+0x34/0x528
[  285.203538]  [<0000000000b97608>] wait_for_common+0x118/0x1b0
[  285.203544]  [<0000000000166d6a>] flush_workqueue+0x182/0x548
[  285.203550]  [<000003ff80243e6e>] vfio_ccw_sch_quiesce+0xce/0x120 [vfio_ccw]
[  285.203556]  [<000003ff80245278>] vfio_ccw_mdev_reset+0x38/0x70 [vfio_ccw]
[  285.203562]  [<000003ff802458b0>] vfio_ccw_mdev_remove+0x40/0x78 [vfio_ccw]
[  285.203567]  [<000003ff801a499c>] mdev_device_remove_ops+0x3c/0x80 [mdev]
[  285.203573]  [<000003ff801a4d5c>] mdev_device_remove+0xc4/0x130 [mdev]
[  285.203578]  [<000003ff801a5074>] remove_store+0x6c/0xa8 [mdev]
[  285.203582]  [<000000000046f494>] kernfs_fop_write+0x14c/0x1f8
[  285.203588]  [<00000000003c1530>] __vfs_write+0x38/0x1a8
[  285.203593]  [<00000000003c187c>] vfs_write+0xb4/0x198
[  285.203597]  [<00000000003c1af2>] ksys_write+0x5a/0xb0
[  285.203601]  [<0000000000b9e270>] system_call+0xdc/0x2d8

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 8f06125..5aca475 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -57,9 +57,9 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 
 			wait_for_completion_timeout(&completion, 3*HZ);
 
-			spin_lock_irq(sch->lock);
 			private->completion = NULL;
 			flush_workqueue(vfio_ccw_work_q);
+			spin_lock_irq(sch->lock);
 			ret = cio_cancel_halt_clear(sch, &iretry);
 		};
 
-- 
2.7.4

[RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop

[Farhan Ali]

The quiesce function calls cio_cancel_halt_clear() and if we
get an -EBUSY we go into a loop where we:
	- wait for any interrupts
	- flush all I/O in the workqueue
	- retry cio_cancel_halt_clear

During the period where we are waiting for interrupts or
flushing all I/O, the channel subsystem could have completed
a halt/clear action and turned off the corresponding activity
control bits in the subchannel status word. This means the next
time we call cio_cancel_halt_clear(), we will again start by
calling cancel subchannel and so we can be stuck between calling
cancel and halt forever.

Rather than calling cio_cancel_halt_clear() immediately after
waiting, let's try to disable the subchannel. If we succeed in
disabling the subchannel then we know nothing else can happen
with the device.

Suggested-by: Eric Farman <farman@linux.ibm.com>
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 5aca475..4405f2a 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -43,26 +43,23 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 	if (ret != -EBUSY)
 		goto out_unlock;
 
+	iretry = 255;
 	do {
-		iretry = 255;
 
 		ret = cio_cancel_halt_clear(sch, &iretry);
-		while (ret == -EBUSY) {
-			/*
-			 * Flush all I/O and wait for
-			 * cancel/halt/clear completion.
-			 */
-			private->completion = &completion;
-			spin_unlock_irq(sch->lock);
-
+		/*
+		 * Flush all I/O and wait for
+		 * cancel/halt/clear completion.
+		 */
+		private->completion = &completion;
+		spin_unlock_irq(sch->lock);
+
+		if (ret == -EBUSY)
 			wait_for_completion_timeout(&completion, 3*HZ);
 
-			private->completion = NULL;
-			flush_workqueue(vfio_ccw_work_q);
-			spin_lock_irq(sch->lock);
-			ret = cio_cancel_halt_clear(sch, &iretry);
-		};
-
+		private->completion = NULL;
+		flush_workqueue(vfio_ccw_work_q);
+		spin_lock_irq(sch->lock);
 		ret = cio_disable_subchannel(sch);
 	} while (ret == -EBUSY);
 out_unlock:
-- 
2.7.4

[RFC v2 3/3] vfio-ccw: Release any channel program when releasing/removing vfio-ccw mdev

[Farhan Ali]

When releasing the vfio-ccw mdev, we currently do not release
any existing channel program and it's pinned pages. This can
lead to the following warning:

[1038876.561565] WARNING: CPU: 2 PID: 144727 at drivers/vfio/vfio_iommu_type1.c:1494 vfio_sanity_check_pfn_list+0x40/0x70 [vfio_iommu_type1]

....

1038876.561921] Call Trace:
[1038876.561935] ([<00000009897fb870>] 0x9897fb870)
[1038876.561949]  [<000003ff8013bf62>] vfio_iommu_type1_detach_group+0xda/0x2f0 [vfio_iommu_type1]
[1038876.561965]  [<000003ff8007b634>] __vfio_group_unset_container+0x64/0x190 [vfio]
[1038876.561978]  [<000003ff8007b87e>] vfio_group_put_external_user+0x26/0x38 [vfio]
[1038876.562024]  [<000003ff806fc608>] kvm_vfio_group_put_external_user+0x40/0x60 [kvm]
[1038876.562045]  [<000003ff806fcb9e>] kvm_vfio_destroy+0x5e/0xd0 [kvm]
[1038876.562065]  [<000003ff806f63fc>] kvm_put_kvm+0x2a4/0x3d0 [kvm]
[1038876.562083]  [<000003ff806f655e>] kvm_vm_release+0x36/0x48 [kvm]
[1038876.562098]  [<00000000003c2dc4>] __fput+0x144/0x228
[1038876.562113]  [<000000000016ee82>] task_work_run+0x8a/0xd8
[1038876.562125]  [<000000000014c7a8>] do_exit+0x5d8/0xd90
[1038876.562140]  [<000000000014d084>] do_group_exit+0xc4/0xc8
[1038876.562155]  [<000000000015c046>] get_signal+0x9ae/0xa68
[1038876.562169]  [<0000000000108d66>] do_signal+0x66/0x768
[1038876.562185]  [<0000000000b9e37e>] system_call+0x1ea/0x2d8
[1038876.562195] 2 locks held by qemu-system-s39/144727:
[1038876.562205]  #0: 00000000537abaf9 (&container->group_lock){++++}, at: __vfio_group_unset_container+0x3c/0x190 [vfio]
[1038876.562230]  #1: 00000000670008b5 (&iommu->lock){+.+.}, at: vfio_iommu_type1_detach_group+0x36/0x2f0 [vfio_iommu_type1]
[1038876.562250] Last Breaking-Event-Address:
[1038876.562262]  [<000003ff8013aa24>] vfio_sanity_check_pfn_list+0x3c/0x70 [vfio_iommu_type1]
[1038876.562272] irq event stamp: 4236481
[1038876.562287] hardirqs last  enabled at (4236489): [<00000000001cee7a>] console_unlock+0x6d2/0x740
[1038876.562299] hardirqs last disabled at (4236496): [<00000000001ce87e>] console_unlock+0xd6/0x740
[1038876.562311] softirqs last  enabled at (4234162): [<0000000000b9fa1e>] __do_softirq+0x556/0x598
[1038876.562325] softirqs last disabled at (4234153): [<000000000014e4cc>] irq_exit+0xac/0x108
[1038876.562337] ---[ end trace 6c96d467b1c3ca06 ]---

Similarly we do not free the channel program when we are removing
the vfio-ccw device. Let's fix this by resetting the device and freeing
the channel program and pinned pages in the release path. For the remove
path we can just quiesce the device, since in the remove path the mediated
device is going away for good and so we don't need to do a full reset.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index ec2f796c..aacd528 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -133,11 +133,12 @@ static int vfio_ccw_mdev_remove(struct mdev_device *mdev)
 
 	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
 	    (private->state != VFIO_CCW_STATE_STANDBY)) {
-		if (!vfio_ccw_mdev_reset(mdev))
+		if (!vfio_ccw_sch_quiesce(private->sch))
 			private->state = VFIO_CCW_STATE_STANDBY;
 		/* The state will be NOT_OPER on error. */
 	}
 
+	cp_free(&private->cp);
 	private->mdev = NULL;
 	atomic_inc(&private->avail);
 
@@ -171,6 +172,14 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
 		dev_get_drvdata(mdev_parent_dev(mdev));
 	int i;
 
+	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
+	    (private->state != VFIO_CCW_STATE_STANDBY)) {
+		if (!vfio_ccw_mdev_reset(mdev))
+			private->state = VFIO_CCW_STATE_STANDBY;
+		/* The state will be NOT_OPER on error. */
+	}
+
+	cp_free(&private->cp);
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
 				 &private->nb);
 
-- 
2.7.4

Re: [RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop

[Cornelia Huck]

On Mon,  8 Apr 2019 17:05:32 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> The quiesce function calls cio_cancel_halt_clear() and if we
> get an -EBUSY we go into a loop where we:
> 	- wait for any interrupts
> 	- flush all I/O in the workqueue
> 	- retry cio_cancel_halt_clear
> 
> During the period where we are waiting for interrupts or
> flushing all I/O, the channel subsystem could have completed
> a halt/clear action and turned off the corresponding activity
> control bits in the subchannel status word. This means the next
> time we call cio_cancel_halt_clear(), we will again start by
> calling cancel subchannel and so we can be stuck between calling
> cancel and halt forever.
> 
> Rather than calling cio_cancel_halt_clear() immediately after
> waiting, let's try to disable the subchannel. If we succeed in
> disabling the subchannel then we know nothing else can happen
> with the device.
> 
> Suggested-by: Eric Farman <farman@linux.ibm.com>
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 5aca475..4405f2a 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -43,26 +43,23 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
>  	if (ret != -EBUSY)
>  		goto out_unlock;
>  
> +	iretry = 255;
>  	do {
> -		iretry = 255;
>  
>  		ret = cio_cancel_halt_clear(sch, &iretry);
> -		while (ret == -EBUSY) {
> -			/*
> -			 * Flush all I/O and wait for
> -			 * cancel/halt/clear completion.
> -			 */
> -			private->completion = &completion;
> -			spin_unlock_irq(sch->lock);
> -
> +		/*
> +		 * Flush all I/O and wait for
> +		 * cancel/halt/clear completion.
> +		 */
> +		private->completion = &completion;
> +		spin_unlock_irq(sch->lock);
> +
> +		if (ret == -EBUSY)

I don't think you need to do the unlock/lock and change
private->completion if you don't actually wait, no?

Looking at the possible return codes:
* -ENODEV -> device is not operational anyway, in theory you should even
   not need to bother with disabling the subchannel
* -EIO -> we've run out of retries, and the subchannel still is not
  idle; I'm not sure if we could do anything here, as disable is
  unlikely to work, either
* -EBUSY -> we expect an interrupt (or a timeout), the loop looks fine
  for that
* 0 -> the one thing that might happen is that we get an unsolicited
  interrupt between the successful cancel_halt_clear and the disable;
  not even giving up the lock here might even be better here?

I think this loop will probably work as it is after this patch, but
giving up the lock when not really needed makes me a bit queasy... what
do others think?

>  			wait_for_completion_timeout(&completion, 3*HZ);
>  
> -			private->completion = NULL;
> -			flush_workqueue(vfio_ccw_work_q);
> -			spin_lock_irq(sch->lock);
> -			ret = cio_cancel_halt_clear(sch, &iretry);
> -		};
> -
> +		private->completion = NULL;
> +		flush_workqueue(vfio_ccw_work_q);
> +		spin_lock_irq(sch->lock);
>  		ret = cio_disable_subchannel(sch);
>  	} while (ret == -EBUSY);
>  out_unlock:

Re: [RFC v2 3/3] vfio-ccw: Release any channel program when releasing/removing vfio-ccw mdev

[Cornelia Huck]

On Mon,  8 Apr 2019 17:05:33 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> When releasing the vfio-ccw mdev, we currently do not release
> any existing channel program and it's pinned pages. This can

s/it's/its/

> lead to the following warning:
> 
> [1038876.561565] WARNING: CPU: 2 PID: 144727 at drivers/vfio/vfio_iommu_type1.c:1494 vfio_sanity_check_pfn_list+0x40/0x70 [vfio_iommu_type1]
> 
> ....
> 
> 1038876.561921] Call Trace:
> [1038876.561935] ([<00000009897fb870>] 0x9897fb870)
> [1038876.561949]  [<000003ff8013bf62>] vfio_iommu_type1_detach_group+0xda/0x2f0 [vfio_iommu_type1]
> [1038876.561965]  [<000003ff8007b634>] __vfio_group_unset_container+0x64/0x190 [vfio]
> [1038876.561978]  [<000003ff8007b87e>] vfio_group_put_external_user+0x26/0x38 [vfio]
> [1038876.562024]  [<000003ff806fc608>] kvm_vfio_group_put_external_user+0x40/0x60 [kvm]
> [1038876.562045]  [<000003ff806fcb9e>] kvm_vfio_destroy+0x5e/0xd0 [kvm]
> [1038876.562065]  [<000003ff806f63fc>] kvm_put_kvm+0x2a4/0x3d0 [kvm]
> [1038876.562083]  [<000003ff806f655e>] kvm_vm_release+0x36/0x48 [kvm]
> [1038876.562098]  [<00000000003c2dc4>] __fput+0x144/0x228
> [1038876.562113]  [<000000000016ee82>] task_work_run+0x8a/0xd8
> [1038876.562125]  [<000000000014c7a8>] do_exit+0x5d8/0xd90
> [1038876.562140]  [<000000000014d084>] do_group_exit+0xc4/0xc8
> [1038876.562155]  [<000000000015c046>] get_signal+0x9ae/0xa68
> [1038876.562169]  [<0000000000108d66>] do_signal+0x66/0x768
> [1038876.562185]  [<0000000000b9e37e>] system_call+0x1ea/0x2d8
> [1038876.562195] 2 locks held by qemu-system-s39/144727:
> [1038876.562205]  #0: 00000000537abaf9 (&container->group_lock){++++}, at: __vfio_group_unset_container+0x3c/0x190 [vfio]
> [1038876.562230]  #1: 00000000670008b5 (&iommu->lock){+.+.}, at: vfio_iommu_type1_detach_group+0x36/0x2f0 [vfio_iommu_type1]
> [1038876.562250] Last Breaking-Event-Address:
> [1038876.562262]  [<000003ff8013aa24>] vfio_sanity_check_pfn_list+0x3c/0x70 [vfio_iommu_type1]
> [1038876.562272] irq event stamp: 4236481
> [1038876.562287] hardirqs last  enabled at (4236489): [<00000000001cee7a>] console_unlock+0x6d2/0x740
> [1038876.562299] hardirqs last disabled at (4236496): [<00000000001ce87e>] console_unlock+0xd6/0x740
> [1038876.562311] softirqs last  enabled at (4234162): [<0000000000b9fa1e>] __do_softirq+0x556/0x598
> [1038876.562325] softirqs last disabled at (4234153): [<000000000014e4cc>] irq_exit+0xac/0x108
> [1038876.562337] ---[ end trace 6c96d467b1c3ca06 ]---
> 
> Similarly we do not free the channel program when we are removing
> the vfio-ccw device. Let's fix this by resetting the device and freeing
> the channel program and pinned pages in the release path. For the remove
> path we can just quiesce the device, since in the remove path the mediated
> device is going away for good and so we don't need to do a full reset.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_ops.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index ec2f796c..aacd528 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -133,11 +133,12 @@ static int vfio_ccw_mdev_remove(struct mdev_device *mdev)
>  
>  	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
>  	    (private->state != VFIO_CCW_STATE_STANDBY)) {
> -		if (!vfio_ccw_mdev_reset(mdev))
> +		if (!vfio_ccw_sch_quiesce(private->sch))
>  			private->state = VFIO_CCW_STATE_STANDBY;
>  		/* The state will be NOT_OPER on error. */
>  	}
>  
> +	cp_free(&private->cp);
>  	private->mdev = NULL;
>  	atomic_inc(&private->avail);
>  
> @@ -171,6 +172,14 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
>  		dev_get_drvdata(mdev_parent_dev(mdev));
>  	int i;
>  
> +	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
> +	    (private->state != VFIO_CCW_STATE_STANDBY)) {
> +		if (!vfio_ccw_mdev_reset(mdev))
> +			private->state = VFIO_CCW_STATE_STANDBY;
> +		/* The state will be NOT_OPER on error. */
> +	}
> +
> +	cp_free(&private->cp);
>  	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>  				 &private->nb);
>  

Looks good to me, would love a review/ack.

Re: [RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop

[Farhan Ali]

On 04/11/2019 12:24 PM, Cornelia Huck wrote:
> On Mon,  8 Apr 2019 17:05:32 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> The quiesce function calls cio_cancel_halt_clear() and if we
>> get an -EBUSY we go into a loop where we:
>> 	- wait for any interrupts
>> 	- flush all I/O in the workqueue
>> 	- retry cio_cancel_halt_clear
>>
>> During the period where we are waiting for interrupts or
>> flushing all I/O, the channel subsystem could have completed
>> a halt/clear action and turned off the corresponding activity
>> control bits in the subchannel status word. This means the next
>> time we call cio_cancel_halt_clear(), we will again start by
>> calling cancel subchannel and so we can be stuck between calling
>> cancel and halt forever.
>>
>> Rather than calling cio_cancel_halt_clear() immediately after
>> waiting, let's try to disable the subchannel. If we succeed in
>> disabling the subchannel then we know nothing else can happen
>> with the device.
>>
>> Suggested-by: Eric Farman <farman@linux.ibm.com>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_drv.c | 27 ++++++++++++---------------
>>   1 file changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>> index 5aca475..4405f2a 100644
>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>> @@ -43,26 +43,23 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
>>   	if (ret != -EBUSY)
>>   		goto out_unlock;
>>   
>> +	iretry = 255;
>>   	do {
>> -		iretry = 255;
>>   
>>   		ret = cio_cancel_halt_clear(sch, &iretry);
>> -		while (ret == -EBUSY) {
>> -			/*
>> -			 * Flush all I/O and wait for
>> -			 * cancel/halt/clear completion.
>> -			 */
>> -			private->completion = &completion;
>> -			spin_unlock_irq(sch->lock);
>> -
>> +		/*
>> +		 * Flush all I/O and wait for
>> +		 * cancel/halt/clear completion.
>> +		 */
>> +		private->completion = &completion;
>> +		spin_unlock_irq(sch->lock);
>> +
>> +		if (ret == -EBUSY)
> 
> I don't think you need to do the unlock/lock and change
> private->completion if you don't actually wait, no?

If we don't end up waiting, then changing private->completion would not 
be needed. But we would still need to release the spinlock due to [1].

> 
> Looking at the possible return codes:
> * -ENODEV -> device is not operational anyway, in theory you should even
>     not need to bother with disabling the subchannel
> * -EIO -> we've run out of retries, and the subchannel still is not
>    idle; I'm not sure if we could do anything here, as disable is
>    unlikely to work, either

We could break out of the loop early for these cases. My thinking was I 
wanted to depend on the result of trying to disable, because ultimately 
that's what we want.

I can add the cases to break out of the loop early.


> * -EBUSY -> we expect an interrupt (or a timeout), the loop looks fine
>    for that
> * 0 -> the one thing that might happen is that we get an unsolicited
>    interrupt between the successful cancel_halt_clear and the disable;
>    not even giving up the lock here might even be better here?

I didn't think of this case, but if cancel_halt_clear succeeds with 0 
then we should wait, no?

> 
> I think this loop will probably work as it is after this patch, but
> giving up the lock when not really needed makes me a bit queasy... what
> do others think?
> 
>>   			wait_for_completion_timeout(&completion, 3*HZ);
>>   
>> -			private->completion = NULL;
>> -			flush_workqueue(vfio_ccw_work_q);
>> -			spin_lock_irq(sch->lock);
>> -			ret = cio_cancel_halt_clear(sch, &iretry);
>> -		};
>> -
>> +		private->completion = NULL;

[1]  flush_workqueue can go to sleep so we would still need to release 
spinlock and reacquire it again to try disabling the subchannel.

>> +		flush_workqueue(vfio_ccw_work_q);
>> +		spin_lock_irq(sch->lock);
>>   		ret = cio_disable_subchannel(sch);
>>   	} while (ret == -EBUSY);
>>   out_unlock:
> 
> 

Re: [RFC v2 3/3] vfio-ccw: Release any channel program when releasing/removing vfio-ccw mdev

[Farhan Ali]

On 04/11/2019 12:27 PM, Cornelia Huck wrote:
> On Mon,  8 Apr 2019 17:05:33 -0400
> Farhan Ali<alifm@linux.ibm.com>  wrote:
> 
>> When releasing the vfio-ccw mdev, we currently do not release
>> any existing channel program and it's pinned pages. This can
> s/it's/its/
> 

Will fix.

Thanks for taking a look at the patches.

Thanks
Farhan

Re: [RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop

[Cornelia Huck]

On Thu, 11 Apr 2019 16:30:44 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 04/11/2019 12:24 PM, Cornelia Huck wrote:
> > On Mon,  8 Apr 2019 17:05:32 -0400
> > Farhan Ali <alifm@linux.ibm.com> wrote:
> >   
> >> The quiesce function calls cio_cancel_halt_clear() and if we
> >> get an -EBUSY we go into a loop where we:
> >> 	- wait for any interrupts
> >> 	- flush all I/O in the workqueue
> >> 	- retry cio_cancel_halt_clear
> >>
> >> During the period where we are waiting for interrupts or
> >> flushing all I/O, the channel subsystem could have completed
> >> a halt/clear action and turned off the corresponding activity
> >> control bits in the subchannel status word. This means the next
> >> time we call cio_cancel_halt_clear(), we will again start by
> >> calling cancel subchannel and so we can be stuck between calling
> >> cancel and halt forever.
> >>
> >> Rather than calling cio_cancel_halt_clear() immediately after
> >> waiting, let's try to disable the subchannel. If we succeed in
> >> disabling the subchannel then we know nothing else can happen
> >> with the device.
> >>
> >> Suggested-by: Eric Farman <farman@linux.ibm.com>
> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >> ---
> >>   drivers/s390/cio/vfio_ccw_drv.c | 27 ++++++++++++---------------
> >>   1 file changed, 12 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> >> index 5aca475..4405f2a 100644
> >> --- a/drivers/s390/cio/vfio_ccw_drv.c
> >> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> >> @@ -43,26 +43,23 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
> >>   	if (ret != -EBUSY)
> >>   		goto out_unlock;
> >>   
> >> +	iretry = 255;
> >>   	do {
> >> -		iretry = 255;
> >>   
> >>   		ret = cio_cancel_halt_clear(sch, &iretry);
> >> -		while (ret == -EBUSY) {
> >> -			/*
> >> -			 * Flush all I/O and wait for
> >> -			 * cancel/halt/clear completion.
> >> -			 */
> >> -			private->completion = &completion;
> >> -			spin_unlock_irq(sch->lock);
> >> -
> >> +		/*
> >> +		 * Flush all I/O and wait for
> >> +		 * cancel/halt/clear completion.
> >> +		 */
> >> +		private->completion = &completion;
> >> +		spin_unlock_irq(sch->lock);
> >> +
> >> +		if (ret == -EBUSY)  
> > 
> > I don't think you need to do the unlock/lock and change
> > private->completion if you don't actually wait, no?  
> 
> If we don't end up waiting, then changing private->completion would not 
> be needed. But we would still need to release the spinlock due to [1].
> 
> > 
> > Looking at the possible return codes:
> > * -ENODEV -> device is not operational anyway, in theory you should even
> >     not need to bother with disabling the subchannel
> > * -EIO -> we've run out of retries, and the subchannel still is not
> >    idle; I'm not sure if we could do anything here, as disable is
> >    unlikely to work, either  
> 
> We could break out of the loop early for these cases. My thinking was I 
> wanted to depend on the result of trying to disable, because ultimately 
> that's what we want.
> 
> I can add the cases to break out of the loop early.

The -ENODEV case does not really hurt, as it will get us out of the
loop anyway. But for the -EIO case, I think we'll get -EBUSY from the
disable and stay within the loop endlessly?

> 
> 
> > * -EBUSY -> we expect an interrupt (or a timeout), the loop looks fine
> >    for that
> > * 0 -> the one thing that might happen is that we get an unsolicited
> >    interrupt between the successful cancel_halt_clear and the disable;
> >    not even giving up the lock here might even be better here?  
> 
> I didn't think of this case, but if cancel_halt_clear succeeds with 0 
> then we should wait, no?

For 0 I don't expect a solicited interrupt (documentation for the
functions says that the subchannel is idle in that case); it's just the
unsolicited interrupt that might get into the way.

> 
> > 
> > I think this loop will probably work as it is after this patch, but
> > giving up the lock when not really needed makes me a bit queasy... what
> > do others think?
> >   
> >>   			wait_for_completion_timeout(&completion, 3*HZ);
> >>   
> >> -			private->completion = NULL;
> >> -			flush_workqueue(vfio_ccw_work_q);
> >> -			spin_lock_irq(sch->lock);
> >> -			ret = cio_cancel_halt_clear(sch, &iretry);
> >> -		};
> >> -
> >> +		private->completion = NULL;  
> 
> [1]  flush_workqueue can go to sleep so we would still need to release 
> spinlock and reacquire it again to try disabling the subchannel.

Grr, I thought we could skip the flush in the !-EBUSY case, but I think
we can't due to the possibility of an unsolicited interrupt... what
simply adding handling for -EIO (although I'm not sure what we can
sensibly do in that case) and leave the other cases as they are now?

> 
> >> +		flush_workqueue(vfio_ccw_work_q);
> >> +		spin_lock_irq(sch->lock);
> >>   		ret = cio_disable_subchannel(sch);
> >>   	} while (ret == -EBUSY);
> >>   out_unlock:  
> > 
> >   
> 

Re: [RFC v2 3/3] vfio-ccw: Release any channel program when releasing/removing vfio-ccw mdev

[Cornelia Huck]

On Thu, 11 Apr 2019 16:39:32 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 04/11/2019 12:27 PM, Cornelia Huck wrote:
> > On Mon,  8 Apr 2019 17:05:33 -0400
> > Farhan Ali<alifm@linux.ibm.com>  wrote:
> >   
> >> When releasing the vfio-ccw mdev, we currently do not release
> >> any existing channel program and it's pinned pages. This can  
> > s/it's/its/
> >   
> 
> Will fix.
> 
> Thanks for taking a look at the patches.

NP, I really want to queue them soon :)

Just give me a quick hint about dependencies: Do I need anything else
before I can queue patch 1 and 3? I've lost track a bit...

Re: [RFC v2 3/3] vfio-ccw: Release any channel program when releasing/removing vfio-ccw mdev

[Farhan Ali]

On 04/12/2019 04:12 AM, Cornelia Huck wrote:
> On Thu, 11 Apr 2019 16:39:32 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> On 04/11/2019 12:27 PM, Cornelia Huck wrote:
>>> On Mon,  8 Apr 2019 17:05:33 -0400
>>> Farhan Ali<alifm@linux.ibm.com>  wrote:
>>>    
>>>> When releasing the vfio-ccw mdev, we currently do not release
>>>> any existing channel program and it's pinned pages. This can
>>> s/it's/its/
>>>    
>>
>> Will fix.
>>
>> Thanks for taking a look at the patches.
> 
> NP, I really want to queue them soon :)
> 
> Just give me a quick hint about dependencies: Do I need anything else
> before I can queue patch 1 and 3? I've lost track a bit...
> 
> 

Patch 1 can be queued without any problems and it has no dependencies.

Patch 3 would depend on your [PATCH v4 1/6] vfio-ccw: make it safe to 
access channel programs. Since I want to call cp_free without any problems.

I know you are waiting for acks/r-bs for patch 3, so if no one has 
problems then we can go ahead and queue it.


Thanks
Farhan

Re: [RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop

[Farhan Ali]

On 04/12/2019 04:10 AM, Cornelia Huck wrote:
> On Thu, 11 Apr 2019 16:30:44 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> On 04/11/2019 12:24 PM, Cornelia Huck wrote:
>>> On Mon,  8 Apr 2019 17:05:32 -0400
>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>    
>>>> The quiesce function calls cio_cancel_halt_clear() and if we
>>>> get an -EBUSY we go into a loop where we:
>>>> 	- wait for any interrupts
>>>> 	- flush all I/O in the workqueue
>>>> 	- retry cio_cancel_halt_clear
>>>>
>>>> During the period where we are waiting for interrupts or
>>>> flushing all I/O, the channel subsystem could have completed
>>>> a halt/clear action and turned off the corresponding activity
>>>> control bits in the subchannel status word. This means the next
>>>> time we call cio_cancel_halt_clear(), we will again start by
>>>> calling cancel subchannel and so we can be stuck between calling
>>>> cancel and halt forever.
>>>>
>>>> Rather than calling cio_cancel_halt_clear() immediately after
>>>> waiting, let's try to disable the subchannel. If we succeed in
>>>> disabling the subchannel then we know nothing else can happen
>>>> with the device.
>>>>
>>>> Suggested-by: Eric Farman <farman@linux.ibm.com>
>>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>>> ---
>>>>    drivers/s390/cio/vfio_ccw_drv.c | 27 ++++++++++++---------------
>>>>    1 file changed, 12 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>>>> index 5aca475..4405f2a 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>>>> @@ -43,26 +43,23 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
>>>>    	if (ret != -EBUSY)
>>>>    		goto out_unlock;
>>>>    
>>>> +	iretry = 255;
>>>>    	do {
>>>> -		iretry = 255;
>>>>    
>>>>    		ret = cio_cancel_halt_clear(sch, &iretry);
>>>> -		while (ret == -EBUSY) {
>>>> -			/*
>>>> -			 * Flush all I/O and wait for
>>>> -			 * cancel/halt/clear completion.
>>>> -			 */
>>>> -			private->completion = &completion;
>>>> -			spin_unlock_irq(sch->lock);
>>>> -
>>>> +		/*
>>>> +		 * Flush all I/O and wait for
>>>> +		 * cancel/halt/clear completion.
>>>> +		 */
>>>> +		private->completion = &completion;
>>>> +		spin_unlock_irq(sch->lock);
>>>> +
>>>> +		if (ret == -EBUSY)
>>>
>>> I don't think you need to do the unlock/lock and change
>>> private->completion if you don't actually wait, no?
>>
>> If we don't end up waiting, then changing private->completion would not
>> be needed. But we would still need to release the spinlock due to [1].
>>
>>>
>>> Looking at the possible return codes:
>>> * -ENODEV -> device is not operational anyway, in theory you should even
>>>      not need to bother with disabling the subchannel
>>> * -EIO -> we've run out of retries, and the subchannel still is not
>>>     idle; I'm not sure if we could do anything here, as disable is
>>>     unlikely to work, either
>>
>> We could break out of the loop early for these cases. My thinking was I
>> wanted to depend on the result of trying to disable, because ultimately
>> that's what we want.
>>
>> I can add the cases to break out of the loop early.
> 
> The -ENODEV case does not really hurt, as it will get us out of the
> loop anyway. But for the -EIO case, I think we'll get -EBUSY from the
> disable and stay within the loop endlessly?
> 
>>
>>
>>> * -EBUSY -> we expect an interrupt (or a timeout), the loop looks fine
>>>     for that
>>> * 0 -> the one thing that might happen is that we get an unsolicited
>>>     interrupt between the successful cancel_halt_clear and the disable;
>>>     not even giving up the lock here might even be better here?
>>
>> I didn't think of this case, but if cancel_halt_clear succeeds with 0
>> then we should wait, no?
> 
> For 0 I don't expect a solicited interrupt (documentation for the
> functions says that the subchannel is idle in that case); it's just the
> unsolicited interrupt that might get into the way.
> 
>>
>>>
>>> I think this loop will probably work as it is after this patch, but
>>> giving up the lock when not really needed makes me a bit queasy... what
>>> do others think?
>>>    
>>>>    			wait_for_completion_timeout(&completion, 3*HZ);
>>>>    
>>>> -			private->completion = NULL;
>>>> -			flush_workqueue(vfio_ccw_work_q);
>>>> -			spin_lock_irq(sch->lock);
>>>> -			ret = cio_cancel_halt_clear(sch, &iretry);
>>>> -		};
>>>> -
>>>> +		private->completion = NULL;
>>
>> [1]  flush_workqueue can go to sleep so we would still need to release
>> spinlock and reacquire it again to try disabling the subchannel.
> 
> Grr, I thought we could skip the flush in the !-EBUSY case, but I think
> we can't due to the possibility of an unsolicited interrupt... what
> simply adding handling for -EIO (although I'm not sure what we can
> sensibly do in that case) and leave the other cases as they are now?
> 

Thinking a little bit more about EIO, if the return code is EIO then it 
means we have exhausted all our options with cancel_halt_clear and the 
subchannel/device is still status pending, right?

I think we should still continue to try and disable the subchannel, 
because if not then the subchannel/device could in some point of time 
come back and bite us. So we really should protect the system from this 
behavior.

I think for EIO we should log an error message, but still try to 
continue with disabling the subchannel. What do you or others think?




>>
>>>> +		flush_workqueue(vfio_ccw_work_q);
>>>> +		spin_lock_irq(sch->lock);
>>>>    		ret = cio_disable_subchannel(sch);
>>>>    	} while (ret == -EBUSY);
>>>>    out_unlock:
>>>
>>>    
>>
> 
> 

Re: [RFC v2 3/3] vfio-ccw: Release any channel program when releasing/removing vfio-ccw mdev

[Eric Farman]

On 4/8/19 5:05 PM, Farhan Ali wrote:
> When releasing the vfio-ccw mdev, we currently do not release
> any existing channel program and it's pinned pages. This can
> lead to the following warning:
> 
> [1038876.561565] WARNING: CPU: 2 PID: 144727 at drivers/vfio/vfio_iommu_type1.c:1494 vfio_sanity_check_pfn_list+0x40/0x70 [vfio_iommu_type1]
> 
> ....
> 
> 1038876.561921] Call Trace:
> [1038876.561935] ([<00000009897fb870>] 0x9897fb870)
> [1038876.561949]  [<000003ff8013bf62>] vfio_iommu_type1_detach_group+0xda/0x2f0 [vfio_iommu_type1]
> [1038876.561965]  [<000003ff8007b634>] __vfio_group_unset_container+0x64/0x190 [vfio]
> [1038876.561978]  [<000003ff8007b87e>] vfio_group_put_external_user+0x26/0x38 [vfio]
> [1038876.562024]  [<000003ff806fc608>] kvm_vfio_group_put_external_user+0x40/0x60 [kvm]
> [1038876.562045]  [<000003ff806fcb9e>] kvm_vfio_destroy+0x5e/0xd0 [kvm]
> [1038876.562065]  [<000003ff806f63fc>] kvm_put_kvm+0x2a4/0x3d0 [kvm]
> [1038876.562083]  [<000003ff806f655e>] kvm_vm_release+0x36/0x48 [kvm]
> [1038876.562098]  [<00000000003c2dc4>] __fput+0x144/0x228
> [1038876.562113]  [<000000000016ee82>] task_work_run+0x8a/0xd8
> [1038876.562125]  [<000000000014c7a8>] do_exit+0x5d8/0xd90
> [1038876.562140]  [<000000000014d084>] do_group_exit+0xc4/0xc8
> [1038876.562155]  [<000000000015c046>] get_signal+0x9ae/0xa68
> [1038876.562169]  [<0000000000108d66>] do_signal+0x66/0x768
> [1038876.562185]  [<0000000000b9e37e>] system_call+0x1ea/0x2d8
> [1038876.562195] 2 locks held by qemu-system-s39/144727:
> [1038876.562205]  #0: 00000000537abaf9 (&container->group_lock){++++}, at: __vfio_group_unset_container+0x3c/0x190 [vfio]
> [1038876.562230]  #1: 00000000670008b5 (&iommu->lock){+.+.}, at: vfio_iommu_type1_detach_group+0x36/0x2f0 [vfio_iommu_type1]
> [1038876.562250] Last Breaking-Event-Address:
> [1038876.562262]  [<000003ff8013aa24>] vfio_sanity_check_pfn_list+0x3c/0x70 [vfio_iommu_type1]
> [1038876.562272] irq event stamp: 4236481
> [1038876.562287] hardirqs last  enabled at (4236489): [<00000000001cee7a>] console_unlock+0x6d2/0x740
> [1038876.562299] hardirqs last disabled at (4236496): [<00000000001ce87e>] console_unlock+0xd6/0x740
> [1038876.562311] softirqs last  enabled at (4234162): [<0000000000b9fa1e>] __do_softirq+0x556/0x598
> [1038876.562325] softirqs last disabled at (4234153): [<000000000014e4cc>] irq_exit+0xac/0x108
> [1038876.562337] ---[ end trace 6c96d467b1c3ca06 ]---
> 
> Similarly we do not free the channel program when we are removing
> the vfio-ccw device. Let's fix this by resetting the device and freeing
> the channel program and pinned pages in the release path. For the remove
> path we can just quiesce the device, since in the remove path the mediated
> device is going away for good and so we don't need to do a full reset.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_ops.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index ec2f796c..aacd528 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -133,11 +133,12 @@ static int vfio_ccw_mdev_remove(struct mdev_device *mdev)
>   
>   	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
>   	    (private->state != VFIO_CCW_STATE_STANDBY)) {
> -		if (!vfio_ccw_mdev_reset(mdev))
> +		if (!vfio_ccw_sch_quiesce(private->sch))
>   			private->state = VFIO_CCW_STATE_STANDBY;
>   		/* The state will be NOT_OPER on error. */
>   	}
>   
> +	cp_free(&private->cp);
>   	private->mdev = NULL;
>   	atomic_inc(&private->avail);
>   
> @@ -171,6 +172,14 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
>   		dev_get_drvdata(mdev_parent_dev(mdev));
>   	int i;
>   
> +	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
> +	    (private->state != VFIO_CCW_STATE_STANDBY)) {
> +		if (!vfio_ccw_mdev_reset(mdev))
> +			private->state = VFIO_CCW_STATE_STANDBY;
> +		/* The state will be NOT_OPER on error. */
> +	}
> +
> +	cp_free(&private->cp);
>   	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>   				 &private->nb);
>   
> 

This seems sane to me.

Acked-by: Eric Farman <farman@linux.ibm.com>

Re: [RFC v2 3/3] vfio-ccw: Release any channel program when releasing/removing vfio-ccw mdev

[Eric Farman]

On 4/12/19 10:13 AM, Farhan Ali wrote:
> 
> 
> On 04/12/2019 04:12 AM, Cornelia Huck wrote:
>> On Thu, 11 Apr 2019 16:39:32 -0400
>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>
>>> On 04/11/2019 12:27 PM, Cornelia Huck wrote:
>>>> On Mon,  8 Apr 2019 17:05:33 -0400
>>>> Farhan Ali<alifm@linux.ibm.com>  wrote:
>>>>> When releasing the vfio-ccw mdev, we currently do not release
>>>>> any existing channel program and it's pinned pages. This can
>>>> s/it's/its/
>>>
>>> Will fix.
>>>
>>> Thanks for taking a look at the patches.
>>
>> NP, I really want to queue them soon :)
>>
>> Just give me a quick hint about dependencies: Do I need anything else
>> before I can queue patch 1 and 3? I've lost track a bit...
>>
>>
> 
> Patch 1 can be queued without any problems and it has no dependencies.
> 
> Patch 3 would depend on your [PATCH v4 1/6] vfio-ccw: make it safe to 
> access channel programs. Since I want to call cp_free without any problems.

I'm sorry; I'm working as a LIFO queue this afternoon.  At least I 
reviewed patch 1 of that series.  :)

> 
> I know you are waiting for acks/r-bs for patch 3, so if no one has 
> problems then we can go ahead and queue it.
> 
> 
> Thanks
> Farhan
> 

Re: [RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop

[Cornelia Huck]

On Fri, 12 Apr 2019 10:38:50 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 04/12/2019 04:10 AM, Cornelia Huck wrote:
> > On Thu, 11 Apr 2019 16:30:44 -0400
> > Farhan Ali <alifm@linux.ibm.com> wrote:
> >   
> >> On 04/11/2019 12:24 PM, Cornelia Huck wrote:  
> >>> On Mon,  8 Apr 2019 17:05:32 -0400
> >>> Farhan Ali <alifm@linux.ibm.com> wrote:

> >>> Looking at the possible return codes:
> >>> * -ENODEV -> device is not operational anyway, in theory you should even
> >>>      not need to bother with disabling the subchannel
> >>> * -EIO -> we've run out of retries, and the subchannel still is not
> >>>     idle; I'm not sure if we could do anything here, as disable is
> >>>     unlikely to work, either  

(...)

> Thinking a little bit more about EIO, if the return code is EIO then it 
> means we have exhausted all our options with cancel_halt_clear and the 
> subchannel/device is still status pending, right?

Yes.

> 
> I think we should still continue to try and disable the subchannel, 
> because if not then the subchannel/device could in some point of time 
> come back and bite us. So we really should protect the system from this 
> behavior.

I think trying to disable the subchannel does not really hurt, but I
fear it won't succeed in that case...

> 
> I think for EIO we should log an error message, but still try to 
> continue with disabling the subchannel. What do you or others think?

Logging an error may be useful (it's really fouled up at that time), but...

> 
> 
> 
> 
> >>  
> >>>> +		flush_workqueue(vfio_ccw_work_q);
> >>>> +		spin_lock_irq(sch->lock);
> >>>>    		ret = cio_disable_subchannel(sch);

...there's a good chance that we'd get -EBUSY here, which would keep us
in the loop. We probably need to break out after we got -EIO from
cancel_halt_clear, regardless of which return code we get from the
disable.

(It will be "interesting" to see what happens with such a stuck
subchannel in the calling code; but I don't really see many options.
Panic seems way too strong; maybe mark the subchannel as "broken; no
idea how to fix"? But that would be a follow-on patch; I think if we
avoid the endless loop here, this patch is a real improvement and
should just go in.)

> >>>>    	} while (ret == -EBUSY);
> >>>>    out_unlock:  
> >>>
> >>>      
> >>  
> > 
> >   
> 

Re: [RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop

[Farhan Ali]

On 04/15/2019 04:13 AM, Cornelia Huck wrote:
> On Fri, 12 Apr 2019 10:38:50 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> On 04/12/2019 04:10 AM, Cornelia Huck wrote:
>>> On Thu, 11 Apr 2019 16:30:44 -0400
>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>    
>>>> On 04/11/2019 12:24 PM, Cornelia Huck wrote:
>>>>> On Mon,  8 Apr 2019 17:05:32 -0400
>>>>> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>>>>> Looking at the possible return codes:
>>>>> * -ENODEV -> device is not operational anyway, in theory you should even
>>>>>       not need to bother with disabling the subchannel
>>>>> * -EIO -> we've run out of retries, and the subchannel still is not
>>>>>      idle; I'm not sure if we could do anything here, as disable is
>>>>>      unlikely to work, either
> 
> (...)
> 
>> Thinking a little bit more about EIO, if the return code is EIO then it
>> means we have exhausted all our options with cancel_halt_clear and the
>> subchannel/device is still status pending, right?
> 
> Yes.
> 
>>
>> I think we should still continue to try and disable the subchannel,
>> because if not then the subchannel/device could in some point of time
>> come back and bite us. So we really should protect the system from this
>> behavior.
> 
> I think trying to disable the subchannel does not really hurt, but I
> fear it won't succeed in that case...
> 
>>
>> I think for EIO we should log an error message, but still try to
>> continue with disabling the subchannel. What do you or others think?
> 
> Logging an error may be useful (it's really fouled up at that time), but...
> 
>>
>>
>>
>>
>>>>   
>>>>>> +		flush_workqueue(vfio_ccw_work_q);
>>>>>> +		spin_lock_irq(sch->lock);
>>>>>>     		ret = cio_disable_subchannel(sch);
> 
> ...there's a good chance that we'd get -EBUSY here, which would keep us
> in the loop. We probably need to break out after we got -EIO from
> cancel_halt_clear, regardless of which return code we get from the
> disable.

Okay, for EIO we can log an error message and break out of the loop.

I will send a v3. Are you going to queue patch 1 or patch 3 soon? If you 
are then I will just send this patch separately.

Thanks
Farhan

> 
> (It will be "interesting" to see what happens with such a stuck
> subchannel in the calling code; but I don't really see many options.
> Panic seems way too strong; maybe mark the subchannel as "broken; no
> idea how to fix"? But that would be a follow-on patch; I think if we
> avoid the endless loop here, this patch is a real improvement and
> should just go in.)
> 
>>>>>>     	} while (ret == -EBUSY);
>>>>>>     out_unlock:
>>>>>
>>>>>       
>>>>   
>>>
>>>    
>>
> 
> 

Re: [RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop

[Cornelia Huck]

On Mon, 15 Apr 2019 09:38:37 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 04/15/2019 04:13 AM, Cornelia Huck wrote:
> > On Fri, 12 Apr 2019 10:38:50 -0400
> > Farhan Ali <alifm@linux.ibm.com> wrote:
> >   
> >> On 04/12/2019 04:10 AM, Cornelia Huck wrote:  
> >>> On Thu, 11 Apr 2019 16:30:44 -0400
> >>> Farhan Ali <alifm@linux.ibm.com> wrote:
> >>>      
> >>>> On 04/11/2019 12:24 PM, Cornelia Huck wrote:  
> >>>>> On Mon,  8 Apr 2019 17:05:32 -0400
> >>>>> Farhan Ali <alifm@linux.ibm.com> wrote:  
> >   
> >>>>> Looking at the possible return codes:
> >>>>> * -ENODEV -> device is not operational anyway, in theory you should even
> >>>>>       not need to bother with disabling the subchannel
> >>>>> * -EIO -> we've run out of retries, and the subchannel still is not
> >>>>>      idle; I'm not sure if we could do anything here, as disable is
> >>>>>      unlikely to work, either  
> > 
> > (...)
> >   
> >> Thinking a little bit more about EIO, if the return code is EIO then it
> >> means we have exhausted all our options with cancel_halt_clear and the
> >> subchannel/device is still status pending, right?  
> > 
> > Yes.
> >   
> >>
> >> I think we should still continue to try and disable the subchannel,
> >> because if not then the subchannel/device could in some point of time
> >> come back and bite us. So we really should protect the system from this
> >> behavior.  
> > 
> > I think trying to disable the subchannel does not really hurt, but I
> > fear it won't succeed in that case...
> >   
> >>
> >> I think for EIO we should log an error message, but still try to
> >> continue with disabling the subchannel. What do you or others think?  
> > 
> > Logging an error may be useful (it's really fouled up at that time), but...
> >   
> >>
> >>
> >>
> >>  
> >>>>     
> >>>>>> +		flush_workqueue(vfio_ccw_work_q);
> >>>>>> +		spin_lock_irq(sch->lock);
> >>>>>>     		ret = cio_disable_subchannel(sch);  
> > 
> > ...there's a good chance that we'd get -EBUSY here, which would keep us
> > in the loop. We probably need to break out after we got -EIO from
> > cancel_halt_clear, regardless of which return code we get from the
> > disable.  
> 
> Okay, for EIO we can log an error message and break out of the loop.
> 
> I will send a v3. Are you going to queue patch 1 or patch 3 soon? If you 
> are then I will just send this patch separately.

Yes, please do send it separately. I'm currently testing patch 1 and 3
on top of my patchset, will queue either with or without the halt/clear
patches proper, depending on how soon I get acks/r-bs (hint, hint :)

> 
> Thanks
> Farhan
> 
> > 
> > (It will be "interesting" to see what happens with such a stuck
> > subchannel in the calling code; but I don't really see many options.
> > Panic seems way too strong; maybe mark the subchannel as "broken; no
> > idea how to fix"? But that would be a follow-on patch; I think if we
> > avoid the endless loop here, this patch is a real improvement and
> > should just go in.)
> >   
> >>>>>>     	} while (ret == -EBUSY);
> >>>>>>     out_unlock:  
> >>>>>
> >>>>>         
> >>>>     
> >>>
> >>>      
> >>  
> > 
> >   
> 

Re: [RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop

[Farhan Ali]

On 04/15/2019 10:18 AM, Cornelia Huck wrote:
> On Mon, 15 Apr 2019 09:38:37 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> On 04/15/2019 04:13 AM, Cornelia Huck wrote:
>>> On Fri, 12 Apr 2019 10:38:50 -0400
>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>    
>>>> On 04/12/2019 04:10 AM, Cornelia Huck wrote:
>>>>> On Thu, 11 Apr 2019 16:30:44 -0400
>>>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>>>       
>>>>>> On 04/11/2019 12:24 PM, Cornelia Huck wrote:
>>>>>>> On Mon,  8 Apr 2019 17:05:32 -0400
>>>>>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>    
>>>>>>> Looking at the possible return codes:
>>>>>>> * -ENODEV -> device is not operational anyway, in theory you should even
>>>>>>>        not need to bother with disabling the subchannel
>>>>>>> * -EIO -> we've run out of retries, and the subchannel still is not
>>>>>>>       idle; I'm not sure if we could do anything here, as disable is
>>>>>>>       unlikely to work, either
>>>
>>> (...)
>>>    
>>>> Thinking a little bit more about EIO, if the return code is EIO then it
>>>> means we have exhausted all our options with cancel_halt_clear and the
>>>> subchannel/device is still status pending, right?
>>>
>>> Yes.
>>>    
>>>>
>>>> I think we should still continue to try and disable the subchannel,
>>>> because if not then the subchannel/device could in some point of time
>>>> come back and bite us. So we really should protect the system from this
>>>> behavior.
>>>
>>> I think trying to disable the subchannel does not really hurt, but I
>>> fear it won't succeed in that case...
>>>    
>>>>
>>>> I think for EIO we should log an error message, but still try to
>>>> continue with disabling the subchannel. What do you or others think?
>>>
>>> Logging an error may be useful (it's really fouled up at that time), but...
>>>    
>>>>
>>>>
>>>>
>>>>   
>>>>>>      
>>>>>>>> +		flush_workqueue(vfio_ccw_work_q);
>>>>>>>> +		spin_lock_irq(sch->lock);
>>>>>>>>      		ret = cio_disable_subchannel(sch);
>>>
>>> ...there's a good chance that we'd get -EBUSY here, which would keep us
>>> in the loop. We probably need to break out after we got -EIO from
>>> cancel_halt_clear, regardless of which return code we get from the
>>> disable.
>>
>> Okay, for EIO we can log an error message and break out of the loop.
>>
>> I will send a v3. Are you going to queue patch 1 or patch 3 soon? If you
>> are then I will just send this patch separately.
> 
> Yes, please do send it separately. I'm currently testing patch 1 and 3
> on top of my patchset, will queue either with or without the halt/clear
> patches proper, depending on how soon I get acks/r-bs (hint, hint :)

I am going through your patches 4 and 6 and hopefully will get back to 
you by the end of the day :).

> 
>>
>> Thanks
>> Farhan
>>
>>>
>>> (It will be "interesting" to see what happens with such a stuck
>>> subchannel in the calling code; but I don't really see many options.
>>> Panic seems way too strong; maybe mark the subchannel as "broken; no
>>> idea how to fix"? But that would be a follow-on patch; I think if we
>>> avoid the endless loop here, this patch is a real improvement and
>>> should just go in.)
>>>    
>>>>>>>>      	} while (ret == -EBUSY);
>>>>>>>>      out_unlock:
>>>>>>>
>>>>>>>          
>>>>>>      
>>>>>
>>>>>       
>>>>   
>>>
>>>    
>>
> 
> 

Re: [RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop

[Cornelia Huck]

On Mon, 15 Apr 2019 10:24:53 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 04/15/2019 10:18 AM, Cornelia Huck wrote:

> > Yes, please do send it separately. I'm currently testing patch 1 and 3
> > on top of my patchset, will queue either with or without the halt/clear
> > patches proper, depending on how soon I get acks/r-bs (hint, hint :)  
> 
> I am going through your patches 4 and 6 and hopefully will get back to 
> you by the end of the day :).

Awesome, thanks!

Re: [RFC v2 0/3] fio-ccw fixes for kernel stacktraces

[Cornelia Huck]

On Mon,  8 Apr 2019 17:05:30 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> Hi,
> 
> I have recently seen some interesting kernel stacktraces while
> testing vfio-ccw mediated devices. These stack traces are seen
> when things don't go right, for example when a guest crashes or
> dies while still doing some I/O on the mediated device.
> 
> I have also come across an issue in the common vfio code as
> well which I reported upstream (https://marc.info/?l=kvm&m=155369393523503&w=2)
> and submitted a patch separately for it
> (https://marc.info/?l=kvm&m=155414703613868&w=2).
> 
> I would appreciate any review or feedback on these patches.
> 
> Thanks
> Farhan
> 
> ChangeLog
> ---------
> v1 -> v2
>    - Use the return code from cio_cancel_halt_clear to prevent
>      waiting unnecessarily (patch 2).
> 
>    - No need to reset the device on the remove path (patch 3).
> 
> Farhan Ali (3):
>   vfio-ccw: Do not call flush_workqueue while holding the spinlock
>   vfio-ccw: Prevent quiesce function going into an infinite loop
>   vfio-ccw: Release any channel program when releasing/removing vfio-ccw
>     mdev
> 
>  drivers/s390/cio/vfio_ccw_drv.c | 27 ++++++++++++---------------
>  drivers/s390/cio/vfio_ccw_ops.c | 11 ++++++++++-
>  2 files changed, 22 insertions(+), 16 deletions(-)
> 

Thanks, queued patches 1 and 3.