[PATCH v2 00/10] vfio: ccw: Refactoring the VFIO CCW state machine

[PATCH v2 00/10] vfio: ccw: Refactoring the VFIO CCW state machine

[Pierre Morel]

The goal of the patch serie is to enhance the state machine by:
- centralizing all state changes inside the state machine wrapper
- make the state change atomic using mutexes
- change user busy waiting for a passive wait on completion
- refactoring the initialization to avoid using a subchannel without a guest


This series introduces new states and events and suppressed
others.
- VFIO_CCW_STATE_NOT_OPER : when the Sub-Channel is KO
- VFIO_CCW_STATE_STANDBY  : when it is offline
- VFIO_CCW_STATE_IDLE     : when it is ready for I/O
- VFIO_CCW_STATE_BUSY     : when it is busy doing I/O
- VFIO_CCW_STATE_QUIESCING: when it is busy going offline

- VFIO_CCW_EVENT_INIT     : the channel setup (admin)
- VFIO_CCW_EVENT_NOT_OPER : something really wrong happened
- VFIO_CCW_EVENT_SSCH_REQ : Starting an I/O request (UAPI)
- VFIO_CCW_EVENT_INTERRUPT: Receiving an interrupt (callback)
- VFIO_CCW_EVENT_SCHIB_CHANGED: Receiving a channel event (callback)
- VFIO_CCW_EVENT_ONLINE   : Channel is set online (admin)
- VFIO_CCW_EVENT_OFFLINE  : Channel is set offline (admin)

The ABI did not change a lot beside letting the user sleep
when the driver is busy doing a SSCH, so a standard QEMU
devel branch can be used.


Pierre Morel (10):
  vfio: ccw: Moving state change out of IRQ context
  vfio: ccw: Transform FSM functions to return state
  vfio: ccw: new SCH_EVENT event
  vfio: ccw: replace IO_REQ event with SSCH_REQ event
  vfio: ccw: Suppress unused event parameter
  vfio: ccw: Make FSM functions atomic
  vfio: ccw: FSM and mediated device initialization
  vfio: ccw: Handling reset and shutdown with states
  vfio: ccw: Suppressing the BOXED state
  vfio: ccw: Let user wait when busy on IO

 drivers/s390/cio/vfio_ccw_drv.c     | 112 +++++-------------
 drivers/s390/cio/vfio_ccw_fsm.c     | 218 ++++++++++++++++++++++++------------
 drivers/s390/cio/vfio_ccw_ops.c     |  62 +++++-----
 drivers/s390/cio/vfio_ccw_private.h |  18 ++-
 4 files changed, 219 insertions(+), 191 deletions(-)

-- 
2.7.4

Changelog:
- rebased on current Linux branch
- change the name of VFIO_CCW_EVENT_SCH_EVENT to
  VFIO_CCW_EVENT_SCHIB_CHANGED
- refactoring the initialization (mdev create/open
  and driver probe)
- return -EAGAIN to let the low level retry the
  sending of events
- make wait_for_completion interruptible

[PATCH v2 01/10] vfio: ccw: Moving state change out of IRQ context

[Pierre Morel]

Let's move the state change from the IRQ routine to the
workqueue callback.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++-------------
 drivers/s390/cio/vfio_ccw_fsm.c | 14 ++++++++------
 2 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index ea6a2d0..57ae1ab 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -70,20 +70,9 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 static void vfio_ccw_sch_io_todo(struct work_struct *work)
 {
 	struct vfio_ccw_private *private;
-	struct irb *irb;
 
 	private = container_of(work, struct vfio_ccw_private, io_work);
-	irb = &private->irb;
-
-	if (scsw_is_solicited(&irb->scsw)) {
-		cp_update_scsw(&private->cp, &irb->scsw);
-		cp_free(&private->cp);
-	}
-	memcpy(private->io_region.irb_area, irb, sizeof(*irb));
-
-	if (private->io_trigger)
-		eventfd_signal(private->io_trigger, 1);
-
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
 	if (private->mdev)
 		private->state = VFIO_CCW_STATE_IDLE;
 }
@@ -94,9 +83,14 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 static void vfio_ccw_sch_irq(struct subchannel *sch)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+	struct irb *irb = this_cpu_ptr(&cio_irb);
 
 	inc_irq_stat(IRQIO_CIO);
-	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
+	memcpy(&private->irb, irb, sizeof(*irb));
+
+	queue_work(vfio_ccw_work_q, &private->io_work);
+	if (private->completion)
+		complete(private->completion);
 }
 
 static int vfio_ccw_sch_probe(struct subchannel *sch)
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 3c80064..24e1e04 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -172,14 +172,16 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 static void fsm_irq(struct vfio_ccw_private *private,
 		    enum vfio_ccw_event event)
 {
-	struct irb *irb = this_cpu_ptr(&cio_irb);
+	struct irb *irb = &private->irb;
 
-	memcpy(&private->irb, irb, sizeof(*irb));
-
-	queue_work(vfio_ccw_work_q, &private->io_work);
+	if (scsw_is_solicited(&irb->scsw)) {
+		cp_update_scsw(&private->cp, &irb->scsw);
+		cp_free(&private->cp);
+	}
+	memcpy(private->io_region.irb_area, irb, sizeof(*irb));
 
-	if (private->completion)
-		complete(private->completion);
+	if (private->io_trigger)
+		eventfd_signal(private->io_trigger, 1);
 }
 
 /*
-- 
2.7.4

Re: [PATCH v2 01/10] vfio: ccw: Moving state change out of IRQ context

[Cornelia Huck]

On Fri, 25 May 2018 12:21:09 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Let's move the state change from the IRQ routine to the
> workqueue callback.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++-------------
>  drivers/s390/cio/vfio_ccw_fsm.c | 14 ++++++++------
>  2 files changed, 15 insertions(+), 19 deletions(-)

This causes a change in behaviour for devices in the notoper state.

Now:
- vfio_ccw_sch_irq is called
- via the state machine, disabling the subchannel is (re-)triggered

With your patch:
- the work function is queued in any case; eventually, it will change
  the device's state to idle (unless we don't have an mdev at that
  point in time)
- completion is signaled

I'm not sure that's what we want.

Re: [PATCH v2 01/10] vfio: ccw: Moving state change out of IRQ context

[Pierre Morel]

On 04/06/2018 15:52, Cornelia Huck wrote:
> On Fri, 25 May 2018 12:21:09 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>
>> Let's move the state change from the IRQ routine to the
>> workqueue callback.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++-------------
>>   drivers/s390/cio/vfio_ccw_fsm.c | 14 ++++++++------
>>   2 files changed, 15 insertions(+), 19 deletions(-)
> This causes a change in behaviour for devices in the notoper state.
>
> Now:
> - vfio_ccw_sch_irq is called

This should not be done if the subchannel is not operational.

> - via the state machine, disabling the subchannel is (re-)triggered

I removed the fsm_disabled_irq() callback from VFIO_CCW_STATE_NOT_OPER
because the subchannel is not even initialized at that moment.
We have no reference to the subchannel.

In the previous driver NOT_OPER and STANDBY were quite the same.
Now NOT_OPER means "we can not operate on this sub channel"
because we do not have it in a correct state (no ISC, no mediated device,
the probe is not finiched)

Now STANDBY means we have the device ready but is disabled.
In this case the software infrastructure is ready and if an interrupt comes
(what should not happen) we will disable the subchannel again.

>
> With your patch:
> - the work function is queued in any case; eventually, it will change
>    the device's state to idle (unless we don't have an mdev at that
>    point in time)
> - completion is signaled
>
> I'm not sure that's what we want.
>

Yes it is queued in any case but the IRQ is really treated only if the
subchannel is in the right state (STANDBY, BUSY, IDLE and QUIESCING).

In the NOT_OPER state we do not have the mdev not the driver initialized.


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 01/10] vfio: ccw: Moving state change out of IRQ context

[Cornelia Huck]

On Tue, 5 Jun 2018 15:34:52 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 04/06/2018 15:52, Cornelia Huck wrote:
> > On Fri, 25 May 2018 12:21:09 +0200
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >  
> >> Let's move the state change from the IRQ routine to the
> >> workqueue callback.
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >> ---
> >>   drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++-------------
> >>   drivers/s390/cio/vfio_ccw_fsm.c | 14 ++++++++------
> >>   2 files changed, 15 insertions(+), 19 deletions(-)  
> > This causes a change in behaviour for devices in the notoper state.
> >
> > Now:
> > - vfio_ccw_sch_irq is called  
> 
> This should not be done if the subchannel is not operational.
> 
> > - via the state machine, disabling the subchannel is (re-)triggered  
> 
> I removed the fsm_disabled_irq() callback from VFIO_CCW_STATE_NOT_OPER
> because the subchannel is not even initialized at that moment.
> We have no reference to the subchannel.
> 
> In the previous driver NOT_OPER and STANDBY were quite the same.
> Now NOT_OPER means "we can not operate on this sub channel"
> because we do not have it in a correct state (no ISC, no mediated device,
> the probe is not finiched)
> 
> Now STANDBY means we have the device ready but is disabled.
> In this case the software infrastructure is ready and if an interrupt comes
> (what should not happen) we will disable the subchannel again.
> 
> >
> > With your patch:
> > - the work function is queued in any case; eventually, it will change
> >    the device's state to idle (unless we don't have an mdev at that
> >    point in time)
> > - completion is signaled
> >
> > I'm not sure that's what we want.
> >  
> 
> Yes it is queued in any case but the IRQ is really treated only if the
> subchannel is in the right state (STANDBY, BUSY, IDLE and QUIESCING).
> 
> In the NOT_OPER state we do not have the mdev not the driver initialized.

But all of this is only true after the whole series has been applied,
isn't it? Is there any way to do the changes without breaking things
inbetween?

What would also be very helpful is a sketch of the state machine after
your rework is done. Otherwise, this leaves me a bit unsure about the
intended semantics if I just look at the individual patches.

Re: [PATCH v2 01/10] vfio: ccw: Moving state change out of IRQ context

[Pierre Morel]

On 05/06/2018 15:52, Cornelia Huck wrote:
> On Tue, 5 Jun 2018 15:34:52 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> On 04/06/2018 15:52, Cornelia Huck wrote:
>>> On Fri, 25 May 2018 12:21:09 +0200
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>   
>>>> Let's move the state change from the IRQ routine to the
>>>> workqueue callback.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>> ---
>>>>    drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++-------------
>>>>    drivers/s390/cio/vfio_ccw_fsm.c | 14 ++++++++------
>>>>    2 files changed, 15 insertions(+), 19 deletions(-)
>>> This causes a change in behaviour for devices in the notoper state.
>>>
>>> Now:
>>> - vfio_ccw_sch_irq is called
>> This should not be done if the subchannel is not operational.
>>
>>> - via the state machine, disabling the subchannel is (re-)triggered
>> I removed the fsm_disabled_irq() callback from VFIO_CCW_STATE_NOT_OPER
>> because the subchannel is not even initialized at that moment.
>> We have no reference to the subchannel.
>>
>> In the previous driver NOT_OPER and STANDBY were quite the same.
>> Now NOT_OPER means "we can not operate on this sub channel"
>> because we do not have it in a correct state (no ISC, no mediated device,
>> the probe is not finiched)
>>
>> Now STANDBY means we have the device ready but is disabled.
>> In this case the software infrastructure is ready and if an interrupt comes
>> (what should not happen) we will disable the subchannel again.
>>
>>> With your patch:
>>> - the work function is queued in any case; eventually, it will change
>>>     the device's state to idle (unless we don't have an mdev at that
>>>     point in time)
>>> - completion is signaled
>>>
>>> I'm not sure that's what we want.
>>>   
>> Yes it is queued in any case but the IRQ is really treated only if the
>> subchannel is in the right state (STANDBY, BUSY, IDLE and QUIESCING).
>>
>> In the NOT_OPER state we do not have the mdev not the driver initialized.
> But all of this is only true after the whole series has been applied,
> isn't it? Is there any way to do the changes without breaking things
> inbetween?

I will think about this.
May be just disable the all thing untill all patches applied?

>
> What would also be very helpful is a sketch of the state machine after
> your rework is done. Otherwise, this leaves me a bit unsure about the
> intended semantics if I just look at the individual patches.
>

Right, I must enhance the cover letter.


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

[PATCH v2 02/10] vfio: ccw: Transform FSM functions to return state

[Pierre Morel]

We change the FSM functions to return the next state,
and adapt the fsm_func_t function type.

This will allow to factor FSM state change out of
the action routine.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 drivers/s390/cio/vfio_ccw_fsm.c     | 23 ++++++++++++++---------
 drivers/s390/cio/vfio_ccw_private.h |  5 +++--
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 24e1e04..756effb 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -65,7 +65,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
 	return ret;
 }
 
-static void fsm_notoper(struct vfio_ccw_private *private,
+static int fsm_notoper(struct vfio_ccw_private *private,
 			enum vfio_ccw_event event)
 {
 	struct subchannel *sch = private->sch;
@@ -75,31 +75,34 @@ static void fsm_notoper(struct vfio_ccw_private *private,
 	 * Probably we should send the machine check to the guest.
 	 */
 	css_sched_sch_todo(sch, SCH_TODO_UNREG);
