vfio-ccw: support hsch/csch (kernel part)

Allow to extend the regions used by vfio-ccw. The first user will be handling of halt and clear subchannel.

Signed-off-by Cornelia Huck <[email protected]>

---
  drivers/s390/cio/vfio_ccw_ops.c     | 182 ++++++++++++++++++++++++----
  drivers/s390/cio/vfio_ccw_private.h |  38 ++++++
  include/uapi/linux/vfio.h           |   1 +
  3 files changed, 195 insertions(+), 26 deletions(-)

Quoted 10 lines · Cornelia Huck

Allow to extend the regions used by vfio-ccw. The first user will be handling of halt and clear subchannel.

Signed-off-by Cornelia Huck <[email protected]>

---
  drivers/s390/cio/vfio_ccw_ops.c     | 182 ++++++++++++++++++++++++----
  drivers/s390/cio/vfio_ccw_private.h |  38 ++++++
  include/uapi/linux/vfio.h           |   1 +
  3 files changed, 195 insertions(+), 26 deletions(-)

Halt and clear have no parameters (the sub-channel ID is obviously the one of the mediated device).

Isn't adding a new sub-region for the purpose of handling halt and clear superfluous?

What is the reason not to use simple ioctls ?

Quoted 18 lines · On Fri, 23 Nov 2018 13:28:25 +0100 Pierre Morel <[email protected]> wrote:

Quoted 10 lines · Cornelia Huck

Allow to extend the regions used by vfio-ccw. The first user will be handling of halt and clear subchannel.

Signed-off-by Cornelia Huck <[email protected]>

---
   drivers/s390/cio/vfio_ccw_ops.c     | 182 ++++++++++++++++++++++++----
   drivers/s390/cio/vfio_ccw_private.h |  38 ++++++
   include/uapi/linux/vfio.h           |   1 +
   3 files changed, 195 insertions(+), 26 deletions(-)

Halt and clear have no parameters (the sub-channel ID is obviously the one of the mediated device).

Isn't adding a new sub-region for the purpose of handling halt and clear superfluous?

Quoted 1 line

What is the reason not to use simple ioctls ?

Should it turn out that we missed something and need an enhanced interface, we can simply stop providing this subregion and add a new subregion, without breaking existing userspace. We can't do that with ioctls.

And moreover, this is only the first user of this infrastructure. There's also that path handling series that Dong Jia had posted early this year -- that would be an obvious user as well.

drivers/s390/cio/vfio_ccw_private.h +2 -0

index 078e46f9623d..a6f9f84526e2 100644
@@ -3,9 +3,11 @@
   * Private stuff for vfio_ccw driver
   *
   * Copyright IBM Corp. 2017
+ * Copyright Red Hat, Inc. 2018
   *
   * Author(s): Dong Jia Shi<[email protected]>
   *            Xiao Feng Ren<[email protected]>
+ *            Cornelia Huck<[email protected]>
   */
 
  #ifndef_VFIO_CCW_PRIVATE_H_

Quoted 20 lines · Cornelia Huck

drivers/s390/cio/vfio_ccw_private.h +2 -0

index 078e46f9623d..a6f9f84526e2 100644
@@ -3,9 +3,11 @@
   * Private stuff for vfio_ccw driver
   *
   * Copyright IBM Corp. 2017
+ * Copyright Red Hat, Inc. 2018
   *
   * Author(s): Dong Jia Shi<[email protected]>
   *            Xiao Feng Ren<[email protected]>
+ *            Cornelia Huck<[email protected]>
   */
 
  #ifndef_VFIO_CCW_PRIVATE_H_

@@ -19,6 +21,38 @@
  #include "css.h"
  #include "vfio_ccw_cp.h"
 

drivers/s390/cio/vfio_ccw_private.h line 28

Quoted 5 lines

drivers/s390/cio/vfio_ccw_private.h +5 -0

+#define VFIO_CCW_OFFSET_SHIFT   40
+#define VFIO_CCW_OFFSET_TO_INDEX(off)	(off >> VFIO_CCW_OFFSET_SHIFT)
+#define VFIO_CCW_INDEX_TO_OFFSET(index)	((u64)(index) << VFIO_CCW_OFFSET_SHIFT)
+#define VFIO_CCW_OFFSET_MASK	(((u64)(1) << VFIO_CCW_OFFSET_SHIFT) - 1)
+  

Why is the offset shift 40? I know vfio-pci is also using the same offset shift, but I am curious about the reasoning behind why we are using this? :)

include/uapi/linux/vfio.h +0 -0

  /* 8086 Vendor sub-types */
  #define VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION	(1)
 

On 11/22/2018 11:54 AM, Cornelia Huck wrote:

...snip...

Quoted 6 lines

include/uapi/linux/vfio.h +0 -0

