Some vfio-ccw fixes

The comment is misleading because it tells us that we should set orb.cmd.c64 before calling ccwchain_calc_length, otherwise the function ccwchain_calc_length would return an error. This is not completely accurate.

We want to allow an orb without cmd.c64, and this is fine as long as the channel program does not use IDALs. But we do want to reject any channel program that uses IDALs and does not set the flag, which is what we do in ccwchain_calc_length.

After we have done the ccw processing, we need to set cmd.c64, as we use IDALs for all translated channel programs.

Also for better code readability let's move the setting of cmd.c64 within the non error path.

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

Reviewed-by Cornelia Huck <[email protected]>

---
 drivers/s390/cio/vfio_ccw_cp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/s390/cio/vfio_ccw_cp.c +7 -6

index d6a8dff..c969d48 100644
@@ -645,14 +645,15 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
 	if (ret)
 		cp_free(cp);
 
-	/* It is safe to force: if not set but idals used
-	 * ccwchain_calc_length returns an error.
-	 */
-	cp->orb.cmd.c64 = 1;
-
-	if (!ret)
+	if (!ret) {
 		cp->initialized = true;
 
+		/* It is safe to force: if it was not set but idals used
+		 * ccwchain_calc_length would have returned an error.
+		 */
+		cp->orb.cmd.c64 = 1;
+	}
+
 	return ret;
 }
 

We don't set cp->initialized to true so calling cp_free will just return and not do anything.

Also fix a memory leak where we fail to free a ccwchain on an error.

Fixes 812271b910 ("s390/cio: Squash cp_free() and cp_unpin_free()")

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

---
 drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

drivers/s390/cio/vfio_ccw_cp.c +7 -4

index c969d48..f862d5d 100644
@@ -421,7 +421,7 @@ static int ccwchain_loop_tic(struct ccwchain *chain,
 static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)
 {
 	struct ccwchain *chain;
-	int len;
+	int len, ret;
 
 	/* Copy 2K (the most we support today) of possible CCWs */
 	len = copy_from_iova(cp->mdev, cp->guest_cp, cda,

@@ -448,7 +448,12 @@ static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)
 	memcpy(chain->ch_ccw, cp->guest_cp, len * sizeof(struct ccw1));
 
 	/* Loop for tics on this new chain. */
-	return ccwchain_loop_tic(chain, cp);
+	ret = ccwchain_loop_tic(chain, cp);
+
+	if (ret)
+		ccwchain_free(chain);
+
+	return ret;
 }
 
 /* Loop for TICs. */

@@ -642,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
 
 	/* Build a ccwchain for the first CCW segment */
 	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
