Re: [Qemu-devel] [PATCH v2 2/5] vfio-ccw: concurrent I/O handling

[Cornelia Huck]

On Mon, 28 Jan 2019 20:15:48 +0100
Halil Pasic <address@hidden> wrote:

> On Mon, 28 Jan 2019 18:09:48 +0100
> Cornelia Huck <address@hidden> wrote:
> 
> > On Fri, 25 Jan 2019 15:01:01 +0100
> > Halil Pasic <address@hidden> wrote:
> >   
> > > On Fri, 25 Jan 2019 13:58:35 +0100
> > > Cornelia Huck <address@hidden> wrote:  
> >   
> > > > - The code should not be interrupted while we process the channel
> > > >   program, do the ssch etc. We want the caller to try again later (i.e.
> > > >   return -EAGAIN)    
> > 
> > (...)
> >   
> > > > - With the async interface, we want user space to be able to submit a
> > > >   halt/clear while a start request is still in flight, but not while
> > > >   we're processing a start request with translation etc. We probably
> > > >   want to do -EAGAIN in that case.    
> > > 
> > > This reads very similar to your first point.  
> > 
> > Not quite. ssch() means that we have a cp around; for hsch()/csch() we
> > don't have such a thing. So we want to protect the process of
> > translating the cp etc., but we don't need such protection for the
> > halt/clear processing.
> >   
> 
> What does this don't 'need such protection' mean in terms of code,
> moving the unlock of the io_mutex upward (in
> vfio_ccw_async_region_write())?

We don't have a cp that we need to process, so we don't need protection
for that.

> > 
> > IDLE --- IO_REQ --> BUSY ---> CP_PENDING --- IRQ ---> IDLE (if final  
> 
> There ain't no trigger/action list  between BUSY and CP_PENDING.
> I'm also in the  dark about where the issuing of the ssch() happen
> here (is it an internal transition within CP_PENDING?). I guess if
> the ssch() returns with non cc == 0 the CP_PENDING ---IRQ---> IDLE
> transition
> won't take place. And I guess the IRQ is a final one.

Please refer to the original ideas. This is obviously not supposed to
be a complete description of every case we might encounter.

> > state for I/O)
> > (normal ssch)
> > 
> > BUSY --- IO_REQ ---> return -EAGAIN, stay in BUSY
> > (user space is supposed to retry, as we'll eventually progress from
> > BUSY)
> > 
> > CP_PENDING --- IO_REQ ---> return -EBUSY, stay in CP_PENDING
> > (user space is supposed to map this to the appropriate cc for the guest)  
> 
> From this it seems you don't intend to issue the second  requested ssch()
> any more (and don't want to do any translation). Is that right? (If it
> is, that what I was asking for for a while, but then it's a pity for the
> retries.)

Which "second requested ssch"? In the first case, user space is
supposed to retry; in the second case, it should map it to a cc (and
the guest does whatever it does on busy conditions). We can't issue a
ssch if we're not able to handle multiple cps.

> 
> > 
> > IDLE --- ASYNC_REQ ---> IDLE
> > (user space is welcome to do anything else right away)  
> 
> Your idea is to not issue a requested hsch() if we think we are IDLE
> it seems. Do I understand this right? We would end up with a different
> semantic for hsch()/and csch() (compared to PoP) in the guest with this
> (AFAICT).

Nope, we're doing hsch/csch. We're just not moving out of IDLE, as we
(a) don't have any cp processing we need to protect and (b) no need to
fence of multiple attempts of hsch/csch.

> 
> > 
> > BUSY --- ASYNC_REQ ---> return -EAGAIN, stay in BUSY
> > (user space is supposed to retry, as above)
> > 
> > CP_PENDING --- ASYNC_REQ ---> return success, stay in CP_PENDING
> > (the interrupt will get us out of CP_PENDING eventually)  
> 
> Issue (c|h)sch() is an action that is done on this internal 
> transition (within CP_PENDING).

Yes. hsch/csch do not trigger a state change (other than possibly
dropping into NOT_OPER for cc 3).