s390: vfio-ccw fixes

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 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 and is used to determine whether to call vfio_unpin_pages() upon cleanup.

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 <[email protected]>

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

Quoted 19 lines · On Thu, 16 May 2019 18:14:01 +0200 Eric Farman <[email protected]> wrote:

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 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 and is used to determine whether to call vfio_unpin_pages() upon cleanup.

Quoted 4 lines

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.

I have now read this sentence several times and that this and that confuses me :)

What are we doing, and what is the thing that we might not be doing?

Quoted 6 lines

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

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

Quoted 27 lines · Cornelia Huck

Quoted 23 lines · On Thu, 16 May 2019 18:14:01 +0200 Eric Farman <[email protected]> wrote:

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 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 and is used to determine whether to call vfio_unpin_pages() upon cleanup.

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.

Quoted 2 lines

I have now read this sentence several times and that this and that confuses me :)

I have read this code for several months and I'm still confused. :)

Quoted 3 lines

What are we doing, and what is the thing that we might not be doing?

In the codepath that converts a direct-addressed CCW into an indirect one, we currently rely on the returned value from pfn_array_pin() to tell us how many pages were pinned, and thus how big of an IDAL to allocate. But since this patch causes us to skip the call to pfn_array_pin() for certain CCWs, using that value would be zero (leftover from pfn_array_alloc()) and thus would be weird to pass to the kcalloc() for our IDAL. We definitely want to allocate our own IDAL so that CCW.CDA contains a valid address, regardless of whether the IDAWs will be populated or not, so we calculate the number of pages ourselves here.

(Sidebar, the above is not a concern for the IDAL-to-IDAL codepath, since it has already calculated the size of the IDAL from the guest CCW and is going page-by-page through it.)

pfn_array_pin() doesn't return "partial pin" counts. If we ask for 10 pages to be pinned and it only does 5, we're going to get an error that we have to clean up from, rather than carrying on as if "up to 10" pages pinned was acceptable. To say that another way, there's no SLI bit for the vfio_pin_pages() call, so it's not necessary to rely on the count being returned if we ourselves calculate it.

So, with that... Maybe the paragraph in question should be something like this?

---8<---
The pfn_array_pin() routine returns the number of pages that were pinned, but now might be skipped for some CCWs. Thus we need to calculate the expected number of pages ourselves such that we are guaranteed to allocate a reasonable number of IDAWs, which will provide a valid address in CCW.CDA regardless of whether the IDAWs are filled in with pinned/translated addresses or not.

Quoted 8 lines

Quoted 7 lines

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

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

Quoted 28 lines · On Fri, 17 May 2019 08:57:10 -0400 Eric Farman <[email protected]> wrote:

Quoted 27 lines · Cornelia Huck

Quoted 23 lines · On Thu, 16 May 2019 18:14:01 +0200 Eric Farman <[email protected]> wrote:

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 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 and is used to determine whether to call vfio_unpin_pages() upon cleanup.

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.

Quoted 4 lines

Quoted 2 lines

I have now read this sentence several times and that this and that confuses me :)

I have read this code for several months and I'm still confused. :)

Lol, I guess you are not alone :)

Quoted 29 lines

Quoted 3 lines

What are we doing, and what is the thing that we might not be doing?

In the codepath that converts a direct-addressed CCW into an indirect one, we currently rely on the returned value from pfn_array_pin() to tell us how many pages were pinned, and thus how big of an IDAL to allocate. But since this patch causes us to skip the call to pfn_array_pin() for certain CCWs, using that value would be zero (leftover from pfn_array_alloc()) and thus would be weird to pass to the kcalloc() for our IDAL. We definitely want to allocate our own IDAL so that CCW.CDA contains a valid address, regardless of whether the IDAWs will be populated or not, so we calculate the number of pages ourselves here.

(Sidebar, the above is not a concern for the IDAL-to-IDAL codepath, since it has already calculated the size of the IDAL from the guest CCW and is going page-by-page through it.)

pfn_array_pin() doesn't return "partial pin" counts. If we ask for 10 pages to be pinned and it only does 5, we're going to get an error that we have to clean up from, rather than carrying on as if "up to 10" pages pinned was acceptable. To say that another way, there's no SLI bit for the vfio_pin_pages() call, so it's not necessary to rely on the count being returned if we ourselves calculate it.

So, with that... Maybe the paragraph in question should be something like this?

Quoted 7 lines

---8<---
The pfn_array_pin() routine returns the number of pages that were pinned, but now might be skipped for some CCWs. Thus we need to calculate the expected number of pages ourselves such that we are guaranteed to allocate a reasonable number of IDAWs, which will provide a valid address in CCW.CDA regardless of whether the IDAWs are filled in with pinned/translated addresses or not.

Much better, thanks!

