[RFC PATCH v1 00/10] s390/vfio-ccw: Channel Path Handling

[RFC PATCH v1 00/10] s390/vfio-ccw: Channel Path Handling

[Eric Farman]

Here is a first pass at the channel-path handling code for vfio-ccw.
This was initially developed by Farhan Ali this past summer, and
picked up by me.  For my own benefit/sanity, I made a small changelog
in the commit message for each patch, describing the changes I've
made to his original code beyond just rebasing to master.

I did split a couple of his patches, to (hopefully) make them a little
more understandable.  The entire series is based on top of the trace
rework patches from a few weeks ago, which are currently pending.
But really, the only cause for overlap is the trace patch here.
The bulk of it is really self-contained.

With this, and the corresponding QEMU series (to be posted momentarily),
applied I am able to configure off/on a CHPID (for example, by issuing
"chchp -c 0/1 xx" on the host), and the guest is able to see both the
events and reflect the updated path masks in its structures.

For reasons that are hopefully obvious, issuing chchp within the guest
only works for the logical vary.  Configuring it off/on does not work,
which I think is fine.

Eric Farman (4):
  vfio-ccw: Refactor the unregister of the async regions
  vfio-ccw: Refactor IRQ handlers
  vfio-ccw: Add trace for CRW event
  vfio-ccw: Remove inline get_schid() routine

Farhan Ali (6):
  vfio-ccw: Introduce new helper functions to free/destroy regions
  vfio-ccw: Register a chp_event callback for vfio-ccw
  vfio-ccw: Use subchannel lpm in the orb
  vfio-ccw: Introduce a new schib region
  vfio-ccw: Introduce a new CRW region
  vfio-ccw: Wire up the CRW irq and CRW region

 drivers/s390/cio/Makefile           |   2 +-
 drivers/s390/cio/vfio_ccw_chp.c     | 128 +++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_cp.c      |   4 +-
 drivers/s390/cio/vfio_ccw_drv.c     | 140 ++++++++++++++++++++++++++--
 drivers/s390/cio/vfio_ccw_fsm.c     |   8 +-
 drivers/s390/cio/vfio_ccw_ops.c     |  65 +++++++++----
 drivers/s390/cio/vfio_ccw_private.h |  11 +++
 drivers/s390/cio/vfio_ccw_trace.c   |   1 +
 drivers/s390/cio/vfio_ccw_trace.h   |  30 ++++++
 include/uapi/linux/vfio.h           |   3 +
 include/uapi/linux/vfio_ccw.h       |  10 ++
 11 files changed, 366 insertions(+), 36 deletions(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_chp.c

-- 
2.17.1

[RFC PATCH v1 01/10] vfio-ccw: Introduce new helper functions to free/destroy regions

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

Consolidate some of the cleanup code for the regions, so that
as more are added we reduce code duplication.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v0->v1: [EF]
     - Commit message

 drivers/s390/cio/vfio_ccw_drv.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index e401a3d0aa57..91989269faf1 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -116,6 +116,14 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
 }
 
+static void vfio_ccw_free_regions(struct vfio_ccw_private *private)
+{
+	if (private->cmd_region)
+		kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
+	if (private->io_region)
+		kmem_cache_free(vfio_ccw_io_region, private->io_region);
+}
+
 static int vfio_ccw_sch_probe(struct subchannel *sch)
 {
 	struct pmcw *pmcw = &sch->schib.pmcw;
@@ -176,10 +184,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	cio_disable_subchannel(sch);
 out_free:
 	dev_set_drvdata(&sch->dev, NULL);
-	if (private->cmd_region)
-		kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
-	if (private->io_region)
-		kmem_cache_free(vfio_ccw_io_region, private->io_region);
+	vfio_ccw_free_regions(private);
 	kfree(private->cp.guest_cp);
 	kfree(private);
 	return ret;
@@ -195,8 +200,7 @@ static int vfio_ccw_sch_remove(struct subchannel *sch)
 
 	dev_set_drvdata(&sch->dev, NULL);
 
-	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
-	kmem_cache_free(vfio_ccw_io_region, private->io_region);
+	vfio_ccw_free_regions(private);
 	kfree(private->cp.guest_cp);
 	kfree(private);
 
@@ -299,6 +303,12 @@ static void vfio_ccw_debug_exit(void)
 	debug_unregister(vfio_ccw_debug_trace_id);
 }
 
+static void vfio_ccw_destroy_regions(void)
+{
+	kmem_cache_destroy(vfio_ccw_cmd_region);
+	kmem_cache_destroy(vfio_ccw_io_region);
+}
+
 static int __init vfio_ccw_sch_init(void)
 {
 	int ret;
@@ -341,8 +351,7 @@ static int __init vfio_ccw_sch_init(void)
 	return ret;
 
 out_err:
-	kmem_cache_destroy(vfio_ccw_cmd_region);
-	kmem_cache_destroy(vfio_ccw_io_region);
+	vfio_ccw_destroy_regions();
 	destroy_workqueue(vfio_ccw_work_q);
 	vfio_ccw_debug_exit();
 	return ret;
@@ -352,8 +361,7 @@ static void __exit vfio_ccw_sch_exit(void)
 {
 	css_driver_unregister(&vfio_ccw_sch_driver);
 	isc_unregister(VFIO_CCW_ISC);
-	kmem_cache_destroy(vfio_ccw_io_region);
-	kmem_cache_destroy(vfio_ccw_cmd_region);
+	vfio_ccw_destroy_regions();
 	destroy_workqueue(vfio_ccw_work_q);
 	vfio_ccw_debug_exit();
 }
-- 
2.17.1

Re: [RFC PATCH v1 01/10] vfio-ccw: Introduce new helper functions to free/destroy regions

[Cornelia Huck]

On Fri, 15 Nov 2019 03:56:11 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> Consolidate some of the cleanup code for the regions, so that
> as more are added we reduce code duplication.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - Commit message
> 
>  drivers/s390/cio/vfio_ccw_drv.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)

Makes sense.

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

[RFC PATCH v1 02/10] vfio-ccw: Register a chp_event callback for vfio-ccw

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

Register the chp_event callback to receive channel path related
events for the subchannels managed by vfio-ccw.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v0->v1: [EF]
     - Add s390dbf trace

 drivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 91989269faf1..05da1facee60 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -19,6 +19,7 @@
 
 #include <asm/isc.h>
 
+#include "chp.h"
 #include "ioasm.h"
 #include "css.h"
 #include "vfio_ccw_private.h"
@@ -257,6 +258,48 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
 	return rc;
 }
 
+static int vfio_ccw_chp_event(struct subchannel *sch,
+			      struct chp_link *link, int event)
+{
+	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+	int mask = chp_ssd_get_mask(&sch->ssd_info, link);
+	int retry = 255;
+
+	if (!private || !mask)
+		return 0;
+
+	if (cio_update_schib(sch))
+		return -ENODEV;
+
+	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
+			   mdev_uuid(private->mdev), sch->schid.cssid,
+			   sch->schid.ssid, sch->schid.sch_no,
+			   mask, event);
+
+	switch (event) {
+	case CHP_VARY_OFF:
+		/* Path logically turned off */
+		sch->opm &= ~mask;
+		sch->lpm &= ~mask;
+		break;
+	case CHP_OFFLINE:
+		/* Path is gone */
+		cio_cancel_halt_clear(sch, &retry);
+		break;
+	case CHP_VARY_ON:
+		/* Path logically turned on */
+		sch->opm |= mask;
+		sch->lpm |= mask;
+		break;
+	case CHP_ONLINE:
+		/* Path became available */
+		sch->lpm |= mask & sch->opm;
+		break;
+	}
+
+	return 0;
+}
+
 static struct css_device_id vfio_ccw_sch_ids[] = {
 	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
 	{ /* end of list */ },
@@ -274,6 +317,7 @@ static struct css_driver vfio_ccw_sch_driver = {
 	.remove = vfio_ccw_sch_remove,
 	.shutdown = vfio_ccw_sch_shutdown,
 	.sch_event = vfio_ccw_sch_event,
+	.chp_event = vfio_ccw_chp_event,
 };
 
 static int __init vfio_ccw_debug_init(void)
-- 
2.17.1

Re: [RFC PATCH v1 02/10] vfio-ccw: Register a chp_event callback for vfio-ccw

[Cornelia Huck]

On Fri, 15 Nov 2019 03:56:12 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> Register the chp_event callback to receive channel path related
> events for the subchannels managed by vfio-ccw.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - Add s390dbf trace
> 
>  drivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 91989269faf1..05da1facee60 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -19,6 +19,7 @@
>  
>  #include <asm/isc.h>
>  
> +#include "chp.h"
>  #include "ioasm.h"
>  #include "css.h"
>  #include "vfio_ccw_private.h"
> @@ -257,6 +258,48 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>  	return rc;
>  }
>  
> +static int vfio_ccw_chp_event(struct subchannel *sch,
> +			      struct chp_link *link, int event)
> +{
> +	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
> +	int mask = chp_ssd_get_mask(&sch->ssd_info, link);
> +	int retry = 255;
> +
> +	if (!private || !mask)
> +		return 0;
> +
> +	if (cio_update_schib(sch))
> +		return -ENODEV;

It seems this return code is only checked by the common I/O layer for
the _OFFLINE case; still, it's probably not a bad idea, even though it
is different from what the vanilla I/O subchannel driver does.

> +
> +	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
> +			   mdev_uuid(private->mdev), sch->schid.cssid,
> +			   sch->schid.ssid, sch->schid.sch_no,
> +			   mask, event);

If you log only here, you're missing the case above.

> +
> +	switch (event) {
> +	case CHP_VARY_OFF:
> +		/* Path logically turned off */
> +		sch->opm &= ~mask;
> +		sch->lpm &= ~mask;
> +		break;
> +	case CHP_OFFLINE:
> +		/* Path is gone */
> +		cio_cancel_halt_clear(sch, &retry);
> +		break;
> +	case CHP_VARY_ON:
> +		/* Path logically turned on */
> +		sch->opm |= mask;
> +		sch->lpm |= mask;
> +		break;
> +	case CHP_ONLINE:
> +		/* Path became available */
> +		sch->lpm |= mask & sch->opm;
> +		break;
> +	}

Looks sane as the first round.

