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]>

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

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

index b9febc581b1f..8d1b2771c1aa 100644

When an interrupt is received via the IRQ, the bulk of the work is stacked on a workqueue for later processing. Which means that a concurrent START or HALT/CLEAR operation (via the async_region) will race with this process and require some serialization.

Once we have all our locks acquired, let's just look to see if we're in a window where the process has been started from the IRQ, but not yet picked up by vfio-ccw to clean up an I/O. If there is, mark the request as BUSY so it can be redriven.

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

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

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

index 23e61aa638e4..92d638f10b27 100644
@@ -28,6 +28,11 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
 

Quoted 27 lines · On Tue, 13 Apr 2021 20:24:08 +0200 Eric Farman <[email protected]> wrote:

When an interrupt is received via the IRQ, the bulk of the work is stacked on a workqueue for later processing. Which means that a concurrent START or HALT/CLEAR operation (via the async_region) will race with this process and require some serialization.

Once we have all our locks acquired, let's just look to see if we're
in a window where the process has been started from the IRQ, but not
yet picked up by vfio-ccw to clean up an I/O. If there is, mark the request as BUSY so it can be redriven.

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

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

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

index 23e61aa638e4..92d638f10b27 100644
@@ -28,6 +28,11 @@ static int fsm_io_helper(struct vfio_ccw_private
 *private)
 
 	spin_lock_irqsave(sch->lock, flags);
 

Quoted 8 lines

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

+	if (work_pending(&private->io_work)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
 	orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
 	if (!orb) {

ret = -EIO;

I'm wondering what condition we can consider this situation equivalent
to. I'd say that the virtual version of the subchannel is basically status pending already, even though userspace may not have retrieved that information yet; so probably cc 1?

Following the code path further along, it seems we return -EBUSY both for cc 1 and cc 2 conditions we receive from the device (i.e. they are
not distinguishable from userspace).

I don't think we can change that,
as it is an existing API (QEMU maps -EBUSY to cc 2.) So this change looks fine so far.

I'm wondering what we should do for hsch. We probably want to return -EBUSY for a pending condition as well, if I read the PoP correctly...

the only problem is that QEMU seems to match everything to 0; but that
is arguably not the kernel's problem.

For clear, we obviously don't have busy conditions. Should we clean up
any pending conditions?

[It feels like we have discussed this before, but any information has vanished from my cache :/]

Quoted 3 lines · Cornelia Huck

I'm wondering what we should do for hsch. We probably want to return -EBUSY for a pending condition as well, if I read the PoP correctly...

Ah, yes... I agree that to maintain parity with ssch and pops, the same cc1/-EBUSY would be applicable here. Will make that change in next version.

Quoted 5 lines

the only problem is that QEMU seems to match everything to 0; but that
is arguably not the kernel's problem.

Quoted 3 lines

For clear, we obviously don't have busy conditions. Should we clean up
any pending conditions?

By doing anything other than issuing the csch to the subchannel? I don't think so, that should be more than enough to get the css and vfio-ccw in sync with each other.

Quoted 10 lines · On Thu, 15 Apr 2021 09:48:37 -0400 Eric Farman <[email protected]> wrote:

Quoted 4 lines · Cornelia Huck

I'm wondering what we should do for hsch. We probably want to return
-EBUSY for a pending condition as well, if I read the PoP correctly...

Ah, yes... I agree that to maintain parity with ssch and pops, the same cc1/-EBUSY would be applicable here. Will make that change in next
version.

Yes, just to handle things in the same fashion consistently.

Quoted 9 lines

Quoted 8 lines

the only problem is that QEMU seems to match everything to 0; but that
is arguably not the kernel's problem.

For clear, we obviously don't have busy conditions. Should we clean
up
any pending conditions?

Quoted 3 lines

By doing anything other than issuing the csch to the subchannel? I don't think so, that should be more than enough to get the css and vfio-ccw in sync with each other.

Hm, doesn't a successful csch clear any status pending?

That would mean
that invoking our csch backend implies that we won't deliver the status
pending that is already pending via the workqueue, which therefore needs to be flushed out in some way?

I remember we did some special
csch handling, but I don't immediately see where; might have been only
in QEMU.

Quoted 25 lines · Cornelia Huck

Quoted 10 lines · On Thu, 15 Apr 2021 09:48:37 -0400 Eric Farman <[email protected]> wrote:

Quoted 4 lines · Cornelia Huck

I'm wondering what we should do for hsch. We probably want to return
-EBUSY for a pending condition as well, if I read the PoP correctly...

Ah, yes... I agree that to maintain parity with ssch and pops, the same cc1/-EBUSY would be applicable here. Will make that change in next
version.

Yes, just to handle things in the same fashion consistently.

Quoted 9 lines

Quoted 8 lines

the only problem is that QEMU seems to match everything to 0; but that
is arguably not the kernel's problem.

For clear, we obviously don't have busy conditions. Should we clean
up
any pending conditions?

Quoted 5 lines

Quoted 3 lines

By doing anything other than issuing the csch to the subchannel? I don't think so, that should be more than enough to get the css and vfio-ccw in sync with each other.

Hm, doesn't a successful csch clear any status pending?

Yep.

Quoted 5 lines

That would mean
that invoking our csch backend implies that we won't deliver the status
pending that is already pending via the workqueue, which therefore needs to be flushed out in some way?

Ah, so I misunderstood the direction you were going... I'm not aware of a way to "purge" items from a workqueue, as the flush_workqueue() routine is documented as picking them off and running them.

Perhaps an atomic flag in (private? cp?) that causes vfio_ccw_sch_io_todo() to just exit rather than doing all its stuff?

Quoted 6 lines

I remember we did some special
csch handling, but I don't immediately see where; might have been only
in QEMU.

Maybe. I don't see anything jumping out at me though. :(

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]>

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

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()
					fsm_io_helper()
	vfio_ccw_sch_io_todo()
					fsm_irq()
					vfio_ccw_sch_io_todo()

Let's move the reset of the FSM state to the point where the channel_program struct is cleaned up, which is only done for solicited interrupts anyway.

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

---
 drivers/s390/cio/vfio_ccw_drv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/s390/cio/vfio_ccw_drv.c +1 -1

index 8c625b530035..e51318f23ca8 100644
@@ -94,16 +94,15 @@ 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) {

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

-
 	if (private->io_trigger)
 		eventfd_signal(private->io_trigger, 1);
 }

