Re: [Qemu-devel] [RFC PATCH 1/3] vfio: ccw: introduce schib region

[Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>]

* Cornelia Huck <cohuck@redhat.com> [2018-01-11 15:16:59 +0100]:

Hi Conny,

> On Thu, 11 Jan 2018 04:04:19 +0100
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > This introduces a new region for vfio-ccw to provide subchannel
> > information for user space.
> > 
> > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > ---
> >  drivers/s390/cio/vfio_ccw_fsm.c     | 21 ++++++++++
> >  drivers/s390/cio/vfio_ccw_ops.c     | 79 +++++++++++++++++++++++++++----------
> >  drivers/s390/cio/vfio_ccw_private.h |  3 ++
> >  include/uapi/linux/vfio.h           |  1 +
> >  include/uapi/linux/vfio_ccw.h       |  6 +++
> >  5 files changed, 90 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> > index c30420c517b1..be081ccabea3 100644
> > --- a/drivers/s390/cio/vfio_ccw_fsm.c
> > +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> > @@ -172,6 +172,22 @@ static void fsm_irq(struct vfio_ccw_private *private,
> >  		complete(private->completion);
> >  }
> >  
> > +static void fsm_update_subch(struct vfio_ccw_private *private,
> > +			     enum vfio_ccw_event event)
> > +{
> > +	struct subchannel *sch;
> > +
> > +	sch = private->sch;
> > +	if (cio_update_schib(sch)) {
> 
> This implies device gone. Do we also want to trigger some event, or
> just wait until a machine check comes around and we're notified in the
> normal way? (Probably the latter.)
> 
We'd need to handle machine checks better anyway, and we can trigger
event there. I think we can choose the latter one.

> > +		private->schib_region.cc = 3;
> > +		return;
> > +	}
> > +
> > +	private->schib_region.cc = 0;
> > +	memcpy(private->schib_region.schib_area, &sch->schib,
> > +	       sizeof(sch->schib));
> 
> We might want to add documentation that schib_area contains the schib
> from the last successful invocation of stsch (if any). That makes sense
> as the schib remains unchanged for cc=3 after stsch anyway, but it
> can't hurt to spell it out.
> 
PoP doesn't say anything about the content of SCHIB when cc=3. So it's
fine to remain the last content I guess. I can add comments here and
document in vfio-ccw.txt. Ok?

> > +}
> > +
> >  /*
> >   * Device statemachine
> >   */
> > @@ -180,25 +196,30 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
> >  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_nop,
> >  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
> >  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
> > +		[VFIO_CCW_EVENT_UPDATE_SUBCH]	= fsm_update_subch,
> >  	},
> >  	[VFIO_CCW_STATE_STANDBY] = {
> >  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> >  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
> >  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> > +		[VFIO_CCW_EVENT_UPDATE_SUBCH]	= fsm_update_subch,
> >  	},
> >  	[VFIO_CCW_STATE_IDLE] = {
> >  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> >  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_request,
> >  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> > +		[VFIO_CCW_EVENT_UPDATE_SUBCH]	= fsm_update_subch,
> >  	},
> >  	[VFIO_CCW_STATE_BOXED] = {
> >  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> >  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
> >  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> > +		[VFIO_CCW_EVENT_UPDATE_SUBCH]	= fsm_update_subch,
> >  	},
> >  	[VFIO_CCW_STATE_BUSY] = {
> >  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> >  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
> >  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> > +		[VFIO_CCW_EVENT_UPDATE_SUBCH]	= fsm_update_subch,
> 
> Does it makes to trigger this through the state machine if we always do
> the same action and never change state?
Yes. Ah, are you implying that we can call update_subch directly without
state machine involved? If so, I agree. There seems no benifit to add
a new VFIO_CCW_EVENT_UPDATE_SUBCH event to the FSM.

> 
> >  	},
> >  };
> 
> Else, looks good.
> 
Thanks for the comments. :)

-- 
Dong Jia Shi