-	if (ret)
-		cp_free(cp);
 
 	if (!ret) {
 		cp->initialized = true;

We don't set cp->initialized to true so calling cp_free will just return and not do anything.

Also fix a memory leak where we fail to free a ccwchain on an error.

Fixes 812271b910 ("s390/cio: Squash cp_free() and cp_unpin_free()")

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

---
 drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

We now support CLEAR SUBCHANNEL and HALT SUBCHANNEL via ccw_cmd_region.

Fixes d5afd5d135c8 ("vfio-ccw: add handling for async channel instructions")

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

---
 Documentation/s390/vfio-ccw.rst | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

There is a small window where it's possible that we could be working on an interrupt (queued in the workqueue) and setting up a channel program (i.e allocating memory, pinning pages, translating address). This can lead to allocating and freeing the channel program at the same time and can cause memory corruption.

Let's not call cp_free if we are currently processing a channel program. The only way we know for sure that we don't have a thread setting up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.>

Fixes d5afd5d135c8 ("vfio-ccw: add handling for async channel instructions")

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

Reviewed-by Cornelia Huck <[email protected]>

---
 drivers/s390/cio/vfio_ccw_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index 4e3a903..0357165 100644
@@ -92,7 +92,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
 	if (scsw_is_solicited(&irb->scsw)) {
 		cp_update_scsw(&private->cp, &irb->scsw);
-		if (is_final)
+		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
 			cp_free(&private->cp);
 	}
 	mutex_lock(&private->io_mutex);

Quoted 8 lines · On Thu, 11 Jul 2019 10:28:55 -0400 Farhan Ali <[email protected]> wrote:

We now support CLEAR SUBCHANNEL and HALT SUBCHANNEL via ccw_cmd_region.

Fixes d5afd5d135c8 ("vfio-ccw: add handling for async channel instructions")

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

---
  Documentation/s390/vfio-ccw.rst | 31 ++++++++++++++++++++++++++++---
  1 file changed, 28 insertions(+), 3 deletions(-)

(...)

Documentation/s390/vfio-ccw.rst irb_area stores the I/O result. · line 222

Quoted 6 lines

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

+vfio-ccw cmd region
+-------------------
+
+The vfio-ccw cmd region is used to accept asynchronous instructions
+from userspace.
+

Add :: and indent the structure so that we get proper formatting?

(Sorry about not noticing this last time; but I can add it while applying if there are no other comments.)

Documentation/s390/vfio-ccw.rst irb_area stores the I/O result. · line 233

Quoted 12 lines

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

+#define VFIO_CCW_ASYNC_CMD_HSCH (1 << 0)
+#define VFIO_CCW_ASYNC_CMD_CSCH (1 << 1)
+struct ccw_cmd_region {
+       __u32 command;
+       __u32 ret_code;
+} __packed;
+
+This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD.
+
+Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region.
+

Otherwise,

Reviewed-by Cornelia Huck <[email protected]>

Quoted 8 lines · On Thu, 11 Jul 2019 10:28:55 -0400 Farhan Ali <[email protected]> wrote:

We now support CLEAR SUBCHANNEL and HALT SUBCHANNEL via ccw_cmd_region.

Fixes d5afd5d135c8 ("vfio-ccw: add handling for async channel instructions")

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

---
 Documentation/s390/vfio-ccw.rst | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

(...)

Documentation/s390/vfio-ccw.rst irb_area stores the I/O result. · line 222

Quoted 6 lines

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

+vfio-ccw cmd region
+-------------------
+
+The vfio-ccw cmd region is used to accept asynchronous instructions
+from userspace.
+

Add :: and indent the structure so that we get proper formatting?

(Sorry about not noticing this last time; but I can add it while applying if there are no other comments.)

Documentation/s390/vfio-ccw.rst irb_area stores the I/O result. · line 233

Quoted 11 lines

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

+#define VFIO_CCW_ASYNC_CMD_HSCH (1 << 0)
+#define VFIO_CCW_ASYNC_CMD_CSCH (1 << 1)
+struct ccw_cmd_region {
+       __u32 command;
+       __u32 ret_code;
+} __packed;
+
+This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD.
+
+Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region.
+

Otherwise,

Reviewed-by Cornelia Huck <[email protected]>

Hi,

While trying to chase down the problem regarding the stacktraces, I have also found some minor problems in the code.

Would appreciate any review or feedback regarding them.

Thanks
Farhan

ChangeLog
---------
v2 -> v3
- Minor changes as suggested by Conny
- Add Conny's reviewed-by tags
- Add fixes tag for patch 4 and patch 5

v1 -> v2
- Update docs for csch/hsch since we can support those instructions now (patch 5)
- Fix the memory leak where we fail to free a ccwchain (patch 2)
- Add fixes tag where appropriate.
- Fix comment instead of the order when setting orb.cmd.c64 (patch 1)

Farhan Ali (5):
vfio-ccw: Fix misleading comment when setting orb.cmd.c64
vfio-ccw: Fix memory leak and don't call cp_free in cp_init
vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn
vfio-ccw: Don't call cp_free if we are processing a channel program
vfio-ccw: Update documentation for csch/hsch

 Documentation/s390/vfio-ccw.rst | 31 ++++++++++++++++++++++++++++---
 drivers/s390/cio/vfio_ccw_cp.c  | 28 +++++++++++++++++-----------
 drivers/s390/cio/vfio_ccw_drv.c |  2 +-
 3 files changed, 46 insertions(+), 15 deletions(-)