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

[Cornelia Huck]

On Wed, 28 Nov 2018 09:31:51 -0500
Farhan Ali <address@hidden> wrote:

> On 11/28/2018 04:02 AM, Cornelia Huck wrote:
> > 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.]
> > 
> >   
> 
> According to PoPs (Chapter 16: I/O interruptions, under function control):
> 
> The start-function indication is also cleared at
> the subchannel during the execution of CLEAR SUB-
> CHANNEL.
> 
> So maybe we do need to free the cp.

Hm... so we need to make sure that cp_update_scsw() and cp_free() only
do something when there's actually a valid cp around and call them
unconditionally. Maybe add a ->valid flag to struct channel_program?