s390: vfio-ccw code rework

PATCH Applied Eric Farman 2 participants 22 messages Jun 6, 2019 – Jun 17, 2019

Subsystems (4)

KVM s390 s390 common I/O layer s390 VFIO-CCW driver

lore

Share & tools

lore

Eric Farman

author maintainer

2019-06-06 20:28 UTC #0 lore raw

v2 cover s390: vfio-ccw code rework

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

Signature · 1 line

2.17.1

Eric Farman

author maintainer

2019-06-06 20:28 UTC #1 lore raw

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

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

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

drivers/s390/cio/vfio_ccw_cp.c +16 ~~-20~~

index f73cfcfdd032..47cd7f94f42f 100644
@@ -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);
+	}
 }
 
 /**

Signature · 1 line

2.17.1

Cornelia Huck

Reviewed-by on v2 1/9

2019-06-12 11:07 UTC #2 lore raw

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

Quoted 6 lines · On Thu, 6 Jun 2019 22:28:23 +0200 Eric Farman <[email protected]> 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.

Quoted 10 lines

Signed-off-by Eric Farman <[email protected]>
---
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.

Quoted 5 lines

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

Eric Farman

author maintainer

2019-06-06 20:28 UTC #3 lore raw

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

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

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

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

index 47cd7f94f42f..628daf1a8f9a 100644
@@ -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;

Signature · 1 line

2.17.1

Cornelia Huck

Reviewed-by on v2 2/9

2019-06-12 11:17 UTC #4 lore raw

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

Quoted 8 lines · On Thu, 6 Jun 2019 22:28:24 +0200 Eric Farman <[email protected]> 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 <[email protected]>
---
 drivers/s390/cio/vfio_ccw_cp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by Cornelia Huck <[email protected]>

Eric Farman

author maintainer

2019-06-06 20:28 UTC #5 lore raw

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

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

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

drivers/s390/cio/vfio_ccw_cp.c +6 -5

index 628daf1a8f9a..52735cdb0270 100644
@@ -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;
 	}

Signature · 1 line

2.17.1

Cornelia Huck

Reviewed-by on v2 3/9

2019-06-12 11:18 UTC #6 lore raw

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

Quoted 10 lines · On Thu, 6 Jun 2019 22:28:25 +0200 Eric Farman <[email protected]> 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 <[email protected]>
---
 drivers/s390/cio/vfio_ccw_cp.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Reviewed-by Cornelia Huck <[email protected]>

Eric Farman

author maintainer

2019-06-06 20:28 UTC #7 lore raw

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

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

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

drivers/s390/cio/vfio_ccw_cp.c +4 ~~-23~~

index 52735cdb0270..5b98bea433b7 100644
@@ -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.
 	 */

Signature · 1 line

2.17.1

Cornelia Huck

Reviewed-by on v2 4/9

2019-06-12 11:18 UTC #8 lore raw

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

Quoted 11 lines · On Thu, 6 Jun 2019 22:28:26 +0200 Eric Farman <[email protected]> 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 <[email protected]>
---
 drivers/s390/cio/vfio_ccw_cp.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

Nice cleanup :)

Reviewed-by Cornelia Huck <[email protected]>

Eric Farman

author maintainer

2019-06-06 20:28 UTC #9 lore raw

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

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

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

drivers/s390/cio/vfio_ccw_cp.c +11 ~~-15~~

index 5b98bea433b7..86a0e76ef2b5 100644
@@ -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);

Signature · 1 line

2.17.1

Cornelia Huck

Reviewed-by on v2 5/9

2019-06-13 16:02 UTC #10 lore raw

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

Quoted 32 lines · On Thu, 6 Jun 2019 22:28:27 +0200 Eric Farman <[email protected]> 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.

Quoted 5 lines

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.

Quoted 5 lines

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

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

Reviewed-by Cornelia Huck <[email protected]>

Eric Farman

author maintainer

2019-06-06 20:28 UTC #11 lore raw

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

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

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

drivers/s390/cio/vfio_ccw_cp.c +3 -2

index 86a0e76ef2b5..ab9f8f0d1b44 100644
@@ -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);
 }

Signature · 1 line

2.17.1

Cornelia Huck

Reviewed-by on v2 6/9

2019-06-13 16:02 UTC #12 lore raw

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

Quoted 21 lines · On Thu, 6 Jun 2019 22:28:28 +0200 Eric Farman <[email protected]> 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 <[email protected]>
---
 drivers/s390/cio/vfio_ccw_cp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/s390/cio/vfio_ccw_cp.c +0 -1