-	private->state = VFIO_CCW_STATE_NOT_OPER;
+	return VFIO_CCW_STATE_NOT_OPER;
 }
 
 /*
  * No operation action.
  */
-static void fsm_nop(struct vfio_ccw_private *private,
+static int fsm_nop(struct vfio_ccw_private *private,
 		    enum vfio_ccw_event event)
 {
+	return private->state;
 }
 
-static void fsm_io_error(struct vfio_ccw_private *private,
+static int fsm_io_error(struct vfio_ccw_private *private,
 			 enum vfio_ccw_event event)
 {
 	pr_err("vfio-ccw: FSM: I/O request from state:%d\n", private->state);
 	private->io_region.ret_code = -EIO;
+	return private->state;
 }
 
-static void fsm_io_busy(struct vfio_ccw_private *private,
+static int fsm_io_busy(struct vfio_ccw_private *private,
 			enum vfio_ccw_event event)
 {
 	private->io_region.ret_code = -EBUSY;
+	return private->state;
 }
 
-static void fsm_disabled_irq(struct vfio_ccw_private *private,
+static int fsm_disabled_irq(struct vfio_ccw_private *private,
 			     enum vfio_ccw_event event)
 {
 	struct subchannel *sch = private->sch;
@@ -109,12 +112,13 @@ static void fsm_disabled_irq(struct vfio_ccw_private *private,
 	 * successful - should not happen, but we try to disable again.
 	 */
 	cio_disable_subchannel(sch);
+	return private->state;
 }
 
 /*
  * Deal with the ccw command request from the userspace.
  */
-static void fsm_io_request(struct vfio_ccw_private *private,
+static int fsm_io_request(struct vfio_ccw_private *private,
 			   enum vfio_ccw_event event)
 {
 	union orb *orb;
@@ -163,13 +167,13 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 	}
 
 err_out:
-	private->state = VFIO_CCW_STATE_IDLE;
+	return VFIO_CCW_STATE_IDLE;
 }
 
 /*
  * Got an interrupt for a normal io (state busy).
  */
-static void fsm_irq(struct vfio_ccw_private *private,
+static int fsm_irq(struct vfio_ccw_private *private,
 		    enum vfio_ccw_event event)
 {
 	struct irb *irb = &private->irb;
@@ -182,6 +186,7 @@ static void fsm_irq(struct vfio_ccw_private *private,
 
 	if (private->io_trigger)
 		eventfd_signal(private->io_trigger, 1);
+	return private->state;
 }
 
 /*
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 78a66d9..f526b18 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -83,13 +83,14 @@ enum vfio_ccw_event {
 /*
  * Action called through jumptable.
  */
-typedef void (fsm_func_t)(struct vfio_ccw_private *, enum vfio_ccw_event);
+typedef int (fsm_func_t)(struct vfio_ccw_private *, enum vfio_ccw_event);
 extern fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS];
 
 static inline void vfio_ccw_fsm_event(struct vfio_ccw_private *private,
 				     int event)
 {
-	vfio_ccw_jumptable[private->state][event](private, event);
+	private->state = vfio_ccw_jumptable[private->state][event](private,
+								   event);
 }
 
 extern struct workqueue_struct *vfio_ccw_work_q;
-- 
2.7.4

[PATCH v2 03/10] vfio: ccw: new SCH_EVENT event

[Pierre Morel]

The Sub channel event callback may be called with interrupt
or thread context.
Let's thread the FSM action using workqueues.

The update of the SCHIB is now done inside an FSM action.
using the VFIO_CCW_EVENT_SCHIB_CHANGED event to guaranty
coherency with other FSM events.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 33 ++++++++++++---------------------
 drivers/s390/cio/vfio_ccw_fsm.c     | 23 +++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_private.h |  3 +++
 3 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 57ae1ab..6b7112e 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -77,6 +77,14 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 		private->state = VFIO_CCW_STATE_IDLE;
 }
 
+static void vfio_ccw_sch_event_todo(struct work_struct *work)
+{
+	struct vfio_ccw_private *private;
+
+	private = container_of(work, struct vfio_ccw_private, event_work);
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_SCHIB_CHANGED);
+}
+
 /*
  * Css driver callbacks
  */
@@ -124,6 +132,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 		goto out_disable;
 
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
+	INIT_WORK(&private->event_work, vfio_ccw_sch_event_todo);
 	atomic_set(&private->avail, 1);
 	private->state = VFIO_CCW_STATE_STANDBY;
 
@@ -170,28 +179,10 @@ static void vfio_ccw_sch_shutdown(struct subchannel *sch)
 static int vfio_ccw_sch_event(struct subchannel *sch, int process)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
-	unsigned long flags;
-
-	spin_lock_irqsave(sch->lock, flags);
-	if (!device_is_registered(&sch->dev))
-		goto out_unlock;
 
-	if (work_pending(&sch->todo_work))
-		goto out_unlock;
-
-	if (cio_update_schib(sch)) {
-		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
-		goto out_unlock;
-	}
-
-	private = dev_get_drvdata(&sch->dev);
-	if (private->state == VFIO_CCW_STATE_NOT_OPER) {
-		private->state = private->mdev ? VFIO_CCW_STATE_IDLE :
-				 VFIO_CCW_STATE_STANDBY;
-	}
-
-out_unlock:
-	spin_unlock_irqrestore(sch->lock, flags);
+	if (work_pending(&private->event_work))
+		return -EAGAIN;
+	queue_work(vfio_ccw_work_q, &private->event_work);
 
 	return 0;
 }
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 756effb..2bc4be66 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -190,6 +190,24 @@ static int fsm_irq(struct vfio_ccw_private *private,
 }
 
 /*
+ * Got a sub-channel event .
+ */
+static int fsm_sch_event(struct vfio_ccw_private *private,
+			 enum vfio_ccw_event event)
+{
+	unsigned long flags;
+	int ret = private->state;
+	struct subchannel *sch = private->sch;
+
+	spin_lock_irqsave(sch->lock, flags);
+	if (cio_update_schib(sch))
+		ret = VFIO_CCW_STATE_NOT_OPER;
+	spin_unlock_irqrestore(sch->lock, flags);
+
+	return ret;
+}
+
+/*
  * Device statemachine
  */
 fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
@@ -197,25 +215,30 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_nop,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
+		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_nop,
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
 	[VFIO_CCW_STATE_IDLE] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
 	[VFIO_CCW_STATE_BOXED] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
 	[VFIO_CCW_STATE_BUSY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
 };
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index f526b18..a2be0c9 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -33,6 +33,7 @@
  * @scsw: scsw info
  * @io_trigger: eventfd ctx for signaling userspace I/O results
  * @io_work: work for deferral process of I/O handling
+ * @event_work: work for deferral process of sub-channel event
  */
 struct vfio_ccw_private {
 	struct subchannel	*sch;
@@ -49,6 +50,7 @@ struct vfio_ccw_private {
 
 	struct eventfd_ctx	*io_trigger;
 	struct work_struct	io_work;
+	struct work_struct	event_work;
 } __aligned(8);
 
 extern int vfio_ccw_mdev_reg(struct subchannel *sch);
@@ -76,6 +78,7 @@ enum vfio_ccw_event {
 	VFIO_CCW_EVENT_NOT_OPER,
 	VFIO_CCW_EVENT_IO_REQ,
 	VFIO_CCW_EVENT_INTERRUPT,
+	VFIO_CCW_EVENT_SCHIB_CHANGED,
 	/* last element! */
 	NR_VFIO_CCW_EVENTS
 };
-- 
2.7.4

[PATCH v2 04/10] vfio: ccw: replace IO_REQ event with SSCH_REQ event

[Pierre Morel]

The write callback uses the a memory cell of the io_region
as the instruction to proceed.

Since we currently ever used only one instruction, SSCH,
let's state that we can only handle one instruction at a time
- by using a switch/case
- by changing the name of the event to VFIO_CCW_EVENT_SSCH_REQ

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 drivers/s390/cio/vfio_ccw_fsm.c     | 67 ++++++++++++-------------------------
 drivers/s390/cio/vfio_ccw_ops.c     | 16 +++++----
 drivers/s390/cio/vfio_ccw_private.h |  2 +-
 3 files changed, 31 insertions(+), 54 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 2bc4be66..86dab8c 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -19,14 +19,10 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
 	union orb *orb;
 	int ccode;
 	__u8 lpm;
-	unsigned long flags;
 	int ret;
 
 	sch = private->sch;
 
-	spin_lock_irqsave(sch->lock, flags);
-	private->state = VFIO_CCW_STATE_BUSY;
-
 	orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
 
 	/* Issue "Start Subchannel" */
@@ -61,7 +57,6 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
 	default:
 		ret = ccode;
 	}
-	spin_unlock_irqrestore(sch->lock, flags);
 	return ret;
 }
 
@@ -122,50 +117,30 @@ static int fsm_io_request(struct vfio_ccw_private *private,
 			   enum vfio_ccw_event event)
 {
 	union orb *orb;
-	union scsw *scsw = &private->scsw;
 	struct ccw_io_region *io_region = &private->io_region;
 	struct mdev_device *mdev = private->mdev;
 
 	private->state = VFIO_CCW_STATE_BOXED;
 
-	memcpy(scsw, io_region->scsw_area, sizeof(*scsw));
-
-	if (scsw->cmd.fctl & SCSW_FCTL_START_FUNC) {
-		orb = (union orb *)io_region->orb_area;
-
-		/* Don't try to build a cp if transport mode is specified. */
-		if (orb->tm.b) {
-			io_region->ret_code = -EOPNOTSUPP;
-			goto err_out;
-		}
-		io_region->ret_code = cp_init(&private->cp, mdev_dev(mdev),
-					      orb);
-		if (io_region->ret_code)
-			goto err_out;
-
-		io_region->ret_code = cp_prefetch(&private->cp);
-		if (io_region->ret_code) {
-			cp_free(&private->cp);
-			goto err_out;
-		}
-
-		/* Start channel program and wait for I/O interrupt. */
-		io_region->ret_code = fsm_io_helper(private);
-		if (io_region->ret_code) {
-			cp_free(&private->cp);
-			goto err_out;
-		}
-		return;
-	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
-		/* XXX: Handle halt. */
-		io_region->ret_code = -EOPNOTSUPP;
+	orb = (union orb *)io_region->orb_area;
+
+	io_region->ret_code = cp_init(&private->cp, mdev_dev(mdev), orb);
+	if (io_region->ret_code)
 		goto err_out;
-	} else if (scsw->cmd.fctl & SCSW_FCTL_CLEAR_FUNC) {
-		/* XXX: Handle clear. */
-		io_region->ret_code = -EOPNOTSUPP;
+
+	io_region->ret_code = cp_prefetch(&private->cp);
+	if (io_region->ret_code) {
+		cp_free(&private->cp);
 		goto err_out;
 	}
 
+	io_region->ret_code = fsm_io_helper(private);
+	if (io_region->ret_code) {
+		cp_free(&private->cp);
+		goto err_out;
+	}
+	return VFIO_CCW_STATE_BUSY;
+
 err_out:
 	return VFIO_CCW_STATE_IDLE;
 }
@@ -186,7 +161,7 @@ static int fsm_irq(struct vfio_ccw_private *private,
 
 	if (private->io_trigger)
 		eventfd_signal(private->io_trigger, 1);
-	return private->state;
+	return VFIO_CCW_STATE_IDLE;
 }
 
 /*
@@ -213,31 +188,31 @@ static int fsm_sch_event(struct vfio_ccw_private *private,
 fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 	[VFIO_CCW_STATE_NOT_OPER] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_nop,
-		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
+		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_nop,
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
-		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
+		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
 	[VFIO_CCW_STATE_IDLE] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
-		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_request,
+		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
 	[VFIO_CCW_STATE_BOXED] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
-		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
+		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
 	[VFIO_CCW_STATE_BUSY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
-		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
+		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 41eeb57..0206101 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -188,25 +188,27 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
 {
 	struct vfio_ccw_private *private;
 	struct ccw_io_region *region;
+	union scsw *scsw;
 
 	if (*ppos + count > sizeof(*region))
 		return -EINVAL;
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
-	if (private->state != VFIO_CCW_STATE_IDLE)
-		return -EACCES;
 
 	region = &private->io_region;
 	if (copy_from_user((void *)region + *ppos, buf, count))
 		return -EFAULT;
 
-	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ);
-	if (region->ret_code != 0) {
-		private->state = VFIO_CCW_STATE_IDLE;
-		return region->ret_code;
+	scsw = (union scsw *) &region->scsw_area;
+	switch (scsw->cmd.fctl) {
+	case SCSW_FCTL_START_FUNC:
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_SSCH_REQ);
+		break;
+	default:
+		return -EOPNOTSUPP;
 	}
 
-	return count;
+	return region->ret_code ? region->ret_code : count;
 }
 
 static int vfio_ccw_mdev_get_device_info(struct vfio_device_info *info)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index a2be0c9..94fb408 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -76,7 +76,7 @@ enum vfio_ccw_state {
  */
 enum vfio_ccw_event {
 	VFIO_CCW_EVENT_NOT_OPER,
-	VFIO_CCW_EVENT_IO_REQ,
+	VFIO_CCW_EVENT_SSCH_REQ,
 	VFIO_CCW_EVENT_INTERRUPT,
 	VFIO_CCW_EVENT_SCHIB_CHANGED,
 	/* last element! */
-- 
2.7.4

[PATCH v2 05/10] vfio: ccw: Suppress unused event parameter

[Pierre Morel]

The fsm_func_t function type definition does not need the event
parameter since all functions are in a state/event table.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 drivers/s390/cio/vfio_ccw_fsm.c     | 24 ++++++++----------------
 drivers/s390/cio/vfio_ccw_private.h |  5 ++---
 2 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 86dab8c..077da23 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -60,8 +60,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
 	return ret;
 }
 
-static int fsm_notoper(struct vfio_ccw_private *private,
-			enum vfio_ccw_event event)
+static int fsm_notoper(struct vfio_ccw_private *private)
 {
 	struct subchannel *sch = private->sch;
 
@@ -76,29 +75,25 @@ static int fsm_notoper(struct vfio_ccw_private *private,
 /*
  * No operation action.
  */
-static int fsm_nop(struct vfio_ccw_private *private,
-		    enum vfio_ccw_event event)
+static int fsm_nop(struct vfio_ccw_private *private)
 {
 	return private->state;
 }
 
-static int fsm_io_error(struct vfio_ccw_private *private,
-			 enum vfio_ccw_event event)
+static int fsm_io_error(struct vfio_ccw_private *private)
 {
 	pr_err("vfio-ccw: FSM: I/O request from state:%d\n", private->state);
 	private->io_region.ret_code = -EIO;
 	return private->state;
 }
 
-static int fsm_io_busy(struct vfio_ccw_private *private,
-			enum vfio_ccw_event event)
+static int fsm_io_busy(struct vfio_ccw_private *private)
 {
 	private->io_region.ret_code = -EBUSY;
 	return private->state;
 }
 
-static int fsm_disabled_irq(struct vfio_ccw_private *private,
-			     enum vfio_ccw_event event)
+static int fsm_disabled_irq(struct vfio_ccw_private *private)
 {
 	struct subchannel *sch = private->sch;
 
@@ -113,8 +108,7 @@ static int fsm_disabled_irq(struct vfio_ccw_private *private,
 /*
  * Deal with the ccw command request from the userspace.
  */
-static int fsm_io_request(struct vfio_ccw_private *private,
-			   enum vfio_ccw_event event)
+static int fsm_io_request(struct vfio_ccw_private *private)
 {
 	union orb *orb;
 	struct ccw_io_region *io_region = &private->io_region;
@@ -148,8 +142,7 @@ static int fsm_io_request(struct vfio_ccw_private *private,
 /*
  * Got an interrupt for a normal io (state busy).
  */
-static int fsm_irq(struct vfio_ccw_private *private,
-		    enum vfio_ccw_event event)
+static int fsm_irq(struct vfio_ccw_private *private)
 {
 	struct irb *irb = &private->irb;
 
@@ -167,8 +160,7 @@ static int fsm_irq(struct vfio_ccw_private *private,
 /*
  * Got a sub-channel event .
  */
-static int fsm_sch_event(struct vfio_ccw_private *private,
-			 enum vfio_ccw_event event)
+static int fsm_sch_event(struct vfio_ccw_private *private)
 {
 	unsigned long flags;
 	int ret = private->state;
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 94fb408..6c74f73 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -86,14 +86,13 @@ enum vfio_ccw_event {
 /*
  * Action called through jumptable.
  */
-typedef int (fsm_func_t)(struct vfio_ccw_private *, enum vfio_ccw_event);
+typedef int (fsm_func_t)(struct vfio_ccw_private *);
 extern fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS];
 
 static inline void vfio_ccw_fsm_event(struct vfio_ccw_private *private,
 				     int event)
 {
-	private->state = vfio_ccw_jumptable[private->state][event](private,
-								   event);
+	private->state = vfio_ccw_jumptable[private->state][event](private);
 }
 
 extern struct workqueue_struct *vfio_ccw_work_q;
-- 
2.7.4

[PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

[Pierre Morel]

We use mutex around the FSM function call to make the FSM
event handling and state change atomic.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 3 +--
 drivers/s390/cio/vfio_ccw_private.h | 3 +++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 6b7112e..98951d5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 
 	private = container_of(work, struct vfio_ccw_private, io_work);
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
-	if (private->mdev)
-		private->state = VFIO_CCW_STATE_IDLE;
 }
 
 static void vfio_ccw_sch_event_todo(struct work_struct *work)
@@ -118,6 +116,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 		return -ENOMEM;
 	private->sch = sch;
 	dev_set_drvdata(&sch->dev, private);
+	mutex_init(&private->state_mutex);
 
 	spin_lock_irq(sch->lock);
 	private->state = VFIO_CCW_STATE_NOT_OPER;
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 6c74f73..241176c 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -51,6 +51,7 @@ struct vfio_ccw_private {
 	struct eventfd_ctx	*io_trigger;
 	struct work_struct	io_work;
 	struct work_struct	event_work;
+	struct mutex		state_mutex;
 } __aligned(8);
 
 extern int vfio_ccw_mdev_reg(struct subchannel *sch);
@@ -92,7 +93,9 @@ extern fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS];
 static inline void vfio_ccw_fsm_event(struct vfio_ccw_private *private,
 				     int event)
 {
+	mutex_lock(&private->state_mutex);
 	private->state = vfio_ccw_jumptable[private->state][event](private);
+	mutex_unlock(&private->state_mutex);
 }
 
 extern struct workqueue_struct *vfio_ccw_work_q;
-- 
2.7.4

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

[Cornelia Huck]

On Fri, 25 May 2018 12:21:14 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> We use mutex around the FSM function call to make the FSM
> event handling and state change atomic.

I'm still not really clear as to what this mutex is supposed to
serialize:

- Modification of the state?
- Any calls in the state machine?
- A combination? (That would imply that we only deal with the state in
  the state machine.)

> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     | 3 +--
>  drivers/s390/cio/vfio_ccw_private.h | 3 +++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 6b7112e..98951d5 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>  
>  	private = container_of(work, struct vfio_ccw_private, io_work);
>  	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
> -	if (private->mdev)
> -		private->state = VFIO_CCW_STATE_IDLE;

Looks like an unrelated change? If you want to do all state changes
under the mutex, that should rather be moved than deleted, shouldn't it?

>  }
>  
>  static void vfio_ccw_sch_event_todo(struct work_struct *work)

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

[Pierre Morel]

On 05/06/2018 13:38, Cornelia Huck wrote:
> On Fri, 25 May 2018 12:21:14 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>
>> We use mutex around the FSM function call to make the FSM
>> event handling and state change atomic.
> I'm still not really clear as to what this mutex is supposed to
> serialize:
>
> - Modification of the state?
> - Any calls in the state machine?
> - A combination? (That would imply that we only deal with the state in
>    the state machine.)

yes to all

>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_drv.c     | 3 +--
>>   drivers/s390/cio/vfio_ccw_private.h | 3 +++
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>> index 6b7112e..98951d5 100644
>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>> @@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>>   
>>   	private = container_of(work, struct vfio_ccw_private, io_work);
>>   	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
>> -	if (private->mdev)
>> -		private->state = VFIO_CCW_STATE_IDLE;
> Looks like an unrelated change? If you want to do all state changes
> under the mutex, that should rather be moved than deleted, shouldn't it?

It is moved to fsm_irq() which is called under mutex.
fsm_irq() returns VFIO_CCW_STATE_IDLE.

>
>>   }
>>   
>>   static void vfio_ccw_sch_event_todo(struct work_struct *work)


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

[Cornelia Huck]

On Tue, 5 Jun 2018 15:10:11 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 05/06/2018 13:38, Cornelia Huck wrote:
> > On Fri, 25 May 2018 12:21:14 +0200
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >  
> >> We use mutex around the FSM function call to make the FSM
> >> event handling and state change atomic.  
> > I'm still not really clear as to what this mutex is supposed to
> > serialize:
> >
> > - Modification of the state?
> > - Any calls in the state machine?
> > - A combination? (That would imply that we only deal with the state in
> >    the state machine.)  
> 
> yes to all

But wouldn't that imply that you need to either take the mutex if you
do something dependent on the state, or fire an event in that case?

> 
> >  
> >> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >> ---
> >>   drivers/s390/cio/vfio_ccw_drv.c     | 3 +--
> >>   drivers/s390/cio/vfio_ccw_private.h | 3 +++
> >>   2 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> >> index 6b7112e..98951d5 100644
> >> --- a/drivers/s390/cio/vfio_ccw_drv.c
> >> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> >> @@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
> >>   
> >>   	private = container_of(work, struct vfio_ccw_private, io_work);
> >>   	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
> >> -	if (private->mdev)
> >> -		private->state = VFIO_CCW_STATE_IDLE;  
> > Looks like an unrelated change? If you want to do all state changes
> > under the mutex, that should rather be moved than deleted, shouldn't it?  
> 
> It is moved to fsm_irq() which is called under mutex.
> fsm_irq() returns VFIO_CCW_STATE_IDLE.

So, should that go into another patch?

> 
> >  
> >>   }
> >>   
> >>   static void vfio_ccw_sch_event_todo(struct work_struct *work)  
> 
> 

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

[Pierre Morel]

On 05/06/2018 15:35, Cornelia Huck wrote:
> On Tue, 5 Jun 2018 15:10:11 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> On 05/06/2018 13:38, Cornelia Huck wrote:
>>> On Fri, 25 May 2018 12:21:14 +0200
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>   
>>>> We use mutex around the FSM function call to make the FSM
>>>> event handling and state change atomic.
>>> I'm still not really clear as to what this mutex is supposed to
>>> serialize:
>>>
>>> - Modification of the state?
>>> - Any calls in the state machine?
>>> - A combination? (That would imply that we only deal with the state in
>>>     the state machine.)
>> yes to all
> But wouldn't that imply that you need to either take the mutex if you
> do something dependent on the state, or fire an event in that case?

Yes, if it is not I forgot something important (like I did in patch 10)
vfio_ccw_fsm_event(private, event) takes the mutex on firering an event.

I have some cases where I do not respect this.
This is false and I must handle this with a new private variable,
this is where I test the state after having fired an event.
I will need to change this, in quiesce, reset, probe and open (others?).


>
>>>   
>>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>> ---
>>>>    drivers/s390/cio/vfio_ccw_drv.c     | 3 +--
>>>>    drivers/s390/cio/vfio_ccw_private.h | 3 +++
>>>>    2 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>>>> index 6b7112e..98951d5 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>>>> @@ -73,8 +73,6 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>>>>    
>>>>    	private = container_of(work, struct vfio_ccw_private, io_work);
>>>>    	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
>>>> -	if (private->mdev)
>>>> -		private->state = VFIO_CCW_STATE_IDLE;
>>> Looks like an unrelated change? If you want to do all state changes
>>> under the mutex, that should rather be moved than deleted, shouldn't it?
>> It is moved to fsm_irq() which is called under mutex.
>> fsm_irq() returns VFIO_CCW_STATE_IDLE.
> So, should that go into another patch?

I will see if I can put it inside the patch 01/10 moving state change 
out of IRQ context.


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 06/10] vfio: ccw: Make FSM functions atomic

[Cornelia Huck]

On Tue, 5 Jun 2018 16:21:03 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 05/06/2018 15:35, Cornelia Huck wrote:
> > On Tue, 5 Jun 2018 15:10:11 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >  
> >> On 05/06/2018 13:38, Cornelia Huck wrote:  
> >>> On Fri, 25 May 2018 12:21:14 +0200
> >>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >>>     
> >>>> We use mutex around the FSM function call to make the FSM
> >>>> event handling and state change atomic.  
> >>> I'm still not really clear as to what this mutex is supposed to
> >>> serialize:
> >>>
> >>> - Modification of the state?
> >>> - Any calls in the state machine?
> >>> - A combination? (That would imply that we only deal with the state in
> >>>     the state machine.)  
> >> yes to all  
> > But wouldn't that imply that you need to either take the mutex if you
> > do something dependent on the state, or fire an event in that case?  
> 
> Yes, if it is not I forgot something important (like I did in patch 10)
> vfio_ccw_fsm_event(private, event) takes the mutex on firering an event.
> 
> I have some cases where I do not respect this.
> This is false and I must handle this with a new private variable,
> this is where I test the state after having fired an event.
> I will need to change this, in quiesce, reset, probe and open (others?).

But isn't that inherently racy? Even if you check the return code from
the state machine change, it might have changed in the meantime again.

[PATCH v2 07/10] vfio: ccw: FSM and mediated device initialization

[Pierre Morel]

When the physical device is probed it is existing but should
not be able to do any operations since no driver is available
until a guest is there.
Hence the state is set to VFIO_CCW_STATE_NOT_OPER.

When the mediated device is created, nothing is changed for
the device, it still stay not operational.

When the guest is starting the state machine recieves the
VFIO_CCW_EVENT_INIT event which statrts the fsm_init action
to bring the state to VFIO_CCW_STATE_STANDBY.

The VFIO_DEVICE_RESET command (not part of this patch)
will bring the FSM state to VFIO_CCW_STATE_IDLE.

Before the mediated device is opened by QEMU, the vfio_private
structure is not completely initialized.
Let's change the actions for the VFIO_CCW_STATE_NOT_OPER
to fsm_nop when vfio_private is not initialized.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 22 ++++++----------------
 drivers/s390/cio/vfio_ccw_fsm.c     | 25 +++++++++++++++++++++++--
 drivers/s390/cio/vfio_ccw_ops.c     | 25 +++++++++++++------------
 drivers/s390/cio/vfio_ccw_private.h |  1 +
 4 files changed, 43 insertions(+), 30 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 98951d5..6fc7668 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -114,31 +114,21 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	private = kzalloc(sizeof(*private), GFP_KERNEL | GFP_DMA);
 	if (!private)
 		return -ENOMEM;
+
+	private->state = VFIO_CCW_STATE_NOT_OPER;
 	private->sch = sch;
 	dev_set_drvdata(&sch->dev, private);
 	mutex_init(&private->state_mutex);
-
-	spin_lock_irq(sch->lock);
-	private->state = VFIO_CCW_STATE_NOT_OPER;
-	sch->isc = VFIO_CCW_ISC;
-	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
-	spin_unlock_irq(sch->lock);
-	if (ret)
-		goto out_free;
-
-	ret = vfio_ccw_mdev_reg(sch);
-	if (ret)
-		goto out_disable;
-
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
 	INIT_WORK(&private->event_work, vfio_ccw_sch_event_todo);
 	atomic_set(&private->avail, 1);
-	private->state = VFIO_CCW_STATE_STANDBY;
+
+	ret = vfio_ccw_mdev_reg(sch);
+	if (ret)
+		goto out_free;
 
 	return 0;
 
-out_disable:
-	cio_disable_subchannel(sch);
 out_free:
 	dev_set_drvdata(&sch->dev, NULL);
 	kfree(private);
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 077da23..20b909c 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -9,6 +9,7 @@
 
 #include <linux/vfio.h>
 #include <linux/mdev.h>
+#include <asm/isc.h>
 
 #include "ioasm.h"
 #include "vfio_ccw_private.h"
@@ -174,35 +175,55 @@ static int fsm_sch_event(struct vfio_ccw_private *private)
 	return ret;
 }
 
+static int fsm_init(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch = private->sch;
+	int ret = VFIO_CCW_STATE_STANDBY;
+
+	spin_lock_irq(sch->lock);
+	sch->isc = VFIO_CCW_ISC;
+	if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
+		ret = VFIO_CCW_STATE_NOT_OPER;
+	spin_unlock_irq(sch->lock);
+
+	return ret;
+}
+
+
 /*
  * Device statemachine
  */
 fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 	[VFIO_CCW_STATE_NOT_OPER] = {
+		[VFIO_CCW_EVENT_INIT]		= fsm_init,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_nop,
-		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_error,
-		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
+		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_nop,
+		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_nop,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_nop,
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
+		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
 	[VFIO_CCW_STATE_IDLE] = {
+		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
 	[VFIO_CCW_STATE_BOXED] = {
+		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
 	[VFIO_CCW_STATE_BUSY] = {
+		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 0206101..ea8fd64 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -111,14 +111,10 @@ static int vfio_ccw_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 	struct vfio_ccw_private *private =
 		dev_get_drvdata(mdev_parent_dev(mdev));
 
-	if (private->state == VFIO_CCW_STATE_NOT_OPER)
-		return -ENODEV;
-
 	if (atomic_dec_if_positive(&private->avail) < 0)
 		return -EPERM;
 
 	private->mdev = mdev;
-	private->state = VFIO_CCW_STATE_IDLE;
 
 	return 0;
 }
@@ -128,13 +124,6 @@ static int vfio_ccw_mdev_remove(struct mdev_device *mdev)
 	struct vfio_ccw_private *private =
 		dev_get_drvdata(mdev_parent_dev(mdev));
 
-	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. */
-	}
-
 	private->mdev = NULL;
 	atomic_inc(&private->avail);
 
@@ -146,11 +135,22 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
 	struct vfio_ccw_private *private =
 		dev_get_drvdata(mdev_parent_dev(mdev));
 	unsigned long events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
+	int ret;
 
 	private->nb.notifier_call = vfio_ccw_mdev_notifier;
 
-	return vfio_register_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+	ret = vfio_register_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
 				      &events, &private->nb);
+	if (ret)
+		return ret;
+
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INIT);
+	if (private->state == VFIO_CCW_STATE_STANDBY)
+		return 0;
+
+	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+				 &private->nb);
+	return -EFAULT;
 }
 
 static void vfio_ccw_mdev_release(struct mdev_device *mdev)
@@ -158,6 +158,7 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
 	struct vfio_ccw_private *private =
 		dev_get_drvdata(mdev_parent_dev(mdev));
 
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
 				 &private->nb);
 }
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 241176c..c5455a9 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -76,6 +76,7 @@ enum vfio_ccw_state {
  * Asynchronous events of the device statemachine.
  */
 enum vfio_ccw_event {
+	VFIO_CCW_EVENT_INIT,
 	VFIO_CCW_EVENT_NOT_OPER,
 	VFIO_CCW_EVENT_SSCH_REQ,
 	VFIO_CCW_EVENT_INTERRUPT,
-- 
2.7.4

[PATCH v2 08/10] vfio: ccw: Handling reset and shutdown with states

[Pierre Morel]

Two new events, VFIO_CCW_EVENT_ONLINE and VFIO_CCW_EVENT_OFFLINE
allow to handle the enabling and disabling of a Sub Channel and
the init, shutdown, quiesce and reset operations are changed
accordingly.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 44 ++++------------------
 drivers/s390/cio/vfio_ccw_fsm.c     | 75 +++++++++++++++++++++++++++++++++----
 drivers/s390/cio/vfio_ccw_ops.c     | 15 ++------
 drivers/s390/cio/vfio_ccw_private.h |  3 ++
 4 files changed, 82 insertions(+), 55 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 6fc7668..3e7b514 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -30,41 +30,13 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
 	DECLARE_COMPLETION_ONSTACK(completion);
-	int iretry, ret = 0;
-
-	spin_lock_irq(sch->lock);
-	if (!sch->schib.pmcw.ena)
-		goto out_unlock;
-	ret = cio_disable_subchannel(sch);
-	if (ret != -EBUSY)
-		goto out_unlock;
-
-	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);
-
-			wait_for_completion_timeout(&completion, 3*HZ);
-
-			spin_lock_irq(sch->lock);
-			private->completion = NULL;
-			flush_workqueue(vfio_ccw_work_q);
-			ret = cio_cancel_halt_clear(sch, &iretry);
-		};
-
-		ret = cio_disable_subchannel(sch);
-	} while (ret == -EBUSY);
-out_unlock:
-	private->state = VFIO_CCW_STATE_NOT_OPER;
-	spin_unlock_irq(sch->lock);
-	return ret;
+
+	private->completion = &completion;
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OFFLINE);
+	wait_for_completion_interruptible_timeout(&completion, jiffies + 3*HZ);
+	if (private->state != VFIO_CCW_STATE_STANDBY)
+		return -EFAULT;
+	return 0;
 }
 
 static void vfio_ccw_sch_io_todo(struct work_struct *work)
