| Message ID | 20200206213825.11444-3-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/vfio-ccw: Channel Path Handling | expand |
On Thu, 6 Feb 2020 22:38:18 +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. I'm wondering how useful this patch would be on its own. > > Signed-off-by: Farhan Ali <alifm@linux.ibm.com> > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > > Notes: > v1->v2: > - Move s390dbf before cio_update_schib() call [CH] > > v0->v1: [EF] > - Add s390dbf trace > > drivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > (...) > @@ -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; > + > + 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 (cio_update_schib(sch)) > + return -ENODEV; > + > + 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); Any reason you do this only for CHP_OFFLINE and not for CHP_VARY_OFF? > + 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; If I'm not mistaken, this patch introduces the first usage of sch->opm in the vfio-ccw code. Are we missing something? Or am I missing something? :) > + break; > + } > + > + return 0; > +} > + > static struct css_device_id vfio_ccw_sch_ids[] = { > { .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, }, > { /* end of list */ }, (...)
On 2/14/20 7:11 AM, Cornelia Huck wrote: > On Thu, 6 Feb 2020 22:38:18 +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. > > I'm wondering how useful this patch would be on its own. Probably not much. I can't speak to his original thoughts on the matter, but it doesn't seem to buy us much by itself other than a consumable sized patch. > >> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com> >> Signed-off-by: Eric Farman <farman@linux.ibm.com> >> --- >> >> Notes: >> v1->v2: >> - Move s390dbf before cio_update_schib() call [CH] >> >> v0->v1: [EF] >> - Add s390dbf trace >> >> drivers/s390/cio/vfio_ccw_drv.c | 44 +++++++++++++++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> > (...) >> @@ -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; >> + >> + 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 (cio_update_schib(sch)) >> + return -ENODEV; >> + >> + 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); > > Any reason you do this only for CHP_OFFLINE and not for CHP_VARY_OFF? Hrm... No reason that I can think of. I can fix this. > >> + 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; > > If I'm not mistaken, this patch introduces the first usage of sch->opm > in the vfio-ccw code. Correct. > Are we missing something? Maybe? :) >Or am I missing > something? :) > Since it's only used in this code, for acting as a step between vary/config off/on, maybe this only needs to be dealing with the lpm field itself? >> + break; >> + } >> + >> + return 0; >> +} >> + >> static struct css_device_id vfio_ccw_sch_ids[] = { >> { .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, }, >> { /* end of list */ }, > (...) >
On Fri, 14 Feb 2020 11:35:21 -0500 Eric Farman <farman@linux.ibm.com> wrote: > On 2/14/20 7:11 AM, Cornelia Huck wrote: > > On Thu, 6 Feb 2020 22:38:18 +0100 > > Eric Farman <farman@linux.ibm.com> wrote: > > (...) > >> @@ -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; > >> + > >> + 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 (cio_update_schib(sch)) > >> + return -ENODEV; > >> + > >> + 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); > > > > Any reason you do this only for CHP_OFFLINE and not for CHP_VARY_OFF? > > Hrm... No reason that I can think of. I can fix this. > > > > >> + 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; > > > > If I'm not mistaken, this patch introduces the first usage of sch->opm > > in the vfio-ccw code. > > Correct. > > > Are we missing something? > > Maybe? :) > > >Or am I missing > > something? :) > > > > Since it's only used in this code, for acting as a step between > vary/config off/on, maybe this only needs to be dealing with the lpm > field itself? Ok, I went over this again and also looked at what the standard I/O subchannel driver does, and I think this is fine, as the lpm basically factors in the opm already. (Will need to keep this in mind for the following patches.) > > >> + break; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> static struct css_device_id vfio_ccw_sch_ids[] = { > >> { .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, }, > >> { /* end of list */ }, > > (...) > > >
On 3/24/20 11:58 AM, Cornelia Huck wrote: > On Fri, 14 Feb 2020 11:35:21 -0500 > Eric Farman <farman@linux.ibm.com> wrote: > >> On 2/14/20 7:11 AM, Cornelia Huck wrote: >>> On Thu, 6 Feb 2020 22:38:18 +0100 >>> Eric Farman <farman@linux.ibm.com> wrote: > >>> (...) >>>> @@ -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; >>>> + >>>> + 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 (cio_update_schib(sch)) >>>> + return -ENODEV; >>>> + >>>> + 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); >>> >>> Any reason you do this only for CHP_OFFLINE and not for CHP_VARY_OFF? >> >> Hrm... No reason that I can think of. I can fix this. >> >>> >>>> + 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; >>> >>> If I'm not mistaken, this patch introduces the first usage of sch->opm >>> in the vfio-ccw code. >> >> Correct. >> >>> Are we missing something? >> >> Maybe? :) >> >>> Or am I missing >>> something? :) >>> >> >> Since it's only used in this code, for acting as a step between >> vary/config off/on, maybe this only needs to be dealing with the lpm >> field itself? > > Ok, I went over this again and also looked at what the standard I/O > subchannel driver does, and I think this is fine, as the lpm basically > factors in the opm already. (Will need to keep this in mind for the > following patches.) Just to make sure I don't misunderstand, when you say "I think this is fine" ... Do you mean keeping the opm field within vfio-ccw, as this patch does? Or removing it, and only adjusting the lpm within vfio-ccw, as I suggested in my response just above? (It's long in the day, and should not look at vfio-ccw at this hour.) > >> >>>> + break; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static struct css_device_id vfio_ccw_sch_ids[] = { >>>> { .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, }, >>>> { /* end of list */ }, >>> (...) >>> >> >
On Wed, 25 Mar 2020 22:09:40 -0400 Eric Farman <farman@linux.ibm.com> wrote: > On 3/24/20 11:58 AM, Cornelia Huck wrote: > > On Fri, 14 Feb 2020 11:35:21 -0500 > > Eric Farman <farman@linux.ibm.com> wrote: > > > >> On 2/14/20 7:11 AM, Cornelia Huck wrote: > >>> On Thu, 6 Feb 2020 22:38:18 +0100 > >>> Eric Farman <farman@linux.ibm.com> wrote: > >>>> + case CHP_ONLINE: > >>>> + /* Path became available */ > >>>> + sch->lpm |= mask & sch->opm; > >>> > >>> If I'm not mistaken, this patch introduces the first usage of sch->opm > >>> in the vfio-ccw code. > >> > >> Correct. > >> > >>> Are we missing something? > >> > >> Maybe? :) > >> > >>> Or am I missing > >>> something? :) > >>> > >> > >> Since it's only used in this code, for acting as a step between > >> vary/config off/on, maybe this only needs to be dealing with the lpm > >> field itself? > > > > Ok, I went over this again and also looked at what the standard I/O > > subchannel driver does, and I think this is fine, as the lpm basically > > factors in the opm already. (Will need to keep this in mind for the > > following patches.) > > Just to make sure I don't misunderstand, when you say "I think this is > fine" ... Do you mean keeping the opm field within vfio-ccw, as this > patch does? Or removing it, and only adjusting the lpm within vfio-ccw, > as I suggested in my response just above? I meant the code change done in this patch: We update the lpm whenever the opm is changed, and use the lpm. I'd like to keep the opm separate, just so that we are clear where each value comes from.
On 3/26/20 2:47 AM, Cornelia Huck wrote: > On Wed, 25 Mar 2020 22:09:40 -0400 > Eric Farman <farman@linux.ibm.com> wrote: > >> On 3/24/20 11:58 AM, Cornelia Huck wrote: >>> On Fri, 14 Feb 2020 11:35:21 -0500 >>> Eric Farman <farman@linux.ibm.com> wrote: >>> >>>> On 2/14/20 7:11 AM, Cornelia Huck wrote: >>>>> On Thu, 6 Feb 2020 22:38:18 +0100 >>>>> Eric Farman <farman@linux.ibm.com> wrote: > >>>>>> + case CHP_ONLINE: >>>>>> + /* Path became available */ >>>>>> + sch->lpm |= mask & sch->opm; >>>>> >>>>> If I'm not mistaken, this patch introduces the first usage of sch->opm >>>>> in the vfio-ccw code. >>>> >>>> Correct. >>>> >>>>> Are we missing something? >>>> >>>> Maybe? :) >>>> >>>>> Or am I missing >>>>> something? :) >>>>> >>>> >>>> Since it's only used in this code, for acting as a step between >>>> vary/config off/on, maybe this only needs to be dealing with the lpm >>>> field itself? >>> >>> Ok, I went over this again and also looked at what the standard I/O >>> subchannel driver does, and I think this is fine, as the lpm basically >>> factors in the opm already. (Will need to keep this in mind for the >>> following patches.) >> >> Just to make sure I don't misunderstand, when you say "I think this is >> fine" ... Do you mean keeping the opm field within vfio-ccw, as this >> patch does? Or removing it, and only adjusting the lpm within vfio-ccw, >> as I suggested in my response just above? > > I meant the code change done in this patch: We update the lpm whenever > the opm is changed, and use the lpm. I'd like to keep the opm separate, > just so that we are clear where each value comes from. > Great. Thanks for that clarification.
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index 91989269faf1..a99705e2fd73 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; + + 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 (cio_update_schib(sch)) + return -ENODEV; + + 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)