[RFC PATCH v2 0/9] s390x/vfio-ccw: Channel Path Handling

[RFC PATCH v2 0/9] s390x/vfio-ccw: Channel Path Handling

[Eric Farman]

Here is a new pass at the channel-path handling code for vfio-ccw.
This was initially developed by Farhan Ali this past summer, and
picked up by me.  For my own benefit/sanity, I made a small changelog
in the commit message for each patch, describing the changes I've
made to his original code beyond just rebasing to master, rather than
a giant list appended here.

I had been encountering a host crash which I think was triggered by
this code rather than existing within it.  I'd sent a potential fix
for that separately, but need more diagnosis.  So while that is
outstanding, I think I've gotten most (but probably not all) comments
from v1 addressed within.

With this, and the corresponding QEMU series (to be posted momentarily),
applied I am able to configure off/on a CHPID (for example, by issuing
"chchp -c 0/1 xx" on the host), and the guest is able to see both the
events and reflect the updated path masks in its structures.

For reasons that are hopefully obvious, issuing chchp within the guest
only works for the logical vary.  Configuring it off/on does not work,
which I think is fine.

v1: https://lore.kernel.org/kvm/20191115025620.19593-1-farman@linux.ibm.com/

Eric Farman (4):
  vfio-ccw: Refactor the unregister of the async regions
  vfio-ccw: Refactor IRQ handlers
  vfio-ccw: Add trace for CRW event
  vfio-ccw: Remove inline get_schid() routine

Farhan Ali (5):
  vfio-ccw: Introduce new helper functions to free/destroy regions
  vfio-ccw: Register a chp_event callback for vfio-ccw
  vfio-ccw: Introduce a new schib region
  vfio-ccw: Introduce a new CRW region
  vfio-ccw: Wire up the CRW irq and CRW region

 Documentation/s390/vfio-ccw.rst     |  31 ++++-
 drivers/s390/cio/Makefile           |   2 +-
 drivers/s390/cio/vfio_ccw_chp.c     | 136 ++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_drv.c     | 186 ++++++++++++++++++++++++++--
 drivers/s390/cio/vfio_ccw_fsm.c     |   8 +-
 drivers/s390/cio/vfio_ccw_ops.c     |  65 +++++++---
 drivers/s390/cio/vfio_ccw_private.h |  16 +++
 drivers/s390/cio/vfio_ccw_trace.c   |   1 +
 drivers/s390/cio/vfio_ccw_trace.h   |  30 +++++
 include/uapi/linux/vfio.h           |   3 +
 include/uapi/linux/vfio_ccw.h       |  19 +++
 11 files changed, 463 insertions(+), 34 deletions(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_chp.c

-- 
2.17.1

[RFC PATCH v2 1/9] vfio-ccw: Introduce new helper functions to free/destroy regions

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

Consolidate some of the cleanup code for the regions, so that
as more are added we reduce code duplication.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---

Notes:
    v1->v2:
     - Add Conny's r-b
    
    v0->v1: [EF]
     - Commit message

 drivers/s390/cio/vfio_ccw_drv.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index e401a3d0aa57..91989269faf1 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -116,6 +116,14 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
 }
 
+static void vfio_ccw_free_regions(struct vfio_ccw_private *private)
+{
+	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);
+}
+
 static int vfio_ccw_sch_probe(struct subchannel *sch)
 {
 	struct pmcw *pmcw = &sch->schib.pmcw;
@@ -176,10 +184,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	cio_disable_subchannel(sch);
 out_free:
 	dev_set_drvdata(&sch->dev, NULL);
-	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);
+	vfio_ccw_free_regions(private);
 	kfree(private->cp.guest_cp);
 	kfree(private);
 	return ret;
@@ -195,8 +200,7 @@ static int vfio_ccw_sch_remove(struct subchannel *sch)
 
 	dev_set_drvdata(&sch->dev, NULL);
 
-	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
-	kmem_cache_free(vfio_ccw_io_region, private->io_region);
+	vfio_ccw_free_regions(private);
 	kfree(private->cp.guest_cp);
 	kfree(private);
 
@@ -299,6 +303,12 @@ static void vfio_ccw_debug_exit(void)
 	debug_unregister(vfio_ccw_debug_trace_id);
 }
 
+static void vfio_ccw_destroy_regions(void)
+{
+	kmem_cache_destroy(vfio_ccw_cmd_region);
+	kmem_cache_destroy(vfio_ccw_io_region);
+}
+
 static int __init vfio_ccw_sch_init(void)
 {
 	int ret;
@@ -341,8 +351,7 @@ static int __init vfio_ccw_sch_init(void)
 	return ret;
 
 out_err:
-	kmem_cache_destroy(vfio_ccw_cmd_region);
-	kmem_cache_destroy(vfio_ccw_io_region);
+	vfio_ccw_destroy_regions();
 	destroy_workqueue(vfio_ccw_work_q);
 	vfio_ccw_debug_exit();
 	return ret;
@@ -352,8 +361,7 @@ static void __exit vfio_ccw_sch_exit(void)
 {
 	css_driver_unregister(&vfio_ccw_sch_driver);
 	isc_unregister(VFIO_CCW_ISC);
-	kmem_cache_destroy(vfio_ccw_io_region);
-	kmem_cache_destroy(vfio_ccw_cmd_region);
+	vfio_ccw_destroy_regions();
 	destroy_workqueue(vfio_ccw_work_q);
 	vfio_ccw_debug_exit();
 }
-- 
2.17.1

[RFC PATCH v2 2/9] vfio-ccw: Register a chp_event callback for vfio-ccw

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

Register the chp_event callback to receive channel path related
events for the subchannels managed by vfio-ccw.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v1->v2:
     - Move s390dbf before cio_update_schib() call [CH]
    
    v0->v1: [EF]
     - Add s390dbf trace

 drivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 91989269faf1..a99705e2fd73 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -19,6 +19,7 @@
 
 #include <asm/isc.h>
 
+#include "chp.h"
 #include "ioasm.h"
 #include "css.h"
 #include "vfio_ccw_private.h"
@@ -257,6 +258,48 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
 	return rc;
 }
 
+static int vfio_ccw_chp_event(struct subchannel *sch,
+			      struct chp_link *link, int event)
+{
+	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+	int mask = chp_ssd_get_mask(&sch->ssd_info, link);
+	int retry = 255;
+
+	if (!private || !mask)
+		return 0;
+
+	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
+			   mdev_uuid(private->mdev), sch->schid.cssid,
+			   sch->schid.ssid, sch->schid.sch_no,
+			   mask, event);
+
+	if (cio_update_schib(sch))
+		return -ENODEV;
+
+	switch (event) {
+	case CHP_VARY_OFF:
+		/* Path logically turned off */
+		sch->opm &= ~mask;
+		sch->lpm &= ~mask;
+		break;
+	case CHP_OFFLINE:
+		/* Path is gone */
+		cio_cancel_halt_clear(sch, &retry);
+		break;
+	case CHP_VARY_ON:
+		/* Path logically turned on */
+		sch->opm |= mask;
+		sch->lpm |= mask;
+		break;
+	case CHP_ONLINE:
+		/* Path became available */
+		sch->lpm |= mask & sch->opm;
+		break;
+	}
+
+	return 0;
+}
+
 static struct css_device_id vfio_ccw_sch_ids[] = {
 	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
 	{ /* end of list */ },
@@ -274,6 +317,7 @@ static struct css_driver vfio_ccw_sch_driver = {
 	.remove = vfio_ccw_sch_remove,
 	.shutdown = vfio_ccw_sch_shutdown,
 	.sch_event = vfio_ccw_sch_event,
+	.chp_event = vfio_ccw_chp_event,
 };
 
 static int __init vfio_ccw_debug_init(void)
-- 
2.17.1

[RFC PATCH v2 3/9] vfio-ccw: Refactor the unregister of the async regions

[Eric Farman]

This is mostly for the purposes of a later patch, since
we'll need to do the same thing later.

While we are at it, move the resulting function call to ahead
of the unregistering of the IOMMU notifier, so that it's done
in the reverse order of how it was created.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---

Notes:
    v1->v2:
     - Add Conny's r-b

 drivers/s390/cio/vfio_ccw_ops.c     | 20 ++++++++++++--------
 drivers/s390/cio/vfio_ccw_private.h |  1 +
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index f0d71ab77c50..d4fc84b8867f 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -181,7 +181,6 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
 {
 	struct vfio_ccw_private *private =
 		dev_get_drvdata(mdev_parent_dev(mdev));
-	int i;
 
 	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
 	    (private->state != VFIO_CCW_STATE_STANDBY)) {
@@ -191,15 +190,9 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
 	}
 
 	cp_free(&private->cp);
+	vfio_ccw_unregister_dev_regions(private);
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
 				 &private->nb);
-
-	for (i = 0; i < private->num_regions; i++)
-		private->region[i].ops->release(private, &private->region[i]);
-
-	private->num_regions = 0;
-	kfree(private->region);
-	private->region = NULL;
 }
 
 static ssize_t vfio_ccw_mdev_read_io_region(struct vfio_ccw_private *private,
@@ -482,6 +475,17 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
 	return 0;
 }
 
+void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private)
+{
+	int i;
+
+	for (i = 0; i < private->num_regions; i++)
+		private->region[i].ops->release(private, &private->region[i]);
+	private->num_regions = 0;
+	kfree(private->region);
+	private->region = NULL;
+}
+
 static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
 				   unsigned int cmd,
 				   unsigned long arg)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 9b9bb4982972..ce3834159d98 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -53,6 +53,7 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
 				 unsigned int subtype,
 				 const struct vfio_ccw_regops *ops,
 				 size_t size, u32 flags, void *data);
+void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private);
 
 int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
 
-- 
2.17.1

[RFC PATCH v2 4/9] vfio-ccw: Introduce a new schib region

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

