s390: vfio-ccw fixes

Per the POPs [1], when processing an interrupt the SCSW.CPA field of an IRB generally points to 8 bytes after the last CCW that was executed (there are exceptions, but this is the most common behavior).

In the case of an error, this points us to the first un-executed CCW in the chain. But in the case of normal I/O, the address points beyond the end of the chain. While the guest generally only cares about this when possibly restarting a channel program after error recovery, we should convert the address even in the good scenario so that we provide a consistent, valid, response upon I/O completion.

[1] Figure 16-6 in SA22-7832-11. The footnotes in that table also state that this is true even if the resulting address is invalid or protected, but moving to the end of the guest chain should not be a surprise.

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

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

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

index 384b3987eeb4..41f48b8790bc 100644
@@ -870,7 +870,11 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
  	 */
  	list_for_each_entry(chain, &cp->ccwchain_list, next) {

Otherwise, the guest can believe it's okay to start another I/O and bump into the non-idle state. This results in a cc=2 (with the asynchronous CSCH/HSCH code) returned to the guest, which is unfortunate since everything is otherwise working normally.

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

Reviewed-by Pierre Morel <[email protected]>

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

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

index 0b3b9de45c60..ddd21b6149fd 100644
@@ -86,11 +86,11 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
  	}
  	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
 
-	if (private->io_trigger)

The pfn_array_alloc_pin routine is doing too much. Today, it does the alloc of the pfn_array struct and its member arrays, builds the iova address lists out of a contiguous piece of guest memory, and asks vfio to pin the resulting pages.

Let's effectively revert a significant portion of commit 5c1cfb1c3948 ("vfio: ccw: refactor and improve pfn_array_alloc_pin()") such that we break pfn_array_alloc_pin() into its component pieces, and have one routine that allocates/populates the pfn_array structs, and another that actually pins the memory. In the future, we will be able to handle scenarios where pinning memory isn't actually appropriate.

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

---
  drivers/s390/cio/vfio_ccw_cp.c | 64 ++++++++++++++++++++++++----------
  1 file changed, 46 insertions(+), 18 deletions(-)

drivers/s390/cio/vfio_ccw_cp.c +42 -16

index 41f48b8790bc..60aa784717c5 100644
@@ -50,28 +50,25 @@ struct ccwchain {
  };
 
  /*
- * pfn_array_alloc_pin() - alloc memory for PFNs, then pin user pages in memory
+ * pfn_array_alloc() - alloc memory for PFNs
   * @pa: pfn_array on which to perform the operation
- * @mdev: the mediated device to perform pin/unpin operations
   * @iova: target guest physical address
   * @len: number of bytes that should be pinned from @iova
   *
- * Attempt to allocate memory for PFNs, and pin user pages in memory.
+ * Attempt to allocate memory for PFNs.
   *
   * Usage of pfn_array:
   * We expect (pa_nr == 0) and (pa_iova_pfn == NULL), any field in
   * this structure will be filled in by this function.
   *
   * Returns:
- *   Number of pages pinned on success.
- *   If @pa->pa_nr is not 0, or @pa->pa_iova_pfn is not NULL initially,
- *   returns -EINVAL.
- *   If no pages were pinned, returns -errno.
+ *         0 if PFNs are allocated
+ *   -EINVAL if pa->pa_nr is not initially zero, or pa->pa_iova_pfn is not NULL
+ *   -ENOMEM if alloc failed
   */