Quoted 56 lines · On Tue, 13 Apr 2021 20:24:10 +0200 Eric Farman <[email protected]> wrote:

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()
					fsm_io_helper()
	vfio_ccw_sch_io_todo()
					fsm_irq()
					vfio_ccw_sch_io_todo()

Let's move the reset of the FSM state to the point where the channel_program struct is cleaned up, which is only done for solicited interrupts anyway.

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

---
 drivers/s390/cio/vfio_ccw_drv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/s390/cio/vfio_ccw_drv.c +3 -1

index 8c625b530035..e51318f23ca8 100644
@@ -94,16 +94,15 @@ 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);
+			private->state = VFIO_CCW_STATE_IDLE;
+		}
 	}
 	mutex_lock(&private->io_mutex);
 	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
 	mutex_unlock(&private->io_mutex);
 

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 105

Quoted 2 lines

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

-	if (private->mdev && is_final)
-		private->state = VFIO_CCW_STATE_IDLE;

Isn't that re-allowing new I/O requests a bit too early?

Maybe remember
that we had a final I/O interrupt for an I/O request and only change the state in this case?

Quoted 6 lines

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

-

drivers/s390/cio/vfio_ccw_drv.c +0 -0

 	if (private->io_trigger)
 		eventfd_signal(private->io_trigger, 1);
 }

Quoted 60 lines · Cornelia Huck

Quoted 57 lines · On Tue, 13 Apr 2021 20:24:10 +0200 Eric Farman <[email protected]> wrote:

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()
					fsm_io_helper()
	vfio_ccw_sch_io_todo()
					fsm_irq()
					vfio_ccw_sch_io_todo()

Let's move the reset of the FSM state to the point where the channel_program struct is cleaned up, which is only done for solicited interrupts anyway.

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

---
 drivers/s390/cio/vfio_ccw_drv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/s390/cio/vfio_ccw_drv.c +3 -1

index 8c625b530035..e51318f23ca8 100644
@@ -94,16 +94,15 @@ 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);
+			private->state = VFIO_CCW_STATE_IDLE;
+		}
 	}
 	mutex_lock(&private->io_mutex);
 	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
 	mutex_unlock(&private->io_mutex);
 

Quoted 4 lines

drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 105

Quoted 2 lines

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

-	if (private->mdev && is_final)
-		private->state = VFIO_CCW_STATE_IDLE;

Isn't that re-allowing new I/O requests a bit too early?

Hrm... I guess I don't see what work vfio-ccw has left to do that is presenting it from carrying on. The copying of the IRB data back into the io_region seems like a flimsy gate to me. But...

It seems you're (rightly) concerned with userspace doing SSCH + SSCH, whereas I'v been focused on the CSCH + SSCH sequence. So with this change, we're inviting the possibility of a second SSCH being able to be submitted/started before the IRB data for the first SSCH is copied (and presumably before userspace is tapped to read that data back).

