Re: [PATCH v1 1/2] vfio: ccw: Merge BUSY and BOXED states

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 18 Oct 2018 14:38:22 +0200
Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:

> On 10/18/2018 09:51 AM, Pierre Morel wrote:
> > VFIO_CCW_STATE_BOXED and VFIO_CCW_STATE_BUSY have
> > identical actions for the same events.
> > 
> > Let's merge both into a single state to simplify the code.
> > We choose to keep VFIO_CCW_STATE_BUSY.
> >  
> 
> Your commit message does not convince me. From what I
> see VFIO_CCW_STATE_BOXED was a bad idea in the first
> place:
> * is very very different than DEV_STATE_BOXED,
> * its role is obscure and undocumented.
> 
> With your patch however, we get a weird VFIO_CCW_STATE_BUSY -->
> VFIO_CCW_STATE_BUSY state transition, which I don't consider
> much better:
> First we go into VFIO_CCW_STATE_BUSY in fsm_io_request() and then
> again in fsm_io_helper() which is called by fsm_io_request().
> 
> So good path used to go
> 
> VFIO_CCW_STATE_IDLE --> VFIO_CCW_STATE_BOXED --> VFIO_CCW_STATE_BUSY --> ?
> and with your patch it goes 
> VFIO_CCW_STATE_IDLE --> VFIO_CCW_STATE_BUSY --> VFIO_CCW_STATE_BUSY --> ?
> 
> From what I see, the role of VFIO_CCW_STATE_BOXED is to differentiate
> between 'doing I/O on the host subchannel' and 'looking at the request,
> doing translation, and whatever we need to before interacting with the
> host subchannel'. Normally state is exposed via sysfs, but for vfio-ccw
> it ain't. Does anybody know why?
> 
> I don't think we need to differentiate between the two, so I'm not against
> removing BOXED. But I'm against the VFIO_CCW_STATE_BUSY --> VFIO_CCW_STATE_BUSY
> transition, and we need a better commit message.

OK, so I can pick patch 2 (if you're happy with that) and we can
discuss this one later.

(Well, I'll probably pick patch 2 next week, but you get the meaning :)

> 
> Regards,
> Halil
> 
> > Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> > ---
> >  drivers/s390/cio/vfio_ccw_fsm.c     | 7 +------
> >  drivers/s390/cio/vfio_ccw_private.h | 1 -
> >  2 files changed, 1 insertion(+), 7 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> > index f94aa01..cab1786 100644
> > --- a/drivers/s390/cio/vfio_ccw_fsm.c
> > +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> > @@ -130,7 +130,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_BOXED;
> > +	private->state = VFIO_CCW_STATE_BUSY;
> > 
> >  	memcpy(scsw, io_region->scsw_area, sizeof(*scsw));
> > 
> > @@ -216,11 +216,6 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
> >  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_request,
> >  		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
> >  	},
> > -	[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_STATE_BUSY] = {
> >  		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
> >  		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
> > diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> > index 078e46f..08e9a7d 100644
> > --- a/drivers/s390/cio/vfio_ccw_private.h
> > +++ b/drivers/s390/cio/vfio_ccw_private.h
> > @@ -63,7 +63,6 @@ enum vfio_ccw_state {
> >  	VFIO_CCW_STATE_NOT_OPER,
> >  	VFIO_CCW_STATE_STANDBY,
> >  	VFIO_CCW_STATE_IDLE,
> > -	VFIO_CCW_STATE_BOXED,
> >  	VFIO_CCW_STATE_BUSY,
> >  	/* last element! */
> >  	NR_VFIO_CCW_STATES
> >   
> 




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux