[PATCH v3 0/6] vfio: ccw: VFIO CCW cleanup part1

[PATCH v3 0/6] vfio: ccw: VFIO CCW cleanup part1

[Pierre Morel]

The goal of the patches of this serie is to clarify the code
of state/event handling.

- First patch (already acked/applied) makes sure that every structures are initialized
  before the mediated device is registered.
  (Only here to apply the serie on the main tree).

- vfio: ccw: Rework subchannel state on setup
  makes sure that the device can not be used before a guest is
  ready to drive it.

- vfio: ccw: Rework subchannel state on removing
  could be squash with the previous: same kind of rewriting
  but for the removing/release callbacks.

- vfio: ccw: Rework subchannel state on sch_event
  sch_event handling seems quite poor to me.
  Anyway, I do not understand why we hould have state change there.

- vfio: ccw: Documenting state transitions

- vfio: ccw: serialize the write system calls
  Quite independent of the previous patches, this makes sure
  that the entry in the driver is serialized.

Pierre Morel (6):
  vfio: ccw: Register mediated device once all structures are
    initialized
  vfio: ccw: Rework subchannel state on setup
  vfio: ccw: Rework subchannel state on removing
  vfio: ccw: Rework subchannel state on sch_event
  vfio: ccw: Documenting state transitions
  vfio: ccw: serialize the write system calls

 Documentation/s390/vfio-ccw.txt     | 45 +++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_async.c   | 11 +++++++
 drivers/s390/cio/vfio_ccw_drv.c     | 30 ++++-------------
 drivers/s390/cio/vfio_ccw_ops.c     | 65 ++++++++++++++++++++++++-------------
 drivers/s390/cio/vfio_ccw_private.h |  1 +
 5 files changed, 105 insertions(+), 47 deletions(-)

-- 
2.7.4

Changelog:

from v2:
- adaptation after Conny's hlt/clr patches
- integration of Conny's comments
- reworking of the state changes to make it clearer
- added documentation
- added serialization

from v1:
- change commit message
- add reviewed-by from Eric (Really this time)

from v0:
- isolate these two patches from the previous serie
- Added Reviewed-by Eric
- Ortographic correction

[PATCH v3 1/6] vfio: ccw: Register mediated device once all structures are initialized

[Pierre Morel]

There is a risk that the mediated device is used before all the
data are initialized if it is registered too early.

Let's register the mediated device when all the data structures
which could be used are initialized.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 2f2e5d1..bce7fed 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -141,14 +141,14 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	if (ret)
 		goto out_free;
 
-	ret = vfio_ccw_mdev_reg(sch);
-	if (ret)
-		goto out_disable;
-
 	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;
+
 	return 0;
 
 out_disable:
-- 
2.7.4

[PATCH v3 2/6] vfio: ccw: Rework subchannel state on setup

[Pierre Morel]

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 and set the VFIO_CCW_STATE_STANDBY
on a successful open.

On release the state is set back to VFIO_CCW_STATE_NOT_OPER
by vfio_ccw_sch_quiesce().

When the mediated device is closed,  disable the sub channel
by calling vfio_ccw_sch_quiesce().

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_async.c   | 11 +++++++++++
 drivers/s390/cio/vfio_ccw_drv.c     | 10 +---------
 drivers/s390/cio/vfio_ccw_ops.c     | 27 +++++++++++++++++++++------
 drivers/s390/cio/vfio_ccw_private.h |  1 +
 4 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
index 8c7f51d..033574e 100644
--- a/drivers/s390/cio/vfio_ccw_async.c
+++ b/drivers/s390/cio/vfio_ccw_async.c
@@ -76,6 +76,17 @@ const struct vfio_ccw_regops vfio_ccw_async_region_ops = {
 	.release = vfio_ccw_async_region_release,
 };
 
