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

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

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

---
 drivers/s390/cio/vfio_ccw_cp.c | 3 +++
 drivers/s390/cio/vfio_ccw_cp.h | 2 ++
 2 files changed, 5 insertions(+)

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

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

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

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

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

Quoted 51 lines · On Mon, 21 Jan 2019 12:03:50 +0100 Cornelia Huck <[email protected]> wrote:

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

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

---
 drivers/s390/cio/vfio_ccw_cp.c | 3 +++
 drivers/s390/cio/vfio_ccw_cp.h | 2 ++
 2 files changed, 5 insertions(+)

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

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

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

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

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

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

drivers/s390/cio/vfio_ccw_cp.h struct channel_program { · line 37

Quoted 1 line

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

 extern int cp_init(struct channel_program *cp, struct device *mdev,  

I fail to see how is this patch delivering on the promise made by it's title. I see only code that sets cp->initialized but none that reads it. The follow-up patches don't seem to care for cp->initialized either.

Rework handling of multiple I/O requests to return -EAGAIN if we are already processing an I/O request. Introduce a mutex to disallow concurrent writes to the I/O region.

The expectation is that userspace simply retries the operation if it gets -EAGAIN.

We currently don't allow multiple ssch requests at the same time, as we don't have support for keeping channel programs around for more than one request.

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

---

static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,

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

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
 {
 	struct vfio_ccw_private *private;
 	struct ccw_io_region *region;
+	int ret;
 
 	if (*ppos + count > sizeof(*region))
 		return -EINVAL;
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
-	if (private->state != VFIO_CCW_STATE_IDLE)

drivers/s390/cio/vfio_ccw_ops.c +7 -5

+		ret = -EFAULT;
+		goto out_unlock;
+	}
 
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ);
-	if (region->ret_code != 0) {
-		private->state = VFIO_CCW_STATE_IDLE;
-		return region->ret_code;
-	}
+	ret = (region->ret_code != 0) ? region->ret_code : count;
 
-	return count;
+out_unlock:
+	mutex_unlock(&private->io_mutex);
+	return ret;
 }
 

Quoted 13 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

Rework handling of multiple I/O requests to return -EAGAIN if we are already processing an I/O request. Introduce a mutex to disallow concurrent writes to the I/O region.

The expectation is that userspace simply retries the operation if it gets -EAGAIN.

We currently don't allow multiple ssch requests at the same time, as we don't have support for keeping channel programs around for more than one request.

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

---

[..]

Quoted 18 lines

static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,

drivers/s390/cio/vfio_ccw_ops.c +5 -1

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
 {
 	struct vfio_ccw_private *private;
 	struct ccw_io_region *region;
+	int ret;
 
 	if (*ppos + count > sizeof(*region))
 		return -EINVAL;
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
-	if (private->state != VFIO_CCW_STATE_IDLE)
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
 		return -EACCES;
+	if (!mutex_trylock(&private->io_mutex))
+		return -EAGAIN;
 

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, · line 208

Quoted 4 lines

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

 	region = private->io_region;
-	if (copy_from_user((void *)region + *ppos, buf, count))
-		return -EFAULT;
+	if (copy_from_user((void *)region + *ppos, buf, count)) {  

This might race with vfio_ccw_sch_io_todo() on
private->io_region->irb_area, or?

Quoted 18 lines

drivers/s390/cio/vfio_ccw_ops.c +7 -5

+		ret = -EFAULT;
+		goto out_unlock;
+	}
 
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ);
-	if (region->ret_code != 0) {
-		private->state = VFIO_CCW_STATE_IDLE;
-		return region->ret_code;
-	}
+	ret = (region->ret_code != 0) ? region->ret_code : count;
 
-	return count;
+out_unlock:
+	mutex_unlock(&private->io_mutex);
+	return ret;
 }
 

[..]

Quoted 37 lines · On Mon, 21 Jan 2019 21:20:18 +0100 Halil Pasic <[email protected]> wrote:

Quoted 13 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

Rework handling of multiple I/O requests to return -EAGAIN if we are already processing an I/O request. Introduce a mutex to disallow concurrent writes to the I/O region.

The expectation is that userspace simply retries the operation if it gets -EAGAIN.

We currently don't allow multiple ssch requests at the same time, as we don't have support for keeping channel programs around for more than one request.

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

---

[..]

Quoted 18 lines

static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,