> +
> +	return 0;
> +}
> +
>  static struct css_device_id vfio_ccw_sch_ids[] = {
>  	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
>  	{ /* end of list */ },
> @@ -274,6 +317,7 @@ static struct css_driver vfio_ccw_sch_driver = {
>  	.remove = vfio_ccw_sch_remove,
>  	.shutdown = vfio_ccw_sch_shutdown,
>  	.sch_event = vfio_ccw_sch_event,
> +	.chp_event = vfio_ccw_chp_event,
>  };
>  
>  static int __init vfio_ccw_debug_init(void)

Re: [RFC PATCH v1 02/10] vfio-ccw: Register a chp_event callback for vfio-ccw

[Eric Farman]

On 11/19/19 7:48 AM, Cornelia Huck wrote:
> On Fri, 15 Nov 2019 03:56:12 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> Register the chp_event callback to receive channel path related
>> events for the subchannels managed by vfio-ccw.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v0->v1: [EF]
>>      - Add s390dbf trace
>>
>>  drivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>> index 91989269faf1..05da1facee60 100644
>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>> @@ -19,6 +19,7 @@
>>  
>>  #include <asm/isc.h>
>>  
>> +#include "chp.h"
>>  #include "ioasm.h"
>>  #include "css.h"
>>  #include "vfio_ccw_private.h"
>> @@ -257,6 +258,48 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>>  	return rc;
>>  }
>>  
>> +static int vfio_ccw_chp_event(struct subchannel *sch,
>> +			      struct chp_link *link, int event)
>> +{
>> +	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>> +	int mask = chp_ssd_get_mask(&sch->ssd_info, link);
>> +	int retry = 255;
>> +
>> +	if (!private || !mask)
>> +		return 0;
>> +
>> +	if (cio_update_schib(sch))
>> +		return -ENODEV;
> 
> It seems this return code is only checked by the common I/O layer for
> the _OFFLINE case; still, it's probably not a bad idea, even though it
> is different from what the vanilla I/O subchannel driver does.

cio_update_schib() itself can only return -ENODEV so it seemed sane.

> 
>> +
>> +	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
>> +			   mdev_uuid(private->mdev), sch->schid.cssid,
>> +			   sch->schid.ssid, sch->schid.sch_no,
>> +			   mask, event);
> 
> If you log only here, you're missing the case above.

Ah, yes.  I'll move that up.

> 
>> +
>> +	switch (event) {
>> +	case CHP_VARY_OFF:
>> +		/* Path logically turned off */
>> +		sch->opm &= ~mask;
>> +		sch->lpm &= ~mask;
>> +		break;
>> +	case CHP_OFFLINE:
>> +		/* Path is gone */
>> +		cio_cancel_halt_clear(sch, &retry);
>> +		break;
>> +	case CHP_VARY_ON:
>> +		/* Path logically turned on */
>> +		sch->opm |= mask;
>> +		sch->lpm |= mask;
>> +		break;
>> +	case CHP_ONLINE:
>> +		/* Path became available */
>> +		sch->lpm |= mask & sch->opm;
>> +		break;
>> +	}
> 
> Looks sane as the first round.
> 
>> +
>> +	return 0;
>> +}
>> +
>>  static struct css_device_id vfio_ccw_sch_ids[] = {
>>  	{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
>>  	{ /* end of list */ },
>> @@ -274,6 +317,7 @@ static struct css_driver vfio_ccw_sch_driver = {
>>  	.remove = vfio_ccw_sch_remove,
>>  	.shutdown = vfio_ccw_sch_shutdown,
>>  	.sch_event = vfio_ccw_sch_event,
>> +	.chp_event = vfio_ccw_chp_event,
>>  };
>>  
>>  static int __init vfio_ccw_debug_init(void)
> 

[RFC PATCH v1 03/10] vfio-ccw: Use subchannel lpm in the orb

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

The subchannel logical path mask (lpm) would have the most
up to date information of channel paths that are logically
available for the subchannel.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v0->v1: [EF]
     - None; however I am greatly confused by this one.  Thoughts?

 drivers/s390/cio/vfio_ccw_cp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 3645d1720c4b..d4a86fb9d162 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -779,9 +779,7 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
 	orb->cmd.intparm = intparm;
 	orb->cmd.fmt = 1;
 	orb->cmd.key = PAGE_DEFAULT_KEY >> 4;
-
-	if (orb->cmd.lpm == 0)
-		orb->cmd.lpm = lpm;
+	orb->cmd.lpm = lpm;
 
 	chain = list_first_entry(&cp->ccwchain_list, struct ccwchain, next);
 	cpa = chain->ch_ccw;
-- 
2.17.1

Re: [RFC PATCH v1 03/10] vfio-ccw: Use subchannel lpm in the orb

[Cornelia Huck]

On Fri, 15 Nov 2019 03:56:13 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> The subchannel logical path mask (lpm) would have the most
> up to date information of channel paths that are logically
> available for the subchannel.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - None; however I am greatly confused by this one.  Thoughts?

I think it's actually wrong.

> 
>  drivers/s390/cio/vfio_ccw_cp.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 3645d1720c4b..d4a86fb9d162 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -779,9 +779,7 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
>  	orb->cmd.intparm = intparm;
>  	orb->cmd.fmt = 1;
>  	orb->cmd.key = PAGE_DEFAULT_KEY >> 4;
> -
> -	if (orb->cmd.lpm == 0)
> -		orb->cmd.lpm = lpm;

In the end, the old code will use the lpm from the subchannel
structure, if userspace did not supply anything to be used.

> +	orb->cmd.lpm = lpm;

The new code will always discard any lpm userspace has supplied and
replace it with the lpm from the subchannel structure. This feels
wrong; what if the I/O is supposed to be restricted to a subset of the
paths?

If we want to include the current value of the subchannel lpm in the
requests, we probably want to AND the masks instead.

>  
>  	chain = list_first_entry(&cp->ccwchain_list, struct ccwchain, next);
>  	cpa = chain->ch_ccw;

Re: [RFC PATCH v1 03/10] vfio-ccw: Use subchannel lpm in the orb

[Eric Farman]

On 11/19/19 8:00 AM, Cornelia Huck wrote:
> On Fri, 15 Nov 2019 03:56:13 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> The subchannel logical path mask (lpm) would have the most
>> up to date information of channel paths that are logically
>> available for the subchannel.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v0->v1: [EF]
>>      - None; however I am greatly confused by this one.  Thoughts?
> 
> I think it's actually wrong.
> 
>>
>>  drivers/s390/cio/vfio_ccw_cp.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>> index 3645d1720c4b..d4a86fb9d162 100644
>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>> @@ -779,9 +779,7 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
>>  	orb->cmd.intparm = intparm;
>>  	orb->cmd.fmt = 1;
>>  	orb->cmd.key = PAGE_DEFAULT_KEY >> 4;
>> -
>> -	if (orb->cmd.lpm == 0)
>> -		orb->cmd.lpm = lpm;
> 
> In the end, the old code will use the lpm from the subchannel
> structure, if userspace did not supply anything to be used.
> 
>> +	orb->cmd.lpm = lpm;
> 
> The new code will always discard any lpm userspace has supplied and
> replace it with the lpm from the subchannel structure. This feels
> wrong; what if the I/O is supposed to be restricted to a subset of the
> paths?

I had the same opinion, but didn't want to flat-out discard it from his
series without a second look.  :)

> 
> If we want to include the current value of the subchannel lpm in the
> requests, we probably want to AND the masks instead.

Then we'd be on the hook to return some sort of error if the result is
zero.  Is it better to just send it to hw as-is, and let the response
come back naturally?  (Which is what we do today.)

> 
>>  
>>  	chain = list_first_entry(&cp->ccwchain_list, struct ccwchain, next);
>>  	cpa = chain->ch_ccw;
> 

Re: [RFC PATCH v1 03/10] vfio-ccw: Use subchannel lpm in the orb

[Cornelia Huck]

On Tue, 19 Nov 2019 10:16:30 -0500
Eric Farman <farman@linux.ibm.com> wrote:

> On 11/19/19 8:00 AM, Cornelia Huck wrote:
> > On Fri, 15 Nov 2019 03:56:13 +0100
> > Eric Farman <farman@linux.ibm.com> wrote:
> >   
> >> From: Farhan Ali <alifm@linux.ibm.com>
> >>
> >> The subchannel logical path mask (lpm) would have the most
> >> up to date information of channel paths that are logically
> >> available for the subchannel.
> >>
> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> >> ---
> >>
> >> Notes:
> >>     v0->v1: [EF]
> >>      - None; however I am greatly confused by this one.  Thoughts?  
> > 
> > I think it's actually wrong.
> >   
> >>
> >>  drivers/s390/cio/vfio_ccw_cp.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> >> index 3645d1720c4b..d4a86fb9d162 100644
> >> --- a/drivers/s390/cio/vfio_ccw_cp.c
> >> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> >> @@ -779,9 +779,7 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
> >>  	orb->cmd.intparm = intparm;
> >>  	orb->cmd.fmt = 1;
> >>  	orb->cmd.key = PAGE_DEFAULT_KEY >> 4;
> >> -
> >> -	if (orb->cmd.lpm == 0)
> >> -		orb->cmd.lpm = lpm;  
> > 
> > In the end, the old code will use the lpm from the subchannel
> > structure, if userspace did not supply anything to be used.
> >   
> >> +	orb->cmd.lpm = lpm;  
> > 
> > The new code will always discard any lpm userspace has supplied and
> > replace it with the lpm from the subchannel structure. This feels
> > wrong; what if the I/O is supposed to be restricted to a subset of the
> > paths?  
> 
> I had the same opinion, but didn't want to flat-out discard it from his
> series without a second look.  :)

:)

> 
> > 
> > If we want to include the current value of the subchannel lpm in the
> > requests, we probably want to AND the masks instead.  
> 
> Then we'd be on the hook to return some sort of error if the result is
> zero.  Is it better to just send it to hw as-is, and let the response
> come back naturally?  (Which is what we do today.)

But if a chpid is logically varied off, it is removed from the lpm,
right? Therefore, the caller really should get a 'no path' indication
back, shouldn't it?

> 
> >   
> >>  
> >>  	chain = list_first_entry(&cp->ccwchain_list, struct ccwchain, next);
> >>  	cpa = chain->ch_ccw;  
> >   
> 

Re: [RFC PATCH v1 03/10] vfio-ccw: Use subchannel lpm in the orb

[Eric Farman]

