Skip to contentSkip to conversation

s390x/vfio-ccw: Channel Path Handling

RFC PATCH Superseded Eric Farman 2 participants 31 messages Feb 6, 2020 – Apr 7, 2020

Subsystems (6)
Documentation KVM s390 VFIO driver +2 more subsystems

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Eric Farman

author maintainer
#0 lore raw

v2 cover s390x/vfio-ccw: Channel Path Handling

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

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
Signature · 1 line
2.17.1
author maintainer
#1 lore raw

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

From: Farhan Ali <[email protected]>

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

---

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(-)
drivers/s390/cio/vfio_ccw_drv.c +18 -10
index e401a3d0aa57..91989269faf1 100644
@@ -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();
 }
Signature · 1 line
2.17.1
author maintainer
#2 lore raw

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

In reply to Eric Farman

From: Farhan Ali <[email protected]>

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

---

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(+)
drivers/s390/cio/vfio_ccw_drv.c +44 -0
index 91989269faf1..a99705e2fd73 100644
@@ -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)
Signature · 1 line
2.17.1
#3 lore raw

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

Quoted 4 lines · On Thu, 6 Feb 2020 22:38:18 +0100 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.

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

Quoted 15 lines

---

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

(...)

Quoted 21 lines
drivers/s390/cio/vfio_ccw_drv.c +17 -0
@@ -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;

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

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;
+		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?

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

Quoted 9 lines
drivers/s390/cio/vfio_ccw_drv.c +9 -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;

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? :)

Quoted 9 lines
drivers/s390/cio/vfio_ccw_drv.c +6 -0
+		break;
+	}
+
+	return 0;
+}
+
 static struct css_device_id vfio_ccw_sch_ids[] = {
 	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
 	{ /* end of list */ },

(...)

author maintainer
#4 lore raw

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

Quoted 9 lines · Cornelia Huck
Quoted 4 lines · On Thu, 6 Feb 2020 22:38:18 +0100 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.

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.

Quoted 42 lines
Quoted 16 lines

---

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

(...)

Quoted 25 lines
drivers/s390/cio/vfio_ccw_drv.c +21 -0
@@ -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 */
Quoted 8 lines

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

Quoted 6 lines
drivers/s390/cio/vfio_ccw_drv.c +6 -0
+		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.

Quoted 13 lines

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

Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +9 -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;

If I'm not mistaken, this patch introduces the first usage of sch->opm in the vfio-ccw code.

Correct.

Quoted 1 line

Are we missing something?

Maybe? :)

Quoted 3 lines

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?

Quoted 11 lines
Quoted 9 lines
drivers/s390/cio/vfio_ccw_drv.c +6 -0
+		break;
+	}
+
+	return 0;
+}
+
 static struct css_device_id vfio_ccw_sch_ids[] = {
 	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
 	{ /* end of list */ },

(...)

#5 lore raw

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

Quoted 61 lines · On Fri, 14 Feb 2020 11:35:21 -0500 Eric Farman <[email protected]> wrote:
Quoted 29 lines · Cornelia Huck

On Thu, 6 Feb 2020 22:38:18 +0100
Eric Farman <[email protected]> wrote:

(...)

Quoted 25 lines
drivers/s390/cio/vfio_ccw_drv.c +21 -0
@@ -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 */
Quoted 8 lines

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

Quoted 6 lines
drivers/s390/cio/vfio_ccw_drv.c +6 -0
+		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.

Quoted 13 lines

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

Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +9 -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;  

If I'm not mistaken, this patch introduces the first usage of sch->opm in the vfio-ccw code.

Correct.

Quoted 1 line

Are we missing something?

Maybe? :)

Quoted 7 lines
Quoted 3 lines

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.)

Quoted 13 lines
Quoted 12 lines
Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +6 -0
+		break;
+	}
+
+	return 0;
+}
+
 static struct css_device_id vfio_ccw_sch_ids[] = {
 	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
 	{ /* end of list */ },  

(...)

author maintainer
#6 lore raw

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

Quoted 68 lines · Cornelia Huck
Quoted 65 lines · On Fri, 14 Feb 2020 11:35:21 -0500 Eric Farman <[email protected]> wrote:
Quoted 29 lines · Cornelia Huck

On Thu, 6 Feb 2020 22:38:18 +0100
Eric Farman <[email protected]> wrote:

(...)

Quoted 25 lines
drivers/s390/cio/vfio_ccw_drv.c +21 -0
@@ -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 */
Quoted 8 lines

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

Quoted 6 lines
drivers/s390/cio/vfio_ccw_drv.c +6 -0
+		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.

Quoted 13 lines

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

Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +9 -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;  

If I'm not mistaken, this patch introduces the first usage of sch->opm in the vfio-ccw code.

Correct.

Quoted 1 line

Are we missing something?

Maybe? :)