+void vfio_ccw_unregister_async_dev_regions(struct vfio_ccw_private *private)
+{
+	int i;
+
+	for (i = 0; i < private->num_regions; i++)
+		private->region[i].ops->release(private, &private->region[i]);
+	private->num_regions = 0;
+	kfree(private->region);
+	private->region = NULL;
+}
+
 int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private)
 {
 	return vfio_ccw_register_dev_region(private,
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index bce7fed..d3527b6 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -133,26 +133,18 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	private->sch = sch;
 	dev_set_drvdata(&sch->dev, private);
 
-	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 0e1f7f7b..cc7d46a 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -113,14 +113,11 @@ 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;
+	private->state = VFIO_CCW_STATE_NOT_OPER;
 
 	return 0;
 }
@@ -148,6 +145,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;
@@ -159,8 +157,24 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
 
 	ret = vfio_ccw_register_async_dev_regions(private);
 	if (ret)
-		vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
-					 &private->nb);
+		goto err_regions;
+
+	spin_lock_irq(private->sch->lock);
+	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
+	if (ret)
+		goto err_enable;
+
+	private->state = VFIO_CCW_STATE_STANDBY;
+	spin_unlock_irq(sch->lock);
+	return 0;
+
+err_enable:
+	spin_unlock_irq(sch->lock);
+	vfio_ccw_unregister_async_dev_regions(private);
+
+err_regions:
+	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+				 &private->nb);
 	return ret;
 }
 
@@ -170,6 +184,7 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
 		dev_get_drvdata(mdev_parent_dev(mdev));
 	int i;
 
+	vfio_ccw_sch_quiesce(private->sch);
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
 				 &private->nb);
 
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 1a41a14..5d799b7 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -54,6 +54,7 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
 				 size_t size, u32 flags, void *data);
 
 int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
