vfio-ccw: Fix interrupt handling for HALT/CLEAR

We have a really nice flag in the channel_program struct that indicates if it had been initialized by cp_init(), and use it as a guard in the other cp accessor routines, but not for a duplicate call into cp_init(). The possibility of this occurring is low, because that flow is protected by the private->io_mutex and FSM CP_PROCESSING state. But then why bother checking it in (for example) cp_prefetch() then?

Let's just be consistent and check for that in cp_init() too.

Fixes 71189f263f8a3 ("vfio-ccw: make it safe to access channel programs")

Signed-off-by Eric Farman <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
  drivers/s390/cio/vfio_ccw_cp.c | 4 ++++
  1 file changed, 4 insertions(+)

drivers/s390/cio/vfio_ccw_cp.c +4 -0

index b9febc581b1f..8d1b2771c1aa 100644
@@ -638,6 +638,10 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
  	static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 1);
  	int ret;
 
+	/* this is an error in the caller */
+	if (cp->initialized)
+		return -EBUSY;
+
  	/*
  	 * We only support prefetching the channel program. We assume all channel
  	 * programs executed by supported guests likewise support prefetching.

When an I/O request is made, the fsm_io_request() routine moves the FSM state from IDLE to CP_PROCESSING, and then fsm_io_helper() moves it to CP_PENDING if the START SUBCHANNEL received a cc0. Yet, the error case to go from CP_PROCESSING back to IDLE is done after the FSM call returns.

Let's move this up into the FSM proper, to provide some better symmetry when unwinding in this case.

Signed-off-by Eric Farman <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
  drivers/s390/cio/vfio_ccw_fsm.c | 1 +
  drivers/s390/cio/vfio_ccw_ops.c | 2 --
  2 files changed, 1 insertion(+), 2 deletions(-)

drivers/s390/cio/vfio_ccw_fsm.c +1 -0

index 23e61aa638e4..e435a9cd92da 100644
@@ -318,6 +318,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,
  	}
 
  err_out:
+	private->state = VFIO_CCW_STATE_IDLE;
  	trace_vfio_ccw_fsm_io_request(scsw->cmd.fctl, schid,
  				      io_region->ret_code, errstr);
  }

drivers/s390/cio/vfio_ccw_ops.c +0 -2

index 767ac41686fe..5971641964c6 100644
@@ -276,8 +276,6 @@ static ssize_t vfio_ccw_mdev_write_io_region(struct vfio_ccw_private *private,
  	}
 
  	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_IO_REQ);
-	if (region->ret_code != 0)
-		private->state = VFIO_CCW_STATE_IDLE;
  	ret = (region->ret_code != 0) ? region->ret_code : count;
 
  out_unlock:

Today, the stacked call to vfio_ccw_sch_io_todo() does three things:

1) Update a solicited IRB with CP information, and release the CP if the interrupt was the end of a START operation.
2) Copy the IRB data into the io_region, under the protection of the io_mutex
3) Reset the vfio-ccw FSM state to IDLE to acknowledge that vfio-ccw can accept more work.

The trouble is that step 3 is (A) invoked for both solicited and unsolicited interrupts, and (B) sitting after the mutex for step 2. This second piece becomes a problem if it processes an interrupt for a CLEAR SUBCHANNEL while another thread initiates a START, thus allowing the CP and FSM states to get out of sync. That is:

     CPU 1                           CPU 2
     fsm_do_clear()
     fsm_irq()
                                     fsm_io_request()
     vfio_ccw_sch_io_todo()
                                     fsm_io_helper()

Since the FSM state and CP should be kept in sync, let's make a note when the CP is released, and rely on that as an indication that the FSM should also be reset at the end of this routine and open up the device for more work.

Signed-off-by Eric Farman <[email protected]>

---
  drivers/s390/cio/vfio_ccw_drv.c | 12 ++++++++++--
  1 file changed, 10 insertions(+), 2 deletions(-)

drivers/s390/cio/vfio_ccw_drv.c +10 -2

index 8c625b530035..9b61e9b131ad 100644
@@ -86,6 +86,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
  	struct vfio_ccw_private *private;
  	struct irb *irb;
  	bool is_final;
+	bool cp_is_finished = false;
 
  	private = container_of(work, struct vfio_ccw_private, io_work);
  	irb = &private->irb;

@@ -94,14 +95,21 @@ 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);
-		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
+		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
  			cp_free(&private->cp);
+			cp_is_finished = true;
+		}
  	}
  	mutex_lock(&private->io_mutex);
  	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
  	mutex_unlock(&private->io_mutex);
 
-	if (private->mdev && is_final)
+	/*
+	 * Reset to IDLE only if processing of a channel program
+	 * has finished. Do not overwrite a possible processing
+	 * state if the final interrupt was for HSCH or CSCH.
+	 */
+	if (private->mdev && cp_is_finished)
  		private->state = VFIO_CCW_STATE_IDLE;
 
  	if (private->io_trigger)

Today, the stacked call to vfio_ccw_sch_io_todo() does three things:

1) Update a solicited IRB with CP information, and release the CP if the interrupt was the end of a START operation.
2) Copy the IRB data into the io_region, under the protection of the io_mutex
3) Reset the vfio-ccw FSM state to IDLE to acknowledge that vfio-ccw can accept more work.

