Re: [qemu-s390x] [PATCH v2 5/5] vfio-ccw: add handling for async channel instructions

[Cornelia Huck]

On Fri, 25 Jan 2019 16:00:38 -0500
Eric Farman <address@hidden> wrote:

> On 01/21/2019 06:03 AM, Cornelia Huck wrote:
> > Add a region to the vfio-ccw device that can be used to submit
> > asynchronous I/O instructions. ssch continues to be handled by the
> > existing I/O region; the new region handles hsch and csch.
> > 
> > Interrupt status continues to be reported through the same channels
> > as for ssch.
> > 
> > Signed-off-by: Cornelia Huck <address@hidden>
> > ---
> >   drivers/s390/cio/Makefile           |   3 +-
> >   drivers/s390/cio/vfio_ccw_async.c   |  91 ++++++++++++++++++++++
> >   drivers/s390/cio/vfio_ccw_drv.c     |  45 +++++++----
> >   drivers/s390/cio/vfio_ccw_fsm.c     | 114 +++++++++++++++++++++++++++-
> >   drivers/s390/cio/vfio_ccw_ops.c     |  13 +++-
> >   drivers/s390/cio/vfio_ccw_private.h |   9 ++-
> >   include/uapi/linux/vfio.h           |   2 +
> >   include/uapi/linux/vfio_ccw.h       |  12 +++
> >   8 files changed, 269 insertions(+), 20 deletions(-)
> >   create mode 100644 drivers/s390/cio/vfio_ccw_async.c

(...)

> > @@ -149,7 +155,10 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
> >     cio_disable_subchannel(sch);
> >   out_free:
> >     dev_set_drvdata(&sch->dev, NULL);
> > -   kmem_cache_free(vfio_ccw_io_region, private->io_region);
> > +   if (private->cmd_region)
> > +           kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
> > +   if (private->io_region)
> > +           kmem_cache_free(vfio_ccw_io_region, private->io_region);  
> 
> Well, adding the check if private->xxx_region is non-NULL is fine.  I'd 
> have made it a separate patch for io_region, but whatever.
> 
> Since you're adding that check, you should add the same if statement in 
> vfio_ccw_sch_remove().  And you should certainly call 
> kmem_cache_free(private->cmd_region) there too.  :)

Ehm, yes :)

> 
> >     kfree(private);
> >     return ret;
> >   }

(...)

> > +static int fsm_do_halt(struct vfio_ccw_private *private)
> > +{
> > +   struct subchannel *sch;
> > +   unsigned long flags;
> > +   int ccode;
> > +   int ret;
> > +
> > +   sch = private->sch;
> > +
> > +   spin_lock_irqsave(sch->lock, flags);
> > +
> > +   /* Issue "Halt Subchannel" */
> > +   ccode = hsch(sch->schid);
> > +
> > +   switch (ccode) {
> > +   case 0:
> > +           /*
> > +            * Initialize device status information
> > +            */
> > +           sch->schib.scsw.cmd.actl |= SCSW_ACTL_HALT_PEND;
> > +           ret = 0;
> > +           private->state = VFIO_CCW_STATE_BUSY;
> > +           break;
> > +   case 1:         /* Status pending */
> > +   case 2:         /* Busy */
> > +           ret = -EBUSY;
> > +           break;
> > +   case 3:         /* Device not operational */
> > +   {
> > +           ret = -ENODEV;
> > +           break;
> > +   }  
> 
> Why does cc3 get braces, but no other case does?  (Ditto for clear)
> 
> (I guess the answer is "because fsm_io_helper does" but I didn't notice 
> that before either.  :-)

Yes, the power of copy/paste :) But it makes sense to avoid adding them.

> 
> > +   default:
> > +           ret = ccode;
> > +   }
> > +   spin_unlock_irqrestore(sch->lock, flags);
> > +   return ret;
> > +}

(...)

> > +static void fsm_async_error(struct vfio_ccw_private *private,
> > +                       enum vfio_ccw_event event)
> > +{
> > +   pr_err("vfio-ccw: FSM: halt/clear request from state:%d\n",
> > +          private->state);  
> 
> Had a little deja vu here.  :)
> 
> Can this message use private->cmd_region->command to tell us if it's a 
> halt, clear, or unknown?  Instead of just saying "halt/clear" statically.

Can do that; need to check if we need the mutex.

> 
> > +   private->cmd_region->ret_code = -EIO;
> > +}
> > +