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 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.

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