I can change the description when picking up, if no reason for a respin comes up (series seems sane to me so far).

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

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

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

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 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 and is used to determine whether to call vfio_unpin_pages() upon cleanup.

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<[email protected]>

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

drivers/s390/cio/vfio_ccw_cp.c +19 -1

index 086faf2dacd3..0467838aed23 100644
@@ -294,6 +294,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)

@@ -301,10 +305,39 @@ 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 skip flag is off, then data will be transferred */
+	if (!ccw_is_skip(ccw))
+		return 1;
+

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

+
+	/* The skip flag is on, but it is ignored for this command code. */
+	return 1;
+}

Quoted 73 lines · Eric Farman

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 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 and is used to determine whether to call vfio_unpin_pages() upon cleanup.

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<[email protected]>

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

  1 file changed, 50 insertions(+), 5 deletions(-)

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

index 086faf2dacd3..0467838aed23 100644
@@ -294,6 +294,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)

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

@@ -301,10 +305,39 @@ 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.
+ */

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

+static inline int ccw_does_data_transfer(struct ccw1 *ccw)
+{
+    /* 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.
+     */

Quoted 5 lines

+    if (ccw_is_read(ccw) || ccw_is_read_backward(ccw)) +        return 0;
+
+    if (ccw_is_sense(ccw))
+        return 0;

Just out of curiosity, is there a reason we are checking ccw_is_sense in a separate if statement?

Quoted 5 lines

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

+
+    /* The skip flag is on, but it is ignored for this command code. */
+    return 1;
+}

Here are the remaining patches in my fixes series, to handle the more involved scenario of channel programs that do not move any actual data to/from the device. They were reordered per feedback from v2, which means they received minor massaging because of overlapping code and some cleanup to the commit messages.

They are based on Conny's vfio-ccw tree. :)

Changelog:
v2 -> v3:
- Patches 1-4:
- [Farhan] Added r-b
- [Cornelia] Queued to vfio-ccw, dropped from this version
- Patches 5/6:
- [Cornelia/Farhan] Swapped the order of these patches, minor rework on the placement of bytes/idaw_nr variables and the commit messages that resulted.
v2: https://patchwork.kernel.org/cover/10944075/
v1: https://patchwork.kernel.org/cover/10928799/

Eric Farman (3):
s390/cio: Don't pin vfio pages for empty transfers s390/cio: Allow zero-length CCWs in vfio-ccw s390/cio: Remove vfio-ccw checks of command codes

  drivers/s390/cio/vfio_ccw_cp.c | 92 ++++++++++++++++++++++++----------
  1 file changed, 65 insertions(+), 27 deletions(-)

Quoted 25 lines · Eric Farman

Here are the remaining patches in my fixes series, to handle the more involved scenario of channel programs that do not move any actual data to/from the device. They were reordered per feedback from v2, which means they received minor massaging because of overlapping code and some cleanup to the commit messages.

They are based on Conny's vfio-ccw tree. :)

Changelog:
v2 -> v3:
- Patches 1-4:
- [Farhan] Added r-b
- [Cornelia] Queued to vfio-ccw, dropped from this version
- Patches 5/6:
- [Cornelia/Farhan] Swapped the order of these patches, minor rework on the placement of bytes/idaw_nr variables and the commit messages that resulted.
v2: https://patchwork.kernel.org/cover/10944075/
v1: https://patchwork.kernel.org/cover/10928799/

Eric Farman (3):
s390/cio: Don't pin vfio pages for empty transfers s390/cio: Allow zero-length CCWs in vfio-ccw s390/cio: Remove vfio-ccw checks of command codes

Quoted 3 lines

  drivers/s390/cio/vfio_ccw_cp.c | 92 ++++++++++++++++++++++++----------
  1 file changed, 65 insertions(+), 27 deletions(-)

Acked-by Farhan Ali <[email protected]> for the series.

Here are the remaining patches in my fixes series, to handle the more involved scenario of channel programs that do not move any actual data to/from the device. They were reordered per feedback from v2, which means they received minor massaging because of overlapping code and some cleanup to the commit messages.

They are based on Conny's vfio-ccw tree. :)

Changelog:
v2 -> v3:
- Patches 1-4:
- [Farhan] Added r-b
- [Cornelia] Queued to vfio-ccw, dropped from this version
- Patches 5/6:
- [Cornelia/Farhan] Swapped the order of these patches, minor rework on the placement of bytes/idaw_nr variables and the commit messages that resulted.
v2: https://patchwork.kernel.org/cover/10944075/
v1: https://patchwork.kernel.org/cover/10928799/

Eric Farman (3):
s390/cio: Don't pin vfio pages for empty transfers s390/cio: Allow zero-length CCWs in vfio-ccw s390/cio: Remove vfio-ccw checks of command codes

 drivers/s390/cio/vfio_ccw_cp.c | 92 ++++++++++++++++++++++++----------
 1 file changed, 65 insertions(+), 27 deletions(-)