Re: [qemu-s390x] [PATCH 3/3] vfio-ccw: add handling for asnyc channel instructions

[Cornelia Huck]

On Tue, 27 Nov 2018 14:09:49 -0500
Farhan Ali <address@hidden> wrote:

> On 11/22/2018 11:54 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   |  88 ++++++++++++++++
> >   drivers/s390/cio/vfio_ccw_drv.c     |  48 ++++++---
> >   drivers/s390/cio/vfio_ccw_fsm.c     | 158 +++++++++++++++++++++++++++-
> >   drivers/s390/cio/vfio_ccw_ops.c     |  13 ++-
> >   drivers/s390/cio/vfio_ccw_private.h |   6 ++
> >   include/uapi/linux/vfio.h           |   4 +
> >   include/uapi/linux/vfio_ccw.h       |  12 +++
> >   8 files changed, 313 insertions(+), 19 deletions(-)
> >   create mode 100644 drivers/s390/cio/vfio_ccw_async.c
> > 

> > @@ -76,7 +79,8 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
> >     private = container_of(work, struct vfio_ccw_private, io_work);
> >     irb = &private->irb;
> >   
> > -   if (scsw_is_solicited(&irb->scsw)) {
> > +   if (scsw_is_solicited(&irb->scsw) &&
> > +       (scsw_fctl(&irb->scsw) & SCSW_FCTL_START_FUNC)) {
> >             cp_update_scsw(&private->cp, &irb->scsw);
> >             cp_free(&private->cp);
> >     }  
> 
> I am a little confused about this. Why do we need to update the scsw.cpa 
> if we have the start function function control bit set? Is it an 
> optimization?

No, it's not an optimization. This is the work function that is
scheduled if we get an interrupt for the device. Previously, we only
got an interrupt if either the device presented us an unsolicited
status or if we got an interrupt as a response to the channel program
we submitted. Now, we can get an interrupt for halt/clear subchannel as
well, and in that case, we don't necessarily have a cp.

[Thinking some more about it, we need to verify if the start function
actually remains set if we try to terminate a running channel program
with halt/clear. A clear might scrub too much. If that's the case, we
also need to free the cp if the start function is not set.]