Quoted 3 lines

Or am I missing
something? :)

Quoted 8 lines
Quoted 3 lines

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.)

Quoted 15 lines
Quoted 14 lines
Quoted 13 lines
Quoted 11 lines
drivers/s390/cio/vfio_ccw_drv.c +6 -0
+		break;
+	}
+
+	return 0;
+}
+
 static struct css_device_id vfio_ccw_sch_ids[] = {
 	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
 	{ /* end of list */ },  

(...)

#7 lore raw

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

Quoted 34 lines · On Wed, 25 Mar 2020 22:09:40 -0400 Eric Farman <[email protected]> wrote:
Quoted 24 lines · Cornelia Huck
Quoted 21 lines · On Fri, 14 Feb 2020 11:35:21 -0500 Eric Farman <[email protected]> wrote:
Quoted 9 lines · Cornelia Huck

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

Quoted 3 lines · On Thu, 6 Feb 2020 22:38:18 +0100 Eric Farman <[email protected]> wrote:
drivers/s390/cio/vfio_ccw_drv.c +3 -0
+	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.

Quoted 1 line

Are we missing something?

Maybe? :)

Quoted 3 lines

Or am I missing
something? :)

Quoted 8 lines
Quoted 3 lines

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.)

Quoted 4 lines

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.

author maintainer
#8 lore raw

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

Quoted 37 lines · Cornelia Huck
Quoted 34 lines · On Wed, 25 Mar 2020 22:09:40 -0400 Eric Farman <[email protected]> wrote:
Quoted 32 lines · Cornelia Huck
Quoted 17 lines · On Fri, 14 Feb 2020 11:35:21 -0500 Eric Farman <[email protected]> wrote:
Quoted 9 lines · Cornelia Huck

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

Quoted 3 lines · On Thu, 6 Feb 2020 22:38:18 +0100 Eric Farman <[email protected]> wrote:
drivers/s390/cio/vfio_ccw_drv.c +3 -0
+	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.

Quoted 1 line

Are we missing something?

Maybe? :)

Quoted 7 lines
Quoted 3 lines

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.)

Quoted 9 lines
Quoted 4 lines

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.

author maintainer
#9 lore raw

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

In reply to 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.

---

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(-)
drivers/s390/cio/vfio_ccw_ops.c +12 -8
index f0d71ab77c50..d4fc84b8867f 100644
@@ -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)
drivers/s390/cio/vfio_ccw_private.h +1 -0
index 9b9bb4982972..ce3834159d98 100644
@@ -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);
 
Signature · 1 line
2.17.1
author maintainer
#10 lore raw

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

In reply to Eric Farman

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.

---

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
Documentation/s390/vfio-ccw.rst +15 -1
index fca9c4f5bd9c..b805dc995fc8 100644
@@ -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.
drivers/s390/cio/Makefile +1 -1
index 23eae4188876..a9235f111e79 100644
@@ -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
drivers/s390/cio/vfio_ccw_chp.c new file +75 -0

75 lines not on this page · raw message

drivers/s390/cio/vfio_ccw_drv.c +20 -0
index a99705e2fd73..3023a366ad47 100644
@@ -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) {
drivers/s390/cio/vfio_ccw_ops.c +12 -2
index d4fc84b8867f..22988d67b6bb 100644
@@ -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;
 }
 
drivers/s390/cio/vfio_ccw_private.h +3 -0
index ce3834159d98..d6601a8adf13 100644
@@ -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;
include/uapi/linux/vfio.h +1 -0
index 9e843a147ead..b108e2795ea3 100644
@@ -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
include/uapi/linux/vfio_ccw.h +10 -0
index cbecbf0cd54f..758bf214898d 100644
@@ -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
Signature · 1 line
2.17.1
#11 lore raw

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

Quoted 46 lines · On Thu, 6 Feb 2020 22:38:20 +0100 Eric Farman <[email protected]> wrote:

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.

---

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
Documentation/s390/vfio-ccw.rst +2 -0
index fca9c4f5bd9c..b805dc995fc8 100644
@@ -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
+---------------------

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

Quoted 10 lines
Documentation/s390/vfio-ccw.rst +10 -0
+
+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?

