[PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine

[PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine

[Pierre Morel]

The goal of the patch serie is to secure the state machine by:
- centralizing all state changes inside the state machine wrapper
- make the state change atomic using mutexes
- refactor the initialization to avoid using a subchannel without a guest


This series introduces new states and events and suppressed
others.
Here the list of states and events used in this serie:
- 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(N): 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_IO_REQ          : Starting a SSCH request (UAPI)
- VFIO_CCW_EVENT_INTERRUPT(N)    : Receiving an interrupt (callback)
- VFIO_CCW_EVENT_SCHIB_CHANGED(N): Receiving a channel event (callback)

The user's ABI do not change.



Pierre Morel (8):
  vfio: ccw: Moving state change out of IRQ context
  vfio: ccw: Transform FSM functions to return state
  vfio: ccw: new VFIO_CCW_EVENT_SCHIB_CHANGED event
  vfio: ccw: Only accept SSCH as an IO request
  vfio: ccw: Suppress unused event parameter
  vfio: ccw: Make FSM functions atomic
  vfio: ccw: Introduce the INIT event
  vfio: ccw: Suppressing the BOXED state

 drivers/s390/cio/vfio_ccw_drv.c     |  71 ++++++-----------
 drivers/s390/cio/vfio_ccw_fsm.c     | 147 +++++++++++++++++++++---------------
 drivers/s390/cio/vfio_ccw_ops.c     |  41 +++++-----
 drivers/s390/cio/vfio_ccw_private.h |  12 ++-
 4 files changed, 137 insertions(+), 134 deletions(-)

-- 
2.7.4

Changelog:

>From v2 to v3:
- Concentrate more on securing the FSM:
 - abandonned ONLINE/OFFLINE events
 - abandonned make user wait on busy
 - abandonned switch case on command to keep old
   way of testing the START_SSCH bit in write syscall

>From v1 to v2:
- 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 v3 1/8] vfio: ccw: Moving state change out of IRQ context

[Pierre Morel]

From: Pierre Morel <pmorel@linux.vnet.ibm.com>

Having state changes out of IRQ context allows to protect
critical sections with mutexes.
Next patches in the serie will use this possibility.

We use work queues to thread the interrupts.

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

[PATCH v3 2/8] 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.

Doing this change will allow to move the state changes out
of the action functions to a common point.
This is important to guaranty the coherency of the FSM
and add to readibility.

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

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 57ae1ab..095a2d9 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;
 }
 
 /*
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 24e1e04..1a0585f 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;
@@ -151,7 +155,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 			cp_free(&private->cp);
 			goto err_out;
 		}
-		return;
+		return private->state;
 	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
 		/* XXX: Handle halt. */
 		io_region->ret_code = -EOPNOTSUPP;
@@ -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,8 @@ static void fsm_irq(struct vfio_ccw_private *private,
 
 	if (private->io_trigger)
 		eventfd_signal(private->io_trigger, 1);
+
+	return VFIO_CCW_STATE_IDLE;
 }
 
 /*
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 v3 3/8] vfio: ccw: new VFIO_CCW_EVENT_SCHIB_CHANGED event

[Pierre Morel]

From: Pierre Morel <pmorel@linux.vnet.ibm.com>

The Sub channel event callback is threaded using workqueues.
The work uses the FSM introducing the VFIO_CCW_EVENT_SCHIB_CHANGED
event.
The update of the SCHIB is now done inside the FSM function.

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 095a2d9..e5a516a 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -75,6 +75,14 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
 }
 
+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
  */
@@ -122,6 +130,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;
 
@@ -168,28 +177,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 1a0585f..94bf151 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -191,6 +191,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] = {
@@ -198,25 +216,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 v3 4/8] vfio: ccw: Only accept SSCH as an IO request

[Pierre Morel]

This patch simplifies the IO request handling to handle the only
implemented request: SSCH.
Other requests are invalid and get a return value of -ENOTSUP
as before.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_fsm.c | 55 ++++++++++++++++-------------------------
 drivers/s390/cio/vfio_ccw_ops.c | 15 ++++++-----
 2 files changed, 28 insertions(+), 42 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 94bf151..e672d14 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -122,50 +122,37 @@ 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 private->state;
-	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
-		/* XXX: Handle halt. */
+	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;
-	} else if (scsw->cmd.fctl & SCSW_FCTL_CLEAR_FUNC) {
-		/* XXX: Handle clear. */
-		io_region->ret_code = -EOPNOTSUPP;
+	}
+	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 private->state;
+
 err_out:
 	return VFIO_CCW_STATE_IDLE;
 }
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 41eeb57..af05932 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -188,25 +188,24 @@ 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;
+	if (scsw->cmd.fctl & SCSW_FCTL_START_FUNC)
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ);
+	else
+		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)
-- 
2.7.4

[PATCH v3 5/8] 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.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 e672d14..2d55742 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -65,8 +65,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;
 
@@ -81,29 +80,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;
 
@@ -118,8 +113,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;
@@ -160,8 +154,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;
 
@@ -180,8 +173,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 a2be0c9..521ba0b 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 v3 6/8] 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.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 1 +
 drivers/s390/cio/vfio_ccw_private.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index e5a516a..98951d5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -116,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 521ba0b..e3dc12c 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

[PATCH v3 7/8] vfio: ccw: Introduce the INIT event

[Pierre Morel]

This patch introduce a new event: VFIO_CCW_EVENT_INIT.

This event is sent to the FSM during the open of the mediated
device, when a guest is about to take care of the real device.

At this moment the FSM state is VFIO_CCW_STATE_NOT_OPER,
nothing can be done with the device as no one is there
to take care of it.
The VFIO_CCW_STATE_NOT_OPER is the only state accepting
the VFIO_CCW_EVENT_INIT event.

In the FSM, when receiving the VFIO_CCW_EVENT_INIT event
the sub-channel is enabled and the next state is set to
VFIO_CCW_STATE_STANDBY.

This patch move part of the initialization to different
places:
- In the probe callback,  all driver private structures
  are initialized before registering the mediated device.
- The call to the FSM is done in the mediated device open
  callback.
- The enabling of the real device is moved inside the FSM
  fsm_init function called through the jump table during
  the mediated device open callback.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 21 ++++++---------------
 drivers/s390/cio/vfio_ccw_fsm.c     | 20 ++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_ops.c     | 26 +++++++++++++-------------
 drivers/s390/cio/vfio_ccw_private.h |  1 +
 4 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 98951d5..e1a9806 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -114,31 +114,22 @@ 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 2d55742..5f1035c 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"
