[PATCH v2 0/9] s390: vfio-ccw code rework

[PATCH v2 0/9] s390: vfio-ccw code rework

[Eric Farman]

Now that we've gotten a lot of other series either merged or
pending for the next merge window, I'd like to revisit some
code simplification that I started many moons ago.

In that first series, a couple of fixes got merged into 4.20,
a couple more got some "seems okay" acks/reviews, and the rest
were nearly forgotten about.  I dusted them off and did quite a
bit of rework to make things a little more sequential and
providing a better narrative (I think) based on the lessons we
learned in my earlier changes.  Because of this rework, the
acks/reviews on the first version didn't really translate to the
code that exists here (patch 1 being the closest exception), so
I didn't apply any of them here.  The end result is mostly the
same as before, but now looks like this:

Patch summary:
1:   Squash duplicate code
2-4: Remove duplicate code in CCW processor
5-7: Remove one layer of nested arrays
8-9: Combine direct/indirect addressing CCW processors

Using 5.2.0-rc3 as a base plus the vfio-ccw branch of recent fixes,
we shrink the code quite a bit (8.7% according to the bloat-o-meter),
and we remove one set of mallocs/frees on the I/O path by removing
one layer of the nested arrays.  There are no functional/behavioral
changes with this series; all the tests that I would run previously
continue to pass/fail as they today.

Changelog:
 v1/RFC->v2:
  - [Eric] Dropped first two patches, as they have been merged
  - [Eric] Shuffling of patches for readability/understandability
  - [Halil] Actually added meaningful comments/commit messages
    in the patches
 v1/RFC: https://patchwork.kernel.org/cover/10675251/

Eric Farman (9):
  s390/cio: Squash cp_free() and cp_unpin_free()
  s390/cio: Refactor the routine that handles TIC CCWs
  s390/cio: Generalize the TIC handler
  s390/cio: Use generalized CCW handler in cp_init()
  vfio-ccw: Rearrange pfn_array and pfn_array_table arrays
  vfio-ccw: Adjust the first IDAW outside of the nested loops
  vfio-ccw: Remove pfn_array_table
  vfio-ccw: Rearrange IDAL allocation in direct CCW
  s390/cio: Combine direct and indirect CCW paths

 drivers/s390/cio/vfio_ccw_cp.c | 313 +++++++++++----------------------
 1 file changed, 102 insertions(+), 211 deletions(-)

-- 
2.17.1

[PATCH v2 1/9] s390/cio: Squash cp_free() and cp_unpin_free()

[Eric Farman]

The routine cp_free() does nothing but call cp_unpin_free(), and while
most places call cp_free() there is one caller of cp_unpin_free() used
when the cp is guaranteed to have not been marked initialized.

This seems like a dubious way to make a distinction, so let's combine
these routines and make cp_free() do all the work.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
The RFC version of this patch received r-b's from Farhan [1] and
Pierre [2].  This patch is almost identical to that one, but I
opted to not include those tags because of the cp->initialized
check that now has an impact here.  I still think this patch makes
sense, but want them (well, Farhan) to have a chance to look it
over since it's been six or seven months.

[1] https://patchwork.kernel.org/comment/22310411/
[2] https://patchwork.kernel.org/comment/22317927/
---
 drivers/s390/cio/vfio_ccw_cp.c | 36 +++++++++++++++-------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index f73cfcfdd032..47cd7f94f42f 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -412,23 +412,6 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)
 	kfree((void *)(u64)ccw->cda);
 }
 
-/* Unpin the pages then free the memory resources. */
-static void cp_unpin_free(struct channel_program *cp)
-{
-	struct ccwchain *chain, *temp;
-	int i;
-
-	cp->initialized = false;
-	list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
-		for (i = 0; i < chain->ch_len; i++) {
-			pfn_array_table_unpin_free(chain->ch_pat + i,
-						   cp->mdev);
-			ccwchain_cda_free(chain, i);
-		}
-		ccwchain_free(chain);
-	}
-}
-
 /**
  * ccwchain_calc_length - calculate the length of the ccw chain.
  * @iova: guest physical address of the target ccw chain
@@ -796,7 +779,7 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
 	/* Now loop for its TICs. */
 	ret = ccwchain_loop_tic(chain, cp);
 	if (ret)
-		cp_unpin_free(cp);
+		cp_free(cp);
 	/* It is safe to force: if not set but idals used
 	 * ccwchain_calc_length returns an error.
 	 */
@@ -819,8 +802,21 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
  */
 void cp_free(struct channel_program *cp)
 {
-	if (cp->initialized)
-		cp_unpin_free(cp);
+	struct ccwchain *chain, *temp;
+	int i;
+
+	if (!cp->initialized)
+		return;
+
+	cp->initialized = false;
+	list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
+		for (i = 0; i < chain->ch_len; i++) {
+			pfn_array_table_unpin_free(chain->ch_pat + i,
+						   cp->mdev);
+			ccwchain_cda_free(chain, i);
+		}
+		ccwchain_free(chain);
+	}
 }
 
 /**
-- 
2.17.1

Re: [PATCH v2 1/9] s390/cio: Squash cp_free() and cp_unpin_free()

[Cornelia Huck]

On Thu,  6 Jun 2019 22:28:23 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> The routine cp_free() does nothing but call cp_unpin_free(), and while
> most places call cp_free() there is one caller of cp_unpin_free() used
> when the cp is guaranteed to have not been marked initialized.
> 
> This seems like a dubious way to make a distinction, so let's combine
> these routines and make cp_free() do all the work.

Prior to the introduction of ->initialized, cp_free() only was a
wrapper around cp_unpin_free(), which made even less sense... but
checking ->initialized does not really matter at all here.

> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> The RFC version of this patch received r-b's from Farhan [1] and
> Pierre [2].  This patch is almost identical to that one, but I
> opted to not include those tags because of the cp->initialized
> check that now has an impact here.  I still think this patch makes
> sense, but want them (well, Farhan) to have a chance to look it
> over since it's been six or seven months.
> 
> [1] https://patchwork.kernel.org/comment/22310411/
> [2] https://patchwork.kernel.org/comment/22317927/
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 36 +++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH v2 2/9] s390/cio: Refactor the routine that handles TIC CCWs

[Eric Farman]

Extract the "does the target of this TIC already exist?" check from
ccwchain_handle_tic(), so that it's easier to refactor that function
into one that cp_init() is able to use.

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 47cd7f94f42f..628daf1a8f9a 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -502,10 +502,6 @@ static int ccwchain_handle_tic(struct ccw1 *tic, struct channel_program *cp)
 	struct ccwchain *chain;
 	int len, ret;
 
-	/* May transfer to an existing chain. */
-	if (tic_target_chain_exists(tic, cp))
-		return 0;
-
 	/* Get chain length. */
 	len = ccwchain_calc_length(tic->cda, cp);
 	if (len < 0)
