Re: [RFC PATCH v2 1/7] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO

[Cornelia Huck]

On Wed, 25 Mar 2020 03:24:28 +0100
Halil Pasic <address@hidden> wrote:

> On Tue, 24 Mar 2020 18:04:30 +0100
> Cornelia Huck <address@hidden> wrote:
> 
> > On Thu,  6 Feb 2020 22:45:03 +0100
> > Eric Farman <address@hidden> wrote:
> >   
> > > From: Farhan Ali <address@hidden>
> > > 
> > > EIO is returned by vfio-ccw mediated device when the backing
> > > host subchannel is not operational anymore. So return cc=3
> > > back to the guest, rather than returning a unit check.
> > > This way the guest can take appropriate action such as
> > > issue an 'stsch'.  
> 
> I believe this is not the only situation when vfio-ccw returns
> EIO, or?
> 
> > > 
> > > Signed-off-by: Farhan Ali <address@hidden>
> > > Signed-off-by: Eric Farman <address@hidden>
> > > ---
> > > 
> > > Notes:
> > >     v1->v2: [EF]
> > >      - Add s-o-b
> > >      - [Seems the discussion on v1 centered on the return code
> > >        set in the kernel, rather than anything that needs to
> > >        change here, unless I've missed something.]  
> 
> Does this need to change here? If the kernel is supposed to return ENODEV
> then this does not need to change.
> 
> > 
> > I've stared at this and at the kernel code for some time again; and I'm
> > not sure if "return -EIO == not operational" is even true. That said,
> > I'm not sure a unit check is the right response, either. The only thing
> > I'm sure about is that the kernel code needs some review of return
> > codes and some documentation...  
> 
> I could not agree more, this is semantically uapi and needs to be
> properly documented.
> 
> With regards to "linux error codes: vs "ionist cc's" an where
> the mapping is different example:
> 
> """
> /**                                                                           
>   
>  * cio_cancel_halt_clear - Cancel running I/O by performing cancel, halt      
>   
>  * and clear ordinally if subchannel is valid.                                
>   
>  * @sch: subchannel on which to perform the cancel_halt_clear operation       
>   
>  * @iretry: the number of the times remained to retry the next operation      
>   
>  *                                                                            
>   
>  * This should be called repeatedly since halt/clear are asynchronous         
>   
>  * operations. We do one try with cio_cancel, three tries with cio_halt,      
>   
>  * 255 tries with cio_clear. The caller should initialize @iretry with        
>   
>  * the value 255 for its first call to this, and keep using the same          
>   
>  * @iretry in the subsequent calls until it gets a non -EBUSY return.         
>   
>  *                                                                            
>   
>  * Returns 0 if device now idle, -ENODEV for device not operational,          
>   
>  * -EBUSY if an interrupt is expected (either from halt/clear or from a       
>   
>  * status pending), and -EIO if out of retries.                               
>   
>  */                                                                           
>   
> int cio_cancel_halt_clear(struct subchannel *sch, int *iretry)   
> 
> """
> Here -ENODEV is not operational.

Ok, I went through the kernel code and checked which error may be
returned in which case (hope I caught all of them). Here's what I
currently have:

diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
index fca9c4f5bd9c..43f375a03cce 100644
--- a/Documentation/s390/vfio-ccw.rst
+++ b/Documentation/s390/vfio-ccw.rst
@@ -210,7 +210,36 @@ Subchannel.
 
 irb_area stores the I/O result.
 
-ret_code stores a return code for each access of the region.
+ret_code stores a return code for each access of the region. The following
+values may occur:
+
+0
+  The operation was successful.
+
+-EOPNOTSUPP
+  The orb specified transport mode or an unidentified IDAW format, did not
+  specify prefetch mode, or the scsw specified a function other than the
+  start function.
+
+-EIO
+  A request was issued while the device was not in a state ready to accept
+  requests, or an internal error occurred.
+
+-EBUSY
+  The subchannel was status pending or busy, or a request is already active.
+
+-EAGAIN
+  A request was being processed, and the caller should retry.
+
+-EACCES
+  The channel path(s) used for the I/O were found to be not operational.
+
+-ENODEV
+  The device was found to be not operational.
+
+-EINVAL
+  The orb specified a chain longer than 255 ccws, or an internal error
+  occurred.
 
 This region is always available.
 
@@ -231,6 +260,29 @@ This region is exposed via region type 
VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD.
 
 Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region.
 
+command specifies the command to be issued; ret_code stores a return code
+for each access of the region. The following values may occur:
+
+0
+  The operation was successful.
+
+-ENODEV
+  The device was found to be not operational.
+
+-EINVAL
+  A command other than halt or clear was specified.
+
+-EIO
+  A request was issued while the device was not in a state ready to accept
+  requests.
+
+-EAGAIN
+  A request was being processed, and the caller should retry.
+
+-EBUSY
+  The subchannel was status pending or busy while processing a halt request.
+
+
 vfio-ccw operation details
 --------------------------
 
Unless we interpret "device in wrong state" as "not operational",
mapping -EIO to cc 3 is probably not the right thing to do; but
generating a unit exception probably isn't either. Honestly, I'm unsure
what the right thing to do here would be...

Another problem is that -EIO might signal "something went wrong in the
kernel code" - should not happen, but would certainly not be the
caller's fault, and they can't do anything about it. That "internal
error" thing might also be signaled by -EINVAL (which is odd), but
-EINVAL could also mean "the ccw chain is too long", for which
-EOPNOTSUPP would probably be a better return code, as it's a
limitation in the code, not the architecture, IIRC. Not sure if we can
still change that, though (and QEMU handles both in the same way,
anyway.)

The other return codes look sane, and the return codes for the async
region also seem sane (but have the same issue with -EIO == "device in
wrong state").

Looking at the QEMU handling, I think the -EIO handling is a bit
questionable (without an obvious solution), while mapping -EBUSY to cc
2 is the only reasonable thing to do given that the interface does not
differentiate between "busy" and "status pending". The rest seems sane.