* [PATCH v1 1/2] vfio-ccw: Set subchannel state STANDBY on open
2019-05-06 13:11 [PATCH v1 0/2] New state handling for VFIO CCW Pierre Morel
@ 2019-05-06 13:11 ` Pierre Morel
2019-05-07 19:44 ` Farhan Ali
2019-05-06 13:11 ` [PATCH v1 2/2] vfio-ccw: rework sch_event Pierre Morel
2019-05-08 9:53 ` [PATCH v1 0/2] New state handling for VFIO CCW Cornelia Huck
2 siblings, 1 reply; 9+ messages in thread
From: Pierre Morel @ 2019-05-06 13:11 UTC (permalink / raw)
To: cohuck; +Cc: pasic, farman, alifm, linux-s390, kvm
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.
Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
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_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index ee8767f..a95b6c7 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -143,26 +143,18 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
dev_set_drvdata(&sch->dev, private);
mutex_init(&private->io_mutex);
- spin_lock_irq(sch->lock);
private->state = VFIO_CCW_STATE_NOT_OPER;
sch->isc = VFIO_CCW_ISC;
- ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
- spin_unlock_irq(sch->lock);
- if (ret)
- goto out_free;
INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
atomic_set(&private->avail, 1);
- private->state = VFIO_CCW_STATE_STANDBY;
ret = vfio_ccw_mdev_reg(sch);
if (ret)
- goto out_disable;
+ goto out_free;
return 0;
-out_disable:
- cio_disable_subchannel(sch);
out_free:
dev_set_drvdata(&sch->dev, NULL);
if (private->cmd_region)
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;
+
+ spin_lock_irq(private->sch->lock);
+ if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
+ goto error;
+
+ private->state = VFIO_CCW_STATE_STANDBY;
+ 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);
private->num_regions = 0;
kfree(private->region);
private->region = NULL;
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] vfio-ccw: Set subchannel state STANDBY on open
2019-05-06 13:11 ` [PATCH v1 1/2] vfio-ccw: Set subchannel state STANDBY on open Pierre Morel
@ 2019-05-07 19:44 ` Farhan Ali
2019-05-08 9:52 ` Cornelia Huck
0 siblings, 1 reply; 9+ messages in thread
From: Farhan Ali @ 2019-05-07 19:44 UTC (permalink / raw)
To: Pierre Morel, cohuck; +Cc: pasic, farman, linux-s390, kvm
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@linux.ibm.com>
> ---
> 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_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index ee8767f..a95b6c7 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -143,26 +143,18 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
> dev_set_drvdata(&sch->dev, private);
> mutex_init(&private->io_mutex);
>
> - spin_lock_irq(sch->lock);
> private->state = VFIO_CCW_STATE_NOT_OPER;
> sch->isc = VFIO_CCW_ISC;
> - ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
> - spin_unlock_irq(sch->lock);
> - if (ret)
> - goto out_free;
>
> INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
> atomic_set(&private->avail, 1);
> - private->state = VFIO_CCW_STATE_STANDBY;
>
> ret = vfio_ccw_mdev_reg(sch);
> if (ret)
> - goto out_disable;
> + goto out_free;
>
> return 0;
>
> -out_disable:
> - cio_disable_subchannel(sch);
> out_free:
> dev_set_drvdata(&sch->dev, NULL);
> if (private->cmd_region)
> 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;
> +
> + 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.
> + 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);
> private->num_regions = 0;
> kfree(private->region);
> private->region = NULL;
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] vfio-ccw: Set subchannel state STANDBY on open
2019-05-07 19:44 ` Farhan Ali
@ 2019-05-08 9:52 ` Cornelia Huck
2019-05-16 15:11 ` Pierre Morel
0 siblings, 1 reply; 9+ messages in thread
From: Cornelia Huck @ 2019-05-08 9:52 UTC (permalink / raw)
To: Farhan Ali; +Cc: Pierre Morel, pasic, farman, linux-s390, kvm
On Tue, 7 May 2019 15:44:54 -0400
Farhan Ali <alifm@linux.ibm.com> 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@linux.ibm.com>
> > ---
> > 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;
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] vfio-ccw: Set subchannel state STANDBY on open
2019-05-08 9:52 ` Cornelia Huck
@ 2019-05-16 15:11 ` Pierre Morel
0 siblings, 0 replies; 9+ messages in thread
From: Pierre Morel @ 2019-05-16 15:11 UTC (permalink / raw)
To: Cornelia Huck, Farhan Ali; +Cc: pasic, farman, linux-s390, kvm
On 08/05/2019 11:52, Cornelia Huck wrote:
> On Tue, 7 May 2019 15:44:54 -0400
> Farhan Ali <alifm@linux.ibm.com> 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@linux.ibm.com>
>>> ---
>>> 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?
Yes there should.
I will let it where it was.
>
>>> private->num_regions = 0;
>>> kfree(private->region);
>>> private->region = NULL;
>>>
>>
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 2/2] vfio-ccw: rework sch_event
2019-05-06 13:11 [PATCH v1 0/2] New state handling for VFIO CCW Pierre Morel
2019-05-06 13:11 ` [PATCH v1 1/2] vfio-ccw: Set subchannel state STANDBY on open Pierre Morel
@ 2019-05-06 13:11 ` Pierre Morel
2019-05-07 20:06 ` Farhan Ali
2019-05-08 9:53 ` [PATCH v1 0/2] New state handling for VFIO CCW Cornelia Huck
2 siblings, 1 reply; 9+ messages in thread
From: Pierre Morel @ 2019-05-06 13:11 UTC (permalink / raw)
To: cohuck; +Cc: pasic, farman, alifm, linux-s390, kvm
Set the mediated device as non operational when the
subchannel went non operational.
Otherwise keep the current state.
Since we removed the last use of VFIO_CCW_STATE_STANDBY
remove this state from the state machine.
Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
drivers/s390/cio/vfio_ccw_drv.c | 11 +----------
drivers/s390/cio/vfio_ccw_fsm.c | 7 +------
drivers/s390/cio/vfio_ccw_ops.c | 2 +-
drivers/s390/cio/vfio_ccw_private.h | 1 -
4 files changed, 3 insertions(+), 18 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index a95b6c7..2f6140d5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -210,17 +210,8 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
if (work_pending(&sch->todo_work))
goto out_unlock;
- if (cio_update_schib(sch)) {
+ if (cio_update_schib(sch))
vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
- rc = 0;
- goto out_unlock;
- }
-
- private = dev_get_drvdata(&sch->dev);
- if (private->state == VFIO_CCW_STATE_NOT_OPER) {
- private->state = private->mdev ? VFIO_CCW_STATE_IDLE :
- VFIO_CCW_STATE_STANDBY;
- }
rc = 0;
out_unlock:
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 49d9d3d..a6524ca 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -88,6 +88,7 @@ static int fsm_do_halt(struct vfio_ccw_private *private)
/* Issue "Halt Subchannel" */
ccode = hsch(sch->schid);
+ pr_warn("ccode = hsch(sch->schid);\n");
switch (ccode) {
case 0:
@@ -326,12 +327,6 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
[VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_error,
[VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq,
},
- [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_STATE_IDLE] = {
[VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
[VFIO_CCW_EVENT_IO_REQ] = fsm_io_request,
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 497419c..35445ca 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -162,7 +162,7 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
goto error;
- private->state = VFIO_CCW_STATE_STANDBY;
+ private->state = VFIO_CCW_STATE_IDLE;
spin_unlock_irq(sch->lock);
return 0;
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index f1092c3..ece6a75 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -105,7 +105,6 @@ extern int vfio_ccw_sch_quiesce(struct subchannel *sch);
*/
enum vfio_ccw_state {
VFIO_CCW_STATE_NOT_OPER,
- VFIO_CCW_STATE_STANDBY,
VFIO_CCW_STATE_IDLE,
VFIO_CCW_STATE_CP_PROCESSING,
VFIO_CCW_STATE_CP_PENDING,
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] vfio-ccw: rework sch_event
2019-05-06 13:11 ` [PATCH v1 2/2] vfio-ccw: rework sch_event Pierre Morel
@ 2019-05-07 20:06 ` Farhan Ali
0 siblings, 0 replies; 9+ messages in thread
From: Farhan Ali @ 2019-05-07 20:06 UTC (permalink / raw)
To: Pierre Morel, cohuck; +Cc: pasic, farman, linux-s390, kvm
On 05/06/2019 09:11 AM, Pierre Morel wrote:
> Set the mediated device as non operational when the
> subchannel went non operational.
> Otherwise keep the current state.
>
> Since we removed the last use of VFIO_CCW_STATE_STANDBY
> remove this state from the state machine.
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
> drivers/s390/cio/vfio_ccw_drv.c | 11 +----------
> drivers/s390/cio/vfio_ccw_fsm.c | 7 +------
> drivers/s390/cio/vfio_ccw_ops.c | 2 +-
> drivers/s390/cio/vfio_ccw_private.h | 1 -
> 4 files changed, 3 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index a95b6c7..2f6140d5 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -210,17 +210,8 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
> if (work_pending(&sch->todo_work))
> goto out_unlock;
>
> - if (cio_update_schib(sch)) {
> + if (cio_update_schib(sch))
> vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> - rc = 0;
> - goto out_unlock;
> - }
> -
> - private = dev_get_drvdata(&sch->dev);
> - if (private->state == VFIO_CCW_STATE_NOT_OPER) {
> - private->state = private->mdev ? VFIO_CCW_STATE_IDLE :
> - VFIO_CCW_STATE_STANDBY;
> - }
> rc = 0;
>
> out_unlock:
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 49d9d3d..a6524ca 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -88,6 +88,7 @@ static int fsm_do_halt(struct vfio_ccw_private *private)
>
> /* Issue "Halt Subchannel" */
> ccode = hsch(sch->schid);
> + pr_warn("ccode = hsch(sch->schid);\n");
I am not sure what's the purpose of logging this? Was it for your
debugging purpose?
>
> switch (ccode) {
> case 0:
> @@ -326,12 +327,6 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
> [VFIO_CCW_EVENT_ASYNC_REQ] = fsm_async_error,
> [VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq,
> },
> - [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_STATE_IDLE] = {
> [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> [VFIO_CCW_EVENT_IO_REQ] = fsm_io_request,
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 497419c..35445ca 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -162,7 +162,7 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
> if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
> goto error;
>
> - private->state = VFIO_CCW_STATE_STANDBY;
> + private->state = VFIO_CCW_STATE_IDLE;
> spin_unlock_irq(sch->lock);
> return 0;
>
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index f1092c3..ece6a75 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -105,7 +105,6 @@ extern int vfio_ccw_sch_quiesce(struct subchannel *sch);
> */
> enum vfio_ccw_state {
> VFIO_CCW_STATE_NOT_OPER,
> - VFIO_CCW_STATE_STANDBY,
> VFIO_CCW_STATE_IDLE,
> VFIO_CCW_STATE_CP_PROCESSING,
> VFIO_CCW_STATE_CP_PENDING,
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 0/2] New state handling for VFIO CCW
2019-05-06 13:11 [PATCH v1 0/2] New state handling for VFIO CCW Pierre Morel
2019-05-06 13:11 ` [PATCH v1 1/2] vfio-ccw: Set subchannel state STANDBY on open Pierre Morel
2019-05-06 13:11 ` [PATCH v1 2/2] vfio-ccw: rework sch_event Pierre Morel
@ 2019-05-08 9:53 ` Cornelia Huck
2019-05-16 8:59 ` Pierre Morel
2 siblings, 1 reply; 9+ messages in thread
From: Cornelia Huck @ 2019-05-08 9:53 UTC (permalink / raw)
To: Pierre Morel; +Cc: pasic, farman, alifm, linux-s390, kvm
On Mon, 6 May 2019 15:11:08 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:
> Hi,
>
> I did not integrate all my patches for state handling like I had
> before but just two patches which seems interresting to me:
>
> - The first one allows the device ti be used only when a guest
> is currently using it.
> Otherwise the device is in NOT_OPER state
>
> - The second rework the sch_event callback: AFAIU we can not
> consider that the event moves the device in IDLE state.
> I think we better let it as it is currently.
I agree with the direction of this patch series.
>
> Regards,
> Pierre
>
> Pierre Morel (2):
> vfio-ccw: Set subchannel state STANDBY on open
> vfio-ccw: rework sch_event
>
> drivers/s390/cio/vfio_ccw_drv.c | 21 ++-------------------
> drivers/s390/cio/vfio_ccw_fsm.c | 7 +------
> drivers/s390/cio/vfio_ccw_ops.c | 36 ++++++++++++++++++------------------
> drivers/s390/cio/vfio_ccw_private.h | 1 -
> 4 files changed, 21 insertions(+), 44 deletions(-)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 0/2] New state handling for VFIO CCW
2019-05-08 9:53 ` [PATCH v1 0/2] New state handling for VFIO CCW Cornelia Huck
@ 2019-05-16 8:59 ` Pierre Morel
0 siblings, 0 replies; 9+ messages in thread
From: Pierre Morel @ 2019-05-16 8:59 UTC (permalink / raw)
To: Cornelia Huck; +Cc: pasic, farman, alifm, linux-s390, kvm
On 08/05/2019 11:53, Cornelia Huck wrote:
> On Mon, 6 May 2019 15:11:08 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
>
>> Hi,
>>
>> I did not integrate all my patches for state handling like I had
>> before but just two patches which seems interresting to me:
>>
>> - The first one allows the device ti be used only when a guest
>> is currently using it.
>> Otherwise the device is in NOT_OPER state
>>
>> - The second rework the sch_event callback: AFAIU we can not
>> consider that the event moves the device in IDLE state.
>> I think we better let it as it is currently.
>
> I agree with the direction of this patch series.
>
>>
>> Regards,
>> Pierre
>>
>> Pierre Morel (2):
>> vfio-ccw: Set subchannel state STANDBY on open
>> vfio-ccw: rework sch_event
>>
>> drivers/s390/cio/vfio_ccw_drv.c | 21 ++-------------------
>> drivers/s390/cio/vfio_ccw_fsm.c | 7 +------
>> drivers/s390/cio/vfio_ccw_ops.c | 36 ++++++++++++++++++------------------
>> drivers/s390/cio/vfio_ccw_private.h | 1 -
>> 4 files changed, 21 insertions(+), 44 deletions(-)
>>
>
Thanks,
I will post a v2 with the corrections seen by you and Farhan,
Regards,
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
^ permalink raw reply [flat|nested] 9+ messages in thread