Sigh... I guess that's not the greatest behavior either. Gotta ruminate
on this.

Quoted 4 lines

Maybe remember
that we had a final I/O interrupt for an I/O request and only change
the state in this case?

As a local flag within this routine? Hrm... I have entirely too many "Let's try this" branches that didn't work, but I don't see that one jumping out at me. Will give it a try.

Quoted 6 lines

Quoted 6 lines

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

-

drivers/s390/cio/vfio_ccw_drv.c +0 -0

 	if (private->io_trigger)
 		eventfd_signal(private->io_trigger, 1);
 }

Hi Conny, Halil,

Let's restart our discussion about the collision between interrupts for START SUBCHANNEL and HALT/CLEAR SUBCHANNEL. It's been a quarter million minutes (give or take), so here is the problematic scenario again:

	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.

In this version...

Patch 1 and 2 are defensive checks. Patch 2 was part of v3 [2], but I would love a better option here to guard between steps 2 and 4.

Patch 3 is a subset of the removal of the CP_PENDING FSM state in v3. I've obviously gone away from this idea, but I thought this piece is still valuable.

Patch 4 collapses the code on the interrupt path so that changes to the FSM state and the channel_program struct are handled at the same point, rather than separated by a mutex boundary. Because of the possibility of a START and HALT/CLEAR running concurrently, it does not make sense to split them here.

With the above patches, maybe it then makes sense to hold the io_mutex across the entirety of vfio_ccw_sch_io_todo(). But I'm not completely sure that would be acceptable.

So... Thoughts?

Quoted 60 lines · On Tue, 13 Apr 2021 20:24:06 +0200 Eric Farman <[email protected]> wrote:

Hi Conny, Halil,

Let's restart our discussion about the collision between interrupts for
START SUBCHANNEL and HALT/CLEAR SUBCHANNEL. It's been a quarter million
minutes (give or take), so here is the problematic scenario again:

	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.

In this version...

Patch 1 and 2 are defensive checks. Patch 2 was part of v3 [2], but I
would love a better option here to guard between steps 2 and 4.

Patch 3 is a subset of the removal of the CP_PENDING FSM state in v3.
I've obviously gone away from this idea, but I thought this piece is
still valuable.

Patch 4 collapses the code on the interrupt path so that changes to the FSM state and the channel_program struct are handled at the same
point, rather than separated by a mutex boundary. Because of the possibility of a START and HALT/CLEAR running concurrently, it does not make sense to split them here.

With the above patches, maybe it then makes sense to hold the io_mutex
across the entirety of vfio_ccw_sch_io_todo(). But I'm not completely
sure that would be acceptable.

Quoted 1 line

So... Thoughts?

I believe we should address

the concurrency, encapsulation and layering
issues in the subchannel/ccw pass-through code (vfio-ccw) by taking a holistic approach as soon as possible.

I find the current state of art very hard to reason about, and that adversely affects my ability to reason about attempts at partial improvements.

I understand that such a holistic approach needs a lot of work, and we
may have to stop some bleeding first. In the stop the bleeding phase we
can take a pragmatic approach and accept changes that empirically seem to
work towards stopping the bleeding. I.e. if your tests say it's better,
I'm willing to accept that it is better.

I have to admit, I don't understand how synchronization is done in the
vfio-ccw kernel module (in the sense of avoiding data races).

Regarding your patches, I have to admit, I have a hard time figuring out
which one of these (or what combination of them) is supposed to solve the problem you described above. If I had to guess, I would guess it is
either patch 4, because it has a similar scenario diagram in the commit message like the one in the problem statement. Is my guess right?

If it is right I don't quite understand the mechanics of the fix, because what the patch seems to do is changing the content of step 4 in
the above diagram. And I don't see how is change that code so that it does not "misidentifies the data in the IRB as being associated with the newly active I/O".

Moreover patch 4 seems to rely on
private->state which, AFAIR is still used in a racy fashion.

But if strong empirical evidence shows that it performs better (stops the bleeding), I think we can go ahead with it.

Regards,
Halil

Quoted 63 lines · Halil Pasic

Quoted 60 lines · On Tue, 13 Apr 2021 20:24:06 +0200 Eric Farman <[email protected]> wrote:

Hi Conny, Halil,

Let's restart our discussion about the collision between interrupts for
START SUBCHANNEL and HALT/CLEAR SUBCHANNEL. It's been a quarter million
minutes (give or take), so here is the problematic scenario again:

	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.

In this version...

