kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/5] Some vfio-ccw fixes
@ 2019-07-11 14:28 Farhan Ali
  2019-07-11 14:28 ` [PATCH v3 1/5] vfio-ccw: Fix misleading comment when setting orb.cmd.c64 Farhan Ali
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Farhan Ali @ 2019-07-11 14:28 UTC (permalink / raw)
  To: cohuck, farman, pasic; +Cc: linux-s390, kvm, alifm

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

-- 
2.7.4


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 1/5] vfio-ccw: Fix misleading comment when setting orb.cmd.c64
  2019-07-11 14:28 [PATCH v3 0/5] Some vfio-ccw fixes Farhan Ali
@ 2019-07-11 14:28 ` Farhan Ali
  2019-07-11 20:30   ` Eric Farman
  2019-07-11 14:28 ` [PATCH v3 2/5] vfio-ccw: Fix memory leak and don't call cp_free in cp_init Farhan Ali
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Farhan Ali @ 2019-07-11 14:28 UTC (permalink / raw)
  To: cohuck, farman, pasic; +Cc: linux-s390, kvm, alifm

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 <alifm@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 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 d6a8dff..c969d48 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;
 }
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 2/5] vfio-ccw: Fix memory leak and don't call cp_free in cp_init
  2019-07-11 14:28 [PATCH v3 0/5] Some vfio-ccw fixes Farhan Ali
  2019-07-11 14:28 ` [PATCH v3 1/5] vfio-ccw: Fix misleading comment when setting orb.cmd.c64 Farhan Ali
@ 2019-07-11 14:28 ` Farhan Ali
  2019-07-11 20:30   ` Eric Farman
  2019-07-12 11:26   ` Cornelia Huck
  2019-07-11 14:28 ` [PATCH v3 3/5] vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn Farhan Ali
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 14+ messages in thread
From: Farhan Ali @ 2019-07-11 14:28 UTC (permalink / raw)
  To: cohuck, farman, pasic; +Cc: linux-s390, kvm, alifm

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 <alifm@linux.ibm.com>
---
 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 c969d48..f862d5d 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;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 3/5] vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn
  2019-07-11 14:28 [PATCH v3 0/5] Some vfio-ccw fixes Farhan Ali
  2019-07-11 14:28 ` [PATCH v3 1/5] vfio-ccw: Fix misleading comment when setting orb.cmd.c64 Farhan Ali
  2019-07-11 14:28 ` [PATCH v3 2/5] vfio-ccw: Fix memory leak and don't call cp_free in cp_init Farhan Ali
@ 2019-07-11 14:28 ` Farhan Ali
  2019-07-11 14:28 ` [PATCH v3 4/5] vfio-ccw: Don't call cp_free if we are processing a channel program Farhan Ali
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Farhan Ali @ 2019-07-11 14:28 UTC (permalink / raw)
  To: cohuck, farman, pasic; +Cc: linux-s390, kvm, alifm

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 <alifm@linux.ibm.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 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 f862d5d..b35fb7e 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;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 4/5] vfio-ccw: Don't call cp_free if we are processing a channel program
  2019-07-11 14:28 [PATCH v3 0/5] Some vfio-ccw fixes Farhan Ali
                   ` (2 preceding siblings ...)
  2019-07-11 14:28 ` [PATCH v3 3/5] vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn Farhan Ali
@ 2019-07-11 14:28 ` Farhan Ali
  2019-07-12 13:19   ` Eric Farman
  2019-07-11 14:28 ` [PATCH v3 5/5] vfio-ccw: Update documentation for csch/hsch Farhan Ali
  2019-07-15 13:07 ` [PATCH v3 0/5] Some vfio-ccw fixes Cornelia Huck
  5 siblings, 1 reply; 14+ messages in thread
From: Farhan Ali @ 2019-07-11 14:28 UTC (permalink / raw)
  To: cohuck, farman, pasic; +Cc: linux-s390, kvm, alifm

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 <alifm@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 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 4e3a903..0357165 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -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);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 5/5] vfio-ccw: Update documentation for csch/hsch
  2019-07-11 14:28 [PATCH v3 0/5] Some vfio-ccw fixes Farhan Ali
                   ` (3 preceding siblings ...)
  2019-07-11 14:28 ` [PATCH v3 4/5] vfio-ccw: Don't call cp_free if we are processing a channel program Farhan Ali
@ 2019-07-11 14:28 ` Farhan Ali
  2019-07-12 11:30   ` Cornelia Huck
  2019-07-15 13:07 ` [PATCH v3 0/5] Some vfio-ccw fixes Cornelia Huck
  5 siblings, 1 reply; 14+ messages in thread