@@ -187,35 +188,54 @@ 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_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_INIT]		= fsm_nop,
 		[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_INIT]		= fsm_nop,
 		[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_INIT]		= fsm_nop,
 		[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_INIT]		= fsm_nop,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_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 af05932..e020a836 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,
-				      &events, &private->nb);
+	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)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index e3dc12c..f9e7570 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_IO_REQ,
 	VFIO_CCW_EVENT_INTERRUPT,
-- 
2.7.4

[PATCH v3 8/8] vfio: ccw: Suppressing the BOXED state

[Pierre Morel]

VFIO_CCW_STATE_BOXED and VFIO_CCW_STATE_BUSY are the same.

Let's only keep one: VFIO_CCW_STATE_BUSY

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

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 5f1035c..e588c3c 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -120,7 +120,7 @@ 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;
+	private->state = VFIO_CCW_STATE_BUSY;
 
 	orb = (union orb *)io_region->orb_area;
 
@@ -227,13 +227,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_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_INIT]		= fsm_nop,
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index f9e7570..42cc3b7 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,
 	/* last element! */
 	NR_VFIO_CCW_STATES
-- 
2.7.4

Re: [PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine

[Cornelia Huck]

On Tue, 12 Jun 2018 09:56:42 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> The goal of the patch serie is to secure the state machine by:
> - centralizing all state changes inside the state machine wrapper
> - make the state change atomic using mutexes
> - refactor the initialization to avoid using a subchannel without a guest
> 
> 
> This series introduces new states and events and suppressed
> others.
> Here the list of states and events used in this serie:

I've tried to come up with some annotations (without looking at the
code :); please correct me if I'm wrong. I want to understand the
design better.

> - VFIO_CCW_STATE_NOT_OPER    : when the Sub-Channel is KO

These are cases of "not operational, we can't even talk to it anymore",
right? If so, the only way out is unregistration of the subchannel,
unless we want to keep it around in case it revives and we get a machine
check. So this is likely a transient state?

> - VFIO_CCW_STATE_STANDBY     : when it is offline

Bound to the driver, but no mdev (and subchannel disabled). The target
states that could happen are either not oper (device gone; we notice
via machine check or when we try to enable the subchannel) or idle (we
enable the subchannel and make it ready for use). Non-transient state
(will stay there until something happens).

> - VFIO_CCW_STATE_IDLE        : when it is ready for I/O

Can go to not oper (device gone), standby (via quiescing?) or busy
(guest does I/O). Non-transient state.

> - VFIO_CCW_STATE_BUSY        : when it is busy doing I/O

Can go to idle (all done), or to not oper, I guess. Transient state.

> - VFIO_CCW_STATE_QUIESCING(N): when it is busy going offline

We're doing cancel/halt/clear. Target is standby, but can go to not
oper if device gone. Transient state.

The boxed state you're removing might have served as a non-transient
equivalent to busy (device does not respond, needs a kick to get out of
the state), but we can re-introduce it if we find it useful in the
future.

> 
> - VFIO_CCW_EVENT_INIT            : the channel setup (admin)

By "admin" you mean "action triggered by admin", right?

> - VFIO_CCW_EVENT_NOT_OPER        : something really wrong happened

That's device gone resp. not operational. Anything else?

I suppose that can only trigger as a reaction to some hardware
reconfiguration or malfunction.

> - VFIO_CCW_EVENT_IO_REQ          : Starting a SSCH request (UAPI)

Triggered by guest action.

> - VFIO_CCW_EVENT_INTERRUPT(N)    : Receiving an interrupt (callback)

Triggered by hardware.

> - VFIO_CCW_EVENT_SCHIB_CHANGED(N): Receiving a channel event (callback)

Also triggered by hardware? Can this also trigger a not oper event?

> 
> The user's ABI do not change.
> 
> 
> 
> Pierre Morel (8):
>   vfio: ccw: Moving state change out of IRQ context
>   vfio: ccw: Transform FSM functions to return state
>   vfio: ccw: new VFIO_CCW_EVENT_SCHIB_CHANGED event
>   vfio: ccw: Only accept SSCH as an IO request
>   vfio: ccw: Suppress unused event parameter
>   vfio: ccw: Make FSM functions atomic
>   vfio: ccw: Introduce the INIT event
>   vfio: ccw: Suppressing the BOXED state
> 
>  drivers/s390/cio/vfio_ccw_drv.c     |  71 ++++++-----------
>  drivers/s390/cio/vfio_ccw_fsm.c     | 147 +++++++++++++++++++++---------------
>  drivers/s390/cio/vfio_ccw_ops.c     |  41 +++++-----
>  drivers/s390/cio/vfio_ccw_private.h |  12 ++-
>  4 files changed, 137 insertions(+), 134 deletions(-)
> 

Re: [PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine

[Cornelia Huck]

On Thu, 14 Jun 2018 10:06:31 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> I tried to make a better description to add later in documentation
> or in the next cover-letter.
> 
> Note that in the current patch series I did not implement online/offline
> events but just kept the previous state changes.
> Not sure if it was a good idea, the goal was to be as simple as possible
> for this iteration.
> If you think it is worth to continue in this direction I will re-introduce
> these as events.

I'm still trying to figure out what we want from the state machine.
I've tried sketching your fsm proposal as outlined by you below for
myself and I have some remarks :)

> 
> ======================
> 
> 1) In CCW VFIO, Finite State Machine is used to clarify the VFIO CCW driver
> actions to be take depending on the events occurring in a device.
> 
> To protect the state transitions, a mutex protect the action
> started when an event occurs in a specific state.
> 
> The actions must never sleep or keep the mutex a long timer.
> 
> To be able to take the mutex when hardware events occur we start
> a work on a dedicated workqueue, posting the event from inside the
> workqueue.
> 
> The state machine has very few states describing the device driver life.
> 
> ------------------------------------------------------------
> | NOT_OPERATIONAL  | No guest is associated with the device|

I don't think that this is the right description. It is either the
initial state, or something has happened that rendered the device
inaccessible.

Also, we need to be careful with the virtual machine vs. guest
terminology. The only thing that has an impact from the guest is when
it does I/O and when an interrupt is generated as a result (i.e., the
IDLE/BUSY transitions). The other transitions are triggered by virtual
machine setup.

> ------------------------------------------------------------
> | STANDBY          | A guest is associated but not started |