On 11/19/19 10:38 AM, Cornelia Huck wrote:
> On Tue, 19 Nov 2019 10:16:30 -0500
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> On 11/19/19 8:00 AM, Cornelia Huck wrote:
>>> On Fri, 15 Nov 2019 03:56:13 +0100
>>> Eric Farman <farman@linux.ibm.com> wrote:
>>>   
>>>> From: Farhan Ali <alifm@linux.ibm.com>
>>>>
>>>> The subchannel logical path mask (lpm) would have the most
>>>> up to date information of channel paths that are logically
>>>> available for the subchannel.
>>>>
>>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>>> ---
>>>>
>>>> Notes:
>>>>     v0->v1: [EF]
>>>>      - None; however I am greatly confused by this one.  Thoughts?  
>>>
>>> I think it's actually wrong.
>>>   
>>>>
>>>>  drivers/s390/cio/vfio_ccw_cp.c | 4 +---
>>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>>>> index 3645d1720c4b..d4a86fb9d162 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>>>> @@ -779,9 +779,7 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
>>>>  	orb->cmd.intparm = intparm;
>>>>  	orb->cmd.fmt = 1;
>>>>  	orb->cmd.key = PAGE_DEFAULT_KEY >> 4;
>>>> -
>>>> -	if (orb->cmd.lpm == 0)
>>>> -		orb->cmd.lpm = lpm;  
>>>
>>> In the end, the old code will use the lpm from the subchannel
>>> structure, if userspace did not supply anything to be used.
>>>   
>>>> +	orb->cmd.lpm = lpm;  
>>>
>>> The new code will always discard any lpm userspace has supplied and
>>> replace it with the lpm from the subchannel structure. This feels
>>> wrong; what if the I/O is supposed to be restricted to a subset of the
>>> paths?  
>>
>> I had the same opinion, but didn't want to flat-out discard it from his
>> series without a second look.  :)
> 
> :)
> 
>>
>>>
>>> If we want to include the current value of the subchannel lpm in the
>>> requests, we probably want to AND the masks instead.  
>>
>> Then we'd be on the hook to return some sort of error if the result is
>> zero.  Is it better to just send it to hw as-is, and let the response
>> come back naturally?  (Which is what we do today.)
> 
> But if a chpid is logically varied off, it is removed from the lpm,
> right? Therefore, the caller really should get a 'no path' indication
> back, shouldn't it?

Agreed, just don't know whether we should be kicking it out here, versus
just doing what we do today and sending the guest LPM to the hardware.

The routine being modified here is cp_get_orb(), which doesn't seem like
the right place for such a check.  It does currently return NULL when
the cp is not initialized, claiming an error in the caller.  Not sure
what happens if we start doing that when a path goes away unexpectedly.

I'll poke around and see what happens if we do that, and if I can drive
some path-directed I/O easily.

> 
>>
>>>   
>>>>  
>>>>  	chain = list_first_entry(&cp->ccwchain_list, struct ccwchain, next);
>>>>  	cpa = chain->ch_ccw;  
>>>   
>>
> 

[RFC PATCH v1 04/10] vfio-ccw: Refactor the unregister of the async regions

[Eric Farman]

This is mostly for the purposes of a later patch, since
we'll need to do the same thing later.

While we are at it, move the resulting function call to ahead
of the unregistering of the IOMMU notifier, so that it's done
in the reverse order of how it was created.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c     | 20 ++++++++++++--------
 drivers/s390/cio/vfio_ccw_private.h |  1 +
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index f0d71ab77c50..d4fc84b8867f 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -181,7 +181,6 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
 {
 	struct vfio_ccw_private *private =
 		dev_get_drvdata(mdev_parent_dev(mdev));
-	int i;
 
 	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
 	    (private->state != VFIO_CCW_STATE_STANDBY)) {
@@ -191,15 +190,9 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
 	}
 
 	cp_free(&private->cp);
+	vfio_ccw_unregister_dev_regions(private);
 	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_io_region(struct vfio_ccw_private *private,
@@ -482,6 +475,17 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
 	return 0;
 }
 
+void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private)
+{
+	int i;
+
+	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_ioctl(struct mdev_device *mdev,
 				   unsigned int cmd,
 				   unsigned long arg)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 9b9bb4982972..ce3834159d98 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -53,6 +53,7 @@ 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);
+void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private);
 
 int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
 
-- 
2.17.1

Re: [RFC PATCH v1 04/10] vfio-ccw: Refactor the unregister of the async regions

[Cornelia Huck]

On Fri, 15 Nov 2019 03:56:14 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> This is mostly for the purposes of a later patch, since
> we'll need to do the same thing later.
> 
> While we are at it, move the resulting function call to ahead
> of the unregistering of the IOMMU notifier, so that it's done
> in the reverse order of how it was created.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_ops.c     | 20 ++++++++++++--------
>  drivers/s390/cio/vfio_ccw_private.h |  1 +
>  2 files changed, 13 insertions(+), 8 deletions(-)

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

[RFC PATCH v1 05/10] vfio-ccw: Introduce a new schib region

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

The schib region can be used by userspace to get the SCHIB for the
passthrough subchannel. This can be useful to get information such
as channel path information via the SCHIB.PMCW.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v0->v1: [EF]
     - Clean up checkpatch (#include, whitespace) errors
     - Remove unnecessary includes from vfio_ccw_chp.c
     - Add ret=-ENOMEM in error path for new region
     - Add call to vfio_ccw_unregister_dev_regions() during error exit
       path of vfio_ccw_mdev_open()
     - New info on the module prologue
     - Reorder cleanup of regions

 drivers/s390/cio/Makefile           |  2 +-
 drivers/s390/cio/vfio_ccw_chp.c     | 75 +++++++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_drv.c     | 20 ++++++++
 drivers/s390/cio/vfio_ccw_ops.c     | 14 +++++-
 drivers/s390/cio/vfio_ccw_private.h |  3 ++
 include/uapi/linux/vfio.h           |  1 +
 include/uapi/linux/vfio_ccw.h       |  5 ++
 7 files changed, 117 insertions(+), 3 deletions(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_chp.c

diff --git a/drivers/s390/cio/Makefile b/drivers/s390/cio/Makefile
index 23eae4188876..a9235f111e79 100644
--- a/drivers/s390/cio/Makefile
+++ b/drivers/s390/cio/Makefile
@@ -21,5 +21,5 @@ qdio-objs := qdio_main.o qdio_thinint.o qdio_debug.o qdio_setup.o
 obj-$(CONFIG_QDIO) += qdio.o
 
 vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o \
-	vfio_ccw_async.o vfio_ccw_trace.o
+	vfio_ccw_async.o vfio_ccw_trace.o vfio_ccw_chp.o
 obj-$(CONFIG_VFIO_CCW) += vfio_ccw.o
diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
new file mode 100644
index 000000000000..826d08379fe3
--- /dev/null
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Channel path related status regions for vfio_ccw
+ *
+ * Copyright IBM Corp. 2019
+ *
+ * Author(s): Farhan Ali <alifm@linux.ibm.com>
+ */
+
+#include <linux/vfio.h>
+#include "vfio_ccw_private.h"
+
+static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
+					  char __user *buf, size_t count,
+					  loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_schib_region *region;
+	int ret;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	mutex_lock(&private->io_mutex);
+	region = private->region[i].data;
+
+	if (cio_update_schib(private->sch)) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	memcpy(region, &private->sch->schib, sizeof(*region));
+
+	if (copy_to_user(buf, (void *)region + pos, count)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = count;
+
+out:
+	mutex_unlock(&private->io_mutex);
+	return ret;
+}
+
+static ssize_t vfio_ccw_schib_region_write(struct vfio_ccw_private *private,
+					   const char __user *buf, size_t count,
+					   loff_t *ppos)
+{
+	return -EINVAL;
+}
+
+
+static void vfio_ccw_schib_region_release(struct vfio_ccw_private *private,
+					  struct vfio_ccw_region *region)
+{
+
+}
+
+const struct vfio_ccw_regops vfio_ccw_schib_region_ops = {
+	.read = vfio_ccw_schib_region_read,
+	.write = vfio_ccw_schib_region_write,
+	.release = vfio_ccw_schib_region_release,
+};
+
+int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
+{
+	return vfio_ccw_register_dev_region(private,
+					    VFIO_REGION_SUBTYPE_CCW_SCHIB,
+					    &vfio_ccw_schib_region_ops,
+					    sizeof(struct ccw_schib_region),
+					    VFIO_REGION_INFO_FLAG_READ,
+					    private->schib_region);
+}
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 05da1facee60..de3a09d79ae1 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -27,6 +27,7 @@
 struct workqueue_struct *vfio_ccw_work_q;
 static struct kmem_cache *vfio_ccw_io_region;
 static struct kmem_cache *vfio_ccw_cmd_region;
+static struct kmem_cache *vfio_ccw_schib_region;
 
 debug_info_t *vfio_ccw_debug_msg_id;
 debug_info_t *vfio_ccw_debug_trace_id;
@@ -119,6 +120,8 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
 
 static void vfio_ccw_free_regions(struct vfio_ccw_private *private)
 {
+	if (private->schib_region)
+		kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
 	if (private->cmd_region)
 		kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
 	if (private->io_region)
@@ -156,6 +159,12 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	if (!private->cmd_region)
 		goto out_free;
 
+	private->schib_region = kmem_cache_zalloc(vfio_ccw_schib_region,
+						  GFP_KERNEL | GFP_DMA);
+
+	if (!private->schib_region)
+		goto out_free;
+
 	private->sch = sch;
 	dev_set_drvdata(&sch->dev, private);
 	mutex_init(&private->io_mutex);
@@ -349,6 +358,7 @@ static void vfio_ccw_debug_exit(void)
 
 static void vfio_ccw_destroy_regions(void)
 {
+	kmem_cache_destroy(vfio_ccw_schib_region);
 	kmem_cache_destroy(vfio_ccw_cmd_region);
 	kmem_cache_destroy(vfio_ccw_io_region);
 }
@@ -385,6 +395,16 @@ static int __init vfio_ccw_sch_init(void)
 		goto out_err;
 	}
 
+	vfio_ccw_schib_region = kmem_cache_create_usercopy("vfio_ccw_schib_region",
+					sizeof(struct ccw_schib_region), 0,
+					SLAB_ACCOUNT, 0,
+					sizeof(struct ccw_schib_region), NULL);
+
+	if (!vfio_ccw_schib_region) {
+		ret = -ENOMEM;
+		goto out_err;
+	}
+
 	isc_register(VFIO_CCW_ISC);
 	ret = css_driver_register(&vfio_ccw_sch_driver);
 	if (ret) {
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index d4fc84b8867f..22988d67b6bb 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -172,8 +172,18 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
 
 	ret = vfio_ccw_register_async_dev_regions(private);
 	if (ret)
-		vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
-					 &private->nb);
+		goto out_unregister;
+
+	ret = vfio_ccw_register_schib_dev_regions(private);
+	if (ret)
+		goto out_unregister;
+
+	return ret;
+
+out_unregister:
+	vfio_ccw_unregister_dev_regions(private);
+	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+				 &private->nb);
 	return ret;
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index ce3834159d98..d6601a8adf13 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -56,6 +56,7 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
 void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private);
 
 int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
+int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private);
 
 /**
  * struct vfio_ccw_private
@@ -69,6 +70,7 @@ int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
  * @io_mutex: protect against concurrent update of I/O regions
  * @region: additional regions for other subchannel operations
  * @cmd_region: MMIO region for asynchronous I/O commands other than START
+ * @schib_region: MMIO region for SCHIB information
  * @num_regions: number of additional regions
  * @cp: channel program for the current I/O operation
  * @irb: irb info received from interrupt
@@ -87,6 +89,7 @@ struct vfio_ccw_private {
 	struct mutex		io_mutex;
 	struct vfio_ccw_region *region;
 	struct ccw_cmd_region	*cmd_region;
+	struct ccw_schib_region *schib_region;
 	int num_regions;
 
 	struct channel_program	cp;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 9e843a147ead..b108e2795ea3 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -378,6 +378,7 @@ struct vfio_region_gfx_edid {
 
 /* sub-types for VFIO_REGION_TYPE_CCW */
 #define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD	(1)
