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

[Halil Pasic <pasic@linux.ibm.com>]

On Tue, 24 Mar 2020 18:04:30 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu,  6 Feb 2020 22:45:03 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
> > From: Farhan Ali <alifm@linux.ibm.com>
> > 
> > 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 <alifm@linux.ibm.com>
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> > 
> > 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.

Regards,
Halil

> 
> > 
> >  hw/vfio/ccw.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> > index 50cc2ec75c..19144ecfc7 100644
> > --- a/hw/vfio/ccw.c
> > +++ b/hw/vfio/ccw.c
> > @@ -114,6 +114,7 @@ again:
> >          return IOINST_CC_BUSY;
> >      case -ENODEV:
> >      case -EACCES:
> > +    case -EIO:
> >          return IOINST_CC_NOT_OPERATIONAL;
> >      case -EFAULT:
> >      default:
> 
>