drivers/s390/cio/vfio_ccw_ops.c +5 -1

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
 {
 	struct vfio_ccw_private *private;
 	struct ccw_io_region *region;
+	int ret;
 
 	if (*ppos + count > sizeof(*region))
 		return -EINVAL;
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
-	if (private->state != VFIO_CCW_STATE_IDLE)
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
 		return -EACCES;
+	if (!mutex_trylock(&private->io_mutex))
+		return -EAGAIN;
 

Quoted 7 lines

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, · line 208

Quoted 4 lines

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

 	region = private->io_region;
-	if (copy_from_user((void *)region + *ppos, buf, count))
-		return -EFAULT;
+	if (copy_from_user((void *)region + *ppos, buf, count)) {  

This might race with vfio_ccw_sch_io_todo() on
private->io_region->irb_area, or?

Ah yes, this should also take the mutex (should work because we're on a workqueue).

Quoted 48 lines · On Tue, 22 Jan 2019 11:29:26 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 37 lines · On Mon, 21 Jan 2019 21:20:18 +0100 Halil Pasic <[email protected]> wrote:

Quoted 13 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

Rework handling of multiple I/O requests to return -EAGAIN if we are already processing an I/O request. Introduce a mutex to disallow concurrent writes to the I/O region.

The expectation is that userspace simply retries the operation if it gets -EAGAIN.

We currently don't allow multiple ssch requests at the same time, as we don't have support for keeping channel programs around for more than one request.

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

---

[..]

Quoted 18 lines

static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,

drivers/s390/cio/vfio_ccw_ops.c +5 -1

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
 {
 	struct vfio_ccw_private *private;
 	struct ccw_io_region *region;
+	int ret;
 
 	if (*ppos + count > sizeof(*region))
 		return -EINVAL;
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
-	if (private->state != VFIO_CCW_STATE_IDLE)
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
 		return -EACCES;
+	if (!mutex_trylock(&private->io_mutex))
+		return -EAGAIN;
 

Quoted 7 lines

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, · line 208

Quoted 4 lines

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

 	region = private->io_region;
-	if (copy_from_user((void *)region + *ppos, buf, count))
-		return -EFAULT;
+	if (copy_from_user((void *)region + *ppos, buf, count)) {    

This might race with vfio_ccw_sch_io_todo() on
private->io_region->irb_area, or?

Quoted 3 lines

Ah yes, this should also take the mutex (should work because we're on a workqueue).

I'm not sure that will do the trick (assumed I understood the intention correctly). Let's say the things happen in this order:
1) vfio_ccw_sch_io_todo() goes first, I guess updates private->io_region->irb_area and releases the mutex.
2) Then vfio_ccw_mdev_write() destroys the irb_area by zeriong it out, and finally,
3) userspace reads the destroyed irb_area using vfio_ccw_mdev_read().

Or am I misunderstanding something?

Quoted 63 lines · On Tue, 22 Jan 2019 12:17:37 +0100 Halil Pasic <[email protected]> wrote:

Quoted 48 lines · On Tue, 22 Jan 2019 11:29:26 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 37 lines · On Mon, 21 Jan 2019 21:20:18 +0100 Halil Pasic <[email protected]> wrote:

Quoted 13 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

Rework handling of multiple I/O requests to return -EAGAIN if we are already processing an I/O request. Introduce a mutex to disallow concurrent writes to the I/O region.

The expectation is that userspace simply retries the operation if it gets -EAGAIN.

We currently don't allow multiple ssch requests at the same time, as we don't have support for keeping channel programs around for more than one request.

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

---

[..]

Quoted 18 lines

static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,

drivers/s390/cio/vfio_ccw_ops.c +5 -1

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
 {
 	struct vfio_ccw_private *private;
 	struct ccw_io_region *region;
+	int ret;
 
 	if (*ppos + count > sizeof(*region))
 		return -EINVAL;
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
-	if (private->state != VFIO_CCW_STATE_IDLE)
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
 		return -EACCES;
+	if (!mutex_trylock(&private->io_mutex))
+		return -EAGAIN;
 

Quoted 7 lines

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, · line 208

Quoted 4 lines

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

 	region = private->io_region;
-	if (copy_from_user((void *)region + *ppos, buf, count))
-		return -EFAULT;
+	if (copy_from_user((void *)region + *ppos, buf, count)) {    

This might race with vfio_ccw_sch_io_todo() on
private->io_region->irb_area, or?

Quoted 3 lines

Ah yes, this should also take the mutex (should work because we're on a workqueue).

I'm not sure that will do the trick (assumed I understood the intention correctly). Let's say the things happen in this order:
1) vfio_ccw_sch_io_todo() goes first, I guess updates private->io_region->irb_area and releases the mutex.
2) Then vfio_ccw_mdev_write() destroys the irb_area by zeriong it out, and finally,
3) userspace reads the destroyed irb_area using vfio_ccw_mdev_read().

Quoted 1 line

Or am I misunderstanding something?

You're not, but dealing with that race is outside the scope of this patch. If userspace submits a request and then tries to get the old data for a prior request, I suggest that userspace needs to fix their sequencing.

Quoted 66 lines · On Tue, 22 Jan 2019 12:53:22 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 63 lines · On Tue, 22 Jan 2019 12:17:37 +0100 Halil Pasic <[email protected]> wrote:

Quoted 48 lines · On Tue, 22 Jan 2019 11:29:26 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 37 lines · On Mon, 21 Jan 2019 21:20:18 +0100 Halil Pasic <[email protected]> wrote:

Quoted 13 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

Rework handling of multiple I/O requests to return -EAGAIN if we are already processing an I/O request. Introduce a mutex to disallow concurrent writes to the I/O region.

The expectation is that userspace simply retries the operation if it gets -EAGAIN.

We currently don't allow multiple ssch requests at the same time, as we don't have support for keeping channel programs around for more than one request.

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

---

[..]

Quoted 18 lines

static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,

drivers/s390/cio/vfio_ccw_ops.c +5 -1

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
 {
 	struct vfio_ccw_private *private;
 	struct ccw_io_region *region;
+	int ret;
 
 	if (*ppos + count > sizeof(*region))
 		return -EINVAL;
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
-	if (private->state != VFIO_CCW_STATE_IDLE)
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
 		return -EACCES;
+	if (!mutex_trylock(&private->io_mutex))
+		return -EAGAIN;
 

Quoted 7 lines

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, · line 208

Quoted 4 lines

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

 	region = private->io_region;
-	if (copy_from_user((void *)region + *ppos, buf, count))
-		return -EFAULT;
+	if (copy_from_user((void *)region + *ppos, buf, count)) {      

This might race with vfio_ccw_sch_io_todo() on
private->io_region->irb_area, or?

Quoted 3 lines

Ah yes, this should also take the mutex (should work because we're on a workqueue).

I'm not sure that will do the trick (assumed I understood the intention correctly). Let's say the things happen in this order:
1) vfio_ccw_sch_io_todo() goes first, I guess updates private->io_region->irb_area and releases the mutex.
2) Then vfio_ccw_mdev_write() destroys the irb_area by zeriong it out, and finally,
3) userspace reads the destroyed irb_area using vfio_ccw_mdev_read().

Quoted 7 lines

Quoted 1 line

Or am I misunderstanding something?

You're not, but dealing with that race is outside the scope of this patch. If userspace submits a request and then tries to get the old data for a prior request, I suggest that userspace needs to fix their sequencing.

I tend to disagree, because I think this would be a degradation compared to what we have right now.

Let me explain. I guess the current idea is that the private->state != VFIO_CCW_STATE_IDLE check safeguards against this. Yes we lack proper synchronization (atomic/interlocked access or locks) that would guarantee that different thread observe state transitions as required -- no splint brain. But if state were atomic the scenario I have in mind can not happen, because we get the solicited interrupt in BUSY state (and set IDLE in vfio_ccw_sch_io_todo()).

Unsolicited interrupts are another
piece of cake -- I have no idea how may of those do we get.

And because
of this the broken sequencing in userspace could actually be the kernels fault.

Quoted 80 lines · On Tue, 22 Jan 2019 13:46:12 +0100 Halil Pasic <[email protected]> wrote:

Quoted 66 lines · On Tue, 22 Jan 2019 12:53:22 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 63 lines · On Tue, 22 Jan 2019 12:17:37 +0100 Halil Pasic <[email protected]> wrote:

Quoted 48 lines · On Tue, 22 Jan 2019 11:29:26 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 37 lines · On Mon, 21 Jan 2019 21:20:18 +0100 Halil Pasic <[email protected]> wrote:

Quoted 13 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

Rework handling of multiple I/O requests to return -EAGAIN if we are already processing an I/O request. Introduce a mutex to disallow concurrent writes to the I/O region.

The expectation is that userspace simply retries the operation if it gets -EAGAIN.

We currently don't allow multiple ssch requests at the same time, as we don't have support for keeping channel programs around for more than one request.

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

---

[..]

Quoted 18 lines

static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,

(quoted diff) +5 -1

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
 {
 	struct vfio_ccw_private *private;
 	struct ccw_io_region *region;
+	int ret;
 
 	if (*ppos + count > sizeof(*region))
 		return -EINVAL;
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
-	if (private->state != VFIO_CCW_STATE_IDLE)
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
 		return -EACCES;
+	if (!mutex_trylock(&private->io_mutex))
+		return -EAGAIN;
 

Quoted 7 lines

Review static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, · line 208

Quoted 4 lines

(quoted diff) +1 -2

 	region = private->io_region;
-	if (copy_from_user((void *)region + *ppos, buf, count))
-		return -EFAULT;
+	if (copy_from_user((void *)region + *ppos, buf, count)) {      

This might race with vfio_ccw_sch_io_todo() on
private->io_region->irb_area, or?

Quoted 3 lines

Ah yes, this should also take the mutex (should work because we're on a workqueue).

I'm not sure that will do the trick (assumed I understood the intention correctly). Let's say the things happen in this order:
1) vfio_ccw_sch_io_todo() goes first, I guess updates private->io_region->irb_area and releases the mutex.
2) Then vfio_ccw_mdev_write() destroys the irb_area by zeriong it out, and finally,
3) userspace reads the destroyed irb_area using vfio_ccw_mdev_read().

Quoted 7 lines

Quoted 1 line

Or am I misunderstanding something?

You're not, but dealing with that race is outside the scope of this patch. If userspace submits a request and then tries to get the old data for a prior request, I suggest that userspace needs to fix their sequencing.

I tend to disagree, because I think this would be a degradation compared to what we have right now.

Quoted 7 lines

Let me explain. I guess the current idea is that the private->state != VFIO_CCW_STATE_IDLE check safeguards against this. Yes we lack proper synchronization (atomic/interlocked access or locks) that would guarantee that different thread observe state transitions as required -- no splint brain. But if state were atomic the scenario I have in mind can not happen, because we get the solicited interrupt in BUSY state (and set IDLE in vfio_ccw_sch_io_todo()).

This BUSY handling is broken for another case: If the guest requests intermediate interrupts, there may be more than one interrupt by the hardware -- and we still go out of BUSY state. (Freeing the cp is also broken in that case.) However, the Linux dasd driver does not seem to do that.

Quoted 2 lines

Unsolicited interrupts are another
piece of cake -- I have no idea how may of those do we get.

They at least don't have the "free the cp before we got final state" bug. But I think both are reasons to get away from "use the BUSY state to ensure the right sequence".

Quoted 3 lines

And because
of this the broken sequencing in userspace could actually be the kernels fault.

Here, I can't follow you at all :(

Quoted 10 lines · On Tue, 22 Jan 2019 18:26:17 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 2 lines · On Tue, 22 Jan 2019 13:46:12 +0100 Halil Pasic <[email protected]> wrote:

Unsolicited interrupts are another
piece of cake -- I have no idea how may of those do we get.

They at least don't have the "free the cp before we got final state" bug. But I think both are reasons to get away from "use the BUSY state to ensure the right sequence".

I'm not sure I understand you correctly. I was under the impression that the whole point in having a state machine was to ensure the states are traversed in the right sequence with the right stuff being done on each transition. At least in theory.

You've probably figured out that IMHO vfio-ccw is not in a good shape (to put it mildly). I have a hard time reviewing a non-holistic concurrency fix. Please tell sould I get perceived as non-constructive, I will try to cut back on criticism.

Quoted 7 lines

Quoted 4 lines

And because
of this the broken sequencing in userspace could actually be the kernels fault.

Here, I can't follow you at all :(

Should we ever deliver a zeroed out IRB to the userspace, for the next ioinst it would look like we have no status nor FC bit set. That is, the guest could end up with stuff in parallel that was never supposed to be in parallel (i.e. broken sequencing because kernel feeds false information due to race with unsolicited interrupt).

Does that help?

Quoted 14 lines · On Tue, 22 Jan 2019 20:03:31 +0100 Halil Pasic <[email protected]> wrote:

Quoted 10 lines · On Tue, 22 Jan 2019 18:26:17 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 2 lines · On Tue, 22 Jan 2019 13:46:12 +0100 Halil Pasic <[email protected]> wrote:

Unsolicited interrupts are another
piece of cake -- I have no idea how may of those do we get.

They at least don't have the "free the cp before we got final state" bug. But I think both are reasons to get away from "use the BUSY state to ensure the right sequence".

Quoted 4 lines

I'm not sure I understand you correctly. I was under the impression that the whole point in having a state machine was to ensure the states are traversed in the right sequence with the right stuff being done on each transition. At least in theory.

Sequence in user space programs, not in the state machine.

Quoted 5 lines

You've probably figured out that IMHO vfio-ccw is not in a good shape (to put it mildly). I have a hard time reviewing a non-holistic concurrency fix. Please tell sould I get perceived as non-constructive, I will try to cut back on criticism.

I'm afraid this is just confusing me :(

Quoted 14 lines

Quoted 7 lines

Quoted 4 lines

And because
of this the broken sequencing in userspace could actually be the kernels fault.

Here, I can't follow you at all :(

Should we ever deliver a zeroed out IRB to the userspace, for the next ioinst it would look like we have no status nor FC bit set. That is, the guest could end up with stuff in parallel that was never supposed to be in parallel (i.e. broken sequencing because kernel feeds false information due to race with unsolicited interrupt).

Quoted 1 line

Does that help?

Not at all, I'm afraid :( User space programs still need to make sure they poke the interfaces in the right order IMO...

At this point, I'm mostly confused... I'd prefer to simply fix things as they come up so that we can finally move forward with the halt/clear handling (and probably rework the state machine on top of that.)

On Wed, 23 Jan 2019 11:34:47 +0100
Cornelia Huck <[email protected]> wrote:

Yes, one can usually think of interfaces as contracts: both sides need to keep their end for things to work as intended. Unfortunately the vfio-ccw iterface is not a very well specified one, and that makes reasoning about right order so much harder.

I was under the impression that the right ordering is dictated by the SCSW in userspace. E.g. if there is an FC bit set there userspace is not ought to issue a SSCH request (write to the io_region). The kernel part however may say 'userspace read the actual SCSW' by signaling the io_trigger eventfd. Userspace is supposed to read the IRB from the region and update it's SCSW.

Now if userspace reads a broken SCSW from the IRB, because of a race (due to poorly written kernel part -- userspace not at fault), it is going to make wrong assumptions about currently legal and illegal operations (ordering).

Previously I described a scenario where IRB can break without userspace being at fault (race between unsolicited interrupt -- can happen at any time -- and a legit io request). I was under the impression we agreed on this.

This in turn could lead to userspace violating the contract, as perceived by the kernel side.

Quoted 5 lines

At this point, I'm mostly confused... I'd prefer to simply fix things as they come up so that we can finally move forward with the halt/clear handling (and probably rework the state machine on top of that.)

I understand. I guess you will want to send a new version because of the stuff that got lost in the rebase, or?

Quoted 7 lines · On Wed, 23 Jan 2019 14:06:01 +0100 Halil Pasic <[email protected]> wrote:

On Wed, 23 Jan 2019 11:34:47 +0100
Cornelia Huck <[email protected]> wrote:

Yes, one can usually think of interfaces as contracts: both sides need to keep their end for things to work as intended. Unfortunately the vfio-ccw iterface is not a very well specified one, and that makes reasoning about right order so much harder.

That's probably where our disconnect comes from.

Quoted 8 lines

I was under the impression that the right ordering is dictated by the SCSW in userspace. E.g. if there is an FC bit set there userspace is not ought to issue a SSCH request (write to the io_region). The kernel part however may say 'userspace read the actual SCSW' by signaling the io_trigger eventfd. Userspace is supposed to read the IRB from the region and update it's SCSW.

Quoted 4 lines

Now if userspace reads a broken SCSW from the IRB, because of a race (due to poorly written kernel part -- userspace not at fault), it is going to make wrong assumptions about currently legal and illegal operations (ordering).

My understanding of the interface was that writing to the I/O region triggers a ssch (unless rejected with error) and that reading it just gets whatever the kernel wrote there the last time it updated its internal structures. The eventfd simply triggers to say "the region has been updated with an IRB", not to say "userspace, read this".

Quoted 5 lines

Previously I described a scenario where IRB can break without userspace being at fault (race between unsolicited interrupt -- can happen at any time -- and a legit io request). I was under the impression we agreed on this.

There is a bug in there (clearing the cp for non-final interrupts), and it needs to be fixed. I'm not so sure if the unsolicited interrupt thing is a bug (beyond that the internal state machine is confused).

Quoted 1 line

Quoted 2 lines

This in turn could lead to userspace violating the contract, as perceived by the kernel side.

Which contract? ;)

Also, I'm not sure if we'd rather get a deferred cc 1?

Quoted 5 lines

Quoted 5 lines

At this point, I'm mostly confused... I'd prefer to simply fix things as they come up so that we can finally move forward with the halt/clear handling (and probably rework the state machine on top of that.)

Quoted 4 lines

Quoted 1 line

I understand. I guess you will want to send a new version because of the stuff that got lost in the rebase, or?

Yes, I'll send a new version; but I'll wait for more feedback for a bit.

Quoted 46 lines · Cornelia Huck

Quoted 7 lines · On Wed, 23 Jan 2019 14:06:01 +0100 Halil Pasic <[email protected]> wrote:

On Wed, 23 Jan 2019 11:34:47 +0100
Cornelia Huck <[email protected]> wrote:

Yes, one can usually think of interfaces as contracts: both sides need to keep their end for things to work as intended. Unfortunately the vfio-ccw iterface is not a very well specified one, and that makes reasoning about right order so much harder.

That's probably where our disconnect comes from.

Quoted 8 lines

I was under the impression that the right ordering is dictated by the SCSW in userspace. E.g. if there is an FC bit set there userspace is not ought to issue a SSCH request (write to the io_region). The kernel part however may say 'userspace read the actual SCSW' by signaling the io_trigger eventfd. Userspace is supposed to read the IRB from the region and update it's SCSW.

Quoted 4 lines

Now if userspace reads a broken SCSW from the IRB, because of a race (due to poorly written kernel part -- userspace not at fault), it is going to make wrong assumptions about currently legal and illegal operations (ordering).

My understanding of the interface was that writing to the I/O region triggers a ssch (unless rejected with error) and that reading it just gets whatever the kernel wrote there the last time it updated its internal structures. The eventfd simply triggers to say "the region has been updated with an IRB", not to say "userspace, read this".

Quoted 5 lines

Previously I described a scenario where IRB can break without userspace being at fault (race between unsolicited interrupt -- can happen at any time -- and a legit io request). I was under the impression we agreed on this.

There is a bug in there (clearing the cp for non-final interrupts), and it needs to be fixed. I'm not so sure if the unsolicited interrupt thing is a bug (beyond that the internal state machine is confused).

Quoted 3 lines

This in turn could lead to userspace violating the contract, as perceived by the kernel side.

Quoted 3 lines

Which contract? ;)

Also, I'm not sure if we'd rather get a deferred cc 1?

As I'm encountering dcc=1 quite regularly lately, it's a nice error. But we don't have a good way of recovering from it, and so my test tends to go down in a heap quite quickly. This patch set will probably help; I should really get it applied and try it out.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

At this point, I'm mostly confused... I'd prefer to simply fix things as they come up so that we can finally move forward with the halt/clear handling (and probably rework the state machine on top of that.)

+1 for fixing things as we go. I hear the complaints about this code (and probably say them too), but remain convinced that a large rewrite is unnecessary. Lots of opportunities for improvement, with lots of willing and motivated participants, means it can only get better!

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I understand. I guess you will want to send a new version because of the stuff that got lost in the rebase, or?

Yes, I'll send a new version; but I'll wait for more feedback for a bit.

I'll try to provide some now. Still digging through the emails marked "todo" :)

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

  * @cp: channel program for the current I/O operation
  * @irb: irb info received from interrupt
  * @scsw: scsw info

@@ -42,6 +43,7 @@ struct vfio_ccw_private {
 	struct mdev_device	*mdev;
 	struct notifier_block	nb;
 	struct ccw_io_region	*io_region;
+	struct mutex		io_mutex;
 
 	struct channel_program	cp;
 	struct irb		irb;

drivers/s390/cio/vfio_ccw_private.h line 31

Quoted 7 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

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

@@ -28,6 +28,7 @@
  * @mdev: pointer to the mediated device
  * @nb: notifier for vfio events
  * @io_region: MMIO region to input/output I/O arguments/results
+ * @io_mutex: protect against concurrent update of I/O structures  

We could be a bit more specific about what does this mutex guard. Is it only io_region, or cp, irb and the new regions a well? ->state does not seem to be covered, but should need some sort of synchronisation too, or?

Quoted 13 lines

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

  * @cp: channel program for the current I/O operation
  * @irb: irb info received from interrupt
  * @scsw: scsw info

@@ -42,6 +43,7 @@ struct vfio_ccw_private {
 	struct mdev_device	*mdev;
 	struct notifier_block	nb;
 	struct ccw_io_region	*io_region;
+	struct mutex		io_mutex;
 
 	struct channel_program	cp;
 	struct irb		irb;

Quoted 15 lines · On Tue, 22 Jan 2019 19:33:46 +0100 Halil Pasic <[email protected]> wrote:

drivers/s390/cio/vfio_ccw_private.h line 31

Quoted 7 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

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

@@ -28,6 +28,7 @@
  * @mdev: pointer to the mediated device
  * @nb: notifier for vfio events
  * @io_region: MMIO region to input/output I/O arguments/results
+ * @io_mutex: protect against concurrent update of I/O structures  

We could be a bit more specific about what does this mutex guard. Is it only io_region, or cp, irb and the new regions a well? ->state does not seem to be covered, but should need some sort of synchronisation too, or?

I'm not sure. IIRC Pierre had some ideas about locking in the fsm?

Quoted 14 lines

Quoted 13 lines

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

  * @cp: channel program for the current I/O operation
  * @irb: irb info received from interrupt
  * @scsw: scsw info

@@ -42,6 +43,7 @@ struct vfio_ccw_private {
 	struct mdev_device	*mdev;
 	struct notifier_block	nb;
 	struct ccw_io_region	*io_region;
+	struct mutex		io_mutex;
 
 	struct channel_program	cp;
 	struct irb		irb;

Quoted 19 lines · On Wed, 23 Jan 2019 11:21:12 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 15 lines · On Tue, 22 Jan 2019 19:33:46 +0100 Halil Pasic <[email protected]> wrote:

drivers/s390/cio/vfio_ccw_private.h line 31

Quoted 7 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

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

@@ -28,6 +28,7 @@
  * @mdev: pointer to the mediated device
  * @nb: notifier for vfio events
  * @io_region: MMIO region to input/output I/O arguments/results
+ * @io_mutex: protect against concurrent update of I/O structures    

We could be a bit more specific about what does this mutex guard. Is it only io_region, or cp, irb and the new regions a well? ->state does not seem to be covered, but should need some sort of synchronisation too, or?

Quoted 2 lines

I'm not sure. IIRC Pierre had some ideas about locking in the fsm?

Yes there was something. I usually do review by sanity checking the resulting code. IMHO the fsm stuff is broken now and differently broken after this series. If we don't want to fix what we are touching, maybe a pointing out ignored problems in patch descriptions and a minimal-invasive approach could help ease review.

Regards,
Halil

Quoted 16 lines

Quoted 14 lines

Quoted 13 lines

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

  * @cp: channel program for the current I/O operation
  * @irb: irb info received from interrupt
  * @scsw: scsw info

@@ -42,6 +43,7 @@ struct vfio_ccw_private {
 	struct mdev_device	*mdev;
 	struct notifier_block	nb;
 	struct ccw_io_region	*io_region;
+	struct mutex		io_mutex;
 
 	struct channel_program	cp;
 	struct irb		irb;

Quoted 15 lines · On Tue, 22 Jan 2019 19:33:46 +0100 Halil Pasic <[email protected]> wrote:

drivers/s390/cio/vfio_ccw_private.h line 31

Quoted 7 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

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

@@ -28,6 +28,7 @@
   * @mdev: pointer to the mediated device
   * @nb: notifier for vfio events
   * @io_region: MMIO region to input/output I/O arguments/results
+ * @io_mutex: protect against concurrent update of I/O structures

We could be a bit more specific about what does this mutex guard. Is it only io_region, or cp, irb and the new regions a well? ->state does not seem to be covered, but should need some sort of synchronisation too, or?

I'm not sure. IIRC Pierre had some ideas about locking in the fsm?

Quoted 19 lines · Cornelia Huck

Quoted 15 lines · On Tue, 22 Jan 2019 19:33:46 +0100 Halil Pasic <[email protected]> wrote:

drivers/s390/cio/vfio_ccw_private.h line 31

Quoted 7 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

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

@@ -28,6 +28,7 @@
   * @mdev: pointer to the mediated device
   * @nb: notifier for vfio events
   * @io_region: MMIO region to input/output I/O arguments/results
+ * @io_mutex: protect against concurrent update of I/O structures  

We could be a bit more specific about what does this mutex guard. Is it only io_region, or cp, irb and the new regions a well? ->state does not seem to be covered, but should need some sort of synchronisation too, or?

Quoted 2 lines

I'm not sure. IIRC Pierre had some ideas about locking in the fsm?

Yes I postponed this work to not collide with your patch series.

Do you think I should provide a new version of the FSM reworking series based on the last comment I got?

I would take into account that the asynchronous commands will come with your patch series and only provide the framework changes.

Quoted 23 lines · On Thu, 24 Jan 2019 11:08:02 +0100 Pierre Morel <[email protected]> wrote:

Quoted 19 lines · Cornelia Huck

Quoted 15 lines · On Tue, 22 Jan 2019 19:33:46 +0100 Halil Pasic <[email protected]> wrote:

drivers/s390/cio/vfio_ccw_private.h line 31

Quoted 7 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

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

@@ -28,6 +28,7 @@
    * @mdev: pointer to the mediated device
    * @nb: notifier for vfio events
    * @io_region: MMIO region to input/output I/O arguments/results
+ * @io_mutex: protect against concurrent update of I/O structures

We could be a bit more specific about what does this mutex guard. Is it only io_region, or cp, irb and the new regions a well? ->state does not seem to be covered, but should need some sort of synchronisation too, or?

Quoted 2 lines

I'm not sure. IIRC Pierre had some ideas about locking in the fsm?

Quoted 7 lines

Yes I postponed this work to not collide with your patch series.

Do you think I should provide a new version of the FSM reworking series based on the last comment I got?

I would take into account that the asynchronous commands will come with your patch series and only provide the framework changes.

This was more an answer to Halil's concerns around state synchronization. I would prefer to first get this series (or a variation) into decent shape, and then address state machine handling on top of that (when we know more about the transitions involved), just to avoid confusion.

Does that sound reasonable?

Quoted 23 lines · On Thu, 24 Jan 2019 11:08:02 +0100 Pierre Morel <[email protected]> wrote:

Quoted 19 lines · Cornelia Huck

Quoted 15 lines · On Tue, 22 Jan 2019 19:33:46 +0100 Halil Pasic <[email protected]> wrote:

drivers/s390/cio/vfio_ccw_private.h line 31

Quoted 7 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

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

@@ -28,6 +28,7 @@
   * @mdev: pointer to the mediated device
   * @nb: notifier for vfio events
   * @io_region: MMIO region to input/output I/O arguments/results
+ * @io_mutex: protect against concurrent update of I/O structures  

We could be a bit more specific about what does this mutex guard. Is it only io_region, or cp, irb and the new regions a well? ->state does not seem to be covered, but should need some sort of synchronisation too, or?

Quoted 2 lines

I'm not sure. IIRC Pierre had some ideas about locking in the fsm?

Quoted 7 lines

Yes I postponed this work to not collide with your patch series.

Do you think I should provide a new version of the FSM reworking series based on the last comment I got?

I would take into account that the asynchronous commands will come with your patch series and only provide the framework changes.

This was more an answer to Halil's concerns around state synchronization. I would prefer to first get this series (or a variation) into decent shape, and then address state machine handling on top of that (when we know more about the transitions involved), just to avoid confusion.

Does that sound reasonable?

Quoted 23 lines · On Thu, 24 Jan 2019 11:08:02 +0100 Pierre Morel <[email protected]> wrote:

Quoted 19 lines · Cornelia Huck

Quoted 15 lines · On Tue, 22 Jan 2019 19:33:46 +0100 Halil Pasic <[email protected]> wrote:

drivers/s390/cio/vfio_ccw_private.h line 31

Quoted 7 lines · On Mon, 21 Jan 2019 12:03:51 +0100 Cornelia Huck <[email protected]> wrote:

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

@@ -28,6 +28,7 @@
    * @mdev: pointer to the mediated device
    * @nb: notifier for vfio events
    * @io_region: MMIO region to input/output I/O arguments/results
+ * @io_mutex: protect against concurrent update of I/O structures

We could be a bit more specific about what does this mutex guard. Is it only io_region, or cp, irb and the new regions a well? ->state does not seem to be covered, but should need some sort of synchronisation too, or?

Quoted 2 lines

I'm not sure. IIRC Pierre had some ideas about locking in the fsm?

Quoted 7 lines

Yes I postponed this work to not collide with your patch series.

Do you think I should provide a new version of the FSM reworking series based on the last comment I got?

I would take into account that the asynchronous commands will come with your patch series and only provide the framework changes.

This was more an answer to Halil's concerns around state synchronization. I would prefer to first get this series (or a variation) into decent shape, and then address state machine handling on top of that (when we know more about the transitions involved), just to avoid confusion.

Does that sound reasonable?

Rework handling of multiple I/O requests to return -EAGAIN if we are already processing an I/O request. Introduce a mutex to disallow concurrent writes to the I/O region.

The expectation is that userspace simply retries the operation if it gets -EAGAIN.

We currently don't allow multiple ssch requests at the same time, as we don't have support for keeping channel programs around for more than one request.

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

---
  drivers/s390/cio/vfio_ccw_drv.c     |  1 +
  drivers/s390/cio/vfio_ccw_fsm.c     |  8 +++-----
  drivers/s390/cio/vfio_ccw_ops.c     | 31 +++++++++++++++++++----------
  drivers/s390/cio/vfio_ccw_private.h |  2 ++
  4 files changed, 26 insertions(+), 16 deletions(-)

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

index a10cec0e86eb..2ef189fe45ed 100644
@@ -125,6 +125,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 
  	private->sch = sch;
  	dev_set_drvdata(&sch->dev, private);
+	mutex_init(&private->io_mutex);
 
  	spin_lock_irq(sch->lock);
  	private->state = VFIO_CCW_STATE_NOT_OPER;

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

index cab17865aafe..f6ed934cc565 100644

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

  		break;
  	case 1:		/* Status pending */
  	case 2:		/* Busy */

@@ -99,7 +100,7 @@ static void fsm_io_error(struct vfio_ccw_private *private,
  static void fsm_io_busy(struct vfio_ccw_private *private,
  			enum vfio_ccw_event event)
  {
-	private->io_region->ret_code = -EBUSY;
+	private->io_region->ret_code = -EAGAIN;
  }
 
  static void fsm_disabled_irq(struct vfio_ccw_private *private,

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

  	trace_vfio_ccw_io_fctl(scsw->cmd.fctl, get_schid(private),
  			       io_region->ret_code, errstr);
  }

drivers/s390/cio/vfio_ccw_ops.c +8 -3

index f673e106c041..3fa9fc570400 100644
@@ -169,16 +169,20 @@ static ssize_t vfio_ccw_mdev_read(struct mdev_device *mdev,
  {
  	struct vfio_ccw_private *private;
  	struct ccw_io_region *region;
+	int ret;
 
  	if (*ppos + count > sizeof(*region))
  		return -EINVAL;
 
  	private = dev_get_drvdata(mdev_parent_dev(mdev));
+	mutex_lock(&private->io_mutex);
  	region = private->io_region;
  	if (copy_to_user(buf, (void *)region + *ppos, count))
-		return -EFAULT;
-
-	return count;
+		ret = -EFAULT;
+	else
+		ret = count;
+	mutex_unlock(&private->io_mutex);
+	return ret;
  }
 
  static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
  {
  	struct vfio_ccw_private *private;
  	struct ccw_io_region *region;
+	int ret;
 

drivers/s390/cio/vfio_ccw_ops.c +4 -3

-		return region->ret_code;
-	}
+	ret = (region->ret_code != 0) ? region->ret_code : count;
 
-	return count;
+out_unlock:
+	mutex_unlock(&private->io_mutex);
+	return ret;
  }
 
  static int vfio_ccw_mdev_get_device_info(struct vfio_device_info *info)

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

index 08e9a7dc9176..e88237697f83 100644
@@ -28,6 +28,7 @@
   * @mdev: pointer to the mediated device
   * @nb: notifier for vfio events
   * @io_region: MMIO region to input/output I/O arguments/results
+ * @io_mutex: protect against concurrent update of I/O structures
   * @cp: channel program for the current I/O operation
   * @irb: irb info received from interrupt
   * @scsw: scsw info

@@ -42,6 +43,7 @@ struct vfio_ccw_private {
  	struct mdev_device	*mdev;
  	struct notifier_block	nb;
  	struct ccw_io_region	*io_region;
+	struct mutex		io_mutex;
 
  	struct channel_program	cp;
  	struct irb		irb;

Quoted 34 lines · Cornelia Huck

Rework handling of multiple I/O requests to return -EAGAIN if we are already processing an I/O request. Introduce a mutex to disallow concurrent writes to the I/O region.

The expectation is that userspace simply retries the operation if it gets -EAGAIN.

We currently don't allow multiple ssch requests at the same time, as we don't have support for keeping channel programs around for more than one request.

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

---

  drivers/s390/cio/vfio_ccw_drv.c     |  1 +
  drivers/s390/cio/vfio_ccw_fsm.c     |  8 +++-----
  drivers/s390/cio/vfio_ccw_ops.c     | 31 +++++++++++++++++++----------
  drivers/s390/cio/vfio_ccw_private.h |  2 ++
  4 files changed, 26 insertions(+), 16 deletions(-)

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

index a10cec0e86eb..2ef189fe45ed 100644
@@ -125,6 +125,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
       private->sch = sch;
       dev_set_drvdata(&sch->dev, private);
+    mutex_init(&private->io_mutex);
       spin_lock_irq(sch->lock);
       private->state = VFIO_CCW_STATE_NOT_OPER;

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

index cab17865aafe..f6ed934cc565 100644

drivers/s390/cio/vfio_ccw_fsm.c static int fsm_io_helper(struct vfio_ccw_private · line 31

Quoted 4 lines

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

@@ -28,7 +28,6 @@ static int fsm_io_helper(struct vfio_ccw_private 
 *private)
       sch = private->sch;
       spin_lock_irqsave(sch->lock, flags);
-    private->state = VFIO_CCW_STATE_BUSY;

[1]

drivers/s390/cio/vfio_ccw_fsm.c static int fsm_io_helper(struct vfio_ccw_private · line 46

Quoted 8 lines

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

       orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);

@@ -42,6 +41,8 @@ static int fsm_io_helper(struct vfio_ccw_private 
 *private)
            */
           sch->schib.scsw.cmd.actl |= SCSW_ACTL_START_PEND;
           ret = 0;
+        /* Don't allow another ssch for now */
+        private->state = VFIO_CCW_STATE_BUSY;

[1]

Quoted 13 lines

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

           break;

      case 1:        /* Status pending */
      case 2:        /* Busy */

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

@@ -99,7 +100,7 @@ static void fsm_io_error(struct vfio_ccw_private 
 *private,
   static void fsm_io_busy(struct vfio_ccw_private *private,
               enum vfio_ccw_event event)
   {
-    private->io_region->ret_code = -EBUSY;
+    private->io_region->ret_code = -EAGAIN;
   }
   static void fsm_disabled_irq(struct vfio_ccw_private *private,

drivers/s390/cio/vfio_ccw_fsm.c static void fsm_io_request(struct vfio_ccw_private · line 134

Quoted 5 lines

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

@@ -130,8 +131,6 @@ static void fsm_io_request(struct vfio_ccw_private 
 *private,
       struct mdev_device *mdev = private->mdev;
       char *errstr = "request";
-    private->state = VFIO_CCW_STATE_BUSY;
-

[1]

Quoted 7 lines

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

       memcpy(scsw, io_region->scsw_area, sizeof(*scsw));
       if (scsw->cmd.fctl & SCSW_FCTL_START_FUNC) {

@@ -176,7 +175,6 @@ static void fsm_io_request(struct vfio_ccw_private 
 *private,
       }

  err_out:
-    private->state = VFIO_CCW_STATE_IDLE;

[1] I think these changes are cool.  We end up going into (and staying in) state=BUSY if we get cc=0 on the SSCH, rather than in/out as we bumble along.

But why can't these be separated out from this patch?  It does change the behavior of the state machine, and seem distinct from the addition of the mutex you otherwise add here?  At the very least, this behavior change should be documented in the commit since it's otherwise lost in the mutex/EAGAIN stuff.

Quoted 34 lines

      trace_vfio_ccw_io_fctl(scsw->cmd.fctl, get_schid(private), io_region->ret_code, errstr);
  }

drivers/s390/cio/vfio_ccw_ops.c +7 -3

index f673e106c041..3fa9fc570400 100644
@@ -169,16 +169,20 @@ static ssize_t vfio_ccw_mdev_read(struct 
 mdev_device *mdev,
   {
       struct vfio_ccw_private *private;
       struct ccw_io_region *region;
+    int ret;
       if (*ppos + count > sizeof(*region))
           return -EINVAL;
       private = dev_get_drvdata(mdev_parent_dev(mdev));
+    mutex_lock(&private->io_mutex);
       region = private->io_region;
       if (copy_to_user(buf, (void *)region + *ppos, count))
-        return -EFAULT;
-
-    return count;
+        ret = -EFAULT;
+    else
+        ret = count;
+    mutex_unlock(&private->io_mutex);
+    return ret;
   }
   static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct 
 mdev_device *mdev,
   {
       struct vfio_ccw_private *private;
       struct ccw_io_region *region;

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct · line 204

Quoted 10 lines

drivers/s390/cio/vfio_ccw_ops.c +5 -1

+    int ret;
       if (*ppos + count > sizeof(*region))
           return -EINVAL;
       private = dev_get_drvdata(mdev_parent_dev(mdev));
-    if (private->state != VFIO_CCW_STATE_IDLE)
+    if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+        private->state == VFIO_CCW_STATE_STANDBY)
           return -EACCES;
+    if (!mutex_trylock(&private->io_mutex))
+        return -EAGAIN;

Ah, I see Halil's difficulty here.

It is true there is a race condition today, and that this doesn't address it.  That's fine, add it to the todo list.  But even with that, I don't see what the mutex is enforcing?  Two simultaneous SSCHs will be serialized (one will get kicked out with a failed trylock() call), while still leaving the window open between cc=0 on the SSCH and the subsequent interrupt.  In the latter case, a second SSCH will come through here, do the copy_from_user below, and then jump to fsm_io_busy to return EAGAIN.  Do we really want to stomp on io_region in that case?
 Why can't we simply return EAGAIN if state==BUSY?

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, · line 205

Quoted 11 lines

drivers/s390/cio/vfio_ccw_ops.c +4 -4

       region = private->io_region;
-    if (copy_from_user((void *)region + *ppos, buf, count))
-        return -EFAULT;
+    if (copy_from_user((void *)region + *ppos, buf, count)) {
+        ret = -EFAULT;
+        goto out_unlock;
+    }
       vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ)
-    if (region->ret_code != 0) {
-        private->state = VFIO_CCW_STATE_IDLE;

[1] (above)

Quoted 29 lines

drivers/s390/cio/vfio_ccw_ops.c +4 -3

-        return region->ret_code;
-    }
+    ret = (region->ret_code != 0) ? region->ret_code : count;
-    return count;
+out_unlock:
+    mutex_unlock(&private->io_mutex);
+    return ret;
   }
   static int vfio_ccw_mdev_get_device_info(struct vfio_device_info *info)

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

index 08e9a7dc9176..e88237697f83 100644
@@ -28,6 +28,7 @@
    * @mdev: pointer to the mediated device
    * @nb: notifier for vfio events
    * @io_region: MMIO region to input/output I/O arguments/results
+ * @io_mutex: protect against concurrent update of I/O structures
    * @cp: channel program for the current I/O operation
    * @irb: irb info received from interrupt
    * @scsw: scsw info

@@ -42,6 +43,7 @@ struct vfio_ccw_private {
       struct mdev_device    *mdev;
       struct notifier_block    nb;
       struct ccw_io_region    *io_region;
+    struct mutex        io_mutex;
       struct channel_program    cp;
       struct irb        irb;
 

Quoted 8 lines · Eric Farman

On 01/21/2019 06:03 AM, Cornelia Huck wrote:

[1] I think these changes are cool.  We end up going into (and staying in) state=BUSY if we get cc=0 on the SSCH, rather than in/out as we bumble along.

Quoted 5 lines

But why can't these be separated out from this patch?  It does change the behavior of the state machine, and seem distinct from the addition of the mutex you otherwise add here?  At the very least, this behavior change should be documented in the commit since it's otherwise lost in the mutex/EAGAIN stuff.

Quoted 49 lines

Quoted 36 lines

      trace_vfio_ccw_io_fctl(scsw->cmd.fctl, get_schid(private), io_region->ret_code, errstr);
  }

drivers/s390/cio/vfio_ccw_ops.c +7 -3

index f673e106c041..3fa9fc570400 100644
@@ -169,16 +169,20 @@ static ssize_t vfio_ccw_mdev_read(struct 
 mdev_device *mdev,
   {
       struct vfio_ccw_private *private;
       struct ccw_io_region *region;
+    int ret;
       if (*ppos + count > sizeof(*region))
           return -EINVAL;
       private = dev_get_drvdata(mdev_parent_dev(mdev));
+    mutex_lock(&private->io_mutex);
       region = private->io_region;
       if (copy_to_user(buf, (void *)region + *ppos, count))
-        return -EFAULT;
-
-    return count;
+        ret = -EFAULT;
+    else
+        ret = count;
+    mutex_unlock(&private->io_mutex);
+    return ret;
   }
   static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct 
 mdev_device *mdev,
   {
       struct vfio_ccw_private *private;
       struct ccw_io_region *region;

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct · line 204

Quoted 10 lines

drivers/s390/cio/vfio_ccw_ops.c +5 -1

+    int ret;
       if (*ppos + count > sizeof(*region))
           return -EINVAL;
       private = dev_get_drvdata(mdev_parent_dev(mdev));
-    if (private->state != VFIO_CCW_STATE_IDLE)
+    if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+        private->state == VFIO_CCW_STATE_STANDBY)
           return -EACCES;
+    if (!mutex_trylock(&private->io_mutex))
+        return -EAGAIN;  

Ah, I see Halil's difficulty here.

Quoted 9 lines

It is true there is a race condition today, and that this doesn't address it.  That's fine, add it to the todo list.  But even with that, I don't see what the mutex is enforcing?  Two simultaneous SSCHs will be serialized (one will get kicked out with a failed trylock() call), while still leaving the window open between cc=0 on the SSCH and the subsequent interrupt.  In the latter case, a second SSCH will come through here, do the copy_from_user below, and then jump to fsm_io_busy to return EAGAIN.  Do we really want to stomp on io_region in that case?
 Why can't we simply return EAGAIN if state==BUSY?

(Answering my own questions as I skim patch 5...)

Because of course this series is for async handling, while I was looking specifically at the synchronous code that exists today. I guess then my question just remains on how the mutex is adding protection in the sync case, because that's still not apparent to me. (Perhaps I missed it in a reply to Halil; if so I apologize, there were a lot when I returned.)

Quoted 9 lines · On Thu, 24 Jan 2019 21:37:44 -0500 Eric Farman <[email protected]> wrote:

Quoted 8 lines · Eric Farman

On 01/21/2019 06:03 AM, Cornelia Huck wrote:

[1] I think these changes are cool.  We end up going into (and staying in) state=BUSY if we get cc=0 on the SSCH, rather than in/out as we bumble along.

Quoted 5 lines

Quoted 5 lines

But why can't these be separated out from this patch?  It does change the behavior of the state machine, and seem distinct from the addition of the mutex you otherwise add here?  At the very least, this behavior change should be documented in the commit since it's otherwise lost in the mutex/EAGAIN stuff.

That's a very good idea. I'll factor them out into a separate patch.

Quoted 12 lines · On Fri, 25 Jan 2019 11:24:37 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 9 lines · On Thu, 24 Jan 2019 21:37:44 -0500 Eric Farman <[email protected]> wrote:

Quoted 8 lines · Eric Farman

On 01/21/2019 06:03 AM, Cornelia Huck wrote:

[1] I think these changes are cool.  We end up going into (and staying in) state=BUSY if we get cc=0 on the SSCH, rather than in/out as we bumble along.

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

But why can't these be separated out from this patch?  It does change the behavior of the state machine, and seem distinct from the addition of the mutex you otherwise add here?  At the very least, this behavior change should be documented in the commit since it's otherwise lost in the mutex/EAGAIN stuff.

That's a very good idea. I'll factor them out into a separate patch.

And now that I've factored it out, I noticed some more problems.

What we basically need is the following, I think:

- The code should not be interrupted while we process the channel program, do the ssch etc. We want the caller to try again later (i.e. return -EAGAIN)

- We currently do not want the user space to submit another channel program while the first one is still in flight. As submitting another one is a valid request, however, we should allow this in the future (once we have the code to handle that in place).

- With the async interface, we want user space to be able to submit a halt/clear while a start request is still in flight, but not while we're processing a start request with translation etc. We probably want to do -EAGAIN in that case.

My idea would be:

- The BUSY state denotes "I'm busy processing a request right now, try again". We hold it while processing the cp and doing the ssch and leave it afterwards (i.e., while the start request is processed by the hardware). I/O requests and async requests get -EAGAIN in that state.
- A new state (CP_PENDING?) is entered after ssch returned with cc 0 (from the BUSY state). We stay in there as long as no final state for that request has been received and delivered. (This may be final interrupt for that request, a deferred cc, or successful halt/clear.) I/O requests get -EBUSY, async requests are processed. This state can be removed again once we are able to handle more than one outstanding cp.

Does that make sense?

Quoted 4 lines · On Fri, 25 Jan 2019 13:58:35 +0100 Cornelia Huck <[email protected]> wrote:

- We currently do not want the user space to submit another channel program while the first one is still in flight. As submitting another one is a valid request, however, we should allow this in the future (once we have the code to handle that in place).

I don't agree. There is at most one channel program processed by a subchannel at any time. I would prefer an early error code if channel programs are issued on top of each other (our virtual subchannel is channel pending or FC start function bit set or similar).

Quoted 8 lines · On Fri, 25 Jan 2019 15:01:01 +0100 Halil Pasic <[email protected]> wrote:

Quoted 4 lines · On Fri, 25 Jan 2019 13:58:35 +0100 Cornelia Huck <[email protected]> wrote:

- We currently do not want the user space to submit another channel program while the first one is still in flight. As submitting another one is a valid request, however, we should allow this in the future (once we have the code to handle that in place).

Quoted 4 lines

I don't agree. There is at most one channel program processed by a subchannel at any time. I would prefer an early error code if channel programs are issued on top of each other (our virtual subchannel is channel pending or FC start function bit set or similar).

You can submit a new request if the subchannel is pending with primary, but not with secondary state.

Regardless of that, I think it is much easier to push as much as possible of sorting out of requests to the hardware.

Do we expect userspace/QEMU to fence the bad scenarios as tries to do today, or is this supposed to change to hardware should sort out requests whenever possible.

The problem I see with the let the hardware sort it out is that, for that to work, we need to juggle multiple translations simultaneously (or am I wrong?). Doing that does not appear particularly simple to me.

Furthermore we would go through all that hassle knowingly that the sole reason is working around bugs. We still expect our Linux guests serializing it's ssch() stuff as it does today. Thus I would except this code not getting the love nor the coverage that would guard against bugs in that code.

Quoted 3 lines · On Fri, 25 Jan 2019 17:04:04 +0100 Halil Pasic <[email protected]> wrote:

Do we expect userspace/QEMU to fence the bad scenarios as tries to do today, or is this supposed to change to hardware should sort out requests whenever possible.

Does my other mail answer that?

Quoted 5 lines

The problem I see with the let the hardware sort it out is that, for that to work, we need to juggle multiple translations simultaneously (or am I wrong?). Doing that does not appear particularly simple to me.

None in the first stage, at most two in the second stage, I guess.

Quoted 5 lines

Furthermore we would go through all that hassle knowingly that the sole reason is working around bugs. We still expect our Linux guests serializing it's ssch() stuff as it does today. Thus I would except this code not getting the love nor the coverage that would guard against bugs in that code.

So, we should have test code for that? (Any IBM-internal channel I/O exercisers that may help?)

We should not rely on the guest being sane, although Linux probably is in that respect.

Quoted 7 lines · On Mon, 28 Jan 2019 18:13:55 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 3 lines · On Fri, 25 Jan 2019 17:04:04 +0100 Halil Pasic <[email protected]> wrote:

Do we expect userspace/QEMU to fence the bad scenarios as tries to do today, or is this supposed to change to hardware should sort out requests whenever possible.

Quoted 1 line

Does my other mail answer that?

Sorry, I can't find the answer in your other (Date: Mon, 28 Jan 2019 17:59:10 +0100, Message-Id: <20190128175910.5d9677e7@oc2783563651>) mail. AFAIU that mail talks abut the kernel and not about the userspace.

I guess the answer is we don't expect changes to userspace, so we do expect userspace to fence bad scenarios.

Quoted 9 lines

Quoted 6 lines

The problem I see with the let the hardware sort it out is that, for that to work, we need to juggle multiple translations simultaneously (or am I wrong?). Doing that does not appear particularly simple to me.

None in the first stage, at most two in the second stage, I guess.

Expected benefit of the second stage over the first stage? (I see none.)

Quoted 10 lines

Quoted 6 lines

Furthermore we would go through all that hassle knowingly that the sole reason is working around bugs. We still expect our Linux guests serializing it's ssch() stuff as it does today. Thus I would except this code not getting the love nor the coverage that would guard against bugs in that code.

So, we should have test code for that? (Any IBM-internal channel I/O exercisers that may help?)

None that I'm aware of. Anyone else?

But the point I was trying to make is the following: I prefer keeping the handling for the case "ssch()'s on top of each other" as trivial as possible. (E.g. bail out if CP_PENDING without doing any translation.)

Quoted 3 lines

We should not rely on the guest being sane, although Linux probably is in that respect.

I agree 100%: we should not rely on either guest or userspace emulator being sane. But IMHO we should handle insanity with the least possible investment.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

The problem I see with the let the hardware sort it out is that, for that to work, we need to juggle multiple translations simultaneously (or am I wrong?). Doing that does not appear particularly simple to me.

Quoted 4 lines

Quoted 2 lines

None in the first stage, at most two in the second stage, I guess.

Expected benefit of the second stage over the first stage? (I see none.)

Making something possible that is allowed by the architecture. Not really important, though.

Quoted 21 lines

Quoted 10 lines

Quoted 6 lines

Furthermore we would go through all that hassle knowingly that the sole reason is working around bugs. We still expect our Linux guests serializing it's ssch() stuff as it does today. Thus I would except this code not getting the love nor the coverage that would guard against bugs in that code.

So, we should have test code for that? (Any IBM-internal channel I/O exercisers that may help?)

None that I'm aware of. Anyone else?

But the point I was trying to make is the following: I prefer keeping the handling for the case "ssch()'s on top of each other" as trivial as possible. (E.g. bail out if CP_PENDING without doing any translation.)

Quoted 3 lines

We should not rely on the guest being sane, although Linux probably is in that respect.

Quoted 3 lines

I agree 100%: we should not rely on either guest or userspace emulator being sane. But IMHO we should handle insanity with the least possible investment.

We probably disagree what the least possible investment is.

Quoted 10 lines · On Tue, 29 Jan 2019 10:58:40 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

The problem I see with the let the hardware sort it out is that, for that to work, we need to juggle multiple translations simultaneously (or am I wrong?). Doing that does not appear particularly simple to me.

None in the first stage, at most two in the second stage, I guess.

Expected benefit of the second stage over the first stage? (I see none.)

Quoted 3 lines

Making something possible that is allowed by the architecture. Not really important, though.

I had a chat with Farhan, and he suggested that by 'allowed by architecture' you mean " You can submit a new request if the subchannel is pending with primary, but not with secondary state." (from Message-ID:
<[email protected]>).

So I re-read the PoP.

From the description of the start subchannel instruction:
"""
Special Conditions

Condition code 1 is set, and no other action is taken, when the subchannel is status pending when START SUBCHANNEL is executed. On some mod- els, condition code 1 is not set when the subchannel is status pending with only secondary status; instead, the status-pending condition is discarded.

Condition code 2 is set, and no other action is taken, when a start, halt, or clear function is currently in progress at the subchannel (see “Function Control (FC)” on page 13).

"""

So I guess you mixed primary and secondary up and wanted to say:
"You can submit a new request if the subchannel
is pending with _secondary_, but not with _primary_ _status_."

But does that really mean architecture allows the subchannel to accept multiple ssch() instructions so that it ends up processing two or more channel programs in parallel.

Quoted 13 lines · On Tue, 29 Jan 2019 20:39:33 +0100 Halil Pasic <[email protected]> wrote:

Quoted 10 lines · On Tue, 29 Jan 2019 10:58:40 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

The problem I see with the let the hardware sort it out is that, for that to work, we need to juggle multiple translations simultaneously (or am I wrong?). Doing that does not appear particularly simple to me.

None in the first stage, at most two in the second stage, I guess.

Expected benefit of the second stage over the first stage? (I see none.)

Quoted 8 lines

Quoted 3 lines

Making something possible that is allowed by the architecture. Not really important, though.

I had a chat with Farhan, and he suggested that by 'allowed by architecture' you mean " You can submit a new request if the subchannel is pending with primary, but not with secondary state." (from Message-ID:
<[email protected]>).

Yes. I might have mixed things up, though.

Quoted 21 lines

So I re-read the PoP.

From the description of the start subchannel instruction:
"""
Special Conditions

Condition code 1 is set, and no other action is taken, when the subchannel is status pending when START SUBCHANNEL is executed. On some mod- els, condition code 1 is not set when the subchannel is status pending with only secondary status; instead, the status-pending condition is discarded.

Condition code 2 is set, and no other action is taken, when a start, halt, or clear function is currently in progress at the subchannel (see “Function Control (FC)” on page 13).

"""

Quoted 7 lines

So I guess you mixed primary and secondary up and wanted to say:
"You can submit a new request if the subchannel
is pending with _secondary_, but not with _primary_ _status_."

But does that really mean architecture allows the subchannel to accept multiple ssch() instructions so that it ends up processing two or more channel programs in parallel.

That's not what I meant. The vfio-ccw driver still holds on to one cp, while a second one could be submitted.

But let's just end discussing this here, and continue with discussing the reworked state machine, ok? It's not really relevant for going forward with halt/clear.

Quoted 3 lines · On Fri, 25 Jan 2019 13:58:35 +0100 Cornelia Huck <[email protected]> wrote:

- The code should not be interrupted while we process the channel program, do the ssch etc. We want the caller to try again later (i.e. return -EAGAIN)

Quoted 4 lines

- With the async interface, we want user space to be able to submit a halt/clear while a start request is still in flight, but not while we're processing a start request with translation etc. We probably want to do -EAGAIN in that case.

This reads very similar to your first point.

Quoted 17 lines

My idea would be:

- The BUSY state denotes "I'm busy processing a request right now, try again". We hold it while processing the cp and doing the ssch and leave it afterwards (i.e., while the start request is processed by the hardware). I/O requests and async requests get -EAGAIN in that state.
- A new state (CP_PENDING?) is entered after ssch returned with cc 0 (from the BUSY state). We stay in there as long as no final state for that request has been received and delivered. (This may be final interrupt for that request, a deferred cc, or successful halt/clear.) I/O requests get -EBUSY, async requests are processed. This state can be removed again once we are able to handle more than one outstanding cp.

Quoted 2 lines

Does that make sense?

AFAIU your idea is to split up the busy state into two states: CP_PENDING and of busy without CP_PENDING called BUSY. I like the idea of having a separate state for CP_PENDING but I don't like the new semantic of BUSY.

Hm mashing a conceptual state machine and the jumptabe stuff ain't making reasoning about this simpler either. I'm taking about the conceptual state machine. It would be nice to have a picture of it and then think about how to express that in code.

Quoted 6 lines · On Fri, 25 Jan 2019 15:01:01 +0100 Halil Pasic <[email protected]> wrote:

Quoted 3 lines · On Fri, 25 Jan 2019 13:58:35 +0100 Cornelia Huck <[email protected]> wrote:

- The code should not be interrupted while we process the channel program, do the ssch etc. We want the caller to try again later (i.e. return -EAGAIN)

(...)

Quoted 5 lines

Quoted 4 lines

- With the async interface, we want user space to be able to submit a halt/clear while a start request is still in flight, but not while we're processing a start request with translation etc. We probably want to do -EAGAIN in that case.

Quoted 1 line

This reads very similar to your first point.

Not quite. ssch() means that we have a cp around; for hsch()/csch() we don't have such a thing. So we want to protect the process of translating the cp etc., but we don't need such protection for the halt/clear processing.

Quoted 24 lines

Quoted 17 lines

My idea would be:

- The BUSY state denotes "I'm busy processing a request right now, try again". We hold it while processing the cp and doing the ssch and leave it afterwards (i.e., while the start request is processed by the hardware). I/O requests and async requests get -EAGAIN in that state.
- A new state (CP_PENDING?) is entered after ssch returned with cc 0 (from the BUSY state). We stay in there as long as no final state for that request has been received and delivered. (This may be final interrupt for that request, a deferred cc, or successful halt/clear.) I/O requests get -EBUSY, async requests are processed. This state can be removed again once we are able to handle more than one outstanding cp.

Quoted 2 lines

Does that make sense?

AFAIU your idea is to split up the busy state into two states: CP_PENDING and of busy without CP_PENDING called BUSY. I like the idea of having a separate state for CP_PENDING but I don't like the new semantic of BUSY.

Quoted 4 lines

Hm mashing a conceptual state machine and the jumptabe stuff ain't making reasoning about this simpler either. I'm taking about the conceptual state machine. It would be nice to have a picture of it and then think about how to express that in code.

Sorry, I'm having a hard time parsing your comments. Are you looking for something like the below?

IDLE --- IO_REQ --> BUSY ---> CP_PENDING --- IRQ ---> IDLE (if final

state for I/O)
(normal ssch)

BUSY --- IO_REQ ---> return -EAGAIN, stay in BUSY
(user space is supposed to retry, as we'll eventually progress from BUSY)

CP_PENDING --- IO_REQ ---> return -EBUSY, stay in CP_PENDING (user space is supposed to map this to the appropriate cc for the guest)

IDLE --- ASYNC_REQ ---> IDLE
(user space is welcome to do anything else right away)

BUSY --- ASYNC_REQ ---> return -EAGAIN, stay in BUSY (user space is supposed to retry, as above)

CP_PENDING --- ASYNC_REQ ---> return success, stay in CP_PENDING (the interrupt will get us out of CP_PENDING eventually)

Quoted 17 lines · On Mon, 28 Jan 2019 18:09:48 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 6 lines · On Fri, 25 Jan 2019 15:01:01 +0100 Halil Pasic <[email protected]> wrote:

Quoted 3 lines · On Fri, 25 Jan 2019 13:58:35 +0100 Cornelia Huck <[email protected]> wrote:

- The code should not be interrupted while we process the channel program, do the ssch etc. We want the caller to try again later (i.e. return -EAGAIN)

(...)

Quoted 5 lines

Quoted 4 lines

- With the async interface, we want user space to be able to submit a halt/clear while a start request is still in flight, but not while we're processing a start request with translation etc. We probably want to do -EAGAIN in that case.

Quoted 7 lines

Quoted 1 line

This reads very similar to your first point.

Not quite. ssch() means that we have a cp around; for hsch()/csch() we don't have such a thing. So we want to protect the process of translating the cp etc., but we don't need such protection for the halt/clear processing.

What does this don't 'need such protection' mean in terms of code, moving the unlock of the io_mutex upward (in vfio_ccw_async_region_write())?

Here the function in question for reference:

+static ssize_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,

(quoted diff) +1 -0

+					   const char __user *buf,

size_t count,

(quoted diff) +3 -0

+					   loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) -

VFIO_CCW_NUM_REGIONS;

(quoted diff) +27 -0

+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+	int ret;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
+		return -EACCES;
+	if (!mutex_trylock(&private->io_mutex))
+		return -EAGAIN;
+
+	region = private->region[i].data;
+	if (copy_from_user((void *)region + pos, buf, count)) {
+		ret = -EFAULT;
+		goto out_unlock;
+	}
+
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_ASYNC_REQ);
+
+	ret = region->ret_code ? region->ret_code : count;
+
+out_unlock:
+	mutex_unlock(&private->io_mutex);
+	return ret;
+}

That does not make much sense to me at the moment (so I guess I misunderstood again).

Quoted 29 lines

Quoted 28 lines

Quoted 17 lines

My idea would be:

- The BUSY state denotes "I'm busy processing a request right now, try again". We hold it while processing the cp and doing the ssch and leave it afterwards (i.e., while the start request is processed by the hardware). I/O requests and async requests get -EAGAIN in that state.
- A new state (CP_PENDING?) is entered after ssch returned with cc 0 (from the BUSY state). We stay in there as long as no final state for that request has been received and delivered. (This may be final interrupt for that request, a deferred cc, or successful halt/clear.) I/O requests get -EBUSY, async requests are processed. This state can be removed again once we are able to handle more than one outstanding cp.

Quoted 2 lines

Does that make sense?

AFAIU your idea is to split up the busy state into two states: CP_PENDING and of busy without CP_PENDING called BUSY. I like the idea of having a separate state for CP_PENDING but I don't like the new semantic of BUSY.

Hm mashing a conceptual state machine and the jumptabe stuff ain't making reasoning about this simpler either. I'm taking about the conceptual state machine. It would be nice to have a picture of it and then think about how to express that in code.

Quoted 2 lines

Sorry, I'm having a hard time parsing your comments. Are you looking for something like the below?

I had more something like this
https://en.wikipedia.org/wiki/UML_state_machine,
in mind but the lists of state transitions are also useful.

Quoted 2 lines

IDLE --- IO_REQ --> BUSY ---> CP_PENDING --- IRQ ---> IDLE (if final

There ain't no trigger/action list between BUSY and CP_PENDING.

I'm also in the dark about where the issuing of the ssch() happen here (is it an internal transition within CP_PENDING?).

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

- A new state (CP_PENDING?) is entered after ssch returned with cc 0 (from the BUSY state).

the ssch() returns with non cc == 0 the CP_PENDING ---IRQ---> IDLE transition
won't take place. And I guess the IRQ is a final one.

Sorry abstraction is not a concept unknown to me. But this is too much abstraction for me in this context. The devil is in the details, and AFAIU we are discussing these details right now.

Quoted 9 lines

state for I/O)
(normal ssch)

BUSY --- IO_REQ ---> return -EAGAIN, stay in BUSY
(user space is supposed to retry, as we'll eventually progress from BUSY)

CP_PENDING --- IO_REQ ---> return -EBUSY, stay in CP_PENDING (user space is supposed to map this to the appropriate cc for the guest)

From this it seems you don't intend to issue the second requested ssch()
any more (and don't want to do any translation). Is that right? (If it is, that what I was asking for for a while, but then it's a pity for the retries.)

Quoted 3 lines

IDLE --- ASYNC_REQ ---> IDLE
(user space is welcome to do anything else right away)

Your idea is to not issue a requested hsch() if we think we are IDLE it seems. Do I understand this right? We would end up with a different semantic for hsch()/and csch() (compared to PoP) in the guest with this (AFAICT).

Quoted 1 line

Quoted 5 lines

BUSY --- ASYNC_REQ ---> return -EAGAIN, stay in BUSY (user space is supposed to retry, as above)

CP_PENDING --- ASYNC_REQ ---> return success, stay in CP_PENDING (the interrupt will get us out of CP_PENDING eventually)

Issue (c|h)sch() is an action that is done on this internal transition (within CP_PENDING).

Thank you very much for investing into this description of the state machine. I'm afraid I'm acting like a not so nice person (self censored) at the moment. I can't help myself, sorry. Maybe Farhan and Eric can take this as a starting point and come up with something that we can integrate into our documentation. Maybe not...

Regards,
Halil

Quoted 2 lines · Halil Pasic

On Mon, 28 Jan 2019 18:09:48 +0100
Cornelia Huck <[email protected]> wrote:

I guess if

Quoted 3 lines

the ssch() returns with non cc == 0 the CP_PENDING ---IRQ---> IDLE transition
won't take place. And I guess the IRQ is a final one.

Yes this is the one point I hadn't seen explicitly stated. We shouldn't remain in state=BUSY if the ssch got cc!=0, and probably return to IDLE when processing the failure. In Connie's response (Mon, 28 Jan 2019 18:24:24 +0100) to my note, she expressed some agreement to that.

Quoted 31 lines

Quoted 9 lines

state for I/O)
(normal ssch)

BUSY --- IO_REQ ---> return -EAGAIN, stay in BUSY
(user space is supposed to retry, as we'll eventually progress from BUSY)

CP_PENDING --- IO_REQ ---> return -EBUSY, stay in CP_PENDING (user space is supposed to map this to the appropriate cc for the guest)

From this it seems you don't intend to issue the second requested ssch()
any more (and don't want to do any translation). Is that right? (If it is, that what I was asking for for a while, but then it's a pity for the retries.)

Quoted 3 lines

IDLE --- ASYNC_REQ ---> IDLE
(user space is welcome to do anything else right away)

Your idea is to not issue a requested hsch() if we think we are IDLE it seems. Do I understand this right? We would end up with a different semantic for hsch()/and csch() (compared to PoP) in the guest with this (AFAICT).

Quoted 6 lines

BUSY --- ASYNC_REQ ---> return -EAGAIN, stay in BUSY (user space is supposed to retry, as above)

CP_PENDING --- ASYNC_REQ ---> return success, stay in CP_PENDING (the interrupt will get us out of CP_PENDING eventually)

Quoted 2 lines

Issue (c|h)sch() is an action that is done on this internal transition (within CP_PENDING).

These three do read like CSCH/HSCH are subject to the same rules as SSCH, when in fact they would be (among other reasons) issued to clean up a lost interrupt from a previous SSCH. So maybe return -EAGAIN on state=BUSY (don't race ourselves with the start), but issue to hardware if CP_PENDING.

If we get an async request when state=IDLE, then maybe just issue it for fun, as if it were an SSCH?

Quoted 5 lines · On Mon, 28 Jan 2019 16:48:10 -0500 Eric Farman <[email protected]> wrote:

Quoted 2 lines · Halil Pasic

On Mon, 28 Jan 2019 18:09:48 +0100
Cornelia Huck <[email protected]> wrote:

I guess if

Quoted 8 lines

Quoted 3 lines

the ssch() returns with non cc == 0 the CP_PENDING ---IRQ---> IDLE transition
won't take place. And I guess the IRQ is a final one.

Yes this is the one point I hadn't seen explicitly stated. We shouldn't remain in state=BUSY if the ssch got cc!=0, and probably return to IDLE when processing the failure. In Connie's response (Mon, 28 Jan 2019 18:24:24 +0100) to my note, she expressed some agreement to that.

Yes, I think that's what should happen.

Quoted 31 lines

Quoted 31 lines

Quoted 9 lines

state for I/O)
(normal ssch)

BUSY --- IO_REQ ---> return -EAGAIN, stay in BUSY
(user space is supposed to retry, as we'll eventually progress from BUSY)

CP_PENDING --- IO_REQ ---> return -EBUSY, stay in CP_PENDING (user space is supposed to map this to the appropriate cc for the guest)

From this it seems you don't intend to issue the second requested ssch()
any more (and don't want to do any translation). Is that right? (If it is, that what I was asking for for a while, but then it's a pity for the retries.)

Quoted 3 lines

IDLE --- ASYNC_REQ ---> IDLE
(user space is welcome to do anything else right away)

Your idea is to not issue a requested hsch() if we think we are IDLE it seems. Do I understand this right? We would end up with a different semantic for hsch()/and csch() (compared to PoP) in the guest with this (AFAICT).

Quoted 6 lines

BUSY --- ASYNC_REQ ---> return -EAGAIN, stay in BUSY (user space is supposed to retry, as above)

CP_PENDING --- ASYNC_REQ ---> return success, stay in CP_PENDING (the interrupt will get us out of CP_PENDING eventually)

Quoted 8 lines

Quoted 2 lines

Issue (c|h)sch() is an action that is done on this internal transition (within CP_PENDING).

These three do read like CSCH/HSCH are subject to the same rules as SSCH, when in fact they would be (among other reasons) issued to clean up a lost interrupt from a previous SSCH. So maybe return -EAGAIN on state=BUSY (don't race ourselves with the start), but issue to hardware if CP_PENDING.

I think there are some devices which require a certain hsch/csch sequence during device bringup, so it's not just cleaning up after a ssch.

unless things like "we're in the process of translating a cp, and can't deal with another request right now" prevent it.

Quoted 4 lines

If we get an async request when state=IDLE, then maybe just issue it for fun, as if it were an SSCH?

For fun, but mainly because the guest wants it :)

Quoted 60 lines · Cornelia Huck

Quoted 5 lines · On Mon, 28 Jan 2019 16:48:10 -0500 Eric Farman <[email protected]> wrote:

Quoted 2 lines · Halil Pasic

On Mon, 28 Jan 2019 18:09:48 +0100
Cornelia Huck <[email protected]> wrote:

I guess if

Quoted 8 lines

Quoted 3 lines

the ssch() returns with non cc == 0 the CP_PENDING ---IRQ---> IDLE transition
won't take place. And I guess the IRQ is a final one.

Yes this is the one point I hadn't seen explicitly stated. We shouldn't remain in state=BUSY if the ssch got cc!=0, and probably return to IDLE when processing the failure. In Connie's response (Mon, 28 Jan 2019 18:24:24 +0100) to my note, she expressed some agreement to that.

Yes, I think that's what should happen.

Quoted 31 lines

Quoted 31 lines

Quoted 9 lines

state for I/O)
(normal ssch)

BUSY --- IO_REQ ---> return -EAGAIN, stay in BUSY
(user space is supposed to retry, as we'll eventually progress from BUSY)

CP_PENDING --- IO_REQ ---> return -EBUSY, stay in CP_PENDING (user space is supposed to map this to the appropriate cc for the guest)

From this it seems you don't intend to issue the second requested ssch()
any more (and don't want to do any translation). Is that right? (If it is, that what I was asking for for a while, but then it's a pity for the retries.)

Quoted 3 lines

IDLE --- ASYNC_REQ ---> IDLE
(user space is welcome to do anything else right away)

Your idea is to not issue a requested hsch() if we think we are IDLE it seems. Do I understand this right? We would end up with a different semantic for hsch()/and csch() (compared to PoP) in the guest with this (AFAICT).

Quoted 6 lines

BUSY --- ASYNC_REQ ---> return -EAGAIN, stay in BUSY (user space is supposed to retry, as above)

CP_PENDING --- ASYNC_REQ ---> return success, stay in CP_PENDING (the interrupt will get us out of CP_PENDING eventually)

Quoted 8 lines

Quoted 2 lines

Issue (c|h)sch() is an action that is done on this internal transition (within CP_PENDING).

These three do read like CSCH/HSCH are subject to the same rules as SSCH, when in fact they would be (among other reasons) issued to clean up a lost interrupt from a previous SSCH. So maybe return -EAGAIN on state=BUSY (don't race ourselves with the start), but issue to hardware if CP_PENDING.

Quoted 3 lines

I think there are some devices which require a certain hsch/csch sequence during device bringup, so it's not just cleaning up after a ssch.

Ah, yes.

Therefore, we should always try to do the requested hsch/csch,

Quoted 2 lines

unless things like "we're in the process of translating a cp, and can't deal with another request right now" prevent it.

Agreed. I'm in support of all of this.

Quoted 8 lines

Quoted 5 lines

If we get an async request when state=IDLE, then maybe just issue it for fun, as if it were an SSCH?

For fun, but mainly because the guest wants it :)

Well, that too. ;-)

Quoted 17 lines · On Mon, 28 Jan 2019 18:09:48 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 6 lines · On Fri, 25 Jan 2019 15:01:01 +0100 Halil Pasic <[email protected]> wrote:

Quoted 3 lines · On Fri, 25 Jan 2019 13:58:35 +0100 Cornelia Huck <[email protected]> wrote:

- The code should not be interrupted while we process the channel program, do the ssch etc. We want the caller to try again later (i.e. return -EAGAIN)

(...)

Quoted 5 lines

Quoted 4 lines

- With the async interface, we want user space to be able to submit a halt/clear while a start request is still in flight, but not while we're processing a start request with translation etc. We probably want to do -EAGAIN in that case.

Quoted 7 lines

Quoted 1 line

This reads very similar to your first point.

Not quite. ssch() means that we have a cp around; for hsch()/csch() we don't have such a thing. So we want to protect the process of translating the cp etc., but we don't need such protection for the halt/clear processing.

What does this don't 'need such protection' mean in terms of code, moving the unlock of the io_mutex upward (in vfio_ccw_async_region_write())?

Quoted 2 lines

IDLE --- IO_REQ --> BUSY ---> CP_PENDING --- IRQ ---> IDLE (if final

There ain't no trigger/action list between BUSY and CP_PENDING. I'm also in the dark about where the issuing of the ssch() happen here (is it an internal transition within CP_PENDING?). I guess if the ssch() returns with non cc == 0 the CP_PENDING ---IRQ---> IDLE transition
won't take place. And I guess the IRQ is a final one.

Quoted 7 lines

state for I/O)
(normal ssch)

BUSY --- IO_REQ ---> return -EAGAIN, stay in BUSY
(user space is supposed to retry, as we'll eventually progress from BUSY)

Quoted 2 lines

CP_PENDING --- IO_REQ ---> return -EBUSY, stay in CP_PENDING (user space is supposed to map this to the appropriate cc for the guest)

From this it seems you don't intend to issue the second requested ssch() any more (and don't want to do any translation). Is that right? (If it is, that what I was asking for for a while, but then it's a pity for the retries.)

Quoted 4 lines

IDLE --- ASYNC_REQ ---> IDLE
(user space is welcome to do anything else right away)

Your idea is to not issue a requested hsch() if we think we are IDLE it seems. Do I understand this right? We would end up with a different semantic for hsch()/and csch() (compared to PoP) in the guest with this (AFAICT).

Quoted 7 lines

BUSY --- ASYNC_REQ ---> return -EAGAIN, stay in BUSY (user space is supposed to retry, as above)

CP_PENDING --- ASYNC_REQ ---> return success, stay in CP_PENDING (the interrupt will get us out of CP_PENDING eventually)

Issue (c|h)sch() is an action that is done on this internal transition (within CP_PENDING).

Quoted 18 lines · On Fri, 25 Jan 2019 11:24:37 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 14 lines · On Thu, 24 Jan 2019 21:37:44 -0500 Eric Farman <[email protected]> wrote:

Quoted 13 lines · Eric Farman

On 01/21/2019 06:03 AM, Cornelia Huck wrote:

[1] I think these changes are cool.  We end up going into (and staying in) state=BUSY if we get cc=0 on the SSCH, rather than in/out as we bumble along.

But why can't these be separated out from this patch?  It does change the behavior of the state machine, and seem distinct from the addition of the mutex you otherwise add here?  At the very least, this behavior change should be documented in the commit since it's otherwise lost in the mutex/EAGAIN stuff.

Quoted 1 line

That's a very good idea. I'll factor them out into a separate patch.

And now that I've factored it out, I noticed some more problems.

What we basically need is the following, I think:

- The code should not be interrupted while we process the channel program, do the ssch etc. We want the caller to try again later (i.e. return -EAGAIN)
- We currently do not want the user space to submit another channel program while the first one is still in flight.

one is a valid request, however, we should allow this in the future (once we have the code to handle that in place).
- With the async interface, we want user space to be able to submit a halt/clear while a start request is still in flight, but not while we're processing a start request with translation etc. We probably want to do -EAGAIN in that case.

My idea would be:

- The BUSY state denotes "I'm busy processing a request right now, try again". We hold it while processing the cp and doing the ssch and leave it afterwards (i.e., while the start request is processed by the hardware). I/O requests and async requests get -EAGAIN in that state.
- A new state (CP_PENDING?) is entered after ssch returned with cc 0 (from the BUSY state). We stay in there as long as no final state for that request has been received and delivered. (This may be final interrupt for that request, a deferred cc, or successful halt/clear.) I/O requests get -EBUSY

be removed again once we are able to handle more than one outstanding cp.

Does that make sense?

Quoted 21 lines · Cornelia Huck

Quoted 18 lines · On Fri, 25 Jan 2019 11:24:37 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 14 lines · On Thu, 24 Jan 2019 21:37:44 -0500 Eric Farman <[email protected]> wrote:

Quoted 13 lines · Eric Farman

On 01/21/2019 06:03 AM, Cornelia Huck wrote:

[1] I think these changes are cool.  We end up going into (and staying in) state=BUSY if we get cc=0 on the SSCH, rather than in/out as we bumble along.

But why can't these be separated out from this patch?  It does change the behavior of the state machine, and seem distinct from the addition of the mutex you otherwise add here?  At the very least, this behavior change should be documented in the commit since it's otherwise lost in the mutex/EAGAIN stuff.

Quoted 3 lines

Quoted 1 line

That's a very good idea. I'll factor them out into a separate patch.

And now that I've factored it out, I noticed some more problems.

That's good! Maybe it helps us with the circles we're on :)

Quoted 9 lines

What we basically need is the following, I think:

- The code should not be interrupted while we process the channel program, do the ssch etc. We want the caller to try again later (i.e. return -EAGAIN)
- We currently do not want the user space to submit another channel program while the first one is still in flight.

These two seem to contradict one another. I think you're saying is that we don't _want_ userspace to issue another channel program, even though its _allowed_ to as far as vfio-ccw is concerned.

As submitting another

Quoted 9 lines

one is a valid request, however, we should allow this in the future (once we have the code to handle that in place).
- With the async interface, we want user space to be able to submit a halt/clear while a start request is still in flight, but not while we're processing a start request with translation etc. We probably want to do -EAGAIN in that case.

My idea would be:

Quoted 10 lines

- The BUSY state denotes "I'm busy processing a request right now, try again". We hold it while processing the cp and doing the ssch and leave it afterwards (i.e., while the start request is processed by the hardware). I/O requests and async requests get -EAGAIN in that state.
- A new state (CP_PENDING?) is entered after ssch returned with cc 0 (from the BUSY state). We stay in there as long as no final state for that request has been received and delivered. (This may be final interrupt for that request, a deferred cc, or successful halt/clear.) I/O requests get -EBUSY

I liked CP_PENDING, since it corresponds to the subchannel being marked "start pending" as described in POPS, but this statement suggests that the BUSY/PENDING state to be swapped, such that state=PENDING returns -EAGAIN and state=BUSY returns -EBUSY. Not super-concerned with the terminology though.

, async requests are processed. This state can

Quoted 5 lines

be removed again once we are able to handle more than one outstanding cp.

Does that make sense?

I think so, and I think I like it. So you want to distinguish between (I have swapped BUSY/PENDING in this example per my above comment):

A) SSCH issued by userspace (IDLE->PENDING)
B) SSCH issued (successfully) by kernel (PENDING->BUSY) B') SSCH issued (unsuccessfully) by kernel (PENDING->IDLE?)

C) Interrupt received by kernel (no change?)
D) Interrupt given to userspace (BUSY->IDLE)

If we receive A and A, the second A gets EAGAIN

If we do A+B and A, the second A gets EBUSY (unless async, which is processed)

Does the boundary of "in flight" in the interrupt side (C and D) need to be defined, such that we go BUSY->PENDING->IDLE instead of BUSY->IDLE ?

Quoted 22 lines · On Fri, 25 Jan 2019 10:57:38 -0500 Eric Farman <[email protected]> wrote:

Quoted 21 lines · Cornelia Huck

Quoted 18 lines · On Fri, 25 Jan 2019 11:24:37 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 14 lines · On Thu, 24 Jan 2019 21:37:44 -0500 Eric Farman <[email protected]> wrote:

Quoted 13 lines · Eric Farman

On 01/21/2019 06:03 AM, Cornelia Huck wrote:

[1] I think these changes are cool.  We end up going into (and staying in) state=BUSY if we get cc=0 on the SSCH, rather than in/out as we bumble along.

But why can't these be separated out from this patch?  It does change the behavior of the state machine, and seem distinct from the addition of the mutex you otherwise add here?  At the very least, this behavior change should be documented in the commit since it's otherwise lost in the mutex/EAGAIN stuff.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

That's a very good idea. I'll factor them out into a separate patch.

And now that I've factored it out, I noticed some more problems.

That's good! Maybe it helps us with the circles we're on :)

:)

Quoted 10 lines

Quoted 9 lines

What we basically need is the following, I think:

- The code should not be interrupted while we process the channel program, do the ssch etc. We want the caller to try again later (i.e. return -EAGAIN)
- We currently do not want the user space to submit another channel program while the first one is still in flight.

Quoted 3 lines

These two seem to contradict one another. I think you're saying is that we don't _want_ userspace to issue another channel program, even though its _allowed_ to as far as vfio-ccw is concerned.

What I'm trying to say is that we want to distinguish two things:
- The code is currently doing translation etc. We probably want to keep that atomic, in order not to make things too complicated.
- We have sent the ssch() to the hardware, but have not yet received the final interrupt for that request (that's what I meant with "in flight"). It's easier for the first shot to disallow a second ssch() as that would need handling of more than one cp request, but we may want to allow it in the future.
A hsch()/csch() (which does not generate a new cp) should be fine.

(see also my reply to Halil's mail)

Quoted 22 lines

As submitting another

Quoted 19 lines

one is a valid request, however, we should allow this in the future (once we have the code to handle that in place).
- With the async interface, we want user space to be able to submit a halt/clear while a start request is still in flight, but not while we're processing a start request with translation etc. We probably want to do -EAGAIN in that case.

My idea would be:

- The BUSY state denotes "I'm busy processing a request right now, try again". We hold it while processing the cp and doing the ssch and leave it afterwards (i.e., while the start request is processed by the hardware). I/O requests and async requests get -EAGAIN in that state.
- A new state (CP_PENDING?) is entered after ssch returned with cc 0 (from the BUSY state). We stay in there as long as no final state for that request has been received and delivered. (This may be final interrupt for that request, a deferred cc, or successful halt/clear.) I/O requests get -EBUSY

Quoted 5 lines

I liked CP_PENDING, since it corresponds to the subchannel being marked "start pending" as described in POPS, but this statement suggests that the BUSY/PENDING state to be swapped, such that state=PENDING returns -EAGAIN and state=BUSY returns -EBUSY. Not super-concerned with the terminology though.

What about s/BUSY/CP_PROCESSING/ ?

Quoted 9 lines

, async requests are processed. This state can

Quoted 5 lines

be removed again once we are able to handle more than one outstanding cp.

Does that make sense?

Quoted 6 lines

I think so, and I think I like it. So you want to distinguish between (I have swapped BUSY/PENDING in this example per my above comment):

A) SSCH issued by userspace (IDLE->PENDING)
B) SSCH issued (successfully) by kernel (PENDING->BUSY) B') SSCH issued (unsuccessfully) by kernel (PENDING->IDLE?)

I think so.

Quoted 2 lines

C) Interrupt received by kernel (no change?)
D) Interrupt given to userspace (BUSY->IDLE)

Only if that is the final interrupt for that cp.

Quoted 6 lines

If we receive A and A, the second A gets EAGAIN

If we do A+B and A, the second A gets EBUSY (unless async, which is processed)

Nod.

Quoted 2 lines

Does the boundary of "in flight" in the interrupt side (C and D) need to be defined, such that we go BUSY->PENDING->IDLE instead of BUSY->IDLE ?

I don't think we can go BUSY->PENDING (in your terminology), at that would imply a retry of the ssch()?

Quoted 9 lines · On Thu, 24 Jan 2019 21:37:44 -0500 Eric Farman <[email protected]> wrote:

Quoted 8 lines · Eric Farman

On 01/21/2019 06:03 AM, Cornelia Huck wrote:

[1] I think these changes are cool.  We end up going into (and staying in) state=BUSY if we get cc=0 on the SSCH, rather than in/out as we bumble along.

Quoted 5 lines

Quoted 5 lines

But why can't these be separated out from this patch?  It does change the behavior of the state machine, and seem distinct from the addition of the mutex you otherwise add here?  At the very least, this behavior change should be documented in the commit since it's otherwise lost in the mutex/EAGAIN stuff.

That's a very good idea. I'll factor them out into a separate patch.

Quoted 59 lines

Quoted 49 lines

Quoted 36 lines

      trace_vfio_ccw_io_fctl(scsw->cmd.fctl, get_schid(private), io_region->ret_code, errstr);
  }

drivers/s390/cio/vfio_ccw_ops.c +7 -3

index f673e106c041..3fa9fc570400 100644
@@ -169,16 +169,20 @@ static ssize_t vfio_ccw_mdev_read(struct
 mdev_device *mdev,
   {
       struct vfio_ccw_private *private;
       struct ccw_io_region *region;
+    int ret;
       if (*ppos + count > sizeof(*region))
           return -EINVAL;
       private = dev_get_drvdata(mdev_parent_dev(mdev));
+    mutex_lock(&private->io_mutex);
       region = private->io_region;
       if (copy_to_user(buf, (void *)region + *ppos, count))
-        return -EFAULT;
-
-    return count;
+        ret = -EFAULT;
+    else
+        ret = count;
+    mutex_unlock(&private->io_mutex);
+    return ret;
   }
   static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct
 mdev_device *mdev,
   {
       struct vfio_ccw_private *private;
       struct ccw_io_region *region;

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct · line 204

Quoted 10 lines

drivers/s390/cio/vfio_ccw_ops.c +5 -1

+    int ret;
       if (*ppos + count > sizeof(*region))
           return -EINVAL;
       private = dev_get_drvdata(mdev_parent_dev(mdev));
-    if (private->state != VFIO_CCW_STATE_IDLE)
+    if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+        private->state == VFIO_CCW_STATE_STANDBY)
           return -EACCES;
+    if (!mutex_trylock(&private->io_mutex))
+        return -EAGAIN;

Ah, I see Halil's difficulty here.

Quoted 9 lines

It is true there is a race condition today, and that this doesn't address it.  That's fine, add it to the todo list.  But even with that, I don't see what the mutex is enforcing?  Two simultaneous SSCHs will be serialized (one will get kicked out with a failed trylock() call), while still leaving the window open between cc=0 on the SSCH and the subsequent interrupt.  In the latter case, a second SSCH will come through here, do the copy_from_user below, and then jump to fsm_io_busy to return EAGAIN.  Do we really want to stomp on io_region in that case?
 Why can't we simply return EAGAIN if state==BUSY?

Quoted 7 lines

(Answering my own questions as I skim patch 5...)

Because of course this series is for async handling, while I was looking specifically at the synchronous code that exists today. I guess then my question just remains on how the mutex is adding protection in the sync case, because that's still not apparent to me. (Perhaps I missed it in a reply to Halil; if so I apologize, there were a lot when I returned.)

My idea behind the mutex was to make sure that we get consistent data when reading/writing (e.g. if one user space thread is reading the I/O region while another is writing to it).

If we come into mdev_write with state=BUSY and we get the lock, copy_from_user, and do our jump table we go to fsm_io_busy to set ret_code and return -EAGAIN. Why then don't we set the jump table for state=NOT_OPER||STANDBY to do something that will return -EACCES instead of how we currently do a direct return of -EACCES before all the lock/copy stuff (and the jump table that would take us to fsm_io_error and an error message before returning -EIO)?

Quoted 18 lines

Quoted 5 lines

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

@@ -188,25 +192,30 @@ static ssize_t vfio_ccw_mdev_write(struct 
 mdev_device *mdev,
   {
       struct vfio_ccw_private *private;
       struct ccw_io_region *region;

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct · line 204

Quoted 10 lines

drivers/s390/cio/vfio_ccw_ops.c +5 -1

+    int ret;
       if (*ppos + count > sizeof(*region))
           return -EINVAL;
       private = dev_get_drvdata(mdev_parent_dev(mdev));
-    if (private->state != VFIO_CCW_STATE_IDLE)
+    if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+        private->state == VFIO_CCW_STATE_STANDBY)
           return -EACCES;
+    if (!mutex_trylock(&private->io_mutex))
+        return -EAGAIN;  

Ah, I see Halil's difficulty here.

Quoted 9 lines

It is true there is a race condition today, and that this doesn't address it.  That's fine, add it to the todo list.  But even with that, I don't see what the mutex is enforcing?  Two simultaneous SSCHs will be serialized (one will get kicked out with a failed trylock() call), while still leaving the window open between cc=0 on the SSCH and the subsequent interrupt.  In the latter case, a second SSCH will come through here, do the copy_from_user below, and then jump to fsm_io_busy to return EAGAIN.  Do we really want to stomp on io_region in that case?
 Why can't we simply return EAGAIN if state==BUSY?

(Answering my own questions as I skim patch 5...)

Because of course this series is for async handling, while I was looking specifically at the synchronous code that exists today. I guess then my question just remains on how the mutex is adding protection in the sync case, because that's still not apparent to me. (Perhaps I missed it in a reply to Halil; if so I apologize, there were a lot when I returned.)

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, · line 205

Quoted 7 lines

  	private = dev_get_drvdata(mdev_parent_dev(mdev));

drivers/s390/cio/vfio_ccw_ops.c +4 -1

-	if (private->state != VFIO_CCW_STATE_IDLE)
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
  		return -EACCES;
+	if (!mutex_trylock(&private->io_mutex))
+		return -EAGAIN;  

Ah, I see Halil's difficulty here.

It is true there is a race condition today, and that this doesn't address it. That's fine, add it to the todo list. But even with that, I don't see what the mutex is enforcing?

Two simultaneous SSCHs will be
serialized (one will get kicked out with a failed trylock() call), while still leaving the window open between cc=0 on the SSCH and the subsequent interrupt. In the latter case, a second SSCH will come through here, do the copy_from_user below, and then jump to fsm_io_busy to return EAGAIN. Do we really want to stomp on io_region in that case?

Why can't we simply return EAGAIN if state==BUSY?

Quoted 8 lines

  
  	region = private->io_region;

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

-	if (copy_from_user((void *)region + *ppos, buf, count))
-		return -EFAULT;
+	if (copy_from_user((void *)region + *ppos, buf, count)) {
+		ret = -EFAULT;
+		goto out_unlock;
+	}

Quoted 10 lines · On Thu, 24 Jan 2019 21:25:10 -0500 Eric Farman <[email protected]> wrote:

drivers/s390/cio/vfio_ccw_ops.c static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev, · line 205

Quoted 7 lines

   	private = dev_get_drvdata(mdev_parent_dev(mdev));

drivers/s390/cio/vfio_ccw_ops.c +4 -1

-	if (private->state != VFIO_CCW_STATE_IDLE)
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
   		return -EACCES;
+	if (!mutex_trylock(&private->io_mutex))
+		return -EAGAIN;

Ah, I see Halil's difficulty here.

Quoted 3 lines

It is true there is a race condition today, and that this doesn't address it. That's fine, add it to the todo list. But even with that, I don't see what the mutex is enforcing?

It is protecting the io regions. AFAIU the idea was that only one thread is accessing the io region(s) at a time to prevent corruption and reading half-morphed data.

Quoted 6 lines

Two simultaneous SSCHs will be
serialized (one will get kicked out with a failed trylock() call), while still leaving the window open between cc=0 on the SSCH and the subsequent interrupt. In the latter case, a second SSCH will come through here, do the copy_from_user below, and then jump to fsm_io_busy to return EAGAIN. Do we really want to stomp on io_region in that case?

I'm not sure I understood you correctly. The interrupt handler does not take the lock before writing to the io_region. That is one race but it is easy to fix.

The bigger problem is that between the interrupt handler has written IRB area and userspace has read it we may end up destroying it by stomping on it (to use your words). The userspace reading a wrong (given todays qemu zeroed out) IRB could lead to follow on problems.

Quoted 3 lines

Why can't we simply return EAGAIN if state==BUSY?

Sure we can. That would essentially go back to the old way of things:
if not idle return with error.

form EACCESS to EAGAIN. Which Isn't necessarily a win, because conceptually here should be never two interleaved io_requests/start commands hitting the module.

Quoted 8 lines

Quoted 8 lines

   
   	region = private->io_region;

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

-	if (copy_from_user((void *)region + *ppos, buf, count))
-		return -EFAULT;
+	if (copy_from_user((void *)region + *ppos, buf, count)) {
+		ret = -EFAULT;
+		goto out_unlock;
+	}

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

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

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

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

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

drivers/s390/cio/vfio_ccw_ops.c +155 -26

index 3fa9fc570400..5a89d09f9271 100644
@@ -3,9 +3,11 @@
   * Physical device callbacks for vfio_ccw
   *
   * Copyright IBM Corp. 2017
+ * Copyright Red Hat, Inc. 2019
   *
   * Author(s): Dong Jia Shi <[email protected]>
   *            Xiao Feng Ren <[email protected]>
+ *            Cornelia Huck <[email protected]>
   */
 
  #include <linux/vfio.h>

@@ -157,27 +159,33 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
  {
  	struct vfio_ccw_private *private =
  		dev_get_drvdata(mdev_parent_dev(mdev));
+	int i;
 
  	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(struct mdev_device *mdev,
-				  char __user *buf,
-				  size_t count,
-				  loff_t *ppos)
+static ssize_t vfio_ccw_mdev_read_io_region(struct vfio_ccw_private *private,
+					    char __user *buf, size_t count,
+					    loff_t *ppos)
  {
-	struct vfio_ccw_private *private;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
  	struct ccw_io_region *region;
  	int ret;
 
-	if (*ppos + count > sizeof(*region))
+	if (pos + count > sizeof(*region))
  		return -EINVAL;
 
-	private = dev_get_drvdata(mdev_parent_dev(mdev));
  	mutex_lock(&private->io_mutex);
  	region = private->io_region;
-	if (copy_to_user(buf, (void *)region + *ppos, count))
+	if (copy_to_user(buf, (void *)region + pos, count))
  		ret = -EFAULT;
  	else
  		ret = count;

@@ -185,19 +193,42 @@ static ssize_t vfio_ccw_mdev_read(struct mdev_device *mdev,
  	return ret;
  }
 
-static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
-				   const char __user *buf,
-				   size_t count,
-				   loff_t *ppos)
+static ssize_t vfio_ccw_mdev_read(struct mdev_device *mdev,
+				  char __user *buf,
+				  size_t count,
+				  loff_t *ppos)
  {
+	unsigned int index = VFIO_CCW_OFFSET_TO_INDEX(*ppos);
  	struct vfio_ccw_private *private;
+
+	private = dev_get_drvdata(mdev_parent_dev(mdev));
+
+	if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions)
+		return -EINVAL;
+
+	switch (index) {
+	case VFIO_CCW_CONFIG_REGION_INDEX:
+		return vfio_ccw_mdev_read_io_region(private, buf, count, ppos);
+	default:
+		index -= VFIO_CCW_NUM_REGIONS;
+		return private->region[index].ops->read(private, buf, count,
+							ppos);
+	}
+
+	return -EINVAL;
+}
+
+static ssize_t vfio_ccw_mdev_write_io_region(struct vfio_ccw_private *private,
+					     const char __user *buf,
+					     size_t count, loff_t *ppos)
+{
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
  	struct ccw_io_region *region;
  	int ret;
 
-	if (*ppos + count > sizeof(*region))
+	if (pos + count > sizeof(*region))
  		return -EINVAL;
 
-	private = dev_get_drvdata(mdev_parent_dev(mdev));
  	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
  	    private->state == VFIO_CCW_STATE_STANDBY)
  		return -EACCES;

@@ -205,7 +236,7 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
  		return -EAGAIN;
 
  	region = private->io_region;
-	if (copy_from_user((void *)region + *ppos, buf, count)) {
+	if (copy_from_user((void *)region + pos, buf, count)) {
  		ret = -EFAULT;
  		goto out_unlock;
  	}

@@ -218,19 +249,52 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
  	return ret;
  }
 
-static int vfio_ccw_mdev_get_device_info(struct vfio_device_info *info)
+static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
+				   const char __user *buf,
+				   size_t count,
+				   loff_t *ppos)
+{
+	unsigned int index = VFIO_CCW_OFFSET_TO_INDEX(*ppos);
+	struct vfio_ccw_private *private;
+
+	private = dev_get_drvdata(mdev_parent_dev(mdev));
+
+	if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions)
+		return -EINVAL;
+
+	switch (index) {
+	case VFIO_CCW_CONFIG_REGION_INDEX:
+		return vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
+	default:
+		index -= VFIO_CCW_NUM_REGIONS;
+		return private->region[index].ops->write(private, buf, count,
+							 ppos);
+	}
+
+	return -EINVAL;
+}
+
+static int vfio_ccw_mdev_get_device_info(struct vfio_device_info *info,
+					 struct mdev_device *mdev)
  {
+	struct vfio_ccw_private *private;
+
+	private = dev_get_drvdata(mdev_parent_dev(mdev));
  	info->flags = VFIO_DEVICE_FLAGS_CCW | VFIO_DEVICE_FLAGS_RESET;
-	info->num_regions = VFIO_CCW_NUM_REGIONS;
+	info->num_regions = VFIO_CCW_NUM_REGIONS + private->num_regions;
  	info->num_irqs = VFIO_CCW_NUM_IRQS;
 
  	return 0;
  }
 
  static int vfio_ccw_mdev_get_region_info(struct vfio_region_info *info,
-					 u16 *cap_type_id,
-					 void **cap_type)
+					 struct mdev_device *mdev,
+					 unsigned long arg)
  {
+	struct vfio_ccw_private *private;
+	int i;
+
+	private = dev_get_drvdata(mdev_parent_dev(mdev));
  	switch (info->index) {
  	case VFIO_CCW_CONFIG_REGION_INDEX:
  		info->offset = 0;

@@ -238,9 +302,51 @@ static int vfio_ccw_mdev_get_region_info(struct vfio_region_info *info,
  		info->flags = VFIO_REGION_INFO_FLAG_READ
  			      | VFIO_REGION_INFO_FLAG_WRITE;
  		return 0;
-	default:
-		return -EINVAL;
+	default: /* all other regions are handled via capability chain */
+	{
+		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
+		struct vfio_region_info_cap_type cap_type = {
+			.header.id = VFIO_REGION_INFO_CAP_TYPE,
+			.header.version = 1 };
+		int ret;
+
+		if (info->index >=
+		    VFIO_CCW_NUM_REGIONS + private->num_regions)
+			return -EINVAL;
+
+		i = info->index - VFIO_CCW_NUM_REGIONS;
+
+		info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index);
+		info->size = private->region[i].size;
+		info->flags = private->region[i].flags;
+
+		cap_type.type = private->region[i].type;
+		cap_type.subtype = private->region[i].subtype;
+
+		ret = vfio_info_add_capability(&caps, &cap_type.header,
+					       sizeof(cap_type));
+		if (ret)
+			return ret;
+
+		info->flags |= VFIO_REGION_INFO_FLAG_CAPS;
+		if (info->argsz < sizeof(*info) + caps.size) {
+			info->argsz = sizeof(*info) + caps.size;
+			info->cap_offset = 0;
+		} else {
+			vfio_info_cap_shift(&caps, sizeof(*info));
+			if (copy_to_user((void __user *)arg + sizeof(*info),
+					 caps.buf, caps.size)) {
+				kfree(caps.buf);
+				return -EFAULT;
+			}
+			info->cap_offset = sizeof(*info);
+		}
+
+		kfree(caps.buf);
+
+	}
  	}
+	return 0;
  }
 
  static int vfio_ccw_mdev_get_irq_info(struct vfio_irq_info *info)

@@ -317,6 +423,32 @@ static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
  	}
  }
 
