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

[Cornelia Huck]

On Tue, 29 Jan 2019 09:14:40 -0500
Eric Farman <address@hidden> wrote:

> On 01/29/2019 05:20 AM, Cornelia Huck wrote:
> > On Mon, 28 Jan 2019 16:48:10 -0500
> > Eric Farman <address@hidden> wrote:
> >   
> >> On 01/28/2019 02:15 PM, Halil Pasic wrote:  
> >>> On Mon, 28 Jan 2019 18:09:48 +0100
> >>> Cornelia Huck <address@hidden> wrote:  
> >   
> >> 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.  
> >>
> >> Yes this is the one point I hadn't seen explicitly stated.  We shouldn't
> >> remain in state=BUSY if the ssch got cc!=0, and probably return to IDLE
> >> when processing the failure.  In Connie's response (Mon, 28 Jan 2019
> >> 18:24:24 +0100) to my note, she expressed some agreement to that.  
> > 
> > Yes, I think that's what should happen.
> > 
> >   
> >>>> 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.)
> >>>      
> >>>>
> >>>> 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).
> >>>      
> >>>>
> >>>> 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).  
> >>
> >> These three do read like CSCH/HSCH are subject to the same rules as
> >> SSCH, when in fact they would be (among other reasons) issued to clean
> >> up a lost interrupt from a previous SSCH.  So maybe return -EAGAIN on
> >> state=BUSY (don't race ourselves with the start), but issue to hardware
> >> if CP_PENDING.  
> > 
> > I think there are some devices which require a certain hsch/csch
> > sequence during device bringup, so it's not just cleaning up after a
> > ssch.   
> 
> Ah, yes.
> 
> Therefore, we should always try to do the requested hsch/csch,
> > unless things like "we're in the process of translating a cp, and can't
> > deal with another request right now" prevent it.  
> 
> Agreed.  I'm in support of all of this.

Cool. In the meantime, I've coded the changes, and I think the result
looks reasonable. I'll give it some testing and then send it out; it's
probably easier to discuss it with some code in front of us.

[The QEMU part should not need any changes.]

> 
> >   
> >>
> >> If we get an async request when state=IDLE, then maybe just issue it for
> >> fun, as if it were an SSCH?  
> > 
> > For fun, but mainly because the guest wants it :)
> >   
> 
> Well, that too.  ;-)
>