@@ -95,8 +67,6 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
 	memcpy(&private->irb, irb, sizeof(*irb));
 
 	queue_work(vfio_ccw_work_q, &private->io_work);
-	if (private->completion)
-		complete(private->completion);
 }
 
 static int vfio_ccw_sch_probe(struct subchannel *sch)
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 20b909c..0acab2f 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -73,6 +73,53 @@ static int fsm_notoper(struct vfio_ccw_private *private)
 	return VFIO_CCW_STATE_NOT_OPER;
 }
 
+static int fsm_online(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch = private->sch;
+	int ret = VFIO_CCW_STATE_IDLE;
+
+	spin_lock_irq(sch->lock);
+	if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
+		ret = VFIO_CCW_STATE_NOT_OPER;
+	spin_unlock_irq(sch->lock);
+
+	return ret;
+}
+static int fsm_offline(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch = private->sch;
+	int ret = VFIO_CCW_STATE_STANDBY;
+
+	spin_lock_irq(sch->lock);
+	if (cio_disable_subchannel(sch))
+		ret = VFIO_CCW_STATE_NOT_OPER;
+	spin_unlock_irq(sch->lock);
+	if (private->completion)
+		complete(private->completion);
+
+	return ret;
+}
+static int fsm_quiescing(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch = private->sch;
+	int ret = VFIO_CCW_STATE_STANDBY;
+	int iretry = 255;
+
+	spin_lock_irq(sch->lock);
+	ret = cio_cancel_halt_clear(sch, &iretry);
+	if (ret == -EBUSY)
+		ret = VFIO_CCW_STATE_QUIESCING;
+	else if (private->completion)
+		complete(private->completion);
+	spin_unlock_irq(sch->lock);
+	return ret;
+}
+static int fsm_quiescing_done(struct vfio_ccw_private *private)
+{
+	if (private->completion)
+		complete(private->completion);
+	return VFIO_CCW_STATE_STANDBY;
+}
 /*
  * No operation action.
  */