+#define VFIO_REGION_SUBTYPE_CCW_SCHIB		(2)
 
 /*
  * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
index cbecbf0cd54f..7c0a834e5d7a 100644
--- a/include/uapi/linux/vfio_ccw.h
+++ b/include/uapi/linux/vfio_ccw.h
@@ -34,4 +34,9 @@ struct ccw_cmd_region {
 	__u32 ret_code;
 } __packed;
 
+struct ccw_schib_region {
+#define SCHIB_AREA_SIZE 52
+	__u8 schib_area[SCHIB_AREA_SIZE];
+} __packed;
+
 #endif
-- 
2.17.1

Re: [RFC PATCH v1 05/10] vfio-ccw: Introduce a new schib region

[Cornelia Huck]

On Fri, 15 Nov 2019 03:56:15 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> The schib region can be used by userspace to get the SCHIB for the
> passthrough subchannel. This can be useful to get information such
> as channel path information via the SCHIB.PMCW.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - Clean up checkpatch (#include, whitespace) errors
>      - Remove unnecessary includes from vfio_ccw_chp.c
>      - Add ret=-ENOMEM in error path for new region
>      - Add call to vfio_ccw_unregister_dev_regions() during error exit
>        path of vfio_ccw_mdev_open()
>      - New info on the module prologue
>      - Reorder cleanup of regions
> 
>  drivers/s390/cio/Makefile           |  2 +-
>  drivers/s390/cio/vfio_ccw_chp.c     | 75 +++++++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_drv.c     | 20 ++++++++
>  drivers/s390/cio/vfio_ccw_ops.c     | 14 +++++-
>  drivers/s390/cio/vfio_ccw_private.h |  3 ++
>  include/uapi/linux/vfio.h           |  1 +
>  include/uapi/linux/vfio_ccw.h       |  5 ++
>  7 files changed, 117 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/s390/cio/vfio_ccw_chp.c
> 

> diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
> index cbecbf0cd54f..7c0a834e5d7a 100644
> --- a/include/uapi/linux/vfio_ccw.h
> +++ b/include/uapi/linux/vfio_ccw.h
> @@ -34,4 +34,9 @@ struct ccw_cmd_region {
>  	__u32 ret_code;
>  } __packed;
>

Let's add a comment:
- that reading this region triggers a stsch()
- that this region is guarded by a capability

?
  
> +struct ccw_schib_region {
> +#define SCHIB_AREA_SIZE 52
> +	__u8 schib_area[SCHIB_AREA_SIZE];
> +} __packed;
> +
>  #endif

Seems sane; but I need to continue reading this and the QEMU series to
see how it is used.

Oh, and please update Documentation/s390/vfio-ccw.rst :)

Re: [RFC PATCH v1 05/10] vfio-ccw: Introduce a new schib region

[Eric Farman]

On 11/19/19 11:52 AM, Cornelia Huck wrote:
> On Fri, 15 Nov 2019 03:56:15 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> The schib region can be used by userspace to get the SCHIB for the
>> passthrough subchannel. This can be useful to get information such
>> as channel path information via the SCHIB.PMCW.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v0->v1: [EF]
>>      - Clean up checkpatch (#include, whitespace) errors
>>      - Remove unnecessary includes from vfio_ccw_chp.c
>>      - Add ret=-ENOMEM in error path for new region
>>      - Add call to vfio_ccw_unregister_dev_regions() during error exit
>>        path of vfio_ccw_mdev_open()
>>      - New info on the module prologue
>>      - Reorder cleanup of regions
>>
>>  drivers/s390/cio/Makefile           |  2 +-
>>  drivers/s390/cio/vfio_ccw_chp.c     | 75 +++++++++++++++++++++++++++++
>>  drivers/s390/cio/vfio_ccw_drv.c     | 20 ++++++++
>>  drivers/s390/cio/vfio_ccw_ops.c     | 14 +++++-
>>  drivers/s390/cio/vfio_ccw_private.h |  3 ++
>>  include/uapi/linux/vfio.h           |  1 +
>>  include/uapi/linux/vfio_ccw.h       |  5 ++
>>  7 files changed, 117 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/s390/cio/vfio_ccw_chp.c
>>
> 
>> diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
>> index cbecbf0cd54f..7c0a834e5d7a 100644
>> --- a/include/uapi/linux/vfio_ccw.h
>> +++ b/include/uapi/linux/vfio_ccw.h
>> @@ -34,4 +34,9 @@ struct ccw_cmd_region {
>>  	__u32 ret_code;
>>  } __packed;
>>
> 
> Let's add a comment:
> - that reading this region triggers a stsch()
> - that this region is guarded by a capability
> 
> ?

Agreed, and ditto for patch 6.

>   
>> +struct ccw_schib_region {
>> +#define SCHIB_AREA_SIZE 52
>> +	__u8 schib_area[SCHIB_AREA_SIZE];
>> +} __packed;
>> +
>>  #endif
> 
> Seems sane; but I need to continue reading this and the QEMU series to
> see how it is used.
> 
> Oh, and please update Documentation/s390/vfio-ccw.rst :)
> 

Whoops!  Yes, I'll do that here and in patch 6.

[RFC PATCH v1 06/10] vfio-ccw: Introduce a new CRW region

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

This region can be used by userspace to get channel report
words from vfio-ccw driver.

We provide space for two CRWs, per the limit in the
base driver (see crw_collect_info()).

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v0->v1: [EF]
     - Clean up checkpatch (whitespace) errors
     - Add ret=-ENOMEM in error path for new region
     - Add io_mutex for region read (originally in last patch)
     - Change crw1/crw2 to crw0/crw1
     - Reorder cleanup of regions

 drivers/s390/cio/vfio_ccw_chp.c     | 53 +++++++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_drv.c     | 20 +++++++++++
 drivers/s390/cio/vfio_ccw_ops.c     |  4 +++
 drivers/s390/cio/vfio_ccw_private.h |  3 ++
 include/uapi/linux/vfio.h           |  1 +
 include/uapi/linux/vfio_ccw.h       |  5 +++
 6 files changed, 86 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index 826d08379fe3..d1e8bfef06be 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -73,3 +73,56 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
 					    VFIO_REGION_INFO_FLAG_READ,
 					    private->schib_region);
 }
+
+static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
+					char __user *buf, size_t count,
+					loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_crw_region *region;
+	int ret;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	mutex_lock(&private->io_mutex);
+	region = private->region[i].data;
+
+	if (copy_to_user(buf, (void *)region + pos, count))
+		ret = -EFAULT;
+	else
+		ret = count;
+
+	mutex_unlock(&private->io_mutex);
+	return ret;
+}
+
+static ssize_t vfio_ccw_crw_region_write(struct vfio_ccw_private *private,
+					 const char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	return -EINVAL;
+}
+
+static void vfio_ccw_crw_region_release(struct vfio_ccw_private *private,
+					struct vfio_ccw_region *region)
+{
+
+}
+
+const struct vfio_ccw_regops vfio_ccw_crw_region_ops = {
+	.read = vfio_ccw_crw_region_read,
+	.write = vfio_ccw_crw_region_write,
+	.release = vfio_ccw_crw_region_release,
+};
+
+int vfio_ccw_register_crw_dev_regions(struct vfio_ccw_private *private)
+{
+	return vfio_ccw_register_dev_region(private,
+					    VFIO_REGION_SUBTYPE_CCW_CRW,
+					    &vfio_ccw_crw_region_ops,
+					    sizeof(struct ccw_crw_region),
+					    VFIO_REGION_INFO_FLAG_READ,
+					    private->crw_region);
+}
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index de3a09d79ae1..d1b9020d037b 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -28,6 +28,7 @@ struct workqueue_struct *vfio_ccw_work_q;
 static struct kmem_cache *vfio_ccw_io_region;
 static struct kmem_cache *vfio_ccw_cmd_region;
 static struct kmem_cache *vfio_ccw_schib_region;
+static struct kmem_cache *vfio_ccw_crw_region;
 
 debug_info_t *vfio_ccw_debug_msg_id;
 debug_info_t *vfio_ccw_debug_trace_id;
@@ -120,6 +121,8 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
 
 static void vfio_ccw_free_regions(struct vfio_ccw_private *private)
 {
+	if (private->crw_region)
+		kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
 	if (private->schib_region)
 		kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
 	if (private->cmd_region)
@@ -165,6 +168,12 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	if (!private->schib_region)
 		goto out_free;
 
+	private->crw_region = kmem_cache_zalloc(vfio_ccw_crw_region,
+						GFP_KERNEL | GFP_DMA);
+
+	if (!private->crw_region)
+		goto out_free;
+
 	private->sch = sch;
 	dev_set_drvdata(&sch->dev, private);
 	mutex_init(&private->io_mutex);
@@ -358,6 +367,7 @@ static void vfio_ccw_debug_exit(void)
 
 static void vfio_ccw_destroy_regions(void)
 {
+	kmem_cache_destroy(vfio_ccw_crw_region);
 	kmem_cache_destroy(vfio_ccw_schib_region);
 	kmem_cache_destroy(vfio_ccw_cmd_region);
 	kmem_cache_destroy(vfio_ccw_io_region);
@@ -405,6 +415,16 @@ static int __init vfio_ccw_sch_init(void)
 		goto out_err;
 	}
 
+	vfio_ccw_crw_region = kmem_cache_create_usercopy("vfio_ccw_crw_region",
+					sizeof(struct ccw_crw_region), 0,
+					SLAB_ACCOUNT, 0,
+					sizeof(struct ccw_crw_region), NULL);
+
+	if (!vfio_ccw_crw_region) {
+		ret = -ENOMEM;
+		goto out_err;
+	}
+
 	isc_register(VFIO_CCW_ISC);
 	ret = css_driver_register(&vfio_ccw_sch_driver);
 	if (ret) {
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 22988d67b6bb..edec0fb7ace8 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -178,6 +178,10 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
 	if (ret)
 		goto out_unregister;
 
+	ret = vfio_ccw_register_crw_dev_regions(private);
+	if (ret)
+		goto out_unregister;
+
 	return ret;
 
 out_unregister:
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index d6601a8adf13..8289b6850e59 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -57,6 +57,7 @@ void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private);
 
 int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
 int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private);
+int vfio_ccw_register_crw_dev_regions(struct vfio_ccw_private *private);
 
 /**
  * struct vfio_ccw_private
@@ -71,6 +72,7 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private);
  * @region: additional regions for other subchannel operations
  * @cmd_region: MMIO region for asynchronous I/O commands other than START
  * @schib_region: MMIO region for SCHIB information
+ * @crw_region: MMIO region for getting channel report words
  * @num_regions: number of additional regions
  * @cp: channel program for the current I/O operation
  * @irb: irb info received from interrupt
@@ -90,6 +92,7 @@ struct vfio_ccw_private {
 	struct vfio_ccw_region *region;
 	struct ccw_cmd_region	*cmd_region;
 	struct ccw_schib_region *schib_region;
+	struct ccw_crw_region	*crw_region;
 	int num_regions;
 
 	struct channel_program	cp;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index b108e2795ea3..5024636d7615 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -379,6 +379,7 @@ struct vfio_region_gfx_edid {
 /* sub-types for VFIO_REGION_TYPE_CCW */
 #define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD	(1)
 #define VFIO_REGION_SUBTYPE_CCW_SCHIB		(2)
