Re: [RFC v1 1/1] vfio-ccw: Don't call cp_free if we are processing a channel program

From: Cornelia Huck <cohuck@redhat.com>
To: Farhan Ali <alifm@linux.ibm.com>
Cc: Eric Farman <farman@linux.ibm.com>,
	pasic@linux.ibm.com, linux-s390@vger.kernel.org,
	kvm@vger.kernel.org
Subject: Re: [RFC v1 1/1] vfio-ccw: Don't call cp_free if we are processing a channel program
Date: Mon, 24 Jun 2019 14:07:23 +0200	[thread overview]
Message-ID: <20190624140723.5aa7b0b1.cohuck@redhat.com> (raw)
In-Reply-To: <20190624134622.2bb3bba2.cohuck@redhat.com>

On Mon, 24 Jun 2019 13:46:22 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, 24 Jun 2019 12:05:14 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Mon, 24 Jun 2019 11:42:31 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Fri, 21 Jun 2019 14:34:10 -0400
> > > Farhan Ali <alifm@linux.ibm.com> wrote:
> > >     
> > > > On 06/21/2019 01:40 PM, Eric Farman wrote:    
> > > > > 
> > > > > 
> > > > > On 6/21/19 10:17 AM, Farhan Ali wrote:      
> > > > >>
> > > > >>
> > > > >> On 06/20/2019 04:27 PM, Eric Farman wrote:      
> > > > >>>
> > > > >>>
> > > > >>> On 6/20/19 3:40 PM, Farhan Ali wrote:      
> 
> > > > >>>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> > > > >>>> b/drivers/s390/cio/vfio_ccw_drv.c
> > > > >>>> index 66a66ac..61ece3f 100644
> > > > >>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
> > > > >>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> > > > >>>> @@ -88,7 +88,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct
> > > > >>>> *work)
> > > > >>>>                 (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
> > > > >>>>        if (scsw_is_solicited(&irb->scsw)) {
> > > > >>>>            cp_update_scsw(&private->cp, &irb->scsw);      
> > > > >>>
> > > > >>> As I alluded earlier, do we know this irb is for this cp?  If no, what
> > > > >>> does this function end up putting in the scsw?    
> > > 
> > > Yes, I think this also needs to check whether we have at least a prior
> > > start function around. (We use the orb provided by the guest; maybe we
> > > should check if that intparm is set in the irb?)    
> > 
> > Hrm; not so easy as we always set the intparm to the address of the
> > subchannel structure... 
> > 
> > Maybe check if we have have one of the conditions of the large table
> > 16-6 and correlate to the ccw address? Or is it enough to check the
> > function control? (Don't remember when the hardware resets it.)  
> 
> Nope, we cannot look at the function control, as csch clears any set
> start function bit :( (see "Function Control", pg 16-13)
> 
> I think this problem mostly boils down to "csch clears pending status;
> therefore, we may only get one interrupt, even though there had been a
> start function going on". If we only go with what the hardware gives
> us, I don't see a way to distinguish "clear with a prior start" from
> "clear only". Maybe we want to track an "issued" status in the cp?

Sorry for replying to myself again :), but maybe we should simply call
cp_free() if we got cc 0 from a csch? Any start function has been
terminated at the subchannel during successful execution of csch, and
cp_free does nothing if !cp->initialized, so we should hopefully be
safe there as well. We can then add a check for the start function in
the function control in the check above and should be fine, I think.