Patch 1 and 2 are defensive checks. Patch 2 was part of v3 [2], but I
would love a better option here to guard between steps 2 and 4.

Patch 3 is a subset of the removal of the CP_PENDING FSM state in v3.
I've obviously gone away from this idea, but I thought this piece is
still valuable.

Patch 4 collapses the code on the interrupt path so that changes to the FSM state and the channel_program struct are handled at the same
point, rather than separated by a mutex boundary. Because of the possibility of a START and HALT/CLEAR running concurrently, it does not make sense to split them here.

With the above patches, maybe it then makes sense to hold the io_mutex
across the entirety of vfio_ccw_sch_io_todo(). But I'm not completely
sure that would be acceptable.

Quoted 3 lines

Quoted 1 line

So... Thoughts?

I believe we should address

Who is the "we" here?

Quoted 3 lines

the concurrency, encapsulation and layering
issues in the subchannel/ccw pass-through code (vfio-ccw) by taking a holistic approach as soon as possible.

Quoted 5 lines

I find the current state of art very hard to reason about, and that adversely affects my ability to reason about attempts at partial improvements.

Quoted 9 lines

I understand that such a holistic approach needs a lot of work, and we
may have to stop some bleeding first. In the stop the bleeding phase we
can take a pragmatic approach and accept changes that empirically seem to
work towards stopping the bleeding. I.e. if your tests say it's better,
I'm willing to accept that it is better.

So much bleeding!

RE: my tests... I have only been seeing the described problem in pathological tests, and this series lets those tests run without issue.

Quoted 6 lines

I have to admit, I don't understand how synchronization is done in the
vfio-ccw kernel module (in the sense of avoiding data races).

Quoted 8 lines

Regarding your patches, I have to admit, I have a hard time figuring out
which one of these (or what combination of them) is supposed to solve the problem you described above. If I had to guess, I would guess it is
either patch 4, because it has a similar scenario diagram in the commit message like the one in the problem statement. Is my guess right?

Sort of. It is true that Patch 4 is the last piece of the puzzle, and the diagram is included in that commit message so it is kept with the change, instead of being lost with the cover letter.

As I said in the cover letter, "Patch 1 and 2 are defensive checks" which are simply included to provide a more robust solution. You could argue that Patch 3 should be held out separately, but as it came from the previous version of this series it made sense to include here.

Quoted 7 lines

If it is right I don't quite understand the mechanics of the fix, because what the patch seems to do is changing the content of step 4 in
the above diagram. And I don't see how is change that code so that it does not "misidentifies the data in the IRB as being associated with the newly active I/O".

Consider that the cp_update_scsw() and cp_free() routines that get called here are looking at the cp->initialized flag to determine whether to perform any work. For a system that is otherwise idle, the cp->initialized flag will be false when processing an IRB related to a CSCH, meaning the bulk of this routine will be a NOP.

In the failing scenario, as I describe in the commit message for patch 4, we could be processing an interrupt that is unaffiliated with the CP that was (or is being) built. It need not even be a solicited interrupt; it just happened that the CSCH interrupt is what got me looking at this path. The whole situation boils down to the FSM state and cp->initialized flag being out of sync from one another after coming through this function.

Quoted 6 lines

Moreover patch 4 seems to rely on
private->state which, AFAIR is still used in a racy fashion.

But if strong empirical evidence shows that it performs better (stops the bleeding), I think we can go ahead with it.

Again with the bleeding. Is there a Doctor in the house? :)

Quoted 70 lines · On Thu, 22 Apr 2021 16:49:21 -0400 Eric Farman <[email protected]> wrote:

Quoted 63 lines · Halil Pasic

Quoted 60 lines · On Tue, 13 Apr 2021 20:24:06 +0200 Eric Farman <[email protected]> wrote:

Hi Conny, Halil,

Let's restart our discussion about the collision between interrupts for
START SUBCHANNEL and HALT/CLEAR SUBCHANNEL. It's been a quarter million
minutes (give or take), so here is the problematic scenario again:

	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.

In this version...

Patch 1 and 2 are defensive checks. Patch 2 was part of v3 [2], but I
would love a better option here to guard between steps 2 and 4.

Patch 3 is a subset of the removal of the CP_PENDING FSM state in v3.
I've obviously gone away from this idea, but I thought this piece is
still valuable.

Patch 4 collapses the code on the interrupt path so that changes to the FSM state and the channel_program struct are handled at the same
point, rather than separated by a mutex boundary. Because of the possibility of a START and HALT/CLEAR running concurrently, it does not make sense to split them here.

