Re: [PATCH 04/10] vfio: ccw: replace IO_REQ event with SSCH_REQ event

[Pierre Morel]

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.

Regards,

Pierre

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