Quoted 4 lines
Documentation/s390/vfio-ccw.rst +1 -0
+
 vfio-ccw operation details
 --------------------------
 

(...)

drivers/s390/cio/vfio_ccw_chp.c line 5

Quoted 11 lines
drivers/s390/cio/vfio_ccw_chp.c new file +5 -0
new file mode 100644
index 000000000000..826d08379fe3
@@ -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?

Quoted 3 lines
drivers/s390/cio/vfio_ccw_chp.c new file +3 -0
+ *
+ * Author(s): Farhan Ali <[email protected]>
+ */

(...)

author maintainer
#12 lore raw

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

Quoted 47 lines · Cornelia Huck
Quoted 44 lines · On Thu, 6 Feb 2020 22:38:20 +0100 Eric Farman <[email protected]> wrote:

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.

---

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
Documentation/s390/vfio-ccw.rst +0 -0
index fca9c4f5bd9c..b805dc995fc8 100644
@@ -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.
 
Quoted 14 lines

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

Quoted 12 lines
Documentation/s390/vfio-ccw.rst +12 -0
+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.

Quoted 8 lines
Quoted 5 lines
Documentation/s390/vfio-ccw.rst +1 -0
+
 vfio-ccw operation details
 --------------------------
 

(...)

Quoted 13 lines

drivers/s390/cio/vfio_ccw_chp.c line 5

Quoted 11 lines
drivers/s390/cio/vfio_ccw_chp.c new file +5 -0
new file mode 100644
index 000000000000..826d08379fe3
@@ -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. :)

Quoted 7 lines
Quoted 4 lines
drivers/s390/cio/vfio_ccw_chp.c new file +3 -0
+ *
+ * Author(s): Farhan Ali <[email protected]>
+ */

(...)

author maintainer
#13 lore raw

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

In reply to Eric Farman

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.

---

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(+)
Documentation/s390/vfio-ccw.rst +15 -0
index b805dc995fc8..b8e4b10e9988 100644
@@ -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
 --------------------------
 
drivers/s390/cio/vfio_ccw_chp.c +56 -0

59 lines not on this page · raw message

drivers/s390/cio/vfio_ccw_drv.c +20 -0
index 3023a366ad47..1e1360af1b34 100644
@@ -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) {
drivers/s390/cio/vfio_ccw_ops.c +4 -0
index 22988d67b6bb..edec0fb7ace8 100644
@@ -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:
drivers/s390/cio/vfio_ccw_private.h +3 -0
index d6601a8adf13..8289b6850e59 100644
@@ -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;
include/uapi/linux/vfio.h +1 -0
index b108e2795ea3..5024636d7615 100644
@@ -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
include/uapi/linux/vfio_ccw.h +9 -0
index 758bf214898d..3a98bc4c80b6 100644
@@ -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
Signature · 1 line
2.17.1
#14 lore raw

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

Quoted 37 lines · On Thu, 6 Feb 2020 22:38:21 +0100 Eric Farman <[email protected]> wrote:

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.

---

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

(...)

Quoted 25 lines
drivers/s390/cio/vfio_ccw_chp.c +17 -0
index 826d08379fe3..8fde94552149 100644
@@ -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);

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

Quoted 10 lines
drivers/s390/cio/vfio_ccw_chp.c +10 -0
+	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?

author maintainer
#15 lore raw

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

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

Quoted 58 lines · Cornelia Huck
Quoted 37 lines · On Thu, 6 Feb 2020 22:38:21 +0100 Eric Farman <[email protected]> wrote:

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.

---

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

(...)

Quoted 15 lines
drivers/s390/cio/vfio_ccw_chp.c +7 -0
index 826d08379fe3..8fde94552149 100644
@@ -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;
Quoted 8 lines

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

Quoted 8 lines
drivers/s390/cio/vfio_ccw_chp.c +8 -0
+	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.

Quoted 7 lines
Quoted 7 lines
drivers/s390/cio/vfio_ccw_chp.c +7 -0
+
+	mutex_lock(&private->io_mutex);
+	region = private->region[i].data;
+
+	if (copy_to_user(buf, (void *)region + pos, count))
+		ret = -EFAULT;
+	else
Quoted 8 lines

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

Quoted 5 lines
drivers/s390/cio/vfio_ccw_chp.c +5 -0
+		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.

Quoted 8 lines

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?

#16 lore raw

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

Quoted 89 lines · On Mon, 6 Apr 2020 17:43:42 -0400 Eric Farman <[email protected]> wrote:

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