From: Farhan Ali @ 2019-07-11 14:28 UTC (permalink / raw)
  To: cohuck, farman, pasic; +Cc: linux-s390, kvm, alifm

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 <alifm@linux.ibm.com>
---
 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 1f6d0b5..be2af10 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.
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/5] vfio-ccw: Fix misleading comment when setting orb.cmd.c64
  2019-07-11 14:28 ` [PATCH v3 1/5] vfio-ccw: Fix misleading comment when setting orb.cmd.c64 Farhan Ali
@ 2019-07-11 20:30   ` Eric Farman
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Farman @ 2019-07-11 20:30 UTC (permalink / raw)
  To: Farhan Ali, cohuck, pasic; +Cc: linux-s390, kvm



On 7/11/19 10:28 AM, Farhan Ali wrote:
> 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.
> 

Per Conny in v2:

Fixes: fb9e7880af35 ("vfio: ccw: push down unsupported IDA check")

> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  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 d6a8dff..c969d48 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;
>  }
>  
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/5] vfio-ccw: Fix memory leak and don't call cp_free in cp_init
  2019-07-11 14:28 ` [PATCH v3 2/5] vfio-ccw: Fix memory leak and don't call cp_free in cp_init Farhan Ali
@ 2019-07-11 20:30   ` Eric Farman
  2019-07-12 11:26   ` Cornelia Huck
  1 sibling, 0 replies; 14+ messages in thread
From: Eric Farman @ 2019-07-11 20:30 UTC (permalink / raw)
  To: Farhan Ali, cohuck, pasic; +Cc: linux-s390, kvm



On 7/11/19 10:28 AM, Farhan Ali wrote:
> 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 <alifm@linux.ibm.com>

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  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 c969d48..f862d5d 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;
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/5] vfio-ccw: Fix memory leak and don't call cp_free in cp_init
  2019-07-11 14:28 ` [PATCH v3 2/5] vfio-ccw: Fix memory leak and don't call cp_free in cp_init Farhan Ali
  2019-07-11 20:30   ` Eric Farman
@ 2019-07-12 11:26   ` Cornelia Huck
  1 sibling, 0 replies; 14+ messages in thread
From: Cornelia Huck @ 2019-07-12 11:26 UTC (permalink / raw)
  To: Farhan Ali; +Cc: farman, pasic, linux-s390, kvm

On Thu, 11 Jul 2019 10:28:52 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> 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 <alifm@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 5/5] vfio-ccw: Update documentation for csch/hsch
  2019-07-11 14:28 ` [PATCH v3 5/5] vfio-ccw: Update documentation for csch/hsch Farhan Ali
@ 2019-07-12 11:30   ` Cornelia Huck
  2019-07-12 13:27     ` Farhan Ali
  2019-07-12 13:28     ` Eric Farman
  0 siblings, 2 replies; 14+ messages in thread
From: Cornelia Huck @ 2019-07-12 11:30 UTC (permalink / raw)
  To: Farhan Ali; +Cc: farman, pasic, linux-s390, kvm

On Thu, 11 Jul 2019 10:28:55 -0400
Farhan Ali <alifm@linux.ibm.com> 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 <alifm@linux.ibm.com>
> ---
>  Documentation/s390/vfio-ccw.rst | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)

(...)

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

> +#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 <cohuck@redhat.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 4/5] vfio-ccw: Don't call cp_free if we are processing a channel program
  2019-07-11 14:28 ` [PATCH v3 4/5] vfio-ccw: Don't call cp_free if we are processing a channel program Farhan Ali
@ 2019-07-12 13:19   ` Eric Farman
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Farman @ 2019-07-12 13:19 UTC (permalink / raw)
  To: Farhan Ali, cohuck, pasic; +Cc: linux-s390, kvm



On 7/11/19 10:28 AM, Farhan Ali wrote:
> 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 <alifm@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

