On Tue, 7 May 2019 15:44:54 -0400
Farhan Ali <alifm@xxxxxxxxxxxxx> wrote:
> On 05/06/2019 09:11 AM, Pierre Morel wrote:
> > When no guest is associated with the mediated device,
> > i.e. the mediated device is not opened, the state of
> > the mediated device is VFIO_CCW_STATE_NOT_OPER.
> >
> > The subchannel enablement and the according setting to the
> > VFIO_CCW_STATE_STANDBY state should only be done when all
> > parts of the VFIO mediated device have been initialized
> > i.e. after the mediated device has been successfully opened.
> >
> > Let's stay in VFIO_CCW_STATE_NOT_OPER until the mediated
> > device has been opened.
> >
> > When the mediated device is closed, disable the sub channel
> > by calling vfio_ccw_sch_quiesce() no reset needs to be done
> > the mediated devce will be enable on next open.
>
> s/devce/device
>
> >
> > Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> > ---
> > drivers/s390/cio/vfio_ccw_drv.c | 10 +---------
> > drivers/s390/cio/vfio_ccw_ops.c | 36 ++++++++++++++++++------------------
> > 2 files changed, 19 insertions(+), 27 deletions(-)
> >
(...)
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> > index 5eb6111..497419c 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -115,14 +115,10 @@ static int vfio_ccw_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
> > struct vfio_ccw_private *private =
> > dev_get_drvdata(mdev_parent_dev(mdev));
> >
> > - if (private->state == VFIO_CCW_STATE_NOT_OPER)
> > - return -ENODEV;
> > -
> > if (atomic_dec_if_positive(&private->avail) < 0)
> > return -EPERM;
> >
> > private->mdev = mdev;
> > - private->state = VFIO_CCW_STATE_IDLE;
> >
> > return 0;
> > }
> > @@ -132,12 +128,7 @@ static int vfio_ccw_mdev_remove(struct mdev_device *mdev)
> > struct vfio_ccw_private *private =
> > dev_get_drvdata(mdev_parent_dev(mdev));
> >
> > - if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
> > - (private->state != VFIO_CCW_STATE_STANDBY)) {
> > - if (!vfio_ccw_sch_quiesce(private->sch))
> > - private->state = VFIO_CCW_STATE_STANDBY;
> > - /* The state will be NOT_OPER on error. */
> > - }
> > + vfio_ccw_sch_quiesce(private->sch);
> >
> > cp_free(&private->cp);
> > private->mdev = NULL;
> > @@ -151,6 +142,7 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
> > struct vfio_ccw_private *private =
> > dev_get_drvdata(mdev_parent_dev(mdev));
> > unsigned long events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
> > + struct subchannel *sch = private->sch;
> > int ret;
> >
> > private->nb.notifier_call = vfio_ccw_mdev_notifier;
> > @@ -165,6 +157,20 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
> > vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> > &private->nb);
> > return ret;
I think this "return ret;" needs to go into the if branch above it;
otherwise, the code below won't be reached :)
> > +
> > + spin_lock_irq(private->sch->lock);
> > + if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
> > + goto error;
> > +
> > + private->state = VFIO_CCW_STATE_STANDBY;
>
> I don't think we should set the state to STANDBY here, because with just
> this patch applied, any VFIO_CCW_EVENT_IO_REQ will return an error (due
> to fsm_io_error).
>
> It might be safe to set it to IDLE in this patch.
Agreed, this should be IDLE; otherwise, I don't see how a device might
move into IDLE state?
(That change happens in the next patch anyway.)
>
>
> > + spin_unlock_irq(sch->lock);
> > + return 0;
> > +
> > +error:
> > + spin_unlock_irq(sch->lock);
> > + vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> > + &private->nb);
> > + return -EFAULT;
> > }
> >
> > static void vfio_ccw_mdev_release(struct mdev_device *mdev)
> > @@ -173,20 +179,14 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
> > dev_get_drvdata(mdev_parent_dev(mdev));
> > int i;
> >
> > - if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
> > - (private->state != VFIO_CCW_STATE_STANDBY)) {
> > - if (!vfio_ccw_mdev_reset(mdev))
> > - private->state = VFIO_CCW_STATE_STANDBY;
> > - /* The state will be NOT_OPER on error. */
> > - }
> > -
> > - cp_free(&private->cp);
> > + vfio_ccw_sch_quiesce(private->sch);
> > vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> > &private->nb);
> >
> > for (i = 0; i < private->num_regions; i++)
> > private->region[i].ops->release(private, &private->region[i]);
> >
> > + cp_free(&private->cp);
I'm wondering why this cp_free is moved -- there should not be any
activity related to it after quiesce, should there?
> > private->num_regions = 0;
> > kfree(private->region);
> > private->region = NULL;
> >
>