The schib region can be used by userspace to get the subchannel-
information block (SCHIB) for the passthrough subchannel.
This can be useful to get information such as channel path
information via the SCHIB.PMCW fields.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v1->v2:
     - Add new region info to Documentation/s390/vfio-ccw.rst [CH]
     - Add a block comment to struct ccw_schib_region [CH]
    
    v0->v1: [EF]
     - Clean up checkpatch (#include, whitespace) errors
     - Remove unnecessary includes from vfio_ccw_chp.c
     - Add ret=-ENOMEM in error path for new region
     - Add call to vfio_ccw_unregister_dev_regions() during error exit
       path of vfio_ccw_mdev_open()
     - New info on the module prologue
     - Reorder cleanup of regions

 Documentation/s390/vfio-ccw.rst     | 16 +++++-
 drivers/s390/cio/Makefile           |  2 +-
 drivers/s390/cio/vfio_ccw_chp.c     | 75 +++++++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_drv.c     | 20 ++++++++
 drivers/s390/cio/vfio_ccw_ops.c     | 14 +++++-
 drivers/s390/cio/vfio_ccw_private.h |  3 ++
 include/uapi/linux/vfio.h           |  1 +
 include/uapi/linux/vfio_ccw.h       | 10 ++++
 8 files changed, 137 insertions(+), 4 deletions(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_chp.c

diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
index fca9c4f5bd9c..b805dc995fc8 100644
--- a/Documentation/s390/vfio-ccw.rst
+++ b/Documentation/s390/vfio-ccw.rst
@@ -231,6 +231,19 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD.
 
 Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region.
 
+vfio-ccw schib region
+---------------------
+
+The vfio-ccw schib region is used to return Subchannel-Information
+Block (SCHIB) data to userspace::
+
+  struct ccw_schib_region {
+  #define SCHIB_AREA_SIZE 52
+         __u8 schib_area[SCHIB_AREA_SIZE];
+  } __packed;
+
+This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
+
 vfio-ccw operation details
 --------------------------
 
@@ -333,7 +346,8 @@ through DASD/ECKD device online in a guest now and use it as a block
 device.
 
 The current code allows the guest to start channel programs via
-START SUBCHANNEL, and to issue HALT SUBCHANNEL and CLEAR SUBCHANNEL.
+START SUBCHANNEL, and to issue HALT SUBCHANNEL, CLEAR SUBCHANNEL,
+and STORE SUBCHANNEL.
 
 vfio-ccw supports classic (command mode) channel I/O only. Transport
 mode (HPF) is not supported.
diff --git a/drivers/s390/cio/Makefile b/drivers/s390/cio/Makefile
index 23eae4188876..a9235f111e79 100644
--- a/drivers/s390/cio/Makefile
+++ b/drivers/s390/cio/Makefile
@@ -21,5 +21,5 @@ 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_async.o vfio_ccw_trace.o
+	vfio_ccw_async.o vfio_ccw_trace.o vfio_ccw_chp.o
 obj-$(CONFIG_VFIO_CCW) += vfio_ccw.o
diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
new file mode 100644
index 000000000000..826d08379fe3
--- /dev/null
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Channel path related status regions for vfio_ccw
+ *
+ * Copyright IBM Corp. 2019
+ *
+ * Author(s): Farhan Ali <alifm@linux.ibm.com>
+ */
+
+#include <linux/vfio.h>
+#include "vfio_ccw_private.h"
+
+static ssize_t vfio_ccw_schib_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_schib_region *region;
+	int ret;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	mutex_lock(&private->io_mutex);
+	region = private->region[i].data;
+
+	if (cio_update_schib(private->sch)) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	memcpy(region, &private->sch->schib, sizeof(*region));
+
+	if (copy_to_user(buf, (void *)region + pos, count)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = count;
+
+out:
+	mutex_unlock(&private->io_mutex);
+	return ret;
+}
+
+static ssize_t vfio_ccw_schib_region_write(struct vfio_ccw_private *private,
+					   const char __user *buf, size_t count,
+					   loff_t *ppos)
+{
+	return -EINVAL;
+}
+
+
+static void vfio_ccw_schib_region_release(struct vfio_ccw_private *private,
+					  struct vfio_ccw_region *region)
+{
+
+}
+
+const struct vfio_ccw_regops vfio_ccw_schib_region_ops = {
+	.read = vfio_ccw_schib_region_read,
+	.write = vfio_ccw_schib_region_write,
+	.release = vfio_ccw_schib_region_release,
+};
+
+int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
+{
+	return vfio_ccw_register_dev_region(private,
+					    VFIO_REGION_SUBTYPE_CCW_SCHIB,
+					    &vfio_ccw_schib_region_ops,
+					    sizeof(struct ccw_schib_region),
+					    VFIO_REGION_INFO_FLAG_READ,
+					    private->schib_region);
+}
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index a99705e2fd73..3023a366ad47 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -27,6 +27,7 @@
 struct workqueue_struct *vfio_ccw_work_q;
 static struct kmem_cache *vfio_ccw_io_region;
 static struct kmem_cache *vfio_ccw_cmd_region;
+static struct kmem_cache *vfio_ccw_schib_region;
 
 debug_info_t *vfio_ccw_debug_msg_id;
 debug_info_t *vfio_ccw_debug_trace_id;
@@ -119,6 +120,8 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
 
 static void vfio_ccw_free_regions(struct vfio_ccw_private *private)
 {
+	if (private->schib_region)
+		kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
 	if (private->cmd_region)
 		kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
 	if (private->io_region)
@@ -156,6 +159,12 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	if (!private->cmd_region)
 		goto out_free;
 
+	private->schib_region = kmem_cache_zalloc(vfio_ccw_schib_region,
+						  GFP_KERNEL | GFP_DMA);
+
+	if (!private->schib_region)
+		goto out_free;
+
 	private->sch = sch;
 	dev_set_drvdata(&sch->dev, private);
 	mutex_init(&private->io_mutex);
@@ -349,6 +358,7 @@ static void vfio_ccw_debug_exit(void)
 
 static void vfio_ccw_destroy_regions(void)
 {
+	kmem_cache_destroy(vfio_ccw_schib_region);
 	kmem_cache_destroy(vfio_ccw_cmd_region);
 	kmem_cache_destroy(vfio_ccw_io_region);
 }
@@ -385,6 +395,16 @@ static int __init vfio_ccw_sch_init(void)
 		goto out_err;
 	}
 
+	vfio_ccw_schib_region = kmem_cache_create_usercopy("vfio_ccw_schib_region",
+					sizeof(struct ccw_schib_region), 0,
+					SLAB_ACCOUNT, 0,
+					sizeof(struct ccw_schib_region), NULL);
+
+	if (!vfio_ccw_schib_region) {
+		ret = -ENOMEM;
+		goto out_err;
+	}
+
 	isc_register(VFIO_CCW_ISC);
 	ret = css_driver_register(&vfio_ccw_sch_driver);
 	if (ret) {
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index d4fc84b8867f..22988d67b6bb 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -172,8 +172,18 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
 
 	ret = vfio_ccw_register_async_dev_regions(private);
 	if (ret)
-		vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
-					 &private->nb);
+		goto out_unregister;
+
+	ret = vfio_ccw_register_schib_dev_regions(private);
+	if (ret)
+		goto out_unregister;
+
+	return ret;
+
+out_unregister:
+	vfio_ccw_unregister_dev_regions(private);
+	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+				 &private->nb);
 	return ret;
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index ce3834159d98..d6601a8adf13 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -56,6 +56,7 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
 void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private);
 
 int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
+int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private);
 
 /**
  * struct vfio_ccw_private
@@ -69,6 +70,7 @@ int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
  * @io_mutex: protect against concurrent update of I/O regions
  * @region: additional regions for other subchannel operations
  * @cmd_region: MMIO region for asynchronous I/O commands other than START
+ * @schib_region: MMIO region for SCHIB information
  * @num_regions: number of additional regions
  * @cp: channel program for the current I/O operation
  * @irb: irb info received from interrupt
@@ -87,6 +89,7 @@ struct vfio_ccw_private {
 	struct mutex		io_mutex;
 	struct vfio_ccw_region *region;
 	struct ccw_cmd_region	*cmd_region;
+	struct ccw_schib_region *schib_region;
 	int num_regions;
 
 	struct channel_program	cp;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 9e843a147ead..b108e2795ea3 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -378,6 +378,7 @@ struct vfio_region_gfx_edid {
 
 /* sub-types for VFIO_REGION_TYPE_CCW */
 #define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD	(1)
+#define VFIO_REGION_SUBTYPE_CCW_SCHIB		(2)
 
 /*
  * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
index cbecbf0cd54f..758bf214898d 100644
--- a/include/uapi/linux/vfio_ccw.h
+++ b/include/uapi/linux/vfio_ccw.h
@@ -34,4 +34,14 @@ struct ccw_cmd_region {
 	__u32 ret_code;
 } __packed;
 
+/*
+ * Used for processing commands that read the subchannel-information block
+ * Reading this region triggers a stsch() to hardware
+ * Note: this is controlled by a capability
+ */
+struct ccw_schib_region {
+#define SCHIB_AREA_SIZE 52
+	__u8 schib_area[SCHIB_AREA_SIZE];
+} __packed;
+
 #endif
-- 
2.17.1

[RFC PATCH v2 5/9] vfio-ccw: Introduce a new CRW region

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

This region provides a mechanism to pass Channel Report Word(s)
that affect vfio-ccw devices, and need to be passed to the guest
for its awareness and/or processing.

The base driver (see crw_collect_info()) provides space for two
CRWs, as a subchannel event may have two CRWs chained together
(one for the ssid, one for the subcahnnel).  All other CRWs will
only occupy the first one.  Even though this support will also
only utilize the first one, we'll provide space for two also.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v1-v2:
     - Add new region info to Documentation/s390/vfio-ccw.rst [CH]
     - Add a block comment to struct ccw_crw_region [CH]
    
    v0->v1: [EF]
     - Clean up checkpatch (whitespace) errors
     - Add ret=-ENOMEM in error path for new region
     - Add io_mutex for region read (originally in last patch)
     - Change crw1/crw2 to crw0/crw1
     - Reorder cleanup of regions

 Documentation/s390/vfio-ccw.rst     | 15 ++++++++
 drivers/s390/cio/vfio_ccw_chp.c     | 56 +++++++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_drv.c     | 20 +++++++++++
 drivers/s390/cio/vfio_ccw_ops.c     |  4 +++
 drivers/s390/cio/vfio_ccw_private.h |  3 ++
 include/uapi/linux/vfio.h           |  1 +
 include/uapi/linux/vfio_ccw.h       |  9 +++++
 7 files changed, 108 insertions(+)

diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
index b805dc995fc8..b8e4b10e9988 100644
--- a/Documentation/s390/vfio-ccw.rst
+++ b/Documentation/s390/vfio-ccw.rst
@@ -244,6 +244,21 @@ Block (SCHIB) data to userspace::
 
 This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
 
+vfio-ccw crw region
+---------------------
+
+The vfio-ccw crw region is used to return Channel Report Word (CRW)
+data to userspace::
+
+  struct ccw_crw_region {
+         __u32 crw[2];
+  } __packed;
+
+This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW.
+
+Currently, space is provided for two CRWs, as hardware may chain
+two together for subchannel events even if vfio-ccw does not today.
+
 vfio-ccw operation details
 --------------------------
 
diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index 826d08379fe3..8fde94552149 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -73,3 +73,59 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
 					    VFIO_REGION_INFO_FLAG_READ,
 					    private->schib_region);
 }
+
+static ssize_t vfio_ccw_crw_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_crw_region *region;
+	int ret;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	if (list_empty(&private->crw))
+		return 0;
+
+	mutex_lock(&private->io_mutex);
+	region = private->region[i].data;
+
+	if (copy_to_user(buf, (void *)region + pos, count))
+		ret = -EFAULT;
+	else
+		ret = count;
+
+	mutex_unlock(&private->io_mutex);
+	return ret;
+}
+
+static ssize_t vfio_ccw_crw_region_write(struct vfio_ccw_private *private,
+					 const char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	return -EINVAL;
+}
+
+static void vfio_ccw_crw_region_release(struct vfio_ccw_private *private,
+					struct vfio_ccw_region *region)
+{
+
+}
+
+const struct vfio_ccw_regops vfio_ccw_crw_region_ops = {
+	.read = vfio_ccw_crw_region_read,
+	.write = vfio_ccw_crw_region_write,
+	.release = vfio_ccw_crw_region_release,
+};
+
+int vfio_ccw_register_crw_dev_regions(struct vfio_ccw_private *private)
+{
+	return vfio_ccw_register_dev_region(private,
+					    VFIO_REGION_SUBTYPE_CCW_CRW,
+					    &vfio_ccw_crw_region_ops,
+					    sizeof(struct ccw_crw_region),
+					    VFIO_REGION_INFO_FLAG_READ,
+					    private->crw_region);
+}
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 3023a366ad47..1e1360af1b34 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -28,6 +28,7 @@ struct workqueue_struct *vfio_ccw_work_q;
 static struct kmem_cache *vfio_ccw_io_region;
 static struct kmem_cache *vfio_ccw_cmd_region;
 static struct kmem_cache *vfio_ccw_schib_region;
+static struct kmem_cache *vfio_ccw_crw_region;
 
 debug_info_t *vfio_ccw_debug_msg_id;
 debug_info_t *vfio_ccw_debug_trace_id;
@@ -120,6 +121,8 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
 
 static void vfio_ccw_free_regions(struct vfio_ccw_private *private)
 {
+	if (private->crw_region)
+		kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
 	if (private->schib_region)
 		kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
 	if (private->cmd_region)
@@ -165,6 +168,12 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	if (!private->schib_region)
 		goto out_free;
 
+	private->crw_region = kmem_cache_zalloc(vfio_ccw_crw_region,
+						GFP_KERNEL | GFP_DMA);
+
+	if (!private->crw_region)
+		goto out_free;
+
 	private->sch = sch;
 	dev_set_drvdata(&sch->dev, private);
 	mutex_init(&private->io_mutex);
@@ -358,6 +367,7 @@ static void vfio_ccw_debug_exit(void)
 
 static void vfio_ccw_destroy_regions(void)
 {
+	kmem_cache_destroy(vfio_ccw_crw_region);
 	kmem_cache_destroy(vfio_ccw_schib_region);
 	kmem_cache_destroy(vfio_ccw_cmd_region);
 	kmem_cache_destroy(vfio_ccw_io_region);
@@ -405,6 +415,16 @@ static int __init vfio_ccw_sch_init(void)
 		goto out_err;
 	}
 
+	vfio_ccw_crw_region = kmem_cache_create_usercopy("vfio_ccw_crw_region",
+					sizeof(struct ccw_crw_region), 0,
+					SLAB_ACCOUNT, 0,
+					sizeof(struct ccw_crw_region), NULL);
+
+	if (!vfio_ccw_crw_region) {
+		ret = -ENOMEM;
+		goto out_err;
+	}
+
 	isc_register(VFIO_CCW_ISC);
 	ret = css_driver_register(&vfio_ccw_sch_driver);
 	if (ret) {
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 22988d67b6bb..edec0fb7ace8 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -178,6 +178,10 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
 	if (ret)
 		goto out_unregister;
 
+	ret = vfio_ccw_register_crw_dev_regions(private);
+	if (ret)
+		goto out_unregister;
+
 	return ret;
 
 out_unregister:
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index d6601a8adf13..8289b6850e59 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -57,6 +57,7 @@ void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private);
 
 int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
 int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private);