index 813102810f53..565669f95534 100644
@@ -297,6 +297,7 @@ struct vfio_region_info_cap_type {
 

include/uapi/linux/vfio.h struct vfio_region_info_cap_type { · line 300

Quoted 3 lines

include/uapi/linux/vfio.h +1 -0

  #define VFIO_REGION_TYPE_PCI_VENDOR_TYPE	(1 << 31)
  #define VFIO_REGION_TYPE_PCI_VENDOR_MASK	(0xffff)
+#define VFIO_REGION_TYPE_CCW			(1 << 30)  

Oof. So the existing VFIO_REGION_TYPE_PCI_VENDOR_TYPE gets OR'd with another value (e.g., 8086). But in 4.20, there was a VFIO_REGION_TYPE_GFX is added as simply "1" ... Which direction are these definitions being added from? I guess asked another way, is _TYPE_CCW going to be OR'd with anything else that necessitates its presence as an identifier with some Other Thing, or should this follow the TYPE_GFX enumeration? Perhaps the type field needs to be tidied up to help this sit more cleanly now? (Sorry!)

- Eric

Quoted 4 lines

include/uapi/linux/vfio.h +0 -0

  /* 8086 Vendor sub-types */
  #define VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION	(1)
 

Quoted 10 lines · On Mon, 17 Dec 2018 16:53:34 -0500 Eric Farman <[email protected]> wrote:

On 11/22/2018 11:54 AM, Cornelia Huck wrote:

...snip...

Quoted 6 lines

include/uapi/linux/vfio.h +0 -0

index 813102810f53..565669f95534 100644
@@ -297,6 +297,7 @@ struct vfio_region_info_cap_type {
 

Quoted 12 lines

include/uapi/linux/vfio.h struct vfio_region_info_cap_type { · line 300

Quoted 3 lines

include/uapi/linux/vfio.h +1 -0

   #define VFIO_REGION_TYPE_PCI_VENDOR_TYPE	(1 << 31)
   #define VFIO_REGION_TYPE_PCI_VENDOR_MASK	(0xffff)
+#define VFIO_REGION_TYPE_CCW			(1 << 30)

Oof. So the existing VFIO_REGION_TYPE_PCI_VENDOR_TYPE gets OR'd with another value (e.g., 8086). But in 4.20, there was a VFIO_REGION_TYPE_GFX is added as simply "1" ... Which direction are these definitions being added from? I guess asked another way, is _TYPE_CCW going to be OR'd with anything else that necessitates its presence as an identifier with some Other Thing, or should this follow the TYPE_GFX enumeration? Perhaps the type field needs to be tidied up to help this sit more cleanly now? (Sorry!)

The semantics of that type stuff are really a bit unclear to me :(

I don't think we'll ever do any fancy mask handling for ccw. It is probably enough to have any kind of uniqueness within the different types, so maybe counting up would be indeed enough...

Quoted 9 lines

- Eric

Quoted 4 lines

include/uapi/linux/vfio.h +0 -0

   /* 8086 Vendor sub-types */
   #define VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION	(1)
 

Quoted 10 lines · On Mon, 17 Dec 2018 16:53:34 -0500 Eric Farman <[email protected]> wrote:

On 11/22/2018 11:54 AM, Cornelia Huck wrote:

...snip...

Quoted 6 lines

include/uapi/linux/vfio.h +0 -0

index 813102810f53..565669f95534 100644
@@ -297,6 +297,7 @@ struct vfio_region_info_cap_type {
 

Quoted 12 lines

include/uapi/linux/vfio.h struct vfio_region_info_cap_type { · line 300

Quoted 3 lines

include/uapi/linux/vfio.h +1 -0

  #define VFIO_REGION_TYPE_PCI_VENDOR_TYPE	(1 << 31)
  #define VFIO_REGION_TYPE_PCI_VENDOR_MASK	(0xffff)
+#define VFIO_REGION_TYPE_CCW			(1 << 30)    

Oof. So the existing VFIO_REGION_TYPE_PCI_VENDOR_TYPE gets OR'd with another value (e.g., 8086). But in 4.20, there was a VFIO_REGION_TYPE_GFX is added as simply "1" ... Which direction are these definitions being added from? I guess asked another way, is _TYPE_CCW going to be OR'd with anything else that necessitates its presence as an identifier with some Other Thing, or should this follow the TYPE_GFX enumeration? Perhaps the type field needs to be tidied up to help this sit more cleanly now? (Sorry!)

The semantics of that type stuff are really a bit unclear to me :(

I don't think we'll ever do any fancy mask handling for ccw. It is probably enough to have any kind of uniqueness within the different types, so maybe counting up would be indeed enough...

Quoted 26 lines · On Tue, 18 Dec 2018 18:24:00 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 10 lines · On Mon, 17 Dec 2018 16:53:34 -0500 Eric Farman <[email protected]> wrote:

On 11/22/2018 11:54 AM, Cornelia Huck wrote:

...snip...

Quoted 6 lines

include/uapi/linux/vfio.h +0 -0

index 813102810f53..565669f95534 100644
@@ -297,6 +297,7 @@ struct vfio_region_info_cap_type {
 

Quoted 12 lines

include/uapi/linux/vfio.h struct vfio_region_info_cap_type { · line 300

Quoted 3 lines

include/uapi/linux/vfio.h +1 -0

  #define VFIO_REGION_TYPE_PCI_VENDOR_TYPE	(1 << 31)
  #define VFIO_REGION_TYPE_PCI_VENDOR_MASK	(0xffff)
+#define VFIO_REGION_TYPE_CCW			(1 << 30)      

Oof. So the existing VFIO_REGION_TYPE_PCI_VENDOR_TYPE gets OR'd with another value (e.g., 8086). But in 4.20, there was a VFIO_REGION_TYPE_GFX is added as simply "1" ... Which direction are these definitions being added from? I guess asked another way, is _TYPE_CCW going to be OR'd with anything else that necessitates its presence as an identifier with some Other Thing, or should this follow the TYPE_GFX enumeration? Perhaps the type field needs to be tidied up to help this sit more cleanly now? (Sorry!)

Quoted 5 lines

The semantics of that type stuff are really a bit unclear to me :(

I don't think we'll ever do any fancy mask handling for ccw. It is probably enough to have any kind of uniqueness within the different types, so maybe counting up would be indeed enough...

Just to confirm, this is the intended usage, simply reserve a new type following the GFX region example. We can define VFIO_REGION_TYPE_CCW as 2 and then there's a whole address space of sub-types to fill in within that. I might have over-engineered PCI a bit with the address space split, but it seemed like a good idea at the time to pre-define a type address space for each vendor, such that they only need to define the sub-types and we can avoid namespace collisions. Unfortunately this implicit definition for each PCI vendor also contributes to the confusion here. Sorry. Thanks,

Alex

The vfio-ccw code will need this, and it matches treatment of ssch and csch.

Signed-off-by Cornelia Huck <[email protected]>

---
  drivers/s390/cio/ioasm.c | 1 +
  1 file changed, 1 insertion(+)

drivers/s390/cio/ioasm.c +0 -0

index 14d328338ce2..08eb10283b18 100644

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
  drivers/s390/cio/Makefile           |   3 +-
  drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
  drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
  drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
  drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
  drivers/s390/cio/vfio_ccw_private.h |   6 ++
  include/uapi/linux/vfio.h           |   4 +
  include/uapi/linux/vfio_ccw.h       |  12 +++
  8 files changed, 313 insertions(+), 19 deletions(-)

create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/Makefile +2 -1

index f230516abb96..f6a8db04177c 100644
@@ -20,5 +20,6 @@ obj-$(CONFIG_CCWGROUP) += ccwgroup.o
  qdio-objs := qdio_main.o qdio_thinint.o qdio_debug.o qdio_setup.o
  obj-$(CONFIG_QDIO) += qdio.o
 
-vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o
+vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o \
+	vfio_ccw_async.o
  obj-$(CONFIG_VFIO_CCW) += vfio_ccw.o

drivers/s390/cio/vfio_ccw_async.c new file +1 -0

new file mode 100644
index 000000000000..8c7f51d17d70
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0

static void __exit vfio_ccw_sch_exit(void)

drivers/s390/cio/vfio_ccw_fsm.c +27 -0

index f94aa01f9c36..0caf77e8f377 100644
@@ -3,8 +3,10 @@
   * Finite state machine for vfio-ccw device handling
   *
   * Copyright IBM Corp. 2017
+ * Copyright Red Hat, Inc. 2018
   *
   * Author(s): Dong Jia Shi <[email protected]>
+ *            Cornelia Huck <[email protected]>
   */
 
  #include <linux/vfio.h>

@@ -68,6 +70,81 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
  	return ret;
  }
 
+static int fsm_do_halt(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch;
+	unsigned long flags;
+	int ccode;
+	int ret;
+
+	sch = private->sch;
+
+	spin_lock_irqsave(sch->lock, flags);
+	private->state = VFIO_CCW_STATE_BUSY;
+
+	/* Issue "Halt Subchannel" */
+	ccode = hsch(sch->schid);
+
+	switch (ccode) {
+	case 0:
+		/*
+		 * Initialize device status information
+		 */
+		sch->schib.scsw.cmd.actl |= SCSW_ACTL_HALT_PEND;
+		ret = 0;
+		break;
+	case 1:		/* Status pending */
+	case 2:		/* Busy */

drivers/s390/cio/vfio_ccw_fsm.c +54 -0

+	spin_unlock_irqrestore(sch->lock, flags);
+	return ret;
+}
+
+static int fsm_do_clear(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch;
+	unsigned long flags;
+	int ccode;
+	int ret;
+
+	sch = private->sch;
+
+	spin_lock_irqsave(sch->lock, flags);
+	private->state = VFIO_CCW_STATE_BUSY;
+
+	/* Issue "Clear Subchannel" */
+	ccode = csch(sch->schid);
+
+	switch (ccode) {
+	case 0:
+		/*
+		 * Initialize device status information
+		 */
+		sch->schib.scsw.cmd.actl = SCSW_ACTL_CLEAR_PEND;
+		/* TODO: check what else we might need to clear */
+		ret = 0;
+		break;
+	case 3:		/* Device not operational */
+	{
+		ret = -ENODEV;
+		break;
+	}
+	default:
+		ret = ccode;
+	}
+	spin_unlock_irqrestore(sch->lock, flags);
+	return ret;
+}
+
  static void fsm_notoper(struct vfio_ccw_private *private,
  			enum vfio_ccw_event event)
  {

@@ -102,6 +179,20 @@ static void fsm_io_busy(struct vfio_ccw_private *private,
  	private->io_region->ret_code = -EBUSY;
  }
 
+static void fsm_async_error(struct vfio_ccw_private *private,
+			    enum vfio_ccw_event event)
+{
+	pr_err("vfio-ccw: FSM: halt/clear request from state:%d\n",
+	       private->state);
+	private->cmd_region->ret_code = -EIO;
+}
+
+static void fsm_async_busy(struct vfio_ccw_private *private,
+			   enum vfio_ccw_event event)
+{
+	private->cmd_region->ret_code = -EBUSY;
+}
+
  static void fsm_disabled_irq(struct vfio_ccw_private *private,
  			     enum vfio_ccw_event event)
  {

@@ -166,11 +257,11 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  		}
  		return;

drivers/s390/cio/vfio_ccw_fsm.c +3 -0

  	}

@@ -181,6 +272,59 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  			       io_region->ret_code, errstr);
  }
 
+/*
+ * Deal with a halt request from userspace.
+ */

Quoted 19 lines · Cornelia Huck

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
  drivers/s390/cio/Makefile           |   3 +-
  drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
  drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
  drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
  drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
  drivers/s390/cio/vfio_ccw_private.h |   6 ++
  include/uapi/linux/vfio.h           |   4 +
  include/uapi/linux/vfio_ccw.h       |  12 +++
  8 files changed, 313 insertions(+), 19 deletions(-)

create mode 100644 drivers/s390/cio/vfio_ccw_async.c

Quoted 29 lines

drivers/s390/cio/vfio_ccw_fsm.c +29 -0

+static int fsm_do_halt(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch;
+	unsigned long flags;
+	int ccode;
+	int ret;
+
+	sch = private->sch;
+
+	spin_lock_irqsave(sch->lock, flags);
+	private->state = VFIO_CCW_STATE_BUSY;
+
+	/* Issue "Halt Subchannel" */
+	ccode = hsch(sch->schid);
+
+	switch (ccode) {
+	case 0:
+		/*
+		 * Initialize device status information
+		 */
+		sch->schib.scsw.cmd.actl |= SCSW_ACTL_HALT_PEND;
+		ret = 0;
+		break;
+	case 1:		/* Status pending */
+	case 2:		/* Busy */
+		ret = -EBUSY;
+		break;
+	case 3:		/* Device not operational */
+	{

drivers/s390/cio/vfio_ccw_fsm.c static int fsm_io_helper(struct vfio_ccw_private *private) · line 107

Quoted 6 lines

drivers/s390/cio/vfio_ccw_fsm.c +6 -0

+		ret = -ENODEV;
+		break;
+	}
+	default:
+		ret = ccode;
+	}  

Shouldn't you set the state back here?

Quoted 31 lines

drivers/s390/cio/vfio_ccw_fsm.c +30 -0

+	spin_unlock_irqrestore(sch->lock, flags);
+	return ret;
+}
+
+static int fsm_do_clear(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch;
+	unsigned long flags;
+	int ccode;
+	int ret;
+
+	sch = private->sch;
+
+	spin_lock_irqsave(sch->lock, flags);
+	private->state = VFIO_CCW_STATE_BUSY;
+
+	/* Issue "Clear Subchannel" */
+	ccode = csch(sch->schid);
+
+	switch (ccode) {
+	case 0:
+		/*
+		 * Initialize device status information
+		 */
+		sch->schib.scsw.cmd.actl = SCSW_ACTL_CLEAR_PEND;
+		/* TODO: check what else we might need to clear */
+		ret = 0;
+		break;
+	case 3:		/* Device not operational */
+	{

drivers/s390/cio/vfio_ccw_fsm.c static int fsm_io_helper(struct vfio_ccw_private *private) · line 109

Quoted 8 lines

drivers/s390/cio/vfio_ccw_fsm.c +8 -0

+		ret = -ENODEV;
+		break;
+	}
+	default:
+		ret = ccode;
+	}
+	spin_unlock_irqrestore(sch->lock, flags);
+	return ret;

Quoted 34 lines

drivers/s390/cio/vfio_ccw_fsm.c +17 -1

+}
+
  static void fsm_notoper(struct vfio_ccw_private *private,
  			enum vfio_ccw_event event)
  {

@@ -102,6 +179,20 @@ static void fsm_io_busy(struct vfio_ccw_private *private,
  	private->io_region->ret_code = -EBUSY;
  }
 
+static void fsm_async_error(struct vfio_ccw_private *private,
+			    enum vfio_ccw_event event)
+{
+	pr_err("vfio-ccw: FSM: halt/clear request from state:%d\n",
+	       private->state);
+	private->cmd_region->ret_code = -EIO;
+}
+
+static void fsm_async_busy(struct vfio_ccw_private *private,
+			   enum vfio_ccw_event event)
+{
+	private->cmd_region->ret_code = -EBUSY;
+}
+
  static void fsm_disabled_irq(struct vfio_ccw_private *private,
  			     enum vfio_ccw_event event)
  {

@@ -166,11 +257,11 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  		}
  		return;
  	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
-		/* XXX: Handle halt. */
+		/* halt is handled via the async cmd region */
  		io_region->ret_code = -EOPNOTSUPP;
  		goto err_out;

drivers/s390/cio/vfio_ccw_fsm.c static void fsm_io_request(struct vfio_ccw_private *private, · line 266

Quoted 5 lines

drivers/s390/cio/vfio_ccw_fsm.c +1 -1

  	} else if (scsw->cmd.fctl & SCSW_FCTL_CLEAR_FUNC) {
-		/* XXX: Handle clear. */
+		/* clear is handled via the async cmd region */
  		io_region->ret_code = -EOPNOTSUPP;
  		goto err_out;  

What about filtering inside the vfio_ccw_mdev_write_io_region() before the call to the FSM?

Quoted 7 lines

drivers/s390/cio/vfio_ccw_fsm.c +0 -0

  	}

@@ -181,6 +272,59 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  			       io_region->ret_code, errstr);
  }
 

drivers/s390/cio/vfio_ccw_fsm.c static void fsm_io_request(struct vfio_ccw_private *private, · line 287

Quoted 13 lines

drivers/s390/cio/vfio_ccw_fsm.c +13 -0

+/*
+ * Deal with a halt request from userspace.
+ */
+static void fsm_halt_request(struct vfio_ccw_private *private,
+			     enum vfio_ccw_event event)
+{
+	struct ccw_cmd_region *cmd_region = private->cmd_region;
+	int state = private->state;
+
+	private->state = VFIO_CCW_STATE_BOXED;
+
+	if (cmd_region->command != VFIO_CCW_ASYNC_CMD_HSCH) {
+		/* should not happen? */  

I think we should make sure it does not happen before we get here. Like serializing HALT and CLEAR before the FSM.

drivers/s390/cio/vfio_ccw_fsm.c static void fsm_io_request(struct vfio_ccw_private *private, · line 292

Quoted 6 lines

drivers/s390/cio/vfio_ccw_fsm.c +5 -0

+		cmd_region->ret_code = -EINVAL;
+		goto err_out;
+	}
+
+	cmd_region->ret_code = fsm_do_halt(private);  

fsm_do_halt() set the state to BUSY.
Do we need a state change here and in fsm_do_halt ?

Why not only the BUSY state?

drivers/s390/cio/vfio_ccw_fsm.c static void fsm_io_request(struct vfio_ccw_private *private, · line 301

Quoted 10 lines

drivers/s390/cio/vfio_ccw_fsm.c +9 -0

+	if (cmd_region->ret_code)
+		goto err_out;
+
+	return;
+
+err_out:
+	private->state = state;
+}
+  

...snip...

Regards,
Pierre

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
  drivers/s390/cio/Makefile           |   3 +-
  drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
  drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
  drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
  drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
  drivers/s390/cio/vfio_ccw_private.h |   6 ++
  include/uapi/linux/vfio.h           |   4 +
  include/uapi/linux/vfio_ccw.h       |  12 +++
  8 files changed, 313 insertions(+), 19 deletions(-)

create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/Makefile +2 -1

index f230516abb96..f6a8db04177c 100644
@@ -20,5 +20,6 @@ obj-$(CONFIG_CCWGROUP) += ccwgroup.o
  qdio-objs := qdio_main.o qdio_thinint.o qdio_debug.o qdio_setup.o
  obj-$(CONFIG_QDIO) += qdio.o
 
-vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o
+vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o \
+	vfio_ccw_async.o
  obj-$(CONFIG_VFIO_CCW) += vfio_ccw.o

drivers/s390/cio/vfio_ccw_async.c new file +88 -0

new file mode 100644
index 000000000000..8c7f51d17d70
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Async I/O region for vfio_ccw
+ *
+ * Copyright Red Hat, Inc. 2018
+ *
+ * Author(s): Cornelia Huck <[email protected]>
+ */
+
+#include <linux/vfio.h>
+#include <linux/mdev.h>
+
+#include "vfio_ccw_private.h"
+
+static size_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
+					 char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	region = private->region[i].data;
+	if (copy_to_user(buf, (void *)region + pos, count))
+		return -EFAULT;
+
+	return count;
+
+}
+
+static size_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
+					  const char __user *buf, size_t count,
+					  loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
+		return -EACCES;
+
+	region = private->region[i].data;
+	if (copy_from_user((void *)region + pos, buf, count))
+		return -EFAULT;
+
+	switch (region->command) {
+	case VFIO_CCW_ASYNC_CMD_HSCH:
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_HALT_REQ);
+		break;
+	case VFIO_CCW_ASYNC_CMD_CSCH:
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLEAR_REQ);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return region->ret_code ? region->ret_code : count;
+}
+
+static void vfio_ccw_async_region_release(struct vfio_ccw_private *private,
+					  struct vfio_ccw_region *region)
+{
+
+}
+
+const struct vfio_ccw_regops vfio_ccw_async_region_ops = {
+	.read = vfio_ccw_async_region_read,
+	.write = vfio_ccw_async_region_write,
+	.release = vfio_ccw_async_region_release,
+};
+
+int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private)
+{
+	return vfio_ccw_register_dev_region(private,
+					    VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD,
+					    &vfio_ccw_async_region_ops,
+					    sizeof(struct ccw_cmd_region),
+					    VFIO_REGION_INFO_FLAG_READ |
+					    VFIO_REGION_INFO_FLAG_WRITE,
+					    private->cmd_region);
+}

drivers/s390/cio/vfio_ccw_drv.c +3 -0

index a10cec0e86eb..890c588a3a61 100644
@@ -3,9 +3,11 @@
   * VFIO based Physical Subchannel device driver
   *
   * Copyright IBM Corp. 2017
+ * Copyright Red Hat, Inc. 2018
   *
   * Author(s): Dong Jia Shi <[email protected]>
   *            Xiao Feng Ren <[email protected]>
+ *            Cornelia Huck <[email protected]>
   */
 
  #include <linux/module.h>

@@ -23,6 +25,7 @@
 
  struct workqueue_struct *vfio_ccw_work_q;
  static struct kmem_cache *vfio_ccw_io_region;
+static struct kmem_cache *vfio_ccw_cmd_region;
 
  /*
   * Helpers

drivers/s390/cio/vfio_ccw_drv.c +29 -13

@@ -104,7 +108,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
  {
  	struct pmcw *pmcw = &sch->schib.pmcw;
  	struct vfio_ccw_private *private;
-	int ret;
+	int ret = -ENOMEM;
 
  	if (pmcw->qf) {
  		dev_warn(&sch->dev, "vfio: ccw: does not support QDIO: %s\n",

@@ -118,10 +122,13 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 
  	private->io_region = kmem_cache_zalloc(vfio_ccw_io_region,
  					       GFP_KERNEL | GFP_DMA);
-	if (!private->io_region) {
-		kfree(private);
-		return -ENOMEM;
-	}
+	if (!private->io_region)
+		goto out_free;
+
+	private->cmd_region = kmem_cache_zalloc(vfio_ccw_cmd_region,
+						GFP_KERNEL | GFP_DMA);
+	if (!private->cmd_region)
+		goto out_free;
 
  	private->sch = sch;
  	dev_set_drvdata(&sch->dev, private);

@@ -148,7 +155,10 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
  	cio_disable_subchannel(sch);
  out_free:
  	dev_set_drvdata(&sch->dev, NULL);
-	kmem_cache_free(vfio_ccw_io_region, private->io_region);
+	if (private->cmd_region)
+		kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
+	if (private->io_region)
+		kmem_cache_free(vfio_ccw_io_region, private->io_region);
  	kfree(private);
  	return ret;
  }

@@ -237,7 +247,7 @@ static struct css_driver vfio_ccw_sch_driver = {
 
  static int __init vfio_ccw_sch_init(void)
  {
-	int ret;
+	int ret = -ENOMEM;
 
  	vfio_ccw_work_q = create_singlethread_workqueue("vfio-ccw");
  	if (!vfio_ccw_work_q)

@@ -247,20 +257,30 @@ static int __init vfio_ccw_sch_init(void)
  					sizeof(struct ccw_io_region), 0,
  					SLAB_ACCOUNT, 0,
  					sizeof(struct ccw_io_region), NULL);
-	if (!vfio_ccw_io_region) {
-		destroy_workqueue(vfio_ccw_work_q);
-		return -ENOMEM;
-	}
+	if (!vfio_ccw_io_region)
+		goto out_err;
+
+	vfio_ccw_cmd_region = kmem_cache_create_usercopy("vfio_ccw_cmd_region",
+					sizeof(struct ccw_cmd_region), 0,
+					SLAB_ACCOUNT, 0,
+					sizeof(struct ccw_cmd_region), NULL);
+	if (!vfio_ccw_cmd_region)
+		goto out_err;
 
  	isc_register(VFIO_CCW_ISC);
  	ret = css_driver_register(&vfio_ccw_sch_driver);
  	if (ret) {
  		isc_unregister(VFIO_CCW_ISC);
-		kmem_cache_destroy(vfio_ccw_io_region);
-		destroy_workqueue(vfio_ccw_work_q);
+		goto out_err;
  	}
 
  	return ret;
+
+out_err:
+	kmem_cache_destroy(vfio_ccw_cmd_region);
+	kmem_cache_destroy(vfio_ccw_io_region);
+	destroy_workqueue(vfio_ccw_work_q);
+	return ret;
  }
 
  static void __exit vfio_ccw_sch_exit(void)

drivers/s390/cio/vfio_ccw_fsm.c +156 -2

index f94aa01f9c36..0caf77e8f377 100644
@@ -3,8 +3,10 @@
   * Finite state machine for vfio-ccw device handling
   *
   * Copyright IBM Corp. 2017
+ * Copyright Red Hat, Inc. 2018
   *
   * Author(s): Dong Jia Shi <[email protected]>
+ *            Cornelia Huck <[email protected]>
   */
 
  #include <linux/vfio.h>

@@ -68,6 +70,81 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
  	return ret;
  }
 
+static int fsm_do_halt(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch;
+	unsigned long flags;
+	int ccode;
+	int ret;
+
+	sch = private->sch;
+
+	spin_lock_irqsave(sch->lock, flags);
+	private->state = VFIO_CCW_STATE_BUSY;
+
+	/* Issue "Halt Subchannel" */
+	ccode = hsch(sch->schid);
+
+	switch (ccode) {
+	case 0:
+		/*
+		 * Initialize device status information
+		 */
+		sch->schib.scsw.cmd.actl |= SCSW_ACTL_HALT_PEND;
+		ret = 0;
+		break;
+	case 1:		/* Status pending */
+	case 2:		/* Busy */
+		ret = -EBUSY;
+		break;
+	case 3:		/* Device not operational */
+	{
+		ret = -ENODEV;
+		break;
+	}
+	default:
+		ret = ccode;
+	}
+	spin_unlock_irqrestore(sch->lock, flags);
+	return ret;
+}
+
+static int fsm_do_clear(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch;
+	unsigned long flags;
+	int ccode;
+	int ret;
+
+	sch = private->sch;
+
+	spin_lock_irqsave(sch->lock, flags);
+	private->state = VFIO_CCW_STATE_BUSY;
+
+	/* Issue "Clear Subchannel" */
+	ccode = csch(sch->schid);
+
+	switch (ccode) {
+	case 0:
+		/*
+		 * Initialize device status information
+		 */
+		sch->schib.scsw.cmd.actl = SCSW_ACTL_CLEAR_PEND;
+		/* TODO: check what else we might need to clear */
+		ret = 0;
+		break;
+	case 3:		/* Device not operational */
+	{
+		ret = -ENODEV;
+		break;
+	}
+	default:
+		ret = ccode;
+	}
+	spin_unlock_irqrestore(sch->lock, flags);
+	return ret;
+}
+
  static void fsm_notoper(struct vfio_ccw_private *private,
  			enum vfio_ccw_event event)
  {

@@ -102,6 +179,20 @@ static void fsm_io_busy(struct vfio_ccw_private *private,
  	private->io_region->ret_code = -EBUSY;
  }
 
+static void fsm_async_error(struct vfio_ccw_private *private,
+			    enum vfio_ccw_event event)
+{
+	pr_err("vfio-ccw: FSM: halt/clear request from state:%d\n",
+	       private->state);
+	private->cmd_region->ret_code = -EIO;
+}
+
+static void fsm_async_busy(struct vfio_ccw_private *private,
+			   enum vfio_ccw_event event)
+{
+	private->cmd_region->ret_code = -EBUSY;
+}
+
  static void fsm_disabled_irq(struct vfio_ccw_private *private,
  			     enum vfio_ccw_event event)
  {

@@ -166,11 +257,11 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  		}
  		return;
  	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
-		/* XXX: Handle halt. */
+		/* halt is handled via the async cmd region */
  		io_region->ret_code = -EOPNOTSUPP;
  		goto err_out;
  	} else if (scsw->cmd.fctl & SCSW_FCTL_CLEAR_FUNC) {
-		/* XXX: Handle clear. */
+		/* clear is handled via the async cmd region */
  		io_region->ret_code = -EOPNOTSUPP;
  		goto err_out;
  	}

@@ -181,6 +272,59 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  			       io_region->ret_code, errstr);
  }
 
+/*
+ * Deal with a halt request from userspace.
+ */
+static void fsm_halt_request(struct vfio_ccw_private *private,
+			     enum vfio_ccw_event event)
+{
+	struct ccw_cmd_region *cmd_region = private->cmd_region;
+	int state = private->state;
+
+	private->state = VFIO_CCW_STATE_BOXED;
+
+	if (cmd_region->command != VFIO_CCW_ASYNC_CMD_HSCH) {
+		/* should not happen? */
+		cmd_region->ret_code = -EINVAL;
+		goto err_out;
+	}
+
+	cmd_region->ret_code = fsm_do_halt(private);
+	if (cmd_region->ret_code)
+		goto err_out;
+
+	return;
+
+err_out:
+	private->state = state;
+}
+
+/*
+ * Deal with a clear request from userspace.
+ */
+static void fsm_clear_request(struct vfio_ccw_private *private,
+			      enum vfio_ccw_event event)
+{
+	struct ccw_cmd_region *cmd_region = private->cmd_region;
+	int state = private->state;
+
+	private->state = VFIO_CCW_STATE_BOXED;
+
+	if (cmd_region->command != VFIO_CCW_ASYNC_CMD_CSCH) {
+		/* should not happen? */
+		cmd_region->ret_code = -EINVAL;
+		goto err_out;
+	}
+
+	cmd_region->ret_code = fsm_do_clear(private);
+	if (cmd_region->ret_code)
+		goto err_out;
+
+	return;
+
+err_out:
+	private->state = state;
+}
  /*
   * Got an interrupt for a normal io (state busy).
   */

@@ -204,26 +348,36 @@ 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_HALT_REQ]	= fsm_async_error,
+		[VFIO_CCW_EVENT_CLEAR_REQ]	= fsm_async_error,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
  	},
  	[VFIO_CCW_STATE_STANDBY] = {
  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
+		[VFIO_CCW_EVENT_HALT_REQ]	= fsm_async_error,
+		[VFIO_CCW_EVENT_CLEAR_REQ]	= fsm_async_error,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
  	},
  	[VFIO_CCW_STATE_IDLE] = {
  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_request,
+		[VFIO_CCW_EVENT_HALT_REQ]	= fsm_halt_request,
+		[VFIO_CCW_EVENT_CLEAR_REQ]	= fsm_clear_request,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
  	},
  	[VFIO_CCW_STATE_BOXED] = {
  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
+		[VFIO_CCW_EVENT_HALT_REQ]	= fsm_async_busy,
+		[VFIO_CCW_EVENT_CLEAR_REQ]	= fsm_async_busy,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
  	},
  	[VFIO_CCW_STATE_BUSY] = {
  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
+		[VFIO_CCW_EVENT_HALT_REQ]	= fsm_halt_request,
+		[VFIO_CCW_EVENT_CLEAR_REQ]	= fsm_clear_request,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
  	},
  };

drivers/s390/cio/vfio_ccw_ops.c +11 -2

index a5d731ed2a39..0e1f7f7bf927 100644
@@ -148,11 +148,20 @@ 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;
+
+	ret = vfio_ccw_register_async_dev_regions(private);
+	if (ret)
+		vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+					 &private->nb);
+	return ret;
  }
 
  static void vfio_ccw_mdev_release(struct mdev_device *mdev)