+#define VFIO_REGION_SUBTYPE_CCW_CRW		(3)
 
 /*
  * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
index 7c0a834e5d7a..88b125aad279 100644
--- a/include/uapi/linux/vfio_ccw.h
+++ b/include/uapi/linux/vfio_ccw.h
@@ -39,4 +39,9 @@ struct ccw_schib_region {
 	__u8 schib_area[SCHIB_AREA_SIZE];
 } __packed;
 
+struct ccw_crw_region {
+	__u32 crw0;
+	__u32 crw1;
+} __packed;
+
 #endif
-- 
2.17.1

Re: [RFC PATCH v1 06/10] vfio-ccw: Introduce a new CRW region

[Cornelia Huck]

On Fri, 15 Nov 2019 03:56:16 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> This region can be used by userspace to get channel report
> words from vfio-ccw driver.

I think this needs a bit more explanation; this is for channel report
words concerning vfio-ccw devices that are supposed to be relayed to
the guest, IIUC?

> 
> We provide space for two CRWs, per the limit in the
> base driver (see crw_collect_info()).

That rationale seems a bit sketchy.

As far as I know, current systems provide at most two crws chained
together for an event (one for the ssid + one for the subchannel id in
case of a subchannel event, one crw for other events); and that's the
reason why we provide space for two crws (unless there's something
upcoming which would need three crws chained together?)

> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - Clean up checkpatch (whitespace) errors
>      - Add ret=-ENOMEM in error path for new region
>      - Add io_mutex for region read (originally in last patch)
>      - Change crw1/crw2 to crw0/crw1
>      - Reorder cleanup of regions
> 
>  drivers/s390/cio/vfio_ccw_chp.c     | 53 +++++++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_drv.c     | 20 +++++++++++
>  drivers/s390/cio/vfio_ccw_ops.c     |  4 +++
>  drivers/s390/cio/vfio_ccw_private.h |  3 ++
>  include/uapi/linux/vfio.h           |  1 +
>  include/uapi/linux/vfio_ccw.h       |  5 +++
>  6 files changed, 86 insertions(+)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 826d08379fe3..d1e8bfef06be 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -73,3 +73,56 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
>  					    VFIO_REGION_INFO_FLAG_READ,
>  					    private->schib_region);
>  }
> +
> +static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
> +					char __user *buf, size_t count,
> +					loff_t *ppos)
> +{
> +	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
> +	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
> +	struct ccw_crw_region *region;
> +	int ret;
> +
> +	if (pos + count > sizeof(*region))
> +		return -EINVAL;
> +
> +	mutex_lock(&private->io_mutex);
> +	region = private->region[i].data;
> +
> +	if (copy_to_user(buf, (void *)region + pos, count))
> +		ret = -EFAULT;
> +	else
> +		ret = count;
> +

Can userspace read the same crw(s) multiple times? How can it find out
if there's something new in there?

> +	mutex_unlock(&private->io_mutex);
> +	return ret;
> +}
> +

(...)

> diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
> index 7c0a834e5d7a..88b125aad279 100644
> --- a/include/uapi/linux/vfio_ccw.h
> +++ b/include/uapi/linux/vfio_ccw.h
> @@ -39,4 +39,9 @@ struct ccw_schib_region {
>  	__u8 schib_area[SCHIB_AREA_SIZE];
>  } __packed;
>

I think this one wants an explaining comment as well.
  
> +struct ccw_crw_region {
> +	__u32 crw0;
> +	__u32 crw1;
> +} __packed;
> +
>  #endif

[RFC PATCH v1 07/10] vfio-ccw: Refactor IRQ handlers

[Eric Farman]

To simplify future expansion.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index edec0fb7ace8..f3033f8fc96d 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -391,17 +391,21 @@ static int vfio_ccw_mdev_get_region_info(struct vfio_region_info *info,
 
 static int vfio_ccw_mdev_get_irq_info(struct vfio_irq_info *info)
 {
-	if (info->index != VFIO_CCW_IO_IRQ_INDEX)
+	switch (info->index) {
+	case VFIO_CCW_IO_IRQ_INDEX:
+		info->count = 1;
+		info->flags = VFIO_IRQ_INFO_EVENTFD;
+		break;
+	default:
 		return -EINVAL;
-
-	info->count = 1;
-	info->flags = VFIO_IRQ_INFO_EVENTFD;
+	}
 
 	return 0;
 }
 
 static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
 				  uint32_t flags,
+				  uint32_t index,
 				  void __user *data)
 {
 	struct vfio_ccw_private *private;
@@ -411,7 +415,14 @@ static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
 		return -EINVAL;
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
-	ctx = &private->io_trigger;
+
+	switch (index) {
+	case VFIO_CCW_IO_IRQ_INDEX:
+		ctx = &private->io_trigger;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
 	case VFIO_IRQ_SET_DATA_NONE:
@@ -583,7 +594,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
 			return ret;
 
 		data = (void __user *)(arg + minsz);
-		return vfio_ccw_mdev_set_irqs(mdev, hdr.flags, data);
+		return vfio_ccw_mdev_set_irqs(mdev, hdr.flags, hdr.index, data);
 	}
 	case VFIO_DEVICE_RESET:
 		return vfio_ccw_mdev_reset(mdev);
-- 
2.17.1

Re: [RFC PATCH v1 07/10] vfio-ccw: Refactor IRQ handlers

[Cornelia Huck]

On Fri, 15 Nov 2019 03:56:17 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> To simplify future expansion.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_ops.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)

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

[RFC PATCH v1 08/10] vfio-ccw: Wire up the CRW irq and CRW region

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

Use an IRQ to notify userspace that there is a CRW
pending in the region, related to path-availability
changes on the passthrough subchannel.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v0->v1: [EF]
     - Place the non-refactoring changes from the previous patch here
     - Clean up checkpatch (whitespace) errors
     - s/chp_crw/crw/
     - Move acquire/release of io_mutex in vfio_ccw_crw_region_read()
       into patch that introduces that region
     - Remove duplicate include from vfio_ccw_drv.c
     - Reorder include in vfio_ccw_private.h

 drivers/s390/cio/vfio_ccw_drv.c     | 27 +++++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_ops.c     |  4 ++++
 drivers/s390/cio/vfio_ccw_private.h |  4 ++++
 include/uapi/linux/vfio.h           |  1 +
 4 files changed, 36 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index d1b9020d037b..ab20c32e5319 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -108,6 +108,22 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 		eventfd_signal(private->io_trigger, 1);
 }
 
+static void vfio_ccw_crw_todo(struct work_struct *work)
+{
+	struct vfio_ccw_private *private;
+	struct crw *crw;
+
+	private = container_of(work, struct vfio_ccw_private, crw_work);
+	crw = &private->crw;
+
+	mutex_lock(&private->io_mutex);
+	memcpy(&private->crw_region->crw0, crw, sizeof(*crw));
+	mutex_unlock(&private->io_mutex);
+
+	if (private->crw_trigger)
+		eventfd_signal(private->crw_trigger, 1);
+}
+
 /*
  * Css driver callbacks
  */
@@ -187,6 +203,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 		goto out_free;
 
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
+	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
 	atomic_set(&private->avail, 1);
 	private->state = VFIO_CCW_STATE_STANDBY;
 
@@ -303,6 +320,11 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
 	case CHP_OFFLINE:
 		/* Path is gone */
 		cio_cancel_halt_clear(sch, &retry);
+		private->crw.rsc = CRW_RSC_CPATH;
+		private->crw.rsid = 0x0 | (link->chpid.cssid << 8) |
+				    link->chpid.id;
+		private->crw.erc = CRW_ERC_PERRN;
+		queue_work(vfio_ccw_work_q, &private->crw_work);
 		break;
 	case CHP_VARY_ON:
 		/* Path logically turned on */
@@ -312,6 +334,11 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
 	case CHP_ONLINE:
 		/* Path became available */
 		sch->lpm |= mask & sch->opm;