+int vfio_ccw_register_crw_dev_regions(struct vfio_ccw_private *private);
 
 /**
  * struct vfio_ccw_private
@@ -71,6 +72,7 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private);
  * @region: additional regions for other subchannel operations
  * @cmd_region: MMIO region for asynchronous I/O commands other than START
  * @schib_region: MMIO region for SCHIB information
+ * @crw_region: MMIO region for getting channel report words
  * @num_regions: number of additional regions
  * @cp: channel program for the current I/O operation
  * @irb: irb info received from interrupt
@@ -90,6 +92,7 @@ struct vfio_ccw_private {
 	struct vfio_ccw_region *region;
 	struct ccw_cmd_region	*cmd_region;
 	struct ccw_schib_region *schib_region;
+	struct ccw_crw_region	*crw_region;
 	int num_regions;
 
 	struct channel_program	cp;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index b108e2795ea3..5024636d7615 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -379,6 +379,7 @@ struct vfio_region_gfx_edid {
 /* sub-types for VFIO_REGION_TYPE_CCW */
 #define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD	(1)
 #define VFIO_REGION_SUBTYPE_CCW_SCHIB		(2)
+#define VFIO_REGION_SUBTYPE_CCW_CRW		(3)
 
 /*
  * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
index 758bf214898d..3a98bc4c80b6 100644
--- a/include/uapi/linux/vfio_ccw.h
+++ b/include/uapi/linux/vfio_ccw.h
@@ -44,4 +44,13 @@ struct ccw_schib_region {
 	__u8 schib_area[SCHIB_AREA_SIZE];
 } __packed;
 
+/*
+ * Used for returning Channel Report Word(s) to userspace.
+ * Note: this is controlled by a capability
+ */
+struct ccw_crw_region {
+	__u32 crw0;
+	__u32 crw1;
+} __packed;
+
 #endif
-- 
2.17.1

[RFC PATCH v2 6/9] vfio-ccw: Refactor IRQ handlers

[Eric Farman]

To simplify future expansion.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---

Notes:
    v1->v2:
     - Add Conny's r-b

 drivers/s390/cio/vfio_ccw_ops.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index edec0fb7ace8..f3033f8fc96d 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -391,17 +391,21 @@ static int vfio_ccw_mdev_get_region_info(struct vfio_region_info *info,
 
 static int vfio_ccw_mdev_get_irq_info(struct vfio_irq_info *info)
 {
-	if (info->index != VFIO_CCW_IO_IRQ_INDEX)
+	switch (info->index) {
+	case VFIO_CCW_IO_IRQ_INDEX:
+		info->count = 1;
+		info->flags = VFIO_IRQ_INFO_EVENTFD;
+		break;
+	default:
 		return -EINVAL;
-
-	info->count = 1;
-	info->flags = VFIO_IRQ_INFO_EVENTFD;
+	}
 
 	return 0;
 }
 
 static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
 				  uint32_t flags,
+				  uint32_t index,
 				  void __user *data)
 {
 	struct vfio_ccw_private *private;
@@ -411,7 +415,14 @@ static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
 		return -EINVAL;
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
-	ctx = &private->io_trigger;
+
+	switch (index) {
+	case VFIO_CCW_IO_IRQ_INDEX:
+		ctx = &private->io_trigger;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
 	case VFIO_IRQ_SET_DATA_NONE:
@@ -583,7 +594,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
 			return ret;
 
 		data = (void __user *)(arg + minsz);
-		return vfio_ccw_mdev_set_irqs(mdev, hdr.flags, data);
+		return vfio_ccw_mdev_set_irqs(mdev, hdr.flags, hdr.index, data);
 	}
 	case VFIO_DEVICE_RESET:
 		return vfio_ccw_mdev_reset(mdev);
-- 
2.17.1

[RFC PATCH v2 7/9] vfio-ccw: Wire up the CRW irq and CRW region

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

Use an IRQ to notify userspace that there is a CRW
pending in the region, related to path-availability
changes on the passthrough subchannel.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v1->v2:
     - Remove extraneous 0x0 in crw.rsid assignment [CH]
     - Refactor the building/queueing of a crw into its own routine [EF]
    
    v0->v1: [EF]
     - Place the non-refactoring changes from the previous patch here
     - Clean up checkpatch (whitespace) errors
     - s/chp_crw/crw/
     - Move acquire/release of io_mutex in vfio_ccw_crw_region_read()
       into patch that introduces that region
     - Remove duplicate include from vfio_ccw_drv.c
     - Reorder include in vfio_ccw_private.h

 drivers/s390/cio/vfio_ccw_chp.c     |  5 ++
 drivers/s390/cio/vfio_ccw_drv.c     | 73 +++++++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_ops.c     |  4 ++
 drivers/s390/cio/vfio_ccw_private.h |  9 ++++
 include/uapi/linux/vfio.h           |  1 +
 5 files changed, 92 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index 8fde94552149..328b4e1d1972 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -98,6 +98,11 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 		ret = count;
 
 	mutex_unlock(&private->io_mutex);
+
+	/* Notify the guest if more CRWs are on our queue */
+	if (!list_empty(&private->crw) && private->crw_trigger)
+		eventfd_signal(private->crw_trigger, 1);
+
 	return ret;
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 1e1360af1b34..c48c260a129d 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -108,6 +108,31 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 		eventfd_signal(private->io_trigger, 1);
 }
 
+static void vfio_ccw_crw_todo(struct work_struct *work)
+{
+	struct vfio_ccw_private *private;
+	struct vfio_ccw_crw *crw;
+
+	private = container_of(work, struct vfio_ccw_private, crw_work);
+
+	/* FIXME Ugh, need better control of this list */
+	crw = list_first_entry_or_null(&private->crw,
+				       struct vfio_ccw_crw, next);
+
+	if (crw) {
+		list_del(&crw->next);
+
+		mutex_lock(&private->io_mutex);
+		memcpy(&private->crw_region->crw0, crw->crw, sizeof(*crw->crw));
+		mutex_unlock(&private->io_mutex);
+
+		kfree(crw);
+
+		if (private->crw_trigger)
+			eventfd_signal(private->crw_trigger, 1);
+	}
+}
+
 /*
  * Css driver callbacks
  */
@@ -186,7 +211,9 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	if (ret)
 		goto out_free;
 
+	INIT_LIST_HEAD(&private->crw);
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
+	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
 	atomic_set(&private->avail, 1);
 	private->state = VFIO_CCW_STATE_STANDBY;
 
@@ -212,9 +239,15 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 static int vfio_ccw_sch_remove(struct subchannel *sch)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_crw *crw, *temp;
 
 	vfio_ccw_sch_quiesce(sch);
 
+	list_for_each_entry_safe(crw, temp, &private->crw, next) {
+		list_del(&crw->next);
+		kfree(crw);
+	}
+
 	vfio_ccw_mdev_unreg(sch);
 
 	dev_set_drvdata(&sch->dev, NULL);
@@ -276,6 +309,44 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
 	return rc;
 }
 
+static void vfio_ccw_alloc_crw(struct vfio_ccw_private *private,
+			       struct chp_link *link,
+			       unsigned int erc)
+{
+	struct vfio_ccw_crw *vc_crw;
+	struct crw *crw;
+
+	/*
+	 * If unable to allocate a CRW, just drop the event and
+	 * carry on.  The guest will either see a later one or
+	 * learn when it issues its own store subchannel.
+	 */
+	vc_crw = kzalloc(sizeof(*vc_crw), GFP_ATOMIC);
+	if (!vc_crw)
+		return;
+
+	/*
+	 * Build in the first CRW space, but don't chain anything
+	 * into the second one even though the space exists.
+	 */
+	crw = &vc_crw->crw[0];
+
+	/*
+	 * Presume every CRW we handle is reported by a channel-path.
+	 * Maybe not future-proof, but good for what we're doing now.
+	 *
+	 * FIXME Sort of a lie, since we're converting a CRW
+	 * reported by a channel-path into one issued to each
+	 * subchannel, but still saying it's coming from the path.
+	 */
+	crw->rsc = CRW_RSC_CPATH;
+	crw->rsid = (link->chpid.cssid << 8) | link->chpid.id;
+	crw->erc = erc;
+
+	list_add_tail(&vc_crw->next, &private->crw);
+	queue_work(vfio_ccw_work_q, &private->crw_work);
+}
+
 static int vfio_ccw_chp_event(struct subchannel *sch,
 			      struct chp_link *link, int event)
 {
@@ -303,6 +374,7 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
 	case CHP_OFFLINE:
 		/* Path is gone */
 		cio_cancel_halt_clear(sch, &retry);
+		vfio_ccw_alloc_crw(private, link, CRW_ERC_PERRN);
 		break;
 	case CHP_VARY_ON:
 		/* Path logically turned on */
@@ -312,6 +384,7 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
 	case CHP_ONLINE:
 		/* Path became available */
 		sch->lpm |= mask & sch->opm;
+		vfio_ccw_alloc_crw(private, link, CRW_ERC_INIT);
 		break;
 	}
 
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index f3033f8fc96d..8b3ed5b45277 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -393,6 +393,7 @@ static int vfio_ccw_mdev_get_irq_info(struct vfio_irq_info *info)
 {
 	switch (info->index) {
 	case VFIO_CCW_IO_IRQ_INDEX:
+	case VFIO_CCW_CRW_IRQ_INDEX:
 		info->count = 1;
 		info->flags = VFIO_IRQ_INFO_EVENTFD;
 		break;
@@ -420,6 +421,9 @@ static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
 	case VFIO_CCW_IO_IRQ_INDEX:
 		ctx = &private->io_trigger;
 		break;
+	case VFIO_CCW_CRW_IRQ_INDEX:
+		ctx = &private->crw_trigger;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 8289b6850e59..a701f09c6943 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -17,6 +17,7 @@
 #include <linux/eventfd.h>
 #include <linux/workqueue.h>
 #include <linux/vfio_ccw.h>
+#include <asm/crw.h>
 #include <asm/debug.h>
 
 #include "css.h"
@@ -59,6 +60,11 @@ int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
 int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private);
 int vfio_ccw_register_crw_dev_regions(struct vfio_ccw_private *private);
 
+struct vfio_ccw_crw {
+	struct list_head	next;
+	struct crw		crw[2];
+};
+
 /**
  * struct vfio_ccw_private
  * @sch: pointer to the subchannel
@@ -98,9 +104,12 @@ struct vfio_ccw_private {
 	struct channel_program	cp;
 	struct irb		irb;
 	union scsw		scsw;
+	struct list_head	crw;
 
 	struct eventfd_ctx	*io_trigger;
+	struct eventfd_ctx	*crw_trigger;
 	struct work_struct	io_work;
+	struct work_struct	crw_work;
 } __aligned(8);
 
 extern int vfio_ccw_mdev_reg(struct subchannel *sch);
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 5024636d7615..1bdf32772545 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -579,6 +579,7 @@ enum {
 
 enum {
 	VFIO_CCW_IO_IRQ_INDEX,
+	VFIO_CCW_CRW_IRQ_INDEX,
 	VFIO_CCW_NUM_IRQS
 };
 
-- 
2.17.1

[RFC PATCH v2 8/9] vfio-ccw: Add trace for CRW event

[Eric Farman]

Since CRW events are (should be) rare, let's put a trace
in that routine too.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c   |  1 +
 drivers/s390/cio/vfio_ccw_trace.c |  1 +
 drivers/s390/cio/vfio_ccw_trace.h | 30 ++++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index c48c260a129d..21952258d71a 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -357,6 +357,7 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
 	if (!private || !mask)
 		return 0;
 
+	trace_vfio_ccw_chp_event(private->sch->schid, mask, event);
 	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
 			   mdev_uuid(private->mdev), sch->schid.cssid,
 			   sch->schid.ssid, sch->schid.sch_no,
diff --git a/drivers/s390/cio/vfio_ccw_trace.c b/drivers/s390/cio/vfio_ccw_trace.c
index 8c671d2519f6..4a0205905afc 100644
--- a/drivers/s390/cio/vfio_ccw_trace.c
+++ b/drivers/s390/cio/vfio_ccw_trace.c
@@ -9,6 +9,7 @@
 #define CREATE_TRACE_POINTS
 #include "vfio_ccw_trace.h"
 
+EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_chp_event);
 EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_fsm_async_request);
 EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_fsm_event);
 EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_fsm_io_request);
diff --git a/drivers/s390/cio/vfio_ccw_trace.h b/drivers/s390/cio/vfio_ccw_trace.h
index 30162a318a8a..8c82d0c27e81 100644
--- a/drivers/s390/cio/vfio_ccw_trace.h
+++ b/drivers/s390/cio/vfio_ccw_trace.h
@@ -17,6 +17,36 @@
 
 #include <linux/tracepoint.h>
 
+TRACE_EVENT(vfio_ccw_chp_event,
+	TP_PROTO(struct subchannel_id schid,
+		 int mask,
+		 int event),
+	TP_ARGS(schid, mask, event),
+
+	TP_STRUCT__entry(
+		__field(u8, cssid)
+		__field(u8, ssid)
+		__field(u16, sch_no)
+		__field(int, mask)
+		__field(int, event)
+	),
+
+	TP_fast_assign(
+		__entry->cssid = schid.cssid;
+		__entry->ssid = schid.ssid;
+		__entry->sch_no = schid.sch_no;
+		__entry->mask = mask;
+		__entry->event = event;
+	),
+
+	TP_printk("schid=%x.%x.%04x mask=0x%x event=%d",
+		  __entry->cssid,
+		  __entry->ssid,
+		  __entry->sch_no,
+		  __entry->mask,
+		  __entry->event)
+);
+
 TRACE_EVENT(vfio_ccw_fsm_async_request,
 	TP_PROTO(struct subchannel_id schid,
 		 int command,
-- 
2.17.1

[RFC PATCH v2 9/9] vfio-ccw: Remove inline get_schid() routine

[Eric Farman]

This seems misplaced in the middle of FSM, returning the schid
field from inside the private struct.  We could move this macro
into vfio_ccw_private.h, but this doesn't seem to simplify things
that much.  Let's just remove it, and use the field directly.

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

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 23e61aa638e4..c4c303645d7d 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -228,10 +228,6 @@ static void fsm_disabled_irq(struct vfio_ccw_private *private,
 	 */
 	cio_disable_subchannel(sch);
 }
-inline struct subchannel_id get_schid(struct vfio_ccw_private *p)
-{
-	return p->sch->schid;
-}
 
 /*
  * Deal with the ccw command request from the userspace.
@@ -244,7 +240,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 	struct ccw_io_region *io_region = private->io_region;
 	struct mdev_device *mdev = private->mdev;
 	char *errstr = "request";
-	struct subchannel_id schid = get_schid(private);
+	struct subchannel_id schid = private->sch->schid;
 
 	private->state = VFIO_CCW_STATE_CP_PROCESSING;
 	memcpy(scsw, io_region->scsw_area, sizeof(*scsw));
@@ -342,7 +338,7 @@ static void fsm_async_request(struct vfio_ccw_private *private,
 		cmd_region->ret_code = -EINVAL;
 	}
 
-	trace_vfio_ccw_fsm_async_request(get_schid(private),
+	trace_vfio_ccw_fsm_async_request(private->sch->schid,
 					 cmd_region->command,
 					 cmd_region->ret_code);
 }
-- 
2.17.1

Re: [RFC PATCH v2 0/9] s390x/vfio-ccw: Channel Path Handling

[Cornelia Huck]

On Thu,  6 Feb 2020 22:38:16 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> Here is a new pass at the channel-path handling code for vfio-ccw.
> This was initially developed by Farhan Ali this past summer, and
> picked up by me.  For my own benefit/sanity, I made a small changelog
> in the commit message for each patch, describing the changes I've
> made to his original code beyond just rebasing to master, rather than
> a giant list appended here.
> 
> I had been encountering a host crash which I think was triggered by
> this code rather than existing within it.  I'd sent a potential fix
> for that separately, but need more diagnosis.  So while that is
> outstanding, I think I've gotten most (but probably not all) comments
> from v1 addressed within.
> 
> With this, and the corresponding QEMU series (to be posted momentarily),
> applied I am able to configure off/on a CHPID (for example, by issuing
> "chchp -c 0/1 xx" on the host), and the guest is able to see both the
> events and reflect the updated path masks in its structures.
> 
> For reasons that are hopefully obvious, issuing chchp within the guest
> only works for the logical vary.  Configuring it off/on does not work,
> which I think is fine.

Before I delve into this: While the basic architecture here (and in the
QEMU part) is still similar, you changed things like handling multiple
CRWs? That's at least the impression I got from a very high-level skim.

> 
> v1: https://lore.kernel.org/kvm/20191115025620.19593-1-farman@linux.ibm.com/
> 
> Eric Farman (4):
>   vfio-ccw: Refactor the unregister of the async regions
>   vfio-ccw: Refactor IRQ handlers
>   vfio-ccw: Add trace for CRW event
>   vfio-ccw: Remove inline get_schid() routine
> 
> Farhan Ali (5):
>   vfio-ccw: Introduce new helper functions to free/destroy regions
>   vfio-ccw: Register a chp_event callback for vfio-ccw
>   vfio-ccw: Introduce a new schib region
>   vfio-ccw: Introduce a new CRW region
>   vfio-ccw: Wire up the CRW irq and CRW region
> 
>  Documentation/s390/vfio-ccw.rst     |  31 ++++-
>  drivers/s390/cio/Makefile           |   2 +-
>  drivers/s390/cio/vfio_ccw_chp.c     | 136 ++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_drv.c     | 186 ++++++++++++++++++++++++++--
>  drivers/s390/cio/vfio_ccw_fsm.c     |   8 +-
>  drivers/s390/cio/vfio_ccw_ops.c     |  65 +++++++---
>  drivers/s390/cio/vfio_ccw_private.h |  16 +++
>  drivers/s390/cio/vfio_ccw_trace.c   |   1 +
>  drivers/s390/cio/vfio_ccw_trace.h   |  30 +++++
>  include/uapi/linux/vfio.h           |   3 +
>  include/uapi/linux/vfio_ccw.h       |  19 +++
>  11 files changed, 463 insertions(+), 34 deletions(-)
>  create mode 100644 drivers/s390/cio/vfio_ccw_chp.c
> 

Re: [RFC PATCH v2 0/9] s390x/vfio-ccw: Channel Path Handling

[Eric Farman]

On 2/7/20 4:12 AM, Cornelia Huck wrote:
> On Thu,  6 Feb 2020 22:38:16 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> Here is a new pass at the channel-path handling code for vfio-ccw.
>> This was initially developed by Farhan Ali this past summer, and
>> picked up by me.  For my own benefit/sanity, I made a small changelog
>> in the commit message for each patch, describing the changes I've
>> made to his original code beyond just rebasing to master, rather than
>> a giant list appended here.
>>
>> I had been encountering a host crash which I think was triggered by
>> this code rather than existing within it.  I'd sent a potential fix
>> for that separately, but need more diagnosis.  So while that is
>> outstanding, I think I've gotten most (but probably not all) comments
>> from v1 addressed within.
>>
>> With this, and the corresponding QEMU series (to be posted momentarily),
>> applied I am able to configure off/on a CHPID (for example, by issuing
>> "chchp -c 0/1 xx" on the host), and the guest is able to see both the
>> events and reflect the updated path masks in its structures.
>>
>> For reasons that are hopefully obvious, issuing chchp within the guest
>> only works for the logical vary.  Configuring it off/on does not work,
>> which I think is fine.
> 
> Before I delve into this: While the basic architecture here (and in the
> QEMU part) is still similar, you changed things like handling multiple
> CRWs? That's at least the impression I got from a very high-level skim.

I hope so.  :)

I'm not chaining any CRWs together using the CRW.C flag, since that will
expose other more interesting problems if we go beyond two.  It should
help handle the "guest processes one while another is added" scenario,
even though I've got some racing during the management of the list itself.

> 
>>
>> v1: https://lore.kernel.org/kvm/20191115025620.19593-1-farman@linux.ibm.com/
>>
>> Eric Farman (4):
>>   vfio-ccw: Refactor the unregister of the async regions
>>   vfio-ccw: Refactor IRQ handlers
>>   vfio-ccw: Add trace for CRW event
>>   vfio-ccw: Remove inline get_schid() routine
>>
>> Farhan Ali (5):
>>   vfio-ccw: Introduce new helper functions to free/destroy regions
>>   vfio-ccw: Register a chp_event callback for vfio-ccw
>>   vfio-ccw: Introduce a new schib region
>>   vfio-ccw: Introduce a new CRW region
>>   vfio-ccw: Wire up the CRW irq and CRW region
>>
>>  Documentation/s390/vfio-ccw.rst     |  31 ++++-
>>  drivers/s390/cio/Makefile           |   2 +-
>>  drivers/s390/cio/vfio_ccw_chp.c     | 136 ++++++++++++++++++++
>>  drivers/s390/cio/vfio_ccw_drv.c     | 186 ++++++++++++++++++++++++++--
>>  drivers/s390/cio/vfio_ccw_fsm.c     |   8 +-
>>  drivers/s390/cio/vfio_ccw_ops.c     |  65 +++++++---
>>  drivers/s390/cio/vfio_ccw_private.h |  16 +++
>>  drivers/s390/cio/vfio_ccw_trace.c   |   1 +
>>  drivers/s390/cio/vfio_ccw_trace.h   |  30 +++++
>>  include/uapi/linux/vfio.h           |   3 +
>>  include/uapi/linux/vfio_ccw.h       |  19 +++
>>  11 files changed, 463 insertions(+), 34 deletions(-)
>>  create mode 100644 drivers/s390/cio/vfio_ccw_chp.c
>>
> 

Re: [RFC PATCH v2 2/9] vfio-ccw: Register a chp_event callback for vfio-ccw

[Cornelia Huck]

On Thu,  6 Feb 2020 22:38:18 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> Register the chp_event callback to receive channel path related
> events for the subchannels managed by vfio-ccw.

I'm wondering how useful this patch would be on its own.

> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v1->v2:
>      - Move s390dbf before cio_update_schib() call [CH]
>     
>     v0->v1: [EF]
>      - Add s390dbf trace
> 
>  drivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
(...)
> @@ -257,6 +258,48 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>  	return rc;
>  }
>  
> +static int vfio_ccw_chp_event(struct subchannel *sch,
> +			      struct chp_link *link, int event)
> +{
> +	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
> +	int mask = chp_ssd_get_mask(&sch->ssd_info, link);
> +	int retry = 255;
> +
> +	if (!private || !mask)
> +		return 0;
> +
> +	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
> +			   mdev_uuid(private->mdev), sch->schid.cssid,
> +			   sch->schid.ssid, sch->schid.sch_no,
> +			   mask, event);
> +
> +	if (cio_update_schib(sch))
> +		return -ENODEV;
> +
> +	switch (event) {
> +	case CHP_VARY_OFF:
> +		/* Path logically turned off */
> +		sch->opm &= ~mask;
> +		sch->lpm &= ~mask;
> +		break;
> +	case CHP_OFFLINE:
> +		/* Path is gone */
> +		cio_cancel_halt_clear(sch, &retry);

Any reason you do this only for CHP_OFFLINE and not for CHP_VARY_OFF?

> +		break;
> +	case CHP_VARY_ON:
> +		/* Path logically turned on */
> +		sch->opm |= mask;
> +		sch->lpm |= mask;
> +		break;
> +	case CHP_ONLINE:
> +		/* Path became available */
> +		sch->lpm |= mask & sch->opm;