@@ -178,15 +225,10 @@ static int fsm_sch_event(struct vfio_ccw_private *private)
 static int fsm_init(struct vfio_ccw_private *private)
 {
 	struct subchannel *sch = private->sch;
-	int ret = VFIO_CCW_STATE_STANDBY;
 
-	spin_lock_irq(sch->lock);
 	sch->isc = VFIO_CCW_ISC;
-	if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
-		ret = VFIO_CCW_STATE_NOT_OPER;
-	spin_unlock_irq(sch->lock);
 
-	return ret;
+	return VFIO_CCW_STATE_STANDBY;
 }
 
 
@@ -196,6 +238,8 @@ static int fsm_init(struct vfio_ccw_private *private)
 fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 	[VFIO_CCW_STATE_NOT_OPER] = {
 		[VFIO_CCW_EVENT_INIT]		= fsm_init,
+		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
+		[VFIO_CCW_EVENT_OFFLINE]	= fsm_nop,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_nop,
 		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_nop,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_nop,
@@ -203,13 +247,17 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
 		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
+		[VFIO_CCW_EVENT_ONLINE]		= fsm_online,
+		[VFIO_CCW_EVENT_OFFLINE]	= fsm_offline,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_error,
-		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
 	[VFIO_CCW_STATE_IDLE] = {
 		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
+		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
+		[VFIO_CCW_EVENT_OFFLINE]	= fsm_offline,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
@@ -217,6 +265,8 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 	},
 	[VFIO_CCW_STATE_BOXED] = {
 		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
+		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
+		[VFIO_CCW_EVENT_OFFLINE]	= fsm_quiescing,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
@@ -224,9 +274,20 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 	},
 	[VFIO_CCW_STATE_BUSY] = {
 		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
+		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
+		[VFIO_CCW_EVENT_OFFLINE]	= fsm_quiescing,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
+	[VFIO_CCW_STATE_QUIESCING] = {
+		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
+		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
+		[VFIO_CCW_EVENT_OFFLINE]	= fsm_nop,
+		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
+		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
+		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_quiescing_done,
+		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
+	},
 };
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index ea8fd64..b202e73 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -21,21 +21,14 @@ static int vfio_ccw_mdev_reset(struct mdev_device *mdev)
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
 	sch = private->sch;
-	/*
-	 * TODO:
-	 * In the cureent stage, some things like "no I/O running" and "no
-	 * interrupt pending" are clear, but we are not sure what other state
-	 * we need to care about.
-	 * There are still a lot more instructions need to be handled. We
-	 * should come back here later.
-	 */
+
 	ret = vfio_ccw_sch_quiesce(sch);
 	if (ret)
 		return ret;
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_ONLINE);
 