Quoted 58 lines · Cornelia Huck
Quoted 37 lines · On Thu, 6 Feb 2020 22:38:21 +0100 Eric Farman <[email protected]> wrote:

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.

---

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

(...)

Quoted 15 lines
drivers/s390/cio/vfio_ccw_chp.c +7 -0
index 826d08379fe3..8fde94552149 100644
@@ -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;
Quoted 8 lines

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

Quoted 8 lines
drivers/s390/cio/vfio_ccw_chp.c +8 -0
+	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.

Quoted 7 lines
Quoted 7 lines
drivers/s390/cio/vfio_ccw_chp.c +7 -0
+
+	mutex_lock(&private->io_mutex);
+	region = private->region[i].data;
+
+	if (copy_to_user(buf, (void *)region + pos, count))
+		ret = -EFAULT;
+	else
Quoted 8 lines

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

Quoted 5 lines
drivers/s390/cio/vfio_ccw_chp.c +5 -0
+		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?

Quoted 7 lines

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.

Quoted 10 lines
Quoted 9 lines

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?

author maintainer
#17 lore raw

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

In reply to Eric Farman

To simplify future expansion.

---

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

 drivers/s390/cio/vfio_ccw_ops.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)
drivers/s390/cio/vfio_ccw_ops.c +17 -6
index edec0fb7ace8..f3033f8fc96d 100644
@@ -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);
Signature · 1 line
2.17.1
author maintainer
#18 lore raw

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

In reply to Eric Farman

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.

---

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(+)
drivers/s390/cio/vfio_ccw_chp.c +5 -0
index 8fde94552149..328b4e1d1972 100644
@@ -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;
 }
 
drivers/s390/cio/vfio_ccw_drv.c +73 -0

113 lines not on this page · raw message

drivers/s390/cio/vfio_ccw_ops.c +4 -0
index f3033f8fc96d..8b3ed5b45277 100644
@@ -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;
 	}
drivers/s390/cio/vfio_ccw_private.h +9 -0
index 8289b6850e59..a701f09c6943 100644
@@ -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);
include/uapi/linux/vfio.h +1 -0
index 5024636d7615..1bdf32772545 100644
@@ -579,6 +579,7 @@ enum {
 
 enum {
 	VFIO_CCW_IO_IRQ_INDEX,
+	VFIO_CCW_CRW_IRQ_INDEX,
 	VFIO_CCW_NUM_IRQS
 };
 
Signature · 1 line
2.17.1
#19 lore raw

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

Quoted 31 lines · On Thu, 6 Feb 2020 22:38:23 +0100 Eric Farman <[email protected]> wrote:

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.

---

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

(...)

Quoted 15 lines
drivers/s390/cio/vfio_ccw_drv.c +15 -0
+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;

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

Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +10 -0
+
+	/*
+	 * 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...

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

Quoted 4 lines
drivers/s390/cio/vfio_ccw_drv.c +4 -0
+	 *
+	 * 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.

Quoted 12 lines
drivers/s390/cio/vfio_ccw_drv.c +9 -0
+	 */
+	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)
 {

(...)

author maintainer
#20 lore raw

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

Quoted 55 lines · Cornelia Huck
Quoted 31 lines · On Thu, 6 Feb 2020 22:38:23 +0100 Eric Farman <[email protected]> wrote:

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.

---

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

(...)

Quoted 20 lines
drivers/s390/cio/vfio_ccw_drv.c +20 -0
+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.
+	 */
Quoted 8 lines

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

Quoted 5 lines
drivers/s390/cio/vfio_ccw_drv.c +5 -0
+	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.

Quoted 8 lines

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

Quoted 5 lines
drivers/s390/cio/vfio_ccw_drv.c +4 -0
+	 *
+	 * 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 :)

Quoted 4 lines

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.