@@ -540,6 +536,10 @@ static int ccwchain_loop_tic(struct ccwchain *chain, struct channel_program *cp)
 		if (!ccw_is_tic(tic))
 			continue;
 
+		/* May transfer to an existing chain. */
+		if (tic_target_chain_exists(tic, cp))
+			continue;
+
 		ret = ccwchain_handle_tic(tic, cp);
 		if (ret)
 			return ret;
-- 
2.17.1

Re: [PATCH v2 2/9] s390/cio: Refactor the routine that handles TIC CCWs

[Cornelia Huck]

On Thu,  6 Jun 2019 22:28:24 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> Extract the "does the target of this TIC already exist?" check from
> ccwchain_handle_tic(), so that it's easier to refactor that function
> into one that cp_init() is able to use.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH v2 3/9] s390/cio: Generalize the TIC handler

[Eric Farman]

Refactor ccwchain_handle_tic() into a routine that handles a channel
program address (which itself is a CCW pointer), rather than a CCW pointer
that is only a TIC CCW.  This will make it easier to reuse this code for
other CCW commands.

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 628daf1a8f9a..52735cdb0270 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -497,13 +497,13 @@ static int tic_target_chain_exists(struct ccw1 *tic, struct channel_program *cp)
 static int ccwchain_loop_tic(struct ccwchain *chain,
 			     struct channel_program *cp);
 
-static int ccwchain_handle_tic(struct ccw1 *tic, struct channel_program *cp)
+static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)
 {
 	struct ccwchain *chain;
 	int len, ret;
 
 	/* Get chain length. */
-	len = ccwchain_calc_length(tic->cda, cp);
+	len = ccwchain_calc_length(cda, cp);
 	if (len < 0)
 		return len;
 
@@ -511,10 +511,10 @@ static int ccwchain_handle_tic(struct ccw1 *tic, struct channel_program *cp)
 	chain = ccwchain_alloc(cp, len);
 	if (!chain)
 		return -ENOMEM;
-	chain->ch_iova = tic->cda;
+	chain->ch_iova = cda;
 
 	/* Copy the new chain from user. */
-	ret = copy_ccw_from_iova(cp, chain->ch_ccw, tic->cda, len);
+	ret = copy_ccw_from_iova(cp, chain->ch_ccw, cda, len);
 	if (ret) {
 		ccwchain_free(chain);
 		return ret;
@@ -540,7 +540,8 @@ static int ccwchain_loop_tic(struct ccwchain *chain, struct channel_program *cp)
 		if (tic_target_chain_exists(tic, cp))
 			continue;
 
-		ret = ccwchain_handle_tic(tic, cp);
+		/* Build a ccwchain for the next segment */
+		ret = ccwchain_handle_ccw(tic->cda, cp);
 		if (ret)
 			return ret;
 	}
-- 
2.17.1

Re: [PATCH v2 3/9] s390/cio: Generalize the TIC handler

[Cornelia Huck]

On Thu,  6 Jun 2019 22:28:25 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> Refactor ccwchain_handle_tic() into a routine that handles a channel
> program address (which itself is a CCW pointer), rather than a CCW pointer
> that is only a TIC CCW.  This will make it easier to reuse this code for
> other CCW commands.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH v2 4/9] s390/cio: Use generalized CCW handler in cp_init()

[Eric Farman]

It is now pretty apparent that ccwchain_handle_ccw()
(nee ccwchain_handle_tic()) does everything that cp_init()
wants to do.