drivers/s390/cio/vfio_ccw_private.h +6 -0

index a6f9f84526e2..1a41a14831ae 100644
@@ -53,6 +53,8 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
  				 const struct vfio_ccw_regops *ops,
  				 size_t size, u32 flags, void *data);
 
+int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
+
  /**
   * struct vfio_ccw_private
   * @sch: pointer to the subchannel

@@ -62,6 +64,7 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
   * @mdev: pointer to the mediated device
   * @nb: notifier for vfio events
   * @io_region: MMIO region to input/output I/O arguments/results
+ * @cmd_region: MMIO region for asynchronous I/O commands other than START
   * @region: additional regions for other subchannel operations
   * @num_regions: number of additional regions
   * @cp: channel program for the current I/O operation

@@ -79,6 +82,7 @@ struct vfio_ccw_private {
  	struct notifier_block	nb;
  	struct ccw_io_region	*io_region;
  	struct vfio_ccw_region *region;
+	struct ccw_cmd_region	*cmd_region;
  	int num_regions;
 
  	struct channel_program	cp;

@@ -114,6 +118,8 @@ enum vfio_ccw_event {
  	VFIO_CCW_EVENT_NOT_OPER,
  	VFIO_CCW_EVENT_IO_REQ,
  	VFIO_CCW_EVENT_INTERRUPT,
+	VFIO_CCW_EVENT_HALT_REQ,
+	VFIO_CCW_EVENT_CLEAR_REQ,
  	/* last element! */
  	NR_VFIO_CCW_EVENTS
  };