This is basically "device is bound to driver, but no mdev has been set
up". In my understanding, we need to get the device to IDLE state
before the vm can present it to the guest (be it before the machine is
up or during hotplug).

> ------------------------------------------------------------
> | IDLE             | Guest started device ready            |

Agree with "device ready", disagree with "guest started" (see above).
"Device ready, accepting I/O requests"?

> ------------------------------------------------------------
> | BUSY             | The device is busy doing IO           |
> ------------------------------------------------------------
> | QUIESCING        | The driver is releasing the device    |

Now you are talking about the driver :) This should probably be done
for the other states as well.

Isn't that state for waiting for outstanding I/O to be terminated
before the mdev is destroyed? IOW, the device may stay bound to the
driver afterwards?

> ------------------------------------------------------------
> 
> 
> 2) The following Events may apply to the state machine:
> 
> If the event is not described, it means it has no influence
> on the state and that no action is required.
> 
> 2.1) FSM in state NOT_OPERATIONAL
> ------------------------------------------------------------
> | INIT             | a guest will drive the SC             |

Better to write out "subchannel", I could not figure out the
abbreviation immediately.

Also, doesn't the event really mean "we're initializing the device"?
The ultimate intention is of course for a guest to use the device, but
the immediate intention is just "get the device through the first
initialization steps".

> | |                                       |
> |                  | triggered by: mdev_open               |
> |                  | action: initialize driver structures  |
> |                  | action: register IOMMU notifier       |
> |                  | state on success: STANDBY             |
> |                  | state on error  : NOT_OPERATIONAL     |
> ------------------------------------------------------------
> 
> 2.2) FSM in state STANDBY
> 
> ------------------------------------------------------------
> | ONLINE           | The host wants the SC online          |

Isn't that rather "an mdev is created"?

> | |                                       |
> |                  | triggered by: vfio_reset              |
> |                  | action: enable SC                     |
> |                  | state on success: IDLE                |
> |                  | state on error  : STANDBY             |

What happens if the subchannel is not operational when we try to get it
ready? Can it go to NOT_OPERATIONAL in that case?

> ------------------------------------------------------------
> 
> Some operations may be intercepted by the state machine but
> will not induce a state change:
> OFFLINE: re-issue the disabling of the SC

Should that even be possible? If we're still busy tearing it down,
shouldn't we be in QUIESCING state?

> IRQ    : re-issue the disabling of the SC

Either we should have cleared the subchannel out during QUIESCING, or
we got an unsolicited interrupt. The latter can be avoided if we make
sure that the device is disabled when we go to STANDBY.

> 
> 2.3) FSM in state IDLE
> 
> ------------------------------------------------------------
> | SSCH             | The guest issue the ssch instruction  |
> | |                                       |
> |                  | triggered by: vfio_write SSCH=1       |
> |                  | action: start an IO request           |
> |                  | state on success: BUSY                |
> |                  | state on error  : IDLE                |

Should there be any way to drop to NOT_OPERATIONAL as well? We'll
probably get a notification from the common I/O layer if that happens,
though.

> ------------------------------------------------------------
> 2.4) FSM in states IDLE or BUSY
> 
> ------------------------------------------------------------
> | IRQ              | The hardware issue an interrupt       |

I think we can get this in QUIESCING as well, and it is an indication
that QUIESCING is done (for final states).

If the subchannel is enabled while in STANDBY, we could get an
interrupt there as well.

> | |                                       |
> |                  | triggered by: vfio_ccw_sch_irq        |
> |                  | action: update SCSW forward IRQ       |
> |                  | state on success: IDLE                |

One thing to consider (and I'm not sure we're handling it correctly
right now): What if we don't have a final status for the I/O yet, i.e.
there will be another interrupt for the I/O? Should we stay in BUSY in
that case?

> |                  | state on error  : IDLE                |

Hm, what error?

> ------------------------------------------------------------
> 
> ------------------------------------------------------------
> | OFFLINE          | The host wants the SC offline         |

That's mdev teardown, I guess?

> | |                                       |
> |                  | triggered by: css remove              |
> |                  | triggered by: css shutdown            |

These as well.

> |                  | action: disable SC                    |
> |                  | state on success: STANDBY             |

If it's really a css remove (unbind from driver or device removal), it
should not really have a target state, as the vfio-ccw device will be
gone, no? This is only correct for mdev removal.

> |                  | state on error  : NOT_OPERATIONAL     |
> ------------------------------------------------------------
> 
> ------------------------------------------------------------
> | STORE_SCHIB      | The hardware issue "schib updated"    |

This is the common I/O layer's sch_event callback. That can mean
different things:
- Something regarding the paths to the device changed. We can translate
  that to "schib updated".
- Device is gone. This is a different situation; we may either try to
  implement the disconnected state, or we may give up the device.

Also, can't we get this event in QUIESCING or STANDBY as well?

> | |                                       |
> |                  | triggered by: sch_event               |
> |                  | action: Store the SCHIB in IO region  |
> |                  | state on success: no change           |

As said, we may want some different handling for "device gone"
situations.

Also, what happens if we get that event in BUSY? Is the I/O still
running?

> |                  | state on error  : NOT_OPERATIONAL     |
> ------------------------------------------------------------
> 
> NOTE 1:
>     All out of band IO related instructions, XSCH, CSCH, HSCH
>     can be started in both states IDLE and BUSY.

Hm. What do you mean by "out of band"?

You can, of course, get all of the instructions above in both IDLE and
BUSY states. The question is what we want to do with them. Do we want
to put them through to the hardware in any case? If we do e.g. a hsch
and get a cc 2, we don't want to drop to IDLE, but stay in BUSY (as the
start function is likely still running).

What about RSCH?

> 
> NOTE 2:
>     Currently IRQ means solicited interrupt, but handle both
>     solicited and unsolicited interrupts.
>     The unsolicited interrupt event may be implemented separatly.

We need to be ready for unsolicited interrupts at any point in time. It
mainly depends on the state whether we want to forward them to the
guest (BUSY, IDLE) or not (QUIESCING, STANDBY).

> 
> 2.5) FSM in any state
> 
> ------------------------------------------------------------
> | NOT_OPERATIONAL  | The device is released                |
> | |                                       |
> |                  | triggered by: mdev_release            |
> |                  | action: unregister IOMMU notifier     |
> |                  | state on success: NOT_OPERATIONAL     |
> ------------------------------------------------------------

Confused. Isn't that "device gone or not operational" as well?

