[PULL 0/3] vfio-ccw: some fixes

[PULL 0/3] vfio-ccw: some fixes

[Cornelia Huck]

The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5:

  Linux 5.13-rc1 (2021-05-09 14:17:44 -0700)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git tags/vfio-ccw-20210520

for you to fetch changes up to 2af7a834a435460d546f0cf0a8b8e4d259f1d910:

  vfio-ccw: Serialize FSM IDLE state with I/O completion (2021-05-12 12:59:50 +0200)

----------------------------------------------------------------
Avoid some races in vfio-ccw request handling.

----------------------------------------------------------------

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(-)

-- 
2.31.1

[PULL 1/3] vfio-ccw: Check initialized flag in cp_init()

[Cornelia Huck]

From: Eric Farman <farman@linux.ibm.com>

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 <farman@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Acked-by: Matthew Rosato <mjrosato@linux.ibm.com>
Message-Id: <20210511195631.3995081-2-farman@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index b9febc581b1f..8d1b2771c1aa 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -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.
-- 
2.31.1

[PULL 2/3] vfio-ccw: Reset FSM state to IDLE inside FSM

[Cornelia Huck]

From: Eric Farman <farman@linux.ibm.com>

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 <farman@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Acked-by: Matthew Rosato <mjrosato@linux.ibm.com>
Message-Id: <20210511195631.3995081-3-farman@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 drivers/s390/cio/vfio_ccw_fsm.c | 1 +
 drivers/s390/cio/vfio_ccw_ops.c | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 23e61aa638e4..e435a9cd92da 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -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);
 }
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 491a64c61fff..c57d2a7f0919 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -279,8 +279,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:
-- 
2.31.1

[PULL 3/3] vfio-ccw: Serialize FSM IDLE state with I/O completion

[Cornelia Huck]

From: Eric Farman <farman@linux.ibm.com>

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 <farman@linux.ibm.com>
Acked-by: Matthew Rosato <mjrosato@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Message-Id: <20210511195631.3995081-4-farman@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 8c625b530035..9b61e9b131ad 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -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)
-- 
2.31.1

Re: [PULL 0/3] vfio-ccw: some fixes

[Vasily Gorbik]

On Thu, May 20, 2021 at 01:34:47PM +0200, Cornelia Huck wrote:
> The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5:
> 
>   Linux 5.13-rc1 (2021-05-09 14:17:44 -0700)
> 
> are available in the Git repository at:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git tags/vfio-ccw-20210520
> 
> for you to fetch changes up to 2af7a834a435460d546f0cf0a8b8e4d259f1d910:
> 
>   vfio-ccw: Serialize FSM IDLE state with I/O completion (2021-05-12 12:59:50 +0200)
> 
> ----------------------------------------------------------------
> Avoid some races in vfio-ccw request handling.
> 
> ----------------------------------------------------------------
> 
> 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

Pulled into fixes, thanks.

BTW, linux-s390@vger.kernel.org is now archived on lore and we started
using b4 (https://git.kernel.org/pub/scm/utils/b4/b4.git) to pick up
changes. Besides all other features, it can convert Message-Id: to Link:

Hm, and b4 also now complains:
✗ BADSIG: DKIM/ibm.com
have to look into that...

b4 usage (was: [PULL 0/3] vfio-ccw: some fixes)

[Cornelia Huck]

On Thu, 27 May 2021 00:33:00 +0200
Vasily Gorbik <gor@linux.ibm.com> wrote:

> On Thu, May 20, 2021 at 01:34:47PM +0200, Cornelia Huck wrote:
> > The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5:
> > 
> >   Linux 5.13-rc1 (2021-05-09 14:17:44 -0700)
> > 
> > are available in the Git repository at:
> > 
> >   https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/vfio-ccw.git tags/vfio-ccw-20210520
> > 
> > for you to fetch changes up to 2af7a834a435460d546f0cf0a8b8e4d259f1d910:
> > 
> >   vfio-ccw: Serialize FSM IDLE state with I/O completion (2021-05-12 12:59:50 +0200)
> > 
> > ----------------------------------------------------------------
> > Avoid some races in vfio-ccw request handling.
> > 
> > ----------------------------------------------------------------
> > 
> > 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  
> 
> Pulled into fixes, thanks.
> 
> BTW, linux-s390@vger.kernel.org is now archived on lore and we started

Oh, nice.

> using b4 (https://git.kernel.org/pub/scm/utils/b4/b4.git) to pick up
> changes. Besides all other features, it can convert Message-Id: to Link:

I've been using b4 to pick patches (Linux and especially QEMU) for
quite some time now, but never felt the need to convert Message-Id: to
Link:. If you prefer the Link: format, I can certainly start using that
for kernel patches.

> 
> Hm, and b4 also now complains:
> ✗ BADSIG: DKIM/ibm.com
> have to look into that...
> 

Re: b4 usage (was: [PULL 0/3] vfio-ccw: some fixes)

[Vasily Gorbik]

> > BTW, linux-s390@vger.kernel.org is now archived on lore and we started
> 
> Oh, nice.
> 
> > using b4 (https://git.kernel.org/pub/scm/utils/b4/b4.git) to pick up
> > changes. Besides all other features, it can convert Message-Id: to Link:
> 
> I've been using b4 to pick patches (Linux and especially QEMU) for
> quite some time now, but never felt the need to convert Message-Id: to
> Link:. If you prefer the Link: format, I can certainly start using that
> for kernel patches.

It's up to you, whatever you prefer. Just wanted to point out that this is
possible now. I personally just follow what seems to be a more common practice.

$ git log --grep=Link:.*lore --oneline linux/master | wc -l
53522
$ git log --grep=Message-Id: --oneline linux/master | wc -l
1666


> > 
> > Hm, and b4 also now complains:
> > ✗ BADSIG: DKIM/ibm.com
> > have to look into that...

Ok, figured out that this is only failing with the company vpn. Internal
dns servers handle ibm.com domain, but fail to serve public key via
"pp1._domainkey.ibm.com" request, which is required to verify DKIM.
All works fine with external dns.

Re: b4 usage

[Cornelia Huck]

On Thu, 27 May 2021 17:25:50 +0200
Vasily Gorbik <gor@linux.ibm.com> wrote:

> > > BTW, linux-s390@vger.kernel.org is now archived on lore and we started  
> > 
> > Oh, nice.
> >   
> > > using b4 (https://git.kernel.org/pub/scm/utils/b4/b4.git) to pick up
> > > changes. Besides all other features, it can convert Message-Id: to Link:  
> > 
> > I've been using b4 to pick patches (Linux and especially QEMU) for
> > quite some time now, but never felt the need to convert Message-Id: to
> > Link:. If you prefer the Link: format, I can certainly start using that
> > for kernel patches.  
> 
> It's up to you, whatever you prefer. Just wanted to point out that this is
> possible now. I personally just follow what seems to be a more common practice.
> 
> $ git log --grep=Link:.*lore --oneline linux/master | wc -l
> 53522
> $ git log --grep=Message-Id: --oneline linux/master | wc -l
> 1666

For QEMU, it's the other way around :)

But I'll try to use Link: for kernel patches, unless I forget.