+void vfio_ccw_unregister_async_dev_regions(struct vfio_ccw_private *private);
 
 /**
  * struct vfio_ccw_private
-- 
2.7.4

Re: [PATCH v3 2/6] vfio: ccw: Rework subchannel state on setup

[Eric Farman]

My questions to this patch from the original RFC series are still 
outstanding.  :(

https://marc.info/?l=linux-s390&m=154223063716128&w=2

On 11/28/2018 07:41 AM, Pierre Morel wrote:
> 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 and set the VFIO_CCW_STATE_STANDBY
> on a successful open.
> 
> On release the state is set back to VFIO_CCW_STATE_NOT_OPER
> by vfio_ccw_sch_quiesce().
> 
> When the mediated device is closed,  disable the sub channel
> by calling vfio_ccw_sch_quiesce().
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_async.c   | 11 +++++++++++

Ah, this series is built on Connie's async changes.  Okay.  [1]

>   drivers/s390/cio/vfio_ccw_drv.c     | 10 +---------
>   drivers/s390/cio/vfio_ccw_ops.c     | 27 +++++++++++++++++++++------
>   drivers/s390/cio/vfio_ccw_private.h |  1 +
>   4 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
> index 8c7f51d..033574e 100644
> --- a/drivers/s390/cio/vfio_ccw_async.c
> +++ b/drivers/s390/cio/vfio_ccw_async.c
> @@ -76,6 +76,17 @@ const struct vfio_ccw_regops vfio_ccw_async_region_ops = {
>   	.release = vfio_ccw_async_region_release,
>   };
>   
> +void vfio_ccw_unregister_async_dev_regions(struct vfio_ccw_private *private)
> +{
> +	int i;
> +
> +	for (i = 0; i < private->num_regions; i++)
> +		private->region[i].ops->release(private, &private->region[i]);
> +	private->num_regions = 0;
> +	kfree(private->region);
> +	private->region = NULL;
> +} > +
>   int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private)
>   {
>   	return vfio_ccw_register_dev_region(private,
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index bce7fed..d3527b6 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -133,26 +133,18 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
>   	private->sch = sch;
>   	dev_set_drvdata(&sch->dev, private);
>   
> -	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 0e1f7f7b..cc7d46a 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -113,14 +113,11 @@ 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;
> +	private->state = VFIO_CCW_STATE_NOT_OPER;
>   
>   	return 0;
>   }
> @@ -148,6 +145,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;
> @@ -159,8 +157,24 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
>   
>   	ret = vfio_ccw_register_async_dev_regions(private);
>   	if (ret)
> -		vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> -					 &private->nb);
> +		goto err_regions;
> +
> +	spin_lock_irq(private->sch->lock);
> +	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
> +	if (ret)
> +		goto err_enable;
> +
> +	private->state = VFIO_CCW_STATE_STANDBY;
> +	spin_unlock_irq(sch->lock);
> +	return 0;
> +
> +err_enable:
> +	spin_unlock_irq(sch->lock);
> +	vfio_ccw_unregister_async_dev_regions(private);
> +
> +err_regions:
> +	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> +				 &private->nb);
>   	return ret;
>   }
>   
> @@ -170,6 +184,7 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
>   		dev_get_drvdata(mdev_parent_dev(mdev));
>   	int i;
>   
> +	vfio_ccw_sch_quiesce(private->sch);
>   	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>   				 &private->nb);

[1] If Connie's async patches go in first, then the stuff in your 
"vfio_ccw_unregister_async_dev_regions" is also added here.  That could 
be removed and replaced with a call to your new function, yes?

>   
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index 1a41a14..5d799b7 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -54,6 +54,7 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
>   				 size_t size, u32 flags, void *data);
>   
>   int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
> +void vfio_ccw_unregister_async_dev_regions(struct vfio_ccw_private *private);
>   
>   /**
>    * struct vfio_ccw_private
> 

Re: [PATCH v3 2/6] vfio: ccw: Rework subchannel state on setup

[Pierre Morel]

On 18/12/2018 18:44, Eric Farman wrote:
> My questions to this patch from the original RFC series are still 
> outstanding.  :(
> 
> https://marc.info/?l=linux-s390&m=154223063716128&w=2


Hi Eric,

Thanks for the following of this patch series.

For your question about quiece during remove I do not think it should be 
a NOP, we must make sure the channel is disabled at that time.

> 
> On 11/28/2018 07:41 AM, Pierre Morel wrote:
>> 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 and set the VFIO_CCW_STATE_STANDBY
>> on a successful open.
>>
>> On release the state is set back to VFIO_CCW_STATE_NOT_OPER
>> by vfio_ccw_sch_quiesce().
>>
>> When the mediated device is closed,  disable the sub channel
>> by calling vfio_ccw_sch_quiesce().
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_async.c   | 11 +++++++++++
> 
> Ah, this series is built on Connie's async changes.  Okay.  [1]

Yes, and after reflections I think the timing is bad so I prefer to wait 
for the series from Connie on hsch/csch to be finished before going on 
with this series.
Otherwise I fear to only add noise to the current discussions.


> 
>>   drivers/s390/cio/vfio_ccw_drv.c     | 10 +---------
>>   drivers/s390/cio/vfio_ccw_ops.c     | 27 +++++++++++++++++++++------
...snip...
>> @@ -170,6 +184,7 @@ static void vfio_ccw_mdev_release(struct 
>> mdev_device *mdev)
>>           dev_get_drvdata(mdev_parent_dev(mdev));
>>       int i;
>> +    vfio_ccw_sch_quiesce(private->sch);
>>       vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>>                    &private->nb);
> 
> [1] If Connie's async patches go in first, then the stuff in your 
> "vfio_ccw_unregister_async_dev_regions" is also added here.  That could 
> be removed and replaced with a call to your new function, yes?

certainly.
Thanks for your comments.

Regards,
Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

[PATCH v3 3/6] vfio: ccw: Rework subchannel state on removing

[Pierre Morel]

This patch can be squash with the previous and is separate
to ease review.

We continue to clarify the state changes.

NOT_OPER is set on unrecoverable error
bind (probe) set the state to STANDBY
open set the state to idle
release set the state back to STANDBY

All functions called during the normal state of the device
do not change the state but on errors.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c |  1 -
 drivers/s390/cio/vfio_ccw_ops.c | 25 ++++++++++---------------
 2 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index d3527b6..1779b46 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -66,7 +66,6 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 		ret = cio_disable_subchannel(sch);
 	} while (ret == -EBUSY);
 out_unlock:
-	private->state = VFIO_CCW_STATE_NOT_OPER;
 	spin_unlock_irq(sch->lock);
 	return ret;
 }
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index cc7d46a..eb5b49d 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -35,11 +35,7 @@ static int vfio_ccw_mdev_reset(struct mdev_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
-	if (!ret)
-		private->state = VFIO_CCW_STATE_IDLE;
-
-	return ret;
+	return cio_enable_subchannel(sch, (u32)(unsigned long)sch);
 }
 
 static int vfio_ccw_mdev_notifier(struct notifier_block *nb,
@@ -117,7 +113,7 @@ static int vfio_ccw_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 		return -EPERM;
 
 	private->mdev = mdev;
-	private->state = VFIO_CCW_STATE_NOT_OPER;
+	private->state = VFIO_CCW_STATE_STANDBY;
 
 	return 0;
 }
@@ -128,11 +124,11 @@ static int vfio_ccw_mdev_remove(struct mdev_device *mdev)
 		dev_get_drvdata(mdev_parent_dev(mdev));
 
 	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. */