Re: [PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine

[Cornelia Huck]

On Tue, 26 Jun 2018 13:04:12 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 19/06/2018 16:00, Cornelia Huck wrote:
> > On Thu, 14 Jun 2018 10:06:31 +0200
> > Pierre Morel<pmorel@linux.ibm.com>  wrote:
> >  
> >> I tried to make a better description to add later in documentation
> >> or in the next cover-letter.
> >>
> >> Note that in the current patch series I did not implement online/offline
> >> events but just kept the previous state changes.
> >> Not sure if it was a good idea, the goal was to be as simple as possible
> >> for this iteration.
> >> If you think it is worth to continue in this direction I will re-introduce
> >> these as events.  
> > I'm still trying to figure out what we want from the state machine.
> > I've tried sketching your fsm proposal as outlined by you below for
> > myself and I have some remarks :)  
> 
> Hi,
> 
> Sorry for the delay in my answer, I was away from my keyboard
> almost the all week :) .

np :)

> 
> >  
> >> ======================
> >>
> >> 1) In CCW VFIO, Finite State Machine is used to clarify the VFIO CCW driver
> >> actions to be take depending on the events occurring in a device.
> >>
> >> To protect the state transitions, a mutex protect the action
> >> started when an event occurs in a specific state.
> >>
> >> The actions must never sleep or keep the mutex a long timer.
> >>
> >> To be able to take the mutex when hardware events occur we start
> >> a work on a dedicated workqueue, posting the event from inside the
> >> workqueue.
> >>
> >> The state machine has very few states describing the device driver life.
> >>
> >> ------------------------------------------------------------
> >> | NOT_OPERATIONAL  | No guest is associated with the device|  
> > I don't think that this is the right description. It is either the
> > initial state, or something has happened that rendered the device
> > inaccessible.  
> 
> The goal of the state machine is to describe the device driver state.
> Not the device state which is hold by the CIO level.

I don't think there really is such a thing as "device driver state" (or
maybe I don't understand what you mean by it). The state is attached to
the individual device, isn't it?

> 
> I think this has lead to some confusion since I tried to keep the old
> naming convention.
> So, I agree that a state named "NON_INIT" or "UNCONFIGUED" would be better
> to distinguish it from the device state "NOT_OPERATIONAL"
> 
> > Also, we need to be careful with the virtual machine vs. guest
> > terminology. The only thing that has an impact from the guest is when
> > it does I/O and when an interrupt is generated as a result (i.e., the
> > IDLE/BUSY transitions). The other transitions are triggered by virtual
> > machine setup.  
> 
> Absolutely.
> 
> >> ------------------------------------------------------------
> >> | STANDBY          | A guest is associated but not started |  
> > This is basically "device is bound to driver, but no mdev has been set
> > up".  
> 
> When the device is bound to the driver, the device driver
> is still in NOT_OPERATIONAL state.
> 
> The transition to STANDBY is done when the virtual machine starts and
> opens the mediated device.
> Note that the device is still not usable until it is reseted.

Hm, isn't that transition happening when the mdev is created?

> 
> >   In my understanding, we need to get the device to IDLE state
> > before the vm can present it to the guest (be it before the machine is
> > up or during hotplug).  
> 
> The virtual machine needs to RESET the device sending the VFIO_RESET
> to the device driver to make the device driver go to the IDLE state.
> 
> >> ------------------------------------------------------------
> >> | IDLE             | Guest started device ready            |  
> > Agree with "device ready", disagree with "guest started" (see above).
> > "Device ready, accepting I/O requests"?  
> 
> Indeed bad description, VM started, and almost as you said
> "Device driver ready, accepting I/O requests for device"
> (Device is ready too)
> 
> >> ------------------------------------------------------------
> >> | BUSY             | The device is busy doing IO           |
> >> ------------------------------------------------------------
> >> | QUIESCING        | The driver is releasing the device    |  
> > Now you are talking about the driver :) This should probably be done
> > for the other states as well.  
> 
> Ill update the description to make clear that the state is the
> driver state (device driver) and not the device state.
> The device state is handled by the firmware.

Now you have managed to confuse me completely... isn't the firmware
only handling the (real) subchannel state?

> 
> > Isn't that state for waiting for outstanding I/O to be terminated
> > before the mdev is destroyed? IOW, the device may stay bound to the
> > driver afterwards?  
> 
> Yes it is.
> 
> >> ------------------------------------------------------------
> >>
> >>
> >> 2) The following Events may apply to the state machine:
> >>
> >> If the event is not described, it means it has no influence
> >> on the state and that no action is required.
> >>
> >> 2.1) FSM in state NOT_OPERATIONAL
> >> ------------------------------------------------------------
> >> | INIT             | a guest will drive the SC             |  
> > Better to write out "subchannel", I could not figure out the
> > abbreviation immediately.  
> 
> OK, right, Ill do.
> 
> > Also, doesn't the event really mean "we're initializing the device"?  
> 
> No, just the device driver.
> 
> > The ultimate intention is of course for a guest to use the device, but
> > the immediate intention is just "get the device through the first
> > initialization steps".
> >  
> >> | |                                       |
> >> |                  | triggered by: mdev_open               |
> >> |                  | action: initialize driver structures  |
> >> |                  | action: register IOMMU notifier       |
> >> |                  | state on success: STANDBY             |
> >> |                  | state on error  : NOT_OPERATIONAL     |
> >> ------------------------------------------------------------
> >>
> >> 2.2) FSM in state STANDBY
> >>
> >> ------------------------------------------------------------
> >> | ONLINE           | The host wants the SC online          |  
> > Isn't that rather "an mdev is created"?  
> 
> No, ONLINE is triggered by VFIO_RESET, after the mdev has been created
> and when the VM is started or restarted by QEMU.
> 
> I see that I did not do a good job describing what triggers the events.
> I will try again in a dedicated document.

It might be good to combine the two.

> 
> >  
> >> | |                                       |
> >> |                  | triggered by: vfio_reset              |
> >> |                  | action: enable SC                     |
> >> |                  | state on success: IDLE                |
> >> |                  | state on error  : STANDBY             |  
> > What happens if the subchannel is not operational when we try to get it
> > ready? Can it go to NOT_OPERATIONAL in that case?  
> 
> I think we have a confusion between the sub-channel being non operational
> and the device driver being non operational.
> Here, the device driver is operational, even the device may not.

How can something be operational if the device is not? It could be in a
state like the ccw device's disconnected state, but it's certainly not
ready for requests.

> 
> For the VM perspective, if the device is not operational we send a RESET.
> 
> For the guest perspective we can do what ever instruction we needs to
> get the device operational, therefor we need the driver to be operational
> to process the instruction.