-	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
-	if (!ret)
-		private->state = VFIO_CCW_STATE_IDLE;
+	if (!(private->state == VFIO_CCW_STATE_IDLE))
+		ret = -EFAULT;
 
 	return ret;
 }
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index c5455a9..ad59091 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -68,6 +68,7 @@ enum vfio_ccw_state {
 	VFIO_CCW_STATE_IDLE,
 	VFIO_CCW_STATE_BOXED,
 	VFIO_CCW_STATE_BUSY,
+	VFIO_CCW_STATE_QUIESCING,
 	/* last element! */
 	NR_VFIO_CCW_STATES
 };
@@ -81,6 +82,8 @@ enum vfio_ccw_event {
 	VFIO_CCW_EVENT_SSCH_REQ,
 	VFIO_CCW_EVENT_INTERRUPT,
 	VFIO_CCW_EVENT_SCHIB_CHANGED,
+	VFIO_CCW_EVENT_ONLINE,
+	VFIO_CCW_EVENT_OFFLINE,
 	/* last element! */
 	NR_VFIO_CCW_EVENTS
 };
-- 
2.7.4

Re: [PATCH v2 08/10] vfio: ccw: Handling reset and shutdown with states

[Cornelia Huck]

On Fri, 25 May 2018 12:21:16 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Two new events, VFIO_CCW_EVENT_ONLINE and VFIO_CCW_EVENT_OFFLINE
> allow to handle the enabling and disabling of a Sub Channel and
> the init, shutdown, quiesce and reset operations are changed
> accordingly.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     | 44 ++++------------------
>  drivers/s390/cio/vfio_ccw_fsm.c     | 75 +++++++++++++++++++++++++++++++++----
>  drivers/s390/cio/vfio_ccw_ops.c     | 15 ++------
>  drivers/s390/cio/vfio_ccw_private.h |  3 ++
>  4 files changed, 82 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 6fc7668..3e7b514 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -30,41 +30,13 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
>  {
>  	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>  	DECLARE_COMPLETION_ONSTACK(completion);
> -	int iretry, ret = 0;
> -
> -	spin_lock_irq(sch->lock);
> -	if (!sch->schib.pmcw.ena)
> -		goto out_unlock;
> -	ret = cio_disable_subchannel(sch);
> -	if (ret != -EBUSY)
> -		goto out_unlock;
> -
> -	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);
> -
> -			wait_for_completion_timeout(&completion, 3*HZ);
> -
> -			spin_lock_irq(sch->lock);
> -			private->completion = NULL;
> -			flush_workqueue(vfio_ccw_work_q);
> -			ret = cio_cancel_halt_clear(sch, &iretry);
> -		};
> -
> -		ret = cio_disable_subchannel(sch);
> -	} while (ret == -EBUSY);
> -out_unlock:
> -	private->state = VFIO_CCW_STATE_NOT_OPER;
> -	spin_unlock_irq(sch->lock);
> -	return ret;
> +
> +	private->completion = &completion;
> +	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OFFLINE);
> +	wait_for_completion_interruptible_timeout(&completion, jiffies + 3*HZ);
> +	if (private->state != VFIO_CCW_STATE_STANDBY)
> +		return -EFAULT;

-EFAULT really looks like the wrong error here. -EIO?

(I'm not sold on the whole concept here, though. See below.)

> +	return 0;
>  }
>  
>  static void vfio_ccw_sch_io_todo(struct work_struct *work)
> @@ -95,8 +67,6 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
>  	memcpy(&private->irb, irb, sizeof(*irb));
>  
>  	queue_work(vfio_ccw_work_q, &private->io_work);
> -	if (private->completion)
> -		complete(private->completion);
>  }
>  
>  static int vfio_ccw_sch_probe(struct subchannel *sch)
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 20b909c..0acab2f 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -73,6 +73,53 @@ static int fsm_notoper(struct vfio_ccw_private *private)
>  	return VFIO_CCW_STATE_NOT_OPER;
>  }
>  
> +static int fsm_online(struct vfio_ccw_private *private)
> +{
> +	struct subchannel *sch = private->sch;
> +	int ret = VFIO_CCW_STATE_IDLE;
> +
> +	spin_lock_irq(sch->lock);
> +	if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
> +		ret = VFIO_CCW_STATE_NOT_OPER;
> +	spin_unlock_irq(sch->lock);
> +
> +	return ret;
> +}
> +static int fsm_offline(struct vfio_ccw_private *private)
> +{
> +	struct subchannel *sch = private->sch;
> +	int ret = VFIO_CCW_STATE_STANDBY;
> +
> +	spin_lock_irq(sch->lock);
> +	if (cio_disable_subchannel(sch))
> +		ret = VFIO_CCW_STATE_NOT_OPER;

So, what about a subchannel that is busy? Why should it go to the not
oper state?

(And you should try to flush pending I/O and then try again in that
case. Otherwise, you may have a still-enabled subchannel which may
throw an interrupt.)

> +	spin_unlock_irq(sch->lock);
> +	if (private->completion)
> +		complete(private->completion);
> +
> +	return ret;
> +}
> +static int fsm_quiescing(struct vfio_ccw_private *private)
> +{
> +	struct subchannel *sch = private->sch;
> +	int ret = VFIO_CCW_STATE_STANDBY;
> +	int iretry = 255;
> +
> +	spin_lock_irq(sch->lock);
> +	ret = cio_cancel_halt_clear(sch, &iretry);
> +	if (ret == -EBUSY)
> +		ret = VFIO_CCW_STATE_QUIESCING;
> +	else if (private->completion)
> +		complete(private->completion);
> +	spin_unlock_irq(sch->lock);
> +	return ret;

If I read this correctly, you're calling cio_cancel_halt_clear() only
once. What happened to the retry loop?

> +}
> +static int fsm_quiescing_done(struct vfio_ccw_private *private)
> +{
> +	if (private->completion)
> +		complete(private->completion);
> +	return VFIO_CCW_STATE_STANDBY;
> +}
>  /*
>   * No operation action.
>   */
> @@ -178,15 +225,10 @@ static int fsm_sch_event(struct vfio_ccw_private *private)
>  static int fsm_init(struct vfio_ccw_private *private)
>  {
>  	struct subchannel *sch = private->sch;
> -	int ret = VFIO_CCW_STATE_STANDBY;
>  
> -	spin_lock_irq(sch->lock);
>  	sch->isc = VFIO_CCW_ISC;
> -	if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
> -		ret = VFIO_CCW_STATE_NOT_OPER;
> -	spin_unlock_irq(sch->lock);
>  
> -	return ret;
> +	return VFIO_CCW_STATE_STANDBY;

Doesn't that change the semantic of the standby state?

>  }
>  
>  
> @@ -196,6 +238,8 @@ static int fsm_init(struct vfio_ccw_private *private)
>  fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
>  	[VFIO_CCW_STATE_NOT_OPER] = {
>  		[VFIO_CCW_EVENT_INIT]		= fsm_init,
> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_nop,
>  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_nop,
>  		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_nop,
>  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_nop,
> @@ -203,13 +247,17 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
>  	},
>  	[VFIO_CCW_STATE_STANDBY] = {
>  		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_online,
> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_offline,
>  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>  		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_error,
> -		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> +		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
>  		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
>  	},
>  	[VFIO_CCW_STATE_IDLE] = {
>  		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_offline,
>  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>  		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_request,
>  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> @@ -217,6 +265,8 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
>  	},
>  	[VFIO_CCW_STATE_BOXED] = {
>  		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_quiescing,
>  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>  		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
>  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> @@ -224,9 +274,20 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
>  	},
>  	[VFIO_CCW_STATE_BUSY] = {
>  		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_quiescing,
>  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>  		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
>  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
>  		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
>  	},
> +	[VFIO_CCW_STATE_QUIESCING] = {
> +		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_nop,
> +		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> +		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
> +		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_quiescing_done,
> +		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
> +	},

