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.

Quoted 4 lines · On Wed, 30 Jan 2019 14:22:07 +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.

I read this like it is supposed to be safe regardless of parallelism and threads. However I don't see any explicit synchronization done for cp->initialized.

I've managed to figure out how is that supposed to be safe for the cp_free() (which is probably our main concern) in vfio_ccw_sch_io_todo(), but if fail when it comes to the one in vfio_ccw_mdev_notifier().

Can you explain us how does the synchronization work?

Quoted 12 lines · On Wed, 30 Jan 2019 19:51:27 +0100 Halil Pasic <[email protected]> wrote:

Quoted 4 lines · On Wed, 30 Jan 2019 14:22:07 +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.

I read this like it is supposed to be safe regardless of parallelism and threads. However I don't see any explicit synchronization done for cp->initialized.

Quoted 6 lines

I've managed to figure out how is that supposed to be safe for the cp_free() (which is probably our main concern) in vfio_ccw_sch_io_todo(), but if fail when it comes to the one in vfio_ccw_mdev_notifier().

Can you explain us how does the synchronization work?

You read that wrong, I don't add synchronization, I just add a check.

Quoted 20 lines · On Thu, 31 Jan 2019 12:52:20 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 17 lines · On Wed, 30 Jan 2019 19:51:27 +0100 Halil Pasic <[email protected]> wrote:

Quoted 4 lines · On Wed, 30 Jan 2019 14:22:07 +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.

I read this like it is supposed to be safe regardless of parallelism and threads. However I don't see any explicit synchronization done for cp->initialized.

I've managed to figure out how is that supposed to be safe for the cp_free() (which is probably our main concern) in vfio_ccw_sch_io_todo(), but if fail when it comes to the one in vfio_ccw_mdev_notifier().

Quoted 4 lines

Quoted 1 line

Can you explain us how does the synchronization work?

You read that wrong, I don't add synchronization, I just add a check.

Now I'm confused. Does that mean we don't need synchronization for this?

Quoted 28 lines · On Thu, 31 Jan 2019 13:34:55 +0100 Halil Pasic <[email protected]> wrote:

Quoted 24 lines · On Thu, 31 Jan 2019 12:52:20 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 12 lines · On Wed, 30 Jan 2019 19:51:27 +0100 Halil Pasic <[email protected]> wrote:

Quoted 4 lines · On Wed, 30 Jan 2019 14:22:07 +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.

I read this like it is supposed to be safe regardless of parallelism and threads. However I don't see any explicit synchronization done for cp->initialized.

Quoted 6 lines

I've managed to figure out how is that supposed to be safe for the cp_free() (which is probably our main concern) in vfio_ccw_sch_io_todo(), but if fail when it comes to the one in vfio_ccw_mdev_notifier().

Can you explain us how does the synchronization work?

You read that wrong, I don't add synchronization, I just add a check.

Quoted 1 line

Now I'm confused. Does that mean we don't need synchronization for this?

If we lack synchronization (that is not provided by the current state machine handling, or the rework here), we should do a patch on top (preferably on top of the whole series, so this does not get even more tangled up.) This is really just about the extra check.

Quoted 31 lines · On Mon, 4 Feb 2019 16:31:02 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 28 lines · On Thu, 31 Jan 2019 13:34:55 +0100 Halil Pasic <[email protected]> wrote:

Quoted 24 lines · On Thu, 31 Jan 2019 12:52:20 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 12 lines · On Wed, 30 Jan 2019 19:51:27 +0100 Halil Pasic <[email protected]> wrote:

Quoted 4 lines · On Wed, 30 Jan 2019 14:22:07 +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.

I read this like it is supposed to be safe regardless of parallelism and threads. However I don't see any explicit synchronization done for cp->initialized.

Quoted 6 lines

I've managed to figure out how is that supposed to be safe for the cp_free() (which is probably our main concern) in vfio_ccw_sch_io_todo(), but if fail when it comes to the one in vfio_ccw_mdev_notifier().

Can you explain us how does the synchronization work?

You read that wrong, I don't add synchronization, I just add a check.

Quoted 7 lines

Quoted 1 line

Now I'm confused. Does that mean we don't need synchronization for this?

If we lack synchronization (that is not provided by the current state machine handling, or the rework here), we should do a patch on top (preferably on top of the whole series, so this does not get even more tangled up.) This is really just about the extra check.

I'm not a huge fan of keeping or introducing races -- it makes things difficult to reason about, but I do have some understanging your position.

This patch-series is AFAICT a big improvement over what we have. I would like Farhan confirming that it makes these hick-ups when he used to hit BUSY with another ssch request disappear. If it does (I hope it does) it's definitely a good thing for anybody who wants to use vfio-ccw.

Yet I find it difficult to slap my r-b over racy code, or partial solutions. In the latter case, when I lack conceptual clarity, I find it difficult to tell if we are heading into the right direction, or is what we build today going to turn against us tomorrow. Sorry for being a drag.

Quoted 42 lines · On Tue, 5 Feb 2019 12:52:29 +0100 Halil Pasic <[email protected]> wrote:

Quoted 31 lines · On Mon, 4 Feb 2019 16:31:02 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 28 lines · On Thu, 31 Jan 2019 13:34:55 +0100 Halil Pasic <[email protected]> wrote:

Quoted 24 lines · On Thu, 31 Jan 2019 12:52:20 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 12 lines · On Wed, 30 Jan 2019 19:51:27 +0100 Halil Pasic <[email protected]> wrote:

Quoted 4 lines · On Wed, 30 Jan 2019 14:22:07 +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.

I read this like it is supposed to be safe regardless of parallelism and threads. However I don't see any explicit synchronization done for cp->initialized.

Quoted 6 lines

I've managed to figure out how is that supposed to be safe for the cp_free() (which is probably our main concern) in vfio_ccw_sch_io_todo(), but if fail when it comes to the one in vfio_ccw_mdev_notifier().

Can you explain us how does the synchronization work?

You read that wrong, I don't add synchronization, I just add a check.