Ah, do you mean 'subchannel enabled'? I was thinking about 'device
dead, we get cc 3 or somesuch'.

> 
> 
> 
> >  
> >> ------------------------------------------------------------
> >>
> >> Some operations may be intercepted by the state machine but
> >> will not induce a state change:
> >> OFFLINE: re-issue the disabling of the SC  
> > Should that even be possible? If we're still busy tearing it down,
> > shouldn't we be in QUIESCING state?  
> 
> So yes, I think you are right, after a OFFLINE, triggered by
> the VFIO_release the state is QUESCING
> 
> >  
> >> IRQ    : re-issue the disabling of the SC  
> > Either we should have cleared the subchannel out during QUIESCING, or
> > we got an unsolicited interrupt. The latter can be avoided if we make
> > sure that the device is disabled when we go to STANDBY.  
> 
> I will take a new look at cause of the unsolicited interrupts and
> awaited actions.
> 
> >  
> >> 2.3) FSM in state IDLE
> >>
> >> ------------------------------------------------------------
> >> | SSCH             | The guest issue the ssch instruction  |
> >> | |                                       |
> >> |                  | triggered by: vfio_write SSCH=1       |
> >> |                  | action: start an IO request           |
> >> |                  | state on success: BUSY                |
> >> |                  | state on error  : IDLE                |  
> > Should there be any way to drop to NOT_OPERATIONAL as well? We'll
> > probably get a notification from the common I/O layer if that happens,
> > though.  
> 
> I think this is the duty of the guest to handle this kind of thing.
> The device driver must stay operational.

See above. I think we'll get a notification from the common I/O layer,
and it probably makes sense to inject the same notification (CRW) into
the guest.

> 
> >  
> >> ------------------------------------------------------------
> >> 2.4) FSM in states IDLE or BUSY
> >>
> >> ------------------------------------------------------------
> >> | IRQ              | The hardware issue an interrupt       |  
> > I think we can get this in QUIESCING as well, and it is an indication
> > that QUIESCING is done (for final states).  
> 
> Yes. I did not describe the QUIESCING state in this email.
> because I did not add the patch on QUIESCING.
> With our discussion things becomes clearer and I will
> add it back after corrections.

Ok.

> 
> >
> > If the subchannel is enabled while in STANDBY, we could get an
> > interrupt there as well.  
> 
> No, the subchannel is not enabled while in STANDBY.
> The cio_enable() is issued during the transition from STANDBY
> to IDLE and the event should only happen when the transition
> completed, in IDLE.
> 
> How ever spurious interrupt may happen which should be handled
> locally in the STANDBY state.

Yes, that's what I meant.

> 
> >  
> >> | |                                       |
> >> |                  | triggered by: vfio_ccw_sch_irq        |
> >> |                  | action: update SCSW forward IRQ       |
> >> |                  | state on success: IDLE                |  
> > One thing to consider (and I'm not sure we're handling it correctly
> > right now): What if we don't have a final status for the I/O yet, i.e.
> > there will be another interrupt for the I/O? Should we stay in BUSY in
> > that case?  
> 
> I will take a new look at the interrupt processing.
> If we can be sure another interrupt will come, we may
> wait for it in BUSY.
> In case of doubt we better not and handle the interrupt in IDLE
> otherwise we hang in BUSY.

Agreed. But we should be able to find out whether we got a final state.

> 
> >  
> >> |                  | state on error  : IDLE                |  
> > Hm, what error?  
> 
> There are no error there, therefor no state change.
> A better description is "state on error: NA"

Ok.

> 
> >  
> >> ------------------------------------------------------------
> >>
> >> ------------------------------------------------------------
> >> | OFFLINE          | The host wants the SC offline         |  
> > That's mdev teardown, I guess?
> >  
> >> | |                                       |
> >> |                  | triggered by: css remove              |
> >> |                  | triggered by: css shutdown            |  
> > These as well.
> >  
> >> |                  | action: disable SC                    |
> >> |                  | state on success: STANDBY             |  
> > If it's really a css remove (unbind from driver or device removal), it
> > should not really have a target state, as the vfio-ccw device will be
> > gone, no? This is only correct for mdev removal.  
> 
> May be more complex.
> 
> The event is not right, we should have two different:
> 
> - On css remove and shutdown
> There we have a serious problem, a sub-channel disappeared.
> CIO level handle the quiescing of the sub-channel.
> The final state is NOT_OPER and we must somehow say the guest
> that the hardware configuration changed (machine check?).
> Don't we ?

I think there's a comment/TODO in the code for that.

> 
> - On vfio_release
> This is another thing, the guest goes away, so we need to
> smoothly shut down the sub channel using the QUIESCING state.
> 
> Once again this an indication to rework the OFFLINE event.
> 
> >  
> >> |                  | state on error  : NOT_OPERATIONAL     |
> >> ------------------------------------------------------------
> >>
> >> ------------------------------------------------------------
> >> | STORE_SCHIB      | The hardware issue "schib updated"    |  
> > This is the common I/O layer's sch_event callback. That can mean
> > different things:
> > - Something regarding the paths to the device changed. We can translate
> >    that to "schib updated".
> > - Device is gone. This is a different situation; we may either try to
> >    implement the disconnected state, or we may give up the device.
> >
> > Also, can't we get this event in QUIESCING or STANDBY as well?  
> 
> I think it should be ignored in STANDBY as we did not even
> start to use the sub-channel we have no information on it
> at that moment.

As long as we do update it some time before we need the information, ok.

> 
> In QUIESCING we may have to handle it for internal purpose
> to make sure to shutdown smoothly.
> I will work on this again.
> (Still this OFFLINE refactoring)
> 
> >  
> >> | |                                       |
> >> |                  | triggered by: sch_event               |
> >> |                  | action: Store the SCHIB in IO region  |
> >> |                  | state on success: no change           |  
> > As said, we may want some different handling for "device gone"
> > situations.
> >
> > Also, what happens if we get that event in BUSY? Is the I/O still
> > running?  
> 
> AFAIK yes, it is an asynchronous event.
> It can happen during BUSY.
> However AFAIU we still get an IRQ even we get the device gone
> during I/O operation.

It depends. We either get a deferred cc 3, or a machine check (but no
interrupt).

> 
> >  
> >> |                  | state on error  : NOT_OPERATIONAL     |
> >> ------------------------------------------------------------
> >>
> >> NOTE 1:
> >>      All out of band IO related instructions, XSCH, CSCH, HSCH
> >>      can be started in both states IDLE and BUSY.  
> > Hm. What do you mean by "out of band"?  
> May be a bad description, I made a difference between SSCH/IRQ
> two event handling I/O operations with a little protocoling:
> We send SSCH during IDLE we move to BUSY we expect IRQ
> and when IRQ we move to IDLE.
> 
> And the other instructions which can go aside of the
> protocoling direct to the hardware whatever the state is.
> 
> >
> > You can, of course, get all of the instructions above in both IDLE and
> > BUSY states. The question is what we want to do with them. Do we want
> > to put them through to the hardware in any case? If we do e.g. a hsch
> > and get a cc 2, we don't want to drop to IDLE, but stay in BUSY (as the
> > start function is likely still running).  
> 
> In the case the instruction is accepted (CC=0):
> 
> CANCEL: XSCH
>      in a first draw should not go to the device driver.
>      later we may check if we got a cancel before starting the SSCH 
> instruction
>      inside of the SSCH action.

Ok. Although we probably don't need to dwell on this too much, the
"always cc 2 on xsch" approach should already cover guest usage, I
guess.

> 
> CLEAR: CSCH
>      goes directly to hardware, no state change.
>      BUSY state: we will get an interrupt with clear pending which drive 
> us to IDLE
> 
> HALT: HSCH
>      goes directly to hardware, no state change.
>      BUSY state: we will get an interrupt with halt pending which drive 
> us to IDLE

I'm not sure about that; csch/hsch are different. csch clears any other
pending state; hsch may return busy, pending on what is currently going
on.

> 
> In all cases the CCW chain is kept until we get the confirmation that the
> program is not used any more by the sub-channel.

For csch, that would be immediately after cc 0.

> 
> In the case the instruction is not accepted (CC!=0)
> we must return the answer to the guest.

Yes, that's only for hsch, though.

> 
> >
> > What about RSCH?  
> 
> The CCW program must be kept "alive" in the kernel waiting to be resumed.
> 
> If we find the SUSPEND bit inside the CCW program during the processing
> of a SSCH, we should return a new "SUSPENDED" state to the state machine.
> 
> When we receive the RSCH from the guest
>      - We verify that guest cleared the SUSPEND bit of the current CCW
>        and fetch the new chain starting at this CCW
>      - we forward it to the hardware and go to BUSY state
> 
> 
> Another thing which will induce a new state is the CCW PCI bit which will
> induce intermediate status.
> (by the way we also should handle the ORB I bit)

This is something that needs handling, agreed.

> 
> >  
> >> NOTE 2:
> >>      Currently IRQ means solicited interrupt, but handle both
> >>      solicited and unsolicited interrupts.
> >>      The unsolicited interrupt event may be implemented separatly.  
> > We need to be ready for unsolicited interrupts at any point in time. It
> > mainly depends on the state whether we want to forward them to the
> > guest (BUSY, IDLE) or not (QUIESCING, STANDBY).  
> 
> 100% agree
> 
> >  
> >> 2.5) FSM in any state
> >>
> >> ------------------------------------------------------------
> >> | NOT_OPERATIONAL  | The device is released                |
> >> | |                                       |
> >> |                  | triggered by: mdev_release            |
> >> |                  | action: unregister IOMMU notifier     |
> >> |                  | state on success: NOT_OPERATIONAL     |
> >> ------------------------------------------------------------  
> > Confused. Isn't that "device gone or not operational" as well?
> >  
> 
> It is confusing, as said before, I need to revisit the OFFLINE event.
> 
> instead of using the cover letter, I will enhance the vfio-ccw documentation
> to cover the state machine.

Sounds like a good idea.

> 
> Thanks a lot for the reviewing.

I hope I did not add to the confusion :)

Re: [PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine

[Pierre Morel]

On 26/06/2018 18:00, Cornelia Huck wrote:
> On Tue, 26 Jun 2018 13:04:12 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> On 19/06/2018 16:00, Cornelia Huck wrote:
>>> On Thu, 14 Jun 2018 10:06:31 +0200
>>> Pierre Morel<pmorel@linux.ibm.com>  wrote:
>>>   
...snip...
>> The goal of the state machine is to describe the device driver state.
>> Not the device state which is hold by the CIO level.
> I don't think there really is such a thing as "device driver state" (or
> maybe I don't understand what you mean by it). The state is attached to
> the individual device, isn't it?
Yes but it is related to the software handling the device and not to the 
device.
This is a difference between QEMU, Linux VFIO driver and Hardware.

In my point of view:

QEMU                  : Emulates a device, internal state is state of 
the device
Linux VFIO driver: Handle the device, internal state is driver state
Hardware            : obviously get the device state


>
>> I think this has lead to some confusion since I tried to keep the old
>> naming convention.
>> So, I agree that a state named "NON_INIT" or "UNCONFIGUED" would be better
>> to distinguish it from the device state "NOT_OPERATIONAL"
>>
>>> Also, we need to be careful with the virtual machine vs. guest
>>> terminology. The only thing that has an impact from the guest is when
>>> it does I/O and when an interrupt is generated as a result (i.e., the
>>> IDLE/BUSY transitions). The other transitions are triggered by virtual
>>> machine setup.
>> Absolutely.
>>
>>>> ------------------------------------------------------------
>>>> | STANDBY          | A guest is associated but not started |
>>> This is basically "device is bound to driver, but no mdev has been set
>>> up".
>> When the device is bound to the driver, the device driver
>> is still in NOT_OPERATIONAL state.
>>
>> The transition to STANDBY is done when the virtual machine starts and
>> opens the mediated device.
>> Note that the device is still not usable until it is reseted.
> Hm, isn't that transition happening when the mdev is created?
No, when the mdev is created the VM is not started.
The STANDBY state is entered when the mdev_open is called,
this is when the VFIO framework is initialized as QEMU starts.

When the mdev is created the mdev_create is called but do not
induce any state change.

>
...snip...
>> Now you are talking about the driver :) This should probably be done
>>> for the other states as well.
>> Ill update the description to make clear that the state is the
>> driver state (device driver) and not the device state.
>> The device state is handled by the firmware.
> Now you have managed to confuse me completely... isn't the firmware
> only handling the (real) subchannel state?

Yes and QEMU the virtual one.
But the driver does not need to handle the device state
but the driver state. Which is quite different.

>