Your idea here seems to be to go to either disabling the subchannel
directly or flushing out I/O first, depending on the state you're in.
The problem is that you may need retries in any case (the subchannel
may be status pending if it is enabled; not necessarily by any I/O that
had been started, but also from an unsolicited notification.)

>  };
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index ea8fd64..b202e73 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -21,21 +21,14 @@ static int vfio_ccw_mdev_reset(struct mdev_device *mdev)
>  
>  	private = dev_get_drvdata(mdev_parent_dev(mdev));
>  	sch = private->sch;
> -	/*
> -	 * TODO:
> -	 * In the cureent stage, some things like "no I/O running" and "no
> -	 * interrupt pending" are clear, but we are not sure what other state
> -	 * we need to care about.
> -	 * There are still a lot more instructions need to be handled. We
> -	 * should come back here later.
> -	 */

This is still true, no? I'm thinking about things like channel monitors
and the like (even if we don't support them yet).

> +
>  	ret = vfio_ccw_sch_quiesce(sch);
>  	if (ret)
>  		return ret;
> +	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_ONLINE);
>  
> -	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
> -	if (!ret)
> -		private->state = VFIO_CCW_STATE_IDLE;
> +	if (!(private->state == VFIO_CCW_STATE_IDLE))
> +		ret = -EFAULT;

The -EFAULT looks wrong here as well.

I'm also not sure whether we should conflate enabling/disabling a
device and doing a reset.

>  
>  	return ret;
>  }
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index c5455a9..ad59091 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -68,6 +68,7 @@ enum vfio_ccw_state {
>  	VFIO_CCW_STATE_IDLE,
>  	VFIO_CCW_STATE_BOXED,
>  	VFIO_CCW_STATE_BUSY,
> +	VFIO_CCW_STATE_QUIESCING,
>  	/* last element! */
>  	NR_VFIO_CCW_STATES
>  };
> @@ -81,6 +82,8 @@ enum vfio_ccw_event {
>  	VFIO_CCW_EVENT_SSCH_REQ,
>  	VFIO_CCW_EVENT_INTERRUPT,
>  	VFIO_CCW_EVENT_SCHIB_CHANGED,
> +	VFIO_CCW_EVENT_ONLINE,
> +	VFIO_CCW_EVENT_OFFLINE,
>  	/* last element! */
>  	NR_VFIO_CCW_EVENTS
>  };

Re: [PATCH v2 08/10] vfio: ccw: Handling reset and shutdown with states

[Pierre Morel]

On 05/06/2018 14:18, Cornelia Huck wrote:
> On Fri, 25 May 2018 12:21:16 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>
>> Two new events, VFIO_CCW_EVENT_ONLINE and VFIO_CCW_EVENT_OFFLINE
>> allow to handle the enabling and disabling of a Sub Channel and
>> the init, shutdown, quiesce and reset operations are changed
>> accordingly.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_drv.c     | 44 ++++------------------
>>   drivers/s390/cio/vfio_ccw_fsm.c     | 75 +++++++++++++++++++++++++++++++++----
>>   drivers/s390/cio/vfio_ccw_ops.c     | 15 ++------
>>   drivers/s390/cio/vfio_ccw_private.h |  3 ++
>>   4 files changed, 82 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>> index 6fc7668..3e7b514 100644
>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>> @@ -30,41 +30,13 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
>>   {
>>   	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>>   	DECLARE_COMPLETION_ONSTACK(completion);

ooops, I have to pay more attention on the completions.
Same error that reported Heiko in patch 10/10.


>> -	int iretry, ret = 0;
>> -
>> -	spin_lock_irq(sch->lock);
>> -	if (!sch->schib.pmcw.ena)
>> -		goto out_unlock;
>> -	ret = cio_disable_subchannel(sch);
>> -	if (ret != -EBUSY)
>> -		goto out_unlock;
>> -
>> -	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);
>> -
>> -			wait_for_completion_timeout(&completion, 3*HZ);
>> -
>> -			spin_lock_irq(sch->lock);
>> -			private->completion = NULL;
>> -			flush_workqueue(vfio_ccw_work_q);
>> -			ret = cio_cancel_halt_clear(sch, &iretry);understood
>> -		};
>> -
>> -		ret = cio_disable_subchannel(sch);
>> -	} while (ret == -EBUSY);
>> -out_unlock:
>> -	private->state = VFIO_CCW_STATE_NOT_OPER;
>> -	spin_unlock_irq(sch->lock);
>> -	return ret;
>> +
>> +	private->completion = &completion;
>> +	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OFFLINE);
>> +	wait_for_completion_interruptible_timeout(&completion, jiffies + 3*HZ);
>> +	if (private->state != VFIO_CCW_STATE_STANDBY)
>> +		return -EFAULT;
> -EFAULT really looks like the wrong error here. -EIO?

OK

>
> (I'm not sold on the whole concept here, though. See below.)
>
>> +	return 0;
>>   }
>>   
>>   static void vfio_ccw_sch_io_todo(struct work_struct *work)
>> @@ -95,8 +67,6 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
>>   	memcpy(&private->irb, irb, sizeof(*irb));
>>   
>>   	queue_work(vfio_ccw_work_q, &private->io_work);
>> -	if (private->completion)
>> -		complete(private->completion);
>>   }
>>   
>>   static int vfio_ccw_sch_probe(struct subchannel *sch)
>> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
>> index 20b909c..0acab2f 100644
>> --- a/drivers/s390/cio/vfio_ccw_fsm.c
>> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
>> @@ -73,6 +73,53 @@ static int fsm_notoper(struct vfio_ccw_private *private)
>>   	return VFIO_CCW_STATE_NOT_OPER;
>>   }
>>   
>> +static int fsm_online(struct vfio_ccw_private *private)
>> +{
>> +	struct subchannel *sch = private->sch;
>> +	int ret = VFIO_CCW_STATE_IDLE;
>> +
>> +	spin_lock_irq(sch->lock);
>> +	if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
>> +		ret = VFIO_CCW_STATE_NOT_OPER;
>> +	spin_unlock_irq(sch->lock);
>> +
>> +	return ret;
>> +}
>> +static int fsm_offline(struct vfio_ccw_private *private)
>> +{
>> +	struct subchannel *sch = private->sch;
>> +	int ret = VFIO_CCW_STATE_STANDBY;
>> +
>> +	spin_lock_irq(sch->lock);
>> +	if (cio_disable_subchannel(sch))
>> +		ret = VFIO_CCW_STATE_NOT_OPER;
> So, what about a subchannel that is busy? Why should it go to the not
> oper state?

right, thanks.

>
> (And you should try to flush pending I/O and then try again in that
> case. Otherwise, you may have a still-enabled subchannel which may
> throw an interrupt.)

What about letting the guest doing this.
After giving him the right information on what happened of course.

>
>> +	spin_unlock_irq(sch->lock);
>> +	if (private->completion)
>> +		complete(private->completion);
>> +
>> +	return ret;
>> +}
>> +static int fsm_quiescing(struct vfio_ccw_private *private)
>> +{
>> +	struct subchannel *sch = private->sch;
>> +	int ret = VFIO_CCW_STATE_STANDBY;
>> +	int iretry = 255;
>> +
>> +	spin_lock_irq(sch->lock);
>> +	ret = cio_cancel_halt_clear(sch, &iretry);
>> +	if (ret == -EBUSY)
>> +		ret = VFIO_CCW_STATE_QUIESCING;
>> +	else if (private->completion)
>> +		complete(private->completion);
>> +	spin_unlock_irq(sch->lock);
>> +	return ret;
> If I read this correctly, you're calling cio_cancel_halt_clear() only
> once. What happened to the retry loop?

Same as above, what about letting the guest doing this?
And there are already 255 retries as part of the interface to cio.

>
>> +}
>> +static int fsm_quiescing_done(struct vfio_ccw_private *private)
>> +{
>> +	if (private->completion)
>> +		complete(private->completion);
>> +	return VFIO_CCW_STATE_STANDBY;
>> +}
>>   /*
>>    * No operation action.
>>    */
>> @@ -178,15 +225,10 @@ static int fsm_sch_event(struct vfio_ccw_private *private)
>>   static int fsm_init(struct vfio_ccw_private *private)
>>   {
>>   	struct subchannel *sch = private->sch;
>> -	int ret = VFIO_CCW_STATE_STANDBY;
>>   
>> -	spin_lock_irq(sch->lock);
>>   	sch->isc = VFIO_CCW_ISC;
>> -	if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
>> -		ret = VFIO_CCW_STATE_NOT_OPER;
>> -	spin_unlock_irq(sch->lock);
>>   
>> -	return ret;
>> +	return VFIO_CCW_STATE_STANDBY;
> Doesn't that change the semantic of the standby state?

It changes the FSM: NOT_OPER and STANDBY are clearly different.
Part of the initialization is now done in when putting the device online.


>
>>   }
>>   
>>   
>> @@ -196,6 +238,8 @@ static int fsm_init(struct vfio_ccw_private *private)
>>   fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
>>   	[VFIO_CCW_STATE_NOT_OPER] = {
>>   		[VFIO_CCW_EVENT_INIT]		= fsm_init,
>> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
>> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_nop,
>>   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_nop,
>>   		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_nop,
>>   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_nop,
>> @@ -203,13 +247,17 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
>>   	},
>>   	[VFIO_CCW_STATE_STANDBY] = {
>>   		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
>> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_online,
>> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_offline,
>>   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>>   		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_error,
>> -		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
>> +		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
>>   		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
>>   	},
>>   	[VFIO_CCW_STATE_IDLE] = {
>>   		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
>> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
>> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_offline,
>>   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>>   		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_request,
>>   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
>> @@ -217,6 +265,8 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
>>   	},
>>   	[VFIO_CCW_STATE_BOXED] = {
>>   		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
>> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
>> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_quiescing,
>>   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>>   		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
>>   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
>> @@ -224,9 +274,20 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
>>   	},
>>   	[VFIO_CCW_STATE_BUSY] = {
>>   		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
>> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
>> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_quiescing,
>>   		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>>   		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
>>   		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
>>   		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
>>   	},
>> +	[VFIO_CCW_STATE_QUIESCING] = {
>> +		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
>> +		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
>> +		[VFIO_CCW_EVENT_OFFLINE]	= fsm_nop,
>> +		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
>> +		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
>> +		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_quiescing_done,
>> +		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
>> +	},
> Your idea here seems to be to go to either disabling the subchannel
> directly or flushing out I/O first, depending on the state you're in.
> The problem is that you may need retries in any case (the subchannel
> may be status pending if it is enabled; not necessarily by any I/O that
> had been started, but also from an unsolicited notification.)

I wanted to let the guest do the retries as he wants to.
Somehow we must give the right response back to the guest
and take care of the error number we give back.

I will get a better look at this.

>
>>   };
>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
>> index ea8fd64..b202e73 100644
>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> @@ -21,21 +21,14 @@ static int vfio_ccw_mdev_reset(struct mdev_device *mdev)
>>   
>>   	private = dev_get_drvdata(mdev_parent_dev(mdev));
>>   	sch = private->sch;
>> -	/*
>> -	 * TODO:
>> -	 * In the cureent stage, some things like "no I/O running" and "no
>> -	 * interrupt pending" are clear, but we are not sure what other state
>> -	 * we need to care about.
>> -	 * There are still a lot more instructions need to be handled. We
>> -	 * should come back here later.
>> -	 */
> This is still true, no? I'm thinking about things like channel monitors
> and the like (even if we don't support them yet).

I think that this is not the place to put this remark since here
we should send an event to the FSM, having new states
will be handled as FSM states.
I put it back, here or where I think it belong if I find another
place after resolving the RESET problem.

>
>> +
>>   	ret = vfio_ccw_sch_quiesce(sch);
>>   	if (ret)
>>   		return ret;
>> +	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_ONLINE);
>>   
>> -	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
>> -	if (!ret)
>> -		private->state = VFIO_CCW_STATE_IDLE;
>> +	if (!(private->state == VFIO_CCW_STATE_IDLE))
>> +		ret = -EFAULT;
> The -EFAULT looks wrong here as well.

