On 21/11/2018 17:48, Cornelia Huck wrote:
On Wed, 17 Oct 2018 11:18:41 +0200
Pierre Morel <pmorel@xxxxxxxxxxxxx> 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.
When the mediated device is closed, disable the sub channel
by calling vfio_ccw_sch_quiesce().
Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
drivers/s390/cio/vfio_ccw_drv.c | 10 +---------
drivers/s390/cio/vfio_ccw_ops.c | 23 +++++++++++++++++++++--
2 files changed, 22 insertions(+), 11 deletions(-)
(...)
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index f673e10..87a8f8c 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -146,11 +146,29 @@ 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;
- return vfio_register_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
- &events, &private->nb);
+ ret = vfio_register_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+ &events, &private->nb);
+ if (ret)
+ 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;
This -EFAULT looks a bit odd. Wouldn't it make more sense to relay the
return code of cio_enable_subchannel() here?
It looks odd.
It took me a long time to go through the code to see what happens with
these error codes and why.
... for at the end agree with your conclusion that returning the
cio_enable_subchannel() code is the best solution.
The QEMU code currently do not make use of the return value but we open
us possibilities for retries or analyze.
Thanks.
Regards,
Pierre
}
static void vfio_ccw_mdev_release(struct mdev_device *mdev)
@@ -158,6 +176,7 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
struct vfio_ccw_private *private =
dev_get_drvdata(mdev_parent_dev(mdev));
+ vfio_ccw_sch_quiesce(private->sch);
vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
&private->nb);
}
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