Let's remove that duplicated code from cp_init() and let
ccwchain_handle_ccw() handle it itself.

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 52735cdb0270..5b98bea433b7 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -744,9 +744,7 @@ static int ccwchain_fetch_one(struct ccwchain *chain,
  */
 int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
 {
-	u64 iova = orb->cmd.cpa;
-	struct ccwchain *chain;
-	int len, ret;
+	int ret;
 
 	/*
 	 * XXX:
@@ -759,28 +757,11 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
 	memcpy(&cp->orb, orb, sizeof(*orb));
 	cp->mdev = mdev;
 
-	/* Get chain length. */
-	len = ccwchain_calc_length(iova, cp);
-	if (len < 0)
-		return len;
-
-	/* Alloc mem for the head chain. */
-	chain = ccwchain_alloc(cp, len);
-	if (!chain)
-		return -ENOMEM;
-	chain->ch_iova = iova;
-
-	/* Copy the head chain from guest. */
-	ret = copy_ccw_from_iova(cp, chain->ch_ccw, iova, len);
-	if (ret) {
-		ccwchain_free(chain);
-		return ret;
-	}
-
-	/* Now loop for its TICs. */
-	ret = ccwchain_loop_tic(chain, cp);
+	/* Build a ccwchain for the first CCW segment */
+	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
 	if (ret)
 		cp_free(cp);
+
 	/* It is safe to force: if not set but idals used
 	 * ccwchain_calc_length returns an error.
 	 */
-- 
2.17.1

Re: [PATCH v2 4/9] s390/cio: Use generalized CCW handler in cp_init()

[Cornelia Huck]

On Thu,  6 Jun 2019 22:28:26 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> It is now pretty apparent that ccwchain_handle_ccw()
> (nee ccwchain_handle_tic()) does everything that cp_init()
> wants to do.
> 
> Let's remove that duplicated code from cp_init() and let
> ccwchain_handle_ccw() handle it itself.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 27 ++++-----------------------
>  1 file changed, 4 insertions(+), 23 deletions(-)

Nice cleanup :)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH v2 5/9] vfio-ccw: Rearrange pfn_array and pfn_array_table arrays

[Eric Farman]

