From c9f597a4d6d7a01590571291f659a2f146111e34 Mon Sep 17 00:00:00 2001 From: Farhan Ali Date: Thu, 11 Jul 2019 10:28:51 -0400 Subject: vfio-ccw: Fix misleading comment when setting orb.cmd.c64 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. Fixes: fb9e7880af35 ("vfio: ccw: push down unsupported IDA check") Signed-off-by: Farhan Ali Reviewed-by: Cornelia Huck Message-Id: Reviewed-by: Eric Farman Signed-off-by: Cornelia Huck --- drivers/s390/cio/vfio_ccw_cp.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index 1d4c893ead23..46967c664c0f 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -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; } -- cgit v1.2.3 From 8b515be512a2435bb8aedc6390cbe140167f9eb9 Mon Sep 17 00:00:00 2001 From: Farhan Ali Date: Thu, 11 Jul 2019 10:28:52 -0400 Subject: vfio-ccw: Fix memory leak and don't call cp_free in cp_init 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 Message-Id: <3173c4216f4555d9765eb6e4922534982bc820e4.1562854091.git.alifm@linux.ibm.com> Reviewed-by: Cornelia Huck Reviewed-by: Eric Farman Signed-off-by: Cornelia Huck --- drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index 46967c664c0f..e4e8724eddaa 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -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; -- cgit v1.2.3 From c1ab69268d124ebdbb3864580808188ccd3ea355 Mon Sep 17 00:00:00 2001 From: Farhan Ali Date: Thu, 11 Jul 2019 10:28:53 -0400 Subject: vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn So we don't call try to call vfio_unpin_pages() incorrectly. Fixes: 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces") Signed-off-by: Farhan Ali Reviewed-by: Eric Farman Reviewed-by: Cornelia Huck Message-Id: <33a89467ad6369196ae6edf820cbcb1e2d8d050c.1562854091.git.alifm@linux.ibm.com> Signed-off-by: Cornelia Huck --- drivers/s390/cio/vfio_ccw_cp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index e4e8724eddaa..3645d1720c4b 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -72,8 +72,10 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len) sizeof(*pa->pa_iova_pfn) + sizeof(*pa->pa_pfn), GFP_KERNEL); - if (unlikely(!pa->pa_iova_pfn)) + if (unlikely(!pa->pa_iova_pfn)) { + pa->pa_nr = 0; return -ENOMEM; + } pa->pa_pfn = pa->pa_iova_pfn + pa->pa_nr; pa->pa_iova_pfn[0] = pa->pa_iova >> PAGE_SHIFT; -- cgit v1.2.3 From f4c9939433bd396d0b08e803b2b880a9d02682b9 Mon Sep 17 00:00:00 2001 From: Farhan Ali Date: Thu, 11 Jul 2019 10:28:54 -0400 Subject: vfio-ccw: Don't call cp_free if we are processing a channel program 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 Reviewed-by: Cornelia Huck Message-Id: <62e87bf67b38dc8d5760586e7c96d400db854ebe.1562854091.git.alifm@linux.ibm.com> Reviewed-by: Eric Farman Signed-off-by: Cornelia Huck --- drivers/s390/cio/vfio_ccw_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index 2b90a5ecaeb9..9208c0e56c33 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -88,7 +88,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); -- cgit v1.2.3 From 127e62174041496b383f82d696e1592ce6838604 Mon Sep 17 00:00:00 2001 From: Farhan Ali Date: Thu, 11 Jul 2019 10:28:55 -0400 Subject: vfio-ccw: Update documentation for csch/hsch 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 Message-Id: <7d977612c3f3152ffb950d77ae11b4b25c1e20c4.1562854091.git.alifm@linux.ibm.com> [CH: properly mark region as literal block] Reviewed-by: Cornelia Huck Reviewed-by: Eric Farman Signed-off-by: Cornelia Huck --- Documentation/s390/vfio-ccw.rst | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst index 1f6d0b56d53e..be2af10e12b4 100644 --- a/Documentation/s390/vfio-ccw.rst +++ b/Documentation/s390/vfio-ccw.rst @@ -180,6 +180,13 @@ The process of how these work together. add it to an iommu_group and a vfio_group. Then we could pass through the mdev to a guest. + +VFIO-CCW Regions +---------------- + +The vfio-ccw driver exposes MMIO regions to accept requests from and return +results to userspace. + vfio-ccw I/O region ------------------- @@ -205,6 +212,25 @@ irb_area stores the I/O result. ret_code stores a return code for each access of the region. +This region is always available. + +vfio-ccw cmd region +------------------- + +The vfio-ccw cmd region is used to accept asynchronous instructions +from userspace. + +#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. + vfio-ccw operation details -------------------------- @@ -306,9 +332,8 @@ Together with the corresponding work in QEMU, we can bring the passed through DASD/ECKD device online in a guest now and use it as a block device. -While the current code allows the guest to start channel programs via -START SUBCHANNEL, support for HALT SUBCHANNEL or CLEAR SUBCHANNEL is -not yet implemented. +The current code allows the guest to start channel programs via +START SUBCHANNEL, and to issue HALT SUBCHANNEL and CLEAR SUBCHANNEL. vfio-ccw supports classic (command mode) channel I/O only. Transport mode (HPF) is not supported. -- cgit v1.2.3 From 4c4cbbaa693a5cc435664f2f220c8b0be873abd1 Mon Sep 17 00:00:00 2001 From: Cornelia Huck Date: Wed, 17 Jul 2019 11:35:35 +0200 Subject: Documentation: fix vfio-ccw doc *Really* mark the literal block as such. Fixes: 127e62174041 ("vfio-ccw: Update documentation for csch/hsch") Signed-off-by: Cornelia Huck --- Documentation/s390/vfio-ccw.rst | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst index be2af10e12b4..3fe918f0c80d 100644 --- a/Documentation/s390/vfio-ccw.rst +++ b/Documentation/s390/vfio-ccw.rst @@ -218,14 +218,14 @@ vfio-ccw cmd region ------------------- The vfio-ccw cmd region is used to accept asynchronous instructions -from userspace. - -#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; +from userspace:: + + #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. -- cgit v1.2.3