Quoted 7 lines

Quoted 1 line

Now I'm confused. Does that mean we don't need synchronization for this?

If we lack synchronization (that is not provided by the current state machine handling, or the rework here), we should do a patch on top (preferably on top of the whole series, so this does not get even more tangled up.) This is really just about the extra check.

Quoted 3 lines

I'm not a huge fan of keeping or introducing races -- it makes things difficult to reason about, but I do have some understanging your position.

The only thing I want to avoid is knowingly making things worse than before, and I don't think this patch does that.

Quoted 1 line

Quoted 4 lines

This patch-series is AFAICT a big improvement over what we have. I would like Farhan confirming that it makes these hick-ups when he used to hit BUSY with another ssch request disappear. If it does (I hope it does) it's definitely a good thing for anybody who wants to use vfio-ccw.

Yep. There remains a lot to be done, but it's a first step.

Quoted 6 lines

Yet I find it difficult to slap my r-b over racy code, or partial solutions. In the latter case, when I lack conceptual clarity, I find it difficult to tell if we are heading into the right direction, or is what we build today going to turn against us tomorrow. Sorry for being a drag.

As long as we don't introduce bad user space interfaces we have to drag around forever, I think anything is fair game if we think it's a good idea at that moment. We can rewrite things if it turned out to be a bad idea (although I'm not arguing for doing random crap, of course :)

Quoted 52 lines · Cornelia Huck

Quoted 31 lines · On Tue, 5 Feb 2019 12:52:29 +0100 Halil Pasic <[email protected]> wrote:

Quoted 28 lines · On Mon, 4 Feb 2019 16:31:02 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 25 lines · On Thu, 31 Jan 2019 13:34:55 +0100 Halil Pasic <[email protected]> wrote:

Quoted 13 lines · On Thu, 31 Jan 2019 12:52:20 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 11 lines · On Wed, 30 Jan 2019 19:51:27 +0100 Halil Pasic <[email protected]> wrote:

Quoted 4 lines · On Wed, 30 Jan 2019 14:22:07 +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.

I read this like it is supposed to be safe regardless of parallelism and threads. However I don't see any explicit synchronization done for cp->initialized.

Quoted 8 lines

Quoted 6 lines

I've managed to figure out how is that supposed to be safe for the cp_free() (which is probably our main concern) in vfio_ccw_sch_io_todo(), but if fail when it comes to the one in vfio_ccw_mdev_notifier().

Can you explain us how does the synchronization work?

You read that wrong, I don't add synchronization, I just add a check.

Now I'm confused. Does that mean we don't need synchronization for this?

Quoted 8 lines

Quoted 4 lines

If we lack synchronization (that is not provided by the current state machine handling, or the rework here), we should do a patch on top (preferably on top of the whole series, so this does not get even more tangled up.) This is really just about the extra check.

I'm not a huge fan of keeping or introducing races -- it makes things difficult to reason about, but I do have some understanging your position.

The only thing I want to avoid is knowingly making things worse than before, and I don't think this patch does that.

Quoted 5 lines

This patch-series is AFAICT a big improvement over what we have. I would like Farhan confirming that it makes these hick-ups when he used to hit BUSY with another ssch request disappear. If it does (I hope it does) it's definitely a good thing for anybody who wants to use vfio-ccw.

Quoted 1 line

Yep. There remains a lot to be done, but it's a first step.

s/a first step/an excellent first step/  :)

Can't speak for Farhan, but this makes things somewhat better for me. I'm still getting some periodic errors, but they happen infrequently enough now that debugging them is frustrating.  ;-)

 - Eric

Quoted 12 lines

Quoted 6 lines

Yet I find it difficult to slap my r-b over racy code, or partial solutions. In the latter case, when I lack conceptual clarity, I find it difficult to tell if we are heading into the right direction, or is what we build today going to turn against us tomorrow. Sorry for being a drag.

As long as we don't introduce bad user space interfaces we have to drag around forever, I think anything is fair game if we think it's a good idea at that moment. We can rewrite things if it turned out to be a bad idea (although I'm not arguing for doing random crap, of course :)

Quoted 13 lines · Eric Farman

Quoted 6 lines

Quoted 4 lines

This patch-series is AFAICT a big improvement over what we have. I would like Farhan confirming that it makes these hick-ups when he used to hit BUSY with another ssch request disappear. If it does (I hope it does) it's definitely a good thing for anybody who wants to use vfio-ccw.

Yep. There remains a lot to be done, but it's a first step.

s/a first step/an excellent first step/  :)

Can't speak for Farhan, but this makes things somewhat better for me. I'm still getting some periodic errors, but they happen infrequently enough now that debugging them is frustrating.  ;-)

Quoted 2 lines

 - Eric

I ran the my workloads/tests with the patches and like Eric I notice the errors I previously hit less frequently.

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.

While at it, also make sure that functions called from other parts of the code return gracefully if the channel program structure has not been initialized (even though that is a bug in the caller).

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

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

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

index ba08fe137c2e..0bc0c38edda7 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,

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

  	return ret;
  }
 

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

@@ -760,6 +764,10 @@ int cp_prefetch(struct channel_program *cp)
  	struct ccwchain *chain;
  	int len, idx, ret;
 
