Re: [PATCH v1 1/1] vfio-ccw: Don't free channel programs for unrelated interrupts

[Eric Farman <farman@linux.ibm.com>]

On 1/27/20 7:52 AM, Cornelia Huck wrote:
> On Fri, 24 Jan 2020 11:08:12 -0500
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> On 1/24/20 10:33 AM, Cornelia Huck wrote:
>>> On Fri, 24 Jan 2020 15:54:55 +0100
>>> Eric Farman <farman@linux.ibm.com> wrote:
> 
>>>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>>>> index e401a3d0aa57..a8ab256a217b 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>>>> @@ -90,8 +90,8 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>>>>  	is_final = !(scsw_actl(&irb->scsw) &
>>>>  		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
>>>>  	if (scsw_is_solicited(&irb->scsw)) {
>>>> -		cp_update_scsw(&private->cp, &irb->scsw);
>>>> -		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
>>>> +		if (cp_update_scsw(&private->cp, &irb->scsw) &&
>>>> +		    is_final && private->state == VFIO_CCW_STATE_CP_PENDING)  
>>>
>>> ...but I still wonder why is_final is not catching non-ssch related
>>> interrupts, as I thought it would. We might want to adapt that check,
>>> instead. (Or was the scsw_is_solicited() check supposed to catch that?
>>> As said, too tired right now...)  
>>
>> I had looked at the (un)solicited bits at one point, and saw very few
>> unsolicited interrupts.  The ones that did show up didn't appear to
>> affect things in the way that would cause the problems I'm seeing.
> 
> Ok, so that check is hopefully fine.
> 
>>
>> As for is_final...  That POPS table states that for "status pending
>> [alone] after termination of HALT or CLEAR ... cpa is unpredictable",
>> which is what happens here.  In the example above, the cpa is the same
>> as the previous (successful) interrupt, and thus unrelated to the
>> current chain.  Perhaps is_final needs to check that the function
>> control in the interrupt is for a start?
> 
> I think our reasoning last time we discussed this function was that we
> only are in CP_PENDING if we actually did a ssch previously. Now, if we

I spent a little time looking at the conversations on the patch that
added the CP_PENDING check.  Sadly, those patches hit the list when I
left for holiday so I came late to those discussions and there appears
some loose ends that I should've chased down at the time.  Sorry.

But yes, we should only be in CP_PENDING because of the SSCH, but the
only check of the interrupt here is the "is_final" check, and not that
the interrupt was for a start function.

> do a hsch/csch before we got final status for the program started by
> the ssch, we don't move out of the CP_PENDING, but the cpa still might
> not be what we're looking for. 

As long as we get an interrupt that's "is_final" then don't we come out
of CP_PENDING state at the end of this routine, regardless of whether or
not it does the cp_free() call?  I think your original diagnosis [1] was
that even if the cpa is invalid, calling cp_update_scsw() is okay
because garbage-in-garbage-out.  This patch makes that part of the
criteria for doing the cp_free(), so maybe that's too heavy?  After all,
it does mean that we may leave private->cp "initialized", but reset the
state back to IDLE.  (More on that in a minute.)

> So, we should probably check that we
> have only the start function indicated in the fctl.

For the call to cp_update_scsw() or cp_free()?  Or both?

> 
> But if we do that, we still have a chain allocated for something that
> has already been terminated... how do we find the right chain to clean
> up, if needed?

Don't we free all/none of the chains?  Ideally, since we only have one
set of chains per cp (and thus, per SSCH), they should either all be
freed or ignored.

But regardless, this patch is at least not complete, if not incorrect.
I left a test running for the weekend and while I don't see the storage
damage I saw before, there's a lot of unreleased memory because of stuff
like this:

950.541644 06 ...sch_io_todo sch 09c5: state=3 orb.cpa=7f586f48
                                               irb.w0=00001001
                                               irb.cpa=02e35d58
                                               irb.w2=0000000c
                                               ccw=0
                                               *cda=0
950.541837 06 ...sch_io_todo sch 09c5: state=2 orb.cpa=030ec750
                                               irb.w0=00c04007
                                               irb.cpa=7f586f50
                                               irb.w2=0c000000
                                               ccw=3424000c030ea840
                                               *cda=190757ef0

(I was only tracing instances where vfio-ccw did NOT call cp_free() on
the interrupt path; so I don't have a complete history of what happened.)

The orb.cpa address in the first trace looks like something which came
from the guest, rather than something built by vfio-ccw.  The irb.cpa
address in the second trace is 8 bytes after the first orb.cpa address.
And the storage referenced by both the CP and IDAL referenced in trace 2
are still active when I started poking at the state of things.

There's a lot just to unravel just with this instance.  Like why a guest
CPA is in orb, and thus an irb.  Or why cp_prefetch() checks that
!cp->initialized, but cp_init() does no such thing.  I guess I'll put in
a check to see how that helps this particular situation, while I sort
out the other problems here.

> 

[1] https://lore.kernel.org/kvm/20190702115134.790f8891.cohuck@redhat.com/