-static int pfn_array_alloc_pin(struct pfn_array *pa, struct device *mdev,
-			       u64 iova, unsigned int len)
+static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len)
  {
-	int i, ret = 0;
+	int i;
 
  	if (!len)
  		return 0;

@@ -97,6 +94,22 @@ static int pfn_array_alloc_pin(struct pfn_array *pa, struct device *mdev,
  	for (i = 1; i < pa->pa_nr; i++)
  		pa->pa_iova_pfn[i] = pa->pa_iova_pfn[i - 1] + 1;
 
+	return 0;
+}
+
+/*
+ * pfn_array_pin() - Pin user pages in memory
+ * @pa: pfn_array on which to perform the operation
+ * @mdev: the mediated device to perform pin operations
+ *
+ * Returns number of pages pinned upon success.
+ * If the pin request partially succeeds, or fails completely,
+ * all pages are left unpinned and a negative error value is returned.
+ */
+static int pfn_array_pin(struct pfn_array *pa, struct device *mdev)
+{
+	int ret = 0;
+
  	ret = vfio_pin_pages(mdev, pa->pa_iova_pfn, pa->pa_nr,
  			     IOMMU_READ | IOMMU_WRITE, pa->pa_pfn);
 

@@ -112,8 +125,6 @@ static int pfn_array_alloc_pin(struct pfn_array *pa, struct device *mdev,
 
  err_out:
  	pa->pa_nr = 0;
-	kfree(pa->pa_iova_pfn);
-	pa->pa_iova_pfn = NULL;
 
  	return ret;
  }

@@ -121,7 +132,9 @@ static int pfn_array_alloc_pin(struct pfn_array *pa, struct device *mdev,
  /* Unpin the pages before releasing the memory. */
  static void pfn_array_unpin_free(struct pfn_array *pa, struct device *mdev)
  {
-	vfio_unpin_pages(mdev, pa->pa_iova_pfn, pa->pa_nr);
+	/* Only unpin if any pages were pinned to begin with */
+	if (pa->pa_nr)
+		vfio_unpin_pages(mdev, pa->pa_iova_pfn, pa->pa_nr);
  	pa->pa_nr = 0;
  	kfree(pa->pa_iova_pfn);
  }

@@ -209,10 +222,16 @@ static long copy_from_iova(struct device *mdev,
  	int i, ret;
  	unsigned long l, m;
 
-	ret = pfn_array_alloc_pin(&pa, mdev, iova, n);
-	if (ret <= 0)
+	ret = pfn_array_alloc(&pa, iova, n);
+	if (ret < 0)
  		return ret;
 
+	ret = pfn_array_pin(&pa, mdev);
+	if (ret < 0) {
+		pfn_array_unpin_free(&pa, mdev);
+		return ret;
+	}
+
  	l = n;
  	for (i = 0; i < pa.pa_nr; i++) {
  		from = pa.pa_pfn[i] << PAGE_SHIFT;

@@ -559,7 +578,11 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
  	if (ret)
  		goto out_init;
 
-	ret = pfn_array_alloc_pin(pat->pat_pa, cp->mdev, ccw->cda, ccw->count);
+	ret = pfn_array_alloc(pat->pat_pa, ccw->cda, ccw->count);
+	if (ret < 0)
+		goto out_unpin;
+
+	ret = pfn_array_pin(pat->pat_pa, cp->mdev);
  	if (ret < 0)
  		goto out_unpin;
 

@@ -589,6 +612,7 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
  {
  	struct ccw1 *ccw;
  	struct pfn_array_table *pat;
+	struct pfn_array *pa;
  	unsigned long *idaws;
  	u64 idaw_iova;
  	unsigned int idaw_nr, idaw_len;

@@ -627,9 +651,13 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
 
  	for (i = 0; i < idaw_nr; i++) {
  		idaw_iova = *(idaws + i);
+		pa = pat->pat_pa + i;
+

Let's initialize the host address to something that is invalid, rather than letting it default to zero. This just makes it easier to notice when a pin operation has failed or been skipped.

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

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

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

index 60aa784717c5..0a97978d1d28 100644
@@ -91,8 +91,11 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len)
  	pa->pa_pfn = pa->pa_iova_pfn + pa->pa_nr;
 

It is possible that a guest might issue a CCW with a length of zero, and will expect a particular response. Consider this chain:

   Address   Format-1 CCW
   --------  -----------------
 0 33110EC0  346022CC 33177468
 1 33110EC8  CF200000 3318300C

CCW[0] moves a little more than two pages, but also has the Suppress Length Indication (SLI) bit set to handle the expectation that considerably less data will be moved. CCW[1] also has the SLI bit set, and has a length of zero. Once vfio-ccw does its magic, the kernel issues a start subchannel on behalf of the guest with this:

   Address   Format-1 CCW
   --------  -----------------
 0 021EDED0  346422CC 021F0000
 1 021EDED8  CF240000 3318300C

Both CCWs were converted to an IDAL and have the corresponding flags set (which is by design), but only the address of the first data address is converted to something the host is aware of. The second CCW still has the address used by the guest, which happens to be (A) (probably) an invalid address for the host, and (B) an invalid IDAW address (doubleword boundary, etc.).

While the I/O fails, it doesn't fail correctly. In this example, we would receive a program check for an invalid IDAW address, instead of a unit check for an invalid command.

To fix this, revert commit 4cebc5d6a6ff ("vfio: ccw: validate the count field of a ccw before pinning") and allow the individual fetch routines to process them like anything else. We'll make a slight adjustment to our allocation of the pfn_array (for direct CCWs) or IDAL (for IDAL CCWs) memory, so that we have room for at least one address even though no data will be transferred.

Note that this doesn't provide us with a channel program that will fail in the expected way. Since our length is zero, vfio_pin_pages() returns -EINVAL and cp_prefetch() will thus fail. This will be fixed in the next patch.

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

---
 drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

Quoted 37 lines · On Wed, 15 May 2019 01:42:46 +0200 Eric Farman <[email protected]> wrote:

It is possible that a guest might issue a CCW with a length of zero, and will expect a particular response. Consider this chain:

    Address   Format-1 CCW
    --------  -----------------
  0 33110EC0  346022CC 33177468
  1 33110EC8  CF200000 3318300C

CCW[0] moves a little more than two pages, but also has the Suppress Length Indication (SLI) bit set to handle the expectation that considerably less data will be moved. CCW[1] also has the SLI bit set, and has a length of zero. Once vfio-ccw does its magic, the kernel issues a start subchannel on behalf of the guest with this:

    Address   Format-1 CCW
    --------  -----------------
  0 021EDED0  346422CC 021F0000
  1 021EDED8  CF240000 3318300C

Both CCWs were converted to an IDAL and have the corresponding flags set (which is by design), but only the address of the first data address is converted to something the host is aware of. The second CCW still has the address used by the guest, which happens to be (A) (probably) an invalid address for the host, and (B) an invalid IDAW address (doubleword boundary, etc.).

While the I/O fails, it doesn't fail correctly. In this example, we would receive a program check for an invalid IDAW address, instead of a unit check for an invalid command.

To fix this, revert commit 4cebc5d6a6ff ("vfio: ccw: validate the count field of a ccw before pinning") and allow the individual fetch routines to process them like anything else. We'll make a slight adjustment to our allocation of the pfn_array (for direct CCWs) or IDAL (for IDAL CCWs) memory, so that we have room for at least one address even though no data will be transferred.

Quoted 2 lines

Note that this doesn't provide us with a channel program that will fail in the expected way. Since our length is zero, vfio_pin_pages()

Quoted 2 lines

returns -EINVAL and cp_prefetch() will thus fail. This will be fixed in the next patch.

So, this failed before, and still fails, just differently?

IOW, this
has no effect on bisectability?

Quoted 6 lines

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

---
  drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
  1 file changed, 8 insertions(+), 18 deletions(-)

Quoted 40 lines · Cornelia Huck

Quoted 37 lines · On Wed, 15 May 2019 01:42:46 +0200 Eric Farman <[email protected]> wrote:

It is possible that a guest might issue a CCW with a length of zero, and will expect a particular response.  Consider this chain:

    Address   Format-1 CCW
    --------  -----------------
  0 33110EC0  346022CC 33177468
  1 33110EC8  CF200000 3318300C

CCW[0] moves a little more than two pages, but also has the Suppress Length Indication (SLI) bit set to handle the expectation that considerably less data will be moved.  CCW[1] also has the SLI bit set, and has a length of zero.  Once vfio-ccw does its magic, the kernel issues a start subchannel on behalf of the guest with this:

    Address   Format-1 CCW
    --------  -----------------
  0 021EDED0  346422CC 021F0000
  1 021EDED8  CF240000 3318300C

Both CCWs were converted to an IDAL and have the corresponding flags set (which is by design), but only the address of the first data address is converted to something the host is aware of.  The second CCW still has the address used by the guest, which happens to be (A) (probably) an invalid address for the host, and (B) an invalid IDAW address (doubleword boundary, etc.).

While the I/O fails, it doesn't fail correctly.  In this example, we would receive a program check for an invalid IDAW address, instead of a unit check for an invalid command.

To fix this, revert commit 4cebc5d6a6ff ("vfio: ccw: validate the count field of a ccw before pinning") and allow the individual fetch routines to process them like anything else.  We'll make a slight adjustment to our allocation of the pfn_array (for direct CCWs) or IDAL (for IDAL CCWs) memory, so that we have room for at least one address even though no data will be transferred.

Quoted 2 lines

Quoted 2 lines

Note that this doesn't provide us with a channel program that will fail in the expected way.  Since our length is zero, vfio_pin_pages()

s/is/was/

Quoted 3 lines

Quoted 2 lines

returns -EINVAL and cp_prefetch() will thus fail.  This will be fixed in the next patch.

Quoted 1 line

So, this failed before, and still fails, just differently?

Probably.  If the guest gave us a valid address, the pin might actually work now whereas before it would fail because the length was zero.  If the address were also invalid,

Quoted 2 lines

IOW, this
has no effect on bisectability?

I think so, but I suppose that either (A) patch 5 and 6 could be squashed together, or (B) I could move the "set pa_nr to zero" (or more accurately, set it to ccw->count) pieces from patch 6 into this patch, so that the vfio_pin_pages() call occurs like it does today.

Quoted 1 line

Quoted 6 lines

Quoted 5 lines

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

---
  drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------

  1 file changed, 8 insertions(+), 18 deletions(-)

Quoted 57 lines · Eric Farman

Quoted 40 lines · Cornelia Huck

Quoted 37 lines · On Wed, 15 May 2019 01:42:46 +0200 Eric Farman <[email protected]> wrote:

It is possible that a guest might issue a CCW with a length of zero, and will expect a particular response.  Consider this chain:

    Address   Format-1 CCW
    --------  -----------------
  0 33110EC0  346022CC 33177468
  1 33110EC8  CF200000 3318300C

CCW[0] moves a little more than two pages, but also has the Suppress Length Indication (SLI) bit set to handle the expectation that considerably less data will be moved.  CCW[1] also has the SLI bit set, and has a length of zero.  Once vfio-ccw does its magic, the kernel issues a start subchannel on behalf of the guest with this:

    Address   Format-1 CCW
    --------  -----------------
  0 021EDED0  346422CC 021F0000
  1 021EDED8  CF240000 3318300C

Both CCWs were converted to an IDAL and have the corresponding flags set (which is by design), but only the address of the first data address is converted to something the host is aware of.  The second CCW still has the address used by the guest, which happens to be (A) (probably) an invalid address for the host, and (B) an invalid IDAW address (doubleword boundary, etc.).

While the I/O fails, it doesn't fail correctly.  In this example, we would receive a program check for an invalid IDAW address, instead of a unit check for an invalid command.

To fix this, revert commit 4cebc5d6a6ff ("vfio: ccw: validate the count field of a ccw before pinning") and allow the individual fetch routines to process them like anything else.  We'll make a slight adjustment to our allocation of the pfn_array (for direct CCWs) or IDAL (for IDAL CCWs) memory, so that we have room for at least one address even though no data will be transferred.

Quoted 2 lines

Quoted 2 lines

Note that this doesn't provide us with a channel program that will fail in the expected way.  Since our length is zero, vfio_pin_pages()

s/is/was/

Quoted 4 lines

Quoted 2 lines

returns -EINVAL and cp_prefetch() will thus fail.  This will be fixed in the next patch.

So, this failed before, and still fails, just differently?

Probably.  If the guest gave us a valid address, the pin might actually work now whereas before it would fail because the length was zero.  If the address were also invalid,

Quoted 9 lines

Quoted 2 lines

IOW, this
has no effect on bisectability?

I think so, but I suppose that either (A) patch 5 and 6 could be squashed together, or (B) I could move the "set pa_nr to zero" (or more accurately, set it to ccw->count) pieces from patch 6 into this patch, so that the vfio_pin_pages() call occurs like it does today.

Quoted 1 line

While going through patch 5, I was confused as to why we need to pin pages if we are only trying to translate the addresses and no data transfer will take place with count==0. Well, you answer that in patch 6 :)

So maybe it might be better to move parts of patch 6 to 5 or squash them, or maybe reverse the order.

Thanks
Farhan

Quoted 7 lines

Quoted 6 lines

Quoted 5 lines

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

---
  drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------

  1 file changed, 8 insertions(+), 18 deletions(-)  

Quoted 72 lines · On Wed, 15 May 2019 16:08:18 -0400 Farhan Ali <[email protected]> wrote:

Quoted 57 lines · Eric Farman

Quoted 40 lines · Cornelia Huck

Quoted 37 lines · On Wed, 15 May 2019 01:42:46 +0200 Eric Farman <[email protected]> wrote:

It is possible that a guest might issue a CCW with a length of zero, and will expect a particular response.  Consider this chain:

    Address   Format-1 CCW
    --------  -----------------
  0 33110EC0  346022CC 33177468
  1 33110EC8  CF200000 3318300C

CCW[0] moves a little more than two pages, but also has the Suppress Length Indication (SLI) bit set to handle the expectation that considerably less data will be moved.  CCW[1] also has the SLI bit set, and has a length of zero.  Once vfio-ccw does its magic, the kernel issues a start subchannel on behalf of the guest with this:

    Address   Format-1 CCW
    --------  -----------------
  0 021EDED0  346422CC 021F0000
  1 021EDED8  CF240000 3318300C

Both CCWs were converted to an IDAL and have the corresponding flags set (which is by design), but only the address of the first data address is converted to something the host is aware of.  The second CCW still has the address used by the guest, which happens to be (A) (probably) an invalid address for the host, and (B) an invalid IDAW address (doubleword boundary, etc.).

While the I/O fails, it doesn't fail correctly.  In this example, we would receive a program check for an invalid IDAW address, instead of a unit check for an invalid command.

To fix this, revert commit 4cebc5d6a6ff ("vfio: ccw: validate the count field of a ccw before pinning") and allow the individual fetch routines to process them like anything else.  We'll make a slight adjustment to our allocation of the pfn_array (for direct CCWs) or IDAL (for IDAL CCWs) memory, so that we have room for at least one address even though no data will be transferred.

Quoted 2 lines

Quoted 2 lines

Note that this doesn't provide us with a channel program that will fail in the expected way.  Since our length is zero, vfio_pin_pages()

s/is/was/

Quoted 4 lines

Quoted 2 lines

returns -EINVAL and cp_prefetch() will thus fail.  This will be fixed in the next patch.

So, this failed before, and still fails, just differently?

Probably.  If the guest gave us a valid address, the pin might actually work now whereas before it would fail because the length was zero.  If the address were also invalid,

Quoted 9 lines

Quoted 2 lines

IOW, this
has no effect on bisectability?

I think so, but I suppose that either (A) patch 5 and 6 could be squashed together, or (B) I could move the "set pa_nr to zero" (or more accurately, set it to ccw->count) pieces from patch 6 into this patch, so that the vfio_pin_pages() call occurs like it does today.

Quoted 1 line

While going through patch 5, I was confused as to why we need to pin pages if we are only trying to translate the addresses and no data transfer will take place with count==0. Well, you answer that in patch 6 :)

Quoted 2 lines

So maybe it might be better to move parts of patch 6 to 5 or squash them, or maybe reverse the order.

I think this will get a bit unwieldy of squashed, so what about simply moving code from 6 to 5? I think people are confused enough by the two patches to make a change look like a good idea.

(I can queue patches 1-4 to get them out of the way :)

Quoted 14 lines

Thanks
Farhan

Quoted 7 lines

Quoted 6 lines

Quoted 5 lines

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

---
   drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
   1 file changed, 8 insertions(+), 18 deletions(-)

If the CCW being processed is a No-Operation, then by definition no data is being transferred. Let's fold those checks into the normal CCW processors, rather than skipping out early.

Likewise, if the CCW being processed is a "test" (an invented definition to simply mean it ends in a zero), let's permit that to go through to the hardware. There's nothing inherently unique about those command codes versus one that ends in an eight [1], or any other otherwise valid command codes that are undefined for the device type in question.

[1] POPS states that a x08 is a TIC CCW, and that having any high-order bits enabled is invalid for format-1 CCWs. For format-0 CCWs, the high-order bits are ignored.

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

---
 drivers/s390/cio/vfio_ccw_cp.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Quoted 10 lines · On Wed, 15 May 2019 01:42:48 +0200 Eric Farman <[email protected]> wrote:

If the CCW being processed is a No-Operation, then by definition no data is being transferred. Let's fold those checks into the normal CCW processors, rather than skipping out early.

Likewise, if the CCW being processed is a "test" (an invented definition to simply mean it ends in a zero), let's permit that to go through to the hardware. There's nothing inherently unique about those command codes versus one that ends in an eight [1], or any other otherwise valid command codes that are undefined for the device type in question.

Hm... let's tweak that a bit? It's not that "test" is an invented category; it's just that this has not been a valid command for post-s/370 and therefore should not get any special treatment and just be sent to the hardware?

Quoted 10 lines

[1] POPS states that a x08 is a TIC CCW, and that having any high-order bits enabled is invalid for format-1 CCWs. For format-0 CCWs, the high-order bits are ignored.

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

---
  drivers/s390/cio/vfio_ccw_cp.c | 11 +++++------
  1 file changed, 5 insertions(+), 6 deletions(-)

Quoted 14 lines · Cornelia Huck

Quoted 10 lines · On Wed, 15 May 2019 01:42:48 +0200 Eric Farman <[email protected]> wrote:

If the CCW being processed is a No-Operation, then by definition no data is being transferred. Let's fold those checks into the normal CCW processors, rather than skipping out early.

Likewise, if the CCW being processed is a "test" (an invented definition to simply mean it ends in a zero), let's permit that to go through to the hardware. There's nothing inherently unique about those command codes versus one that ends in an eight [1], or any other otherwise valid command codes that are undefined for the device type in question.

Quoted 4 lines

Hm... let's tweak that a bit? It's not that "test" is an invented category; it's just that this has not been a valid command for post-s/370 and therefore should not get any special treatment and just be sent to the hardware?

Agreed, I should've re-read that one before I sent it... How about:

Likewise, if the CCW being processed is a "test" (a category defined here as an opcode that contains zero in the lowest four bits) then no special processing is necessary as far as vfio-ccw is concerned. These command codes have not been valid since the S/370 days, meaning they are invalid in the same way as one that ends in an eight [1] or an otherwise valid command code that is undefined for the device type in question. Considering that, let's just process "test" CCWs like any other CCW, and send everything to the hardware.

Quoted 12 lines

Quoted 11 lines

[1] POPS states that a x08 is a TIC CCW, and that having any high-order bits enabled is invalid for format-1 CCWs. For format-0 CCWs, the high-order bits are ignored.

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

---
  drivers/s390/cio/vfio_ccw_cp.c | 11 +++++------
  1 file changed, 5 insertions(+), 6 deletions(-)  

The attached are a few fixes to the vfio-ccw kernel code for potential errors or architecture anomalies. Under normal usage, and even most abnormal usage, they don't expose any problems to a well-behaved guest and its devices. But, they are deficiencies just the same and could cause some weird behavior if they ever popped up in real life.

I have tried to arrange these patches in a "solves a noticeable problem with existing workloads" to "solves a theoretical problem with hypothetical workloads" order. This way, the bigger ones at the end can be discussed without impeding the smaller and more impactful ones at the start.

Per the conversations on patch 7, the last several patches remain unchanged. They continue to buid an IDAL for each CCW, and only pin guest pages and assign the resulting addresses to IDAWs if they are expected to cause a data transfer. This will avoid sending an unmodified guest address, which may be invalid but anyway is not mapped to the same host address, in the IDAL sent to the channel subsystem and any unexpected behavior that may result.

They are based on 5.1.0, not Conny's vfio-ccw tree even though there are some good fixes pending for 5.2 there. I've run this series both with and without that code, but couldn't decide which base would provide an easier time applying patches. "I think" they should apply fine to both, but I apologize in advance if I guessed wrong! :)

Changelog:
v1 -> v2:
- Patch 1:
- [Cornelia] Added a code comment about why we update the SCSW when we've gone past the end of the chain for normal, successful, I/O
- Patch 2:
- [Cornelia] Cleaned up the cc info in the commit message
- [Pierre] Added r-b
- Patch 3:
- [Cornelia] Update the return code information in prologue of pfn_array_pin(), and then only call vfio_unpin_pages() if we pinned anything, rather than silently creating an error (this last bit was mentioned on patch 6, but applied here)
- [Eric] Clean up the error exit in pfn_array_pin()
- Patch 4-7 unchanged

Eric Farman (7):
s390/cio: Update SCSW if it points to the end of the chain s390/cio: Set vfio-ccw FSM state before ioeventfd s390/cio: Split pfn_array_alloc_pin into pieces s390/cio: Initialize the host addresses in pfn_array s390/cio: Allow zero-length CCWs in vfio-ccw s390/cio: Don't pin vfio pages for empty transfers s390/cio: Remove vfio-ccw checks of command codes

 drivers/s390/cio/vfio_ccw_cp.c  | 159 +++++++++++++++++++++++---------
 drivers/s390/cio/vfio_ccw_drv.c |   6 +-
 2 files changed, 119 insertions(+), 46 deletions(-)

Quoted 20 lines · On Wed, 15 May 2019 01:42:41 +0200 Eric Farman <[email protected]> wrote:

The attached are a few fixes to the vfio-ccw kernel code for potential errors or architecture anomalies. Under normal usage, and even most abnormal usage, they don't expose any problems to a well-behaved guest and its devices. But, they are deficiencies just the same and could cause some weird behavior if they ever popped up in real life.

I have tried to arrange these patches in a "solves a noticeable problem with existing workloads" to "solves a theoretical problem with hypothetical workloads" order. This way, the bigger ones at the end can be discussed without impeding the smaller and more impactful ones at the start.

Per the conversations on patch 7, the last several patches remain unchanged. They continue to buid an IDAL for each CCW, and only pin guest pages and assign the resulting addresses to IDAWs if they are expected to cause a data transfer. This will avoid sending an unmodified guest address, which may be invalid but anyway is not mapped to the same host address, in the IDAL sent to the channel subsystem and any unexpected behavior that may result.

Quoted 5 lines

They are based on 5.1.0, not Conny's vfio-ccw tree even though there are some good fixes pending for 5.2 there. I've run this series both with and without that code, but couldn't decide which base would provide an easier time applying patches. "I think" they should apply fine to both, but I apologize in advance if I guessed wrong! :)

They also should apply on current master, no? My 5.2 branch should be all merged by now :)

Nothing really jumped out at me; I'm happy to queue the patches if I get some more feedback.

Quoted 31 lines

Changelog:
v1 -> v2:
- Patch 1:
- [Cornelia] Added a code comment about why we update the SCSW when we've gone past the end of the chain for normal, successful, I/O
- Patch 2:
- [Cornelia] Cleaned up the cc info in the commit message
- [Pierre] Added r-b
- Patch 3:
- [Cornelia] Update the return code information in prologue of pfn_array_pin(), and then only call vfio_unpin_pages() if we pinned anything, rather than silently creating an error (this last bit was mentioned on patch 6, but applied here)
- [Eric] Clean up the error exit in pfn_array_pin()
- Patch 4-7 unchanged

Eric Farman (7):
s390/cio: Update SCSW if it points to the end of the chain s390/cio: Set vfio-ccw FSM state before ioeventfd s390/cio: Split pfn_array_alloc_pin into pieces s390/cio: Initialize the host addresses in pfn_array s390/cio: Allow zero-length CCWs in vfio-ccw s390/cio: Don't pin vfio pages for empty transfers s390/cio: Remove vfio-ccw checks of command codes

  drivers/s390/cio/vfio_ccw_cp.c  | 159 +++++++++++++++++++++++---------
  drivers/s390/cio/vfio_ccw_drv.c |   6 +-
  2 files changed, 119 insertions(+), 46 deletions(-)

They are based on 5.1.0, not Conny's vfio-ccw tree even though there are some good fixes pending for 5.2 there. I've run this series both with and without that code, but couldn't decide which base would provide an easier time applying patches. "I think" they should apply fine to both, but I apologize in advance if I guessed wrong! :)