Re: [PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine
On Wed, 27 Jun 2018 12:00:41 +0200 Pierre Morel wrote: Apologies for the late answer, this topic had dropped off my radar. > On 26/06/2018 18:00, Cornelia Huck wrote: > > On Tue, 26 Jun 2018 13:04:12 +0200 > > Pierre Morel wrote: > > > >> On 19/06/2018 16:00, Cornelia Huck wrote: > >>> On Thu, 14 Jun 2018 10:06:31 +0200 > >>> Pierre Morel wrote: > >>> > ...snip... > >> The goal of the state machine is to describe the device driver state. > >> Not the device state which is hold by the CIO level. > > I don't think there really is such a thing as "device driver state" (or > > maybe I don't understand what you mean by it). The state is attached to > > the individual device, isn't it? > Yes but it is related to the software handling the device and not to the > device. > This is a difference between QEMU, Linux VFIO driver and Hardware. > > In my point of view: > > QEMU : Emulates a device, internal state is state of > the device > Linux VFIO driver: Handle the device, internal state is driver state > Hardware : obviously get the device state OK, I understand where you're coming from, but I find your terminology a bit confusing :) Maybe call the state that is tracked by the vfio driver "vfio device state"? > > | STANDBY | A guest is associated but not started | > >>> This is basically "device is bound to driver, but no mdev has been set > >>> up". > >> When the device is bound to the driver, the device driver > >> is still in NOT_OPERATIONAL state. > >> > >> The transition to STANDBY is done when the virtual machine starts and > >> opens the mediated device. > >> Note that the device is still not usable until it is reseted. > > Hm, isn't that transition happening when the mdev is created? > No, when the mdev is created the VM is not started. > The STANDBY state is entered when the mdev_open is called, > this is when the VFIO framework is initialized as QEMU starts. > > When the mdev is created the mdev_create is called but do not > induce any state change. Ah, now I understand where the disconnect here came from: Your patch 7 changed this :) I have to think further about this (still undecided). > ...snip... > >> Now you are talking about the driver :) This should probably be done > >>> for the other states as well. > >> Ill update the description to make clear that the state is the > >> driver state (device driver) and not the device state. > >> The device state is handled by the firmware. > > Now you have managed to confuse me completely... isn't the firmware > > only handling the (real) subchannel state? > > Yes and QEMU the virtual one. > But the driver does not need to handle the device state > but the driver state. Which is quite different. That's again the terminology confusion from above. > >>> Isn't that rather "an mdev is created"? > >> No, ONLINE is triggered by VFIO_RESET, after the mdev has been created > >> and when the VM is started or restarted by QEMU. > >> > >> I see that I did not do a good job describing what triggers the events. > >> I will try again in a dedicated document. > > It might be good to combine the two. > > When mdev is created the VM does not exist, it is created by the admin. > > The mdev_open() is called once when the VM starts. > VFIO_RESET is called on start and every time the VM is reseted. And this was again /me looking at the current code. > > These are three different events occuring in the life of the driver. > mdev creation do not influence the way the driver works. I'm wondering if it should. Having an mdev is a prereq for any setup. So a device in not oper state without mdev is a different thing from a device in not oper state with an mdev that received e.g. some path failures. > mdev open does, and issues the INIT event > VFIO_RESET does and issues the ONLINE event The last one is possibly a bit unintuitive (might be a naming issue). > | | triggered by: vfio_reset | > | | action: enable SC | > | | state on success: IDLE | > | | state on error : STANDBY | > >>> What happens if the subchannel is not operational when we try to get it > >>> ready? Can it go to NOT_OPERATIONAL in that case? > >> I think we have a confusion between the sub-channel being non operational > >> and the device driver being non operational. > >> Here, the device driver is operational, even the device may not. > > How can something be operational if the device is not? It could be in a > > state like the ccw device's disconnected state, but it's certainly not > > ready for requests. > > The driver may be operational even the device is not. > Depending on the protocol between device and driver it allows > the driver to reset the device. > We have two drivers here speaking wit
Re: [PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine
On 26/06/2018 18:00, Cornelia Huck wrote: On Tue, 26 Jun 2018 13:04:12 +0200 Pierre Morel wrote: On 19/06/2018 16:00, Cornelia Huck wrote: On Thu, 14 Jun 2018 10:06:31 +0200 Pierre Morel wrote: ...snip... The goal of the state machine is to describe the device driver state. Not the device state which is hold by the CIO level. I don't think there really is such a thing as "device driver state" (or maybe I don't understand what you mean by it). The state is attached to the individual device, isn't it? Yes but it is related to the software handling the device and not to the device. This is a difference between QEMU, Linux VFIO driver and Hardware. In my point of view: QEMU : Emulates a device, internal state is state of the device Linux VFIO driver: Handle the device, internal state is driver state Hardware : obviously get the device state I think this has lead to some confusion since I tried to keep the old naming convention. So, I agree that a state named "NON_INIT" or "UNCONFIGUED" would be better to distinguish it from the device state "NOT_OPERATIONAL" Also, we need to be careful with the virtual machine vs. guest terminology. The only thing that has an impact from the guest is when it does I/O and when an interrupt is generated as a result (i.e., the IDLE/BUSY transitions). The other transitions are triggered by virtual machine setup. Absolutely. | STANDBY | A guest is associated but not started | This is basically "device is bound to driver, but no mdev has been set up". When the device is bound to the driver, the device driver is still in NOT_OPERATIONAL state. The transition to STANDBY is done when the virtual machine starts and opens the mediated device. Note that the device is still not usable until it is reseted. Hm, isn't that transition happening when the mdev is created? No, when the mdev is created the VM is not started. The STANDBY state is entered when the mdev_open is called, this is when the VFIO framework is initialized as QEMU starts. When the mdev is created the mdev_create is called but do not induce any state change. ...snip... Now you are talking about the driver :) This should probably be done for the other states as well. Ill update the description to make clear that the state is the driver state (device driver) and not the device state. The device state is handled by the firmware. Now you have managed to confuse me completely... isn't the firmware only handling the (real) subchannel state? Yes and QEMU the virtual one. But the driver does not need to handle the device state but the driver state. Which is quite different. Isn't that rather "an mdev is created"? No, ONLINE is triggered by VFIO_RESET, after the mdev has been created and when the VM is started or restarted by QEMU. I see that I did not do a good job describing what triggers the events. I will try again in a dedicated document. It might be good to combine the two. When mdev is created the VM does not exist, it is created by the admin. The mdev_open() is called once when the VM starts. VFIO_RESET is called on start and every time the VM is reseted. These are three different events occuring in the life of the driver. mdev creation do not influence the way the driver works. mdev open does, and issues the INIT event VFIO_RESET does and issues the ONLINE event | | | | | triggered by: vfio_reset | | | action: enable SC | | | state on success: IDLE | | | state on error : STANDBY | What happens if the subchannel is not operational when we try to get it ready? Can it go to NOT_OPERATIONAL in that case? I think we have a confusion between the sub-channel being non operational and the device driver being non operational. Here, the device driver is operational, even the device may not. How can something be operational if the device is not? It could be in a state like the ccw device's disconnected state, but it's certainly not ready for requests. The driver may be operational even the device is not. Depending on the protocol between device and driver it allows the driver to reset the device. We have two drivers here speaking with the device, the one in the guest and the VFIO driver. The VFIO driver works is to handle the virtualization infrastructure, not to handle the device. Handling the device is done by the guest driver. For the VM perspective, if the device is not operational we send a RESET. For the guest perspective we can do what ever instruction we needs to get the device operational, therefor we need the driver to be operational to process the instruction. Ah, do you mean 'subchannel enabled'? I was thinking about 'device dead, we get cc 3 or somesuch'. What can we do on the host level
Re: [PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine
On Tue, 26 Jun 2018 13:04:12 +0200 Pierre Morel wrote: > On 19/06/2018 16:00, Cornelia Huck wrote: > > On Thu, 14 Jun 2018 10:06:31 +0200 > > Pierre Morel wrote: > > > >> I tried to make a better description to add later in documentation > >> or in the next cover-letter. > >> > >> Note that in the current patch series I did not implement online/offline > >> events but just kept the previous state changes. > >> Not sure if it was a good idea, the goal was to be as simple as possible > >> for this iteration. > >> If you think it is worth to continue in this direction I will re-introduce > >> these as events. > > I'm still trying to figure out what we want from the state machine. > > I've tried sketching your fsm proposal as outlined by you below for > > myself and I have some remarks :) > > Hi, > > Sorry for the delay in my answer, I was away from my keyboard > almost the all week :) . np :) > > > > >> == > >> > >> 1) In CCW VFIO, Finite State Machine is used to clarify the VFIO CCW driver > >> actions to be take depending on the events occurring in a device. > >> > >> To protect the state transitions, a mutex protect the action > >> started when an event occurs in a specific state. > >> > >> The actions must never sleep or keep the mutex a long timer. > >> > >> To be able to take the mutex when hardware events occur we start > >> a work on a dedicated workqueue, posting the event from inside the > >> workqueue. > >> > >> The state machine has very few states describing the device driver life. > >> > >> > >> | NOT_OPERATIONAL | No guest is associated with the device| > > I don't think that this is the right description. It is either the > > initial state, or something has happened that rendered the device > > inaccessible. > > The goal of the state machine is to describe the device driver state. > Not the device state which is hold by the CIO level. I don't think there really is such a thing as "device driver state" (or maybe I don't understand what you mean by it). The state is attached to the individual device, isn't it? > > I think this has lead to some confusion since I tried to keep the old > naming convention. > So, I agree that a state named "NON_INIT" or "UNCONFIGUED" would be better > to distinguish it from the device state "NOT_OPERATIONAL" > > > Also, we need to be careful with the virtual machine vs. guest > > terminology. The only thing that has an impact from the guest is when > > it does I/O and when an interrupt is generated as a result (i.e., the > > IDLE/BUSY transitions). The other transitions are triggered by virtual > > machine setup. > > Absolutely. > > >> > >> | STANDBY | A guest is associated but not started | > > This is basically "device is bound to driver, but no mdev has been set > > up". > > When the device is bound to the driver, the device driver > is still in NOT_OPERATIONAL state. > > The transition to STANDBY is done when the virtual machine starts and > opens the mediated device. > Note that the device is still not usable until it is reseted. Hm, isn't that transition happening when the mdev is created? > > > In my understanding, we need to get the device to IDLE state > > before the vm can present it to the guest (be it before the machine is > > up or during hotplug). > > The virtual machine needs to RESET the device sending the VFIO_RESET > to the device driver to make the device driver go to the IDLE state. > > >> > >> | IDLE | Guest started device ready | > > Agree with "device ready", disagree with "guest started" (see above). > > "Device ready, accepting I/O requests"? > > Indeed bad description, VM started, and almost as you said > "Device driver ready, accepting I/O requests for device" > (Device is ready too) > > >> > >> | BUSY | The device is busy doing IO | > >> > >> | QUIESCING | The driver is releasing the device | > > Now you are talking about the driver :) This should probably be done > > for the other states as well. > > Ill update the description to make clear that the state is the > driver state (device driver) and not the device state. > The device state is handled by the firmware. Now you have managed to confuse me completely... isn't the firmware only handling the (real) subchannel state? > > > Isn't that state for waiting for outstanding I/O to be terminated > > before the mdev is destroyed? IOW, the device may stay bound to the > > driver afterwards? > > Yes it is. > > >> > >> > >> > >> 2) The following Events may apply to the state machine: > >> >
Re: [PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine
On Thu, 14 Jun 2018 10:06:31 +0200 Pierre Morel wrote: > I tried to make a better description to add later in documentation > or in the next cover-letter. > > Note that in the current patch series I did not implement online/offline > events but just kept the previous state changes. > Not sure if it was a good idea, the goal was to be as simple as possible > for this iteration. > If you think it is worth to continue in this direction I will re-introduce > these as events. I'm still trying to figure out what we want from the state machine. I've tried sketching your fsm proposal as outlined by you below for myself and I have some remarks :) > > == > > 1) In CCW VFIO, Finite State Machine is used to clarify the VFIO CCW driver > actions to be take depending on the events occurring in a device. > > To protect the state transitions, a mutex protect the action > started when an event occurs in a specific state. > > The actions must never sleep or keep the mutex a long timer. > > To be able to take the mutex when hardware events occur we start > a work on a dedicated workqueue, posting the event from inside the > workqueue. > > The state machine has very few states describing the device driver life. > > > | NOT_OPERATIONAL | No guest is associated with the device| I don't think that this is the right description. It is either the initial state, or something has happened that rendered the device inaccessible. Also, we need to be careful with the virtual machine vs. guest terminology. The only thing that has an impact from the guest is when it does I/O and when an interrupt is generated as a result (i.e., the IDLE/BUSY transitions). The other transitions are triggered by virtual machine setup. > > | STANDBY | A guest is associated but not started | This is basically "device is bound to driver, but no mdev has been set up". In my understanding, we need to get the device to IDLE state before the vm can present it to the guest (be it before the machine is up or during hotplug). > > | IDLE | Guest started device ready | Agree with "device ready", disagree with "guest started" (see above). "Device ready, accepting I/O requests"? > > | BUSY | The device is busy doing IO | > > | QUIESCING | The driver is releasing the device | Now you are talking about the driver :) This should probably be done for the other states as well. Isn't that state for waiting for outstanding I/O to be terminated before the mdev is destroyed? IOW, the device may stay bound to the driver afterwards? > > > > 2) The following Events may apply to the state machine: > > If the event is not described, it means it has no influence > on the state and that no action is required. > > 2.1) FSM in state NOT_OPERATIONAL > > | INIT | a guest will drive the SC | Better to write out "subchannel", I could not figure out the abbreviation immediately. Also, doesn't the event really mean "we're initializing the device"? The ultimate intention is of course for a guest to use the device, but the immediate intention is just "get the device through the first initialization steps". > | | | > | | triggered by: mdev_open | > | | action: initialize driver structures | > | | action: register IOMMU notifier | > | | state on success: STANDBY | > | | state on error : NOT_OPERATIONAL | > > > 2.2) FSM in state STANDBY > > > | ONLINE | The host wants the SC online | Isn't that rather "an mdev is created"? > | | | > | | triggered by: vfio_reset | > | | action: enable SC | > | | state on success: IDLE | > | | state on error : STANDBY | What happens if the subchannel is not operational when we try to get it ready? Can it go to NOT_OPERATIONAL in that case? > > > Some operations may be intercepted by the state machine but > will not induce a state change: > OFFLINE: re-issue the disabling of the SC Should that even be possible? If we're still busy tearing it down, shouldn't we be in QUIESCING state? > I
Re: [PATCH v3 0/8] vfio: ccw: Refactoring the VFIO CCW state machine
On Tue, 12 Jun 2018 09:56:42 +0200 Pierre Morel wrote: > The goal of the patch serie is to secure the state machine by: > - centralizing all state changes inside the state machine wrapper > - make the state change atomic using mutexes > - refactor the initialization to avoid using a subchannel without a guest > > > This series introduces new states and events and suppressed > others. > Here the list of states and events used in this serie: I've tried to come up with some annotations (without looking at the code :); please correct me if I'm wrong. I want to understand the design better. > - VFIO_CCW_STATE_NOT_OPER: when the Sub-Channel is KO These are cases of "not operational, we can't even talk to it anymore", right? If so, the only way out is unregistration of the subchannel, unless we want to keep it around in case it revives and we get a machine check. So this is likely a transient state? > - VFIO_CCW_STATE_STANDBY : when it is offline Bound to the driver, but no mdev (and subchannel disabled). The target states that could happen are either not oper (device gone; we notice via machine check or when we try to enable the subchannel) or idle (we enable the subchannel and make it ready for use). Non-transient state (will stay there until something happens). > - VFIO_CCW_STATE_IDLE: when it is ready for I/O Can go to not oper (device gone), standby (via quiescing?) or busy (guest does I/O). Non-transient state. > - VFIO_CCW_STATE_BUSY: when it is busy doing I/O Can go to idle (all done), or to not oper, I guess. Transient state. > - VFIO_CCW_STATE_QUIESCING(N): when it is busy going offline We're doing cancel/halt/clear. Target is standby, but can go to not oper if device gone. Transient state. The boxed state you're removing might have served as a non-transient equivalent to busy (device does not respond, needs a kick to get out of the state), but we can re-introduce it if we find it useful in the future. > > - VFIO_CCW_EVENT_INIT: the channel setup (admin) By "admin" you mean "action triggered by admin", right? > - VFIO_CCW_EVENT_NOT_OPER: something really wrong happened That's device gone resp. not operational. Anything else? I suppose that can only trigger as a reaction to some hardware reconfiguration or malfunction. > - VFIO_CCW_EVENT_IO_REQ : Starting a SSCH request (UAPI) Triggered by guest action. > - VFIO_CCW_EVENT_INTERRUPT(N): Receiving an interrupt (callback) Triggered by hardware. > - VFIO_CCW_EVENT_SCHIB_CHANGED(N): Receiving a channel event (callback) Also triggered by hardware? Can this also trigger a not oper event? > > The user's ABI do not change. > > > > Pierre Morel (8): > vfio: ccw: Moving state change out of IRQ context > vfio: ccw: Transform FSM functions to return state > vfio: ccw: new VFIO_CCW_EVENT_SCHIB_CHANGED event > vfio: ccw: Only accept SSCH as an IO request > vfio: ccw: Suppress unused event parameter > vfio: ccw: Make FSM functions atomic > vfio: ccw: Introduce the INIT event > vfio: ccw: Suppressing the BOXED state > > drivers/s390/cio/vfio_ccw_drv.c | 71 ++--- > drivers/s390/cio/vfio_ccw_fsm.c | 147 > +--- > drivers/s390/cio/vfio_ccw_ops.c | 41 +- > drivers/s390/cio/vfio_ccw_private.h | 12 ++- > 4 files changed, 137 insertions(+), 134 deletions(-) >