While processing a channel program, we currently have two nested
arrays that carry a slightly different structure.  The direct CCW
path creates this:

  ccwchain->pfn_array_table[1]->pfn_array[#pages]

while an IDA CCW creates:

  ccwchain->pfn_array_table[#idaws]->pfn_array[1]

The distinction appears to state that each pfn_array_table entry
points to an array of contiguous pages, represented by a pfn_array,
um, array.  Since the direct-addressed scenario can ONLY represent
contiguous pages, it makes the intermediate array necessary but
difficult to recognize.  Meanwhile, since an IDAL can contain
non-contiguous pages and there is no logic in vfio-ccw to detect
adjacent IDAWs, it is the second array that is necessary but appearing
to be superfluous.

I am not aware of any documentation that states the pfn_array[] needs
to be of contiguous pages; it is just what the code does today.
I don't see any reason for this either, let's just flip the IDA
codepath around so that it generates:

  ch_pat->pfn_array_table[1]->pfn_array[#idaws]

This will bring it in line with the direct-addressed codepath,
so that we can understand the behavior of this memory regardless
of what type of CCW is being processed.  And it means the casual
observer does not need to know/care whether the pfn_array[]
represents contiguous pages or not.

NB: The existing vfio-ccw code only supports 4K-block Format-2 IDAs,
so that "#pages" == "#idaws" in this area.  This means that we will
have difficulty with this overlap in terminology if support for
Format-1 or 2K-block Format-2 IDAs is ever added.  I don't think that
this patch changes our ability to make that distinction.

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 5b98bea433b7..86a0e76ef2b5 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -635,7 +635,6 @@ 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;
@@ -656,10 +655,14 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
 
 	/* Pin data page(s) in memory. */
 	pat = chain->ch_pat + idx;
-	ret = pfn_array_table_init(pat, idaw_nr);
+	ret = pfn_array_table_init(pat, 1);
 	if (ret)
 		goto out_init;
 
+	ret = pfn_array_alloc(pat->pat_pa, idaw_iova, bytes);
+	if (ret)
+		goto out_unpin;
+
 	/* Translate idal ccw to use new allocated idaws. */
 	idaws = kzalloc(idaw_len, GFP_DMA | GFP_KERNEL);
 	if (!idaws) {
@@ -673,22 +676,15 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
 
 	ccw->cda = virt_to_phys(idaws);
 
-	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;
-
-		if (!ccw_does_data_transfer(ccw)) {
-			pa->pa_nr = 0;
-			continue;
-		}
+	for (i = 0; i < idaw_nr; i++)
+		pat->pat_pa->pa_iova_pfn[i] = idaws[i] >> PAGE_SHIFT;
 
-		ret = pfn_array_pin(pa, cp->mdev);
+	if (ccw_does_data_transfer(ccw)) {
+		ret = pfn_array_pin(pat->pat_pa, cp->mdev);
 		if (ret < 0)
 			goto out_free_idaws;
+	} else {
+		pat->pat_pa->pa_nr = 0;
 	}
 
 	pfn_array_table_idal_create_words(pat, idaws);
-- 
2.17.1

Re: [PATCH v2 5/9] vfio-ccw: Rearrange pfn_array and pfn_array_table arrays

[Cornelia Huck]

On Thu,  6 Jun 2019 22:28:27 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> While processing a channel program, we currently have two nested
> arrays that carry a slightly different structure.  The direct CCW
> path creates this:
> 
>   ccwchain->pfn_array_table[1]->pfn_array[#pages]
> 
> while an IDA CCW creates:
> 
>   ccwchain->pfn_array_table[#idaws]->pfn_array[1]
> 
> The distinction appears to state that each pfn_array_table entry
> points to an array of contiguous pages, represented by a pfn_array,
> um, array.  Since the direct-addressed scenario can ONLY represent
> contiguous pages, it makes the intermediate array necessary but
> difficult to recognize.  Meanwhile, since an IDAL can contain
> non-contiguous pages and there is no logic in vfio-ccw to detect
> adjacent IDAWs, it is the second array that is necessary but appearing
> to be superfluous.
> 
> I am not aware of any documentation that states the pfn_array[] needs
> to be of contiguous pages; it is just what the code does today.
> I don't see any reason for this either, let's just flip the IDA
> codepath around so that it generates:
> 
>   ch_pat->pfn_array_table[1]->pfn_array[#idaws]
> 
> This will bring it in line with the direct-addressed codepath,
> so that we can understand the behavior of this memory regardless
> of what type of CCW is being processed.  And it means the casual
> observer does not need to know/care whether the pfn_array[]
> represents contiguous pages or not.
> 
> NB: The existing vfio-ccw code only supports 4K-block Format-2 IDAs,
> so that "#pages" == "#idaws" in this area.  This means that we will
> have difficulty with this overlap in terminology if support for
> Format-1 or 2K-block Format-2 IDAs is ever added.  I don't think that
> this patch changes our ability to make that distinction.

I agree; and knowing that later patches will simplify things further, I
think it will even be easier to do than on the current code base.

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

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH v2 6/9] vfio-ccw: Adjust the first IDAW outside of the nested loops

[Eric Farman]

Now that pfn_array_table[] is always an array of 1, it seems silly to
check for the very first entry in an array in the middle of two nested
loops, since we know it'll only ever happen once.

Let's move this outside the loops to simplify things, even though
the "k" variable is still necessary.

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 86a0e76ef2b5..ab9f8f0d1b44 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -201,11 +201,12 @@ static inline void pfn_array_table_idal_create_words(
 		pa = pat->pat_pa + i;
 		for (j = 0; j < pa->pa_nr; j++) {
 			idaws[k] = pa->pa_pfn[j] << PAGE_SHIFT;
-			if (k == 0)
-				idaws[k] += pa->pa_iova & (PAGE_SIZE - 1);
 			k++;
 		}
 	}
+
+	/* Adjust the first IDAW, since it may not start on a page boundary */
+	idaws[0] += pat->pat_pa->pa_iova & (PAGE_SIZE - 1);
 }
 
 
-- 
2.17.1

Re: [PATCH v2 6/9] vfio-ccw: Adjust the first IDAW outside of the nested loops

[Cornelia Huck]

On Thu,  6 Jun 2019 22:28:28 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> Now that pfn_array_table[] is always an array of 1, it seems silly to
> check for the very first entry in an array in the middle of two nested
> loops, since we know it'll only ever happen once.
> 
> Let's move this outside the loops to simplify things, even though
> the "k" variable is still necessary.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 86a0e76ef2b5..ab9f8f0d1b44 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -201,11 +201,12 @@ static inline void pfn_array_table_idal_create_words(
>  		pa = pat->pat_pa + i;
>  		for (j = 0; j < pa->pa_nr; j++) {
>  			idaws[k] = pa->pa_pfn[j] << PAGE_SHIFT;
> -			if (k == 0)
> -				idaws[k] += pa->pa_iova & (PAGE_SIZE - 1);
>  			k++;
>  		}
>  	}
> +
> +	/* Adjust the first IDAW, since it may not start on a page boundary */
> +	idaws[0] += pat->pat_pa->pa_iova & (PAGE_SIZE - 1);
>  }
>  
>  

Also +1 to adding a nice explaining comment :)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH v2 7/9] vfio-ccw: Remove pfn_array_table

[Eric Farman]

Now that both CCW codepaths build this nested array:

  ccwchain->pfn_array_table[1]->pfn_array[#idaws/#pages]

We can collapse this into simply:

  ccwchain->pfn_array[#idaws/#pages]

Let's do that, so that we don't have to continually navigate two
nested arrays when the first array always has a count of one.

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index ab9f8f0d1b44..76ffcc823944 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -33,11 +33,6 @@ struct pfn_array {
 	int			pa_nr;
 };
 
-struct pfn_array_table {
-	struct pfn_array	*pat_pa;
-	int			pat_nr;
-};
-
 struct ccwchain {
 	struct list_head	next;
 	struct ccw1		*ch_ccw;
@@ -46,7 +41,7 @@ struct ccwchain {
 	/* Count of the valid ccws in chain. */
 	int			ch_len;
 	/* Pinned PAGEs for the original data. */
-	struct pfn_array_table	*ch_pat;
+	struct pfn_array	*ch_pa;
 };
 
 /*
@@ -139,55 +134,23 @@ static void pfn_array_unpin_free(struct pfn_array *pa, struct device *mdev)
 	kfree(pa->pa_iova_pfn);
 }
 
-static int pfn_array_table_init(struct pfn_array_table *pat, int nr)
-{
-	pat->pat_pa = kcalloc(nr, sizeof(*pat->pat_pa), GFP_KERNEL);
-	if (unlikely(ZERO_OR_NULL_PTR(pat->pat_pa))) {
-		pat->pat_nr = 0;
-		return -ENOMEM;
-	}
-
-	pat->pat_nr = nr;
-
-	return 0;
-}
-
-static void pfn_array_table_unpin_free(struct pfn_array_table *pat,
-				       struct device *mdev)
-{
-	int i;
-
-	for (i = 0; i < pat->pat_nr; i++)
-		pfn_array_unpin_free(pat->pat_pa + i, mdev);
-
-	if (pat->pat_nr) {
-		kfree(pat->pat_pa);
-		pat->pat_pa = NULL;
-		pat->pat_nr = 0;
-	}
-}
-
-static bool pfn_array_table_iova_pinned(struct pfn_array_table *pat,
-					unsigned long iova)
+static bool pfn_array_iova_pinned(struct pfn_array *pa, unsigned long iova)
 {
-	struct pfn_array *pa = pat->pat_pa;
 	unsigned long iova_pfn = iova >> PAGE_SHIFT;
-	int i, j;
+	int i;
 
-	for (i = 0; i < pat->pat_nr; i++, pa++)
-		for (j = 0; j < pa->pa_nr; j++)
-			if (pa->pa_iova_pfn[j] == iova_pfn)
-				return true;
+	for (i = 0; i < pa->pa_nr; i++)
+		if (pa->pa_iova_pfn[i] == iova_pfn)
+			return true;
 
 	return false;
 }
-/* Create the list idal words for a pfn_array_table. */
-static inline void pfn_array_table_idal_create_words(
-	struct pfn_array_table *pat,
+/* Create the list of IDAL words for a pfn_array. */
+static inline void pfn_array_idal_create_words(
+	struct pfn_array *pa,
 	unsigned long *idaws)
 {
-	struct pfn_array *pa;
-	int i, j, k;
+	int i;
 
 	/*
 	 * Idal words (execept the first one) rely on the memory being 4k
@@ -196,17 +159,12 @@ static inline void pfn_array_table_idal_create_words(
 	 * there will be no problem here to simply use the phys to create an
 	 * idaw.
 	 */
-	k = 0;
-	for (i = 0; i < pat->pat_nr; i++) {
-		pa = pat->pat_pa + i;
-		for (j = 0; j < pa->pa_nr; j++) {
-			idaws[k] = pa->pa_pfn[j] << PAGE_SHIFT;
-			k++;
-		}
-	}
+
+	for (i = 0; i < pa->pa_nr; i++)
+		idaws[i] = pa->pa_pfn[i] << PAGE_SHIFT;
 
 	/* Adjust the first IDAW, since it may not start on a page boundary */
-	idaws[0] += pat->pat_pa->pa_iova & (PAGE_SIZE - 1);
+	idaws[0] += pa->pa_iova & (PAGE_SIZE - 1);
 }
 
 
@@ -378,7 +336,7 @@ static struct ccwchain *ccwchain_alloc(struct channel_program *cp, int len)
 	/* Make ccw address aligned to 8. */
 	size = ((sizeof(*chain) + 7L) & -8L) +
 		sizeof(*chain->ch_ccw) * len +
-		sizeof(*chain->ch_pat) * len;
+		sizeof(*chain->ch_pa) * len;
 	chain = kzalloc(size, GFP_DMA | GFP_KERNEL);
 	if (!chain)
 		return NULL;
@@ -387,7 +345,7 @@ static struct ccwchain *ccwchain_alloc(struct channel_program *cp, int len)
 	chain->ch_ccw = (struct ccw1 *)data;
 
 	data = (u8 *)(chain->ch_ccw) + sizeof(*chain->ch_ccw) * len;
-	chain->ch_pat = (struct pfn_array_table *)data;
+	chain->ch_pa = (struct pfn_array *)data;
 
 	chain->ch_len = len;
 
@@ -575,7 +533,7 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 				 struct channel_program *cp)
 {
 	struct ccw1 *ccw;
-	struct pfn_array_table *pat;
+	struct pfn_array *pa;
 	unsigned long *idaws;
 	int ret;
 	int bytes = 1;
@@ -593,21 +551,17 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	 * The number of pages actually is the count of the idaws which will be
 	 * needed when translating a direct ccw to a idal ccw.
 	 */
-	pat = chain->ch_pat + idx;
-	ret = pfn_array_table_init(pat, 1);
-	if (ret)
-		goto out_init;
-
-	ret = pfn_array_alloc(pat->pat_pa, ccw->cda, bytes);
+	pa = chain->ch_pa + idx;
+	ret = pfn_array_alloc(pa, ccw->cda, bytes);
 	if (ret < 0)
 		goto out_unpin;
 
 	if (ccw_does_data_transfer(ccw)) {
-		ret = pfn_array_pin(pat->pat_pa, cp->mdev);
+		ret = pfn_array_pin(pa, cp->mdev);
 		if (ret < 0)
 			goto out_unpin;
 	} else {
-		pat->pat_pa->pa_nr = 0;
+		pa->pa_nr = 0;
 	}
 
 	/* Translate this direct ccw to a idal ccw. */
@@ -619,12 +573,12 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	ccw->cda = (__u32) virt_to_phys(idaws);
 	ccw->flags |= CCW_FLAG_IDA;
 
-	pfn_array_table_idal_create_words(pat, idaws);
+	pfn_array_idal_create_words(pa, idaws);
 
 	return 0;
 
 out_unpin:
-	pfn_array_table_unpin_free(pat, cp->mdev);
+	pfn_array_unpin_free(pa, cp->mdev);
 out_init:
 	ccw->cda = 0;
 	return ret;
@@ -635,7 +589,7 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
 			       struct channel_program *cp)
 {
 	struct ccw1 *ccw;
-	struct pfn_array_table *pat;
+	struct pfn_array *pa;
 	unsigned long *idaws;
 	u64 idaw_iova;
 	unsigned int idaw_nr, idaw_len;
@@ -655,15 +609,11 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
 	idaw_len = idaw_nr * sizeof(*idaws);
 
 	/* Pin data page(s) in memory. */
-	pat = chain->ch_pat + idx;
-	ret = pfn_array_table_init(pat, 1);
+	pa = chain->ch_pa + idx;
+	ret = pfn_array_alloc(pa, idaw_iova, bytes);
 	if (ret)
 		goto out_init;
 
-	ret = pfn_array_alloc(pat->pat_pa, idaw_iova, bytes);
-	if (ret)
-		goto out_unpin;
-
 	/* Translate idal ccw to use new allocated idaws. */
 	idaws = kzalloc(idaw_len, GFP_DMA | GFP_KERNEL);
 	if (!idaws) {
@@ -678,24 +628,24 @@ static int ccwchain_fetch_idal(struct ccwchain *chain,
 	ccw->cda = virt_to_phys(idaws);
 
 	for (i = 0; i < idaw_nr; i++)
-		pat->pat_pa->pa_iova_pfn[i] = idaws[i] >> PAGE_SHIFT;
+		pa->pa_iova_pfn[i] = idaws[i] >> PAGE_SHIFT;
 
 	if (ccw_does_data_transfer(ccw)) {
-		ret = pfn_array_pin(pat->pat_pa, cp->mdev);
+		ret = pfn_array_pin(pa, cp->mdev);
 		if (ret < 0)
 			goto out_free_idaws;
 	} else {
-		pat->pat_pa->pa_nr = 0;
+		pa->pa_nr = 0;
 	}
 
-	pfn_array_table_idal_create_words(pat, idaws);
+	pfn_array_idal_create_words(pa, idaws);
 
 	return 0;
 
 out_free_idaws:
 	kfree(idaws);
 out_unpin:
-	pfn_array_table_unpin_free(pat, cp->mdev);
+	pfn_array_unpin_free(pa, cp->mdev);
 out_init:
 	ccw->cda = 0;
 	return ret;
@@ -790,8 +740,7 @@ void cp_free(struct channel_program *cp)
 	cp->initialized = false;
 	list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
 		for (i = 0; i < chain->ch_len; i++) {
-			pfn_array_table_unpin_free(chain->ch_pat + i,
-						   cp->mdev);
+			pfn_array_unpin_free(chain->ch_pa + i, cp->mdev);
 			ccwchain_cda_free(chain, i);
 		}
 		ccwchain_free(chain);
@@ -967,8 +916,7 @@ bool cp_iova_pinned(struct channel_program *cp, u64 iova)
 
 	list_for_each_entry(chain, &cp->ccwchain_list, next) {
 		for (i = 0; i < chain->ch_len; i++)
-			if (pfn_array_table_iova_pinned(chain->ch_pat + i,
-							iova))
+			if (pfn_array_iova_pinned(chain->ch_pa + i, iova))
 				return true;
 	}
 
-- 
2.17.1

Re: [PATCH v2 7/9] vfio-ccw: Remove pfn_array_table

[Cornelia Huck]

On Thu,  6 Jun 2019 22:28:29 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> Now that both CCW codepaths build this nested array:
> 
>   ccwchain->pfn_array_table[1]->pfn_array[#idaws/#pages]
> 
> We can collapse this into simply:
> 
>   ccwchain->pfn_array[#idaws/#pages]
> 
> Let's do that, so that we don't have to continually navigate two
> nested arrays when the first array always has a count of one.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 118 +++++++++------------------------
>  1 file changed, 33 insertions(+), 85 deletions(-)

That's how I like my diffstats :) Nice cleanup!

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH v2 8/9] vfio-ccw: Rearrange IDAL allocation in direct CCW

[Eric Farman]

This is purely deck furniture, to help understand the merge of the
direct and indirect handlers.

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 76ffcc823944..8205d0b527fc 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -537,13 +537,21 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	unsigned long *idaws;
 	int ret;
 	int bytes = 1;
-	int idaw_nr = 1;
+	int idaw_nr;
 
 	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);
+
+	/* Calculate size of IDAL */
+	idaw_nr = idal_nr_words((void *)(u64)ccw->cda, bytes);
+
+	/* Allocate an IDAL from host storage */
+	idaws = kcalloc(idaw_nr, sizeof(*idaws), GFP_DMA | GFP_KERNEL);
+	if (!idaws) {
+		ret = -ENOMEM;
+		goto out_init;
 	}
 
 	/*
@@ -554,7 +562,7 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	pa = chain->ch_pa + idx;
 	ret = pfn_array_alloc(pa, ccw->cda, bytes);
 	if (ret < 0)
-		goto out_unpin;
+		goto out_free_idaws;
 
 	if (ccw_does_data_transfer(ccw)) {
 		ret = pfn_array_pin(pa, cp->mdev);
@@ -564,21 +572,18 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 		pa->pa_nr = 0;
 	}
 
-	/* Translate this direct ccw to a idal ccw. */
-	idaws = kcalloc(idaw_nr, sizeof(*idaws), GFP_DMA | GFP_KERNEL);
-	if (!idaws) {
-		ret = -ENOMEM;
-		goto out_unpin;
-	}
 	ccw->cda = (__u32) virt_to_phys(idaws);
 	ccw->flags |= CCW_FLAG_IDA;
 
+	/* Populate the IDAL with pinned/translated addresses from pfn */
 	pfn_array_idal_create_words(pa, idaws);
 
 	return 0;
 
 out_unpin:
 	pfn_array_unpin_free(pa, cp->mdev);
+out_free_idaws:
+	kfree(idaws);
 out_init:
 	ccw->cda = 0;
 	return ret;
-- 
2.17.1

Re: [PATCH v2 8/9] vfio-ccw: Rearrange IDAL allocation in direct CCW

[Cornelia Huck]

On Thu,  6 Jun 2019 22:28:30 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> This is purely deck furniture, to help understand the merge of the
> direct and indirect handlers.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH v2 9/9] s390/cio: Combine direct and indirect CCW paths

[Eric Farman]

With both the direct-addressed and indirect-addressed CCW paths
simplified to this point, the amount of shared code between them is
(hopefully) more easily visible.  Move the processing of IDA-specific
bits into the direct-addressed path, and add some useful commentary of
what the individual pieces are doing.  This allows us to remove the
entire ccwchain_fetch_idal() routine and maintain a single function
for any non-TIC CCW.

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 8205d0b527fc..90d86e1354c1 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -534,10 +534,12 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 {
 	struct ccw1 *ccw;
 	struct pfn_array *pa;
+	u64 iova;
 	unsigned long *idaws;
 	int ret;
 	int bytes = 1;
-	int idaw_nr;
+	int idaw_nr, idal_len;
+	int i;
 
 	ccw = chain->ch_ccw + idx;
 
@@ -545,7 +547,17 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 		bytes = ccw->count;
 
 	/* Calculate size of IDAL */
-	idaw_nr = idal_nr_words((void *)(u64)ccw->cda, bytes);
+	if (ccw_is_idal(ccw)) {
+		/* Read first IDAW to see if it's 4K-aligned or not. */
+		/* All subsequent IDAws will be 4K-aligned. */
+		ret = copy_from_iova(cp->mdev, &iova, ccw->cda, sizeof(iova));
+		if (ret)
+			return ret;
+	} else {
+		iova = ccw->cda;
+	}
+	idaw_nr = idal_nr_words((void *)iova, bytes);
+	idal_len = idaw_nr * sizeof(*idaws);
 
 	/* Allocate an IDAL from host storage */
 	idaws = kcalloc(idaw_nr, sizeof(*idaws), GFP_DMA | GFP_KERNEL);
@@ -555,15 +567,36 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	}
 
 	/*
-	 * Pin data page(s) in memory.
-	 * The number of pages actually is the count of the idaws which will be
-	 * needed when translating a direct ccw to a idal ccw.
+	 * Allocate an array of pfn's for pages to pin/translate.
+	 * The number of pages is actually the count of the idaws
+	 * required for the data transfer, since we only only support
+	 * 4K IDAWs today.
 	 */
 	pa = chain->ch_pa + idx;
-	ret = pfn_array_alloc(pa, ccw->cda, bytes);
+	ret = pfn_array_alloc(pa, iova, bytes);
 	if (ret < 0)
 		goto out_free_idaws;
 
+	if (ccw_is_idal(ccw)) {
+		/* Copy guest IDAL into host IDAL */
+		ret = copy_from_iova(cp->mdev, idaws, ccw->cda, idal_len);
+		if (ret)
+			goto out_unpin;
+
+		/*
+		 * Copy guest IDAWs into pfn_array, in case the memory they
+		 * occupy is not contiguous.
+		 */
+		for (i = 0; i < idaw_nr; i++)
+			pa->pa_iova_pfn[i] = idaws[i] >> PAGE_SHIFT;
+	} else {
+		/*
+		 * No action is required here; the iova addresses in pfn_array
+		 * were initialized sequentially in pfn_array_alloc() beginning
+		 * with the contents of ccw->cda.
+		 */
+	}
+
 	if (ccw_does_data_transfer(ccw)) {
 		ret = pfn_array_pin(pa, cp->mdev);
 		if (ret < 0)
@@ -589,73 +622,6 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
 	return ret;
 }
 
-static int ccwchain_fetch_idal(struct ccwchain *chain,
-			       int idx,
-			       struct channel_program *cp)
-{
-	struct ccw1 *ccw;
-	struct pfn_array *pa;
-	unsigned long *idaws;
-	u64 idaw_iova;
-	unsigned int idaw_nr, idaw_len;
-	int i, ret;
-	int bytes = 1;
-
-	ccw = chain->ch_ccw + idx;
-
-	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), bytes);
-	idaw_len = idaw_nr * sizeof(*idaws);
-
-	/* Pin data page(s) in memory. */
-	pa = chain->ch_pa + idx;
-	ret = pfn_array_alloc(pa, idaw_iova, bytes);
-	if (ret)
-		goto out_init;
-
-	/* Translate idal ccw to use new allocated idaws. */
-	idaws = kzalloc(idaw_len, GFP_DMA | GFP_KERNEL);
-	if (!idaws) {
-		ret = -ENOMEM;
-		goto out_unpin;
-	}
-
-	ret = copy_from_iova(cp->mdev, idaws, ccw->cda, idaw_len);
-	if (ret)
-		goto out_free_idaws;
-
-	ccw->cda = virt_to_phys(idaws);
-
-	for (i = 0; i < idaw_nr; i++)
-		pa->pa_iova_pfn[i] = idaws[i] >> PAGE_SHIFT;
-
-	if (ccw_does_data_transfer(ccw)) {
-		ret = pfn_array_pin(pa, cp->mdev);
-		if (ret < 0)
-			goto out_free_idaws;
-	} else {
-		pa->pa_nr = 0;
-	}
-
-	pfn_array_idal_create_words(pa, idaws);
-
-	return 0;
-
-out_free_idaws:
-	kfree(idaws);
-out_unpin:
-	pfn_array_unpin_free(pa, cp->mdev);
-out_init:
-	ccw->cda = 0;
-	return ret;
-}
-
 /*
  * Fetch one ccw.
  * To reduce memory copy, we'll pin the cda page in memory,
@@ -671,9 +637,6 @@ static int ccwchain_fetch_one(struct ccwchain *chain,
 	if (ccw_is_tic(ccw))
 		return ccwchain_fetch_tic(chain, idx, cp);
 
-	if (ccw_is_idal(ccw))
-		return ccwchain_fetch_idal(chain, idx, cp);
-
 	return ccwchain_fetch_direct(chain, idx, cp);
 }
 
-- 
2.17.1

Re: [PATCH v2 9/9] s390/cio: Combine direct and indirect CCW paths

[Cornelia Huck]

On Thu,  6 Jun 2019 22:28:31 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> With both the direct-addressed and indirect-addressed CCW paths
> simplified to this point, the amount of shared code between them is
> (hopefully) more easily visible.  Move the processing of IDA-specific
> bits into the direct-addressed path, and add some useful commentary of
> what the individual pieces are doing.  This allows us to remove the
> entire ccwchain_fetch_idal() routine and maintain a single function
> for any non-TIC CCW.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 115 +++++++++++----------------------
>  1 file changed, 39 insertions(+), 76 deletions(-)

Another nice cleanup :)

> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 8205d0b527fc..90d86e1354c1 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -534,10 +534,12 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,

The one minor thing I have is that the function name
(ccwchain_fetch_direct) is now slightly confusing. But we can easily do
a patch on top renaming it (if we can come up with a better name.)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v2 9/9] s390/cio: Combine direct and indirect CCW paths

[Eric Farman]

On 6/14/19 6:01 AM, Cornelia Huck wrote:
> On Thu,  6 Jun 2019 22:28:31 +0200
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> With both the direct-addressed and indirect-addressed CCW paths
>> simplified to this point, the amount of shared code between them is
>> (hopefully) more easily visible.  Move the processing of IDA-specific
>> bits into the direct-addressed path, and add some useful commentary of
>> what the individual pieces are doing.  This allows us to remove the
>> entire ccwchain_fetch_idal() routine and maintain a single function
>> for any non-TIC CCW.
>>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>  drivers/s390/cio/vfio_ccw_cp.c | 115 +++++++++++----------------------
>>  1 file changed, 39 insertions(+), 76 deletions(-)
> 
> Another nice cleanup :)

Thanks!  This one makes me feel warm and fuzzy having only CCW processor
to manage in the future.

> 
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>> index 8205d0b527fc..90d86e1354c1 100644
>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>> @@ -534,10 +534,12 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
> 
> The one minor thing I have is that the function name
> (ccwchain_fetch_direct) is now slightly confusing. But we can easily do
> a patch on top renaming it (if we can come up with a better name.)

Agreed!  Maybe just ccwchain_fetch() ?  Or perhaps ccwchain_fetch_ccw()
if that won't cause too much confusion with the ccwchain_handle_ccw()
called from cp_init().

> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks for all of these!  :)

Re: [PATCH v2 0/9] s390: vfio-ccw code rework

[Cornelia Huck]

On Thu,  6 Jun 2019 22:28:22 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> Now that we've gotten a lot of other series either merged or
> pending for the next merge window, I'd like to revisit some
> code simplification that I started many moons ago.
> 
> In that first series, a couple of fixes got merged into 4.20,
> a couple more got some "seems okay" acks/reviews, and the rest
> were nearly forgotten about.  I dusted them off and did quite a
> bit of rework to make things a little more sequential and
> providing a better narrative (I think) based on the lessons we
> learned in my earlier changes.  Because of this rework, the
> acks/reviews on the first version didn't really translate to the
> code that exists here (patch 1 being the closest exception), so
> I didn't apply any of them here.  The end result is mostly the
> same as before, but now looks like this:
> 
> Patch summary:
> 1:   Squash duplicate code
> 2-4: Remove duplicate code in CCW processor
> 5-7: Remove one layer of nested arrays
> 8-9: Combine direct/indirect addressing CCW processors
> 
> Using 5.2.0-rc3 as a base plus the vfio-ccw branch of recent fixes,
> we shrink the code quite a bit (8.7% according to the bloat-o-meter),
> and we remove one set of mallocs/frees on the I/O path by removing
> one layer of the nested arrays.  There are no functional/behavioral
> changes with this series; all the tests that I would run previously
> continue to pass/fail as they today.

Very nice cleanup!

All the patches look good to me; I'll wait if anyone else has any
comments and will probably pick them next week if nobody objects.

Re: [PATCH v2 0/9] s390: vfio-ccw code rework

[Cornelia Huck]

On Thu,  6 Jun 2019 22:28:22 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> Now that we've gotten a lot of other series either merged or
> pending for the next merge window, I'd like to revisit some
> code simplification that I started many moons ago.
> 
> In that first series, a couple of fixes got merged into 4.20,
> a couple more got some "seems okay" acks/reviews, and the rest
> were nearly forgotten about.  I dusted them off and did quite a
> bit of rework to make things a little more sequential and
> providing a better narrative (I think) based on the lessons we
> learned in my earlier changes.  Because of this rework, the
> acks/reviews on the first version didn't really translate to the
> code that exists here (patch 1 being the closest exception), so
> I didn't apply any of them here.  The end result is mostly the
> same as before, but now looks like this:
> 
> Patch summary:
> 1:   Squash duplicate code
> 2-4: Remove duplicate code in CCW processor
> 5-7: Remove one layer of nested arrays
> 8-9: Combine direct/indirect addressing CCW processors
> 
> Using 5.2.0-rc3 as a base plus the vfio-ccw branch of recent fixes,
> we shrink the code quite a bit (8.7% according to the bloat-o-meter),
> and we remove one set of mallocs/frees on the I/O path by removing
> one layer of the nested arrays.  There are no functional/behavioral
> changes with this series; all the tests that I would run previously
> continue to pass/fail as they today.
> 
> Changelog:
>  v1/RFC->v2:
>   - [Eric] Dropped first two patches, as they have been merged
>   - [Eric] Shuffling of patches for readability/understandability
>   - [Halil] Actually added meaningful comments/commit messages
>     in the patches
>  v1/RFC: https://patchwork.kernel.org/cover/10675251/
> 
> Eric Farman (9):
>   s390/cio: Squash cp_free() and cp_unpin_free()
>   s390/cio: Refactor the routine that handles TIC CCWs
>   s390/cio: Generalize the TIC handler
>   s390/cio: Use generalized CCW handler in cp_init()
>   vfio-ccw: Rearrange pfn_array and pfn_array_table arrays
>   vfio-ccw: Adjust the first IDAW outside of the nested loops
>   vfio-ccw: Remove pfn_array_table
>   vfio-ccw: Rearrange IDAL allocation in direct CCW
>   s390/cio: Combine direct and indirect CCW paths
> 
>  drivers/s390/cio/vfio_ccw_cp.c | 313 +++++++++++----------------------
>  1 file changed, 102 insertions(+), 211 deletions(-)
> 

Thanks, applied.