include/uapi/linux/vfio.h +4 -0

index 565669f95534..c01472ec77ea 100644
@@ -304,6 +304,7 @@ struct vfio_region_info_cap_type {
  #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG	(2)
  #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG	(3)
 
+
  #define VFIO_REGION_TYPE_GFX                    (1)
  #define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
 

@@ -354,6 +355,9 @@ struct vfio_region_gfx_edid {
  #define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
  };
 
+/* ccw sub-types */
+#define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD	(1)
+
  /*
   * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
   * which allows direct access to non-MSIX registers which happened to be within

include/uapi/linux/vfio_ccw.h +12 -0

index 2ec5f367ff78..cbecbf0cd54f 100644
@@ -12,6 +12,7 @@
 
  #include <linux/types.h>
 
+/* used for START SUBCHANNEL, always present */
  struct ccw_io_region {
  #define ORB_AREA_SIZE 12
  	__u8	orb_area[ORB_AREA_SIZE];

@@ -22,4 +23,15 @@ struct ccw_io_region {
  	__u32	ret_code;
  } __packed;
 
+/*
+ * used for processing commands that trigger asynchronous actions
+ * Note: this is controlled by a capability
+ */
+#define VFIO_CCW_ASYNC_CMD_HSCH (1 << 0)
+#define VFIO_CCW_ASYNC_CMD_CSCH (1 << 1)
+struct ccw_cmd_region {
+	__u32 command;
+	__u32 ret_code;
+} __packed;
+
  #endif

Quoted 25 lines · Cornelia Huck

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
  drivers/s390/cio/Makefile           |   3 +-
  drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
  drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
  drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
  drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
  drivers/s390/cio/vfio_ccw_private.h |   6 ++
  include/uapi/linux/vfio.h           |   4 +
  include/uapi/linux/vfio_ccw.h       |  12 +++
  8 files changed, 313 insertions(+), 19 deletions(-)

create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/vfio_ccw_drv.c +0 -0

@@ -76,7 +79,8 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
  	private = container_of(work, struct vfio_ccw_private, io_work);
  	irb = &private->irb;
 

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 86

Quoted 6 lines

drivers/s390/cio/vfio_ccw_drv.c +2 -1

-	if (scsw_is_solicited(&irb->scsw)) {
+	if (scsw_is_solicited(&irb->scsw) &&
+	    (scsw_fctl(&irb->scsw) & SCSW_FCTL_START_FUNC)) {
  		cp_update_scsw(&private->cp, &irb->scsw);
  		cp_free(&private->cp);
  	}  

I am a little confused about this. Why do we need to update the scsw.cpa if we have the start function function control bit set? Is it an optimization?

Quoted 26 lines · On Tue, 27 Nov 2018 14:09:49 -0500 Farhan Ali <[email protected]> wrote:

Quoted 25 lines · Cornelia Huck

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
   drivers/s390/cio/Makefile           |   3 +-
   drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
   drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
   drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
   drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
   drivers/s390/cio/vfio_ccw_private.h |   6 ++
   include/uapi/linux/vfio.h           |   4 +
   include/uapi/linux/vfio_ccw.h       |  12 +++
   8 files changed, 313 insertions(+), 19 deletions(-)

create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/vfio_ccw_drv.c +0 -0

@@ -76,7 +79,8 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
   	private = container_of(work, struct vfio_ccw_private, io_work);
   	irb = &private->irb;
 

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 86

Quoted 6 lines

drivers/s390/cio/vfio_ccw_drv.c +2 -1

-	if (scsw_is_solicited(&irb->scsw)) {
+	if (scsw_is_solicited(&irb->scsw) &&
+	    (scsw_fctl(&irb->scsw) & SCSW_FCTL_START_FUNC)) {
   		cp_update_scsw(&private->cp, &irb->scsw);
   		cp_free(&private->cp);
   	}

I am a little confused about this. Why do we need to update the scsw.cpa if we have the start function function control bit set? Is it an optimization?

No, it's not an optimization. This is the work function that is scheduled if we get an interrupt for the device. Previously, we only got an interrupt if either the device presented us an unsolicited status or if we got an interrupt as a response to the channel program we submitted. Now, we can get an interrupt for halt/clear subchannel as well, and in that case, we don't necessarily have a cp.

[Thinking some more about it, we need to verify if the start function actually remains set if we try to terminate a running channel program with halt/clear. A clear might scrub too much. If that's the case, we also need to free the cp if the start function is not set.]

Quoted 47 lines · Cornelia Huck

Quoted 26 lines · On Tue, 27 Nov 2018 14:09:49 -0500 Farhan Ali <[email protected]> wrote:

Quoted 25 lines · Cornelia Huck

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
   drivers/s390/cio/Makefile           |   3 +-
   drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
   drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
   drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
   drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
   drivers/s390/cio/vfio_ccw_private.h |   6 ++
   include/uapi/linux/vfio.h           |   4 +
   include/uapi/linux/vfio_ccw.h       |  12 +++
   8 files changed, 313 insertions(+), 19 deletions(-)

create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/vfio_ccw_drv.c +0 -0

@@ -76,7 +79,8 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
   	private = container_of(work, struct vfio_ccw_private, io_work);
   	irb = &private->irb;
 

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 86

Quoted 6 lines

drivers/s390/cio/vfio_ccw_drv.c +2 -1

-	if (scsw_is_solicited(&irb->scsw)) {
+	if (scsw_is_solicited(&irb->scsw) &&
+	    (scsw_fctl(&irb->scsw) & SCSW_FCTL_START_FUNC)) {
   		cp_update_scsw(&private->cp, &irb->scsw);
   		cp_free(&private->cp);
   	}  

I am a little confused about this. Why do we need to update the scsw.cpa if we have the start function function control bit set? Is it an optimization?

No, it's not an optimization. This is the work function that is scheduled if we get an interrupt for the device. Previously, we only got an interrupt if either the device presented us an unsolicited status or if we got an interrupt as a response to the channel program we submitted. Now, we can get an interrupt for halt/clear subchannel as well, and in that case, we don't necessarily have a cp.

Quoted 6 lines

[Thinking some more about it, we need to verify if the start function actually remains set if we try to terminate a running channel program with halt/clear. A clear might scrub too much. If that's the case, we also need to free the cp if the start function is not set.]

According to PoPs (Chapter 16: I/O interruptions, under function control):

The start-function indication is also cleared at
the subchannel during the execution of CLEAR SUB-
CHANNEL.

So maybe we do need to free the cp.

Quoted 61 lines · On Wed, 28 Nov 2018 09:31:51 -0500 Farhan Ali <[email protected]> wrote:

Quoted 47 lines · Cornelia Huck

Quoted 26 lines · On Tue, 27 Nov 2018 14:09:49 -0500 Farhan Ali <[email protected]> wrote:

Quoted 25 lines · Cornelia Huck

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
    drivers/s390/cio/Makefile           |   3 +-
    drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
    drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
    drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
    drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
    drivers/s390/cio/vfio_ccw_private.h |   6 ++
    include/uapi/linux/vfio.h           |   4 +
    include/uapi/linux/vfio_ccw.h       |  12 +++
    8 files changed, 313 insertions(+), 19 deletions(-)
    create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/vfio_ccw_drv.c +0 -0

@@ -76,7 +79,8 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
    	private = container_of(work, struct vfio_ccw_private, io_work);
    	irb = &private->irb;
 

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 86

Quoted 6 lines

drivers/s390/cio/vfio_ccw_drv.c +2 -1

-	if (scsw_is_solicited(&irb->scsw)) {
+	if (scsw_is_solicited(&irb->scsw) &&
+	    (scsw_fctl(&irb->scsw) & SCSW_FCTL_START_FUNC)) {
    		cp_update_scsw(&private->cp, &irb->scsw);
    		cp_free(&private->cp);
    	}

I am a little confused about this. Why do we need to update the scsw.cpa if we have the start function function control bit set? Is it an optimization?

No, it's not an optimization. This is the work function that is scheduled if we get an interrupt for the device. Previously, we only got an interrupt if either the device presented us an unsolicited status or if we got an interrupt as a response to the channel program we submitted. Now, we can get an interrupt for halt/clear subchannel as well, and in that case, we don't necessarily have a cp.

Quoted 6 lines

[Thinking some more about it, we need to verify if the start function actually remains set if we try to terminate a running channel program with halt/clear. A clear might scrub too much. If that's the case, we also need to free the cp if the start function is not set.]

According to PoPs (Chapter 16: I/O interruptions, under function control):

The start-function indication is also cleared at
the subchannel during the execution of CLEAR SUB-
CHANNEL.

Quoted 1 line

So maybe we do need to free the cp.

Hm... so we need to make sure that cp_update_scsw() and cp_free() only do something when there's actually a valid cp around and call them unconditionally.

Quoted 64 lines · Cornelia Huck

Quoted 61 lines · On Wed, 28 Nov 2018 09:31:51 -0500 Farhan Ali <[email protected]> wrote:

Quoted 47 lines · Cornelia Huck

Quoted 26 lines · On Tue, 27 Nov 2018 14:09:49 -0500 Farhan Ali <[email protected]> wrote:

Quoted 25 lines · Cornelia Huck

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
    drivers/s390/cio/Makefile           |   3 +-
    drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
    drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
    drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
    drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
    drivers/s390/cio/vfio_ccw_private.h |   6 ++
    include/uapi/linux/vfio.h           |   4 +
    include/uapi/linux/vfio_ccw.h       |  12 +++
    8 files changed, 313 insertions(+), 19 deletions(-)
    create mode 100644 drivers/s390/cio/vfio_ccw_async.c

(quoted diff) +0 -0

@@ -76,7 +79,8 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
    	private = container_of(work, struct vfio_ccw_private, io_work);
    	irb = &private->irb;
 

Quoted 10 lines

Review static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 86

Quoted 6 lines

(quoted diff) +2 -1

-	if (scsw_is_solicited(&irb->scsw)) {
+	if (scsw_is_solicited(&irb->scsw) &&
+	    (scsw_fctl(&irb->scsw) & SCSW_FCTL_START_FUNC)) {
    		cp_update_scsw(&private->cp, &irb->scsw);
    		cp_free(&private->cp);
    	}  

I am a little confused about this. Why do we need to update the scsw.cpa if we have the start function function control bit set? Is it an optimization?

No, it's not an optimization. This is the work function that is scheduled if we get an interrupt for the device. Previously, we only got an interrupt if either the device presented us an unsolicited status or if we got an interrupt as a response to the channel program we submitted. Now, we can get an interrupt for halt/clear subchannel as well, and in that case, we don't necessarily have a cp.

Quoted 6 lines

[Thinking some more about it, we need to verify if the start function actually remains set if we try to terminate a running channel program with halt/clear. A clear might scrub too much. If that's the case, we also need to free the cp if the start function is not set.]

According to PoPs (Chapter 16: I/O interruptions, under function control):

The start-function indication is also cleared at
the subchannel during the execution of CLEAR SUB-
CHANNEL.

Quoted 5 lines

Quoted 1 line

So maybe we do need to free the cp.

Hm... so we need to make sure that cp_update_scsw() and cp_free() only do something when there's actually a valid cp around and call them unconditionally.

Yes, I agree.

Quoted 1 line

Maybe add a ->valid flag to struct channel_program?

We could do that. So we would set the flag once we have copied the channel program to kernel memory? since that's when we should care about freeing it.

Quoted 71 lines · On Wed, 28 Nov 2018 10:00:59 -0500 Farhan Ali <[email protected]> wrote:

Quoted 64 lines · Cornelia Huck

Quoted 61 lines · On Wed, 28 Nov 2018 09:31:51 -0500 Farhan Ali <[email protected]> wrote:

Quoted 47 lines · Cornelia Huck

Quoted 26 lines · On Tue, 27 Nov 2018 14:09:49 -0500 Farhan Ali <[email protected]> wrote:

Quoted 25 lines · Cornelia Huck

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
     drivers/s390/cio/Makefile           |   3 +-
     drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
     drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
     drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
     drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
     drivers/s390/cio/vfio_ccw_private.h |   6 ++
     include/uapi/linux/vfio.h           |   4 +
     include/uapi/linux/vfio_ccw.h       |  12 +++
     8 files changed, 313 insertions(+), 19 deletions(-)
     create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/vfio_ccw_drv.c +0 -0

@@ -76,7 +79,8 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
     	private = container_of(work, struct vfio_ccw_private, io_work);
     	irb = &private->irb;
 

Quoted 10 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 85

Quoted 6 lines

drivers/s390/cio/vfio_ccw_drv.c +2 -1

-	if (scsw_is_solicited(&irb->scsw)) {
+	if (scsw_is_solicited(&irb->scsw) &&
+	    (scsw_fctl(&irb->scsw) & SCSW_FCTL_START_FUNC)) {
     		cp_update_scsw(&private->cp, &irb->scsw);
     		cp_free(&private->cp);
     	}

I am a little confused about this. Why do we need to update the scsw.cpa if we have the start function function control bit set? Is it an optimization?

No, it's not an optimization. This is the work function that is scheduled if we get an interrupt for the device. Previously, we only got an interrupt if either the device presented us an unsolicited status or if we got an interrupt as a response to the channel program we submitted. Now, we can get an interrupt for halt/clear subchannel as well, and in that case, we don't necessarily have a cp.

Quoted 6 lines

[Thinking some more about it, we need to verify if the start function actually remains set if we try to terminate a running channel program with halt/clear. A clear might scrub too much. If that's the case, we also need to free the cp if the start function is not set.]

According to PoPs (Chapter 16: I/O interruptions, under function control):

The start-function indication is also cleared at
the subchannel during the execution of CLEAR SUB-
CHANNEL.

Quoted 5 lines

Quoted 1 line

So maybe we do need to free the cp.

Hm... so we need to make sure that cp_update_scsw() and cp_free() only do something when there's actually a valid cp around and call them unconditionally.

Quoted 7 lines

Yes, I agree.

Quoted 1 line

Maybe add a ->valid flag to struct channel_program?

We could do that. So we would set the flag once we have copied the channel program to kernel memory? since that's when we should care about freeing it.

I hacked up the following (still untested):

From e771c8dc5abbfbd19688b452096bab9d032e0df5 Mon Sep 17 00:00:00 2001
From: Cornelia Huck <[email protected]>
Date: Wed, 28 Nov 2018 16:30:51 +0100
Subject: [PATCH] vfio-ccw: make it safe to access channel programs

When we get a solicited interrupt, the start function may have been cleared by a csch, but we still have a channel program structure allocated. Make it safe to call the cp accessors in any case, so we can call them unconditionally.

Signed-off-by Cornelia Huck <[email protected]>

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

drivers/s390/cio/vfio_ccw_cp.c +8 -1

index 70a006ba4d05..35f87514276b 100644
@@ -335,6 +335,7 @@ static void cp_unpin_free(struct channel_program *cp)
  	struct ccwchain *chain, *temp;
  	int i;
 
+	cp->initialized = false;
  	list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
  		for (i = 0; i < chain->ch_len; i++) {
  			pfn_array_table_unpin_free(chain->ch_pat + i,

@@ -701,6 +702,8 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
  	 */
  	cp->orb.cmd.c64 = 1;
 
+	cp->initialized = true;
+
  	return ret;
  }
 

@@ -715,7 +718,8 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
   */
  void cp_free(struct channel_program *cp)
  {
-	cp_unpin_free(cp);
+	if (cp->initialized)
+		cp_unpin_free(cp);
  }
 
  /**

@@ -831,6 +835,9 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
  	u32 cpa = scsw->cmd.cpa;
  	u32 ccw_head, ccw_tail;
 
+	if (!cp->initialized)
+		return;
+
  	/*
  	 * LATER:
  	 * For now, only update the cmd.cpa part. We may need to deal with

drivers/s390/cio/vfio_ccw_cp.h +2 -0

index a4b74fb1aa57..3c20cd208da5 100644
@@ -21,6 +21,7 @@
   * @ccwchain_list: list head of ccwchains
   * @orb: orb for the currently processed ssch request
   * @mdev: the mediated device to perform page pinning/unpinning
+ * @initialized: whether this instance is actually initialized
   *
   * @ccwchain_list is the head of a ccwchain list, that contents the
   * translated result of the guest channel program that pointed out by

@@ -30,6 +31,7 @@ struct channel_program {
  	struct list_head ccwchain_list;
  	union orb orb;
  	struct device *mdev;
+	bool initialized;
  };
 
  extern int cp_init(struct channel_program *cp, struct device *mdev,

drivers/s390/cio/vfio_ccw_drv.c +0 -0

index 890c588a3a61..83d6f43792b6 100644

Quoted 12 lines · Cornelia Huck

I hacked up the following (still untested):

From e771c8dc5abbfbd19688b452096bab9d032e0df5 Mon Sep 17 00:00:00 2001
From: Cornelia Huck <[email protected]>
Date: Wed, 28 Nov 2018 16:30:51 +0100
Subject: [PATCH] vfio-ccw: make it safe to access channel programs

When we get a solicited interrupt, the start function may have been cleared by a csch, but we still have a channel program structure allocated. Make it safe to call the cp accessors in any case, so we can call them unconditionally.

Quoted 6 lines

Signed-off-by Cornelia Huck <[email protected]>

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

Quoted 70 lines

drivers/s390/cio/vfio_ccw_cp.c +8 -1

index 70a006ba4d05..35f87514276b 100644
@@ -335,6 +335,7 @@ static void cp_unpin_free(struct channel_program *cp)
  	struct ccwchain *chain, *temp;
  	int i;
 
+	cp->initialized = false;
  	list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
  		for (i = 0; i < chain->ch_len; i++) {
  			pfn_array_table_unpin_free(chain->ch_pat + i,

@@ -701,6 +702,8 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
  	 */
  	cp->orb.cmd.c64 = 1;
 
+	cp->initialized = true;
+
  	return ret;
  }
 

@@ -715,7 +718,8 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
   */
  void cp_free(struct channel_program *cp)
  {
-	cp_unpin_free(cp);
+	if (cp->initialized)
+		cp_unpin_free(cp);
  }
 
  /**

@@ -831,6 +835,9 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
  	u32 cpa = scsw->cmd.cpa;
  	u32 ccw_head, ccw_tail;
 
+	if (!cp->initialized)
+		return;
+
  	/*
  	 * LATER:
  	 * For now, only update the cmd.cpa part. We may need to deal with

drivers/s390/cio/vfio_ccw_cp.h +2 -0

index a4b74fb1aa57..3c20cd208da5 100644
@@ -21,6 +21,7 @@
   * @ccwchain_list: list head of ccwchains
   * @orb: orb for the currently processed ssch request
   * @mdev: the mediated device to perform page pinning/unpinning
+ * @initialized: whether this instance is actually initialized
   *
   * @ccwchain_list is the head of a ccwchain list, that contents the
   * translated result of the guest channel program that pointed out by

@@ -30,6 +31,7 @@ struct channel_program {
  	struct list_head ccwchain_list;
  	union orb orb;
  	struct device *mdev;
+	bool initialized;
  };
 
  extern int cp_init(struct channel_program *cp, struct device *mdev,

drivers/s390/cio/vfio_ccw_drv.c +0 -0

index 890c588a3a61..83d6f43792b6 100644
@@ -79,8 +79,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
  	private = container_of(work, struct vfio_ccw_private, io_work);
  	irb = &private->irb;
 

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 85

Quoted 7 lines

drivers/s390/cio/vfio_ccw_drv.c +1 -2

-	if (scsw_is_solicited(&irb->scsw) &&
-	    (scsw_fctl(&irb->scsw) & SCSW_FCTL_START_FUNC)) {
+	if (scsw_is_solicited(&irb->scsw)) {
  		cp_update_scsw(&private->cp, &irb->scsw);
  		cp_free(&private->cp);
  	}

The changes look good to me.

Thanks
Farhan

include/uapi/linux/vfio.h +3 -0

  #define VFIO_REGION_TYPE_GFX                    (1)
  #define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
 

@@ -354,6 +355,9 @@ struct vfio_region_gfx_edid {
  #define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
  };
 
+/* ccw sub-types */
+#define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD	(1)
+

Quoted 8 lines · Cornelia Huck

include/uapi/linux/vfio.h +0 -0

index 565669f95534..c01472ec77ea 100644
@@ -304,6 +304,7 @@ struct vfio_region_info_cap_type {
  #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG	(2)
  #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG	(3)
 

include/uapi/linux/vfio.h struct vfio_region_info_cap_type { · line 307

Quoted 1 line

include/uapi/linux/vfio.h +1 -0

+  

Whitespace error?

Quoted 11 lines

include/uapi/linux/vfio.h +3 -0

  #define VFIO_REGION_TYPE_GFX                    (1)
  #define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
 

@@ -354,6 +355,9 @@ struct vfio_region_gfx_edid {
  #define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
  };
 
+/* ccw sub-types */
+#define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD	(1)
+  

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
 drivers/s390/cio/Makefile           |   3 +-
 drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
 drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
 drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
 drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
 drivers/s390/cio/vfio_ccw_private.h |   6 ++
 include/uapi/linux/vfio.h           |   4 +
 include/uapi/linux/vfio_ccw.h       |  12 +++
 8 files changed, 313 insertions(+), 19 deletions(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/Makefile +2 -1

index f230516abb96..f6a8db04177c 100644
@@ -20,5 +20,6 @@ obj-$(CONFIG_CCWGROUP) += ccwgroup.o
 qdio-objs := qdio_main.o qdio_thinint.o qdio_debug.o qdio_setup.o
 obj-$(CONFIG_QDIO) += qdio.o
 
-vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o
+vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o \
+	vfio_ccw_async.o
 obj-$(CONFIG_VFIO_CCW) += vfio_ccw.o

drivers/s390/cio/vfio_ccw_async.c new file +41 -0

new file mode 100644
index 000000000000..8c7f51d17d70
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Async I/O region for vfio_ccw
+ *
+ * Copyright Red Hat, Inc. 2018
+ *
+ * Author(s): Cornelia Huck <[email protected]>
+ */
+
+#include <linux/vfio.h>
+#include <linux/mdev.h>
+
+#include "vfio_ccw_private.h"
+
+static size_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
+					 char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	region = private->region[i].data;
+	if (copy_to_user(buf, (void *)region + pos, count))
+		return -EFAULT;
+
+	return count;
+
+}
+
+static size_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
+					  const char __user *buf, size_t count,
+					  loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+

drivers/s390/cio/vfio_ccw_async.c new file +15 -0

+
+	switch (region->command) {
+	case VFIO_CCW_ASYNC_CMD_HSCH:
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_HALT_REQ);
+		break;
+	case VFIO_CCW_ASYNC_CMD_CSCH:
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLEAR_REQ);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return region->ret_code ? region->ret_code : count;
+}
+

Quoted 47 lines · On Thu, 22 Nov 2018 17:54:32 +0100 Cornelia Huck <[email protected]> wrote:

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
 drivers/s390/cio/Makefile           |   3 +-
 drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
 drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
 drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
 drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
 drivers/s390/cio/vfio_ccw_private.h |   6 ++
 include/uapi/linux/vfio.h           |   4 +
 include/uapi/linux/vfio_ccw.h       |  12 +++
 8 files changed, 313 insertions(+), 19 deletions(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/vfio_ccw_async.c new file +27 -0

+static size_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
+					 char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	region = private->region[i].data;
+	if (copy_to_user(buf, (void *)region + pos, count))
+		return -EFAULT;
+
+	return count;
+
+}
+
+static size_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
+					  const char __user *buf, size_t count,
+					  loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+

drivers/s390/cio/vfio_ccw_async.c line 51

Quoted 10 lines

drivers/s390/cio/vfio_ccw_async.c new file +10 -0

+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
+		return -EACCES;
+
+	region = private->region[i].data;
+	if (copy_from_user((void *)region + pos, buf, count))
+		return -EFAULT;  

I guess vfio_ccw_async_region_write() is supposed to be reentrant in a sense that there may be more that one 'instances' of the function executing at the same time, or am I wrong?

If it is reenarant, I wonder what protects private->region[i].data from corruption or simply being changed 'while at it'?

Regards,
Halil

Quoted 15 lines

drivers/s390/cio/vfio_ccw_async.c new file +15 -0

+
+	switch (region->command) {
+	case VFIO_CCW_ASYNC_CMD_HSCH:
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_HALT_REQ);
+		break;
+	case VFIO_CCW_ASYNC_CMD_CSCH:
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLEAR_REQ);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return region->ret_code ? region->ret_code : count;
+}
+  

Quoted 65 lines · On Wed, 28 Nov 2018 17:36:04 +0100 Halil Pasic <[email protected]> wrote:

Quoted 47 lines · On Thu, 22 Nov 2018 17:54:32 +0100 Cornelia Huck <[email protected]> wrote:

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
 drivers/s390/cio/Makefile           |   3 +-
 drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
 drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
 drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
 drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
 drivers/s390/cio/vfio_ccw_private.h |   6 ++
 include/uapi/linux/vfio.h           |   4 +
 include/uapi/linux/vfio_ccw.h       |  12 +++
 8 files changed, 313 insertions(+), 19 deletions(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/vfio_ccw_async.c new file +27 -0

+static size_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
+					 char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	region = private->region[i].data;
+	if (copy_to_user(buf, (void *)region + pos, count))
+		return -EFAULT;
+
+	return count;
+
+}
+
+static size_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
+					  const char __user *buf, size_t count,
+					  loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+

drivers/s390/cio/vfio_ccw_async.c line 51

Quoted 10 lines

drivers/s390/cio/vfio_ccw_async.c new file +10 -0

+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
+		return -EACCES;
+
+	region = private->region[i].data;
+	if (copy_from_user((void *)region + pos, buf, count))
+		return -EFAULT;  

I guess vfio_ccw_async_region_write() is supposed to be reentrant in a sense that there may be more that one 'instances' of the function executing at the same time, or am I wrong?

Quoted 2 lines

If it is reenarant, I wonder what protects private->region[i].data from corruption or simply being changed 'while at it'?

Interesting question. AFAICS this same issue applies to the existing I/O region as well.

There's nothing in common code enforcing any exclusivity.

If I
understand the code correctly, the common vfio-pci code reads/writes in 1/2/4 byte chunks for most accesses. There's igd code that does not do that, though.

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
  drivers/s390/cio/Makefile           |   3 +-
  drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
  drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
  drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
  drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
  drivers/s390/cio/vfio_ccw_private.h |   6 ++
  include/uapi/linux/vfio.h           |   4 +
  include/uapi/linux/vfio_ccw.h       |  12 +++
  8 files changed, 313 insertions(+), 19 deletions(-)

create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/Makefile +2 -1

index f230516abb96..f6a8db04177c 100644
@@ -20,5 +20,6 @@ obj-$(CONFIG_CCWGROUP) += ccwgroup.o
  qdio-objs := qdio_main.o qdio_thinint.o qdio_debug.o qdio_setup.o
  obj-$(CONFIG_QDIO) += qdio.o
 
-vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o
+vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o \
+	vfio_ccw_async.o
  obj-$(CONFIG_VFIO_CCW) += vfio_ccw.o

drivers/s390/cio/vfio_ccw_async.c new file +5 -0

new file mode 100644
index 000000000000..8c7f51d17d70
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Async I/O region for vfio_ccw
+ *
+ * Copyright Red Hat, Inc. 2018

drivers/s390/cio/vfio_ccw_async.c new file +34 -0

+					 char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	region = private->region[i].data;
+	if (copy_to_user(buf, (void *)region + pos, count))
+		return -EFAULT;
+
+	return count;
+
+}
+
+static size_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
+					  const char __user *buf, size_t count,
+					  loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
+		return -EACCES;
+
+	region = private->region[i].data;

drivers/s390/cio/vfio_ccw_async.c new file +29 -0

+	default:
+		return -EINVAL;
+	}
+
+	return region->ret_code ? region->ret_code : count;
+}
+
+static void vfio_ccw_async_region_release(struct vfio_ccw_private *private,
+					  struct vfio_ccw_region *region)
+{
+
+}
+
+const struct vfio_ccw_regops vfio_ccw_async_region_ops = {
+	.read = vfio_ccw_async_region_read,
+	.write = vfio_ccw_async_region_write,
+	.release = vfio_ccw_async_region_release,
+};
+
+int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private)
+{
+	return vfio_ccw_register_dev_region(private,
+					    VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD,
+					    &vfio_ccw_async_region_ops,
+					    sizeof(struct ccw_cmd_region),
+					    VFIO_REGION_INFO_FLAG_READ |
+					    VFIO_REGION_INFO_FLAG_WRITE,
+					    private->cmd_region);
+}

drivers/s390/cio/vfio_ccw_drv.c +34 -14

index a10cec0e86eb..890c588a3a61 100644
@@ -3,9 +3,11 @@
   * VFIO based Physical Subchannel device driver
   *
   * Copyright IBM Corp. 2017
+ * Copyright Red Hat, Inc. 2018
   *
   * Author(s): Dong Jia Shi <[email protected]>
   *            Xiao Feng Ren <[email protected]>
+ *            Cornelia Huck <[email protected]>
   */
 
  #include <linux/module.h>

@@ -23,6 +25,7 @@
 
  struct workqueue_struct *vfio_ccw_work_q;
  static struct kmem_cache *vfio_ccw_io_region;
+static struct kmem_cache *vfio_ccw_cmd_region;
 
  /*
   * Helpers

@@ -76,7 +79,8 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
  	private = container_of(work, struct vfio_ccw_private, io_work);
  	irb = &private->irb;
 
-	if (scsw_is_solicited(&irb->scsw)) {
+	if (scsw_is_solicited(&irb->scsw) &&
+	    (scsw_fctl(&irb->scsw) & SCSW_FCTL_START_FUNC)) {
  		cp_update_scsw(&private->cp, &irb->scsw);
  		cp_free(&private->cp);
  	}

@@ -104,7 +108,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
  {
  	struct pmcw *pmcw = &sch->schib.pmcw;
  	struct vfio_ccw_private *private;
-	int ret;
+	int ret = -ENOMEM;
 
  	if (pmcw->qf) {
  		dev_warn(&sch->dev, "vfio: ccw: does not support QDIO: %s\n",

@@ -118,10 +122,13 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 
  	private->io_region = kmem_cache_zalloc(vfio_ccw_io_region,
  					       GFP_KERNEL | GFP_DMA);
-	if (!private->io_region) {
-		kfree(private);
-		return -ENOMEM;
-	}
+	if (!private->io_region)
+		goto out_free;
+
+	private->cmd_region = kmem_cache_zalloc(vfio_ccw_cmd_region,
+						GFP_KERNEL | GFP_DMA);
+	if (!private->cmd_region)
+		goto out_free;
 
  	private->sch = sch;
  	dev_set_drvdata(&sch->dev, private);

@@ -148,7 +155,10 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
  	cio_disable_subchannel(sch);
  out_free:
  	dev_set_drvdata(&sch->dev, NULL);
-	kmem_cache_free(vfio_ccw_io_region, private->io_region);
+	if (private->cmd_region)
+		kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
+	if (private->io_region)
+		kmem_cache_free(vfio_ccw_io_region, private->io_region);
  	kfree(private);
  	return ret;
  }

@@ -237,7 +247,7 @@ static struct css_driver vfio_ccw_sch_driver = {
 
  static int __init vfio_ccw_sch_init(void)
  {
-	int ret;
+	int ret = -ENOMEM;
 
  	vfio_ccw_work_q = create_singlethread_workqueue("vfio-ccw");
  	if (!vfio_ccw_work_q)

@@ -247,20 +257,30 @@ static int __init vfio_ccw_sch_init(void)
  					sizeof(struct ccw_io_region), 0,
  					SLAB_ACCOUNT, 0,
  					sizeof(struct ccw_io_region), NULL);
-	if (!vfio_ccw_io_region) {
-		destroy_workqueue(vfio_ccw_work_q);
-		return -ENOMEM;
-	}
+	if (!vfio_ccw_io_region)
+		goto out_err;
+
+	vfio_ccw_cmd_region = kmem_cache_create_usercopy("vfio_ccw_cmd_region",
+					sizeof(struct ccw_cmd_region), 0,
+					SLAB_ACCOUNT, 0,
+					sizeof(struct ccw_cmd_region), NULL);
+	if (!vfio_ccw_cmd_region)
+		goto out_err;
 
  	isc_register(VFIO_CCW_ISC);
  	ret = css_driver_register(&vfio_ccw_sch_driver);
  	if (ret) {
  		isc_unregister(VFIO_CCW_ISC);
-		kmem_cache_destroy(vfio_ccw_io_region);
-		destroy_workqueue(vfio_ccw_work_q);
+		goto out_err;
  	}
 
  	return ret;
+
+out_err:
+	kmem_cache_destroy(vfio_ccw_cmd_region);
+	kmem_cache_destroy(vfio_ccw_io_region);
+	destroy_workqueue(vfio_ccw_work_q);
+	return ret;
  }
 
  static void __exit vfio_ccw_sch_exit(void)

drivers/s390/cio/vfio_ccw_fsm.c +77 -0

index f94aa01f9c36..0caf77e8f377 100644
@@ -3,8 +3,10 @@
   * Finite state machine for vfio-ccw device handling
   *
   * Copyright IBM Corp. 2017
+ * Copyright Red Hat, Inc. 2018
   *
   * Author(s): Dong Jia Shi <[email protected]>
+ *            Cornelia Huck <[email protected]>
   */
 
  #include <linux/vfio.h>

@@ -68,6 +70,81 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
  	return ret;
  }
 
+static int fsm_do_halt(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch;
+	unsigned long flags;
+	int ccode;
+	int ret;
+
+	sch = private->sch;
+
+	spin_lock_irqsave(sch->lock, flags);
+	private->state = VFIO_CCW_STATE_BUSY;
+
+	/* Issue "Halt Subchannel" */
+	ccode = hsch(sch->schid);
+
+	switch (ccode) {
+	case 0:
+		/*
+		 * Initialize device status information
+		 */
+		sch->schib.scsw.cmd.actl |= SCSW_ACTL_HALT_PEND;
+		ret = 0;
+		break;
+	case 1:		/* Status pending */
+	case 2:		/* Busy */
+		ret = -EBUSY;
+		break;
+	case 3:		/* Device not operational */
+	{
+		ret = -ENODEV;
+		break;
+	}
+	default:
+		ret = ccode;
+	}
+	spin_unlock_irqrestore(sch->lock, flags);
+	return ret;
+}
+
+static int fsm_do_clear(struct vfio_ccw_private *private)
+{
+	struct subchannel *sch;
+	unsigned long flags;
+	int ccode;
+	int ret;
+
+	sch = private->sch;
+
+	spin_lock_irqsave(sch->lock, flags);
+	private->state = VFIO_CCW_STATE_BUSY;
+
+	/* Issue "Clear Subchannel" */
+	ccode = csch(sch->schid);
+
+	switch (ccode) {
+	case 0:
+		/*
+		 * Initialize device status information
+		 */
+		sch->schib.scsw.cmd.actl = SCSW_ACTL_CLEAR_PEND;
+		/* TODO: check what else we might need to clear */
+		ret = 0;
+		break;
+	case 3:		/* Device not operational */
+	{
+		ret = -ENODEV;
+		break;
+	}
+	default:
+		ret = ccode;
+	}
+	spin_unlock_irqrestore(sch->lock, flags);
+	return ret;
+}
+
  static void fsm_notoper(struct vfio_ccw_private *private,
  			enum vfio_ccw_event event)
  {

drivers/s390/cio/vfio_ccw_fsm.c +74 -2

+	private->cmd_region->ret_code = -EIO;
+}
+
+static void fsm_async_busy(struct vfio_ccw_private *private,
+			   enum vfio_ccw_event event)
+{
+	private->cmd_region->ret_code = -EBUSY;
+}
+
  static void fsm_disabled_irq(struct vfio_ccw_private *private,
  			     enum vfio_ccw_event event)
  {

@@ -166,11 +257,11 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  		}
  		return;
  	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
-		/* XXX: Handle halt. */
+		/* halt is handled via the async cmd region */
  		io_region->ret_code = -EOPNOTSUPP;
  		goto err_out;
  	} else if (scsw->cmd.fctl & SCSW_FCTL_CLEAR_FUNC) {
-		/* XXX: Handle clear. */
+		/* clear is handled via the async cmd region */
  		io_region->ret_code = -EOPNOTSUPP;
  		goto err_out;
  	}

@@ -181,6 +272,59 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  			       io_region->ret_code, errstr);
  }
 
+/*
+ * Deal with a halt request from userspace.
+ */
+static void fsm_halt_request(struct vfio_ccw_private *private,
+			     enum vfio_ccw_event event)
+{
+	struct ccw_cmd_region *cmd_region = private->cmd_region;
+	int state = private->state;
+
+	private->state = VFIO_CCW_STATE_BOXED;
+
+	if (cmd_region->command != VFIO_CCW_ASYNC_CMD_HSCH) {
+		/* should not happen? */
+		cmd_region->ret_code = -EINVAL;
+		goto err_out;
+	}
+
+	cmd_region->ret_code = fsm_do_halt(private);
+	if (cmd_region->ret_code)
+		goto err_out;
+
+	return;
+
+err_out:
+	private->state = state;
+}
+
+/*
+ * Deal with a clear request from userspace.
+ */
+static void fsm_clear_request(struct vfio_ccw_private *private,
+			      enum vfio_ccw_event event)
+{
+	struct ccw_cmd_region *cmd_region = private->cmd_region;
+	int state = private->state;
+
+	private->state = VFIO_CCW_STATE_BOXED;
+
+	if (cmd_region->command != VFIO_CCW_ASYNC_CMD_CSCH) {
+		/* should not happen? */
+		cmd_region->ret_code = -EINVAL;
+		goto err_out;
+	}
+
+	cmd_region->ret_code = fsm_do_clear(private);
+	if (cmd_region->ret_code)
+		goto err_out;
+
+	return;
+
+err_out:
+	private->state = state;
+}
  /*
   * Got an interrupt for a normal io (state busy).
   */

@@ -204,26 +348,36 @@ 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_HALT_REQ]	= fsm_async_error,
+		[VFIO_CCW_EVENT_CLEAR_REQ]	= fsm_async_error,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
  	},
  	[VFIO_CCW_STATE_STANDBY] = {
  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
+		[VFIO_CCW_EVENT_HALT_REQ]	= fsm_async_error,
+		[VFIO_CCW_EVENT_CLEAR_REQ]	= fsm_async_error,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
  	},
  	[VFIO_CCW_STATE_IDLE] = {
  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_request,
+		[VFIO_CCW_EVENT_HALT_REQ]	= fsm_halt_request,
+		[VFIO_CCW_EVENT_CLEAR_REQ]	= fsm_clear_request,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
  	},
  	[VFIO_CCW_STATE_BOXED] = {
  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
+		[VFIO_CCW_EVENT_HALT_REQ]	= fsm_async_busy,
+		[VFIO_CCW_EVENT_CLEAR_REQ]	= fsm_async_busy,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
  	},
  	[VFIO_CCW_STATE_BUSY] = {
  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
+		[VFIO_CCW_EVENT_HALT_REQ]	= fsm_halt_request,
+		[VFIO_CCW_EVENT_CLEAR_REQ]	= fsm_clear_request,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
  	},
  };