+	/* this is an error in the caller */
+	if (!cp || !cp->initialized)
+		return -EINVAL;
+
  	list_for_each_entry(chain, &cp->ccwchain_list, next) {
  		len = chain->ch_len;
  		for (idx = 0; idx < len; idx++) {

@@ -795,6 +803,10 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
  	struct ccwchain *chain;
  	struct ccw1 *cpa;
 
+	/* this is an error in the caller */
+	if (!cp || !cp->initialized)
+		return NULL;
+
  	orb = &cp->orb;
 
  	orb->cmd.intparm = intparm;

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

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

+		return false;
+
  	list_for_each_entry(chain, &cp->ccwchain_list, next) {
  		for (i = 0; i < chain->ch_len; i++)
  			if (pfn_array_table_iova_pinned(chain->ch_pat + i,

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

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

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

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

index cab17865aafe..e7c9877c9f1e 100644
@@ -31,6 +31,10 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
  	private->state = VFIO_CCW_STATE_BUSY;
 
  	orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
+	if (!orb) {
+		ret = -EIO;
+		goto out;
+	}
 
  	/* Issue "Start Subchannel" */
  	ccode = ssch(sch->schid, orb);

@@ -64,6 +68,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
  	default:
  		ret = ccode;
  	}
+out:
  	spin_unlock_irqrestore(sch->lock, flags);
  	return ret;
  }

Quoted 32 lines · Cornelia Huck

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.

While at it, also make sure that functions called from other parts of the code return gracefully if the channel program structure has not been initialized (even though that is a bug in the caller).

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

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

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

index ba08fe137c2e..0bc0c38edda7 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;
 

drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) · line 706

Quoted 2 lines

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

+	cp->initialized = true;
+  

Not seen in this hunk, but we call ccwchain_loop_tic() just prior to this point. If that returns non-zero, we call cp_unpin_free()[1] (and set initailized to false), and then fall through to here. So this is going to set initialized to true, even though we're taking an error path. :-(

[1] Wait, why is it calling cp_unpin_free()? Oh, I had proposed squashing cp_free() and cp_unpin_free() back in November[2], got an r-b from Pierre but haven't gotten back to tidy up the series for a v2. Okay, I'll try to do that again soon. :-)

[2] https://patchwork.kernel.org/patch/10675261/

Quoted 49 lines

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

  	return ret;
  }
 

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

@@ -760,6 +764,10 @@ int cp_prefetch(struct channel_program *cp)
  	struct ccwchain *chain;
  	int len, idx, ret;
 
