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?
>
> Besides that, I think this looks good to me.
Thanks!