drivers/s390/cio/vfio_ccw_ops.c +11 -2

index a5d731ed2a39..0e1f7f7bf927 100644
@@ -148,11 +148,20 @@ 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;
+
+	ret = vfio_ccw_register_async_dev_regions(private);
+	if (ret)
+		vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+					 &private->nb);
+	return ret;
  }
 
  static void vfio_ccw_mdev_release(struct mdev_device *mdev)

drivers/s390/cio/vfio_ccw_private.h +6 -0

index a6f9f84526e2..1a41a14831ae 100644
@@ -53,6 +53,8 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
  				 const struct vfio_ccw_regops *ops,
  				 size_t size, u32 flags, void *data);
 
+int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
+
  /**
   * struct vfio_ccw_private
   * @sch: pointer to the subchannel

@@ -62,6 +64,7 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
   * @mdev: pointer to the mediated device
   * @nb: notifier for vfio events
   * @io_region: MMIO region to input/output I/O arguments/results
+ * @cmd_region: MMIO region for asynchronous I/O commands other than START
   * @region: additional regions for other subchannel operations
   * @num_regions: number of additional regions
   * @cp: channel program for the current I/O operation

@@ -79,6 +82,7 @@ struct vfio_ccw_private {
  	struct notifier_block	nb;
  	struct ccw_io_region	*io_region;
  	struct vfio_ccw_region *region;
+	struct ccw_cmd_region	*cmd_region;
  	int num_regions;
 
  	struct channel_program	cp;

@@ -114,6 +118,8 @@ enum vfio_ccw_event {
  	VFIO_CCW_EVENT_NOT_OPER,
  	VFIO_CCW_EVENT_IO_REQ,
  	VFIO_CCW_EVENT_INTERRUPT,
+	VFIO_CCW_EVENT_HALT_REQ,
+	VFIO_CCW_EVENT_CLEAR_REQ,
  	/* last element! */
  	NR_VFIO_CCW_EVENTS
  };