>>> Isn't that rather "an mdev is created"?
>> No, ONLINE is triggered by VFIO_RESET, after the mdev has been created
>> and when the VM is started or restarted by QEMU.
>>
>> I see that I did not do a good job describing what triggers the events.
>> I will try again in a dedicated document.
> It might be good to combine the two.

When mdev is created the VM does not exist, it is created by the admin.

The mdev_open() is called once when the VM starts.
VFIO_RESET is called on start and every time the VM is reseted.

These are three different events occuring in the life of the driver.
mdev creation do not influence the way the driver works.
mdev open does, and issues the INIT event
VFIO_RESET does and issues the ONLINE event

>
>>>   
>>>> | |                                       |
>>>> |                  | triggered by: vfio_reset              |
>>>> |                  | action: enable SC                     |
>>>> |                  | state on success: IDLE                |
>>>> |                  | state on error  : STANDBY             |
>>> What happens if the subchannel is not operational when we try to get it
>>> ready? Can it go to NOT_OPERATIONAL in that case?
>> I think we have a confusion between the sub-channel being non operational
>> and the device driver being non operational.
>> Here, the device driver is operational, even the device may not.
> How can something be operational if the device is not? It could be in a
> state like the ccw device's disconnected state, but it's certainly not
> ready for requests.

The driver may be operational even the device is not.
Depending on the protocol between device and driver it allows
the driver to reset the device.
We have two drivers here speaking with the device, the one in the guest
and the VFIO driver.
The VFIO driver works is to handle the virtualization infrastructure,
not to handle the device. Handling the device is done by the guest
driver.

>
>> For the VM perspective, if the device is not operational we send a RESET.
>>
>> For the guest perspective we can do what ever instruction we needs to
>> get the device operational, therefor we need the driver to be operational
>> to process the instruction.
> Ah, do you mean 'subchannel enabled'? I was thinking about 'device
> dead, we get cc 3 or somesuch'.

What can we do on the host level that the guest can not do?
If we have a way to retrieve a dead device in the host, shouldn't
we give the guest this possibility?

Speaking about sub-channel enabled:

We currently have very restricted possibilities for the guest
to manage the sub-channel or even to get real information
on the sub-channel.
Dong-Jia began working in this direction with his last patch series
and I think it is a way to follow.
My point of view is that we should not do too much in the VFIO
driver but only take care of the virtualization of the SCHIB.
But this is beyond the scope of this patch .

>

>>>> |                  | state on error  : IDLE                |
>>> Should there be any way to drop to NOT_OPERATIONAL as well? We'll
>>> probably get a notification from the common I/O layer if that happens,
>>> though.
>> I think this is the duty of the guest to handle this kind of thing.
>> The device driver must stay operational.
> See above. I think we'll get a notification from the common I/O layer,
> and it probably makes sense to inject the same notification (CRW) into
> the guest.

Absolutely, I agree 100%

>
>>>   

...snip...

  

>>> This is the common I/O layer's sch_event callback. That can mean
>>> different things:
>>> - Something regarding the paths to the device changed. We can translate
>>>     that to "schib updated".
>>> - Device is gone. This is a different situation; we may either try to
>>>     implement the disconnected state, or we may give up the device.
>>>
>>> Also, can't we get this event in QUIESCING or STANDBY as well?
>> I think it should be ignored in STANDBY as we did not even
>> start to use the sub-channel we have no information on it
>> at that moment.
> As long as we do update it some time before we need the information, ok.

AFAIK we currently never forward this information to the guest,
when we do it we will need to virtualize this information.


>
>> In QUIESCING we may have to handle it for internal purpose
>> to make sure to shutdown smoothly.
>> I will work on this again.
>> (Still this OFFLINE refactoring)
>>
>>>   
>>>> | |                                       |
>>>> |                  | triggered by: sch_event               |
>>>> |                  | action: Store the SCHIB in IO region  |
>>>> |                  | state on success: no change           |
>>> As said, we may want some different handling for "device gone"
>>> situations.
>>>
>>> Also, what happens if we get that event in BUSY? Is the I/O still
>>> running?
>> AFAIK yes, it is an asynchronous event.
>> It can happen during BUSY.
>> However AFAIU we still get an IRQ even we get the device gone
>> during I/O operation.
> It depends. We either get a deferred cc 3, or a machine check (but no
> interrupt).

I understood from current code that all kind of CRW do not
imply that the sub-channel is gone.
I must investigate a little more.

Anyway, the CRW should be handled and forwarded to the guest
but I would prefer not to change too much in this series,
shouldn't this changes be part of a next series?

best regards,

Pierre


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

Re: [PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine

[Cornelia Huck]

On Wed, 27 Jun 2018 12:00:41 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

Apologies for the late answer, this topic had dropped off my radar.

> On 26/06/2018 18:00, Cornelia Huck wrote:
> > On Tue, 26 Jun 2018 13:04:12 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >  
> >> On 19/06/2018 16:00, Cornelia Huck wrote:  
> >>> On Thu, 14 Jun 2018 10:06:31 +0200
> >>> Pierre Morel<pmorel@linux.ibm.com>  wrote:
> >>>     
> ...snip...
> >> The goal of the state machine is to describe the device driver state.
> >> Not the device state which is hold by the CIO level.  
> > I don't think there really is such a thing as "device driver state" (or
> > maybe I don't understand what you mean by it). The state is attached to
> > the individual device, isn't it?  
> Yes but it is related to the software handling the device and not to the 
> device.
> This is a difference between QEMU, Linux VFIO driver and Hardware.
> 
> In my point of view:
> 
> QEMU                  : Emulates a device, internal state is state of 
> the device
> Linux VFIO driver: Handle the device, internal state is driver state
> Hardware            : obviously get the device state

OK, I understand where you're coming from, but I find your terminology
a bit confusing :)

Maybe call the state that is tracked by the vfio driver "vfio device
state"?

> >>>> ------------------------------------------------------------
> >>>> | STANDBY          | A guest is associated but not started |  
> >>> This is basically "device is bound to driver, but no mdev has been set
> >>> up".  
> >> When the device is bound to the driver, the device driver
> >> is still in NOT_OPERATIONAL state.
> >>
> >> The transition to STANDBY is done when the virtual machine starts and
> >> opens the mediated device.
> >> Note that the device is still not usable until it is reseted.  
> > Hm, isn't that transition happening when the mdev is created?  
> No, when the mdev is created the VM is not started.
> The STANDBY state is entered when the mdev_open is called,
> this is when the VFIO framework is initialized as QEMU starts.
> 
> When the mdev is created the mdev_create is called but do not
> induce any state change.

