On Tue, 5 Jun 2018 16:10:52 +0200
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:
On 05/06/2018 14:18, Cornelia Huck wrote:Why should the guest know anything about this? Getting the device to a
On Fri, 25 May 2018 12:21:16 +0200right, thanks.
Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx> wrote:
+static int fsm_online(struct vfio_ccw_private *private)So, what about a subchannel that is busy? Why should it go to the not
+{
+ struct subchannel *sch = private->sch;
+ int ret = VFIO_CCW_STATE_IDLE;
+
+ spin_lock_irq(sch->lock);
+ if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
+ ret = VFIO_CCW_STATE_NOT_OPER;
+ spin_unlock_irq(sch->lock);
+
+ return ret;
+}
+static int fsm_offline(struct vfio_ccw_private *private)
+{
+ struct subchannel *sch = private->sch;
+ int ret = VFIO_CCW_STATE_STANDBY;
+
+ spin_lock_irq(sch->lock);
+ if (cio_disable_subchannel(sch))
+ ret = VFIO_CCW_STATE_NOT_OPER;
oper state?
(And you should try to flush pending I/O and then try again in thatWhat about letting the guest doing this.
case. Otherwise, you may have a still-enabled subchannel which may
throw an interrupt.)
After giving him the right information on what happened of course.
usable state respectively cleaning up is the responsibility of the host
code. This processing will happen before the guest gets use of the
device or after it has lost use of it already (or it is some internal
handling like reset, which the guest should not be made aware of).
See my reply above.Same as above, what about letting the guest doing this?+ spin_unlock_irq(sch->lock);If I read this correctly, you're calling cio_cancel_halt_clear() only
+ if (private->completion)
+ complete(private->completion);
+
+ return ret;
+}
+static int fsm_quiescing(struct vfio_ccw_private *private)
+{
+ struct subchannel *sch = private->sch;
+ int ret = VFIO_CCW_STATE_STANDBY;
+ int iretry = 255;
+
+ spin_lock_irq(sch->lock);
+ ret = cio_cancel_halt_clear(sch, &iretry);
+ if (ret == -EBUSY)
+ ret = VFIO_CCW_STATE_QUIESCING;
+ else if (private->completion)
+ complete(private->completion);
+ spin_unlock_irq(sch->lock);
+ return ret;
once. What happened to the retry loop?
And there are already 255 retries as part of the interface to cio.From the kerneldoc comment for cio_cancel_halt_clear():
* This should be called repeatedly since halt/clear are asynchronous
* operations. We do one try with cio_cancel, three tries with cio_halt,
* 255 tries with cio_clear. The caller should initialize @iretry with
* the value 255 for its first call to this, and keep using the same
* @iretry in the subsequent calls until it gets a non -EBUSY return.
Hm, I think the changes to the fsm semantics are a bit mixed up betweenIt changes the FSM: NOT_OPER and STANDBY are clearly different.+}Doesn't that change the semantic of the standby state?
+static int fsm_quiescing_done(struct vfio_ccw_private *private)
+{
+ if (private->completion)
+ complete(private->completion);
+ return VFIO_CCW_STATE_STANDBY;
+}
/*
* No operation action.
*/
@@ -178,15 +225,10 @@ static int fsm_sch_event(struct vfio_ccw_private *private)
static int fsm_init(struct vfio_ccw_private *private)
{
struct subchannel *sch = private->sch;
- int ret = VFIO_CCW_STATE_STANDBY;
- spin_lock_irq(sch->lock);
sch->isc = VFIO_CCW_ISC;
- if (cio_enable_subchannel(sch, (u32)(unsigned long)sch))
- ret = VFIO_CCW_STATE_NOT_OPER;
- spin_unlock_irq(sch->lock);
- return ret;
+ return VFIO_CCW_STATE_STANDBY;
Part of the initialization is now done in when putting the device online.
patches. I'll wait for an outline of how this is supposed to look in
the end before commenting further :)
As described above, we need to be clear on what should be guest-visibleYour idea here seems to be to go to either disabling the subchannelI wanted to let the guest do the retries as he wants to.
directly or flushing out I/O first, depending on the state you're in.
The problem is that you may need retries in any case (the subchannel
may be status pending if it is enabled; not necessarily by any I/O that
had been started, but also from an unsolicited notification.)
Somehow we must give the right response back to the guest
and take care of the error number we give back.
and what is just internal handling e.g. during initialization/removal.
I will get a better look at this.The comment basically refers to "we aren't quite sure whether there is
I think that this is not the place to put this remark since here};This is still true, no? I'm thinking about things like channel monitors
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index ea8fd64..b202e73 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -21,21 +21,14 @@ static int vfio_ccw_mdev_reset(struct mdev_device *mdev)
private = dev_get_drvdata(mdev_parent_dev(mdev));
sch = private->sch;
- /*
- * TODO:
- * In the cureent stage, some things like "no I/O running" and "no
- * interrupt pending" are clear, but we are not sure what other state
- * we need to care about.
- * There are still a lot more instructions need to be handled. We
- * should come back here later.
- */
and the like (even if we don't support them yet).
we should send an event to the FSM, having new states
will be handled as FSM states.
I put it back, here or where I think it belong if I find another
place after resolving the RESET problem.
more stuff we need to reset", so I think this is indeed the correct
place.