I think this seems like a reasonable fix.

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  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 4e3a903..0357165 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -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);
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 5/5] vfio-ccw: Update documentation for csch/hsch
  2019-07-12 11:30   ` Cornelia Huck
@ 2019-07-12 13:27     ` Farhan Ali
  2019-07-12 13:28     ` Eric Farman
  1 sibling, 0 replies; 14+ messages in thread
From: Farhan Ali @ 2019-07-12 13:27 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: farman, pasic, linux-s390, kvm



On 07/12/2019 07:30 AM, Cornelia Huck wrote:
> On Thu, 11 Jul 2019 10:28:55 -0400
> Farhan Ali <alifm@linux.ibm.com> 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 <alifm@linux.ibm.com>
>> ---
>>   Documentation/s390/vfio-ccw.rst | 31 ++++++++++++++++++++++++++++---
>>   1 file changed, 28 insertions(+), 3 deletions(-)
> 
> (...)
> 
>> +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.)

There is one other thing, I forgot to add a fixes tag to the first 
patch. If you don't mind fixing that as well that will be great :)

otherwise I could send a new round


> 
>> +#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 <cohuck@redhat.com>
> 

Thanks for reviewing it

Thanks
Farhan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 5/5] vfio-ccw: Update documentation for csch/hsch
  2019-07-12 11:30   ` Cornelia Huck
  2019-07-12 13:27     ` Farhan Ali
@ 2019-07-12 13:28     ` Eric Farman
  1 sibling, 0 replies; 14+ messages in thread
From: Eric Farman @ 2019-07-12 13:28 UTC (permalink / raw)
  To: Cornelia Huck, Farhan Ali; +Cc: pasic, linux-s390, kvm



On 7/12/19 7:30 AM, Cornelia Huck wrote:
> On Thu, 11 Jul 2019 10:28:55 -0400
> Farhan Ali <alifm@linux.ibm.com> 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 <alifm@linux.ibm.com>
>> ---
>>  Documentation/s390/vfio-ccw.rst | 31 ++++++++++++++++++++++++++++---
>>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> (...)
> 
>> +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.)
> 
>> +#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 <cohuck@redhat.com>
> 

Also good...

Reviewed-by: Eric Farman <farman@linux.ibm.com>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 0/5] Some vfio-ccw fixes
  2019-07-11 14:28 [PATCH v3 0/5] Some vfio-ccw fixes Farhan Ali
                   ` (4 preceding siblings ...)
  2019-07-11 14:28 ` [PATCH v3 5/5] vfio-ccw: Update documentation for csch/hsch Farhan Ali
@ 2019-07-15 13:07 ` Cornelia Huck
  5 siblings, 0 replies; 14+ messages in thread
From: Cornelia Huck @ 2019-07-15 13:07 UTC (permalink / raw)
  To: Farhan Ali; +Cc: farman, pasic, linux-s390, kvm

On Thu, 11 Jul 2019 10:28:50 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

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

Thanks, did the fixup for patch 5 and queued to vfio-ccw-fixes.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-07-15 13:07 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-11 14:28 [PATCH v3 0/5] Some vfio-ccw fixes Farhan Ali
2019-07-11 14:28 ` [PATCH v3 1/5] vfio-ccw: Fix misleading comment when setting orb.cmd.c64 Farhan Ali
2019-07-11 20:30   ` Eric Farman
2019-07-11 14:28 ` [PATCH v3 2/5] vfio-ccw: Fix memory leak and don't call cp_free in cp_init Farhan Ali
2019-07-11 20:30   ` Eric Farman
2019-07-12 11:26   ` Cornelia Huck
2019-07-11 14:28 ` [PATCH v3 3/5] vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn Farhan Ali
2019-07-11 14:28 ` [PATCH v3 4/5] vfio-ccw: Don't call cp_free if we are processing a channel program Farhan Ali
2019-07-12 13:19   ` Eric Farman
2019-07-11 14:28 ` [PATCH v3 5/5] vfio-ccw: Update documentation for csch/hsch Farhan Ali
2019-07-12 11:30   ` Cornelia Huck
2019-07-12 13:27     ` Farhan Ali
2019-07-12 13:28     ` Eric Farman
2019-07-15 13:07 ` [PATCH v3 0/5] Some vfio-ccw fixes Cornelia Huck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).