s390x/vfio-ccw: Channel Path Handling [KVM]

From: Farhan Ali <[email protected]>

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

Signed-off-by Farhan Ali <[email protected]>

Signed-off-by Eric Farman <[email protected]>

---

Notes:
v2->v3:
- Add a call to cio_cancel_halt_clear() for CHP_VARY_OFF [CH]

v1->v2:
- Move s390dbf before cio_update_schib() call [CH]

v0->v1: [EF]
- Add s390dbf trace

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

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

index 8715c1c2f1e1..e48967c475e7 100644
@@ -19,6 +19,7 @@
 
 #include <asm/isc.h>
 
+#include "chp.h"
 #include "ioasm.h"
 #include "css.h"
 #include "vfio_ccw_private.h"

@@ -262,6 +263,49 @@ 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;
+

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

+		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 */ },

@@ -279,6 +323,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)

Quoted 56 lines · On Fri, 17 Apr 2020 04:29:55 +0200 Eric Farman <[email protected]> wrote:

From: Farhan Ali <[email protected]>

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

Signed-off-by Farhan Ali <[email protected]>

Signed-off-by Eric Farman <[email protected]>

---

Notes:
v2->v3:
- Add a call to cio_cancel_halt_clear() for CHP_VARY_OFF [CH]

v1->v2:
- Move s390dbf before cio_update_schib() call [CH]

v0->v1: [EF]
- Add s390dbf trace

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

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

index 8715c1c2f1e1..e48967c475e7 100644
@@ -19,6 +19,7 @@
 
 #include <asm/isc.h>
 
+#include "chp.h"
 #include "ioasm.h"
 #include "css.h"
 #include "vfio_ccw_private.h"

@@ -262,6 +263,49 @@ 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;
+

drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_sch_event(struct subchannel *sch, int process) · line 293

Quoted 10 lines

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

+	switch (event) {
+	case CHP_VARY_OFF:
+		/* Path logically turned off */
+		sch->opm &= ~mask;
+		sch->lpm &= ~mask;
+		cio_cancel_halt_clear(sch, &retry);
+		break;
+	case CHP_OFFLINE:
+		/* Path is gone */
+		cio_cancel_halt_clear(sch, &retry);

Looking at this again: While calling the same function for both CHP_VARY_OFF and CHP_OFFLINE is the right thing to do, cio_cancel_halt_clear() is probably not that function. I don't think we want to terminate an I/O that does not use the affected path.

Looking at what the normal I/O subchannel driver does, it first checks for the lpum and does not do anything if the affected path does not show up there. Following down the git history rabbit hole, that basic check (surviving several reworks) precedes the first git import, so there's unfortunately no patch description explaining that. Looking at the PoP, I cannot find a whole lot of details... I think some of the path-handling stuff is explained in non-public documentation, and whoever wrote the original code (probably me) relied on the information there.

tl;dr: We probably should check the lpum here as well.

Quoted 27 lines

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

+		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 */ },

@@ -279,6 +323,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)

From: Farhan Ali <[email protected]>

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 <[email protected]>

Signed-off-by Eric Farman <[email protected]>

---

Notes:
v2->v3:
- Updated Copyright year and Authors [CH]
- Mention that schib region triggers a STSCH [CH]

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     | 19 +++++++-
 drivers/s390/cio/Makefile           |  2 +-
 drivers/s390/cio/vfio_ccw_chp.c     | 76 +++++++++++++++++++++++++++++
 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, 141 insertions(+), 4 deletions(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_chp.c

drivers/s390/cio/vfio_ccw_chp.c new file +30 -0

+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:

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

+
+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,
+};

From: Farhan Ali <[email protected]>

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 <[email protected]>

Signed-off-by Eric Farman <[email protected]>

---

Notes:
v2->v3:
- Remove "if list-empty" check, since there's no list yet [EF]
- Reduce the CRW region to one fullword, instead of two [CH]

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     | 16 +++++++++
 drivers/s390/cio/vfio_ccw_chp.c     | 53 +++++++++++++++++++++++++++++
 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       |  8 +++++
 7 files changed, 105 insertions(+)