The trouble is that step 3 is (A) invoked for both solicited and unsolicited interrupts, and (B) sitting after the mutex for step 2. This second piece becomes a problem if it processes an interrupt for a CLEAR SUBCHANNEL while another thread initiates a START, thus allowing the CP and FSM states to get out of sync. That is:

    CPU 1                           CPU 2
    fsm_do_clear()
    fsm_irq()
                                    fsm_io_request()
    vfio_ccw_sch_io_todo()
                                    fsm_io_helper()

Since the FSM state and CP should be kept in sync, let's make a note when the CP is released, and rely on that as an indication that the FSM should also be reset at the end of this routine and open up the device for more work.

Signed-off-by Eric Farman <[email protected]>

---
 drivers/s390/cio/vfio_ccw_drv.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Hi Conny, Matt, Halil,

Here's one (last?) update to my proposal for handling the collision between interrupts for START SUBCHANNEL and HALT/CLEAR SUBCHANNEL.

Only change here is to include Conny's suggestions on patch 3.

Thanks,

Quoted 8 lines · On Tue, 11 May 2021 21:56:28 +0200 Eric Farman <[email protected]> wrote:

Hi Conny, Matt, Halil,

Here's one (last?) update to my proposal for handling the collision between interrupts for START SUBCHANNEL and HALT/CLEAR SUBCHANNEL.

Only change here is to include Conny's suggestions on patch 3.

Thanks,

I believe these changes are beneficial, although I don't understand everything about them. In that sense I'm happy with the these getting merged.

Let me also spend some words answering the unasked question, what I'm not understanding about these.

Not understanding how the problem stated in the cover letter of v4 is actually resolved is certainly the most important one.

Let me cite
the relevant part of it (your cover letter already contains a link to the full version).

"""

	CPU 1			CPU 2

1 CLEAR SUBCHANNEL
2 fsm_irq()
3 START SUBCHANNEL
4 vfio_ccw_sch_io_todo()
5 fsm_irq()
6 vfio_ccw_sch_io_todo()

From the channel subsystem's point of view the CLEAR SUBCHANNEL (step
1)
is complete once step 2 is called, as the Interrupt Response Block (IRB)
has been presented and the TEST SUBCHANNEL was driven by the cio layer.
Thus, the START SUBCHANNEL (step 3) is submitted [1] and gets a cc=0 to
indicate the I/O was accepted. However, step 2 stacks the bulk of the actual work onto a workqueue for when the subchannel lock is NOT held,
and is unqueued at step 4. That code misidentifies the data in the IRB
as being associated with the newly active I/O, and may release memory that is actively in use by the channel subsystem and/or device. Eww. """

The last sentence clearly states "may release memory that is actively used by ... the device", and I understood it refers to the invocation of cp_free() from vfio_ccw_sch_io_todo(). Patch 3 of this series does not change the conditions under which cp_free() is called.

Looking at the cited diagram, since patch 3 changes things in vfio_ccw_sch_io_todo() it probably ain't affecting steps 1-3 and I understood the description so that bad free happens in step 4.

My guess is that your change from patch 3 somehow via the fsm prevents
the SSCH on CPU 2 (using the diagram) from being executed if it actually
happens to be after vfio_ccw_sch_io_todo().

And patch 1 is supposed to
prevent the SSCH on CPU2 from being executed in the depicted case because
if there is a cp to free, then we would bail out form if we see it while processing the new IO request.

In any case, I don't want to hold this up any further.

Regards,
Halil

Quoted 3 lines

In any case, I don't want to hold this up any further.

Thanks for that. You are correct that there's still a potential issue here, in the handoff between fsm_irq() and vfio_ccw_sch_io_todo(), and another fsm_io_request() that would arrive between those points. But it's not anything that we haven't already discussed, and will hopefully begin discussing in the next couple of weeks.

Hi Conny, Matt, Halil,

Here's one (last?) update to my proposal for handling the collision between interrupts for START SUBCHANNEL and HALT/CLEAR SUBCHANNEL.

Only change here is to include Conny's suggestions on patch 3.

Thanks,
Eric

Changelog:
v5->v6:
- Add a block comment and rename variable in patch 3 [CH]
- Drop RFC tag [EF]

v4->v5:
- Applied Conny's r-b to patches 1 and 3
- Dropped patch 2 and 4
- Use a "finished" flag in the interrupt completion path

Previous versions:
v5: https://lore.kernel.org/kvm/[email protected]/
v4: https://lore.kernel.org/kvm/[email protected]/
v3: https://lore.kernel.org/kvm/[email protected]/
v2: https://lore.kernel.org/kvm/[email protected]/
v1: https://lore.kernel.org/kvm/[email protected]/

Eric Farman (3):
vfio-ccw: Check initialized flag in cp_init()
vfio-ccw: Reset FSM state to IDLE inside FSM
vfio-ccw: Serialize FSM IDLE state with I/O completion

 drivers/s390/cio/vfio_ccw_cp.c  |  4 ++++
 drivers/s390/cio/vfio_ccw_drv.c | 12 ++++++++++--
 drivers/s390/cio/vfio_ccw_fsm.c |  1 +
 drivers/s390/cio/vfio_ccw_ops.c |  2 --
 4 files changed, 15 insertions(+), 4 deletions(-)