Re: [PATCH v3 2/6] vfio-ccw: rework ssch state handling

[Eric Farman <farman@linux.ibm.com>]

On 02/05/2019 07:10 AM, Cornelia Huck wrote:
> On Mon, 4 Feb 2019 16:29:40 -0500
> Eric Farman <farman@linux.ibm.com> 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@redhat.com>
>>> ---
>>>    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.

> 
>>
>> 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@linux.ibm.com>