With the above patches, maybe it then makes sense to hold the io_mutex
across the entirety of vfio_ccw_sch_io_todo(). But I'm not completely
sure that would be acceptable.

Quoted 3 lines

Quoted 1 line

So... Thoughts?

I believe we should address

Who is the "we" here?

Quoted 3 lines

Quoted 3 lines

the concurrency, encapsulation and layering
issues in the subchannel/ccw pass-through code (vfio-ccw) by taking a holistic approach as soon as possible.

Let me also ask: what is "holistic"? If that's a complete rewrite, I definitely don't have the capacity for that; if others want to take over the code, feel free.

Quoted 8 lines

Quoted 6 lines

Moreover patch 4 seems to rely on
private->state which, AFAIR is still used in a racy fashion.

But if strong empirical evidence shows that it performs better (stops the bleeding), I think we can go ahead with it.

Again with the bleeding. Is there a Doctor in the house? :)

No idea, seen any blue boxes around? :)

Quoted 73 lines · On Fri, 23 Apr 2021 13:06:16 +0200 Cornelia Huck <[email protected]> wrote:

Quoted 70 lines · On Thu, 22 Apr 2021 16:49:21 -0400 Eric Farman <[email protected]> wrote:

Quoted 63 lines · Halil Pasic

Quoted 60 lines · On Tue, 13 Apr 2021 20:24:06 +0200 Eric Farman <[email protected]> wrote:

Hi Conny, Halil,

Let's restart our discussion about the collision between interrupts for
START SUBCHANNEL and HALT/CLEAR SUBCHANNEL. It's been a quarter million
minutes (give or take), so here is the problematic scenario again:

	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.

In this version...

Patch 1 and 2 are defensive checks. Patch 2 was part of v3 [2], but I
would love a better option here to guard between steps 2 and 4.

Patch 3 is a subset of the removal of the CP_PENDING FSM state in v3.
I've obviously gone away from this idea, but I thought this piece is
still valuable.

Patch 4 collapses the code on the interrupt path so that changes to the FSM state and the channel_program struct are handled at the same
point, rather than separated by a mutex boundary. Because of the possibility of a START and HALT/CLEAR running concurrently, it does not make sense to split them here.

With the above patches, maybe it then makes sense to hold the io_mutex
across the entirety of vfio_ccw_sch_io_todo(). But I'm not completely
sure that would be acceptable.

Quoted 3 lines

Quoted 1 line

So... Thoughts?

I believe we should address

Who is the "we" here?

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

the concurrency, encapsulation and layering
issues in the subchannel/ccw pass-through code (vfio-ccw) by taking a holistic approach as soon as possible.

Let me also ask: what is "holistic"? If that's a complete rewrite, I definitely don't have the capacity for that; if others want to take over the code, feel free.

In general: https://en.wikipedia.org/wiki/Holism

In this context I mean:
* Fix all data races in in the vfio-ccw module instead of making the "race window" smaller. Reasoning about the behavior of racy programs is very difficult.
* The passed-through subchannel of the VM, as seen by the guest OS is an overlay of the host subchannel (which we have to assume is within the specification), the vfio-ccw kernel module, and an userspace emulator. The interface between the kernel module and the userspace emulator is something the authors of the vfio-ccw kernel module design, and while doing so we have to think about the interface the whole solution needs to implemnet. For example we should ask ourselves: what is the right response in kernel when we encounter the situation described by the steps 1-3 of Eric's scenario. Our VMs subchannel needs to reward the SSC with a cc=2 if the subchannel has the clear FC bit set. If we detect the described condition, does it mean that the userspace emulator is broken? Or is the userspace emulator allowed to rely on the vfio-ccw kernel module to detect this condition and return an -EBUSY (which corresponds to cc=2 because that is apart of the definition of the interface between the kernel and the userspace)? When is the FC bit of our VMs subchannel cleared? I read patch 2 like it is trying to catch the condition and return an -EBUSY, but I don't see it catching all the possible cases. I.e. what if another CPU is executing the first instruction of vfio_ccw_sch_io_todo() when we check work_pending(&private->io_work) in fsm_io_helper()?

[..]

Quoted 9 lines

Quoted 8 lines

Quoted 6 lines

Moreover patch 4 seems to rely on
private->state which, AFAIR is still used in a racy fashion.

But if strong empirical evidence shows that it performs better (stops the bleeding), I think we can go ahead with it.

Again with the bleeding. Is there a Doctor in the house? :)

Quoted 2 lines

No idea, seen any blue boxes around? :)

Let me also ask what: blue boxes do you mean? If you mean
https://en.wikipedia.org/wiki/Blue_box
then, I'm not sure I can follow your association. Are you looking for phone to call a doctor?