Documentation/s390/vfio-ccw.rst +5 -0

index 98832d95f395..3338551ef642 100644
@@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
 Reading this region triggers a STORE SUBCHANNEL to be issued to the
 associated hardware.
 
+vfio-ccw crw region
+---------------------
+
+The vfio-ccw crw region is used to return Channel Report Word (CRW)
+data to userspace::

Documentation/s390/vfio-ccw.rst +1 -0

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

drivers/s390/cio/vfio_ccw_chp.c +10 -0

index 18f3b3e873a9..c1362aae61f5 100644
@@ -74,3 +74,56 @@ 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;
+

drivers/s390/cio/vfio_ccw_chp.c +4 -0

+
+	mutex_unlock(&private->io_mutex);
+	return ret;
+}

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

+ * Note: this is controlled by a capability
+ */
+struct ccw_crw_region {
+	__u32 crw;
+} __packed;
+
 #endif

Quoted 2 lines · On Fri, 17 Apr 2020 04:29:58 +0200 Eric Farman <[email protected]> wrote:

From: Farhan Ali <[email protected]>

Quoted 7 lines

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

Quoted 2 lines

only occupy the first one. Even though this support will also only utilize the first one, we'll provide space for two also.

This is no longer true?

Quoted 44 lines

Signed-off-by Farhan Ali <[email protected]>

Signed-off-by Eric Farman <[email protected]>

---

Notes:
v2->v3:
- Remove "if list-empty" check, since there's no list yet [EF]
- Reduce the CRW region to one fullword, instead of two [CH]

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     | 16 +++++++++
 drivers/s390/cio/vfio_ccw_chp.c     | 53 +++++++++++++++++++++++++++++
 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       |  8 +++++
 7 files changed, 105 insertions(+)

Documentation/s390/vfio-ccw.rst +5 -0

index 98832d95f395..3338551ef642 100644
@@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
 Reading this region triggers a STORE SUBCHANNEL to be issued to the
 associated hardware.
 
+vfio-ccw crw region
+---------------------
+
+The vfio-ccw crw region is used to return Channel Report Word (CRW)
+data to userspace::

Documentation/s390/vfio-ccw.rst This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB. · line 264

Quoted 10 lines

Documentation/s390/vfio-ccw.rst +10 -0

+
+  struct ccw_crw_region {
+         __u32 crw;
+  } __packed;
+
+This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW.
+
+Currently, space is provided for a single CRW. Handling of chained
+CRWs (not implemented in vfio-ccw) can be accomplished by re-reading
+the region for additional CRW data.

What about the following instead:

"Reading this region returns a CRW if one that is relevant for this subchannel (e.g. one reporting changes in channel path state) is pending, or all zeroes if not. If multiple CRWs are pending (including possibly chained CRWs), reading this region again will return the next one, until no more CRWs are pending and zeroes are returned. This is similar to how STORE CHANNEL REPORT WORD works."

Quoted 29 lines

Documentation/s390/vfio-ccw.rst +1 -0

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

drivers/s390/cio/vfio_ccw_chp.c +16 -0

index 18f3b3e873a9..c1362aae61f5 100644
@@ -74,3 +74,56 @@ 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;
+
+	mutex_lock(&private->io_mutex);
+	region = private->region[i].data;
+

drivers/s390/cio/vfio_ccw_chp.c int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private) · line 96

Quoted 4 lines

drivers/s390/cio/vfio_ccw_chp.c +4 -0

+	if (copy_to_user(buf, (void *)region + pos, count))
+		ret = -EFAULT;
+	else
+		ret = count;

I see that you implemented it a bit differently in patch 7, but I think resetting crw to 0 immediately after the copy_to_user() is cleaner. It also can be done in this patch already :)

Quoted 5 lines

drivers/s390/cio/vfio_ccw_chp.c +4 -0

+
+	mutex_unlock(&private->io_mutex);
+	return ret;
+}

(...)

Quoted 8 lines

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

