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
>