+		private->crw.rsc = CRW_RSC_CPATH;
+		private->crw.rsid = 0x0 | (link->chpid.cssid << 8) |
+				    link->chpid.id;
+		private->crw.erc = CRW_ERC_INIT;
+		queue_work(vfio_ccw_work_q, &private->crw_work);
 		break;
 	}
 
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index f3033f8fc96d..8b3ed5b45277 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -393,6 +393,7 @@ static int vfio_ccw_mdev_get_irq_info(struct vfio_irq_info *info)
 {
 	switch (info->index) {
 	case VFIO_CCW_IO_IRQ_INDEX:
+	case VFIO_CCW_CRW_IRQ_INDEX:
 		info->count = 1;
 		info->flags = VFIO_IRQ_INFO_EVENTFD;
 		break;
@@ -420,6 +421,9 @@ static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
 	case VFIO_CCW_IO_IRQ_INDEX:
 		ctx = &private->io_trigger;
 		break;
+	case VFIO_CCW_CRW_IRQ_INDEX:
+		ctx = &private->crw_trigger;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 8289b6850e59..558c658f3583 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -17,6 +17,7 @@
 #include <linux/eventfd.h>
 #include <linux/workqueue.h>
 #include <linux/vfio_ccw.h>
+#include <asm/crw.h>
 #include <asm/debug.h>
 
 #include "css.h"
@@ -98,9 +99,12 @@ struct vfio_ccw_private {
 	struct channel_program	cp;
 	struct irb		irb;
 	union scsw		scsw;
+	struct crw		crw;
 
 	struct eventfd_ctx	*io_trigger;
+	struct eventfd_ctx	*crw_trigger;
 	struct work_struct	io_work;
+	struct work_struct	crw_work;
 } __aligned(8);
 
 extern int vfio_ccw_mdev_reg(struct subchannel *sch);
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 5024636d7615..1bdf32772545 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -579,6 +579,7 @@ enum {
 
 enum {
 	VFIO_CCW_IO_IRQ_INDEX,
+	VFIO_CCW_CRW_IRQ_INDEX,
 	VFIO_CCW_NUM_IRQS
 };
 
-- 
2.17.1

Re: [RFC PATCH v1 08/10] vfio-ccw: Wire up the CRW irq and CRW region

[Cornelia Huck]

On Fri, 15 Nov 2019 03:56:18 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> Use an IRQ to notify userspace that there is a CRW
> pending in the region, related to path-availability
> changes on the passthrough subchannel.

Thinking a bit more about this, it feels a bit odd that a crw for a
chpid ends up on one subchannel. What happens if we have multiple
subchannels passed through by vfio-ccw that use that same chpid?

> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - Place the non-refactoring changes from the previous patch here
>      - Clean up checkpatch (whitespace) errors
>      - s/chp_crw/crw/
>      - Move acquire/release of io_mutex in vfio_ccw_crw_region_read()
>        into patch that introduces that region
>      - Remove duplicate include from vfio_ccw_drv.c
>      - Reorder include in vfio_ccw_private.h
> 
>  drivers/s390/cio/vfio_ccw_drv.c     | 27 +++++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_ops.c     |  4 ++++
>  drivers/s390/cio/vfio_ccw_private.h |  4 ++++
>  include/uapi/linux/vfio.h           |  1 +
>  4 files changed, 36 insertions(+)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index d1b9020d037b..ab20c32e5319 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -108,6 +108,22 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>  		eventfd_signal(private->io_trigger, 1);
>  }
>  
> +static void vfio_ccw_crw_todo(struct work_struct *work)
> +{
> +	struct vfio_ccw_private *private;
> +	struct crw *crw;
> +
> +	private = container_of(work, struct vfio_ccw_private, crw_work);
> +	crw = &private->crw;
> +
> +	mutex_lock(&private->io_mutex);
> +	memcpy(&private->crw_region->crw0, crw, sizeof(*crw));

This looks a bit inflexible. Should we want to support subchannel crws
in the future, we'd need to copy two crws.

Maybe keep two crws (they're not that large, anyway) in the private
structure and copy the second one iff the first one has the chaining
bit on?

> +	mutex_unlock(&private->io_mutex);
> +
> +	if (private->crw_trigger)
> +		eventfd_signal(private->crw_trigger, 1);
> +}
> +
>  /*
>   * Css driver callbacks
>   */
> @@ -187,6 +203,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
>  		goto out_free;
>  
>  	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
> +	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
>  	atomic_set(&private->avail, 1);
>  	private->state = VFIO_CCW_STATE_STANDBY;
>  
> @@ -303,6 +320,11 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
>  	case CHP_OFFLINE:
>  		/* Path is gone */
>  		cio_cancel_halt_clear(sch, &retry);
> +		private->crw.rsc = CRW_RSC_CPATH;
> +		private->crw.rsid = 0x0 | (link->chpid.cssid << 8) |

What's the leading '0x0' for?

> +				    link->chpid.id;
> +		private->crw.erc = CRW_ERC_PERRN;
> +		queue_work(vfio_ccw_work_q, &private->crw_work);
>  		break;
>  	case CHP_VARY_ON:
>  		/* Path logically turned on */
> @@ -312,6 +334,11 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
>  	case CHP_ONLINE:
>  		/* Path became available */
>  		sch->lpm |= mask & sch->opm;
> +		private->crw.rsc = CRW_RSC_CPATH;
> +		private->crw.rsid = 0x0 | (link->chpid.cssid << 8) |
> +				    link->chpid.id;
> +		private->crw.erc = CRW_ERC_INIT;
> +		queue_work(vfio_ccw_work_q, &private->crw_work);

Isn't that racy? Imagine you get one notification for a chpid and queue
it. Then, you get another notification for another chpid and queue it
as well. Depending on when userspace reads, it gets different chpids.
Moreover, a crw may be lost... or am I missing something obvious?

Maybe you need a real queue for the generated crws?

>  		break;
>  	}
>  

Re: [RFC PATCH v1 08/10] vfio-ccw: Wire up the CRW irq and CRW region

[Eric Farman]

On 11/19/19 1:52 PM, Cornelia Huck wrote:
> On Fri, 15 Nov 2019 03:56:18 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> Use an IRQ to notify userspace that there is a CRW
>> pending in the region, related to path-availability
>> changes on the passthrough subchannel.
> 
> Thinking a bit more about this, it feels a bit odd that a crw for a
> chpid ends up on one subchannel. What happens if we have multiple
> subchannels passed through by vfio-ccw that use that same chpid?

Yeah...  It doesn't end up on one subchannel, it ends up on every
affected subchannel, based on the loops in (for example)
chsc_chp_offline().  This means that "let's configure off a CHPID to the
LPAR" translates one channel-path CRW into N channel-path CRWs (one each
sent to N subchannels).  It would make more sense if we just presented
one channel-path CRW to the guest, but I'm having difficulty seeing how
we could wire this up.  What we do here is use the channel-path event
handler in vfio-ccw also create a channel-path CRW to be presented to
the guest, even though it's processing something at the subchannel level.

The actual CRW handlers are in the base cio code, and we only get into
vfio-ccw when processing the individual subchannels.  Do we need to make
a device (or something?) at the guest level for the chpids that exist?
Or do something to say "hey we got this from a subchannel, put it on a
global queue if it's unique, or throw it away if it's a duplicate we
haven't processed yet" ?  Thoughts?

> 
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v0->v1: [EF]
>>      - Place the non-refactoring changes from the previous patch here
>>      - Clean up checkpatch (whitespace) errors
>>      - s/chp_crw/crw/
>>      - Move acquire/release of io_mutex in vfio_ccw_crw_region_read()
>>        into patch that introduces that region
>>      - Remove duplicate include from vfio_ccw_drv.c
>>      - Reorder include in vfio_ccw_private.h
>>
>>  drivers/s390/cio/vfio_ccw_drv.c     | 27 +++++++++++++++++++++++++++
>>  drivers/s390/cio/vfio_ccw_ops.c     |  4 ++++
>>  drivers/s390/cio/vfio_ccw_private.h |  4 ++++
>>  include/uapi/linux/vfio.h           |  1 +
>>  4 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>> index d1b9020d037b..ab20c32e5319 100644
>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>> @@ -108,6 +108,22 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>>  		eventfd_signal(private->io_trigger, 1);
>>  }
>>  
>> +static void vfio_ccw_crw_todo(struct work_struct *work)
>> +{
>> +	struct vfio_ccw_private *private;
>> +	struct crw *crw;
>> +
>> +	private = container_of(work, struct vfio_ccw_private, crw_work);
>> +	crw = &private->crw;
>> +
>> +	mutex_lock(&private->io_mutex);
>> +	memcpy(&private->crw_region->crw0, crw, sizeof(*crw));
> 
> This looks a bit inflexible. Should we want to support subchannel crws
> in the future, we'd need to copy two crws.
> 
> Maybe keep two crws (they're not that large, anyway) in the private
> structure and copy the second one iff the first one has the chaining
> bit on?

That's a good idea.

> 
>> +	mutex_unlock(&private->io_mutex);
>> +
>> +	if (private->crw_trigger)
>> +		eventfd_signal(private->crw_trigger, 1);
>> +}
>> +
>>  /*
>>   * Css driver callbacks
>>   */
>> @@ -187,6 +203,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
>>  		goto out_free;
>>  
>>  	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
>> +	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
>>  	atomic_set(&private->avail, 1);
>>  	private->state = VFIO_CCW_STATE_STANDBY;
>>  
>> @@ -303,6 +320,11 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
>>  	case CHP_OFFLINE:
>>  		/* Path is gone */
>>  		cio_cancel_halt_clear(sch, &retry);
>> +		private->crw.rsc = CRW_RSC_CPATH;
>> +		private->crw.rsid = 0x0 | (link->chpid.cssid << 8) |
> 
> What's the leading '0x0' for?

Um, yeah.  It's SUPER important.  :)

> 
>> +				    link->chpid.id;
>> +		private->crw.erc = CRW_ERC_PERRN;
>> +		queue_work(vfio_ccw_work_q, &private->crw_work);
>>  		break;
>>  	case CHP_VARY_ON:
>>  		/* Path logically turned on */
>> @@ -312,6 +334,11 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
>>  	case CHP_ONLINE:
>>  		/* Path became available */
>>  		sch->lpm |= mask & sch->opm;
>> +		private->crw.rsc = CRW_RSC_CPATH;
>> +		private->crw.rsid = 0x0 | (link->chpid.cssid << 8) |
>> +				    link->chpid.id;
>> +		private->crw.erc = CRW_ERC_INIT;
>> +		queue_work(vfio_ccw_work_q, &private->crw_work);
> 
> Isn't that racy? Imagine you get one notification for a chpid and queue
> it. Then, you get another notification for another chpid and queue it
> as well. Depending on when userspace reads, it gets different chpids.
> Moreover, a crw may be lost... or am I missing something obvious?

Nope, you're right on.  If I start thrashing config on/off chpids on the
host, I eventually fall down with all sorts of weirdness.

> 
> Maybe you need a real queue for the generated crws?

I guess this is what I'm wrestling with...  We don't have a queue for
guest-wide work items, as it's currently broken apart by subchannel.  Is
adding one at the vfio-ccw level right?  Feels odd to me, since multiple
guests could use devices connected via vfio-ccw, which may or may share
common chpids.

I have a rough hack that serializes things a bit, while still keeping
the CRW duplication at the subchannel level.  Things improve
considerably, but it still seems odd to me.  I'll keep working on that
unless anyone has any better ideas.

> 
>>  		break;
>>  	}
>>  
> 

Re: [RFC PATCH v1 08/10] vfio-ccw: Wire up the CRW irq and CRW region

[Cornelia Huck]

On Thu, 5 Dec 2019 15:43:55 -0500
Eric Farman <farman@linux.ibm.com> wrote:

> On 11/19/19 1:52 PM, Cornelia Huck wrote:
> > On Fri, 15 Nov 2019 03:56:18 +0100
> > Eric Farman <farman@linux.ibm.com> wrote:
> >   
> >> From: Farhan Ali <alifm@linux.ibm.com>
> >>
> >> Use an IRQ to notify userspace that there is a CRW
> >> pending in the region, related to path-availability
> >> changes on the passthrough subchannel.  
> > 
> > Thinking a bit more about this, it feels a bit odd that a crw for a
> > chpid ends up on one subchannel. What happens if we have multiple
> > subchannels passed through by vfio-ccw that use that same chpid?  
> 
> Yeah...  It doesn't end up on one subchannel, it ends up on every
> affected subchannel, based on the loops in (for example)
> chsc_chp_offline().  This means that "let's configure off a CHPID to the
> LPAR" translates one channel-path CRW into N channel-path CRWs (one each
> sent to N subchannels).  It would make more sense if we just presented
> one channel-path CRW to the guest, but I'm having difficulty seeing how
> we could wire this up.  What we do here is use the channel-path event
> handler in vfio-ccw also create a channel-path CRW to be presented to
> the guest, even though it's processing something at the subchannel level.

Yes, it's a bit odd that we need to do 1 -> N -> 1 conversion here, but
we can't really avoid it without introducing a new way to report
information that is relevant for more than one subchannel. The thing we
need to make sure is that userspace gets the same information,
regardless of which affected subchannel it looks at.

> 
> The actual CRW handlers are in the base cio code, and we only get into
> vfio-ccw when processing the individual subchannels.  Do we need to make
> a device (or something?) at the guest level for the chpids that exist?
> Or do something to say "hey we got this from a subchannel, put it on a
> global queue if it's unique, or throw it away if it's a duplicate we
> haven't processed yet" ?  Thoughts?

The problem is that you can easily get several crws that look identical
(consider e.g. a chpid that is set online and offline in a tight loop).
The only entity that should make decisions as to what to process here
is the guest.

(...)

> >> @@ -312,6 +334,11 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
> >>  	case CHP_ONLINE:
> >>  		/* Path became available */
> >>  		sch->lpm |= mask & sch->opm;
> >> +		private->crw.rsc = CRW_RSC_CPATH;
> >> +		private->crw.rsid = 0x0 | (link->chpid.cssid << 8) |
> >> +				    link->chpid.id;
> >> +		private->crw.erc = CRW_ERC_INIT;
> >> +		queue_work(vfio_ccw_work_q, &private->crw_work);  
> > 
> > Isn't that racy? Imagine you get one notification for a chpid and queue
> > it. Then, you get another notification for another chpid and queue it
> > as well. Depending on when userspace reads, it gets different chpids.
> > Moreover, a crw may be lost... or am I missing something obvious?  
> 
> Nope, you're right on.  If I start thrashing config on/off chpids on the
> host, I eventually fall down with all sorts of weirdness.
> 
> > 
> > Maybe you need a real queue for the generated crws?  
> 
> I guess this is what I'm wrestling with...  We don't have a queue for
> guest-wide work items, as it's currently broken apart by subchannel.  Is
> adding one at the vfio-ccw level right?  Feels odd to me, since multiple
> guests could use devices connected via vfio-ccw, which may or may share
> common chpids.

One problem is that the common I/O layer already processes the crws and
translates them into different per-subchannel events. We don't even
know what the original crw was: IIUC, we translate both a crw for a
chpid and a link incident event (reported by a crw with source css and
event information via chsc) concerning the concrete link to the same
event. That *probably* doesn't matter too much, but it makes things
harder. Access to the original crw queue would be nice, but hard to
implement without stepping on each others' toes.

> 
> I have a rough hack that serializes things a bit, while still keeping
> the CRW duplication at the subchannel level.  Things improve
> considerably, but it still seems odd to me.  I'll keep working on that
> unless anyone has any better ideas.

The main issue is that we're trying to report a somewhat global event
via individual devices...

...what about not reporting crws at all, but something derived from the
events we get at the subchannel driver level? Have four masks that
indicate online/offline/vary on/vary off for the respective chpids, and
have userspace decide how they want to report these to the guest? A
drawback (?) would be that a series of on/off events would only be
reported as one on and one off event, though. Feasible, or complete
lunacy?

Re: [RFC PATCH v1 08/10] vfio-ccw: Wire up the CRW irq and CRW region

[Eric Farman]

On 12/6/19 5:21 AM, Cornelia Huck wrote:
> On Thu, 5 Dec 2019 15:43:55 -0500
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> On 11/19/19 1:52 PM, Cornelia Huck wrote:
>>> On Fri, 15 Nov 2019 03:56:18 +0100
>>> Eric Farman <farman@linux.ibm.com> wrote:
>>>   
>>>> From: Farhan Ali <alifm@linux.ibm.com>
>>>>
>>>> Use an IRQ to notify userspace that there is a CRW
>>>> pending in the region, related to path-availability
>>>> changes on the passthrough subchannel.  
>>>
>>> Thinking a bit more about this, it feels a bit odd that a crw for a
>>> chpid ends up on one subchannel. What happens if we have multiple
>>> subchannels passed through by vfio-ccw that use that same chpid?  
>>
>> Yeah...  It doesn't end up on one subchannel, it ends up on every
>> affected subchannel, based on the loops in (for example)
>> chsc_chp_offline().  This means that "let's configure off a CHPID to the
>> LPAR" translates one channel-path CRW into N channel-path CRWs (one each
>> sent to N subchannels).  It would make more sense if we just presented
>> one channel-path CRW to the guest, but I'm having difficulty seeing how
>> we could wire this up.  What we do here is use the channel-path event
>> handler in vfio-ccw also create a channel-path CRW to be presented to
>> the guest, even though it's processing something at the subchannel level.
> 
> Yes, it's a bit odd that we need to do 1 -> N -> 1 conversion here, but
> we can't really avoid it without introducing a new way to report
> information that is relevant for more than one subchannel. The thing we
> need to make sure is that userspace gets the same information,
> regardless of which affected subchannel it looks at.
> 
>>
>> The actual CRW handlers are in the base cio code, and we only get into
>> vfio-ccw when processing the individual subchannels.  Do we need to make
>> a device (or something?) at the guest level for the chpids that exist?
>> Or do something to say "hey we got this from a subchannel, put it on a
>> global queue if it's unique, or throw it away if it's a duplicate we
>> haven't processed yet" ?  Thoughts?
> 
> The problem is that you can easily get several crws that look identical
> (consider e.g. a chpid that is set online and offline in a tight loop).

Yeah, I have a little program that does such a loop.  Things don't work
too well even with a random delay between on/off, though a hack I'm
trying to formalize for v2 improves matters.  If I drop that delay to
zero, um, well I haven't had the nerve to try that.  :)

> The only entity that should make decisions as to what to process here
> is the guest.

Agreed.  So your suggestion in the QEMU series of acting like stcrw is
good; give the guest all the information it can, and let it decide what
thrashing is needed.  I guess if I can just queue everything on the
vfio_ccw_private, and move one (two?) into the crw_region each time it's
read then that should work well enough.  Thanks!

> 
> (...)
> 
>>>> @@ -312,6 +334,11 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
>>>>  	case CHP_ONLINE:
>>>>  		/* Path became available */
>>>>  		sch->lpm |= mask & sch->opm;
>>>> +		private->crw.rsc = CRW_RSC_CPATH;
>>>> +		private->crw.rsid = 0x0 | (link->chpid.cssid << 8) |
>>>> +				    link->chpid.id;
>>>> +		private->crw.erc = CRW_ERC_INIT;
>>>> +		queue_work(vfio_ccw_work_q, &private->crw_work);  
>>>
>>> Isn't that racy? Imagine you get one notification for a chpid and queue
>>> it. Then, you get another notification for another chpid and queue it
>>> as well. Depending on when userspace reads, it gets different chpids.
>>> Moreover, a crw may be lost... or am I missing something obvious?  
>>
>> Nope, you're right on.  If I start thrashing config on/off chpids on the
>> host, I eventually fall down with all sorts of weirdness.
>>
>>>
>>> Maybe you need a real queue for the generated crws?  
>>
>> I guess this is what I'm wrestling with...  We don't have a queue for
>> guest-wide work items, as it's currently broken apart by subchannel.  Is
>> adding one at the vfio-ccw level right?  Feels odd to me, since multiple
>> guests could use devices connected via vfio-ccw, which may or may share
>> common chpids.
> 
> One problem is that the common I/O layer already processes the crws and
> translates them into different per-subchannel events. We don't even
> know what the original crw was: IIUC, we translate both a crw for a
> chpid and a link incident event (reported by a crw with source css and
> event information via chsc) concerning the concrete link to the same
> event. That *probably* doesn't matter too much, but it makes things
> harder. Access to the original crw queue would be nice, but hard to
> implement without stepping on each others' toes.>
>>
>> I have a rough hack that serializes things a bit, while still keeping
>> the CRW duplication at the subchannel level.  Things improve
>> considerably, but it still seems odd to me.  I'll keep working on that
>> unless anyone has any better ideas.
> 
> The main issue is that we're trying to report a somewhat global event
> via individual devices...

+1

> 
> ...what about not reporting crws at all, but something derived from the
> events we get at the subchannel driver level? Have four masks that
> indicate online/offline/vary on/vary off for the respective chpids, and
> have userspace decide how they want to report these to the guest? A
> drawback (?) would be that a series of on/off events would only be
> reported as one on and one off event, though. Feasible, or complete
> lunacy?
> 

Not complete lunacy, but brings concerns of its own as we'd need to
ensure the masks don't say something nonsensical, like (for example)
both vary on and vary off.  Or what happens if both vary on and config
off gets set?  Not a huge amount of work, but just seems to carry more
risk than a queue of the existing CRWs and letting the guest process
them itself, even if things are duplicated more than necessary.  In
reality, these events aren't that common anyway unless things go REALLY
sideways.

Re: [RFC PATCH v1 08/10] vfio-ccw: Wire up the CRW irq and CRW region

[Cornelia Huck]

On Fri, 6 Dec 2019 16:24:31 -0500
Eric Farman <farman@linux.ibm.com> wrote:

> On 12/6/19 5:21 AM, Cornelia Huck wrote:
> > On Thu, 5 Dec 2019 15:43:55 -0500
> > Eric Farman <farman@linux.ibm.com> wrote:
> >   
> >> On 11/19/19 1:52 PM, Cornelia Huck wrote:  
> >>> On Fri, 15 Nov 2019 03:56:18 +0100
> >>> Eric Farman <farman@linux.ibm.com> wrote:

> >> The actual CRW handlers are in the base cio code, and we only get into
> >> vfio-ccw when processing the individual subchannels.  Do we need to make
> >> a device (or something?) at the guest level for the chpids that exist?
> >> Or do something to say "hey we got this from a subchannel, put it on a
> >> global queue if it's unique, or throw it away if it's a duplicate we
> >> haven't processed yet" ?  Thoughts?  
> > 
> > The problem is that you can easily get several crws that look identical
> > (consider e.g. a chpid that is set online and offline in a tight loop).  
> 
> Yeah, I have a little program that does such a loop.  Things don't work
> too well even with a random delay between on/off, though a hack I'm
> trying to formalize for v2 improves matters.  If I drop that delay to
> zero, um, well I haven't had the nerve to try that.  :)

:)

I'm also not quite sure what our expectations need to be here. IIRC, it
is not guaranteed that we get a CRW for each and every of the
operations anyway. From what I remember, the only sane way for the
guest to process channel reports is to retrieve the current state (via
stsch) and process that, as the state may have changed again between
generating the CRW and the guest retrieving it.

> 
> > The only entity that should make decisions as to what to process here
> > is the guest.  
> 
> Agreed.  So your suggestion in the QEMU series of acting like stcrw is
> good; give the guest all the information it can, and let it decide what
> thrashing is needed.  I guess if I can just queue everything on the
> vfio_ccw_private, and move one (two?) into the crw_region each time it's
> read then that should work well enough.  Thanks!

I *think* we can assume that the callback is invoked by the common I/O
layer for every affected subchannel if there's actually an event from
the hardware, so we can be reasonably sure that we can relay every
event to userspace.

> 
> > 
> > (...)
> >   
> >>>> @@ -312,6 +334,11 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
> >>>>  	case CHP_ONLINE:
> >>>>  		/* Path became available */
> >>>>  		sch->lpm |= mask & sch->opm;
> >>>> +		private->crw.rsc = CRW_RSC_CPATH;
> >>>> +		private->crw.rsid = 0x0 | (link->chpid.cssid << 8) |
> >>>> +				    link->chpid.id;
> >>>> +		private->crw.erc = CRW_ERC_INIT;
> >>>> +		queue_work(vfio_ccw_work_q, &private->crw_work);    
> >>>
> >>> Isn't that racy? Imagine you get one notification for a chpid and queue
> >>> it. Then, you get another notification for another chpid and queue it
> >>> as well. Depending on when userspace reads, it gets different chpids.
> >>> Moreover, a crw may be lost... or am I missing something obvious?    
> >>
> >> Nope, you're right on.  If I start thrashing config on/off chpids on the
> >> host, I eventually fall down with all sorts of weirdness.
> >>  
> >>>
> >>> Maybe you need a real queue for the generated crws?    
> >>
> >> I guess this is what I'm wrestling with...  We don't have a queue for
> >> guest-wide work items, as it's currently broken apart by subchannel.  Is
> >> adding one at the vfio-ccw level right?  Feels odd to me, since multiple
> >> guests could use devices connected via vfio-ccw, which may or may share
> >> common chpids.  
> > 
> > One problem is that the common I/O layer already processes the crws and
> > translates them into different per-subchannel events. We don't even
> > know what the original crw was: IIUC, we translate both a crw for a
> > chpid and a link incident event (reported by a crw with source css and
> > event information via chsc) concerning the concrete link to the same
> > event. That *probably* doesn't matter too much, but it makes things
> > harder. Access to the original crw queue would be nice, but hard to
> > implement without stepping on each others' toes.>  
> >>
> >> I have a rough hack that serializes things a bit, while still keeping
> >> the CRW duplication at the subchannel level.  Things improve
> >> considerably, but it still seems odd to me.  I'll keep working on that
> >> unless anyone has any better ideas.  
> > 
> > The main issue is that we're trying to report a somewhat global event
> > via individual devices...  
> 
> +1

If only we could use some kind of global interface... but I can't think
of a sane way to do that :/

> 
> > 
> > ...what about not reporting crws at all, but something derived from the
> > events we get at the subchannel driver level? Have four masks that
> > indicate online/offline/vary on/vary off for the respective chpids, and
> > have userspace decide how they want to report these to the guest? A
> > drawback (?) would be that a series of on/off events would only be
> > reported as one on and one off event, though. Feasible, or complete
> > lunacy?
> >   
> 
> Not complete lunacy, but brings concerns of its own as we'd need to
> ensure the masks don't say something nonsensical, like (for example)
> both vary on and vary off.  Or what happens if both vary on and config
> off gets set?  Not a huge amount of work, but just seems to carry more
> risk than a queue of the existing CRWs and letting the guest process
> them itself, even if things are duplicated more than necessary.  In
> reality, these events aren't that common anyway unless things go REALLY
> sideways.

Yeah, that approach probably just brings a different set of issues... I
think we would need to relay all mask changes, and QEMU would need to
inject all events, and the guest would need to figure out what to do.
Not sure if that is better.

[RFC PATCH v1 09/10] vfio-ccw: Add trace for CRW event

[Eric Farman]

Since CRW events are (should be) rare, let's put a trace
in that routine too.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c   |  1 +
 drivers/s390/cio/vfio_ccw_trace.c |  1 +
 drivers/s390/cio/vfio_ccw_trace.h | 30 ++++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index ab20c32e5319..0316b158f51f 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -306,6 +306,7 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
 	if (cio_update_schib(sch))
 		return -ENODEV;
 
+	trace_vfio_ccw_chp_event(private->sch->schid, mask, event);
 	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
 			   mdev_uuid(private->mdev), sch->schid.cssid,
 			   sch->schid.ssid, sch->schid.sch_no,
diff --git a/drivers/s390/cio/vfio_ccw_trace.c b/drivers/s390/cio/vfio_ccw_trace.c
index 8c671d2519f6..4a0205905afc 100644
--- a/drivers/s390/cio/vfio_ccw_trace.c
+++ b/drivers/s390/cio/vfio_ccw_trace.c
@@ -9,6 +9,7 @@
 #define CREATE_TRACE_POINTS
 #include "vfio_ccw_trace.h"
 
+EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_chp_event);
 EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_fsm_async_request);
 EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_fsm_event);
 EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_fsm_io_request);
diff --git a/drivers/s390/cio/vfio_ccw_trace.h b/drivers/s390/cio/vfio_ccw_trace.h
index 30162a318a8a..8c82d0c27e81 100644
--- a/drivers/s390/cio/vfio_ccw_trace.h
+++ b/drivers/s390/cio/vfio_ccw_trace.h
@@ -17,6 +17,36 @@
 
 #include <linux/tracepoint.h>
 
+TRACE_EVENT(vfio_ccw_chp_event,
+	TP_PROTO(struct subchannel_id schid,
+		 int mask,
+		 int event),
+	TP_ARGS(schid, mask, event),
+
+	TP_STRUCT__entry(
+		__field(u8, cssid)
+		__field(u8, ssid)
+		__field(u16, sch_no)
+		__field(int, mask)
+		__field(int, event)
+	),
+
+	TP_fast_assign(
+		__entry->cssid = schid.cssid;
+		__entry->ssid = schid.ssid;
+		__entry->sch_no = schid.sch_no;
+		__entry->mask = mask;
+		__entry->event = event;
+	),
+
+	TP_printk("schid=%x.%x.%04x mask=0x%x event=%d",
+		  __entry->cssid,
+		  __entry->ssid,
+		  __entry->sch_no,
+		  __entry->mask,
+		  __entry->event)
+);
+
 TRACE_EVENT(vfio_ccw_fsm_async_request,
 	TP_PROTO(struct subchannel_id schid,
 		 int command,
-- 
2.17.1

[RFC PATCH v1 10/10] vfio-ccw: Remove inline get_schid() routine

[Eric Farman]

This seems misplaced in the middle of FSM, returning the schid
field from inside the private struct.  We could move this macro
into vfio_ccw_private.h, but this doesn't seem to simplify things
that much.  Let's just remove it, and use the field directly.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_fsm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 23e61aa638e4..c4c303645d7d 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -228,10 +228,6 @@ static void fsm_disabled_irq(struct vfio_ccw_private *private,
 	 */
 	cio_disable_subchannel(sch);
 }
-inline struct subchannel_id get_schid(struct vfio_ccw_private *p)
-{
-	return p->sch->schid;
-}
 
 /*
  * Deal with the ccw command request from the userspace.
@@ -244,7 +240,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 	struct ccw_io_region *io_region = private->io_region;
 	struct mdev_device *mdev = private->mdev;
 	char *errstr = "request";
-	struct subchannel_id schid = get_schid(private);
+	struct subchannel_id schid = private->sch->schid;
 
 	private->state = VFIO_CCW_STATE_CP_PROCESSING;
 	memcpy(scsw, io_region->scsw_area, sizeof(*scsw));
@@ -342,7 +338,7 @@ static void fsm_async_request(struct vfio_ccw_private *private,
 		cmd_region->ret_code = -EINVAL;
 	}
 
-	trace_vfio_ccw_fsm_async_request(get_schid(private),
+	trace_vfio_ccw_fsm_async_request(private->sch->schid,
 					 cmd_region->command,
 					 cmd_region->ret_code);
 }
-- 
2.17.1

Re: [RFC PATCH v1 00/10] s390/vfio-ccw: Channel Path Handling

[Cornelia Huck]

On Fri, 15 Nov 2019 03:56:10 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> Here is a first pass at the channel-path handling code for vfio-ccw.
> This was initially developed by Farhan Ali this past summer, and
> picked up by me.  For my own benefit/sanity, I made a small changelog
> in the commit message for each patch, describing the changes I've
> made to his original code beyond just rebasing to master.
> 
> I did split a couple of his patches, to (hopefully) make them a little
> more understandable.  The entire series is based on top of the trace
> rework patches from a few weeks ago, which are currently pending.
> But really, the only cause for overlap is the trace patch here.
> The bulk of it is really self-contained.
> 
> With this, and the corresponding QEMU series (to be posted momentarily),
> applied I am able to configure off/on a CHPID (for example, by issuing
> "chchp -c 0/1 xx" on the host), and the guest is able to see both the
> events and reflect the updated path masks in its structures.

Nice.

> 
> For reasons that are hopefully obvious, issuing chchp within the guest
> only works for the logical vary.  Configuring it off/on does not work,
> which I think is fine.

Yes, I think that's completely ok.

> 
> Eric Farman (4):
>   vfio-ccw: Refactor the unregister of the async regions
>   vfio-ccw: Refactor IRQ handlers
>   vfio-ccw: Add trace for CRW event
>   vfio-ccw: Remove inline get_schid() routine
> 
> Farhan Ali (6):
>   vfio-ccw: Introduce new helper functions to free/destroy regions
>   vfio-ccw: Register a chp_event callback for vfio-ccw
>   vfio-ccw: Use subchannel lpm in the orb
>   vfio-ccw: Introduce a new schib region
>   vfio-ccw: Introduce a new CRW region
>   vfio-ccw: Wire up the CRW irq and CRW region
> 
>  drivers/s390/cio/Makefile           |   2 +-
>  drivers/s390/cio/vfio_ccw_chp.c     | 128 +++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_cp.c      |   4 +-
>  drivers/s390/cio/vfio_ccw_drv.c     | 140 ++++++++++++++++++++++++++--
>  drivers/s390/cio/vfio_ccw_fsm.c     |   8 +-
>  drivers/s390/cio/vfio_ccw_ops.c     |  65 +++++++++----
>  drivers/s390/cio/vfio_ccw_private.h |  11 +++
>  drivers/s390/cio/vfio_ccw_trace.c   |   1 +
>  drivers/s390/cio/vfio_ccw_trace.h   |  30 ++++++
>  include/uapi/linux/vfio.h           |   3 +
>  include/uapi/linux/vfio_ccw.h       |  10 ++
>  11 files changed, 366 insertions(+), 36 deletions(-)
>  create mode 100644 drivers/s390/cio/vfio_ccw_chp.c

I just glanced at the general approach taken here, which seems sane to
me. I'm, however, currently a bit short on free cycles for reviewing
this, so I'd appreciate if other folks could take a look at this as
well.

(Same applies to the QEMU patches.)