include/uapi/linux/vfio.h +4 -0

index 565669f95534..c01472ec77ea 100644
@@ -304,6 +304,7 @@ struct vfio_region_info_cap_type {
  #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG	(2)
  #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG	(3)
 
+
  #define VFIO_REGION_TYPE_GFX                    (1)
  #define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
 

@@ -354,6 +355,9 @@ struct vfio_region_gfx_edid {
  #define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
  };
 
+/* ccw sub-types */
+#define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD	(1)
+
  /*
   * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
   * which allows direct access to non-MSIX registers which happened to be within

include/uapi/linux/vfio_ccw.h +12 -0

index 2ec5f367ff78..cbecbf0cd54f 100644
@@ -12,6 +12,7 @@
 
  #include <linux/types.h>
 
+/* used for START SUBCHANNEL, always present */
  struct ccw_io_region {
  #define ORB_AREA_SIZE 12
  	__u8	orb_area[ORB_AREA_SIZE];

@@ -22,4 +23,15 @@ struct ccw_io_region {
  	__u32	ret_code;
  } __packed;
 
+/*
+ * used for processing commands that trigger asynchronous actions
+ * Note: this is controlled by a capability
+ */
+#define VFIO_CCW_ASYNC_CMD_HSCH (1 << 0)
+#define VFIO_CCW_ASYNC_CMD_CSCH (1 << 1)
+struct ccw_cmd_region {
+	__u32 command;
+	__u32 ret_code;
+} __packed;
+
  #endif

Quoted 35 lines · Cornelia Huck

Add a region to the vfio-ccw device that can be used to submit asynchronous I/O instructions. ssch continues to be handled by the existing I/O region; the new region handles hsch and csch.

Interrupt status continues to be reported through the same channels as for ssch.

Signed-off-by Cornelia Huck <[email protected]>

---
  drivers/s390/cio/Makefile           |   3 +-
  drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++++++
  drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
  drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
  drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
  drivers/s390/cio/vfio_ccw_private.h |   6 ++
  include/uapi/linux/vfio.h           |   4 +
  include/uapi/linux/vfio_ccw.h       |  12 +++
  8 files changed, 313 insertions(+), 19 deletions(-)

create mode 100644 drivers/s390/cio/vfio_ccw_async.c

drivers/s390/cio/vfio_ccw_async.c new file +9 -0

new file mode 100644
index 000000000000..8c7f51d17d70
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Async I/O region for vfio_ccw
+ *
+ * Copyright Red Hat, Inc. 2018
+ *
+ * Author(s): Cornelia Huck <[email protected]>
+ */
+

drivers/s390/cio/vfio_ccw_async.c line 15

Quoted 6 lines

drivers/s390/cio/vfio_ccw_async.c new file +6 -0

+#include <linux/vfio.h>
+#include <linux/mdev.h>
+
+#include "vfio_ccw_private.h"
+
+static size_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,  

I think this should return ssize_t ? (same for _write, below)

Quoted 42 lines

drivers/s390/cio/vfio_ccw_async.c new file +41 -0

+					 char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	region = private->region[i].data;
+	if (copy_to_user(buf, (void *)region + pos, count))
+		return -EFAULT;
+
+	return count;
+
+}
+
+static size_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
+					  const char __user *buf, size_t count,
+					  loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
+		return -EACCES;
+
+	region = private->region[i].data;
+	if (copy_from_user((void *)region + pos, buf, count))
+		return -EFAULT;
+
+	switch (region->command) {
+	case VFIO_CCW_ASYNC_CMD_HSCH:
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_HALT_REQ);
+		break;

drivers/s390/cio/vfio_ccw_async.c line 59

Quoted 3 lines

drivers/s390/cio/vfio_ccw_async.c new file +3 -0

+	case VFIO_CCW_ASYNC_CMD_CSCH:
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLEAR_REQ);
+		break;  