-	}
+	    (private->state != VFIO_CCW_STATE_STANDBY))
+		return -EBUSY;
+
+	vfio_ccw_mdev_reset(mdev);
+	private->state = VFIO_CCW_STATE_NOT_OPER;
 
 	private->mdev = NULL;
 	atomic_inc(&private->avail);
@@ -164,7 +160,7 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
 	if (ret)
 		goto err_enable;
 
-	private->state = VFIO_CCW_STATE_STANDBY;
+	private->state = VFIO_CCW_STATE_IDLE;
 	spin_unlock_irq(sch->lock);
 	return 0;
 
@@ -184,6 +180,7 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
 		dev_get_drvdata(mdev_parent_dev(mdev));
 	int i;
 
+	private->state = VFIO_CCW_STATE_STANDBY;
 	vfio_ccw_sch_quiesce(private->sch);
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
 				 &private->nb);
@@ -256,10 +253,8 @@ static ssize_t vfio_ccw_mdev_write_io_region(struct vfio_ccw_private *private,
 		return -EFAULT;
 
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ);
-	if (region->ret_code != 0) {
-		private->state = VFIO_CCW_STATE_IDLE;
+	if (region->ret_code != 0)
 		return region->ret_code;
-	}
 
 	return count;
 
-- 
2.7.4

[PATCH v3 4/6] vfio: ccw: Rework subchannel state on sch_event

[Pierre Morel]

We do not need to change the state when handling events.

