[PATCH v2 0/7] s390: vfio-ccw fixes

[PATCH v2 0/7] s390: vfio-ccw fixes

[Eric Farman]

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

-- 
2.17.1

[PATCH v2 1/7] s390/cio: Update SCSW if it points to the end of the chain

[Eric Farman]

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 <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 384b3987eeb4..41f48b8790bc 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -870,7 +870,11 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
 	 */
 	list_for_each_entry(chain, &cp->ccwchain_list, next) {
 		ccw_head = (u32)(u64)chain->ch_ccw;
-		if (is_cpa_within_range(cpa, ccw_head, chain->ch_len)) {
+		/*
+		 * On successful execution, cpa points just beyond the end
+		 * of the chain.
+		 */
+		if (is_cpa_within_range(cpa, ccw_head, chain->ch_len + 1)) {
 			/*
 			 * (cpa - ccw_head) is the offset value of the host
 			 * physical ccw to its chain head.
-- 
2.17.1

[PATCH v2 2/7] s390/cio: Set vfio-ccw FSM state before ioeventfd

[Eric Farman]

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 <farman@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 0b3b9de45c60..ddd21b6149fd 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -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)
-		eventfd_signal(private->io_trigger, 1);
-
 	if (private->mdev && is_final)
 		private->state = VFIO_CCW_STATE_IDLE;
+
+	if (private->io_trigger)
+		eventfd_signal(private->io_trigger, 1);
 }
 
 /*
-- 
2.17.1

[PATCH v2 3/7] s390/cio: Split pfn_array_alloc_pin into pieces

[Eric Farman]

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 <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 64 ++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 18 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 41f48b8790bc..60aa784717c5 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -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;
+
+		ret = pfn_array_alloc(pa, idaw_iova, 1);
+		if (ret < 0)
+			goto out_free_idaws;
 
-		ret = pfn_array_alloc_pin(pat->pat_pa + i, cp->mdev,
-					  idaw_iova, 1);
+		ret = pfn_array_pin(pa, cp->mdev);
 		if (ret < 0)
 			goto out_free_idaws;
 	}
-- 
2.17.1

[PATCH v2 4/7] s390/cio: Initialize the host addresses in pfn_array

[Eric Farman]

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 <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 60aa784717c5..0a97978d1d28 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -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;
 
 	pa->pa_iova_pfn[0] = pa->pa_iova >> PAGE_SHIFT;
-	for (i = 1; i < pa->pa_nr; i++)
+	pa->pa_pfn[0] = -1ULL;
+	for (i = 1; i < pa->pa_nr; i++) {
 		pa->pa_iova_pfn[i] = pa->pa_iova_pfn[i - 1] + 1;
+		pa->pa_pfn[i] = -1ULL;
+	}
 
 	return 0;
 }
-- 
2.17.1

[PATCH v2 5/7] s390/cio: Allow zero-length CCWs in vfio-ccw

[Eric Farman]

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 <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 0a97978d1d28..a68711114aab 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -70,9 +70,6 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len)
 {
 	int i;
 
-	if (!len)
-		return 0;
-
 	if (pa->pa_nr || pa->pa_iova_pfn)
 		return -EINVAL;
 
@@ -372,8 +369,6 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)
 
 	if (ccw_is_test(ccw) || ccw_is_noop(ccw) || ccw_is_tic(ccw))
 		return;
-	if (!ccw->count)
-		return;
 
 	kfree((void *)(u64)ccw->cda);
 }
@@ -558,18 +553,12 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	struct pfn_array_table *pat;
 	unsigned long *idaws;
 	int ret;
+	int bytes = 1;
 
 	ccw = chain->ch_ccw + idx;
 
-	if (!ccw->count) {
-		/*
-		 * We just want the translation result of any direct ccw
-		 * to be an IDA ccw, so let's add the IDA flag for it.
-		 * Although the flag will be ignored by firmware.
-		 */
-		ccw->flags |= CCW_FLAG_IDA;
-		return 0;
-	}
+	if (ccw->count)
+		bytes = ccw->count;
 
 	/*
 	 * Pin data page(s) in memory.
@@ -581,7 +570,7 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	if (ret)
 		goto out_init;
 
-	ret = pfn_array_alloc(pat->pat_pa, ccw->cda, ccw->count);
+	ret = pfn_array_alloc(pat->pat_pa, ccw->cda, bytes);
 	if (ret < 0)
 		goto out_unpin;
 
@@ -620,17 +609,18 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
 	u64 idaw_iova;
 	unsigned int idaw_nr, idaw_len;
 	int i, ret;
+	int bytes = 1;
 
 	ccw = chain->ch_ccw + idx;
 
-	if (!ccw->count)
-		return 0;
+	if (ccw->count)
+		bytes = ccw->count;
 
 	/* Calculate size of idaws. */
 	ret = copy_from_iova(cp->mdev, &idaw_iova, ccw->cda, sizeof(idaw_iova));
 	if (ret)
 		return ret;
-	idaw_nr = idal_nr_words((void *)(idaw_iova), ccw->count);
+	idaw_nr = idal_nr_words((void *)(idaw_iova), bytes);
 	idaw_len = idaw_nr * sizeof(*idaws);
 
 	/* Pin data page(s) in memory. */
-- 
2.17.1

[PATCH v2 6/7] s390/cio: Don't pin vfio pages for empty transfers

[Eric Farman]

If a CCW has a count of zero, then no data will be transferred and
pinning/unpinning memory is unnecessary.

In addition to that, the skip flag of a CCW offers the possibility of
data not being transferred, but is only meaningful for certain commands.
Specifically, it is only applicable for a read, read backward, sense, or
sense ID CCW and will be ignored for any other command code
(SA22-7832-11 page 15-64, and figure 15-30 on page 15-75).

(A sense ID is xE4, while a sense is x04 with possible modifiers in the
upper four bits.  So we will cover the whole "family" of sense CCWs.)

For all those scenarios, since there is no requirement for the target
address to be valid, we should skip the call to vfio_pin_pages() and
rely on the IDAL address we have allocated/built for the channel
program.  The fact that the individual IDAWs within the IDAL are
invalid is fine, since they aren't actually checked in these cases.

Set pa_nr to zero, when skipping the pfn_array_pin() call, since it is
defined as the number of pages pinned.  This will cause the vfio unpin
logic to return -EINVAL, but since the return code is not checked it
will not harm our cleanup path.

As we do this, since the pfn_array_pin() routine returns the number of
pages pinned, and we might not be doing that, the logic for converting
a CCW from direct-addressed to IDAL needs to ensure there is room for
one IDAW in the IDAL being built since a zero-length IDAL isn't great.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 61 ++++++++++++++++++++++++++++++----
 1 file changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index a68711114aab..94888094ed2c 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -291,6 +291,10 @@ static long copy_ccw_from_iova(struct channel_program *cp,
 /*
  * Helpers to operate ccwchain.
  */
+#define ccw_is_read(_ccw) (((_ccw)->cmd_code & 0x03) == 0x02)
+#define ccw_is_read_backward(_ccw) (((_ccw)->cmd_code & 0x0F) == 0x0C)
+#define ccw_is_sense(_ccw) (((_ccw)->cmd_code & 0x0F) == CCW_CMD_BASIC_SENSE)
+
 #define ccw_is_test(_ccw) (((_ccw)->cmd_code & 0x0F) == 0)
 
 #define ccw_is_noop(_ccw) ((_ccw)->cmd_code == CCW_CMD_NOOP)
@@ -298,10 +302,43 @@ static long copy_ccw_from_iova(struct channel_program *cp,
 #define ccw_is_tic(_ccw) ((_ccw)->cmd_code == CCW_CMD_TIC)
 
 #define ccw_is_idal(_ccw) ((_ccw)->flags & CCW_FLAG_IDA)
-
+#define ccw_is_skip(_ccw) ((_ccw)->flags & CCW_FLAG_SKIP)
 
 #define ccw_is_chain(_ccw) ((_ccw)->flags & (CCW_FLAG_CC | CCW_FLAG_DC))
 
+/*
+ * ccw_does_data_transfer()
+ *
+ * Determine whether a CCW will move any data, such that the guest pages
+ * would need to be pinned before performing the I/O.
+ *
+ * Returns 1 if yes, 0 if no.
+ */
+static inline int ccw_does_data_transfer(struct ccw1 *ccw)
+{
+	/* If the count field is zero, then no data will be transferred */
+	if (ccw->count == 0)
+		return 0;
+
+	/* If the skip flag is off, then data will be transferred */
+	if (!ccw_is_skip(ccw))
+		return 1;
+
+	/*
+	 * If the skip flag is on, it is only meaningful if the command
+	 * code is a read, read backward, sense, or sense ID.  In those
+	 * cases, no data will be transferred.
+	 */
+	if (ccw_is_read(ccw) || ccw_is_read_backward(ccw))
+		return 0;
+
+	if (ccw_is_sense(ccw))
+		return 0;
+
+	/* The skip flag is on, but it is ignored for this command code. */
+	return 1;
+}
+
 /*
  * is_cpa_within_range()
  *
@@ -554,11 +591,14 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	unsigned long *idaws;
 	int ret;
 	int bytes = 1;
+	int idaw_nr = 1;
 
 	ccw = chain->ch_ccw + idx;
 
-	if (ccw->count)
+	if (ccw->count) {
 		bytes = ccw->count;
+		idaw_nr = idal_nr_words((void *)(u64)ccw->cda, ccw->count);
+	}
 
 	/*
 	 * Pin data page(s) in memory.
@@ -574,12 +614,16 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	if (ret < 0)
 		goto out_unpin;
 
-	ret = pfn_array_pin(pat->pat_pa, cp->mdev);
-	if (ret < 0)
-		goto out_unpin;
+	if (ccw_does_data_transfer(ccw)) {
+		ret = pfn_array_pin(pat->pat_pa, cp->mdev);
+		if (ret < 0)
+			goto out_unpin;
+	} else {
+		pat->pat_pa->pa_nr = 0;
+	}
 
 	/* Translate this direct ccw to a idal ccw. */
-	idaws = kcalloc(ret, sizeof(*idaws), GFP_DMA | GFP_KERNEL);
+	idaws = kcalloc(idaw_nr, sizeof(*idaws), GFP_DMA | GFP_KERNEL);
 	if (!idaws) {
 		ret = -ENOMEM;
 		goto out_unpin;
@@ -650,6 +694,11 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
 		if (ret < 0)
 			goto out_free_idaws;
 
+		if (!ccw_does_data_transfer(ccw)) {
+			pa->pa_nr = 0;
+			continue;
+		}
+
 		ret = pfn_array_pin(pa, cp->mdev);
 		if (ret < 0)
 			goto out_free_idaws;
-- 
2.17.1

[PATCH v2 7/7] s390/cio: Remove vfio-ccw checks of command codes

[Eric Farman]

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 <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 94888094ed2c..abe0f501a963 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -295,8 +295,6 @@ static long copy_ccw_from_iova(struct channel_program *cp,
 #define ccw_is_read_backward(_ccw) (((_ccw)->cmd_code & 0x0F) == 0x0C)
 #define ccw_is_sense(_ccw) (((_ccw)->cmd_code & 0x0F) == CCW_CMD_BASIC_SENSE)
 
-#define ccw_is_test(_ccw) (((_ccw)->cmd_code & 0x0F) == 0)
-
 #define ccw_is_noop(_ccw) ((_ccw)->cmd_code == CCW_CMD_NOOP)
 
 #define ccw_is_tic(_ccw) ((_ccw)->cmd_code == CCW_CMD_TIC)
@@ -320,6 +318,10 @@ static inline int ccw_does_data_transfer(struct ccw1 *ccw)
 	if (ccw->count == 0)
 		return 0;
 
+	/* If the command is a NOP, then no data will be transferred */
+	if (ccw_is_noop(ccw))
+		return 0;
+
 	/* If the skip flag is off, then data will be transferred */
 	if (!ccw_is_skip(ccw))
 		return 1;
@@ -404,7 +406,7 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)
 {
 	struct ccw1 *ccw = chain->ch_ccw + idx;
 
-	if (ccw_is_test(ccw) || ccw_is_noop(ccw) || ccw_is_tic(ccw))
+	if (ccw_is_tic(ccw))
 		return;
 
 	kfree((void *)(u64)ccw->cda);
@@ -729,9 +731,6 @@ static int ccwchain_fetch_one(struct ccwchain *chain,
 {
 	struct ccw1 *ccw = chain->ch_ccw + idx;
 
-	if (ccw_is_test(ccw) || ccw_is_noop(ccw))
-		return 0;
-
 	if (ccw_is_tic(ccw))
 		return ccwchain_fetch_tic(chain, idx, cp);
 
-- 
2.17.1

Re: [PATCH v2 5/7] s390/cio: Allow zero-length CCWs in vfio-ccw

[Cornelia Huck]

On Wed, 15 May 2019 01:42:46 +0200
Eric Farman <farman@linux.ibm.com> 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.
> 
> 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.

So, this failed before, and still fails, just differently? IOW, this
has no effect on bisectability?

> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
>  1 file changed, 8 insertions(+), 18 deletions(-)

Re: [PATCH v2 7/7] s390/cio: Remove vfio-ccw checks of command codes

[Cornelia Huck]

On Wed, 15 May 2019 01:42:48 +0200
Eric Farman <farman@linux.ibm.com> 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?

> 
> [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 <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)

Re: [PATCH v2 0/7] s390: vfio-ccw fixes

[Cornelia Huck]

On Wed, 15 May 2019 01:42:41 +0200
Eric Farman <farman@linux.ibm.com> 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.
> 
> 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.

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

Re: [PATCH v2 0/7] s390: vfio-ccw fixes

[Eric Farman]

On 5/15/19 8:45 AM, Cornelia Huck wrote:
> On Wed, 15 May 2019 01:42:41 +0200
> Eric Farman <farman@linux.ibm.com> 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.
>>
>> 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 :)

Yeah, I just haven't updated my branches yet.  :)

> 
> Nothing really jumped out at me; I'm happy to queue the patches if I
> get some more feedback.
> 
>>
>>
>> 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(-)
>>
> 

Re: [PATCH v2 7/7] s390/cio: Remove vfio-ccw checks of command codes

[Eric Farman]

On 5/15/19 8:43 AM, Cornelia Huck wrote:
> On Wed, 15 May 2019 01:42:48 +0200
> Eric Farman <farman@linux.ibm.com> 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?

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.

> 
>>
>> [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 <farman@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_cp.c | 11 +++++------
>>   1 file changed, 5 insertions(+), 6 deletions(-)
> 

Re: [PATCH v2 7/7] s390/cio: Remove vfio-ccw checks of command codes

[Cornelia Huck]

On Wed, 15 May 2019 09:36:01 -0400
Eric Farman <farman@linux.ibm.com> wrote:

> On 5/15/19 8:43 AM, Cornelia Huck wrote:
> > On Wed, 15 May 2019 01:42:48 +0200
> > Eric Farman <farman@linux.ibm.com> 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?  
> 
> 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.

Sounds good to me!

> 
> >   
> >>
> >> [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 <farman@linux.ibm.com>
> >> ---
> >>   drivers/s390/cio/vfio_ccw_cp.c | 11 +++++------
> >>   1 file changed, 5 insertions(+), 6 deletions(-)  
> >   
> 

Re: [PATCH v2 1/7] s390/cio: Update SCSW if it points to the end of the chain

[Farhan Ali]

On 05/14/2019 07:42 PM, Eric Farman wrote:
> 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 <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_cp.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 384b3987eeb4..41f48b8790bc 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -870,7 +870,11 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
>   	 */
>   	list_for_each_entry(chain, &cp->ccwchain_list, next) {
>   		ccw_head = (u32)(u64)chain->ch_ccw;
> -		if (is_cpa_within_range(cpa, ccw_head, chain->ch_len)) {
> +		/*
> +		 * On successful execution, cpa points just beyond the end
> +		 * of the chain.
> +		 */
> +		if (is_cpa_within_range(cpa, ccw_head, chain->ch_len + 1)) {
>   			/*
>   			 * (cpa - ccw_head) is the offset value of the host
>   			 * physical ccw to its chain head.
> 

Reviewed-by: Farhan Ali <alifm@linux.ibm.com>

Re: [PATCH v2 2/7] s390/cio: Set vfio-ccw FSM state before ioeventfd

[Farhan Ali]

On 05/14/2019 07:42 PM, Eric Farman wrote:
> 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 <farman@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_drv.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 0b3b9de45c60..ddd21b6149fd 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -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)
> -		eventfd_signal(private->io_trigger, 1);
> -
>   	if (private->mdev && is_final)
>   		private->state = VFIO_CCW_STATE_IDLE;
> +
> +	if (private->io_trigger)
> +		eventfd_signal(private->io_trigger, 1);
>   }
>   
>   /*
> 
Reviewed-by: Farhan Ali <alifm@linux.ibm.com>

Re: [PATCH v2 5/7] s390/cio: Allow zero-length CCWs in vfio-ccw

[Eric Farman]

On 5/15/19 8:23 AM, Cornelia Huck wrote:
> On Wed, 15 May 2019 01:42:46 +0200
> Eric Farman <farman@linux.ibm.com> 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.
>>
>> 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/

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

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

> 
>>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
>>   1 file changed, 8 insertions(+), 18 deletions(-)
> 

Re: [PATCH v2 3/7] s390/cio: Split pfn_array_alloc_pin into pieces

[Farhan Ali]

On 05/14/2019 07:42 PM, Eric Farman wrote:
> 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 <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_cp.c | 64 ++++++++++++++++++++++++----------
>   1 file changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 41f48b8790bc..60aa784717c5 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -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;
> +
> +		ret = pfn_array_alloc(pa, idaw_iova, 1);
> +		if (ret < 0)
> +			goto out_free_idaws;
>   
> -		ret = pfn_array_alloc_pin(pat->pat_pa + i, cp->mdev,
> -					  idaw_iova, 1);
> +		ret = pfn_array_pin(pa, cp->mdev);
>   		if (ret < 0)
>   			goto out_free_idaws;
>   	}
> 
Reviewed-by: Farhan Ali <alifm@linux.ibm.com>

Re: [PATCH v2 4/7] s390/cio: Initialize the host addresses in pfn_array

[Farhan Ali]

On 05/14/2019 07:42 PM, Eric Farman wrote:
> 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 <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_cp.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 60aa784717c5..0a97978d1d28 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -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;
>   
>   	pa->pa_iova_pfn[0] = pa->pa_iova >> PAGE_SHIFT;
> -	for (i = 1; i < pa->pa_nr; i++)
> +	pa->pa_pfn[0] = -1ULL;
> +	for (i = 1; i < pa->pa_nr; i++) {
>   		pa->pa_iova_pfn[i] = pa->pa_iova_pfn[i - 1] + 1;
> +		pa->pa_pfn[i] = -1ULL;
> +	}
>   
>   	return 0;
>   }
> 

Reviewed-by: Farhan Ali <alifm@linux.ibm.com>

Re: [PATCH v2 5/7] s390/cio: Allow zero-length CCWs in vfio-ccw

[Farhan Ali]

On 05/15/2019 11:04 AM, Eric Farman wrote:
> 
> 
> On 5/15/19 8:23 AM, Cornelia Huck wrote:
>> On Wed, 15 May 2019 01:42:46 +0200
>> Eric Farman <farman@linux.ibm.com> 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.
>>>
>>> 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/
> 
>>> 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,
> 
>  >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.
> 
>>

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


>>>
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>> ---
>>>   drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
>>>   1 file changed, 8 insertions(+), 18 deletions(-)
>>
> 

Re: [PATCH v2 5/7] s390/cio: Allow zero-length CCWs in vfio-ccw

[Cornelia Huck]

On Wed, 15 May 2019 16:08:18 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 05/15/2019 11:04 AM, Eric Farman wrote:
> > 
> > 
> > On 5/15/19 8:23 AM, Cornelia Huck wrote:  
> >> On Wed, 15 May 2019 01:42:46 +0200
> >> Eric Farman <farman@linux.ibm.com> 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.
> >>>
> >>> 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/
> >   
> >>> 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,
> >   
> >  >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.
> >   
> >>  
> 
> 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.

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

> 
> Thanks
> Farhan
> 
> 
> >>>
> >>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> >>> ---
> >>>   drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
> >>>   1 file changed, 8 insertions(+), 18 deletions(-)  
> >>  
> >   
> 

Re: [PATCH v2 5/7] s390/cio: Allow zero-length CCWs in vfio-ccw

[Eric Farman]

On 5/16/19 5:59 AM, Cornelia Huck wrote:
> On Wed, 15 May 2019 16:08:18 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> On 05/15/2019 11:04 AM, Eric Farman wrote:
>>>
>>>
>>> On 5/15/19 8:23 AM, Cornelia Huck wrote:
>>>> On Wed, 15 May 2019 01:42:46 +0200
>>>> Eric Farman <farman@linux.ibm.com> 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.
>>>>>
>>>>> 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/
>>>    
>>>>> 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,
>>>    
>>>   >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.
>>>    
>>>>   
>>
>> 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.
> 
> 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.

Agreed.  I swapped them locally yesterday to see how bad that work might 
become, and I think got them reworked to fit properly.  Will be making 
sure they don't break anything in this order today, but shouldn't take 
long to be sure.

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

I wouldn't mind that.  :)

  - Eric

> 
>>
>> Thanks
>> Farhan
>>
>>
>>>>>
>>>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>>>> ---
>>>>>    drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
>>>>>    1 file changed, 8 insertions(+), 18 deletions(-)
>>>>   
>>>    
>>
> 

Re: [PATCH v2 0/7] s390: vfio-ccw fixes

[Cornelia Huck]

On Wed, 15 May 2019 01:42:41 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> 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!  :)

Patch 2 stumbled over a trivial-to-fix context change; else, things
applied cleanly.

Thanks, queued patches 1-4.