On Tue, 5 Feb 2019 09:31:55 -0500 Eric Farman <farman@xxxxxxxxxxxxx> wrote: > On 02/05/2019 07:10 AM, Cornelia Huck wrote: > > On Mon, 4 Feb 2019 16:29:40 -0500 > > Eric Farman <farman@xxxxxxxxxxxxx> wrote: > > > >> On 01/30/2019 08:22 AM, Cornelia Huck wrote: > >>> The flow for processing ssch requests can be improved by splitting > >>> the BUSY state: > >>> > >>> - CP_PROCESSING: We reject any user space requests while we are in > >>> the process of translating a channel program and submitting it to > >>> the hardware. Use -EAGAIN to signal user space that it should > >>> retry the request. > >>> - CP_PENDING: We have successfully submitted a request with ssch and > >>> are now expecting an interrupt. As we can't handle more than one > >>> channel program being processed, reject any further requests with > >>> -EBUSY. A final interrupt will move us out of this state; this also > >>> fixes a latent bug where a non-final interrupt might have freed up > >>> a channel program that still was in progress. > >>> By making this a separate state, we make it possible to issue a > >>> halt or a clear while we're still waiting for the final interrupt > >>> for the ssch (in a follow-on patch). > >>> > >>> It also makes a lot of sense not to preemptively filter out writes to > >>> the io_region if we're in an incorrect state: the state machine will > >>> handle this correctly. > >>> > >>> Signed-off-by: Cornelia Huck <cohuck@xxxxxxxxxx> > >>> --- > >>> drivers/s390/cio/vfio_ccw_drv.c | 8 ++++++-- > >>> drivers/s390/cio/vfio_ccw_fsm.c | 19 ++++++++++++++----- > >>> drivers/s390/cio/vfio_ccw_ops.c | 2 -- > >>> drivers/s390/cio/vfio_ccw_private.h | 3 ++- > >>> 4 files changed, 22 insertions(+), 10 deletions(-) > > > >>> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c > >>> index e7c9877c9f1e..b4a141fbd1a8 100644 > >>> --- a/drivers/s390/cio/vfio_ccw_fsm.c > >>> +++ b/drivers/s390/cio/vfio_ccw_fsm.c > >>> @@ -28,7 +28,6 @@ static int fsm_io_helper(struct vfio_ccw_private *private) > >>> sch = private->sch; > >>> > >>> spin_lock_irqsave(sch->lock, flags); > >>> - private->state = VFIO_CCW_STATE_BUSY; > >>> > >>> orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm); > >>> if (!orb) { > >>> @@ -46,6 +45,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private) > >>> */ > >>> sch->schib.scsw.cmd.actl |= SCSW_ACTL_START_PEND; > >>> ret = 0; > >>> + private->state = VFIO_CCW_STATE_CP_PENDING; > >> > >> [1] > >> > >>> break; > >>> case 1: /* Status pending */ > >>> case 2: /* Busy */ > >>> @@ -107,6 +107,12 @@ static void fsm_io_busy(struct vfio_ccw_private *private, > >>> private->io_region->ret_code = -EBUSY; > >>> } > >>> > >>> +static void fsm_io_retry(struct vfio_ccw_private *private, > >>> + enum vfio_ccw_event event) > >>> +{ > >>> + private->io_region->ret_code = -EAGAIN; > >>> +} > >>> + > >>> static void fsm_disabled_irq(struct vfio_ccw_private *private, > >>> enum vfio_ccw_event event) > >>> { > >>> @@ -135,8 +141,7 @@ static void fsm_io_request(struct vfio_ccw_private *private, > >>> struct mdev_device *mdev = private->mdev; > >>> char *errstr = "request"; > >>> > >>> - private->state = VFIO_CCW_STATE_BUSY; > >>> - > >>> + private->state = VFIO_CCW_STATE_CP_PROCESSING; > >> > >> [1] > >> > >>> memcpy(scsw, io_region->scsw_area, sizeof(*scsw)); > >>> > >>> if (scsw->cmd.fctl & SCSW_FCTL_START_FUNC) { > >>> @@ -181,7 +186,6 @@ static void fsm_io_request(struct vfio_ccw_private *private, > >>> } > >>> > >>> err_out: > >>> - private->state = VFIO_CCW_STATE_IDLE; > >> > >> [1] Revisiting these locations as from an earlier discussion [2]... > >> These go IDLE->CP_PROCESSING->CP_PENDING if we get a cc=0 on the SSCH, > >> but we stop in CP_PROCESSING if the SSCH gets a nonzero cc. Shouldn't > >> we cleanup and go back to IDLE in this scenario, rather than forcing > >> userspace to escalate to CSCH/HSCH after some number of retries (via FSM)? > >> > >> [2] https://patchwork.kernel.org/patch/10773611/#22447997 > > > > It does do that (in vfio_ccw_mdev_write), it was not needed here. Or do > > you think doing it here would be more obvious? > > Ah, my mistake, I missed that. (That function is renamed to > vfio_ccw_mdev_write_io_region in patch 4.) > > I don't think keeping it here is necessary then. I got too focused > looking at what you ripped out that I lost the things that stayed. Once > this series gets in its entirety, and Pierre has a chance to rebase his > FSM series on top of it all, this should be in great shape. Yeah, it's probably easier to look at the end result. > > > > >> > >> Besides that, I think this looks good to me. > > > > Thanks! > > > > You're welcome! Here, have a thing to add to this patch: > > Reviewed-by: Eric Farman <farman@xxxxxxxxxxxxx> > Thanks a lot!