If we can not update the SCHIB it is an unrecoverable error
independent of the real sub channel.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 1779b46..687ca42 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -198,18 +198,9 @@ 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)) {
-		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;
+	if (cio_update_schib(sch) && private)
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
 
 out_unlock:
 	spin_unlock_irqrestore(sch->lock, flags);
-- 
2.7.4

[PATCH v3 5/6] vfio: ccw: Documenting state transitions

[Pierre Morel]

Let's document the state transitions.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 Documentation/s390/vfio-ccw.txt | 45 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/Documentation/s390/vfio-ccw.txt b/Documentation/s390/vfio-ccw.txt
index 2be11ad..0a1a19f 100644
--- a/Documentation/s390/vfio-ccw.txt
+++ b/Documentation/s390/vfio-ccw.txt
@@ -263,6 +263,51 @@ Q6. Get the signal and event handler reads out the result from the I/O
     region.
 Q7. Update the irb for the guest.
 
+
+States transitions
+------------------
+
+The CCW VFIO driver maintain a device states to determine the response
+- to external events like interruptions, data send, halt and clear
+  from sub-channels.
+- to configuration events, device binding, mdev creation, VM start/stop.
+- to other events like errors or reset of the VM
+
+The event and states are summed up in the diagramme below:
+
+      bind() unbind()
+        |       ^
+        v       |
+      -------------
+      | NOT_OPER  |<---- error
+      -------------    |
+        |       ^      |
+     create() remove() |
+        v       |      |
+      -------------    |
+      | STANDBY   |<--- release()
+      -------------    |
+        |       ^      |
+      open() release() |
+        v       |      |
+      -------------    |
+   -->|   IDLE    |<--- reset
+   |  -------------    |
+   |    |       ^      |
+   |  write()  err     |
+   |    v       |      |
+   |  -------------    |
+   |  |   BOXED   |    |
+   |  -------------    |
+   |       |           |
+  irq   success        |
+   |       v           |
+   |  -------------    |
+   ---|   BUSY    |>----
+      -------------
+
+
+
 Limitations
 -----------
 
-- 
2.7.4

[PATCH v3 6/6] vfio: ccw: serialize the write system calls

[Pierre Morel]

When the user program is QEMU we rely on the QEMU lock to serialize
the calls to the driver.

In the general case we need to make sure that two data transfer are
not started at the same time.
It would in the current implementation resul in a overwriting of the
IO region.

We also need to make sure a clear or a halt started after a data
transfer do not win the race agains the data transfer.
Which would result in the data transfer being started after the
halt/clear.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index eb5b49d..b316966 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -267,22 +267,31 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
 {
 	unsigned int index = VFIO_CCW_OFFSET_TO_INDEX(*ppos);
 	struct vfio_ccw_private *private;
+	static atomic_t serialize  = ATOMIC_INIT(0);
+	int ret = -EINVAL;
+
+	if (!atomic_add_unless(&serialize, 1, 1))
+		return -EBUSY;
 
 	private = dev_get_drvdata(mdev_parent_dev(mdev));
 
 	if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions)
-		return -EINVAL;
+		goto end;
 
 	switch (index) {
 	case VFIO_CCW_CONFIG_REGION_INDEX:
-		return vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
+		ret = vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
+		break;
 	default:
 		index -= VFIO_CCW_NUM_REGIONS;
-		return private->region[index].ops->write(private, buf, count,
+		ret = private->region[index].ops->write(private, buf, count,
 							 ppos);
+		break;
 	}
 
-	return -EINVAL;
+end:
+	atomic_dec(&serialize);
+	return ret;
 }
 
 static int vfio_ccw_mdev_get_device_info(struct vfio_device_info *info,
-- 
2.7.4

Re: [PATCH v3 6/6] vfio: ccw: serialize the write system calls

[Cornelia Huck]

On Wed, 28 Nov 2018 13:41:07 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> When the user program is QEMU we rely on the QEMU lock to serialize
> the calls to the driver.
> 
> In the general case we need to make sure that two data transfer are
> not started at the same time.
> It would in the current implementation resul in a overwriting of the
> IO region.
> 
> We also need to make sure a clear or a halt started after a data
> transfer do not win the race agains the data transfer.
> Which would result in the data transfer being started after the
> halt/clear.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_ops.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index eb5b49d..b316966 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -267,22 +267,31 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
>  {
>  	unsigned int index = VFIO_CCW_OFFSET_TO_INDEX(*ppos);
>  	struct vfio_ccw_private *private;
> +	static atomic_t serialize  = ATOMIC_INIT(0);
> +	int ret = -EINVAL;
> +
> +	if (!atomic_add_unless(&serialize, 1, 1))
> +		return -EBUSY;

I think that hammer is far too big: This serializes _all_ write
operations across _all_ devices.

There are various cases of simultaneous writes that may happen
(assuming any userspace; QEMU locking will prevent some of them from
happening):

- One thread does a write for one mdev, another thread does a write for
  another mdev. For example, if two vcpus issue an I/O instruction on
  two different devices. This should be fine.
- One thread does a write for one mdev, another thread does a write for
  the same mdev. Maybe a guest has confused/no locking and is trying to
  do ssch on the same device from different vcpus. There, we want to
  exclude simultaneous writes; the desired outcome may be that one ssch
  gets forwarded to the hardware, and the second one either gets
  forwarded after processing for the first one has finished, or
  userspace gets an error immediately (hopefully resulting in a
  appropriate condition code for that second ssch in any case). Both
  handing the second ssch to the hardware or signaling device busy
  immediately are probably sane in that case.
- If those writes for the same device involve hsch/csch, things get
  more complicated. First, we have two different regions, and allowing
  simultaneous writes to the I/O region and to the async region should
  not really be a problem, so I don't think fencing should be done in
  the generic write handler. Second, the semantics for device busy are
  different: a hsch immediately after a ssch should not give device
  busy, and csch cannot return device busy at all.

I don't think we'll be able to get around some kind of "let's retry
sending this" logic for hsch/csch; maybe we should already do that for
ssch. (Like the -EINVAL logic I described in the other thread.)


>  
>  	private = dev_get_drvdata(mdev_parent_dev(mdev));
>  
>  	if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions)
> -		return -EINVAL;
> +		goto end;
>  
>  	switch (index) {
>  	case VFIO_CCW_CONFIG_REGION_INDEX:
> -		return vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
> +		ret = vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
> +		break;
>  	default:
>  		index -= VFIO_CCW_NUM_REGIONS;
> -		return private->region[index].ops->write(private, buf, count,
> +		ret = private->region[index].ops->write(private, buf, count,
>  							 ppos);
> +		break;
>  	}
>  
> -	return -EINVAL;
> +end:
> +	atomic_dec(&serialize);
> +	return ret;
>  }
>  
>  static int vfio_ccw_mdev_get_device_info(struct vfio_device_info *info,

Re: [PATCH v3 6/6] vfio: ccw: serialize the write system calls

[Halil Pasic]

On Thu, 13 Dec 2018 16:39:53 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 28 Nov 2018 13:41:07 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> > When the user program is QEMU we rely on the QEMU lock to serialize
> > the calls to the driver.
> > 
> > In the general case we need to make sure that two data transfer are
> > not started at the same time.
> > It would in the current implementation resul in a overwriting of the
> > IO region.
> > 
> > We also need to make sure a clear or a halt started after a data
> > transfer do not win the race agains the data transfer.
> > Which would result in the data transfer being started after the
> > halt/clear.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >  drivers/s390/cio/vfio_ccw_ops.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> > index eb5b49d..b316966 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -267,22 +267,31 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
> >  {
> >  	unsigned int index = VFIO_CCW_OFFSET_TO_INDEX(*ppos);
> >  	struct vfio_ccw_private *private;
> > +	static atomic_t serialize  = ATOMIC_INIT(0);
> > +	int ret = -EINVAL;
> > +
> > +	if (!atomic_add_unless(&serialize, 1, 1))
> > +		return -EBUSY;
> 
> I think that hammer is far too big: This serializes _all_ write
> operations across _all_ devices.
> 
> There are various cases of simultaneous writes that may happen
> (assuming any userspace; QEMU locking will prevent some of them from
> happening):
> 
> - One thread does a write for one mdev, another thread does a write for
>   another mdev. For example, if two vcpus issue an I/O instruction on
>   two different devices. This should be fine.
> - One thread does a write for one mdev, another thread does a write for
>   the same mdev. Maybe a guest has confused/no locking and is trying to
>   do ssch on the same device from different vcpus. There, we want to
>   exclude simultaneous writes; the desired outcome may be that one ssch
>   gets forwarded to the hardware, and the second one either gets
>   forwarded after processing for the first one has finished, or
>   userspace gets an error immediately (hopefully resulting in a
>   appropriate condition code for that second ssch in any case). Both
>   handing the second ssch to the hardware or signaling device busy
>   immediately are probably sane in that case.
> - If those writes for the same device involve hsch/csch, things get
>   more complicated. First, we have two different regions, and allowing
>   simultaneous writes to the I/O region and to the async region should
>   not really be a problem, so I don't think fencing should be done in
>   the generic write handler. Second, the semantics for device busy are
>   different: a hsch immediately after a ssch should not give device
>   busy, and csch cannot return device busy at all.
> 
> I don't think we'll be able to get around some kind of "let's retry
> sending this" logic for hsch/csch; maybe we should already do that for
> ssch. (Like the -EINVAL logic I described in the other thread.)
> 
> 

Nice write-up! I agree with the conclusion, and also with the most of
the analysis. IMHO, to sort this out properly, we really have to think
end-to-end (i.e. guest, userspace, vfio-ccw, backing-device). Striving
towards an comprehensively documented the user-space facing vfio-ccw
interface could help as well.

I hope we can figure out a good solution in the context of hsch/csch.

Regards,
Halil

Re: [PATCH v3 6/6] vfio: ccw: serialize the write system calls

[Pierre Morel]

On 13/12/2018 16:39, Cornelia Huck wrote:
> On Wed, 28 Nov 2018 13:41:07 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> When the user program is QEMU we rely on the QEMU lock to serialize
>> the calls to the driver.
>>
>> In the general case we need to make sure that two data transfer are
>> not started at the same time.
>> It would in the current implementation resul in a overwriting of the
>> IO region.
>>
>> We also need to make sure a clear or a halt started after a data
>> transfer do not win the race agains the data transfer.
>> Which would result in the data transfer being started after the
>> halt/clear.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_ops.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
>> index eb5b49d..b316966 100644
>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> @@ -267,22 +267,31 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
>>   {
>>   	unsigned int index = VFIO_CCW_OFFSET_TO_INDEX(*ppos);
>>   	struct vfio_ccw_private *private;
>> +	static atomic_t serialize  = ATOMIC_INIT(0);
>> +	int ret = -EINVAL;
>> +
>> +	if (!atomic_add_unless(&serialize, 1, 1))
>> +		return -EBUSY;
> 
> I think that hammer is far too big: This serializes _all_ write
> operations across _all_ devices.

Right, much too much.
This should go inside the device.
(Don't know what I was thinking of).


> 
> There are various cases of simultaneous writes that may happen
> (assuming any userspace; QEMU locking will prevent some of them from
> happening):
> 
> - One thread does a write for one mdev, another thread does a write for
>    another mdev. For example, if two vcpus issue an I/O instruction on
>    two different devices. This should be fine.
> - One thread does a write for one mdev, another thread does a write for
>    the same mdev. Maybe a guest has confused/no locking and is trying to
>    do ssch on the same device from different vcpus. There, we want to
>    exclude simultaneous writes; the desired outcome may be that one ssch
>    gets forwarded to the hardware, and the second one either gets
>    forwarded after processing for the first one has finished, or
>    userspace gets an error immediately (hopefully resulting in a
>    appropriate condition code for that second ssch in any case). Both
>    handing the second ssch to the hardware or signaling device busy
>    immediately are probably sane in that case.

should be.

> - If those writes for the same device involve hsch/csch, things get
>    more complicated. First, we have two different regions, and allowing
>    simultaneous writes to the I/O region and to the async region should
>    not really be a problem, so I don't think fencing should be done in
>    the generic write handler. Second, the semantics for device busy are
>    different: a hsch immediately after a ssch should not give device
>    busy, and csch cannot return device busy at all.

The lock should be done in the device and only for SSCH.
We did not have CSCH or HSCH at the moment I sent the patch.

It is clear that CSCH or HSCH should not be blocked.

> 
> I don't think we'll be able to get around some kind of "let's retry
> sending this" logic for hsch/csch; maybe we should already do that for
> ssch. (Like the -EINVAL logic I described in the other thread.)
> 
> 

As I wrote in the cover letter this (stupid) patch is decoupled from the 
series.
I made the mistake to attach it here but I hope you will consider the 
rest of the serie which I think is much more important.

The handling of a lock / busy waiting here should wait for your serie on 
HSCH/CSCH anyway.

Regards,
Pierre




-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany