s390/vfio-ccw: Channel Path Handling
Subsystems (5)
Eric Farman
maintainercover s390/vfio-ccw: Channel Path Handling
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
Signature · 1 line
2.17.1
Eric Farman
maintainerRFC PATCH 1/10 vfio-ccw: Introduce new helper functions to free/destroy regions
From: Farhan Ali <[email protected]>
Consolidate some of the cleanup code for the regions, so that as more are added we reduce code duplication.
---
Notes:
v0->v1: [EF]
- Commit message
drivers/s390/cio/vfio_ccw_drv.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
drivers/s390/cio/vfio_ccw_drv.c +18 -10
@@ -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 (); }
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 01/10] vfio-ccw: Introduce new helper functions to free/destroy regions
Quoted 7 lines · On Fri, 15 Nov 2019 03:56:11 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
Consolidate some of the cleanup code for the regions, so that as more are added we reduce code duplication.
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>
Quoted 8 lines
---
Notes:
v0->v1: [EF]
- Commit messagedrivers/s390/cio/vfio_ccw_drv.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
Makes sense.
Eric Farman
maintainerRFC PATCH 2/10 vfio-ccw: Register a chp_event callback for vfio-ccw
In reply to Eric FarmanFrom: Farhan Ali <[email protected]>
Register the chp_event callback to receive channel path related events for the subchannels managed by vfio-ccw.
---
Notes:
v0->v1: [EF]
- Add s390dbf trace
drivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
drivers/s390/cio/vfio_ccw_drv.c +44 -0
@@ -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 )
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 02/10] vfio-ccw: Register a chp_event callback for vfio-ccw
Quoted 34 lines · On Fri, 15 Nov 2019 03:56:12 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
Register the
chp_eventcallback to receive channel path related events for the subchannels managed by vfio-ccw.Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v0->v1: [EF]
- Add s390dbf tracedrivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)drivers/s390/cio/vfio_ccw_drv.c +3
-0@@ -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)
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_sch_event(struct subchannel *sch, int process) · line 272
Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +10
-0+{ +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.
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_sch_event(struct subchannel *sch, int process) · line 277
Quoted 5 lines
drivers/s390/cio/vfio_ccw_drv.c +5
-0+ +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.
Quoted 11 lines
drivers/s390/cio/vfio_ccw_drv.c +11
-0+ +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 ;
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_sch_event(struct subchannel *sch, int process) · line 298
Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +10
-0+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.
Quoted 15 lines
drivers/s390/cio/vfio_ccw_drv.c +5
-0+ +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 )
Eric Farman
maintainerRe: [RFC PATCH v1 02/10] vfio-ccw: Register a chp_event callback for vfio-ccw
Quoted 35 lines · Cornelia Huck
Quoted 32 lines · On Fri, 15 Nov 2019 03:56:12 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
Register the
chp_eventcallback to receive channel path related events for the subchannels managed by vfio-ccw.Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v0->v1: [EF]
- Add s390dbf tracedrivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)drivers/s390/cio/vfio_ccw_drv.c +1
-0@@ -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; }
Quoted 16 lines
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_sch_event(struct subchannel *sch, int process) · line 272
Quoted 12 lines
drivers/s390/cio/vfio_ccw_drv.c +12
-0+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
_OFFLINEcase; 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.
Quoted 8 lines
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_sch_event(struct subchannel *sch, int process) · line 277
Quoted 6 lines
drivers/s390/cio/vfio_ccw_drv.c +5
-0+ +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.
Quoted 41 lines
Quoted 12 lines
drivers/s390/cio/vfio_ccw_drv.c +11
-0+ +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 ;drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_sch_event(struct subchannel *sch, int process) · line 298
Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +10
-0+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.
Quoted 15 lines
drivers/s390/cio/vfio_ccw_drv.c +5
-0+ +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 )
Eric Farman
maintainerRFC PATCH 3/10 vfio-ccw: Use subchannel lpm in the orb
In reply to Eric FarmanFrom: Farhan Ali <[email protected]>
The subchannel logical path mask (lpm) would have the most up to date information of channel paths that are logically available for the subchannel.
---
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(-)
drivers/s390/cio/vfio_ccw_cp.c +1 -3
@@ -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;
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 03/10] vfio-ccw: Use subchannel lpm in the orb
Quoted 6 lines · On Fri, 15 Nov 2019 03:56:13 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
The subchannel logical path mask (lpm) would have the most up to date information of channel paths that are logically available for the subchannel.
Quoted 7 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v0->v1: [EF]
- None; however I am greatly confused by this one. Thoughts?
I think it's actually wrong.
Quoted 9 lines
drivers/s390/cio/vfio_ccw_cp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-0
drivers/s390/cio/vfio_ccw_cp.c union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm) · line 784
Quoted 6 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-3@@ -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.
drivers/s390/cio/vfio_ccw_cp.c union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm) · line 782
Quoted 1 line
drivers/s390/cio/vfio_ccw_cp.c +1
-0+ 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.
Quoted 3 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-0chain = list_first_entry(&cp->ccwchain_list, struct ccwchain, next); cpa = chain->ch_ccw;
Eric Farman
maintainerRe: [RFC PATCH v1 03/10] vfio-ccw: Use subchannel lpm in the orb
Quoted 38 lines · Cornelia Huck
Quoted 6 lines · On Fri, 15 Nov 2019 03:56:13 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
The subchannel logical path mask (lpm) would have the most up to date information of channel paths that are logically available for the subchannel.
Quoted 7 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v0->v1: [EF]
- None; however I am greatly confused by this one. Thoughts?I think it's actually wrong.
Quoted 9 lines
drivers/s390/cio/vfio_ccw_cp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-0drivers/s390/cio/vfio_ccw_cp.c union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm) · line 784
Quoted 6 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-3@@ -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.
Quoted 6 lines
drivers/s390/cio/vfio_ccw_cp.c union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm) · line 782
Quoted 1 line
drivers/s390/cio/vfio_ccw_cp.c +1
-0+ 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. :)
Quoted 3 lines
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.)
Quoted 5 lines
Quoted 4 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-0chain = 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
Quoted 46 lines · On Tue, 19 Nov 2019 10:16:30 -0500 Eric Farman <[email protected]> wrote:
Quoted 38 lines · Cornelia Huck
Quoted 6 lines · On Fri, 15 Nov 2019 03:56:13 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
The subchannel logical path mask (lpm) would have the most up to date information of channel paths that are logically available for the subchannel.
Quoted 7 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v0->v1: [EF]
- None; however I am greatly confused by this one. Thoughts?I think it's actually wrong.
Quoted 9 lines
drivers/s390/cio/vfio_ccw_cp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-0drivers/s390/cio/vfio_ccw_cp.c union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm) · line 784
Quoted 6 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-3@@ -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.
Quoted 6 lines
drivers/s390/cio/vfio_ccw_cp.c union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm) · line 782
Quoted 1 line
drivers/s390/cio/vfio_ccw_cp.c +1
-0+ 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?
Quoted 2 lines
I had the same opinion, but didn't want to flat-out discard it from his series without a second look. :)
:)
Quoted 8 lines
Quoted 4 lines
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?
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-0chain = list_first_entry(&cp->ccwchain_list, struct ccwchain, next); cpa = chain->ch_ccw;
Eric Farman
maintainerRe: [RFC PATCH v1 03/10] vfio-ccw: Use subchannel lpm in the orb
Quoted 59 lines · Cornelia Huck
Quoted 46 lines · On Tue, 19 Nov 2019 10:16:30 -0500 Eric Farman <[email protected]> wrote:
Quoted 38 lines · Cornelia Huck
Quoted 6 lines · On Fri, 15 Nov 2019 03:56:13 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
The subchannel logical path mask (lpm) would have the most up to date information of channel paths that are logically available for the subchannel.
Quoted 7 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v0->v1: [EF]
- None; however I am greatly confused by this one. Thoughts?I think it's actually wrong.
Quoted 9 lines
drivers/s390/cio/vfio_ccw_cp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-0drivers/s390/cio/vfio_ccw_cp.c union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm) · line 784
Quoted 6 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-3@@ -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.
Quoted 6 lines
drivers/s390/cio/vfio_ccw_cp.c union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm) · line 782
Quoted 1 line
drivers/s390/cio/vfio_ccw_cp.c +1
-0+ 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?
Quoted 2 lines
I had the same opinion, but didn't want to flat-out discard it from his series without a second look. :)
:)
Quoted 5 lines
Quoted 4 lines
If we want to include the current value of the subchannel lpm in the requests, we probably want to AND the masks instead.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 9 lines
Quoted 8 lines
Quoted 7 lines
Quoted 6 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-0chain = list_first_entry(&cp->ccwchain_list, struct ccwchain, next); cpa = chain->ch_ccw;
Eric Farman
maintainerRFC PATCH 4/10 vfio-ccw: Refactor the unregister of the async regions
In reply to Eric FarmanThis 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.
--- drivers/s390/cio/vfio_ccw_ops.c | 20 ++++++++++++-------- drivers/s390/cio/vfio_ccw_private.h | 1 + 2 files changed, 13 insertions(+), 8 deletions(-)
drivers/s390/cio/vfio_ccw_ops.c +12 -8
@@ -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)
drivers/s390/cio/vfio_ccw_private.h +1 -0
@@ -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);
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 04/10] vfio-ccw: Refactor the unregister of the async regions
Quoted 7 lines · On Fri, 15 Nov 2019 03:56:14 +0100 Eric Farman <[email protected]> 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.
Quoted 5 lines
Signed-off-by Eric Farman <[email protected]>--- drivers/s390/cio/vfio_ccw_ops.c | 20 ++++++++++++-------- drivers/s390/cio/vfio_ccw_private.h | 1 + 2 files changed, 13 insertions(+), 8 deletions(-)
Eric Farman
maintainerRFC PATCH 5/10 vfio-ccw: Introduce a new schib region
In reply to Eric FarmanFrom: Farhan Ali <[email protected]>
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.
---
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
drivers/s390/cio/Makefile +1 -1
@@ -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
drivers/s390/cio/vfio_ccw_chp.c new file +75 -0
75 lines not on this page · raw message
drivers/s390/cio/vfio_ccw_drv.c +20 -0
@@ -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) {
drivers/s390/cio/vfio_ccw_ops.c +12 -2
@@ -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; }
drivers/s390/cio/vfio_ccw_private.h +3 -0
@@ -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;
include/uapi/linux/vfio.h +1 -0
@@ -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
include/uapi/linux/vfio_ccw.h +5 -0
@@ -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
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 05/10] vfio-ccw: Introduce a new schib region
Quoted 36 lines · On Fri, 15 Nov 2019 03:56:15 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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 <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v0->v1: [EF]
- Clean up checkpatch (#include, whitespace) errors
- Remove unnecessary includes fromvfio_ccw_chp.c
- Add ret=-ENOMEM in error path for new region
- Add call tovfio_ccw_unregister_dev_regions()during error exit path ofvfio_ccw_mdev_open()
- New info on the module prologue
- Reorder cleanup of regionsdrivers/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.cinclude/uapi/linux/vfio_ccw.h +0
-0
include/uapi/linux/vfio_ccw.h struct ccw_cmd_region { · line 36
Quoted 3 lines
include/uapi/linux/vfio_ccw.h +0
-0@@ -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
?
include/uapi/linux/vfio_ccw.h struct ccw_cmd_region { · line 42
Quoted 6 lines
include/uapi/linux/vfio_ccw.h +5
-0+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 :)
Eric Farman
maintainerRe: [RFC PATCH v1 05/10] vfio-ccw: Introduce a new schib region
Quoted 47 lines · Cornelia Huck
Quoted 36 lines · On Fri, 15 Nov 2019 03:56:15 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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 <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v0->v1: [EF]
- Clean up checkpatch (#include, whitespace) errors
- Remove unnecessary includes fromvfio_ccw_chp.c
- Add ret=-ENOMEM in error path for new region
- Add call tovfio_ccw_unregister_dev_regions()during error exit path ofvfio_ccw_mdev_open()
- New info on the module prologue
- Reorder cleanup of regionsdrivers/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.cinclude/uapi/linux/vfio_ccw.h +0
-0include/uapi/linux/vfio_ccw.h struct ccw_cmd_region { · line 36
Quoted 3 lines
include/uapi/linux/vfio_ccw.h +0
-0@@ -34,4 +34,9 @@ struct ccw_cmd_region {__u32 ret_code; } __packed;Let's add a comment:
- that reading this region triggers astsch()
- that this region is guarded by a capability
Quoted 1 line
?
Agreed, and ditto for patch 6.
Quoted 11 lines
include/uapi/linux/vfio_ccw.h struct ccw_cmd_region { · line 42
Quoted 7 lines
include/uapi/linux/vfio_ccw.h +5
-0+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.
Quoted 2 lines
Oh, and please update
Documentation/s390/vfio-ccw.rst:)
Whoops! Yes, I'll do that here and in patch 6.
Eric Farman
maintainerRFC PATCH 6/10 vfio-ccw: Introduce a new CRW region
In reply to Eric FarmanFrom: Farhan Ali <[email protected]>
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()).
---
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(+)
drivers/s390/cio/vfio_ccw_chp.c +53 -0
@@ -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); +}
drivers/s390/cio/vfio_ccw_drv.c +20 -0
@@ -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) {
drivers/s390/cio/vfio_ccw_ops.c +4 -0
@@ -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:
drivers/s390/cio/vfio_ccw_private.h +3 -0
@@ -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;
include/uapi/linux/vfio.h +1 -0
@@ -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
include/uapi/linux/vfio_ccw.h +5 -0
@@ -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
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 06/10] vfio-ccw: Introduce a new CRW region
Quoted 4 lines · On Fri, 15 Nov 2019 03:56:16 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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?
Quoted 3 lines
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?)
Quoted 40 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v0->v1: [EF]
- Clean up checkpatch (whitespace) errors
- Add ret=-ENOMEM in error path for new region
- Addio_mutexfor region read (originally in last patch)
- Change crw1/crw2 to crw0/crw1
- Reorder cleanup of regionsdrivers/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(+)drivers/s390/cio/vfio_ccw_chp.c +11
-0@@ -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))
drivers/s390/cio/vfio_ccw_chp.c int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private) · line 96
Quoted 10 lines
drivers/s390/cio/vfio_ccw_chp.c +10
-0+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?
Quoted 4 lines
drivers/s390/cio/vfio_ccw_chp.c +4
-0+mutex_unlock (&private->io_mutex); +return ret; +} +
(...)
include/uapi/linux/vfio_ccw.h struct ccw_schib_region { · line 41
Quoted 8 lines
include/uapi/linux/vfio_ccw.h +0
-0@@ -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.
Quoted 6 lines
include/uapi/linux/vfio_ccw.h +5
-0+struct ccw_crw_region { +__u32 crw0; +__u32 crw1; +} __packed; +#endif
Eric Farman
maintainerRFC PATCH 7/10 vfio-ccw: Refactor IRQ handlers
In reply to Eric FarmanTo simplify future expansion.
--- drivers/s390/cio/vfio_ccw_ops.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
drivers/s390/cio/vfio_ccw_ops.c +17 -6
@@ -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);
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 07/10] vfio-ccw: Refactor IRQ handlers
Quoted 6 lines · On Fri, 15 Nov 2019 03:56:17 +0100 Eric Farman <[email protected]> wrote:
To simplify future expansion.
Signed-off-by Eric Farman <[email protected]>--- drivers/s390/cio/vfio_ccw_ops.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
Eric Farman
maintainerRFC PATCH 8/10 vfio-ccw: Wire up the CRW irq and CRW region
In reply to Eric FarmanFrom: Farhan Ali <[email protected]>
Use an IRQ to notify userspace that there is a CRW pending in the region, related to path-availability changes on the passthrough subchannel.
---
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(+)
drivers/s390/cio/vfio_ccw_drv.c +27 -0
@@ -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 ; }
drivers/s390/cio/vfio_ccw_ops.c +4 -0
@@ -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 ; }
drivers/s390/cio/vfio_ccw_private.h +4 -0
@@ -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);
include/uapi/linux/vfio.h +1 -0
@@ -579,6 +579,7 @@ enum {enum {VFIO_CCW_IO_IRQ_INDEX , +VFIO_CCW_CRW_IRQ_INDEX ,VFIO_CCW_NUM_IRQS };
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 08/10] vfio-ccw: Wire up the CRW irq and CRW region
Quoted 5 lines · On Fri, 15 Nov 2019 03:56:18 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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?
Quoted 29 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
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 ofio_mutexinvfio_ccw_crw_region_read()into patch that introduces that region
- Remove duplicate include fromvfio_ccw_drv.c
- Reorder include invfio_ccw_private.hdrivers/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(+)drivers/s390/cio/vfio_ccw_drv.c +0
-0@@ -108,6 +108,22 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)eventfd_signal (private->io_trigger,1 ); }
drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 120
Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +10
-0+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?
Quoted 18 lines
drivers/s390/cio/vfio_ccw_drv.c +7
-0+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 ;
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_chp_event(struct subchannel *sch, · line 324
Quoted 5 lines
drivers/s390/cio/vfio_ccw_drv.c +2
-0@@ -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?
Quoted 7 lines
drivers/s390/cio/vfio_ccw_drv.c +3
-0+ 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 */
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_chp_event(struct subchannel *sch, · line 341
Quoted 8 lines
drivers/s390/cio/vfio_ccw_drv.c +5
-0@@ -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?
Quoted 3 lines
drivers/s390/cio/vfio_ccw_drv.c +0
-0break ; }
Eric Farman
maintainerRe: [RFC PATCH v1 08/10] vfio-ccw: Wire up the CRW irq and CRW region
Quoted 5 lines · Cornelia Huck
Quoted 2 lines · On Fri, 15 Nov 2019 03:56:18 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
Quoted 7 lines
Quoted 3 lines
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?
Quoted 44 lines
Quoted 30 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
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 ofio_mutexinvfio_ccw_crw_region_read()into patch that introduces that region
- Remove duplicate include fromvfio_ccw_drv.c
- Reorder include invfio_ccw_private.hdrivers/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(+)drivers/s390/cio/vfio_ccw_drv.c +0
-0@@ -108,6 +108,22 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)eventfd_signal (private->io_trigger,1 ); }drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 120
Quoted 10 lines
drivers/s390/cio/vfio_ccw_drv.c +10
-0+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.
Quoted 3 lines
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.
Quoted 18 lines
Quoted 18 lines
drivers/s390/cio/vfio_ccw_drv.c +7
-0+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 ;
Quoted 8 lines
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_chp_event(struct subchannel *sch, · line 324
Quoted 6 lines
drivers/s390/cio/vfio_ccw_drv.c +2
-0@@ -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. :)
Quoted 13 lines
Quoted 13 lines
drivers/s390/cio/vfio_ccw_drv.c +5
-0+ 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) |
Quoted 8 lines
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_chp_event(struct subchannel *sch, · line 341
Quoted 3 lines
drivers/s390/cio/vfio_ccw_drv.c +3
-0+ 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.
Quoted 2 lines
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.
Quoted 5 lines
Quoted 4 lines
drivers/s390/cio/vfio_ccw_drv.c +0
-0break ; }
Re: [RFC PATCH v1 08/10] vfio-ccw: Wire up the CRW irq and CRW region
Quoted 14 lines · On Thu, 5 Dec 2019 15:43:55 -0500 Eric Farman <[email protected]> wrote:
Quoted 5 lines · Cornelia Huck
Quoted 2 lines · On Fri, 15 Nov 2019 03:56:18 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
Quoted 7 lines
Quoted 3 lines
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?
Quoted 9 lines
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.
Quoted 7 lines
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.
(...)
Quoted 19 lines
Quoted 6 lines
Quoted 6 lines
drivers/s390/cio/vfio_ccw_drv.c +2
-0@@ -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) |Quoted 8 lines
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_chp_event(struct subchannel *sch, · line 341
Quoted 3 lines
drivers/s390/cio/vfio_ccw_drv.c +3
-0+ 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.
Quoted 1 line
Quoted 7 lines
Quoted 1 line
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.
Quoted 5 lines
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?
Eric Farman
maintainerRe: [RFC PATCH v1 08/10] vfio-ccw: Wire up the CRW irq and CRW region
Quoted 41 lines · Cornelia Huck
Quoted 14 lines · On Thu, 5 Dec 2019 15:43:55 -0500 Eric Farman <[email protected]> wrote:
Quoted 5 lines · Cornelia Huck
Quoted 2 lines · On Fri, 15 Nov 2019 03:56:18 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
Quoted 7 lines
Quoted 3 lines
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?
Quoted 9 lines
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.
Quoted 7 lines
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?
Quoted 2 lines
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. :)
Quoted 2 lines
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!
Quoted 39 lines
(...)
Quoted 19 lines
Quoted 6 lines
Quoted 6 lines
drivers/s390/cio/vfio_ccw_drv.c +2
-0@@ -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) |Quoted 8 lines
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_chp_event(struct subchannel *sch, · line 341
Quoted 3 lines
drivers/s390/cio/vfio_ccw_drv.c +3
-0+ 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.
Quoted 1 line
Quoted 7 lines
Quoted 1 line
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.>
Quoted 1 line
Quoted 7 lines
Quoted 4 lines
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
Quoted 9 lines
...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
Quoted 15 lines · On Fri, 6 Dec 2019 16:24:31 -0500 Eric Farman <[email protected]> wrote:
Quoted 14 lines · Cornelia Huck
Quoted 10 lines · On Thu, 5 Dec 2019 15:43:55 -0500 Eric Farman <[email protected]> wrote:
Quoted 2 lines · Cornelia Huck
On Fri, 15 Nov 2019 03:56:18 +0100
Eric Farman <[email protected]> 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?
Quoted 7 lines
Quoted 2 lines
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.
Quoted 9 lines
Quoted 3 lines
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 thecrw_regioneach 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.
Quoted 45 lines
Quoted 45 lines
(...)
Quoted 19 lines
Quoted 6 lines
Quoted 6 lines
drivers/s390/cio/vfio_ccw_drv.c +2
-0@@ -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) |Quoted 8 lines
drivers/s390/cio/vfio_ccw_drv.c static int vfio_ccw_chp_event(struct subchannel *sch, · line 341
Quoted 3 lines
drivers/s390/cio/vfio_ccw_drv.c +3
-0+ 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.
Quoted 1 line
Quoted 7 lines
Quoted 1 line
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.>
Quoted 5 lines
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.
Quoted 4 lines
Quoted 2 lines
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 :/
Quoted 11 lines
Quoted 10 lines
...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?
Quoted 8 lines
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.
Eric Farman
maintainerRFC PATCH 9/10 vfio-ccw: Add trace for CRW event
In reply to Eric FarmanSince CRW events are (should be) rare, let's put a trace in that routine too.
--- 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(+)
drivers/s390/cio/vfio_ccw_drv.c +1 -0
@@ -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,
drivers/s390/cio/vfio_ccw_trace.c +1 -0
@@ -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);
drivers/s390/cio/vfio_ccw_trace.h +30 -0
@@ -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,
Signature · 1 line
2.17.1
Eric Farman
maintainerRFC PATCH 10/10 vfio-ccw: Remove inline get_schid() routine
In reply to Eric FarmanThis 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.
--- drivers/s390/cio/vfio_ccw_fsm.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/s390/cio/vfio_ccw_fsm.c +2 -6
@@ -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); }
Signature · 1 line
2.17.1
Quoted 12 lines · On Fri, 15 Nov 2019 03:56:10 +0100 Eric Farman <[email protected]> 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.
Quoted 4 lines
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.
Quoted 4 lines
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.
Quoted 15 lines
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 inlineget_schid()routineFarhan Ali (6):
vfio-ccw: Introduce new helper functions to free/destroy regions
vfio-ccw: Register achp_eventcallback 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
Quoted 13 lines
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.)