Quoted 15 lines
Quoted 13 lines
drivers/s390/cio/vfio_ccw_drv.c +9 -0
+	 */
+	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)
 {

(...)

#21 lore raw

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

Quoted 34 lines · On Fri, 14 Feb 2020 11:24:39 -0500 Eric Farman <[email protected]> wrote:
Quoted 24 lines · Cornelia Huck

On Thu, 6 Feb 2020 22:38:23 +0100
Eric Farman <[email protected]> wrote:

(...)

Quoted 20 lines
drivers/s390/cio/vfio_ccw_drv.c +20 -0
+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.
+	 */
Quoted 8 lines

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

Quoted 5 lines
drivers/s390/cio/vfio_ccw_drv.c +5 -0
+	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...

Quoted 4 lines

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.

author maintainer
#22 lore raw

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

Quoted 37 lines · Cornelia Huck
Quoted 34 lines · On Fri, 14 Feb 2020 11:24:39 -0500 Eric Farman <[email protected]> wrote:
Quoted 32 lines · Cornelia Huck

On Thu, 6 Feb 2020 22:38:23 +0100
Eric Farman <[email protected]> wrote:

(...)

Quoted 15 lines
drivers/s390/cio/vfio_ccw_drv.c +15 -0
+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;

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

Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +10 -0
+
+	/*
+	 * 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...

Quoted 8 lines
Quoted 4 lines

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.

#23 lore raw

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

In reply to Eric Farman
Quoted 24 lines · On Thu, 6 Feb 2020 22:38:23 +0100 Eric Farman <[email protected]> wrote:

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.

---

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

Quoted 6 lines
 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...]

drivers/s390/cio/vfio_ccw_chp.c static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private, · line 104

Quoted 12 lines
drivers/s390/cio/vfio_ccw_chp.c +4 -0
index 8fde94552149..328b4e1d1972 100644
@@ -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.

Quoted 26 lines
drivers/s390/cio/vfio_ccw_chp.c +1 -0
+
 	return ret;
 }
 
drivers/s390/cio/vfio_ccw_drv.c +14 -0
index 1e1360af1b34..c48c260a129d 100644
@@ -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);
+

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

Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +10 -0
+		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.

Quoted 4 lines
drivers/s390/cio/vfio_ccw_drv.c +1 -0
+
 /*
  * Css driver callbacks
  */

(...)

Quoted 30 lines
drivers/s390/cio/vfio_ccw_drv.c +26 -0
@@ -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.
+	 *

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

Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +10 -0
+	 * 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.

Quoted 14 lines
drivers/s390/cio/vfio_ccw_drv.c +3 -0
+}
+
 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 */

drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_chp_event(struct subchannel *sch, · line 390

Quoted 7 lines
drivers/s390/cio/vfio_ccw_drv.c +1 -0
@@ -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?

author maintainer
#24 lore raw

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

Quoted 37 lines · Cornelia Huck
Quoted 24 lines · On Thu, 6 Feb 2020 22:38:23 +0100 Eric Farman <[email protected]> wrote:

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.

---

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

Quoted 6 lines
 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...

Quoted 4 lines

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.)

author maintainer
#25 lore raw

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

In reply to Eric Farman

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

---
 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(+)
drivers/s390/cio/vfio_ccw_drv.c +1 -0
index c48c260a129d..21952258d71a 100644
@@ -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,
drivers/s390/cio/vfio_ccw_trace.c +1 -0
index 8c671d2519f6..4a0205905afc 100644
@@ -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);
drivers/s390/cio/vfio_ccw_trace.h +30 -0
index 30162a318a8a..8c82d0c27e81 100644
@@ -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,
Signature · 1 line
2.17.1
author maintainer
#26 lore raw

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

In reply to 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.

---
 drivers/s390/cio/vfio_ccw_fsm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/s390/cio/vfio_ccw_fsm.c +2 -6
index 23e61aa638e4..c4c303645d7d 100644
@@ -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);
 }
Signature · 1 line
2.17.1
#27 lore raw

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

Quoted 4 lines · On Thu, 6 Feb 2020 22:38:25 +0100 Eric Farman <[email protected]> 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.

Quoted 5 lines
---
 drivers/s390/cio/vfio_ccw_fsm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
author maintainer
#28 lore raw

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

Quoted 10 lines · Cornelia Huck
Quoted 4 lines · On Thu, 6 Feb 2020 22:38:25 +0100 Eric Farman <[email protected]> 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.

Quoted 7 lines
Quoted 6 lines
---
 drivers/s390/cio/vfio_ccw_fsm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
#29 lore raw
In reply to Eric Farman
Quoted 13 lines · On Thu, 6 Feb 2020 22:38:16 +0100 Eric Farman <[email protected]> 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.

Quoted 8 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.

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.

Quoted 30 lines

v1: https://lore.kernel.org/kvm/[email protected]/

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
author maintainer
#30 lore raw
Quoted 21 lines · Cornelia Huck
Quoted 18 lines · On Thu, 6 Feb 2020 22:38:16 +0100 Eric Farman <[email protected]> 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.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 32 lines
Quoted 31 lines

v1: https://lore.kernel.org/kvm/[email protected]/

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