+	/* this is an error in the caller */
+	if (!cp || !cp->initialized)
+		return -EINVAL;
+
  	list_for_each_entry(chain, &cp->ccwchain_list, next) {
  		len = chain->ch_len;
  		for (idx = 0; idx < len; idx++) {

@@ -795,6 +803,10 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
  	struct ccwchain *chain;
  	struct ccw1 *cpa;
 
+	/* this is an error in the caller */
+	if (!cp || !cp->initialized)
+		return NULL;
+
  	orb = &cp->orb;
 
  	orb->cmd.intparm = intparm;

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

@@ -869,6 +884,9 @@ bool cp_iova_pinned(struct channel_program *cp, u64 iova)
  	struct ccwchain *chain;
  	int i;
 

drivers/s390/cio/vfio_ccw_cp.c bool cp_iova_pinned(struct channel_program *cp, u64 iova) · line 887

Quoted 1 line

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

+	if (!cp->initialized)  

So, two of the checks added above look for a nonzero cp pointer prior to checking initialized, while two don't. I guess cp can't be NULL, since it's embedded in the private struct directly and that's only free'd when we do vfio_ccw_sch_remove() ... But I guess some consistency in how we look would be nice.

Quoted 6 lines

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

+		return false;
+
  	list_for_each_entry(chain, &cp->ccwchain_list, next) {
  		for (i = 0; i < chain->ch_len; i++)
  			if (pfn_array_table_iova_pinned(chain->ch_pat + i,

Quoted 33 lines · On Mon, 4 Feb 2019 14:25:34 -0500 Eric Farman <[email protected]> wrote:

Quoted 32 lines · Cornelia Huck

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.

While at it, also make sure that functions called from other parts of the code return gracefully if the channel program structure has not been initialized (even though that is a bug in the caller).

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

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

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

index ba08fe137c2e..0bc0c38edda7 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;
 

Quoted 8 lines

drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) · line 706

Quoted 2 lines

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

+	cp->initialized = true;
+

Not seen in this hunk, but we call ccwchain_loop_tic() just prior to this point. If that returns non-zero, we call cp_unpin_free()[1] (and set initailized to false), and then fall through to here. So this is going to set initialized to true, even though we're taking an error path. :-(

Eek, setting c64 unconditionally threw me off. This needs to check for !ret, of course.

Quoted 5 lines

[1] Wait, why is it calling cp_unpin_free()? Oh, I had proposed squashing cp_free() and cp_unpin_free() back in November[2], got an r-b from Pierre but haven't gotten back to tidy up the series for a v2. Okay, I'll try to do that again soon. :-)

:)

Quoted 51 lines

[2] https://patchwork.kernel.org/patch/10675261/

Quoted 49 lines

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

   	return ret;
   }
 

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

@@ -760,6 +764,10 @@ int cp_prefetch(struct channel_program *cp)
   	struct ccwchain *chain;
   	int len, idx, ret;
 
+	/* this is an error in the caller */
+	if (!cp || !cp->initialized)
+		return -EINVAL;
+
   	list_for_each_entry(chain, &cp->ccwchain_list, next) {
   		len = chain->ch_len;
   		for (idx = 0; idx < len; idx++) {

@@ -795,6 +803,10 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
   	struct ccwchain *chain;
   	struct ccw1 *cpa;
 
+	/* this is an error in the caller */
+	if (!cp || !cp->initialized)
+		return NULL;
+
   	orb = &cp->orb;
 
   	orb->cmd.intparm = intparm;

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

@@ -869,6 +884,9 @@ bool cp_iova_pinned(struct channel_program *cp, u64 iova)
   	struct ccwchain *chain;
   	int i;
 

Quoted 7 lines

drivers/s390/cio/vfio_ccw_cp.c bool cp_iova_pinned(struct channel_program *cp, u64 iova) · line 887

Quoted 1 line

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

+	if (!cp->initialized)

So, two of the checks added above look for a nonzero cp pointer prior to checking initialized, while two don't. I guess cp can't be NULL, since it's embedded in the private struct directly and that's only free'd when we do vfio_ccw_sch_remove() ... But I guess some consistency in how we look would be nice.

The idea was: In which context is this called? Is there a legitimate reason for the caller to pass in an uninitialized cp, or would that mean the caller had messed up (and we should not trust cp to be !NULL either?)

But you're right, that does look inconsistent. Always checking for cp != NULL probably looks least odd, although it is overkill. Opinions?

Quoted 7 lines

Quoted 7 lines

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

+		return false;
+
   	list_for_each_entry(chain, &cp->ccwchain_list, next) {
   		for (i = 0; i < chain->ch_len; i++)
   			if (pfn_array_table_iova_pinned(chain->ch_pat + i,

Quoted 54 lines · Cornelia Huck

Quoted 38 lines · On Mon, 4 Feb 2019 14:25:34 -0500 Eric Farman <[email protected]> wrote:

Quoted 36 lines · Cornelia Huck

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

@@ -760,6 +764,10 @@ int cp_prefetch(struct channel_program *cp)
   	struct ccwchain *chain;
   	int len, idx, ret;
 
+	/* this is an error in the caller */
+	if (!cp || !cp->initialized)
+		return -EINVAL;
+
   	list_for_each_entry(chain, &cp->ccwchain_list, next) {
   		len = chain->ch_len;
   		for (idx = 0; idx < len; idx++) {

@@ -795,6 +803,10 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
   	struct ccwchain *chain;
   	struct ccw1 *cpa;
 
+	/* this is an error in the caller */
+	if (!cp || !cp->initialized)
+		return NULL;
+
   	orb = &cp->orb;
 
   	orb->cmd.intparm = intparm;

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

@@ -869,6 +884,9 @@ bool cp_iova_pinned(struct channel_program *cp, u64 iova)
   	struct ccwchain *chain;
   	int i;
 

Quoted 7 lines

Review bool cp_iova_pinned(struct channel_program *cp, u64 iova) · line 887

Quoted 1 line

(quoted diff) +1 -0

+	if (!cp->initialized)  

So, two of the checks added above look for a nonzero cp pointer prior to checking initialized, while two don't. I guess cp can't be NULL, since it's embedded in the private struct directly and that's only free'd when we do vfio_ccw_sch_remove() ... But I guess some consistency in how we look would be nice.

The idea was: In which context is this called? Is there a legitimate reason for the caller to pass in an uninitialized cp, or would that mean the caller had messed up (and we should not trust cp to be !NULL either?)

Quoted 2 lines

But you're right, that does look inconsistent. Always checking for cp != NULL probably looks least odd, although it is overkill. Opinions?

My opinion? Since cp is embedded in vfio_ccw_private, rather than a pointer to a separately malloc'd struct, we pass &private->cp to those functions. So a check for !cp doesn't really buy us anything because what we are actually concerned about is whether or not private is NULL, which only changes on the probe/remove boundaries.

The flow for processing ssch requests can be improved by splitting the BUSY state:

- CP_PROCESSING: We reject any user space requests while we are in the process of translating a channel program and submitting it to the hardware. Use -EAGAIN to signal user space that it should retry the request.
- CP_PENDING: We have successfully submitted a request with ssch and are now expecting an interrupt. As we can't handle more than one channel program being processed, reject any further requests with -EBUSY. A final interrupt will move us out of this state; this also fixes a latent bug where a non-final interrupt might have freed up a channel program that still was in progress.
By making this a separate state, we make it possible to issue a halt or a clear while we're still waiting for the final interrupt for the ssch (in a follow-on patch).

It also makes a lot of sense not to preemptively filter out writes to the io_region if we're in an incorrect state: the state machine will handle this correctly.

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

---
  drivers/s390/cio/vfio_ccw_drv.c     |  8 ++++++--
  drivers/s390/cio/vfio_ccw_fsm.c     | 19 ++++++++++++++-----
  drivers/s390/cio/vfio_ccw_ops.c     |  2 --
  drivers/s390/cio/vfio_ccw_private.h |  3 ++-
  4 files changed, 22 insertions(+), 10 deletions(-)

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

index a10cec0e86eb..0b3b9de45c60 100644
@@ -72,20 +72,24 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
  {
  	struct vfio_ccw_private *private;
  	struct irb *irb;
+	bool is_final;
 
  	private = container_of(work, struct vfio_ccw_private, io_work);
  	irb = &private->irb;
 
+	is_final = !(scsw_actl(&irb->scsw) &
+		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
  	if (scsw_is_solicited(&irb->scsw)) {
  		cp_update_scsw(&private->cp, &irb->scsw);
-		cp_free(&private->cp);
+		if (is_final)
+			cp_free(&private->cp);
  	}
  	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
 
  	if (private->io_trigger)
  		eventfd_signal(private->io_trigger, 1);
 
-	if (private->mdev)
+	if (private->mdev && is_final)
  		private->state = VFIO_CCW_STATE_IDLE;
  }
 

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

index e7c9877c9f1e..b4a141fbd1a8 100644
@@ -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;
 
  	orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
  	if (!orb) {

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

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

@@ -107,6 +107,12 @@ static void fsm_io_busy(struct vfio_ccw_private *private,
  	private->io_region->ret_code = -EBUSY;
  }
 
+static void fsm_io_retry(struct vfio_ccw_private *private,
+			 enum vfio_ccw_event event)
+{
+	private->io_region->ret_code = -EAGAIN;
+}
+
  static void fsm_disabled_irq(struct vfio_ccw_private *private,
  			     enum vfio_ccw_event event)
  {

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

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

@@ -221,7 +225,12 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_request,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
  	},
-	[VFIO_CCW_STATE_BUSY] = {
+	[VFIO_CCW_STATE_CP_PROCESSING] = {
+		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
+		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_retry,
+		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+	},
+	[VFIO_CCW_STATE_CP_PENDING] = {
  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,

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

index f673e106c041..3fdcc6dfe0bf 100644
@@ -193,8 +193,6 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
  		return -EINVAL;
 
  	private = dev_get_drvdata(mdev_parent_dev(mdev));
-	if (private->state != VFIO_CCW_STATE_IDLE)
-		return -EACCES;
 
  	region = private->io_region;
  	if (copy_from_user((void *)region + *ppos, buf, count))

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

index 08e9a7dc9176..50c52efb4fcb 100644
@@ -63,7 +63,8 @@ enum vfio_ccw_state {
  	VFIO_CCW_STATE_NOT_OPER,
  	VFIO_CCW_STATE_STANDBY,
  	VFIO_CCW_STATE_IDLE,
-	VFIO_CCW_STATE_BUSY,
+	VFIO_CCW_STATE_CP_PROCESSING,
+	VFIO_CCW_STATE_CP_PENDING,
  	/* last element! */
  	NR_VFIO_CCW_STATES
  };

Quoted 42 lines · Cornelia Huck

The flow for processing ssch requests can be improved by splitting the BUSY state:

- CP_PROCESSING: We reject any user space requests while we are in the process of translating a channel program and submitting it to the hardware. Use -EAGAIN to signal user space that it should retry the request.
- CP_PENDING: We have successfully submitted a request with ssch and are now expecting an interrupt. As we can't handle more than one channel program being processed, reject any further requests with -EBUSY. A final interrupt will move us out of this state; this also fixes a latent bug where a non-final interrupt might have freed up a channel program that still was in progress.
By making this a separate state, we make it possible to issue a halt or a clear while we're still waiting for the final interrupt for the ssch (in a follow-on patch).

It also makes a lot of sense not to preemptively filter out writes to the io_region if we're in an incorrect state: the state machine will handle this correctly.

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

---
  drivers/s390/cio/vfio_ccw_drv.c     |  8 ++++++--
  drivers/s390/cio/vfio_ccw_fsm.c     | 19 ++++++++++++++-----
  drivers/s390/cio/vfio_ccw_ops.c     |  2 --
  drivers/s390/cio/vfio_ccw_private.h |  3 ++-
  4 files changed, 22 insertions(+), 10 deletions(-)

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

index e7c9877c9f1e..b4a141fbd1a8 100644
@@ -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;
 
  	orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
  	if (!orb) {

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

Quoted 4 lines

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

@@ -46,6 +45,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
  		 */
  		sch->schib.scsw.cmd.actl |= SCSW_ACTL_START_PEND;
  		ret = 0;
+		private->state = VFIO_CCW_STATE_CP_PENDING;  

[1]

Quoted 17 lines

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

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

@@ -107,6 +107,12 @@ static void fsm_io_busy(struct vfio_ccw_private *private,
  	private->io_region->ret_code = -EBUSY;
  }
 
+static void fsm_io_retry(struct vfio_ccw_private *private,
+			 enum vfio_ccw_event event)
+{
+	private->io_region->ret_code = -EAGAIN;
+}
+
  static void fsm_disabled_irq(struct vfio_ccw_private *private,
  			     enum vfio_ccw_event event)
  {

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

Quoted 6 lines

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

@@ -135,8 +141,7 @@ 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;
-
+	private->state = VFIO_CCW_STATE_CP_PROCESSING;  

[1]

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

Quoted 8 lines

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

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

@@ -181,7 +186,6 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  	}
 
  err_out:
-	private->state = VFIO_CCW_STATE_IDLE;  

[1] Revisiting these locations as from an earlier discussion [2]... These go IDLE->CP_PROCESSING->CP_PENDING if we get a cc=0 on the SSCH, but we stop in CP_PROCESSING if the SSCH gets a nonzero cc. Shouldn't we cleanup and go back to IDLE in this scenario, rather than forcing userspace to escalate to CSCH/HSCH after some number of retries (via FSM)?

[2] https://patchwork.kernel.org/patch/10773611/#22447997

Besides that, I think this looks good to me.

Quoted 91 lines · On Mon, 4 Feb 2019 16:29:40 -0500 Eric Farman <[email protected]> wrote:

Quoted 42 lines · Cornelia Huck

The flow for processing ssch requests can be improved by splitting the BUSY state:

- CP_PROCESSING: We reject any user space requests while we are in the process of translating a channel program and submitting it to the hardware. Use -EAGAIN to signal user space that it should retry the request.
- CP_PENDING: We have successfully submitted a request with ssch and are now expecting an interrupt. As we can't handle more than one channel program being processed, reject any further requests with -EBUSY. A final interrupt will move us out of this state; this also fixes a latent bug where a non-final interrupt might have freed up a channel program that still was in progress.
By making this a separate state, we make it possible to issue a halt or a clear while we're still waiting for the final interrupt for the ssch (in a follow-on patch).

It also makes a lot of sense not to preemptively filter out writes to the io_region if we're in an incorrect state: the state machine will handle this correctly.

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

---
   drivers/s390/cio/vfio_ccw_drv.c     |  8 ++++++--
   drivers/s390/cio/vfio_ccw_fsm.c     | 19 ++++++++++++++-----
   drivers/s390/cio/vfio_ccw_ops.c     |  2 --
   drivers/s390/cio/vfio_ccw_private.h |  3 ++-
   4 files changed, 22 insertions(+), 10 deletions(-)

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

index e7c9877c9f1e..b4a141fbd1a8 100644
@@ -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;
 
   	orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
   	if (!orb) {

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

Quoted 4 lines

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

@@ -46,6 +45,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
   		 */
   		sch->schib.scsw.cmd.actl |= SCSW_ACTL_START_PEND;
   		ret = 0;
+		private->state = VFIO_CCW_STATE_CP_PENDING;

[1]

Quoted 17 lines

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

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

@@ -107,6 +107,12 @@ static void fsm_io_busy(struct vfio_ccw_private *private,
   	private->io_region->ret_code = -EBUSY;
   }
 
+static void fsm_io_retry(struct vfio_ccw_private *private,
+			 enum vfio_ccw_event event)
+{
+	private->io_region->ret_code = -EAGAIN;
+}
+
   static void fsm_disabled_irq(struct vfio_ccw_private *private,
   			     enum vfio_ccw_event event)
   {

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

Quoted 6 lines

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

@@ -135,8 +141,7 @@ 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;
-
+	private->state = VFIO_CCW_STATE_CP_PROCESSING;

[1]

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

Quoted 8 lines

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

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

@@ -181,7 +186,6 @@ static void fsm_io_request(struct vfio_ccw_private *private,
   	}
 
   err_out:
-	private->state = VFIO_CCW_STATE_IDLE;

[1] Revisiting these locations as from an earlier discussion [2]... These go IDLE->CP_PROCESSING->CP_PENDING if we get a cc=0 on the SSCH, but we stop in CP_PROCESSING if the SSCH gets a nonzero cc. Shouldn't we cleanup and go back to IDLE in this scenario, rather than forcing userspace to escalate to CSCH/HSCH after some number of retries (via FSM)?

Quoted 1 line

[2] https://patchwork.kernel.org/patch/10773611/#22447997

It does do that (in vfio_ccw_mdev_write), it was not needed here. Or do you think doing it here would be more obvious?

Quoted 3 lines

Besides that, I think this looks good to me.

Thanks!

Quoted 94 lines · Cornelia Huck

Quoted 91 lines · On Mon, 4 Feb 2019 16:29:40 -0500 Eric Farman <[email protected]> wrote:

Quoted 42 lines · Cornelia Huck

The flow for processing ssch requests can be improved by splitting the BUSY state:

- CP_PROCESSING: We reject any user space requests while we are in the process of translating a channel program and submitting it to the hardware. Use -EAGAIN to signal user space that it should retry the request.
- CP_PENDING: We have successfully submitted a request with ssch and are now expecting an interrupt. As we can't handle more than one channel program being processed, reject any further requests with -EBUSY. A final interrupt will move us out of this state; this also fixes a latent bug where a non-final interrupt might have freed up a channel program that still was in progress.
By making this a separate state, we make it possible to issue a halt or a clear while we're still waiting for the final interrupt for the ssch (in a follow-on patch).

It also makes a lot of sense not to preemptively filter out writes to the io_region if we're in an incorrect state: the state machine will handle this correctly.

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

---
   drivers/s390/cio/vfio_ccw_drv.c     |  8 ++++++--
   drivers/s390/cio/vfio_ccw_fsm.c     | 19 ++++++++++++++-----
   drivers/s390/cio/vfio_ccw_ops.c     |  2 --
   drivers/s390/cio/vfio_ccw_private.h |  3 ++-
   4 files changed, 22 insertions(+), 10 deletions(-)  

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

index e7c9877c9f1e..b4a141fbd1a8 100644
@@ -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;
 
   	orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
   	if (!orb) {

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

Quoted 4 lines

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

@@ -46,6 +45,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
   		 */
   		sch->schib.scsw.cmd.actl |= SCSW_ACTL_START_PEND;
   		ret = 0;
+		private->state = VFIO_CCW_STATE_CP_PENDING;  

[1]

Quoted 17 lines

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

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

@@ -107,6 +107,12 @@ static void fsm_io_busy(struct vfio_ccw_private *private,
   	private->io_region->ret_code = -EBUSY;
   }
 
+static void fsm_io_retry(struct vfio_ccw_private *private,
+			 enum vfio_ccw_event event)
+{
+	private->io_region->ret_code = -EAGAIN;
+}
+
   static void fsm_disabled_irq(struct vfio_ccw_private *private,
   			     enum vfio_ccw_event event)
   {

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

Quoted 6 lines

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

@@ -135,8 +141,7 @@ 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;
-
+	private->state = VFIO_CCW_STATE_CP_PROCESSING;  

[1]

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

Quoted 8 lines

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

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

@@ -181,7 +186,6 @@ static void fsm_io_request(struct vfio_ccw_private *private,
   	}
 
   err_out:
-	private->state = VFIO_CCW_STATE_IDLE;  

[1] Revisiting these locations as from an earlier discussion [2]... These go IDLE->CP_PROCESSING->CP_PENDING if we get a cc=0 on the SSCH, but we stop in CP_PROCESSING if the SSCH gets a nonzero cc. Shouldn't we cleanup and go back to IDLE in this scenario, rather than forcing userspace to escalate to CSCH/HSCH after some number of retries (via FSM)?

Quoted 4 lines

Quoted 1 line

[2] https://patchwork.kernel.org/patch/10773611/#22447997

It does do that (in vfio_ccw_mdev_write), it was not needed here. Or do you think doing it here would be more obvious?

Ah, my mistake, I missed that. (That function is renamed to vfio_ccw_mdev_write_io_region in patch 4.)

I don't think keeping it here is necessary then. I got too focused looking at what you ripped out that I lost the things that stayed. Once this series gets in its entirety, and Pierre has a chance to rebase his FSM series on top of it all, this should be in great shape.

Quoted 7 lines

Quoted 4 lines

Besides that, I think this looks good to me.

Thanks!

You're welcome! Here, have a thing to add to this patch:

Reviewed-by Eric Farman <[email protected]>

Introduce a mutex to disallow concurrent reads or writes to the I/O region. This makes sure that the data the kernel or user space see is always consistent.

The same mutex will be used to protect the async region as well.

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

---
  drivers/s390/cio/vfio_ccw_drv.c     |  3 +++
  drivers/s390/cio/vfio_ccw_ops.c     | 28 +++++++++++++++++++---------
  drivers/s390/cio/vfio_ccw_private.h |  2 ++
  3 files changed, 24 insertions(+), 9 deletions(-)

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

index 0b3b9de45c60..5ea0da1dd954 100644
@@ -84,7 +84,9 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
  		if (is_final)
  			cp_free(&private->cp);
  	}
+	mutex_lock(&private->io_mutex);
  	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
+	mutex_unlock(&private->io_mutex);
 
  	if (private->io_trigger)
  		eventfd_signal(private->io_trigger, 1);

@@ -129,6 +131,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_ops.c +19 -9

index 3fdcc6dfe0bf..025c8a832bc8 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,23 +192,29 @@ 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 (!mutex_trylock(&private->io_mutex))
+		return -EAGAIN;
 
  	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) {
+	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;
  }
 
  static int vfio_ccw_mdev_get_device_info(struct vfio_device_info *info)

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

index 50c52efb4fcb..32173cbd838d 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 regions
   * @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 7 lines · Cornelia Huck

Introduce a mutex to disallow concurrent reads or writes to the I/O region. This makes sure that the data the kernel or user space see is always consistent.

The same mutex will be used to protect the async region as well.

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

I keep wondering how the FSM could provide this, but I end up getting into chicken/egg rabbit holes.

So, until my brain becomes wiser...

Reviewed-by Eric Farman <[email protected]>

Quoted 6 lines

---
  drivers/s390/cio/vfio_ccw_drv.c     |  3 +++
  drivers/s390/cio/vfio_ccw_ops.c     | 28 +++++++++++++++++++---------
  drivers/s390/cio/vfio_ccw_private.h |  2 ++
  3 files changed, 24 insertions(+), 9 deletions(-)

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 +84 -21

index 025c8a832bc8..48b2d7930ea8 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,24 +193,47 @@ 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 (!mutex_trylock(&private->io_mutex))
  		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;
  	}

@@ -217,19 +248,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;

@@ -237,9 +301,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 */

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

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

@@ -316,6 +422,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)

@@ -336,7 +468,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;
 

@@ -345,8 +477,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);
 

@@ -356,8 +486,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 +2 -0

index 32173cbd838d..c979eb32fb1c 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_

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

+
+/* capability chain handling similar to vfio-pci */
+struct vfio_ccw_private;
+struct vfio_ccw_region;
+
+struct vfio_ccw_regops {
+	ssize_t	(*read)(struct vfio_ccw_private *private, char __user *buf,
+			size_t count, loff_t *ppos);
+	ssize_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 regions
+ * @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

Quoted 9 lines · Cornelia Huck

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

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

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

Quoted 7 lines

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

@@ -237,9 +301,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 */

drivers/s390/cio/vfio_ccw_ops.c static int vfio_ccw_mdev_get_region_info(struct vfio_region_info *info, · line 314

Quoted 10 lines

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

+	{
+		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 notice the similarity of this hunk to drivers/vfio/pci/vfio_pci.c ... While I was trying to discern the likelihood/possibility/usefulness of combining the two, I noticed that there is one difference at this point in the other file, which was added by commit 0e714d27786c ("vfio/pci:
Fix potential Spectre v1")

This got me off on a tangent of setting up smatch in my environment, and sure enough it flags this point [1] as being problematic:

drivers/s390/cio/vfio_ccw_ops.c:328 vfio_ccw_mdev_get_region_info()
warn: potential spectre issue 'private->region' [r]

Might need to consider the same? (And lends credence to my concern about the capability chain code being duplicated.)

drivers/s390/cio/vfio_ccw_ops.c static int vfio_ccw_mdev_get_region_info(struct vfio_region_info *info, · line 319

Quoted 6 lines

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

+
+		i = info->index - VFIO_CCW_NUM_REGIONS;
+
+		info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index);
+		info->size = private->region[i].size;  

[1] smatch actually points to this line, though the referenced commit inserts a line up there.

Quoted 30 lines

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

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

Quoted 4 lines

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

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

drivers/s390/cio/vfio_ccw_private.h line 27

Quoted 4 lines

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

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

I know Farhan asked this back in v1, but I'd still love a better answer than "vfio-pci did this" to what this is. There's a lot more regions prior to the capability chain in vfio-pci than here (9 versus 1), so I'd like to be certain it's not related to that.

Quoted 8 lines

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

index 02bb7ad6e986..56e2413d3e00 100644
@@ -353,6 +353,8 @@ struct vfio_region_gfx_edid {
  #define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
  };
 

include/uapi/linux/vfio.h struct vfio_region_gfx_edid { · line 357

Quoted 2 lines

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

+#define VFIO_REGION_TYPE_CCW			(2)
+  

I'm not sure if this should be here to keep it in its own area (esp. for when patch 6 comes along), or with VFIO_REGION_TYPE_GFX to make it noticeable where we are in the list without grepping for VFIO_REGION_TYPE. I guess it's just what it is, even if I'm not thrilled about it.

include/uapi/linux/vfio.h struct vfio_region_gfx_edid { · line 360

Quoted 5 lines

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

  /*
   * 10de vendor sub-type
   *

This generally looks sane to me, even though I can't get past the idea that there's opportunities for improvement between the two. Maybe that's refactoring for a day when someone is bored. ;-)

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

@@ -213,26 +323,31 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 	[VFIO_CCW_STATE_NOT_OPER] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_nop,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
+		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
+		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 	},

(quoted diff) +0 -0

 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 	},

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

@@ -213,26 +323,31 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 	[VFIO_CCW_STATE_NOT_OPER] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_nop,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
+		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
+		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 	},

(quoted diff) +0 -0

 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 	},

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

Quoted 6 lines · On Wed, 30 Jan 2019 14:22:12 +0100 Cornelia Huck <[email protected]> wrote:

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

+static void fsm_async_retry(struct vfio_ccw_private *private,
+			    enum vfio_ccw_event event)
+{
+	private->cmd_region->ret_code = -EAGAIN;
+}
+  

This is essentially dead code at the moment, isn't it? I mean we hold the io_mutex whenever we are in state VFIO_CCW_STATE_CP_PROCESSING, and we call vfio_ccw_fsm_event() with the very same mutex held.

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

The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps
This is the previously posted v2 version, which should continue to work.]

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

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

Lightly tested (I can interact with a dasd as before, and reserve/release seems to work well.) Not sure if there is a better way to test this, ideas welcome.

Changes v2->v3:
- Unb0rked patch 1, improved scope
- Split out the new mutex from patch 2 into new patch 3; added missing locking and hopefully improved description
- Patch 2 now reworks the state handling by splitting the BUSY state into CP_PROCESSING and CP_PENDING
- Patches 3 and 5 adapted on top of the reworked patches; hsch/csch are allowed in CP_PENDING, but not in CP_PROCESSING (did not add any R-b due to that)
- Added missing free in patch 5
- Probably some small changes I forgot to note down

Changes v1->v2:
- New patch 1: make it safe to use the cp accessors at any time; this should avoid problems with unsolicited interrupt handling
- New patch 2: handle concurrent accesses to the io region; the idea is to return -EAGAIN to userspace more often (so it can simply retry)
- also handle concurrent accesses to the async io region
- change VFIO_REGION_TYPE_CCW
- merge events for halt and clear to a single async event; this turned out to make the code quite a bit simpler
- probably some small changes I forgot to note down

Cornelia Huck (6):
vfio-ccw: make it safe to access channel programs
vfio-ccw: rework ssch state handling
vfio-ccw: protect the I/O region
vfio-ccw: add capabilities chain
s390/cio: export hsch to modules
vfio-ccw: add handling for async channel instructions

 drivers/s390/cio/Makefile           |   3 +-
 drivers/s390/cio/ioasm.c            |   1 +
 drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++
 drivers/s390/cio/vfio_ccw_cp.c      |  20 ++-
 drivers/s390/cio/vfio_ccw_cp.h      |   2 +
 drivers/s390/cio/vfio_ccw_drv.c     |  57 ++++++--
 drivers/s390/cio/vfio_ccw_fsm.c     | 143 ++++++++++++++++++-
 drivers/s390/cio/vfio_ccw_ops.c     | 210 +++++++++++++++++++++++-----
 drivers/s390/cio/vfio_ccw_private.h |  48 ++++++-
 include/uapi/linux/vfio.h           |   4 +
 include/uapi/linux/vfio_ccw.h       |  12 ++
 11 files changed, 531 insertions(+), 57 deletions(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_async.c

Quoted 2 lines · On Wed, 30 Jan 2019 14:22:06 +0100 Cornelia Huck <[email protected]> wrote:

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

I've pushed out the changes I've made so far (patch 1) to vfio-ccw-eagain-caps-v3.5. I'll wait a bit for more comments before sending a new version.

Quoted 4 lines

The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps
This is the previously posted v2 version, which should continue to work.]

I would not mind if somebody looked at those as well :)

Quoted 60 lines

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

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

Lightly tested (I can interact with a dasd as before, and reserve/release seems to work well.) Not sure if there is a better way to test this, ideas welcome.

Changes v2->v3:
- Unb0rked patch 1, improved scope
- Split out the new mutex from patch 2 into new patch 3; added missing locking and hopefully improved description
- Patch 2 now reworks the state handling by splitting the BUSY state into CP_PROCESSING and CP_PENDING
- Patches 3 and 5 adapted on top of the reworked patches; hsch/csch are allowed in CP_PENDING, but not in CP_PROCESSING (did not add any R-b due to that)
- Added missing free in patch 5
- Probably some small changes I forgot to note down

Changes v1->v2:
- New patch 1: make it safe to use the cp accessors at any time; this should avoid problems with unsolicited interrupt handling
- New patch 2: handle concurrent accesses to the io region; the idea is to return -EAGAIN to userspace more often (so it can simply retry)
- also handle concurrent accesses to the async io region
- change VFIO_REGION_TYPE_CCW
- merge events for halt and clear to a single async event; this turned out to make the code quite a bit simpler
- probably some small changes I forgot to note down

Cornelia Huck (6):
vfio-ccw: make it safe to access channel programs
vfio-ccw: rework ssch state handling
vfio-ccw: protect the I/O region
vfio-ccw: add capabilities chain
s390/cio: export hsch to modules
vfio-ccw: add handling for async channel instructions

  drivers/s390/cio/Makefile           |   3 +-
  drivers/s390/cio/ioasm.c            |   1 +
  drivers/s390/cio/vfio_ccw_async.c   |  88 ++++++++++++
  drivers/s390/cio/vfio_ccw_cp.c      |  20 ++-
  drivers/s390/cio/vfio_ccw_cp.h      |   2 +
  drivers/s390/cio/vfio_ccw_drv.c     |  57 ++++++--
  drivers/s390/cio/vfio_ccw_fsm.c     | 143 ++++++++++++++++++-
  drivers/s390/cio/vfio_ccw_ops.c     | 210 +++++++++++++++++++++++-----
  drivers/s390/cio/vfio_ccw_private.h |  48 ++++++-
  include/uapi/linux/vfio.h           |   4 +
  include/uapi/linux/vfio_ccw.h       |  12 ++
  11 files changed, 531 insertions(+), 57 deletions(-)

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

Quoted 6 lines · Cornelia Huck

Quoted 2 lines · On Wed, 30 Jan 2019 14:22:06 +0100 Cornelia Huck <[email protected]> wrote:

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

Quoted 4 lines

I've pushed out the changes I've made so far (patch 1) to vfio-ccw-eagain-caps-v3.5. I'll wait a bit for more comments before sending a new version.

Thanks for that branch... For patch 1 in v3.5:

Reviewed-by Eric Farman <[email protected]>

Quoted 7 lines

Quoted 5 lines

The companion QEMU patches are available at
https://github.com/cohuck/qemu vfio-ccw-caps
This is the previously posted v2 version, which should continue to work.]

I would not mind if somebody looked at those as well :)

Not precluding anyone else from doing so :) ... I'd planned on looking at them as I get into the meat of patches 4-6 on the kernel side, where the overlap occurs. I'm getting close. :)

FWIW, I've been running with both series for the last week or two, along with some host kernel traces to prove things got executed the way I thought, and it's seemed to be working well. So that makes me optimistic for the later patches.

Quoted 5 lines · On Fri, 8 Feb 2019 16:19:58 -0500 Eric Farman <[email protected]> wrote:

FWIW, I've been running with both series for the last week or two, along with some host kernel traces to prove things got executed the way I thought, and it's seemed to be working well. So that makes me optimistic for the later patches.

That's good news, thanks for testing. Do you have a special test load that you run in the guest that you can share?