Regards,
Halil

Quoted 98 lines · On Fri, 23 Apr 2021 13:06:16 +0200 Cornelia Huck <[email protected]> wrote:

Quoted 94 lines · On Thu, 22 Apr 2021 16:49:21 -0400 Eric Farman <[email protected]> wrote:

Quoted 83 lines · Halil Pasic

Quoted 80 lines · On Tue, 13 Apr 2021 20:24:06 +0200 Eric Farman <[email protected]> wrote:

Hi Conny, Halil,

Let's restart our discussion about the collision between interrupts
for
START SUBCHANNEL and HALT/CLEAR SUBCHANNEL. It's been a quarter
million
minutes (give or take), so here is the problematic scenario again:

	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.

In this version...

Patch 1 and 2 are defensive checks. Patch 2 was part of v3 [2], but
I
would love a better option here to guard between steps 2 and 4.

Patch 3 is a subset of the removal of the CP_PENDING FSM state in
v3.
I've obviously gone away from this idea, but I thought this piece
is
still valuable.

Patch 4 collapses the code on the interrupt path so that changes to
the FSM state and the channel_program struct are handled at the
same
point, rather than separated by a mutex boundary. Because of the
possibility of a START and HALT/CLEAR running concurrently, it does
not make sense to split them here.

With the above patches, maybe it then makes sense to hold the io_mutex
across the entirety of vfio_ccw_sch_io_todo(). But I'm not completely
sure that would be acceptable.

Quoted 3 lines

Quoted 1 line

So... Thoughts?

I believe we should address

Who is the "we" here?

Quoted 4 lines

the concurrency, encapsulation and layering
issues in the subchannel/ccw pass-through code (vfio-ccw) by taking a
holistic approach as soon as possible.

Quoted 5 lines

Let me also ask: what is "holistic"? If that's a complete rewrite, I
definitely don't have the capacity for that; if others want to take over the code, feel free.

In general: https://en.wikipedia.org/wiki/Holism

In this context I mean:
* Fix all data races in in the vfio-ccw module instead of making the "race window" smaller. Reasoning about the behavior of racy programs is very difficult.
* The passed-through subchannel of the VM, as seen by the guest OS is an
overlay of the host subchannel (which we have to assume is within the specification), the vfio-ccw kernel module, and an userspace emulator.
The interface between the kernel module and the userspace emulator is something the authors of the vfio-ccw kernel module design, and while doing so we have to think about the interface the whole solution needs
to implemnet. For example we should ask ourselves: what is the right response in kernel when we encounter the situation described by the steps 1-3 of Eric's scenario. Our VMs subchannel needs to reward the SSC with a cc=2 if the subchannel has the clear FC bit set. If we detect
the described condition, does it mean that the userspace emulator is broken? Or is the userspace emulator allowed to rely on the vfio-ccw kernel module to detect this condition and return an -EBUSY (which corresponds to cc=2 because that is apart of the definition of the interface between the kernel and the userspace)? When is the FC bit of our VMs subchannel cleared?

I read patch 2 like it is trying to catch
the condition and return an -EBUSY, but I don't see it catching all the possible cases. I.e. what if another CPU is executing the first instruction of vfio_ccw_sch_io_todo() when we check work_pending(&private->io_work) in fsm_io_helper()?

[..]

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Moreover patch 4 seems to rely on
private->state which, AFAIR is still used in a racy fashion.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

But if strong empirical evidence shows that it performs better (stops
the bleeding), I think we can go ahead with it.

Again with the bleeding. Is there a Doctor in the house? :)

No idea, seen any blue boxes around? :)

Let me also ask what: blue boxes do you mean? If you mean
https://en.wikipedia.org/wiki/Blue_box
then, I'm not sure I can follow your association. Are you looking for phone to call a doctor?

Regards,
Halil

Quoted 63 lines · Halil Pasic

Quoted 60 lines · On Tue, 13 Apr 2021 20:24:06 +0200 Eric Farman <[email protected]> wrote:

Hi Conny, Halil,

Let's restart our discussion about the collision between interrupts for
START SUBCHANNEL and HALT/CLEAR SUBCHANNEL. It's been a quarter million
minutes (give or take), so here is the problematic scenario again:

	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.

In this version...

Patch 1 and 2 are defensive checks. Patch 2 was part of v3 [2], but I
would love a better option here to guard between steps 2 and 4.

Patch 3 is a subset of the removal of the CP_PENDING FSM state in v3.
I've obviously gone away from this idea, but I thought this piece is
still valuable.