yes

>
> I'm also not sure whether we should conflate enabling/disabling a
> device and doing a reset.

I fully agree, just did not change the existing.

>
>>   
>>   	return ret;
>>   }
>> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
>> index c5455a9..ad59091 100644
>> --- a/drivers/s390/cio/vfio_ccw_private.h
>> +++ b/drivers/s390/cio/vfio_ccw_private.h
>> @@ -68,6 +68,7 @@ enum vfio_ccw_state {
>>   	VFIO_CCW_STATE_IDLE,
>>   	VFIO_CCW_STATE_BOXED,
>>   	VFIO_CCW_STATE_BUSY,
>> +	VFIO_CCW_STATE_QUIESCING,
>>   	/* last element! */
>>   	NR_VFIO_CCW_STATES
>>   };
>> @@ -81,6 +82,8 @@ enum vfio_ccw_event {
>>   	VFIO_CCW_EVENT_SSCH_REQ,
>>   	VFIO_CCW_EVENT_INTERRUPT,
>>   	VFIO_CCW_EVENT_SCHIB_CHANGED,
>> +	VFIO_CCW_EVENT_ONLINE,
>> +	VFIO_CCW_EVENT_OFFLINE,
>>   	/* last element! */
>>   	NR_VFIO_CCW_EVENTS
>>   };

Thanks a lot for the review.

I will address all the remarks in the next version.

Thanks,

Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 08/10] vfio: ccw: Handling reset and shutdown with states

[Cornelia Huck]

On Tue, 5 Jun 2018 16:10:52 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 05/06/2018 14:18, Cornelia Huck wrote:
> > On Fri, 25 May 2018 12:21:16 +0200
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> >> +static int fsm_online(struct vfio_ccw_private *private)
> >> +{
> >> +	struct subchannel *sch = private->sch;
> >> +	int ret = VFIO_CCW_STATE_IDLE;
> >> +
> >> +	spin_lock_irq(sch->lock);
> >> +	if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
> >> +		ret = VFIO_CCW_STATE_NOT_OPER;
> >> +	spin_unlock_irq(sch->lock);
> >> +
> >> +	return ret;
> >> +}
> >> +static int fsm_offline(struct vfio_ccw_private *private)
> >> +{
> >> +	struct subchannel *sch = private->sch;
> >> +	int ret = VFIO_CCW_STATE_STANDBY;
> >> +
> >> +	spin_lock_irq(sch->lock);
> >> +	if (cio_disable_subchannel(sch))
> >> +		ret = VFIO_CCW_STATE_NOT_OPER;  
> > So, what about a subchannel that is busy? Why should it go to the not
> > oper state?  
> 
> right, thanks.
> 
> >
> > (And you should try to flush pending I/O and then try again in that
> > case. Otherwise, you may have a still-enabled subchannel which may
> > throw an interrupt.)  
> 
> What about letting the guest doing this.
> After giving him the right information on what happened of course.

Why should the guest know anything about this? Getting the device to a
usable state respectively cleaning up is the responsibility of the host
code. This processing will happen before the guest gets use of the
device or after it has lost use of it already (or it is some internal
handling like reset, which the guest should not be made aware of).

> 
> >  
> >> +	spin_unlock_irq(sch->lock);
> >> +	if (private->completion)
> >> +		complete(private->completion);
> >> +
> >> +	return ret;
> >> +}
> >> +static int fsm_quiescing(struct vfio_ccw_private *private)
> >> +{
> >> +	struct subchannel *sch = private->sch;
> >> +	int ret = VFIO_CCW_STATE_STANDBY;
> >> +	int iretry = 255;
> >> +
> >> +	spin_lock_irq(sch->lock);
> >> +	ret = cio_cancel_halt_clear(sch, &iretry);
> >> +	if (ret == -EBUSY)
> >> +		ret = VFIO_CCW_STATE_QUIESCING;
> >> +	else if (private->completion)
> >> +		complete(private->completion);
> >> +	spin_unlock_irq(sch->lock);
> >> +	return ret;  
> > If I read this correctly, you're calling cio_cancel_halt_clear() only
> > once. What happened to the retry loop?  
> 
> Same as above, what about letting the guest doing this?

See my reply above.

> And there are already 255 retries as part of the interface to cio.

>From the kerneldoc comment for cio_cancel_halt_clear():

 * This should be called repeatedly since halt/clear are asynchronous           
 * operations. We do one try with cio_cancel, three tries with cio_halt,        
 * 255 tries with cio_clear. The caller should initialize @iretry with          
 * the value 255 for its first call to this, and keep using the same            
 * @iretry in the subsequent calls until it gets a non -EBUSY return. 

> 
> >  
> >> +}
> >> +static int fsm_quiescing_done(struct vfio_ccw_private *private)
> >> +{
> >> +	if (private->completion)
> >> +		complete(private->completion);
> >> +	return VFIO_CCW_STATE_STANDBY;
> >> +}
> >>   /*
> >>    * No operation action.
> >>    */
> >> @@ -178,15 +225,10 @@ static int fsm_sch_event(struct vfio_ccw_private *private)
> >>   static int fsm_init(struct vfio_ccw_private *private)
> >>   {
> >>   	struct subchannel *sch = private->sch;
> >> -	int ret = VFIO_CCW_STATE_STANDBY;
> >>   
> >> -	spin_lock_irq(sch->lock);
> >>   	sch->isc = VFIO_CCW_ISC;
> >> -	if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
> >> -		ret = VFIO_CCW_STATE_NOT_OPER;
> >> -	spin_unlock_irq(sch->lock);
> >>   
> >> -	return ret;
> >> +	return VFIO_CCW_STATE_STANDBY;  
> > Doesn't that change the semantic of the standby state?  
> 
> It changes the FSM: NOT_OPER and STANDBY are clearly different.
> Part of the initialization is now done in when putting the device online.

Hm, I think the changes to the fsm semantics are a bit mixed up between
patches. I'll wait for an outline of how this is supposed to look in
the end before commenting further :)

> > Your idea here seems to be to go to either disabling the subchannel
> > directly or flushing out I/O first, depending on the state you're in.
> > The problem is that you may need retries in any case (the subchannel
> > may be status pending if it is enabled; not necessarily by any I/O that
> > had been started, but also from an unsolicited notification.)  
> 
> I wanted to let the guest do the retries as he wants to.
> Somehow we must give the right response back to the guest
> and take care of the error number we give back.

As described above, we need to be clear on what should be guest-visible
and what is just internal handling e.g. during initialization/removal.

