Re: [PATCH v3 11/11] vfio/ccw: Move FSM open/close to MDEV open/close

Matthew Rosato
2022-07-05T20:17:14Z

On 6/30/22 4:36 PM, Eric Farman wrote:
> Part of the confusion that has existed is the FSM lifecycle of
> subchannels between the common CSS driver and the vfio-ccw driver.
> During configuration, the FSM state goes from NOT_OPER to STANDBY
> to IDLE, but then back to NOT_OPER. For example:
>
> vfio_ccw_sch_probe: VFIO_CCW_STATE_NOT_OPER
> vfio_ccw_sch_probe: VFIO_CCW_STATE_STANDBY
> vfio_ccw_mdev_probe: VFIO_CCW_STATE_IDLE
> vfio_ccw_mdev_remove: VFIO_CCW_STATE_NOT_OPER
> vfio_ccw_sch_remove: VFIO_CCW_STATE_NOT_OPER
> vfio_ccw_sch_shutdown: VFIO_CCW_STATE_NOT_OPER
>
> Rearrange the open/close events to align with the mdev open/close,
> to better manage the memory and state of the devices as time
> progresses. Specifically, make mdev_open() perform the FSM open,
> and mdev_close() perform the FSM close instead of reset (which is
> both close and open).
>
> This makes the NOT_OPER state a dead-end path, indicating the
> device is probably not recoverable without fully probing and
> re-configuring the device.
>
> This has the nice side-effect of removing a number of special-cases
> where the FSM state is managed outside of the FSM itself (such as
> the aforementioned mdev_close() routine).
>
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
> drivers/s390/cio/vfio_ccw_drv.c | 11 +++--------
> drivers/s390/cio/vfio_ccw_fsm.c | 32 +++++++++++++++++++++++---------
> drivers/s390/cio/vfio_ccw_ops.c | 26 +++++++++++---------------
> 3 files changed, 37 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index f98c9915e73d..4804101ccb0f 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -138,7 +138,7 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
>
> private->sch = sch;
> mutex_init(&private->io_mutex);
> - private->state = VFIO_CCW_STATE_NOT_OPER;
> + private->state = VFIO_CCW_STATE_STANDBY;
> INIT_LIST_HEAD(&private->crw);
> INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
> INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
> @@ -222,21 +222,15 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
>
> dev_set_drvdata(&sch->dev, private);
>
> - vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
> - if (private->state == VFIO_CCW_STATE_NOT_OPER)
> - goto out_free;
> -
> ret = mdev_register_device(&sch->dev, &vfio_ccw_mdev_driver);
> if (ret)
> - goto out_disable;
> + goto out_free;
>
> VFIO_CCW_MSG_EVENT(4, "bound to subchannel %x.%x.%04x\n",
> sch->schid.cssid, sch->schid.ssid,
> sch->schid.sch_no);
> return 0;
>
> -out_disable:
> - cio_disable_subchannel(sch);
> out_free:
> dev_set_drvdata(&sch->dev, NULL);
> vfio_ccw_free_private(private);
> @@ -264,6 +258,7 @@ static void vfio_ccw_sch_shutdown(struct subchannel *sch)
> struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>
> vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
> + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> }
>
> /**
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 89eb3feffa41..472e77f1bb6e 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -175,6 +175,7 @@ static void fsm_notoper(struct vfio_ccw_private *private,
> */
> css_sched_sch_todo(sch, SCH_TODO_UNREG);
> private->state = VFIO_CCW_STATE_NOT_OPER;
> + cp_free(&private->cp);
> }
>
> /*
> @@ -379,9 +380,16 @@ static void fsm_open(struct vfio_ccw_private *private,
> spin_lock_irq(sch->lock);
> sch->isc = VFIO_CCW_ISC;
> ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
> - if (!ret)
> - private->state = VFIO_CCW_STATE_STANDBY;
> + if (ret)
> + goto err_unlock;
> +
> + private->state = VFIO_CCW_STATE_IDLE;
> spin_unlock_irq(sch->lock);
> + return;
> +
> +err_unlock:
> + spin_unlock_irq(sch->lock);
> + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> }
>
> static void fsm_close(struct vfio_ccw_private *private,
> @@ -393,16 +401,22 @@ static void fsm_close(struct vfio_ccw_private *private,
> spin_lock_irq(sch->lock);
>
> if (!sch->schib.pmcw.ena)
> - goto out_unlock;
> + goto err_unlock;
>
> ret = cio_disable_subchannel(sch);
> if (ret == -EBUSY)
> vfio_ccw_sch_quiesce(sch);
> + if (ret)
> + goto err_unlock;
>
> -out_unlock:
> - private->state = VFIO_CCW_STATE_NOT_OPER;
> + private->state = VFIO_CCW_STATE_STANDBY;
> spin_unlock_irq(sch->lock);
> cp_free(&private->cp);
> + return;
> +
> +err_unlock:
> + spin_unlock_irq(sch->lock);
> + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> }
>
> /*
> @@ -414,16 +428,16 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
> [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error,
> [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_error,
> [VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq,
> - [VFIO_CCW_EVENT_OPEN] = fsm_open,
> + [VFIO_CCW_EVENT_OPEN] = fsm_nop,
> [VFIO_CCW_EVENT_CLOSE] = fsm_nop,
> },
> [VFIO_CCW_STATE_STANDBY] = {
> [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error,
> [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_error,
> - [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
> - [VFIO_CCW_EVENT_OPEN] = fsm_notoper,
> - [VFIO_CCW_EVENT_CLOSE] = fsm_close,
> + [VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq,
> + [VFIO_CCW_EVENT_OPEN] = fsm_open,
> + [VFIO_CCW_EVENT_CLOSE] = fsm_notoper,
> },
> [VFIO_CCW_STATE_IDLE] = {
> [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 4673b7ddfe20..bc2176421dc5 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -24,17 +24,12 @@ static int vfio_ccw_mdev_reset(struct vfio_ccw_private *private)
> /*
> * If the FSM state is seen as Not Operational after closing
> * and re-opening the mdev, return an error.
> - *
> - * Otherwise, change the FSM from STANDBY to IDLE which is
> - * normally done by vfio_ccw_mdev_probe() in current lifecycle.
> */
> vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
> vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
> if (private->state == VFIO_CCW_STATE_NOT_OPER)
> return -EINVAL;
>
> - private->state = VFIO_CCW_STATE_IDLE;
> -
> return 0;
> }
>
> @@ -121,8 +116,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
> vfio_init_group_dev(&private->vdev, &mdev->dev,
> &vfio_ccw_dev_ops);
>
> - private->state = VFIO_CCW_STATE_IDLE;
> -
> VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
> private->sch->schid.cssid,
> private->sch->schid.ssid,
> @@ -137,7 +130,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
> err_atomic:
> vfio_uninit_group_dev(&private->vdev);
> atomic_inc(&private->avail);
> - private->state = VFIO_CCW_STATE_STANDBY;
> return ret;
> }
>
> @@ -165,6 +157,10 @@ static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
> unsigned long events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
> int ret;
>
> + /* Device cannot simply be opened again from this state */
> + if (private->state == VFIO_CCW_STATE_NOT_OPER)
> + return -EINVAL;
> +
> private->nb.notifier_call = vfio_ccw_mdev_notifier;
>
> ret = vfio_register_notifier(vdev, VFIO_IOMMU_NOTIFY,
> @@ -184,6 +180,12 @@ static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
> if (ret)
> goto out_unregister;
>
> + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
> + if (private->state == VFIO_CCW_STATE_NOT_OPER) {
> + ret = -EINVAL;
> + goto out_unregister;
> + }
> +
> return ret;
>
> out_unregister:
> @@ -197,13 +199,7 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
> struct vfio_ccw_private *private =
> container_of(vdev, struct vfio_ccw_private, vdev);
>
> - if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
> - (private->state != VFIO_CCW_STATE_STANDBY)) {
> - if (!vfio_ccw_mdev_reset(private))
> - private->state = VFIO_CCW_STATE_STANDBY;
> - /* The state will be NOT_OPER on error. */
> - }
> -
> + vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
> vfio_ccw_unregister_dev_regions(private);
> vfio_unregister_notifier(vdev, VFIO_IOMMU_NOTIFY, &private->nb);
> }