index 758bf214898d..faf57e691d4a 100644
@@ -44,4 +44,12 @@ struct ccw_schib_region {
 	__u8 schib_area[SCHIB_AREA_SIZE];
 } __packed;
 

include/uapi/linux/vfio_ccw.h struct ccw_schib_region { · line 48

Quoted 2 lines

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

+/*
+ * Used for returning Channel Report Word(s) to userspace.

s/Channel Report Word(s)/a Channel Report Word/ ?

Quoted 8 lines

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

+ * Note: this is controlled by a capability
+ */
+struct ccw_crw_region {
+	__u32 crw;
+} __packed;
+
 #endif

Quoted 29 lines · Cornelia Huck

Quoted 13 lines · On Fri, 17 Apr 2020 04:29:58 +0200 Eric Farman <[email protected]> wrote:

Documentation/s390/vfio-ccw.rst +5 -0

index 98832d95f395..3338551ef642 100644
@@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
 Reading this region triggers a STORE SUBCHANNEL to be issued to the
 associated hardware.
 
+vfio-ccw crw region
+---------------------
+
+The vfio-ccw crw region is used to return Channel Report Word (CRW)
+data to userspace::

Documentation/s390/vfio-ccw.rst This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB. · line 264

Quoted 10 lines

Documentation/s390/vfio-ccw.rst +10 -0

+
+  struct ccw_crw_region {
+         __u32 crw;
+  } __packed;
+
+This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW.
+
+Currently, space is provided for a single CRW. Handling of chained
+CRWs (not implemented in vfio-ccw) can be accomplished by re-reading
+the region for additional CRW data.  

What about the following instead:

Quoted 6 lines

"Reading this region returns a CRW if one that is relevant for this subchannel (e.g. one reporting changes in channel path state) is pending, or all zeroes if not. If multiple CRWs are pending (including possibly chained CRWs), reading this region again will return the next one, until no more CRWs are pending and zeroes are returned. This is similar to how STORE CHANNEL REPORT WORD works."

Sounds good to me.

Hrm... Maybe coffee hasn't hit yet. Should I wire STCRW into this, or just rely on the notification from the host to trigger the read?

Quoted 37 lines · On Tue, 21 Apr 2020 07:02:03 -0400 Eric Farman <[email protected]> wrote:

Quoted 35 lines · Cornelia Huck

Quoted 13 lines · On Fri, 17 Apr 2020 04:29:58 +0200 Eric Farman <[email protected]> wrote:

Documentation/s390/vfio-ccw.rst +5 -0

index 98832d95f395..3338551ef642 100644
@@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
 Reading this region triggers a STORE SUBCHANNEL to be issued to the
 associated hardware.
 
+vfio-ccw crw region
+---------------------
+
+The vfio-ccw crw region is used to return Channel Report Word (CRW)
+data to userspace::

Documentation/s390/vfio-ccw.rst This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB. · line 264

Quoted 10 lines

Documentation/s390/vfio-ccw.rst +10 -0

+
+  struct ccw_crw_region {
+         __u32 crw;
+  } __packed;
+
+This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW.
+
+Currently, space is provided for a single CRW. Handling of chained
+CRWs (not implemented in vfio-ccw) can be accomplished by re-reading
+the region for additional CRW data.  

What about the following instead:

"Reading this region returns a CRW if one that is relevant for this subchannel (e.g. one reporting changes in channel path state) is pending, or all zeroes if not. If multiple CRWs are pending (including possibly chained CRWs), reading this region again will return the next one, until no more CRWs are pending and zeroes are returned. This is similar to how STORE CHANNEL REPORT WORD works."

Quoted 4 lines

Sounds good to me.

Hrm... Maybe coffee hasn't hit yet. Should I wire STCRW into this, or just rely on the notification from the host to trigger the read?

Userspace is supposed to use this to get crws to inject into the guest, no stcrw involved until the guest actually got the machine check for it.

From: Farhan Ali <[email protected]>

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 <[email protected]>

Signed-off-by Eric Farman <[email protected]>

---

Notes:
v2->v3:
- Refactor vfio_ccw_alloc_crw() to accept rsc, erc, and rsid fields of a CRW as input [CH]
- Copy the right amount of CRWs to the crw_region [EF]
- Use sizeof(target) for the memcpy, rather than sizeof(source) [EF]
- Ensure the CRW region is empty if no CRW is present [EF/CH]
- Refactor how data goes from private-to-region-to-user [CH]
- Reduce the number of CRWs from two to one [CH]
- s/vc_crw/crw/ [EF]

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     | 19 +++++++++++
 drivers/s390/cio/vfio_ccw_drv.c     | 49 +++++++++++++++++++++++++++++
 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, 82 insertions(+)

+static void vfio_ccw_alloc_crw(struct vfio_ccw_private *private,

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

+			       unsigned int rsc,
+			       unsigned int erc,
+			       unsigned int rsid)
+{

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

+
+	list_add_tail(&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)
 {

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

 		break;
 	case CHP_VARY_ON:
 		/* Path logically turned on */

@@ -318,6 +365,8 @@ 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, CRW_RSC_CPATH, CRW_ERC_INIT,
+				   (link->chpid.cssid << 8) | link->chpid.id);
 		break;
 	}
 

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

Signed-off-by Eric Farman <[email protected]>

---
 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(+)

Here is a new pass at the channel-path handling code for vfio-ccw. Changes from previous versions are recorded in git notes for each patch.

I dropped the "Remove inline get_schid()" patch from this version. When I made the change suggested in v2, it seemed rather frivolous and better to just drop it for the time being.

I suspect that patches 5 and 7 would be better squashed together, but I have not done that here. For future versions, I guess.

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.

Quoted 3 lines · On Fri, 17 Apr 2020 04:29:53 +0200 Eric Farman <[email protected]> wrote:

Here is a new pass at the channel-path handling code for vfio-ccw. Changes from previous versions are recorded in git notes for each patch.

Quoted 6 lines

I dropped the "Remove inline get_schid()" patch from this version. When I made the change suggested in v2, it seemed rather frivolous and better to just drop it for the time being.

I suspect that patches 5 and 7 would be better squashed together, but I have not done that here. For future versions, I guess.

The result also might get a bit large.

Quoted 6 lines

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.

Basically, this looks good to me (modulo my comments).

One thing though that keeps coming up: do we need any kind of serialization? Can there be any confusion from concurrent reads from userspace, or are we sure that we always provide consistent data?

Quoted 6 lines · Cornelia Huck

Quoted 3 lines · On Fri, 17 Apr 2020 04:29:53 +0200 Eric Farman <[email protected]> wrote:

Here is a new pass at the channel-path handling code for vfio-ccw. Changes from previous versions are recorded in git notes for each patch.

Quoted 8 lines

Quoted 6 lines

I dropped the "Remove inline get_schid()" patch from this version. When I made the change suggested in v2, it seemed rather frivolous and better to just drop it for the time being.

I suspect that patches 5 and 7 would be better squashed together, but I have not done that here. For future versions, I guess.

The result also might get a bit large.

True.

Not that someone would pick patch 5 and not 7, but vfio-ccw is broken between them, because of a mismatch in IRQs. An example from hotplug:

error: internal error: unable to execute QEMU command 'device_add':
vfio: unexpected number of irqs 1

Maybe I just pull the CRW_IRQ definition into 5, and leave the wiring of the CRW stuff in 7. That seems to leave a better behavior.

Quoted 9 lines

Quoted 7 lines

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.

Basically, this looks good to me (modulo my comments).

Woo! Thanks for the feedback; I'm going to try to get them all addressed in the next couple of days.

Quoted 5 lines

One thing though that keeps coming up: do we need any kind of serialization? Can there be any confusion from concurrent reads from userspace, or are we sure that we always provide consistent data?

I'm feeling better with the rearrangement in this version of how we get data from the queue of CRWs into the region and off to the guest. The weirdness I described a few months ago seems to have been triggered by one of the patches that's now been dropped. But I'll walk through this code again once I get your latest comments applied.