If I'm not mistaken, this patch introduces the first usage of sch->opm
in the vfio-ccw code. Are we missing something? Or am I missing
something? :)

> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct css_device_id vfio_ccw_sch_ids[] = {
>  	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
>  	{ /* end of list */ },
(...)

Re: [RFC PATCH v2 4/9] vfio-ccw: Introduce a new schib region

[Cornelia Huck]

On Thu,  6 Feb 2020 22:38:20 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> The schib region can be used by userspace to get the subchannel-
> information block (SCHIB) for the passthrough subchannel.
> This can be useful to get information such as channel path
> information via the SCHIB.PMCW fields.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v1->v2:
>      - Add new region info to Documentation/s390/vfio-ccw.rst [CH]
>      - Add a block comment to struct ccw_schib_region [CH]
>     
>     v0->v1: [EF]
>      - Clean up checkpatch (#include, whitespace) errors
>      - Remove unnecessary includes from vfio_ccw_chp.c
>      - Add ret=-ENOMEM in error path for new region
>      - Add call to vfio_ccw_unregister_dev_regions() during error exit
>        path of vfio_ccw_mdev_open()
>      - New info on the module prologue
>      - Reorder cleanup of regions
> 
>  Documentation/s390/vfio-ccw.rst     | 16 +++++-
>  drivers/s390/cio/Makefile           |  2 +-
>  drivers/s390/cio/vfio_ccw_chp.c     | 75 +++++++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_drv.c     | 20 ++++++++
>  drivers/s390/cio/vfio_ccw_ops.c     | 14 +++++-
>  drivers/s390/cio/vfio_ccw_private.h |  3 ++
>  include/uapi/linux/vfio.h           |  1 +
>  include/uapi/linux/vfio_ccw.h       | 10 ++++
>  8 files changed, 137 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/s390/cio/vfio_ccw_chp.c
> 
> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
> index fca9c4f5bd9c..b805dc995fc8 100644
> --- a/Documentation/s390/vfio-ccw.rst
> +++ b/Documentation/s390/vfio-ccw.rst
> @@ -231,6 +231,19 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD.
>  
>  Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region.
>  
> +vfio-ccw schib region
> +---------------------
> +
> +The vfio-ccw schib region is used to return Subchannel-Information
> +Block (SCHIB) data to userspace::
> +
> +  struct ccw_schib_region {
> +  #define SCHIB_AREA_SIZE 52
> +         __u8 schib_area[SCHIB_AREA_SIZE];
> +  } __packed;
> +
> +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.

Also mention that reading this triggers a stsch() updating the schib?

> +
>  vfio-ccw operation details
>  --------------------------
>  

(...)

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> new file mode 100644
> index 000000000000..826d08379fe3
> --- /dev/null
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -0,0 +1,75 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Channel path related status regions for vfio_ccw
> + *
> + * Copyright IBM Corp. 2019

Should the year be updated?

> + *
> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
> + */

(...)

Re: [RFC PATCH v2 9/9] vfio-ccw: Remove inline get_schid() routine

[Cornelia Huck]

On Thu,  6 Feb 2020 22:38:25 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> This seems misplaced in the middle of FSM, returning the schid
> field from inside the private struct.  We could move this macro
> into vfio_ccw_private.h, but this doesn't seem to simplify things
> that much.  Let's just remove it, and use the field directly.

It had been introduced with the first set of traces, I'm now wondering
why, as it doesn't do any checking.

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

Re: [RFC PATCH v2 7/9] vfio-ccw: Wire up the CRW irq and CRW region

[Cornelia Huck]

On Thu,  6 Feb 2020 22:38:23 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> Use an IRQ to notify userspace that there is a CRW
> pending in the region, related to path-availability
> changes on the passthrough subchannel.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v1->v2:
>      - Remove extraneous 0x0 in crw.rsid assignment [CH]
>      - Refactor the building/queueing of a crw into its own routine [EF]
>     
>     v0->v1: [EF]
>      - Place the non-refactoring changes from the previous patch here
>      - Clean up checkpatch (whitespace) errors
>      - s/chp_crw/crw/
>      - Move acquire/release of io_mutex in vfio_ccw_crw_region_read()
>        into patch that introduces that region
>      - Remove duplicate include from vfio_ccw_drv.c
>      - Reorder include in vfio_ccw_private.h
> 
>  drivers/s390/cio/vfio_ccw_chp.c     |  5 ++
>  drivers/s390/cio/vfio_ccw_drv.c     | 73 +++++++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_ops.c     |  4 ++
>  drivers/s390/cio/vfio_ccw_private.h |  9 ++++
>  include/uapi/linux/vfio.h           |  1 +
>  5 files changed, 92 insertions(+)
> 
(...)
> +static void vfio_ccw_alloc_crw(struct vfio_ccw_private *private,
> +			       struct chp_link *link,
> +			       unsigned int erc)
> +{
> +	struct vfio_ccw_crw *vc_crw;
> +	struct crw *crw;
> +
> +	/*
> +	 * If unable to allocate a CRW, just drop the event and
> +	 * carry on.  The guest will either see a later one or
> +	 * learn when it issues its own store subchannel.
> +	 */
> +	vc_crw = kzalloc(sizeof(*vc_crw), GFP_ATOMIC);
> +	if (!vc_crw)
> +		return;
> +
> +	/*
> +	 * Build in the first CRW space, but don't chain anything
> +	 * into the second one even though the space exists.
> +	 */
> +	crw = &vc_crw->crw[0];
> +
> +	/*
> +	 * Presume every CRW we handle is reported by a channel-path.
> +	 * Maybe not future-proof, but good for what we're doing now.

You could pass in a source indication, maybe? Presumably, at least one
of the callers further up the chain knows...

> +	 *
> +	 * FIXME Sort of a lie, since we're converting a CRW
> +	 * reported by a channel-path into one issued to each
> +	 * subchannel, but still saying it's coming from the path.

It's still channel-path related, though :)

The important point is probably is that userspace needs to be aware
that the same channel-path related event is reported on all affected
subchannels, and they therefore need some appropriate handling on their
side.

> +	 */
> +	crw->rsc = CRW_RSC_CPATH;
> +	crw->rsid = (link->chpid.cssid << 8) | link->chpid.id;
> +	crw->erc = erc;
> +
> +	list_add_tail(&vc_crw->next, &private->crw);
> +	queue_work(vfio_ccw_work_q, &private->crw_work);
> +}
> +
>  static int vfio_ccw_chp_event(struct subchannel *sch,
>  			      struct chp_link *link, int event)
>  {
(...)

Re: [RFC PATCH v2 9/9] vfio-ccw: Remove inline get_schid() routine

[Eric Farman]

On 2/14/20 8:27 AM, Cornelia Huck wrote:
> On Thu,  6 Feb 2020 22:38:25 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> This seems misplaced in the middle of FSM, returning the schid
>> field from inside the private struct.  We could move this macro
>> into vfio_ccw_private.h, but this doesn't seem to simplify things
>> that much.  Let's just remove it, and use the field directly.
> 
> It had been introduced with the first set of traces, I'm now wondering
> why, as it doesn't do any checking.

Hrm, that's a decent question.  I could refactor this, moving the
routine into vfio_ccw_private.h and add those checks, rather than
removing it outright.

I honestly only stumbled on this when I tried to use get_schid()
somewhere else, not realizing the definition was actually tucked away in
here.  And so I've left it on the end of this series as a matter of
convenience/non-forgetfulness, but splitting it out by itself would be fine.

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

Re: [RFC PATCH v2 4/9] vfio-ccw: Introduce a new schib region

[Eric Farman]

On 2/14/20 7:32 AM, Cornelia Huck wrote:
> On Thu,  6 Feb 2020 22:38:20 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> The schib region can be used by userspace to get the subchannel-
>> information block (SCHIB) for the passthrough subchannel.
>> This can be useful to get information such as channel path
>> information via the SCHIB.PMCW fields.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v1->v2:
>>      - Add new region info to Documentation/s390/vfio-ccw.rst [CH]
>>      - Add a block comment to struct ccw_schib_region [CH]
>>     
>>     v0->v1: [EF]
>>      - Clean up checkpatch (#include, whitespace) errors
>>      - Remove unnecessary includes from vfio_ccw_chp.c
>>      - Add ret=-ENOMEM in error path for new region
>>      - Add call to vfio_ccw_unregister_dev_regions() during error exit
>>        path of vfio_ccw_mdev_open()
>>      - New info on the module prologue
>>      - Reorder cleanup of regions
>>
>>  Documentation/s390/vfio-ccw.rst     | 16 +++++-
>>  drivers/s390/cio/Makefile           |  2 +-
>>  drivers/s390/cio/vfio_ccw_chp.c     | 75 +++++++++++++++++++++++++++++
>>  drivers/s390/cio/vfio_ccw_drv.c     | 20 ++++++++
>>  drivers/s390/cio/vfio_ccw_ops.c     | 14 +++++-
>>  drivers/s390/cio/vfio_ccw_private.h |  3 ++
>>  include/uapi/linux/vfio.h           |  1 +
>>  include/uapi/linux/vfio_ccw.h       | 10 ++++
>>  8 files changed, 137 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/s390/cio/vfio_ccw_chp.c
>>
>> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
>> index fca9c4f5bd9c..b805dc995fc8 100644
>> --- a/Documentation/s390/vfio-ccw.rst
>> +++ b/Documentation/s390/vfio-ccw.rst
>> @@ -231,6 +231,19 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD.
>>  
>>  Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region.
>>  
>> +vfio-ccw schib region
>> +---------------------
>> +
>> +The vfio-ccw schib region is used to return Subchannel-Information
>> +Block (SCHIB) data to userspace::
>> +
>> +  struct ccw_schib_region {
>> +  #define SCHIB_AREA_SIZE 52
>> +         __u8 schib_area[SCHIB_AREA_SIZE];
>> +  } __packed;
>> +
>> +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
> 
> Also mention that reading this triggers a stsch() updating the schib?

Yeah, I tucked that in the uapi header, but it should be mentioned here too.

> 
>> +
>>  vfio-ccw operation details
>>  --------------------------
>>  
> 
> (...)
> 
>> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
>> new file mode 100644
>> index 000000000000..826d08379fe3
>> --- /dev/null
>> +++ b/drivers/s390/cio/vfio_ccw_chp.c
>> @@ -0,0 +1,75 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Channel path related status regions for vfio_ccw
>> + *
>> + * Copyright IBM Corp. 2019
> 
> Should the year be updated?

Probably.  :)

> 
>> + *
>> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
>> + */
> 
> (...)
> 

Re: [RFC PATCH v2 7/9] vfio-ccw: Wire up the CRW irq and CRW region

[Eric Farman]

On 2/14/20 8:34 AM, Cornelia Huck wrote:
> On Thu,  6 Feb 2020 22:38:23 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> Use an IRQ to notify userspace that there is a CRW
>> pending in the region, related to path-availability
>> changes on the passthrough subchannel.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v1->v2:
>>      - Remove extraneous 0x0 in crw.rsid assignment [CH]
>>      - Refactor the building/queueing of a crw into its own routine [EF]
>>     
>>     v0->v1: [EF]
>>      - Place the non-refactoring changes from the previous patch here
>>      - Clean up checkpatch (whitespace) errors
>>      - s/chp_crw/crw/
>>      - Move acquire/release of io_mutex in vfio_ccw_crw_region_read()
>>        into patch that introduces that region
>>      - Remove duplicate include from vfio_ccw_drv.c
>>      - Reorder include in vfio_ccw_private.h
>>
>>  drivers/s390/cio/vfio_ccw_chp.c     |  5 ++
>>  drivers/s390/cio/vfio_ccw_drv.c     | 73 +++++++++++++++++++++++++++++
>>  drivers/s390/cio/vfio_ccw_ops.c     |  4 ++
>>  drivers/s390/cio/vfio_ccw_private.h |  9 ++++
>>  include/uapi/linux/vfio.h           |  1 +
>>  5 files changed, 92 insertions(+)
>>
> (...)
>> +static void vfio_ccw_alloc_crw(struct vfio_ccw_private *private,
>> +			       struct chp_link *link,
>> +			       unsigned int erc)
>> +{
>> +	struct vfio_ccw_crw *vc_crw;
>> +	struct crw *crw;
>> +
>> +	/*
>> +	 * If unable to allocate a CRW, just drop the event and
>> +	 * carry on.  The guest will either see a later one or
>> +	 * learn when it issues its own store subchannel.
>> +	 */
>> +	vc_crw = kzalloc(sizeof(*vc_crw), GFP_ATOMIC);
>> +	if (!vc_crw)
>> +		return;
>> +
>> +	/*
>> +	 * Build in the first CRW space, but don't chain anything
>> +	 * into the second one even though the space exists.
>> +	 */
>> +	crw = &vc_crw->crw[0];
>> +
>> +	/*
>> +	 * Presume every CRW we handle is reported by a channel-path.
>> +	 * Maybe not future-proof, but good for what we're doing now.
> 
> You could pass in a source indication, maybe? Presumably, at least one
> of the callers further up the chain knows...

The "chain" is the vfio_ccw_chp_event() function called off the
.chp_event callback, and then to this point.  So I don't think there's
much we can get back from our callchain, other than the CHP_xxxLINE
event that got us here.

> 
>> +	 *
>> +	 * FIXME Sort of a lie, since we're converting a CRW
>> +	 * reported by a channel-path into one issued to each
>> +	 * subchannel, but still saying it's coming from the path.
> 
> It's still channel-path related, though :)
> 
> The important point is probably is that userspace needs to be aware
> that the same channel-path related event is reported on all affected
> subchannels, and they therefore need some appropriate handling on their
> side.

This is true.  And the fact that the RSC and RSID fields will be in
agreement is helpful.  But yes, the fact that userspace should expect
the possibility of more than one CRW per channel path is the thing I'm
still not enjoying.  Mostly because of the race between queueing
additional ones, and unqueuing them on the other side.  So probably not
much that can be done here but awareness.

> 
>> +	 */
>> +	crw->rsc = CRW_RSC_CPATH;
>> +	crw->rsid = (link->chpid.cssid << 8) | link->chpid.id;
>> +	crw->erc = erc;
>> +
>> +	list_add_tail(&vc_crw->next, &private->crw);
>> +	queue_work(vfio_ccw_work_q, &private->crw_work);
>> +}
>> +
>>  static int vfio_ccw_chp_event(struct subchannel *sch,
>>  			      struct chp_link *link, int event)
>>  {
> (...)
> 

Re: [RFC PATCH v2 2/9] vfio-ccw: Register a chp_event callback for vfio-ccw

[Eric Farman]

On 2/14/20 7:11 AM, Cornelia Huck wrote:
> On Thu,  6 Feb 2020 22:38:18 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> Register the chp_event callback to receive channel path related
>> events for the subchannels managed by vfio-ccw.
> 
> I'm wondering how useful this patch would be on its own.

Probably not much.  I can't speak to his original thoughts on the
matter, but it doesn't seem to buy us much by itself other than a
consumable sized patch.

> 
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v1->v2:
>>      - Move s390dbf before cio_update_schib() call [CH]
>>     
>>     v0->v1: [EF]
>>      - Add s390dbf trace
>>
>>  drivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>
> (...)
>> @@ -257,6 +258,48 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>>  	return rc;
>>  }
>>  
>> +static int vfio_ccw_chp_event(struct subchannel *sch,
>> +			      struct chp_link *link, int event)
>> +{
>> +	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>> +	int mask = chp_ssd_get_mask(&sch->ssd_info, link);
>> +	int retry = 255;
>> +
>> +	if (!private || !mask)
>> +		return 0;
>> +
>> +	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
>> +			   mdev_uuid(private->mdev), sch->schid.cssid,
>> +			   sch->schid.ssid, sch->schid.sch_no,
>> +			   mask, event);
>> +
>> +	if (cio_update_schib(sch))
>> +		return -ENODEV;
>> +
>> +	switch (event) {
>> +	case CHP_VARY_OFF:
>> +		/* Path logically turned off */
>> +		sch->opm &= ~mask;
>> +		sch->lpm &= ~mask;
>> +		break;
>> +	case CHP_OFFLINE:
>> +		/* Path is gone */
>> +		cio_cancel_halt_clear(sch, &retry);
> 
> Any reason you do this only for CHP_OFFLINE and not for CHP_VARY_OFF?

Hrm...  No reason that I can think of.  I can fix this.

> 
>> +		break;
>> +	case CHP_VARY_ON:
>> +		/* Path logically turned on */
>> +		sch->opm |= mask;
>> +		sch->lpm |= mask;
>> +		break;
>> +	case CHP_ONLINE:
>> +		/* Path became available */
>> +		sch->lpm |= mask & sch->opm;
> 
> If I'm not mistaken, this patch introduces the first usage of sch->opm
> in the vfio-ccw code. 

Correct.

> Are we missing something?

Maybe?  :)

>Or am I missing
> something? :)
> 

Since it's only used in this code, for acting as a step between
vary/config off/on, maybe this only needs to be dealing with the lpm
field itself?

>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static struct css_device_id vfio_ccw_sch_ids[] = {
>>  	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
>>  	{ /* end of list */ },
> (...)
> 

Re: [RFC PATCH v2 2/9] vfio-ccw: Register a chp_event callback for vfio-ccw

[Cornelia Huck]

On Fri, 14 Feb 2020 11:35:21 -0500
Eric Farman <farman@linux.ibm.com> wrote:

> On 2/14/20 7:11 AM, Cornelia Huck wrote:
> > On Thu,  6 Feb 2020 22:38:18 +0100
> > Eric Farman <farman@linux.ibm.com> wrote:

> > (...)  
> >> @@ -257,6 +258,48 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
> >>  	return rc;
> >>  }
> >>  
> >> +static int vfio_ccw_chp_event(struct subchannel *sch,
> >> +			      struct chp_link *link, int event)
> >> +{
> >> +	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
> >> +	int mask = chp_ssd_get_mask(&sch->ssd_info, link);
> >> +	int retry = 255;
> >> +
> >> +	if (!private || !mask)
> >> +		return 0;
> >> +
> >> +	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
> >> +			   mdev_uuid(private->mdev), sch->schid.cssid,
> >> +			   sch->schid.ssid, sch->schid.sch_no,
> >> +			   mask, event);
> >> +
> >> +	if (cio_update_schib(sch))
> >> +		return -ENODEV;
> >> +
> >> +	switch (event) {
> >> +	case CHP_VARY_OFF:
> >> +		/* Path logically turned off */
> >> +		sch->opm &= ~mask;
> >> +		sch->lpm &= ~mask;
> >> +		break;
> >> +	case CHP_OFFLINE:
> >> +		/* Path is gone */
> >> +		cio_cancel_halt_clear(sch, &retry);  
> > 
> > Any reason you do this only for CHP_OFFLINE and not for CHP_VARY_OFF?  
> 
> Hrm...  No reason that I can think of.  I can fix this.
> 
> >   
> >> +		break;
> >> +	case CHP_VARY_ON:
> >> +		/* Path logically turned on */
> >> +		sch->opm |= mask;
> >> +		sch->lpm |= mask;
> >> +		break;
> >> +	case CHP_ONLINE:
> >> +		/* Path became available */
> >> +		sch->lpm |= mask & sch->opm;  
> > 
> > If I'm not mistaken, this patch introduces the first usage of sch->opm
> > in the vfio-ccw code.   
> 
> Correct.
> 
> > Are we missing something?  
> 
> Maybe?  :)
> 
> >Or am I missing
> > something? :)
> >   
> 
> Since it's only used in this code, for acting as a step between
> vary/config off/on, maybe this only needs to be dealing with the lpm
> field itself?

Ok, I went over this again and also looked at what the standard I/O
subchannel driver does, and I think this is fine, as the lpm basically
factors in the opm already. (Will need to keep this in mind for the
following patches.)

> 
> >> +		break;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static struct css_device_id vfio_ccw_sch_ids[] = {
> >>  	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
> >>  	{ /* end of list */ },  
> > (...)
> >   
> 

Re: [RFC PATCH v2 7/9] vfio-ccw: Wire up the CRW irq and CRW region

[Cornelia Huck]

On Fri, 14 Feb 2020 11:24:39 -0500
Eric Farman <farman@linux.ibm.com> wrote:

> On 2/14/20 8:34 AM, Cornelia Huck wrote:
> > On Thu,  6 Feb 2020 22:38:23 +0100
> > Eric Farman <farman@linux.ibm.com> wrote:

> > (...)  
> >> +static void vfio_ccw_alloc_crw(struct vfio_ccw_private *private,
> >> +			       struct chp_link *link,
> >> +			       unsigned int erc)
> >> +{
> >> +	struct vfio_ccw_crw *vc_crw;
> >> +	struct crw *crw;
> >> +
> >> +	/*
> >> +	 * If unable to allocate a CRW, just drop the event and
> >> +	 * carry on.  The guest will either see a later one or
> >> +	 * learn when it issues its own store subchannel.
> >> +	 */
> >> +	vc_crw = kzalloc(sizeof(*vc_crw), GFP_ATOMIC);
> >> +	if (!vc_crw)
> >> +		return;
> >> +
> >> +	/*
> >> +	 * Build in the first CRW space, but don't chain anything
> >> +	 * into the second one even though the space exists.
> >> +	 */
> >> +	crw = &vc_crw->crw[0];
> >> +
> >> +	/*
> >> +	 * Presume every CRW we handle is reported by a channel-path.
> >> +	 * Maybe not future-proof, but good for what we're doing now.  
> > 
> > You could pass in a source indication, maybe? Presumably, at least one
> > of the callers further up the chain knows...  
> 
> The "chain" is the vfio_ccw_chp_event() function called off the
> .chp_event callback, and then to this point.  So I don't think there's
> much we can get back from our callchain, other than the CHP_xxxLINE
> event that got us here.

We might want to pass in CRW_RSC_CPATH, that would make it a bit more
flexible. We can easily rearrange code internally later, though.

Re: [RFC PATCH v2 2/9] vfio-ccw: Register a chp_event callback for vfio-ccw

[Eric Farman]

On 3/24/20 11:58 AM, Cornelia Huck wrote:
> On Fri, 14 Feb 2020 11:35:21 -0500
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> On 2/14/20 7:11 AM, Cornelia Huck wrote:
>>> On Thu,  6 Feb 2020 22:38:18 +0100
>>> Eric Farman <farman@linux.ibm.com> wrote:
> 
>>> (...)  
>>>> @@ -257,6 +258,48 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>>>>  	return rc;
>>>>  }
>>>>  
>>>> +static int vfio_ccw_chp_event(struct subchannel *sch,
>>>> +			      struct chp_link *link, int event)
>>>> +{
>>>> +	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>>>> +	int mask = chp_ssd_get_mask(&sch->ssd_info, link);
>>>> +	int retry = 255;
>>>> +
>>>> +	if (!private || !mask)
>>>> +		return 0;
>>>> +
>>>> +	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
>>>> +			   mdev_uuid(private->mdev), sch->schid.cssid,
>>>> +			   sch->schid.ssid, sch->schid.sch_no,
>>>> +			   mask, event);
>>>> +
>>>> +	if (cio_update_schib(sch))
>>>> +		return -ENODEV;
>>>> +
>>>> +	switch (event) {
>>>> +	case CHP_VARY_OFF:
>>>> +		/* Path logically turned off */
>>>> +		sch->opm &= ~mask;
>>>> +		sch->lpm &= ~mask;
>>>> +		break;
>>>> +	case CHP_OFFLINE:
>>>> +		/* Path is gone */
>>>> +		cio_cancel_halt_clear(sch, &retry);  
>>>
>>> Any reason you do this only for CHP_OFFLINE and not for CHP_VARY_OFF?  
>>
>> Hrm...  No reason that I can think of.  I can fix this.
>>
>>>   
>>>> +		break;
>>>> +	case CHP_VARY_ON:
>>>> +		/* Path logically turned on */
>>>> +		sch->opm |= mask;
>>>> +		sch->lpm |= mask;
>>>> +		break;
>>>> +	case CHP_ONLINE:
>>>> +		/* Path became available */
>>>> +		sch->lpm |= mask & sch->opm;  
>>>
>>> If I'm not mistaken, this patch introduces the first usage of sch->opm
>>> in the vfio-ccw code.   
>>
>> Correct.
>>
>>> Are we missing something?  
>>
>> Maybe?  :)
>>
>>> Or am I missing
>>> something? :)
>>>   
>>
>> Since it's only used in this code, for acting as a step between
>> vary/config off/on, maybe this only needs to be dealing with the lpm
>> field itself?
> 
> Ok, I went over this again and also looked at what the standard I/O
> subchannel driver does, and I think this is fine, as the lpm basically
> factors in the opm already. (Will need to keep this in mind for the
> following patches.)

Just to make sure I don't misunderstand, when you say "I think this is
fine" ... Do you mean keeping the opm field within vfio-ccw, as this
patch does?  Or removing it, and only adjusting the lpm within vfio-ccw,
as I suggested in my response just above?

(It's long in the day, and should not look at vfio-ccw at this hour.)

> 
>>
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static struct css_device_id vfio_ccw_sch_ids[] = {
>>>>  	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
>>>>  	{ /* end of list */ },  
>>> (...)
>>>   
>>
> 

Re: [RFC PATCH v2 2/9] vfio-ccw: Register a chp_event callback for vfio-ccw

[Cornelia Huck]

On Wed, 25 Mar 2020 22:09:40 -0400
Eric Farman <farman@linux.ibm.com> wrote:

> On 3/24/20 11:58 AM, Cornelia Huck wrote:
> > On Fri, 14 Feb 2020 11:35:21 -0500
> > Eric Farman <farman@linux.ibm.com> wrote:
> >   
> >> On 2/14/20 7:11 AM, Cornelia Huck wrote:  
> >>> On Thu,  6 Feb 2020 22:38:18 +0100
> >>> Eric Farman <farman@linux.ibm.com> wrote:  

> >>>> +	case CHP_ONLINE:
> >>>> +		/* Path became available */
> >>>> +		sch->lpm |= mask & sch->opm;    
> >>>
> >>> If I'm not mistaken, this patch introduces the first usage of sch->opm
> >>> in the vfio-ccw code.     
> >>
> >> Correct.
> >>  
> >>> Are we missing something?    
> >>
> >> Maybe?  :)
> >>  
> >>> Or am I missing
> >>> something? :)
> >>>     
> >>
> >> Since it's only used in this code, for acting as a step between
> >> vary/config off/on, maybe this only needs to be dealing with the lpm
> >> field itself?  
> > 
> > Ok, I went over this again and also looked at what the standard I/O
> > subchannel driver does, and I think this is fine, as the lpm basically
> > factors in the opm already. (Will need to keep this in mind for the
> > following patches.)  
> 
> Just to make sure I don't misunderstand, when you say "I think this is
> fine" ... Do you mean keeping the opm field within vfio-ccw, as this
> patch does?  Or removing it, and only adjusting the lpm within vfio-ccw,
> as I suggested in my response just above?

I meant the code change done in this patch: We update the lpm whenever
the opm is changed, and use the lpm. I'd like to keep the opm separate,
just so that we are clear where each value comes from.

Re: [RFC PATCH v2 2/9] vfio-ccw: Register a chp_event callback for vfio-ccw

[Eric Farman]

On 3/26/20 2:47 AM, Cornelia Huck wrote:
> On Wed, 25 Mar 2020 22:09:40 -0400
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> On 3/24/20 11:58 AM, Cornelia Huck wrote:
>>> On Fri, 14 Feb 2020 11:35:21 -0500
>>> Eric Farman <farman@linux.ibm.com> wrote:
>>>   
>>>> On 2/14/20 7:11 AM, Cornelia Huck wrote:  
>>>>> On Thu,  6 Feb 2020 22:38:18 +0100
>>>>> Eric Farman <farman@linux.ibm.com> wrote:  
> 
>>>>>> +	case CHP_ONLINE:
>>>>>> +		/* Path became available */
>>>>>> +		sch->lpm |= mask & sch->opm;    
>>>>>
>>>>> If I'm not mistaken, this patch introduces the first usage of sch->opm
>>>>> in the vfio-ccw code.     
>>>>
>>>> Correct.
>>>>  
>>>>> Are we missing something?    
>>>>
>>>> Maybe?  :)
>>>>  
>>>>> Or am I missing
>>>>> something? :)
>>>>>     
>>>>
>>>> Since it's only used in this code, for acting as a step between
>>>> vary/config off/on, maybe this only needs to be dealing with the lpm
>>>> field itself?  
>>>
>>> Ok, I went over this again and also looked at what the standard I/O
>>> subchannel driver does, and I think this is fine, as the lpm basically
>>> factors in the opm already. (Will need to keep this in mind for the
>>> following patches.)  
>>
>> Just to make sure I don't misunderstand, when you say "I think this is
>> fine" ... Do you mean keeping the opm field within vfio-ccw, as this
>> patch does?  Or removing it, and only adjusting the lpm within vfio-ccw,
>> as I suggested in my response just above?
> 
> I meant the code change done in this patch: We update the lpm whenever
> the opm is changed, and use the lpm. I'd like to keep the opm separate,
> just so that we are clear where each value comes from.
> 

Great.  Thanks for that clarification.

Re: [RFC PATCH v2 7/9] vfio-ccw: Wire up the CRW irq and CRW region

[Eric Farman]

On 3/24/20 12:34 PM, Cornelia Huck wrote:
> On Fri, 14 Feb 2020 11:24:39 -0500
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> On 2/14/20 8:34 AM, Cornelia Huck wrote:
>>> On Thu,  6 Feb 2020 22:38:23 +0100
>>> Eric Farman <farman@linux.ibm.com> wrote:
> 
>>> (...)  
>>>> +static void vfio_ccw_alloc_crw(struct vfio_ccw_private *private,
>>>> +			       struct chp_link *link,
>>>> +			       unsigned int erc)
>>>> +{
>>>> +	struct vfio_ccw_crw *vc_crw;
>>>> +	struct crw *crw;
>>>> +
>>>> +	/*
>>>> +	 * If unable to allocate a CRW, just drop the event and
>>>> +	 * carry on.  The guest will either see a later one or
>>>> +	 * learn when it issues its own store subchannel.
>>>> +	 */
>>>> +	vc_crw = kzalloc(sizeof(*vc_crw), GFP_ATOMIC);
>>>> +	if (!vc_crw)
>>>> +		return;
>>>> +
>>>> +	/*
>>>> +	 * Build in the first CRW space, but don't chain anything
>>>> +	 * into the second one even though the space exists.
>>>> +	 */
>>>> +	crw = &vc_crw->crw[0];
>>>> +
>>>> +	/*
>>>> +	 * Presume every CRW we handle is reported by a channel-path.
>>>> +	 * Maybe not future-proof, but good for what we're doing now.  
>>>
>>> You could pass in a source indication, maybe? Presumably, at least one
>>> of the callers further up the chain knows...  
>>
>> The "chain" is the vfio_ccw_chp_event() function called off the
>> .chp_event callback, and then to this point.  So I don't think there's
>> much we can get back from our callchain, other than the CHP_xxxLINE
>> event that got us here.
> 
> We might want to pass in CRW_RSC_CPATH, that would make it a bit more
> flexible. We can easily rearrange code internally later, though.
> 

This is true...  I'll rearrange it so the routine takes the rsid as
input instead of the link, as well as the rsc, so we don't have to do
that fiddling down the road.

Re: [RFC PATCH v2 5/9] vfio-ccw: Introduce a new CRW region

[Cornelia Huck]

On Thu,  6 Feb 2020 22:38:21 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> This region provides a mechanism to pass Channel Report Word(s)
> that affect vfio-ccw devices, and need to be passed to the guest
> for its awareness and/or processing.
> 
> The base driver (see crw_collect_info()) provides space for two
> CRWs, as a subchannel event may have two CRWs chained together
> (one for the ssid, one for the subcahnnel).  All other CRWs will
> only occupy the first one.  Even though this support will also
> only utilize the first one, we'll provide space for two also.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v1-v2:
>      - Add new region info to Documentation/s390/vfio-ccw.rst [CH]
>      - Add a block comment to struct ccw_crw_region [CH]
>     
>     v0->v1: [EF]
>      - Clean up checkpatch (whitespace) errors
>      - Add ret=-ENOMEM in error path for new region
>      - Add io_mutex for region read (originally in last patch)
>      - Change crw1/crw2 to crw0/crw1
>      - Reorder cleanup of regions
> 
>  Documentation/s390/vfio-ccw.rst     | 15 ++++++++
>  drivers/s390/cio/vfio_ccw_chp.c     | 56 +++++++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_drv.c     | 20 +++++++++++
>  drivers/s390/cio/vfio_ccw_ops.c     |  4 +++
>  drivers/s390/cio/vfio_ccw_private.h |  3 ++
>  include/uapi/linux/vfio.h           |  1 +
>  include/uapi/linux/vfio_ccw.h       |  9 +++++
>  7 files changed, 108 insertions(+)
> 

(...)

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 826d08379fe3..8fde94552149 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -73,3 +73,59 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
>  					    VFIO_REGION_INFO_FLAG_READ,
>  					    private->schib_region);
>  }
> +
> +static ssize_t vfio_ccw_crw_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_crw_region *region;
> +	int ret;
> +
> +	if (pos + count > sizeof(*region))
> +		return -EINVAL;
> +
> +	if (list_empty(&private->crw))
> +		return 0;
> +
> +	mutex_lock(&private->io_mutex);
> +	region = private->region[i].data;
> +
> +	if (copy_to_user(buf, (void *)region + pos, count))
> +		ret = -EFAULT;
> +	else
> +		ret = count;
> +
> +	mutex_unlock(&private->io_mutex);
> +	return ret;
> +}

Would it make sense to clear out the crw after it has been read by
userspace?

In patch 7, you add a notification for a new crw via eventfd, but
nothing is preventing userspace from reading this even if not
triggered. I also don't see the region being updated there until a new
crw is posted.

Or am I missing something?

Re: [RFC PATCH v2 7/9] vfio-ccw: Wire up the CRW irq and CRW region

[Cornelia Huck]

On Thu,  6 Feb 2020 22:38:23 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> Use an IRQ to notify userspace that there is a CRW
> pending in the region, related to path-availability
> changes on the passthrough subchannel.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v1->v2:
>      - Remove extraneous 0x0 in crw.rsid assignment [CH]
>      - Refactor the building/queueing of a crw into its own routine [EF]
>     
>     v0->v1: [EF]
>      - Place the non-refactoring changes from the previous patch here
>      - Clean up checkpatch (whitespace) errors
>      - s/chp_crw/crw/
>      - Move acquire/release of io_mutex in vfio_ccw_crw_region_read()
>        into patch that introduces that region
>      - Remove duplicate include from vfio_ccw_drv.c
>      - Reorder include in vfio_ccw_private.h
> 
>  drivers/s390/cio/vfio_ccw_chp.c     |  5 ++
>  drivers/s390/cio/vfio_ccw_drv.c     | 73 +++++++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_ops.c     |  4 ++
>  drivers/s390/cio/vfio_ccw_private.h |  9 ++++
>  include/uapi/linux/vfio.h           |  1 +
>  5 files changed, 92 insertions(+)

[I may have gotten all muddled up from staring at this, but please bear
with me...]

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 8fde94552149..328b4e1d1972 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -98,6 +98,11 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  		ret = count;
>  
>  	mutex_unlock(&private->io_mutex);
> +
> +	/* Notify the guest if more CRWs are on our queue */
> +	if (!list_empty(&private->crw) && private->crw_trigger)
> +		eventfd_signal(private->crw_trigger, 1);

Here we possibly arm the eventfd again, but don't do anything regarding
queued crws and the region.

> +
>  	return ret;
>  }
>  
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 1e1360af1b34..c48c260a129d 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -108,6 +108,31 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>  		eventfd_signal(private->io_trigger, 1);
>  }
>  
> +static void vfio_ccw_crw_todo(struct work_struct *work)
> +{
> +	struct vfio_ccw_private *private;
> +	struct vfio_ccw_crw *crw;
> +
> +	private = container_of(work, struct vfio_ccw_private, crw_work);
> +
> +	/* FIXME Ugh, need better control of this list */
> +	crw = list_first_entry_or_null(&private->crw,
> +				       struct vfio_ccw_crw, next);
> +
> +	if (crw) {
> +		list_del(&crw->next);
> +
> +		mutex_lock(&private->io_mutex);
> +		memcpy(&private->crw_region->crw0, crw->crw, sizeof(*crw->crw));
> +		mutex_unlock(&private->io_mutex);
> +
> +		kfree(crw);
> +
> +		if (private->crw_trigger)
> +			eventfd_signal(private->crw_trigger, 1);
> +	}
> +}

This function copies one outstanding crw and arms the eventfd.

> +
>  /*
>   * Css driver callbacks
>   */

(...)

> @@ -276,6 +309,44 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>  	return rc;
>  }
>  
> +static void vfio_ccw_alloc_crw(struct vfio_ccw_private *private,
> +			       struct chp_link *link,
> +			       unsigned int erc)
> +{
> +	struct vfio_ccw_crw *vc_crw;
> +	struct crw *crw;
> +
> +	/*
> +	 * If unable to allocate a CRW, just drop the event and
> +	 * carry on.  The guest will either see a later one or
> +	 * learn when it issues its own store subchannel.
> +	 */
> +	vc_crw = kzalloc(sizeof(*vc_crw), GFP_ATOMIC);
> +	if (!vc_crw)
> +		return;
> +
> +	/*
> +	 * Build in the first CRW space, but don't chain anything
> +	 * into the second one even though the space exists.
> +	 */
> +	crw = &vc_crw->crw[0];
> +
> +	/*
> +	 * Presume every CRW we handle is reported by a channel-path.
> +	 * Maybe not future-proof, but good for what we're doing now.
> +	 *
> +	 * FIXME Sort of a lie, since we're converting a CRW
> +	 * reported by a channel-path into one issued to each
> +	 * subchannel, but still saying it's coming from the path.
> +	 */
> +	crw->rsc = CRW_RSC_CPATH;
> +	crw->rsid = (link->chpid.cssid << 8) | link->chpid.id;
> +	crw->erc = erc;
> +
> +	list_add_tail(&vc_crw->next, &private->crw);
> +	queue_work(vfio_ccw_work_q, &private->crw_work);

This function allocates a new crw and queues it. After that, it
triggers the function doing the copy-to-region-and-notify stuff.

> +}
> +
>  static int vfio_ccw_chp_event(struct subchannel *sch,
>  			      struct chp_link *link, int event)
>  {
> @@ -303,6 +374,7 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
>  	case CHP_OFFLINE:
>  		/* Path is gone */
>  		cio_cancel_halt_clear(sch, &retry);
> +		vfio_ccw_alloc_crw(private, link, CRW_ERC_PERRN);
>  		break;
>  	case CHP_VARY_ON:
>  		/* Path logically turned on */
> @@ -312,6 +384,7 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
>  	case CHP_ONLINE:
>  		/* Path became available */
>  		sch->lpm |= mask & sch->opm;
> +		vfio_ccw_alloc_crw(private, link, CRW_ERC_INIT);
>  		break;
>  	}
>  