Patch 4 collapses the code on the interrupt path so that changes to the FSM state and the channel_program struct are handled at the same
point, rather than separated by a mutex boundary. Because of the possibility of a START and HALT/CLEAR running concurrently, it does not make sense to split them here.

With the above patches, maybe it then makes sense to hold the io_mutex
across the entirety of vfio_ccw_sch_io_todo(). But I'm not completely
sure that would be acceptable.

Quoted 3 lines

Quoted 1 line

So... Thoughts?

I believe we should address

Who is the "we" here?

Quoted 8 lines

the concurrency, encapsulation and layering
issues in the subchannel/ccw pass-through code (vfio-ccw) by taking a holistic approach as soon as possible.

I find the current state of art very hard to reason about, and that adversely affects my ability to reason about attempts at partial improvements.

Quoted 9 lines

I understand that such a holistic approach needs a lot of work, and we
may have to stop some bleeding first. In the stop the bleeding phase we
can take a pragmatic approach and accept changes that empirically seem to
work towards stopping the bleeding. I.e. if your tests say it's better,
I'm willing to accept that it is better.

So much bleeding!

RE: my tests... I have only been seeing the described problem in pathological tests, and this series lets those tests run without issue.

Quoted 5 lines

I have to admit, I don't understand how synchronization is done in the
vfio-ccw kernel module (in the sense of avoiding data races).

Quoted 8 lines

Regarding your patches, I have to admit, I have a hard time figuring out
which one of these (or what combination of them) is supposed to solve the problem you described above. If I had to guess, I would guess it is
either patch 4, because it has a similar scenario diagram in the commit message like the one in the problem statement. Is my guess right?

Sort of. It is true that Patch 4 is the last piece of the puzzle, and the diagram is included in that commit message so it is kept with the change, instead of being lost with the cover letter.

As I said in the cover letter, "Patch 1 and 2 are defensive checks" which are simply included to provide a more robust solution. You could argue that Patch 3 should be held out separately, but as it came from the previous version of this series it made sense to include here.

Quoted 7 lines

If it is right I don't quite understand the mechanics of the fix, because what the patch seems to do is changing the content of step 4 in
the above diagram. And I don't see how is change that code so that it does not "misidentifies the data in the IRB as being associated with the newly active I/O".

Consider that the cp_update_scsw() and cp_free() routines that get called here are looking at the cp->initialized flag to determine whether to perform any work. For a system that is otherwise idle, the cp->initialized flag will be false when processing an IRB related to a CSCH, meaning the bulk of this routine will be a NOP.

In the failing scenario, as I describe in the commit message for patch 4, we could be processing an interrupt that is unaffiliated with the CP that was (or is being) built. It need not even be a solicited interrupt; it just happened that the CSCH interrupt is what got me looking at this path. The whole situation boils down to the FSM state and cp->initialized flag being out of sync from one another after coming through this function.

Quoted 5 lines

Moreover patch 4 seems to rely on
private->state which, AFAIR is still used in a racy fashion.

But if strong empirical evidence shows that it performs better (stops the bleeding), I think we can go ahead with it.

Again with the bleeding. Is there a Doctor in the house? :)

Quoted 77 lines · On Thu, 22 Apr 2021 16:49:21 -0400 Eric Farman <[email protected]> wrote:

Quoted 76 lines · Halil Pasic

Quoted 73 lines · On Tue, 13 Apr 2021 20:24:06 +0200 Eric Farman <[email protected]> wrote:

Hi Conny, Halil,

Let's restart our discussion about the collision between interrupts
for
START SUBCHANNEL and HALT/CLEAR SUBCHANNEL. It's been a quarter million
minutes (give or take), so here is the problematic scenario again:

	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.

In this version...

Patch 1 and 2 are defensive checks. Patch 2 was part of v3 [2], but
I
would love a better option here to guard between steps 2 and 4.

Patch 3 is a subset of the removal of the CP_PENDING FSM state in
v3.
I've obviously gone away from this idea, but I thought this piece
is
still valuable.

Patch 4 collapses the code on the interrupt path so that changes to
the FSM state and the channel_program struct are handled at the same
point, rather than separated by a mutex boundary. Because of the
possibility of a START and HALT/CLEAR running concurrently, it does
not make sense to split them here.

With the above patches, maybe it then makes sense to hold the io_mutex
across the entirety of vfio_ccw_sch_io_todo(). But I'm not completely
sure that would be acceptable.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

So... Thoughts?

I believe we should address

Who is the "we" here?

The people that are responsible for vfio-ccw.

Quoted 22 lines

Quoted 10 lines

