On 22/05/2018 17:41, Cornelia Huck wrote:
On Fri, 4 May 2018 13:02:36 +0200
Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx> wrote:
On 04/05/2018 03:19, Dong Jia Shi wrote:
* Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx> [2018-05-03 16:26:29 +0200]:
On 02/05/2018 09:46, Dong Jia Shi wrote:
* Cornelia Huck <cohuck@xxxxxxxxxx> [2018-04-30 17:33:05 +0200]:
On Thu, 26 Apr 2018 15:48:06 +0800
Dong Jia Shi <bjsdjshi@xxxxxxxxxxxxxxxxxx> wrote:
* Dong Jia Shi <bjsdjshi@xxxxxxxxxxxxxxxxxx> [2018-04-26 15:30:54 +0800]:
[...]
@@ -179,7 +160,7 @@ static int fsm_irq(struct vfio_ccw_private *private,
if (private->io_trigger)
eventfd_signal(private->io_trigger, 1);
- return private->state;
+ return VFIO_CCW_STATE_IDLE;
This is not right. For example, if we are in STANDBY state (subch driver
is probed, but mdev device is not created), we can not jump to IDLE
state.
I see my problem, for STANDBY state, we should introduce another event
callback for VFIO_CCW_EVENT_INTERRUPT. It doesn't make sense to call
fsm_irq() which tries to signal userspace with interrupt notification
when mdev is not created yet... So we'd need a separated fix for this
issue too.
But how do we even get into that situation when we don't have an mdev
yet?
We cann't... So let's assign fsm_nop() as the interrupt callback for
STANDBY state?
:) Isn't it exactly what my patch series handle?
As far as I see, that's not true. ;)
After this series applied,
vfio_ccw_jumptable[VFIO_CCW_STATE_STANDBY][VFIO_CCW_EVENT_INTERRUPT] is
still fsm_irq().
What I mean is, this code tries to handle design problems
without changing too much of the original code at first.
The problem here is not that the fsm_irq function is called on interrupt,
if we have an interrupt it must be signaled to user land.
The problem is that this state is entered at the wrong moment.
STANDBY should be entered, during the mdev_open when we realize the QEMU
device,
and not during the probe, in which we should stay in NOT_OPER until we
get the QEMU device.
The probe() and mdev_open() function should be modified, not the state
table.
So, the takeaway is that we should handle starting via the init
callbacks and not via the state machine?
hum, sorry, I think that my previous answer was not completely right,
and did not really answer to Dong Jia comment, yes fsm_irq was not
at its place, thinking again about the comments of both of you
I think that we can suppress the INIT event.
I would like to rebase the patch to include the comments you both did.
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
