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

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 24 Jun 2019 10:44:17 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 06/24/2019 08:07 AM, Cornelia Huck wrote:
> > 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.
> > 
> >   
> 
> So you mean not call cp_free in vfio_ccw_sch_io_todo, and instead call 
> cp_free for a cc=0 for csch (and hsch) ?
> 
> Won't we end up with memory leak for a successful for ssch then?

No; both:

- free if cc=0 for csch (as this clears the status; hsch doesn't)
- free in _todo if the start function is set in the irb and the status
  is final

> 
> But even if we don't remove the cp_free from vfio_ccw_sch_io_todo, I am 
> not sure if your suggestion will fix the problem. The problem here is 
> that we can call vfio_ccw_sch_io_todo (for a clear or halt interrupt) at 
> the same time we are handling an ssch request. So depending on the order 
> of the operations we could still end up calling cp_free from both from 
> threads (i refer to the threads I mentioned in response to Eric's 
> earlier email).

What I don't see is why this is a problem with ->initialized; wasn't
the problem that we misinterpreted an interrupt for csch as one for a
not-yet-issued ssch?

> 
> Another thing that concerns me is that vfio-ccw can also issue csch/hsch 
> in the quiesce path, independently of what the guest issues. So in that 
> case we could have a similar scenario to processing an ssch request and 
> issuing halt/clear in parallel. But maybe I am being paranoid :)

I think the root problem is really trying to clear a cp while another
thread is trying to set it up. Should we maybe use something like rcu?