index 86a0e76ef2b5..ab9f8f0d1b44 100644
@@ -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)

drivers/s390/cio/vfio_ccw_cp.c static inline void pfn_array_table_idal_create_words( · line 212

Quoted 10 lines

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

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

Eric Farman

author maintainer

2019-06-06 20:28 UTC #13 lore raw

PATCH v2 7/9 vfio-ccw: Remove pfn_array_table

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

---
 drivers/s390/cio/vfio_ccw_cp.c | 118 +++++++++------------------------
 1 file changed, 33 insertions(+), 85 deletions(-)

drivers/s390/cio/vfio_ccw_cp.c +33 ~~-85~~

index ab9f8f0d1b44..76ffcc823944 100644
@@ -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;
 	}

Signature · 1 line

2.17.1

Cornelia Huck

Reviewed-by on v2 7/9

2019-06-13 16:03 UTC #14 lore raw

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

Quoted 8 lines · On Thu, 6 Jun 2019 22:28:29 +0200 Eric Farman <[email protected]> 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]

Quoted 7 lines

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

Eric Farman

author maintainer

2019-06-06 20:28 UTC #15 lore raw

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

In reply to Eric Farman

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

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

---
 drivers/s390/cio/vfio_ccw_cp.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

drivers/s390/cio/vfio_ccw_cp.c +15 ~~-10~~

index 76ffcc823944..8205d0b527fc 100644
@@ -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;

Signature · 1 line

2.17.1

Cornelia Huck

Reviewed-by on v2 8/9

2019-06-14 09:42 UTC #16 lore raw

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

Quoted 7 lines · On Thu, 6 Jun 2019 22:28:30 +0200 Eric Farman <[email protected]> wrote:

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

Reviewed-by Cornelia Huck <[email protected]>

Eric Farman

author maintainer

2019-06-06 20:28 UTC #17 lore raw

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

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

---
 drivers/s390/cio/vfio_ccw_cp.c | 115 +++++++++++----------------------
 1 file changed, 39 insertions(+), 76 deletions(-)

drivers/s390/cio/vfio_ccw_cp.c +39 ~~-76~~

index 8205d0b527fc..90d86e1354c1 100644
@@ -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);
 }

Signature · 1 line

2.17.1

Cornelia Huck

Reviewed-by on v2 9/9

2019-06-14 10:01 UTC #18 lore raw

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

Quoted 8 lines · On Thu, 6 Jun 2019 22:28:31 +0200 Eric Farman <[email protected]> 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.

Quoted 4 lines

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

---
 drivers/s390/cio/vfio_ccw_cp.c | 115 +++++++++++----------------------
 1 file changed, 39 insertions(+), 76 deletions(-)

Another nice cleanup :)

drivers/s390/cio/vfio_ccw_cp.c static int ccwchain_fetch_direct(struct ccwchain *chain,

Quoted 6 lines

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

index 8205d0b527fc..90d86e1354c1 100644
@@ -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 <[email protected]>

Eric Farman

author maintainer

2019-06-14 10:30 UTC #19 lore raw

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

Quoted 11 lines · Cornelia Huck

Quoted 8 lines · On Thu, 6 Jun 2019 22:28:31 +0200 Eric Farman <[email protected]> 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.

Quoted 6 lines

Quoted 4 lines
Signed-off-by Eric Farman <[email protected]>
---
 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.

Quoted 11 lines

drivers/s390/cio/vfio_ccw_cp.c static int ccwchain_fetch_direct(struct ccwchain *chain,
Quoted 7 lines
drivers/s390/cio/vfio_ccw_cp.c +0 -0
index 8205d0b527fc..90d86e1354c1 100644
@@ -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().

Quoted 3 lines

Reviewed-by Cornelia Huck <[email protected]>

Thanks for all of these! :)

Cornelia Huck

2019-06-14 10:03 UTC #20 lore raw

In reply to Eric Farman

Quoted 21 lines · On Thu, 6 Jun 2019 22:28:22 +0200 Eric Farman <[email protected]> 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

Quoted 6 lines

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.

Cornelia Huck

Applied

2019-06-17 13:32 UTC #21 lore raw

In reply to Eric Farman

Quoted 47 lines · On Thu, 6 Jun 2019 22:28:22 +0200 Eric Farman <[email protected]> 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

Quoted 3 lines

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

Thanks, applied.