[PATCH v3 0/3] s390: vfio-ccw fixes

[PATCH v3 0/3] s390: vfio-ccw fixes

[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

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

-- 
2.17.1

[PATCH v3 1/3] s390/cio: Don't pin vfio pages for empty transfers

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 086faf2dacd3..0467838aed23 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -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;
+
+	/*
+	 * 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()
  *
@@ -559,6 +592,7 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	struct pfn_array_table *pat;
 	unsigned long *idaws;
 	int ret;
+	int idaw_nr = 1;
 
 	ccw = chain->ch_ccw + idx;
 
@@ -570,6 +604,8 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 		 */
 		ccw->flags |= CCW_FLAG_IDA;
 		return 0;
+	} else {
+		idaw_nr = idal_nr_words((void *)(u64)ccw->cda, ccw->count);
 	}
 
 	/*
@@ -586,12 +622,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;
@@ -661,6 +701,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 v3 2/3] 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 guest memory will be pinned and thus the
IDAW will not be populated with a host address.

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 0467838aed23..c77c9b4cd2a8 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;
 
@@ -319,6 +316,10 @@ static long copy_ccw_from_iova(struct channel_program *cp,
  */
 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;
@@ -405,8 +406,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);
 }
@@ -592,19 +591,13 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	struct pfn_array_table *pat;
 	unsigned long *idaws;
 	int ret;
+	int bytes = 1;
 	int idaw_nr = 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;
-	} else {
+	if (ccw->count) {
+		bytes = ccw->count;
 		idaw_nr = idal_nr_words((void *)(u64)ccw->cda, ccw->count);
 	}
 
@@ -618,7 +611,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;
 
@@ -661,17 +654,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 v3 3/3] 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" (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(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index c77c9b4cd2a8..f73cfcfdd032 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);
@@ -730,9 +732,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 v3 1/3] s390/cio: Don't pin vfio pages for empty transfers

[Cornelia Huck]

On Thu, 16 May 2019 18:14:01 +0200
Eric Farman <farman@linux.ibm.com> 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.

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?

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

Re: [PATCH v3 1/3] s390/cio: Don't pin vfio pages for empty transfers

[Eric Farman]

On 5/17/19 5:06 AM, Cornelia Huck wrote:
> On Thu, 16 May 2019 18:14:01 +0200
> Eric Farman <farman@linux.ibm.com> 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.
> 
> 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.  :)

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

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

Re: [PATCH v3 1/3] s390/cio: Don't pin vfio pages for empty transfers

[Cornelia Huck]

On Fri, 17 May 2019 08:57:10 -0400
Eric Farman <farman@linux.ibm.com> wrote:

> On 5/17/19 5:06 AM, Cornelia Huck wrote:
> > On Thu, 16 May 2019 18:14:01 +0200
> > Eric Farman <farman@linux.ibm.com> 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.  
> > 
> > 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 :)

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

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

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

Re: [PATCH v3 1/3] s390/cio: Don't pin vfio pages for empty transfers

[Eric Farman]

On 5/17/19 10:06 AM, Cornelia Huck wrote:
> On Fri, 17 May 2019 08:57:10 -0400
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> On 5/17/19 5:06 AM, Cornelia Huck wrote:
>>> On Thu, 16 May 2019 18:14:01 +0200
>>> Eric Farman <farman@linux.ibm.com> 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.  
>>>
>>> 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 :)
> 
>>
>>> 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.
> 
> 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).

I appreciate that, thank you!  Looking forward to what others may say.

 - Eric

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

Re: [PATCH v3 1/3] s390/cio: Don't pin vfio pages for empty transfers

[Farhan Ali]

On 05/16/2019 12:14 PM, Eric Farman 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.
> 
> Signed-off-by: Eric Farman<farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_cp.c | 55 ++++++++++++++++++++++++++++++----
>   1 file changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 086faf2dacd3..0467838aed23 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -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;
> +
> +	/*
> +	 * 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;

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

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

Re: [PATCH v3 1/3] s390/cio: Don't pin vfio pages for empty transfers

[Eric Farman]

On 5/20/19 4:35 PM, Farhan Ali wrote:
> 
> 
> On 05/16/2019 12:14 PM, Eric Farman 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.
>>
>> Signed-off-by: Eric Farman<farman@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_cp.c | 55 ++++++++++++++++++++++++++++++----
>>   1 file changed, 50 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c
>> b/drivers/s390/cio/vfio_ccw_cp.c
>> index 086faf2dacd3..0467838aed23 100644
>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>> @@ -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;
>> +
>> +    /*
>> +     * 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;
> 
> Just out of curiosity, is there a reason we are checking ccw_is_sense in
> a separate if statement?

No reason besides I thought it read nicer this way, with read
forward/backward being grouped together and not needing to force
everything to fit in 80 columns.  Knowing another opcode (NOP) would be
added later made this layout seem logical too.

The generated assembly is identical regardless of how it's written,
which is not surprising based on the different masks that have to be
employed.

 - Eric

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

Re: [PATCH v3 0/3] s390: vfio-ccw fixes

[Farhan Ali]

On 05/16/2019 12:14 PM, Eric Farman wrote:
> 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(-)
> 


Acked-by: Farhan Ali <alifm@linux.ibm.com> for the series.

Thanks
Farhan

Re: [PATCH v3 0/3] s390: vfio-ccw fixes

[Cornelia Huck]

On Wed, 22 May 2019 08:20:43 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 05/16/2019 12:14 PM, Eric Farman wrote:
> > 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(-)
> >   
> 
> 
> Acked-by: Farhan Ali <alifm@linux.ibm.com> for the series.

Thank you!

Re: [PATCH v3 0/3] s390: vfio-ccw fixes

[Cornelia Huck]

On Thu, 16 May 2019 18:14:00 +0200
Eric Farman <farman@linux.ibm.com> wrote:

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

Thanks, applied.