Ah, now I understand where the disconnect here came from: Your patch 7
changed this :)

I have to think further about this (still undecided).

> ...snip...
> >> Now you are talking about the driver :) This should probably be done  
> >>> for the other states as well.  
> >> Ill update the description to make clear that the state is the
> >> driver state (device driver) and not the device state.
> >> The device state is handled by the firmware.  
> > Now you have managed to confuse me completely... isn't the firmware
> > only handling the (real) subchannel state?  
> 
> Yes and QEMU the virtual one.
> But the driver does not need to handle the device state
> but the driver state. Which is quite different.

That's again the terminology confusion from above.

> >>> Isn't that rather "an mdev is created"?  
> >> No, ONLINE is triggered by VFIO_RESET, after the mdev has been created
> >> and when the VM is started or restarted by QEMU.
> >>
> >> I see that I did not do a good job describing what triggers the events.
> >> I will try again in a dedicated document.  
> > It might be good to combine the two.  
> 
> When mdev is created the VM does not exist, it is created by the admin.
> 
> The mdev_open() is called once when the VM starts.
> VFIO_RESET is called on start and every time the VM is reseted.

And this was again /me looking at the current code.

> 
> These are three different events occuring in the life of the driver.
> mdev creation do not influence the way the driver works.

I'm wondering if it should. Having an mdev is a prereq for any setup.
So a device in not oper state without mdev is a different thing from a
device in not oper state with an mdev that received e.g. some path
failures.

> mdev open does, and issues the INIT event
> VFIO_RESET does and issues the ONLINE event

The last one is possibly a bit unintuitive (might be a naming issue).

> >>>> |                  | triggered by: vfio_reset              |
> >>>> |                  | action: enable SC                     |
> >>>> |                  | state on success: IDLE                |
> >>>> |                  | state on error  : STANDBY             |  
> >>> What happens if the subchannel is not operational when we try to get it
> >>> ready? Can it go to NOT_OPERATIONAL in that case?  
> >> I think we have a confusion between the sub-channel being non operational
> >> and the device driver being non operational.
> >> Here, the device driver is operational, even the device may not.  
> > How can something be operational if the device is not? It could be in a
> > state like the ccw device's disconnected state, but it's certainly not
> > ready for requests.  
> 
> The driver may be operational even the device is not.
> Depending on the protocol between device and driver it allows
> the driver to reset the device.
> We have two drivers here speaking with the device, the one in the guest
> and the VFIO driver.
> The VFIO driver works is to handle the virtualization infrastructure,
> not to handle the device. Handling the device is done by the guest
> driver.

Hm, let's assume the hardware device is not operational (cc 3 on ssch
or whatever).

What I'd assume to happen would be:
- The vfio driver notices that the subchannel is not operational, and
  switches state or flips an indication somewhere.
- The vfio driver is probably also notified by the common I/O layer
  (because of machine check on the hardware). It has two choices: give
  up on the device (the common I/O layer will deregister the device),
  or keep it (which would mean something similar to the disconnected
  state for ccw devices; I guess that is what you meant with 'the
  driver is operational'?).
- If the (subchannel) device is gone, the child mdev should be gone as
  well.

What would be the best way to interact with the guest? On real
hardware, I'd expect both cc 3 and crws. Regardless whether we
unregister or do something like the disconnected state, we should relay
the cc 3 to the guest *and* inject a machine check to kick the guest's
handling.

If we use the disconnected state approach, what should the guest get if
it tries to interact with the device anyway? We could answer with cc 3
immediately, or let the hardware give us the cc 3 that we relay back to
the guest. (We should get a notification from the common I/O layer when
it is informed via a machine check that the device is alive again.)

What do you think?

> >> For the VM perspective, if the device is not operational we send a RESET.
> >>
> >> For the guest perspective we can do what ever instruction we needs to
> >> get the device operational, therefor we need the driver to be operational
> >> to process the instruction.  
> > Ah, do you mean 'subchannel enabled'? I was thinking about 'device
> > dead, we get cc 3 or somesuch'.  
> 
> What can we do on the host level that the guest can not do?
> If we have a way to retrieve a dead device in the host, shouldn't
> we give the guest this possibility?

See above... we can get the machine checks, but the guest can't.

> Speaking about sub-channel enabled:
> 
> We currently have very restricted possibilities for the guest
> to manage the sub-channel or even to get real information
> on the sub-channel.
> Dong-Jia began working in this direction with his last patch series
> and I think it is a way to follow.

Is anyone at IBM picking up this work?

> My point of view is that we should not do too much in the VFIO
> driver but only take care of the virtualization of the SCHIB.
> But this is beyond the scope of this patch .

The main thing missing is probably machine check handling. But probably
msch passthrough as well.

> >>> This is the common I/O layer's sch_event callback. That can mean
> >>> different things:
> >>> - Something regarding the paths to the device changed. We can translate
> >>>     that to "schib updated".
> >>> - Device is gone. This is a different situation; we may either try to
> >>>     implement the disconnected state, or we may give up the device.
> >>>
> >>> Also, can't we get this event in QUIESCING or STANDBY as well?  
> >> I think it should be ignored in STANDBY as we did not even
> >> start to use the sub-channel we have no information on it
> >> at that moment.  
> > As long as we do update it some time before we need the information, ok.  
> 
> AFAIK we currently never forward this information to the guest,
> when we do it we will need to virtualize this information.

Agreed.
  
> >>>> | |                                       |
> >>>> |                  | triggered by: sch_event               |
> >>>> |                  | action: Store the SCHIB in IO region  |
> >>>> |                  | state on success: no change           |  
> >>> As said, we may want some different handling for "device gone"
> >>> situations.
> >>>
> >>> Also, what happens if we get that event in BUSY? Is the I/O still
> >>> running?  
> >> AFAIK yes, it is an asynchronous event.
> >> It can happen during BUSY.
> >> However AFAIU we still get an IRQ even we get the device gone
> >> during I/O operation.  
> > It depends. We either get a deferred cc 3, or a machine check (but no
> > interrupt).  
> 
> I understood from current code that all kind of CRW do not
> imply that the sub-channel is gone.
> I must investigate a little more.

Path handling is sometimes a bit odd...

> 
> Anyway, the CRW should be handled and forwarded to the guest
> but I would prefer not to change too much in this series,
> shouldn't this changes be part of a next series?

Yes, that should be a different series, as it will probably be quite
complex and needs thought. Anyway, I've updated
https://wiki.qemu.org/ToDo/Channel_I/O_Passthrough with some machine
check considerations.