I find myself wondering why we add separate VFIO_CCW_EVENT_x_REQ entries for HALT and CLEAR, rather than a single VFIO_CCW_EVENT_ASYNC_REQ and a switch on cmd_region->command within it to go to fsm_do_halt, fsm_do_clear, or whatever.

Quoted 7 lines

drivers/s390/cio/vfio_ccw_async.c new file +6 -0

+	default:
+		return -EINVAL;
+	}
+
+	return region->ret_code ? region->ret_code : count;
+}

Quoted 9 lines

drivers/s390/cio/vfio_ccw_fsm.c +0 -0

index f94aa01f9c36..0caf77e8f377 100644
@@ -102,6 +179,20 @@ static void fsm_io_busy(struct vfio_ccw_private *private,
  	private->io_region->ret_code = -EBUSY;
  }
 

drivers/s390/cio/vfio_ccw_fsm.c static void fsm_io_busy(struct vfio_ccw_private *private, · line 186

Quoted 5 lines

drivers/s390/cio/vfio_ccw_fsm.c +5 -0

+static void fsm_async_error(struct vfio_ccw_private *private,
+			    enum vfio_ccw_event event)
+{
+	pr_err("vfio-ccw: FSM: halt/clear request from state:%d\n",
+	       private->state);  

Worth stating whether it's a Halt or Clear here, rather than leaving it ambiguous?

[This is the Linux kernel part, git tree is available at
https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git
vfio-ccw-caps

The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps]

Currently, vfio-ccw only relays START SUBCHANNEL requests to the real device. This tends to work well for the most common 'good path' scenarios; however, as we emulate {HALT,CLEAR} SUBCHANNEL in QEMU, things like clearing pending requests at the device is currently not supported. This may be a problem for e.g. error recovery.

This patch series introduces capabilities (similar to what vfio-pci uses) and exposes a new async region for handling hsch/csch.

Quoted 13 lines · On Thu, 22 Nov 2018 17:54:29 +0100 Cornelia Huck <[email protected]> wrote:

[This is the Linux kernel part, git tree is available at
https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git
vfio-ccw-caps

The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps]

Currently, vfio-ccw only relays START SUBCHANNEL requests to the real device. This tends to work well for the most common 'good path' scenarios; however, as we emulate {HALT,CLEAR} SUBCHANNEL in QEMU, things like clearing pending requests at the device is currently not supported. This may be a problem for e.g. error recovery.

Quoted 2 lines

This patch series introduces capabilities (similar to what vfio-pci uses) and exposes a new async region for handling hsch/csch.

I'm on vacation and could not do more than skim over it real quick. FWIW it looks very promising. I intend to give it an in depth review once I'm back (i.e. second half of next week).

[This is the Linux kernel part, git tree is available at
https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git vfio-ccw-caps

The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps]

Currently, vfio-ccw only relays START SUBCHANNEL requests to the real device. This tends to work well for the most common 'good path' scenarios; however, as we emulate {HALT,CLEAR} SUBCHANNEL in QEMU, things like clearing pending requests at the device is currently not supported. This may be a problem for e.g. error recovery.

This patch series introduces capabilities (similar to what vfio-pci uses) and exposes a new async region for handling hsch/csch.

Very lightly tested (I can interact with a dasd as before; I have not found a reliable way to trigger hsch/csch in the Linux dasd guest driver.)

Cornelia Huck (3):
vfio-ccw: add capabilities chain
s390/cio: export hsch to modules
vfio-ccw: add handling for asnyc channel instructions

  drivers/s390/cio/Makefile           |   3 +-
  drivers/s390/cio/ioasm.c            |   1 +
  drivers/s390/cio/vfio_ccw_async.c   |  88 +++++++++++++
  drivers/s390/cio/vfio_ccw_drv.c     |  48 +++++--
  drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++-
  drivers/s390/cio/vfio_ccw_ops.c     | 195 ++++++++++++++++++++++++----
  drivers/s390/cio/vfio_ccw_private.h |  44 +++++++
  include/uapi/linux/vfio.h           |   5 +
  include/uapi/linux/vfio_ccw.h       |  12 ++
  9 files changed, 509 insertions(+), 45 deletions(-)

create mode 100644 drivers/s390/cio/vfio_ccw_async.c

Quoted 12 lines · Cornelia Huck

[This is the Linux kernel part, git tree is available at
https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git vfio-ccw-caps

The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps]

Currently, vfio-ccw only relays START SUBCHANNEL requests to the real device. This tends to work well for the most common 'good path' scenarios; however, as we emulate {HALT,CLEAR} SUBCHANNEL in QEMU, things like clearing pending requests at the device is currently not supported. This may be a problem for e.g. error recovery.

Quoted 6 lines

This patch series introduces capabilities (similar to what vfio-pci uses) and exposes a new async region for handling hsch/csch.

Very lightly tested (I can interact with a dasd as before; I have not found a reliable way to trigger hsch/csch in the Linux dasd guest driver.)

I was able to trigger the guest DASD driver to issue a csch instruction, and from my brief testing it seems to be working just like it would on the LPAR. (I basically tested with 2 threads trying to issue DASD device reserve and release ioctls, on the same DASD device, in a busy loop).

I am going to spend some time doing a deeper review.