the concurrency, encapsulation and layering
issues in the subchannel/ccw pass-through code (vfio-ccw) by taking a
holistic approach as soon as possible.

I find the current state of art very hard to reason about, and that
adversely affects my ability to reason about attempts at partial improvements.

Quoted 11 lines

I understand that such a holistic approach needs a lot of work, and
we
may have to stop some bleeding first. In the stop the bleeding phase
we
can take a pragmatic approach and accept changes that empirically seem to
work towards stopping the bleeding. I.e. if your tests say it's better,
I'm willing to accept that it is better.

Quoted 6 lines

So much bleeding!

RE: my tests... I have only been seeing the described problem in pathological tests, and this series lets those tests run without issue.

Good to know.

Quoted 23 lines

Quoted 5 lines

I have to admit, I don't understand how synchronization is done in
the
vfio-ccw kernel module (in the sense of avoiding data races).

Quoted 11 lines

Regarding your patches, I have to admit, I have a hard time figuring
out
which one of these (or what combination of them) is supposed to solve
the problem you described above. If I had to guess, I would guess it
is
either patch 4, because it has a similar scenario diagram in the commit message like the one in the problem statement. Is my guess right?

Sort of. It is true that Patch 4 is the last piece of the puzzle, and
the diagram is included in that commit message so it is kept with the
change, instead of being lost with the cover letter.

Quoted 7 lines

As I said in the cover letter, "Patch 1 and 2 are defensive checks" which are simply included to provide a more robust solution. You could
argue that Patch 3 should be held out separately, but as it came from
the previous version of this series it made sense to include here.

Does that mean we need patches 1, 2 and 4 to fix the issue or is just 4 sufficient?

Quoted 17 lines

Quoted 8 lines

If it is right I don't quite understand the mechanics of the fix, because what the patch seems to do is changing the content of step 4
in
the above diagram. And I don't see how is change that code so that it does not "misidentifies the data in the IRB as being associated with the newly active I/O".

Consider that the cp_update_scsw() and cp_free() routines that get called here are looking at the cp->initialized flag to determine whether to perform any work. For a system that is otherwise idle, the
cp->initialized flag will be false when processing an IRB related to a
CSCH, meaning the bulk of this routine will be a NOP.

Quoted 11 lines

In the failing scenario, as I describe in the commit message for patch
4, we could be processing an interrupt that is unaffiliated with the CP
that was (or is being) built. It need not even be a solicited interrupt; it just happened that the CSCH interrupt is what got me looking at this path. The whole situation boils down to the FSM state
and cp->initialized flag being out of sync from one another after coming through this function.

Thanks for the explanation. Since you are about to send out a new verison which I understand won't be just about cosmetic fixes, I won't
invest any more in understanding this one. But I hope this will help me
understand that one.

Quoted 7 lines

Quoted 6 lines

Moreover patch 4 seems to rely on
private->state which, AFAIR is still used in a racy fashion.

But if strong empirical evidence shows that it performs better (stops
the bleeding), I think we can go ahead with it.

Quoted 2 lines

Again with the bleeding. Is there a Doctor in the house? :)

Sorry if I expressed myself comically. Was not my intention. I'm puzzled.

Is in your opinion the vfio-ccw kernel module data race free with this
series applied?

Regards,
Halil

Quoted 4 lines

Is in your opinion the vfio-ccw kernel module data race free with this
series applied?

I have no further concerns.

Quoted 5 lines · On Fri, 23 Apr 2021 11:53:06 -0400 Eric Farman <[email protected]> wrote:

Quoted 3 lines

Is in your opinion the vfio-ccw kernel module data race free with this
series applied?

I have no further concerns.

I take this for a "yes, in my opinion it is data race free".

Please explain me, how do we synchronize access to
a) private->state
b) cp->initialized?

Asuming we both use the definition from the C standard, the multiple threads potentially concurrently accessing private->state or cp->initialized are not hard to find.

For the former you can take vfio_ccw_sch_io_todo() (has both a read and a write, and runs from the workqueue) and fsm_io_request() (has both read and write). I guess the accesses from fsm_io_request() are all with the io_mutex held, but the accesses form vfio_ccw_sch_io_todo()
are not with the io_mutex held AFAICT.

And the same is true for the latter with the difference that vfio_ccw_sch_io_todo() manipulates cp->initialized via cp_update_scsw()
and cp_free().

These are either data races, or I'm missing something. Let me also note that C programs data races are bad news, because of undefined behavior.

Regards,
Halil

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Is in your opinion the vfio-ccw kernel module data race free with this
series applied?

I have no further concerns.

I take this for a "yes, in my opinion it is data race free".

You asked about once this series is applied, which it is not.