+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)
+{
+	struct vfio_ccw_region *region;
+
+	region = krealloc(private->region,
+			  (private->num_regions + 1) * sizeof(*region),
+			  GFP_KERNEL);
+	if (!region)
+		return -ENOMEM;
+
+	private->region = region;
+	private->region[private->num_regions].type = VFIO_REGION_TYPE_CCW;
+	private->region[private->num_regions].subtype = subtype;
+	private->region[private->num_regions].ops = ops;
+	private->region[private->num_regions].size = size;
+	private->region[private->num_regions].flags = flags;
+	private->region[private->num_regions].data = data;
+
+	private->num_regions++;
+
+	return 0;
+}
+
  static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
  				   unsigned int cmd,
  				   unsigned long arg)

@@ -337,7 +469,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
  		if (info.argsz < minsz)
  			return -EINVAL;
 
-		ret = vfio_ccw_mdev_get_device_info(&info);
+		ret = vfio_ccw_mdev_get_device_info(&info, mdev);
  		if (ret)
  			return ret;
 

@@ -346,8 +478,6 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
  	case VFIO_DEVICE_GET_REGION_INFO:
  	{
  		struct vfio_region_info info;
-		u16 cap_type_id = 0;
-		void *cap_type = NULL;
 
  		minsz = offsetofend(struct vfio_region_info, offset);
 

@@ -357,8 +487,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
  		if (info.argsz < minsz)
  			return -EINVAL;
 
-		ret = vfio_ccw_mdev_get_region_info(&info, &cap_type_id,
-						    &cap_type);
+		ret = vfio_ccw_mdev_get_region_info(&info, mdev, arg);
  		if (ret)
  			return ret;
 

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

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

@@ -19,6 +21,38 @@
  #include "css.h"
  #include "vfio_ccw_cp.h"
 
+#define VFIO_CCW_OFFSET_SHIFT   40
+#define VFIO_CCW_OFFSET_TO_INDEX(off)	(off >> VFIO_CCW_OFFSET_SHIFT)
+#define VFIO_CCW_INDEX_TO_OFFSET(index)	((u64)(index) << VFIO_CCW_OFFSET_SHIFT)
+#define VFIO_CCW_OFFSET_MASK	(((u64)(1) << VFIO_CCW_OFFSET_SHIFT) - 1)
+
+/* capability chain handling similar to vfio-pci */
+struct vfio_ccw_private;
+struct vfio_ccw_region;
+
+struct vfio_ccw_regops {
+	size_t	(*read)(struct vfio_ccw_private *private, char __user *buf,
+			size_t count, loff_t *ppos);
+	size_t	(*write)(struct vfio_ccw_private *private,
+			 const char __user *buf, size_t count, loff_t *ppos);
+	void	(*release)(struct vfio_ccw_private *private,
+			   struct vfio_ccw_region *region);
+};
+
+struct vfio_ccw_region {
+	u32				type;
+	u32				subtype;
+	const struct vfio_ccw_regops	*ops;
+	void				*data;
+	size_t				size;
+	u32				flags;
+};
+
+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);
+
  /**
   * struct vfio_ccw_private
   * @sch: pointer to the subchannel

@@ -29,6 +63,8 @@
   * @nb: notifier for vfio events
   * @io_region: MMIO region to input/output I/O arguments/results
   * @io_mutex: protect against concurrent update of I/O structures
+ * @region: additional regions for other subchannel operations
+ * @num_regions: number of additional regions
   * @cp: channel program for the current I/O operation
   * @irb: irb info received from interrupt
   * @scsw: scsw info

@@ -44,6 +80,8 @@ struct vfio_ccw_private {
  	struct notifier_block	nb;
  	struct ccw_io_region	*io_region;
  	struct mutex		io_mutex;
+	struct vfio_ccw_region *region;
+	int num_regions;
 
  	struct channel_program	cp;
  	struct irb		irb;

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

index 02bb7ad6e986..56e2413d3e00 100644