These two (path online/offline handling) are the only code paths
triggering an update to the queued crws.

Aren't we missing copying in a new queued crw after userspace had done
a read?

Re: [RFC PATCH v2 5/9] vfio-ccw: Introduce a new CRW region

[Eric Farman]

(Ah, didn't see this come in earlier.)

On 4/6/20 9:40 AM, Cornelia Huck wrote:
> On Thu,  6 Feb 2020 22:38:21 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> This region provides a mechanism to pass Channel Report Word(s)
>> that affect vfio-ccw devices, and need to be passed to the guest
>> for its awareness and/or processing.
>>
>> The base driver (see crw_collect_info()) provides space for two
>> CRWs, as a subchannel event may have two CRWs chained together
>> (one for the ssid, one for the subcahnnel).  All other CRWs will
>> only occupy the first one.  Even though this support will also
>> only utilize the first one, we'll provide space for two also.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v1-v2:
>>      - Add new region info to Documentation/s390/vfio-ccw.rst [CH]
>>      - Add a block comment to struct ccw_crw_region [CH]
>>     
>>     v0->v1: [EF]
>>      - Clean up checkpatch (whitespace) errors
>>      - Add ret=-ENOMEM in error path for new region
>>      - Add io_mutex for region read (originally in last patch)
>>      - Change crw1/crw2 to crw0/crw1
>>      - Reorder cleanup of regions
>>
>>  Documentation/s390/vfio-ccw.rst     | 15 ++++++++
>>  drivers/s390/cio/vfio_ccw_chp.c     | 56 +++++++++++++++++++++++++++++
>>  drivers/s390/cio/vfio_ccw_drv.c     | 20 +++++++++++
>>  drivers/s390/cio/vfio_ccw_ops.c     |  4 +++
>>  drivers/s390/cio/vfio_ccw_private.h |  3 ++
>>  include/uapi/linux/vfio.h           |  1 +
>>  include/uapi/linux/vfio_ccw.h       |  9 +++++
>>  7 files changed, 108 insertions(+)
>>
> 
> (...)
> 
>> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
>> index 826d08379fe3..8fde94552149 100644
>> --- a/drivers/s390/cio/vfio_ccw_chp.c
>> +++ b/drivers/s390/cio/vfio_ccw_chp.c
>> @@ -73,3 +73,59 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
>>  					    VFIO_REGION_INFO_FLAG_READ,
>>  					    private->schib_region);
>>  }
>> +
>> +static ssize_t vfio_ccw_crw_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_crw_region *region;
>> +	int ret;
>> +
>> +	if (pos + count > sizeof(*region))
>> +		return -EINVAL;
>> +
>> +	if (list_empty(&private->crw))
>> +		return 0;

This is actually nonsense, because the list isn't introduced for a
couple of patches.

>> +
>> +	mutex_lock(&private->io_mutex);
>> +	region = private->region[i].data;
>> +
>> +	if (copy_to_user(buf, (void *)region + pos, count))
>> +		ret = -EFAULT;
>> +	else
>> +		ret = count;
>> +
>> +	mutex_unlock(&private->io_mutex);
>> +	return ret;
>> +}
> 
> Would it make sense to clear out the crw after it has been read by
> userspace?

Yes, I fixed this up in my in-progress v3 last week.  Sorry to waste
some time, I should have mentioned it when I changed my branch locally.

I changed the list_empty() check above to bail out if the region is
already zero, which I guess is why the userspace check looks for both.
But if we only look at the contents of the region, then checking both is
unnecessary.  Just do the copy and be done with it.

> 
> In patch 7, you add a notification for a new crw via eventfd, but
> nothing is preventing userspace from reading this even if not
> triggered. I also don't see the region being updated there until a new
> crw is posted.
> 
> Or am I missing something?
> 

Re: [RFC PATCH v2 7/9] vfio-ccw: Wire up the CRW irq and CRW region

[Eric Farman]

On 4/6/20 9:52 AM, Cornelia Huck wrote:
> On Thu,  6 Feb 2020 22:38:23 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> Use an IRQ to notify userspace that there is a CRW
>> pending in the region, related to path-availability
>> changes on the passthrough subchannel.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v1->v2:
>>      - Remove extraneous 0x0 in crw.rsid assignment [CH]
>>      - Refactor the building/queueing of a crw into its own routine [EF]
>>     
>>     v0->v1: [EF]
>>      - Place the non-refactoring changes from the previous patch here
>>      - Clean up checkpatch (whitespace) errors
>>      - s/chp_crw/crw/
>>      - Move acquire/release of io_mutex in vfio_ccw_crw_region_read()
>>        into patch that introduces that region
>>      - Remove duplicate include from vfio_ccw_drv.c
>>      - Reorder include in vfio_ccw_private.h
>>
>>  drivers/s390/cio/vfio_ccw_chp.c     |  5 ++
>>  drivers/s390/cio/vfio_ccw_drv.c     | 73 +++++++++++++++++++++++++++++
>>  drivers/s390/cio/vfio_ccw_ops.c     |  4 ++
>>  drivers/s390/cio/vfio_ccw_private.h |  9 ++++
>>  include/uapi/linux/vfio.h           |  1 +
>>  5 files changed, 92 insertions(+)
> 
> [I may have gotten all muddled up from staring at this, but please bear
> with me...]
> 
...snip...
> 
> Aren't we missing copying in a new queued crw after userspace had done
> a read?
> 

Um, huh.  I'll doublecheck that after dinner, but it sure looks like
you're right.

(Might not get back to you tomorrow, because I don't have much time
until Wednesday.)

Re: [RFC PATCH v2 5/9] vfio-ccw: Introduce a new CRW region

[Cornelia Huck]

On Mon, 6 Apr 2020 17:43:42 -0400
Eric Farman <farman@linux.ibm.com> wrote:

> (Ah, didn't see this come in earlier.)
> 
> On 4/6/20 9:40 AM, Cornelia Huck wrote:
> > On Thu,  6 Feb 2020 22:38:21 +0100
> > Eric Farman <farman@linux.ibm.com> wrote:
> >   
> >> From: Farhan Ali <alifm@linux.ibm.com>
> >>
> >> This region provides a mechanism to pass Channel Report Word(s)
> >> that affect vfio-ccw devices, and need to be passed to the guest
> >> for its awareness and/or processing.
> >>
> >> The base driver (see crw_collect_info()) provides space for two
> >> CRWs, as a subchannel event may have two CRWs chained together
> >> (one for the ssid, one for the subcahnnel).  All other CRWs will
> >> only occupy the first one.  Even though this support will also
> >> only utilize the first one, we'll provide space for two also.
> >>
> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> >> ---
> >>
> >> Notes:
> >>     v1-v2:
> >>      - Add new region info to Documentation/s390/vfio-ccw.rst [CH]
> >>      - Add a block comment to struct ccw_crw_region [CH]
> >>     
> >>     v0->v1: [EF]
> >>      - Clean up checkpatch (whitespace) errors
> >>      - Add ret=-ENOMEM in error path for new region
> >>      - Add io_mutex for region read (originally in last patch)
> >>      - Change crw1/crw2 to crw0/crw1
> >>      - Reorder cleanup of regions
> >>
> >>  Documentation/s390/vfio-ccw.rst     | 15 ++++++++
> >>  drivers/s390/cio/vfio_ccw_chp.c     | 56 +++++++++++++++++++++++++++++
> >>  drivers/s390/cio/vfio_ccw_drv.c     | 20 +++++++++++
> >>  drivers/s390/cio/vfio_ccw_ops.c     |  4 +++
> >>  drivers/s390/cio/vfio_ccw_private.h |  3 ++
> >>  include/uapi/linux/vfio.h           |  1 +
> >>  include/uapi/linux/vfio_ccw.h       |  9 +++++
> >>  7 files changed, 108 insertions(+)
> >>  
> > 
> > (...)
> >   
> >> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> >> index 826d08379fe3..8fde94552149 100644
> >> --- a/drivers/s390/cio/vfio_ccw_chp.c
> >> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> >> @@ -73,3 +73,59 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
> >>  					    VFIO_REGION_INFO_FLAG_READ,
> >>  					    private->schib_region);
> >>  }
> >> +
> >> +static ssize_t vfio_ccw_crw_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_crw_region *region;
> >> +	int ret;
> >> +
> >> +	if (pos + count > sizeof(*region))
> >> +		return -EINVAL;
> >> +
> >> +	if (list_empty(&private->crw))
> >> +		return 0;  
> 
> This is actually nonsense, because the list isn't introduced for a
> couple of patches.
> 
> >> +
> >> +	mutex_lock(&private->io_mutex);
> >> +	region = private->region[i].data;
> >> +
> >> +	if (copy_to_user(buf, (void *)region + pos, count))
> >> +		ret = -EFAULT;
> >> +	else
> >> +		ret = count;
> >> +
> >> +	mutex_unlock(&private->io_mutex);
> >> +	return ret;
> >> +}  
> > 
> > Would it make sense to clear out the crw after it has been read by
> > userspace?  
> 
> Yes, I fixed this up in my in-progress v3 last week.  Sorry to waste
> some time, I should have mentioned it when I changed my branch locally.
> 
> I changed the list_empty() check above to bail out if the region is
> already zero, which I guess is why the userspace check looks for both.
> But if we only look at the contents of the region, then checking both is
> unnecessary.  Just do the copy and be done with it.

Yes, that makes sense.

> 
> > 
> > In patch 7, you add a notification for a new crw via eventfd, but
> > nothing is preventing userspace from reading this even if not
> > triggered. I also don't see the region being updated there until a new
> > crw is posted.
> > 
> > Or am I missing something?
> >   
>