> 
> I will get a better look at this.
> 
> >  
> >>   };
> >> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> >> index ea8fd64..b202e73 100644
> >> --- a/drivers/s390/cio/vfio_ccw_ops.c
> >> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> >> @@ -21,21 +21,14 @@ static int vfio_ccw_mdev_reset(struct mdev_device *mdev)
> >>   
> >>   	private = dev_get_drvdata(mdev_parent_dev(mdev));
> >>   	sch = private->sch;
> >> -	/*
> >> -	 * TODO:
> >> -	 * In the cureent stage, some things like "no I/O running" and "no
> >> -	 * interrupt pending" are clear, but we are not sure what other state
> >> -	 * we need to care about.
> >> -	 * There are still a lot more instructions need to be handled. We
> >> -	 * should come back here later.
> >> -	 */  
> > This is still true, no? I'm thinking about things like channel monitors
> > and the like (even if we don't support them yet).  
> 
> I think that this is not the place to put this remark since here
> we should send an event to the FSM, having new states
> will be handled as FSM states.
> I put it back, here or where I think it belong if I find another
> place after resolving the RESET problem.

The comment basically refers to "we aren't quite sure whether there is
more stuff we need to reset", so I think this is indeed the correct
place.

Re: [PATCH v2 08/10] vfio: ccw: Handling reset and shutdown with states

[Pierre Morel]

On 05/06/2018 17:27, Cornelia Huck wrote:
> On Tue, 5 Jun 2018 16:10:52 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> On 05/06/2018 14:18, Cornelia Huck wrote:
>>> On Fri, 25 May 2018 12:21:16 +0200
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>> +static int fsm_online(struct vfio_ccw_private *private)
>>>> +{
>>>> +	struct subchannel *sch = private->sch;
>>>> +	int ret = VFIO_CCW_STATE_IDLE;
>>>> +
>>>> +	spin_lock_irq(sch->lock);
>>>> +	if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
>>>> +		ret = VFIO_CCW_STATE_NOT_OPER;
>>>> +	spin_unlock_irq(sch->lock);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +static int fsm_offline(struct vfio_ccw_private *private)
>>>> +{
>>>> +	struct subchannel *sch = private->sch;
>>>> +	int ret = VFIO_CCW_STATE_STANDBY;
>>>> +
>>>> +	spin_lock_irq(sch->lock);
>>>> +	if (cio_disable_subchannel(sch))
>>>> +		ret = VFIO_CCW_STATE_NOT_OPER;
>>> So, what about a subchannel that is busy? Why should it go to the not
>>> oper state?
>> right, thanks.
>>
>>> (And you should try to flush pending I/O and then try again in that
>>> case. Otherwise, you may have a still-enabled subchannel which may
>>> throw an interrupt.)
>> What about letting the guest doing this.
>> After giving him the right information on what happened of course.
> Why should the guest know anything about this? Getting the device to a
> usable state respectively cleaning up is the responsibility of the host
> code. This processing will happen before the guest gets use of the
> device or after it has lost use of it already (or it is some internal
> handling like reset, which the guest should not be made aware of).

Hum, not inspired today,
sorry I should have take a day to recover from holidays. :)

>
>>>   
>>>> +	spin_unlock_irq(sch->lock);
>>>> +	if (private->completion)
>>>> +		complete(private->completion);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +static int fsm_quiescing(struct vfio_ccw_private *private)
>>>> +{
>>>> +	struct subchannel *sch = private->sch;
>>>> +	int ret = VFIO_CCW_STATE_STANDBY;
>>>> +	int iretry = 255;
>>>> +
>>>> +	spin_lock_irq(sch->lock);
>>>> +	ret = cio_cancel_halt_clear(sch, &iretry);
>>>> +	if (ret == -EBUSY)
>>>> +		ret = VFIO_CCW_STATE_QUIESCING;
>>>> +	else if (private->completion)
>>>> +		complete(private->completion);
>>>> +	spin_unlock_irq(sch->lock);
>>>> +	return ret;
>>> If I read this correctly, you're calling cio_cancel_halt_clear() only
>>> once. What happened to the retry loop?
>> Same as above, what about letting the guest doing this?
> See my reply above.
>
>> And there are already 255 retries as part of the interface to cio.
>  From the kerneldoc comment for cio_cancel_halt_clear():
>
>   * This should be called repeatedly since halt/clear are asynchronous
>   * operations. We do one try with cio_cancel, three tries with cio_halt,
>   * 255 tries with cio_clear. The caller should initialize @iretry with
>   * the value 255 for its first call to this, and keep using the same
>   * @iretry in the subsequent calls until it gets a non -EBUSY return.

OK thanks, I do so.

>
>>>   
>>>> +}
>>>> +static int fsm_quiescing_done(struct vfio_ccw_private *private)
>>>> +{
>>>> +	if (private->completion)
>>>> +		complete(private->completion);
>>>> +	return VFIO_CCW_STATE_STANDBY;
>>>> +}
>>>>    /*
>>>>     * No operation action.
>>>>     */
>>>> @@ -178,15 +225,10 @@ static int fsm_sch_event(struct vfio_ccw_private *private)
>>>>    static int fsm_init(struct vfio_ccw_private *private)
>>>>    {
>>>>    	struct subchannel *sch = private->sch;
>>>> -	int ret = VFIO_CCW_STATE_STANDBY;
>>>>    
>>>> -	spin_lock_irq(sch->lock);
>>>>    	sch->isc = VFIO_CCW_ISC;
>>>> -	if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
>>>> -		ret = VFIO_CCW_STATE_NOT_OPER;
>>>> -	spin_unlock_irq(sch->lock);
>>>>    
>>>> -	return ret;
>>>> +	return VFIO_CCW_STATE_STANDBY;
>>> Doesn't that change the semantic of the standby state?
>> It changes the FSM: NOT_OPER and STANDBY are clearly different.
>> Part of the initialization is now done in when putting the device online.
> Hm, I think the changes to the fsm semantics are a bit mixed up between
> patches. I'll wait for an outline of how this is supposed to look in
> the end before commenting further :)

Yes, I do this in the next cover letter.

>
>>> Your idea here seems to be to go to either disabling the subchannel
>>> directly or flushing out I/O first, depending on the state you're in.
>>> The problem is that you may need retries in any case (the subchannel
>>> may be status pending if it is enabled; not necessarily by any I/O that
>>> had been started, but also from an unsolicited notification.)
>> I wanted to let the guest do the retries as he wants to.
>> Somehow we must give the right response back to the guest
>> and take care of the error number we give back.
> As described above, we need to be clear on what should be guest-visible
> and what is just internal handling e.g. during initialization/removal.

Yes.

>
>> I will get a better look at this.
>>
>>>   
>>>>    };
>>>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
>>>> index ea8fd64..b202e73 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>>>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>>>> @@ -21,21 +21,14 @@ static int vfio_ccw_mdev_reset(struct mdev_device *mdev)
>>>>    
>>>>    	private = dev_get_drvdata(mdev_parent_dev(mdev));
>>>>    	sch = private->sch;
>>>> -	/*
>>>> -	 * TODO:
>>>> -	 * In the cureent stage, some things like "no I/O running" and "no
>>>> -	 * interrupt pending" are clear, but we are not sure what other state
>>>> -	 * we need to care about.
>>>> -	 * There are still a lot more instructions need to be handled. We
>>>> -	 * should come back here later.
>>>> -	 */
>>> This is still true, no? I'm thinking about things like channel monitors
>>> and the like (even if we don't support them yet).
>> I think that this is not the place to put this remark since here
>> we should send an event to the FSM, having new states
>> will be handled as FSM states.
>> I put it back, here or where I think it belong if I find another
>> place after resolving the RESET problem.
> The comment basically refers to "we aren't quite sure whether there is
> more stuff we need to reset", so I think this is indeed the correct
> place.

OK

>

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

[PATCH v2 09/10] vfio: ccw: Suppressing the BOXED state

[Pierre Morel]

VFIO_CCW_STATE_BOXED and VFIO_CCW_STATE_BUSY are the same
states.
Let's only keep one: VFIO_CCW_STATE_BUSY

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 drivers/s390/cio/vfio_ccw_fsm.c     | 11 -----------
 drivers/s390/cio/vfio_ccw_private.h |  1 -
 2 files changed, 12 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 0acab2f..c37052d 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -162,8 +162,6 @@ static int fsm_io_request(struct vfio_ccw_private *private)
 	struct ccw_io_region *io_region = &private->io_region;
 	struct mdev_device *mdev = private->mdev;
 
-	private->state = VFIO_CCW_STATE_BOXED;
-
 	orb = (union orb *)io_region->orb_area;
 
 	io_region->ret_code = cp_init(&private->cp, mdev_dev(mdev), orb);
@@ -263,15 +261,6 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
 	},
-	[VFIO_CCW_STATE_BOXED] = {
-		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
-		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
-		[VFIO_CCW_EVENT_OFFLINE]	= fsm_quiescing,
-		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
-		[VFIO_CCW_EVENT_SSCH_REQ]	= fsm_io_busy,
-		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
-		[VFIO_CCW_EVENT_SCHIB_CHANGED]	= fsm_sch_event,
-	},
 	[VFIO_CCW_STATE_BUSY] = {
 		[VFIO_CCW_EVENT_INIT]		= fsm_nop,
 		[VFIO_CCW_EVENT_ONLINE]		= fsm_nop,
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index ad59091..ab5e37fa 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -66,7 +66,6 @@ enum vfio_ccw_state {
 	VFIO_CCW_STATE_NOT_OPER,
 	VFIO_CCW_STATE_STANDBY,
 	VFIO_CCW_STATE_IDLE,
-	VFIO_CCW_STATE_BOXED,
 	VFIO_CCW_STATE_BUSY,
 	VFIO_CCW_STATE_QUIESCING,
 	/* last element! */
-- 
2.7.4

[PATCH v2 10/10] vfio: ccw: Let user wait when busy on IO

[Pierre Morel]

In the current implementation, we do not want to start a new SSCH
command before the last one ends.

Currently the user needs to poll on the -EBUSY error to
wait before sending a new request.

Let's be friendly with global warming and let the user sleep
until he may send a new request.

Let's make the caller wait until the last SSCH ends.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 drivers/s390/cio/vfio_ccw_fsm.c     | 4 ++++
 drivers/s390/cio/vfio_ccw_ops.c     | 6 ++++++
 drivers/s390/cio/vfio_ccw_private.h | 1 +
 3 files changed, 11 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index c37052d..97b74a1 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -200,6 +200,10 @@ static int fsm_irq(struct vfio_ccw_private *private)
 
 	if (private->io_trigger)
 		eventfd_signal(private->io_trigger, 1);
+
+	if (private->io_completion)
+		complete(private->io_completion);
+
 	return VFIO_CCW_STATE_IDLE;
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index b202e73..39beb6e 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -183,6 +183,7 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
 	struct vfio_ccw_private *private;
 	struct ccw_io_region *region;
 	union scsw *scsw;
+	DECLARE_COMPLETION_ONSTACK(completion);
 
 	if (*ppos + count > sizeof(*region))
 		return -EINVAL;
@@ -196,6 +197,11 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
 	scsw = (union scsw *) &region->scsw_area;
 	switch (scsw->cmd.fctl) {
 	case SCSW_FCTL_START_FUNC:
+		if (private->state == VFIO_CCW_STATE_BUSY) {
+			private->io_completion = &completion;
+			if (wait_for_completion_interruptible(&completion))
+				return -EINTR;
+		}
 		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_SSCH_REQ);
 		break;
 	default:
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index ab5e37fa..e2e4f6d 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -39,6 +39,7 @@ struct vfio_ccw_private {
 	struct subchannel	*sch;
 	int			state;
 	struct completion	*completion;
+	struct completion	*io_completion;
 	atomic_t		avail;
 	struct mdev_device	*mdev;
 	struct notifier_block	nb;
-- 
2.7.4

Re: [PATCH v2 10/10] vfio: ccw: Let user wait when busy on IO

[Heiko Carstens]

On Fri, May 25, 2018 at 12:21:18PM +0200, Pierre Morel wrote:
> In the current implementation, we do not want to start a new SSCH
> command before the last one ends.
> 
> Currently the user needs to poll on the -EBUSY error to
> wait before sending a new request.
> 
> Let's be friendly with global warming and let the user sleep
> until he may send a new request.
> 
> Let's make the caller wait until the last SSCH ends.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_fsm.c     | 4 ++++
>  drivers/s390/cio/vfio_ccw_ops.c     | 6 ++++++
>  drivers/s390/cio/vfio_ccw_private.h | 1 +
>  3 files changed, 11 insertions(+)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index c37052d..97b74a1 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -200,6 +200,10 @@ static int fsm_irq(struct vfio_ccw_private *private)
>  
>  	if (private->io_trigger)
>  		eventfd_signal(private->io_trigger, 1);
> +
> +	if (private->io_completion)
> +		complete(private->io_completion);
> +
>  	return VFIO_CCW_STATE_IDLE;
>  }
>  
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index b202e73..39beb6e 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -183,6 +183,7 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
>  	struct vfio_ccw_private *private;
>  	struct ccw_io_region *region;
>  	union scsw *scsw;
> +	DECLARE_COMPLETION_ONSTACK(completion);
>  
>  	if (*ppos + count > sizeof(*region))
>  		return -EINVAL;
> @@ -196,6 +197,11 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
>  	scsw = (union scsw *) &region->scsw_area;
>  	switch (scsw->cmd.fctl) {
>  	case SCSW_FCTL_START_FUNC:
> +		if (private->state == VFIO_CCW_STATE_BUSY) {
> +			private->io_completion = &completion;
> +			if (wait_for_completion_interruptible(&completion))
> +				return -EINTR;
> +		}

What prevents a state change between checking the state and before
private->io_completion is set? If that happens you would end with an
endless wait.

Similarly, you would have memory corruption if the task would be
interrupted and if the function would be left, ending up with a stale
private->io_completion completion pointer.
The complete(private->io_completion) call will then write to a memory
location that might already be reused.

Just my 0.02 after having a very very short look ;)

Re: [PATCH v2 10/10] vfio: ccw: Let user wait when busy on IO

[Pierre Morel]

On 25/05/2018 16:04, Heiko Carstens wrote:
> On Fri, May 25, 2018 at 12:21:18PM +0200, Pierre Morel wrote:
>> In the current implementation, we do not want to start a new SSCH
>> command before the last one ends.
>>
>> Currently the user needs to poll on the -EBUSY error to
>> wait before sending a new request.
>>
>> Let's be friendly with global warming and let the user sleep
>> until he may send a new request.
>>
>> Let's make the caller wait until the last SSCH ends.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_fsm.c     | 4 ++++
>>   drivers/s390/cio/vfio_ccw_ops.c     | 6 ++++++
>>   drivers/s390/cio/vfio_ccw_private.h | 1 +
>>   3 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
>> index c37052d..97b74a1 100644
>> --- a/drivers/s390/cio/vfio_ccw_fsm.c
>> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
>> @@ -200,6 +200,10 @@ static int fsm_irq(struct vfio_ccw_private *private)
>>   
>>   	if (private->io_trigger)
>>   		eventfd_signal(private->io_trigger, 1);
>> +
>> +	if (private->io_completion)
>> +		complete(private->io_completion);
>> +
>>   	return VFIO_CCW_STATE_IDLE;
>>   }
>>   
>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
>> index b202e73..39beb6e 100644
>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> @@ -183,6 +183,7 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
>>   	struct vfio_ccw_private *private;
>>   	struct ccw_io_region *region;
>>   	union scsw *scsw;
>> +	DECLARE_COMPLETION_ONSTACK(completion);
>>   
>>   	if (*ppos + count > sizeof(*region))
>>   		return -EINVAL;
>> @@ -196,6 +197,11 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
>>   	scsw = (union scsw *) &region->scsw_area;
>>   	switch (scsw->cmd.fctl) {
>>   	case SCSW_FCTL_START_FUNC:
>> +		if (private->state == VFIO_CCW_STATE_BUSY) {
>> +			private->io_completion = &completion;
>> +			if (wait_for_completion_interruptible(&completion))
>> +				return -EINTR;
>> +		}
> What prevents a state change between checking the state and before
> private->io_completion is set? If that happens you would end with an
> endless wait.
>
> Similarly, you would have memory corruption if the task would be
> interrupted and if the function would be left, ending up with a stale
> private->io_completion completion pointer.
> The complete(private->io_completion) call will then write to a memory
> location that might already be reused.
>
> Just my 0.02 after having a very very short look ;)

Right, completely false, I should pay a little more (at least) attention.

Thanks to have had a very very short (but sharp)look.

Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany