s390: more vfio-ccw code rework
Subsystems (4)
Eric Farman
maintainercover s390: more vfio-ccw code rework
A couple little improvements to the malloc load in vfio-ccw. Really, there were just (the first) two patches, but then I got excited and added a few stylistic ones to the end.
The routine ccwchain_calc_length() has this basic structure:
ccwchain_calc_length
a0 = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1))
copy_ccw_from_iova(a0, src)
copy_from_iova
pfn_array_alloc
b = kcalloc(len, sizeof(*pa_iova_pfn + *pa_pfn)
pfn_array_pin
vfio_pin_pages
memcpy(a0, src)
pfn_array_unpin_free
vfio_unpin_pages
kfree(b)
kfree(a0)We do this EVERY time we process a new channel program chain, meaning at least once per SSCH and more if TICs are involved, to figure out how many CCWs are chained together. Once that is determined, a new piece of memory is allocated (call it a1) and then passed to copy_ccw_from_iova() again, but for the value calculated by ccwchain_calc_length().
This seems inefficient.
Patch 1 moves the malloc of a0 from the CCW processor to the initialization of the device. Since only one SSCH can be handled concurrently, we can use this space safely to determine how long the chain being processed actually is.
Patch 2 then removes the second copy_ccw_from_iova() call entirely, and replaces it with a memcpy from a0 to a1. This is done before we process a TIC and thus a second chain, so there is no overlap in the storage in channel_program.
Patches 3-5 clean up some things that aren't as clear as I'd like, but didn't want to pollute the first two changes. For example, patch 3 moves the population of guest_cp to the same routine that copies from it, rather than in a called function. Meanwhile, patch 4 (and thus, 5) was something I had lying around for quite some time, because it looked to be structured weird. Maybe that's one bridge too far.
Eric Farman (5):
vfio-ccw: Move guest_cp storage into common struct
vfio-ccw: Skip second copy of guest cp to host
vfio-ccw: Copy CCW data outside length calculation
vfio-ccw: Factor out the ccw0-to-ccw1 transition
vfio-ccw: Remove copy_ccw_from_iova()
drivers/s390/cio/vfio_ccw_cp.c | 108 +++++++++++--------------------- drivers/s390/cio/vfio_ccw_cp.h | 7 +++ drivers/s390/cio/vfio_ccw_drv.c | 7 +++ 3 files changed, 52 insertions(+), 70 deletions(-)
Signature · 1 line
2.17.1
Eric Farman
maintainerRFC PATCH 1/5 vfio-ccw: Move guest_cp storage into common struct
Rather than allocating/freeing a piece of memory every time we try to figure out how long a CCW chain is, let's use a piece of memory allocated for each device.
The io_mutex added with commit 4f76617378ee9 ("vfio-ccw: protect the I/O region") is held for the duration of the VFIO_CCW_EVENT_IO_REQ event that accesses/uses this space, so there should be no race concerns with another CPU attempting an (unexpected) SSCH for the same device.
---
Conny, your suggestion [1] did not go unnoticed. :)
[1] https://patchwork.kernel.org/comment/22312659/
--- drivers/s390/cio/vfio_ccw_cp.c | 23 ++++------------------- drivers/s390/cio/vfio_ccw_cp.h | 7 +++++++ drivers/s390/cio/vfio_ccw_drv.c | 7 +++++++ 3 files changed, 18 insertions(+), 19 deletions(-)
drivers/s390/cio/vfio_ccw_cp.c +4 -19
@@ -16,12 +16,6 @@#include "vfio_ccw_cp.h" -/* -* Max length for ccw chain. -* XXX: Limit to 256, need to check more? -*/ -#define CCWCHAIN_LEN_MAX 256 -struct pfn_array {/* Starting guest physical I/O address. */ unsigned long pa_iova;
@@ -386,7 +380,7 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)*/ static int ccwchain_calc_length (u64 iova,struct channel_program *cp) { -struct ccw1 *ccw, *p; +struct ccw1 *ccw = cp->guest_cp;int cnt;/*
@@ -394,15 +388,9 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)* Currently the chain length is limited to CCWCHAIN_LEN_MAX (256). * So copying 2K is enough (safe). */ - p = ccw =kcalloc (CCWCHAIN_LEN_MAX ,sizeof (*ccw),GFP_KERNEL ); -if (!ccw) -return -ENOMEM ; - cnt =copy_ccw_from_iova (cp, ccw, iova,CCWCHAIN_LEN_MAX ); -if (cnt) { -kfree (ccw); +if (cnt)return cnt; - } cnt =0 ; do {
@@ -413,10 +401,8 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)* orb specified one of the unsupported formats, we defer * checking for IDAWs in unsupported formats to here. */ -if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) &&ccw_is_idal (ccw)) { -kfree (p); +if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) &&ccw_is_idal (ccw))return -EOPNOTSUPP ; - }/* * We want to keep counting if the current CCW has the
@@ -435,7 +421,6 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)if (cnt ==CCWCHAIN_LEN_MAX +1 ) cnt = -EINVAL ; -kfree (p);return cnt; }
@@ -461,7 +446,7 @@ static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)struct ccwchain *chain;int len, ret; -/* Get chain length. */ +/* Copy the chain from cda to cp, and count the CCWs in it */ len =ccwchain_calc_length (cda, cp);if (len <0 )return len;
drivers/s390/cio/vfio_ccw_cp.h +7 -0
@@ -16,6 +16,12 @@#include "orb.h" +/* +* Max length for ccw chain. +* XXX: Limit to 256, need to check more? +*/ +#define CCWCHAIN_LEN_MAX 256 +/** * struct channel_program - manage information for channel program * @ccwchain_list: list head of ccwchains
@@ -32,6 +38,7 @@ struct channel_program {union orb orb;struct device *mdev;bool initialized; +struct ccw1 *guest_cp; }; extern int cp_init(struct channel_program *cp, struct device *mdev,
drivers/s390/cio/vfio_ccw_drv.c +7 -0
@@ -129,6 +129,11 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)if (!private)return -ENOMEM ; + private->cp.guest_cp =kcalloc (CCWCHAIN_LEN_MAX ,sizeof (struct ccw1 ), +GFP_KERNEL ); +if (!private->cp.guest_cp) + goto out_free; + private->io_region =kmem_cache_zalloc (vfio_ccw_io_region,GFP_KERNEL |GFP_DMA );if (!private->io_region)
@@ -169,6 +174,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)kmem_cache_free (vfio_ccw_cmd_region, private->cmd_region);if (private->io_region)kmem_cache_free (vfio_ccw_io_region, private->io_region); +kfree (private->cp.guest_cp);kfree (private);return ret; }
@@ -185,6 +191,7 @@ static int vfio_ccw_sch_remove(struct subchannel *sch)kmem_cache_free (vfio_ccw_cmd_region, private->cmd_region);kmem_cache_free (vfio_ccw_io_region, private->io_region); +kfree (private->cp.guest_cp);kfree (private);return 0 ;
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 1/5] vfio-ccw: Move guest_cp storage into common struct
Quoted 10 lines · On Tue, 18 Jun 2019 22:23:48 +0200 Eric Farman <[email protected]> wrote:
Rather than allocating/freeing a piece of memory every time we try to figure out how long a CCW chain is, let's use a piece of memory allocated for each device.
The
io_mutexadded with commit4f76617378ee9("vfio-ccw: protect the I/O region") is held for the duration of theVFIO_CCW_EVENT_IO_REQevent that accesses/uses this space, so there should be no race concerns with another CPU attempting an (unexpected) SSCH for the same device.
Quoted 4 lines
Suggested-by Cornelia Huck <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Conny, your suggestion [1] did not go unnoticed. :)
:)
Quoted 7 lines
[1] https://patchwork.kernel.org/comment/22312659/
--- drivers/s390/cio/vfio_ccw_cp.c | 23 ++++------------------- drivers/s390/cio/vfio_ccw_cp.h | 7 +++++++ drivers/s390/cio/vfio_ccw_drv.c | 7 +++++++ 3 files changed, 18 insertions(+), 19 deletions(-)
Nice!
Re: [RFC PATCH v1 1/5] vfio-ccw: Move guest_cp storage into common struct
In reply to Eric FarmanQuoted 49 lines · Eric Farman
Rather than allocating/freeing a piece of memory every time we try to figure out how long a CCW chain is, let's use a piece of memory allocated for each device.
The
io_mutexadded with commit4f76617378ee9("vfio-ccw: protect the I/O region") is held for the duration of theVFIO_CCW_EVENT_IO_REQevent that accesses/uses this space, so there should be no race concerns with another CPU attempting an (unexpected) SSCH for the same device.Suggested-by Cornelia Huck <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Conny, your suggestion [1] did not go unnoticed. :)[1] https://patchwork.kernel.org/comment/22312659/
--- drivers/s390/cio/vfio_ccw_cp.c | 23 ++++------------------- drivers/s390/cio/vfio_ccw_cp.h | 7 +++++++ drivers/s390/cio/vfio_ccw_drv.c | 7 +++++++ 3 files changed, 18 insertions(+), 19 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +1
-7@@ -16,12 +16,6 @@#include "vfio_ccw_cp.h" -/* -* Max length for ccw chain. -* XXX: Limit to 256, need to check more? -*/ -#define CCWCHAIN_LEN_MAX 256 -struct pfn_array {/* Starting guest physical I/O address. */ unsigned long pa_iova;@@ -386,7 +380,7 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)*/ static int ccwchain_calc_length (u64 iova,struct channel_program *cp) { -struct ccw1 *ccw, *p; +struct ccw1 *ccw = cp->guest_cp;int cnt;/*
drivers/s390/cio/vfio_ccw_cp.c static int ccwchain_calc_length(u64 iova, struct channel_program *cp) · line 391
Quoted 8 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-4@@ -394,15 +388,9 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)* Currently the chain length is limited to CCWCHAIN_LEN_MAX (256). * So copying 2K is enough (safe). */ - p = ccw =kcalloc (CCWCHAIN_LEN_MAX ,sizeof (*ccw),GFP_KERNEL ); -if (!ccw) -return -ENOMEM ; - cnt =copy_ccw_from_iova (cp, ccw, iova,CCWCHAIN_LEN_MAX );
Just a minor concern, should we clear out cp->guest_cp memory before we do the copying? Given that the ccwchain_calc_length will also call be called during tic handling, it's possible there might be some garbage data in guest_cp, no?
Quoted 95 lines
drivers/s390/cio/vfio_ccw_cp.c +3
-8-if (cnt) { -kfree (ccw); +if (cnt)return cnt; - } cnt =0 ; do {@@ -413,10 +401,8 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)* orb specified one of the unsupported formats, we defer * checking for IDAWs in unsupported formats to here. */ -if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) &&ccw_is_idal (ccw)) { -kfree (p); +if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) &&ccw_is_idal (ccw))return -EOPNOTSUPP ; - }/* * We want to keep counting if the current CCW has the @@ -435,7 +421,6 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)if (cnt ==CCWCHAIN_LEN_MAX +1 ) cnt = -EINVAL ; -kfree (p);return cnt; }@@ -461,7 +446,7 @@ static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)struct ccwchain *chain;int len, ret; -/* Get chain length. */ +/* Copy the chain from cda to cp, and count the CCWs in it */ len =ccwchain_calc_length (cda, cp);if (len <0 )return len;drivers/s390/cio/vfio_ccw_cp.h +7
-0@@ -16,6 +16,12 @@#include "orb.h" +/* +* Max length for ccw chain. +* XXX: Limit to 256, need to check more? +*/ +#define CCWCHAIN_LEN_MAX 256 +/** * struct channel_program - manage information for channel program * @ccwchain_list: list head of ccwchains @@ -32,6 +38,7 @@ struct channel_program {union orb orb;struct device *mdev;bool initialized; +struct ccw1 *guest_cp; }; extern int cp_init(struct channel_program *cp, struct device *mdev,drivers/s390/cio/vfio_ccw_drv.c +7
-0@@ -129,6 +129,11 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)if (!private)return -ENOMEM ; + private->cp.guest_cp =kcalloc (CCWCHAIN_LEN_MAX ,sizeof (struct ccw1 ), +GFP_KERNEL ); +if (!private->cp.guest_cp) + goto out_free; + private->io_region =kmem_cache_zalloc (vfio_ccw_io_region,GFP_KERNEL |GFP_DMA );if (!private->io_region)@@ -169,6 +174,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)kmem_cache_free (vfio_ccw_cmd_region, private->cmd_region);if (private->io_region)kmem_cache_free (vfio_ccw_io_region, private->io_region); +kfree (private->cp.guest_cp);kfree (private);return ret; }@@ -185,6 +191,7 @@ static int vfio_ccw_sch_remove(struct subchannel *sch)kmem_cache_free (vfio_ccw_cmd_region, private->cmd_region);kmem_cache_free (vfio_ccw_io_region, private->io_region); +kfree (private->cp.guest_cp);kfree (private);return 0 ;
Eric Farman
maintainerRe: [RFC PATCH v1 1/5] vfio-ccw: Move guest_cp storage into common struct
Quoted 57 lines · Farhan Ali
Quoted 54 lines · Eric Farman
Rather than allocating/freeing a piece of memory every time we try to figure out how long a CCW chain is, let's use a piece of memory allocated for each device.
The
io_mutexadded with commit4f76617378ee9("vfio-ccw: protect the I/O region") is held for the duration of theVFIO_CCW_EVENT_IO_REQevent that accesses/uses this space, so there should be no race concerns with another CPU attempting an (unexpected) SSCH for the same device.Suggested-by Cornelia Huck <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Conny, your suggestion [1] did not go unnoticed. :)[1] https://patchwork.kernel.org/comment/22312659/
---
drivers/s390/cio/vfio_ccw_cp.c | 23 ++++------------------- drivers/s390/cio/vfio_ccw_cp.h | 7 +++++++drivers/s390/cio/vfio_ccw_drv.c| 7 +++++++3 files changed, 18 insertions(+), 19 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-0@@ -16,12 +16,6 @@ #include "vfio_ccw_cp.h"-/*
- * Max length for ccw chain.
- * XXX: Limit to 256, need to check more?
- */drivers/s390/cio/vfio_ccw_cp.c +0
-2-#define CCWCHAIN_LEN_MAX 256 -struct
pfn_array{
/* Starting guest physical I/O address. */
unsigned longpa_iova;drivers/s390/cio/vfio_ccw_cp.c +1
-3@@ -386,7 +380,7 @@ static void ccwchain_cda_free(struct ccwchain*chain, int idx) */ static int ccwchain_calc_length(u64 iova, struct channel_program *cp) { - struct ccw1 *ccw, *p; + struct ccw1 *ccw = cp->guest_cp; int cnt; /*@@ -394,15 +388,9 @@ static int ccwchain_calc_length(u64 iova, structchannel_program *cp) * Currently the chain length is limited to CCWCHAIN_LEN_MAX (256). * So copying 2K is enough (safe). */ - p = ccw = kcalloc(CCWCHAIN_LEN_MAX, sizeof(*ccw), GFP_KERNEL); - if (!ccw)
Quoted 8 lines
drivers/s390/cio/vfio_ccw_cp.c static int ccwchain_calc_length(u64 iova, struct channel_program *cp) · line 391
Quoted 3 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-2- return -ENOMEM; - cnt = copy_ccw_from_iova(cp, ccw, iova, CCWCHAIN_LEN_MAX);Just a minor concern, should we clear out
cp->guest_cpmemory before we do the copying? Given that theccwchain_calc_lengthwill also call be called during tic handling, it's possible there might be some garbage data inguest_cp, no?
Yeah, they'll be garbage there, but I'm not sure it's a problem. By the time we get here again (ccwchain_loop_tic() -> ccwchain_handle_ccw()), we'll have saved the relevant CCWs for the first segment. And the second time through we'll be copying a fresh 2K from the target of the TIC to cp->guest_cp, overwriting all that stale data with new CCWs (and new garbage data).
Quoted 94 lines
Quoted 94 lines
drivers/s390/cio/vfio_ccw_cp.c +2
-6- if (cnt) { - kfree(ccw); + if (cnt) return cnt; - } cnt = 0; do {@@ -413,10 +401,8 @@ static int ccwchain_calc_length(u64 iova, structchannel_program *cp) * orb specified one of the unsupported formats, we defer * checking for IDAWs in unsupported formats to here. */ - if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) { - kfree(p); + if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) return -EOPNOTSUPP; - } /** We want to keep counting if the current CCW has the @@ -435,7 +421,6 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp) if (cnt == CCWCHAIN_LEN_MAX + 1) cnt = -EINVAL; - kfree(p); return cnt; }@@ -461,7 +446,7 @@ static int
ccwchain_handle_ccw(u32 cda, struct channel_program *cp)
struct ccwchain *chain;
int len, ret;
- /* Get chain length. */
+ /* Copy the chain from cda to cp, and count the CCWs in it */ len =ccwchain_calc_length(cda, cp);
if (len < 0)
return len;drivers/s390/cio/vfio_ccw_cp.h +0
-0@@ -16,6 +16,12 @@ #include "orb.h"+/*
+ * Max length for ccw chain.
+ * XXX: Limit to 256, need to check more?
+ */drivers/s390/cio/vfio_ccw_cp.h +2
-0+#define CCWCHAIN_LEN_MAX 256 +/**
* structchannel_program- manage information for channel program
* @ccwchain_list: list head of ccwchainsdrivers/s390/cio/vfio_ccw_cp.h +1
-0@@ -32,6 +38,7 @@ struct channel_program { union orb orb; struct device *mdev; bool initialized; + struct ccw1 *guest_cp; }; extern int cp_init(struct channel_program *cp, struct device *mdev,drivers/s390/cio/vfio_ccw_drv.c +0
-0@@ -129,6 +129,11 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) if (!private) return -ENOMEM;+
private->cp.guest_cp= kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1),
+GFP_KERNEL);
+ if (!private->cp.guest_cp)
+ gotoout_free;
+
private->io_region=kmem_cache_zalloc(vfio_ccw_io_region, GFP_KERNEL | GFP_DMA);
if (!private->io_region)drivers/s390/cio/vfio_ccw_drv.c +2
-0@@ -169,6 +174,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region); if (private->io_region) kmem_cache_free(vfio_ccw_io_region, private->io_region); + kfree(private->cp.guest_cp); kfree(private); return ret; }@@ -185,6 +191,7 @@ static int vfio_ccw_sch_remove(struct subchannel *sch) kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region); kmem_cache_free(vfio_ccw_io_region, private->io_region); + kfree(private->cp.guest_cp); kfree(private); return 0;
Re: [RFC PATCH v1 1/5] vfio-ccw: Move guest_cp storage into common struct
Quoted 69 lines · Eric Farman
Quoted 57 lines · Farhan Ali
Quoted 54 lines · Eric Farman
Rather than allocating/freeing a piece of memory every time we try to figure out how long a CCW chain is, let's use a piece of memory allocated for each device.
The
io_mutexadded with commit4f76617378ee9("vfio-ccw: protect the I/O region") is held for the duration of theVFIO_CCW_EVENT_IO_REQevent that accesses/uses this space, so there should be no race concerns with another CPU attempting an (unexpected) SSCH for the same device.Suggested-by Cornelia Huck <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Conny, your suggestion [1] did not go unnoticed. :)[1] https://patchwork.kernel.org/comment/22312659/
--- drivers/s390/cio/vfio_ccw_cp.c | 23 ++++------------------- drivers/s390/cio/vfio_ccw_cp.h | 7 +++++++ drivers/s390/cio/vfio_ccw_drv.c | 7 +++++++ 3 files changed, 18 insertions(+), 19 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +1
-8@@ -16,12 +16,6 @@ #include "vfio_ccw_cp.h" -/* -* Max length for ccw chain. -* XXX: Limit to 256, need to check more? -*/ -#define CCWCHAIN_LEN_MAX 256 - struct pfn_array { /* Starting guest physical I/O address. */ unsigned long pa_iova;@@ -386,7 +380,7 @@ static void ccwchain_cda_free(struct ccwchain*chain, int idx) */ static int ccwchain_calc_length(u64 iova, struct channel_program *cp) { - struct ccw1 *ccw, *p; + struct ccw1 *ccw = cp->guest_cp; int cnt; /*@@ -394,15 +388,9 @@ static int ccwchain_calc_length(u64 iova, structchannel_program *cp) * Currently the chain length is limited to CCWCHAIN_LEN_MAX (256). * So copying 2K is enough (safe). */ - p = ccw = kcalloc(CCWCHAIN_LEN_MAX, sizeof(*ccw), GFP_KERNEL); - if (!ccw)Quoted 8 lines
drivers/s390/cio/vfio_ccw_cp.c static int ccwchain_calc_length(u64 iova, struct channel_program *cp) · line 391
Quoted 3 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-2- return -ENOMEM; - cnt = copy_ccw_from_iova(cp, ccw, iova, CCWCHAIN_LEN_MAX);Just a minor concern, should we clear out
cp->guest_cpmemory before we do the copying? Given that theccwchain_calc_lengthwill also call be called during tic handling, it's possible there might be some garbage data inguest_cp, no?
Quoted 7 lines
Yeah, they'll be garbage there, but I'm not sure it's a problem. By the time we get here again (
ccwchain_loop_tic()->ccwchain_handle_ccw()), we'll have saved the relevant CCWs for the first segment. And the second time through we'll be copying a fresh 2K from the target of the TIC tocp->guest_cp, overwriting all that stale data with new CCWs (and new garbage data).
Yes, you are right. Please disregard my concern :)
Quoted 95 lines
Quoted 94 lines
Quoted 94 lines
drivers/s390/cio/vfio_ccw_cp.c +2
-6- if (cnt) { - kfree(ccw); + if (cnt) return cnt; - } cnt = 0; do {@@ -413,10 +401,8 @@ static int ccwchain_calc_length(u64 iova, structchannel_program *cp) * orb specified one of the unsupported formats, we defer * checking for IDAWs in unsupported formats to here. */ - if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) { - kfree(p); + if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) return -EOPNOTSUPP; - } /** We want to keep counting if the current CCW has the @@ -435,7 +421,6 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp) if (cnt == CCWCHAIN_LEN_MAX + 1) cnt = -EINVAL; - kfree(p); return cnt; } @@ -461,7 +446,7 @@ static int ccwchain_handle_ccw(u32 cda, struct
channel_program*cp)
struct ccwchain *chain;
int len, ret;
- /* Get chain length. */
+ /* Copy the chain from cda to cp, and count the CCWs in it */
len =ccwchain_calc_length(cda, cp);
if (len < 0)
return len;drivers/s390/cio/vfio_ccw_cp.h +6
-0@@ -16,6 +16,12 @@ #include "orb.h" +/* +* Max length for ccw chain. +* XXX: Limit to 256, need to check more? +*/ +#define CCWCHAIN_LEN_MAX 256 + /*** struct channel_program - manage information for channel program * @ccwchain_list: list head of ccwchains @@ -32,6 +38,7 @@ struct channel_program { union orb orb; struct device *mdev; bool initialized; + struct ccw1 *guest_cp; }; extern int cp_init(struct channel_program *cp, struct device *mdev,drivers/s390/cio/vfio_ccw_drv.c +6
-0@@ -129,6 +129,11 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) if (!private) return -ENOMEM; + private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX, sizeof(structccw1 ), + GFP_KERNEL); + if (!private->cp.guest_cp) + goto out_free; + private->io_region = kmem_cache_zalloc(vfio_ccw_io_region, GFP_KERNEL | GFP_DMA); if (!private->io_region)@@ -169,6 +174,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region); if (private->io_region) kmem_cache_free(vfio_ccw_io_region, private->io_region); + kfree(private->cp.guest_cp); kfree(private); return ret; }@@ -185,6 +191,7 @@ static int vfio_ccw_sch_remove(struct subchannel *sch) kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region); kmem_cache_free(vfio_ccw_io_region, private->io_region); + kfree(private->cp.guest_cp); kfree(private); return 0;
Eric Farman
maintainerRFC PATCH 2/5 vfio-ccw: Skip second copy of guest cp to host
In reply to Eric FarmanWe already pinned/copied/unpinned 2K (256 CCWs) of guest memory to the host space anchored off vfio_ccw_private. There's no need to do that again once we have the length calculated, when we could just copy the section we need to the "permanent" space for the I/O.
--- drivers/s390/cio/vfio_ccw_cp.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
drivers/s390/cio/vfio_ccw_cp.c +3 -7
@@ -444,7 +444,7 @@ static int ccwchain_loop_tic(struct ccwchain *chain,static int ccwchain_handle_ccw (u32 cda,struct channel_program *cp) {struct ccwchain *chain; -int len, ret; +int len;/* Copy the chain from cda to cp, and count the CCWs in it */ len =ccwchain_calc_length (cda, cp);
@@ -457,12 +457,8 @@ static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)return -ENOMEM ; chain->ch_iova = cda; -/* Copy the new chain from user. */ - ret =copy_ccw_from_iova (cp, chain->ch_ccw, cda, len); -if (ret) { -ccwchain_free (chain); -return ret; - } +/* Copy the actual CCWs into the new chain */ +memcpy (chain->ch_ccw, cp->guest_cp, len *sizeof (struct ccw1 ));/* Loop for tics on this new chain. */ return ccwchain_loop_tic (chain, cp);
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 2/5] vfio-ccw: Skip second copy of guest cp to host
Quoted 9 lines · On Tue, 18 Jun 2019 22:23:49 +0200 Eric Farman <[email protected]> wrote:
We already pinned/copied/unpinned 2K (256 CCWs) of guest memory to the host space anchored off
vfio_ccw_private. There's no need to do that again once we have the length calculated, when we could just copy the section we need to the "permanent" space for the I/O.Signed-off-by Eric Farman <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
Eric Farman
maintainerRFC PATCH 3/5 vfio-ccw: Copy CCW data outside length calculation
In reply to Eric FarmanIt doesn't make much sense to "hide" the copy to the channel_program struct inside a routine that calculates the length of the chain.
Let's move it to the calling routine, which will later copy from channel_program to the memory it allocated itself.
--- drivers/s390/cio/vfio_ccw_cp.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
drivers/s390/cio/vfio_ccw_cp.c +7 -12
@@ -381,18 +381,8 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)static int ccwchain_calc_length (u64 iova,struct channel_program *cp) {struct ccw1 *ccw = cp->guest_cp; -int cnt; +int cnt =0 ; -/* -* Copy current chain from guest to host kernel. -* Currently the chain length is limited to CCWCHAIN_LEN_MAX (256). -* So copying 2K is enough (safe). -*/ - cnt =copy_ccw_from_iova (cp, ccw, iova,CCWCHAIN_LEN_MAX ); -if (cnt) -return cnt; - - cnt =0 ; do { cnt++;
@@ -446,7 +436,12 @@ static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)struct ccwchain *chain;int len; -/* Copy the chain from cda to cp, and count the CCWs in it */ +/* Copy 2K (the most we support today) of possible CCWs */ + len =copy_ccw_from_iova (cp, cp->guest_cp, cda,CCWCHAIN_LEN_MAX ); +if (len) +return len; + +/* Count the CCWs in the current chain */ len =ccwchain_calc_length (cda, cp);if (len <0 )return len;
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 3/5] vfio-ccw: Copy CCW data outside length calculation
Quoted 10 lines · On Tue, 18 Jun 2019 22:23:50 +0200 Eric Farman <[email protected]> wrote:
It doesn't make much sense to "hide" the copy to the
channel_programstruct inside a routine that calculates the length of the chain.Let's move it to the calling routine, which will later copy from
channel_programto the memory it allocated itself.Signed-off-by Eric Farman <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
Eric Farman
maintainerRFC PATCH 4/5 vfio-ccw: Factor out the ccw0-to-ccw1 transition
In reply to Eric FarmanThis is a really useful function, but it's buried in the copy_ccw_from_iova() routine so that ccwchain_calc_length() can just work with Format-1 CCWs while doing its counting. But it means we're translating a full 2K of "CCWs" to Format-1, when in reality there's probably far fewer in that space.
Let's factor it out, so maybe we can do something with it later.
--- drivers/s390/cio/vfio_ccw_cp.c | 48 ++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 23 deletions(-)
drivers/s390/cio/vfio_ccw_cp.c +25 -23
@@ -161,6 +161,27 @@ static inline void pfn_array_idal_create_words( idaws[0 ] += pa->pa_iova & (PAGE_SIZE -1 ); } +void convert_ccw0_to_ccw1 (struct ccw1 *source,unsigned long len) +{ +struct ccw0 ccw0; +struct ccw1 *pccw1 = source; +int i; + +for (i =0 ; i < len; i++) { + ccw0 = *(struct ccw0 *)pccw1; +if ((pccw1->cmd_code &0x0f ) ==CCW_CMD_TIC ) { + pccw1->cmd_code =CCW_CMD_TIC ; + pccw1->flags =0 ; + pccw1->count =0 ; + }else { + pccw1->cmd_code = ccw0.cmd_code; + pccw1->flags = ccw0.flags; + pccw1->count = ccw0.count; + } + pccw1->cda = ccw0.cda; + pccw1++; + } +}/* * Within the domain (@mdev), copy @n bytes from a guest physical
@@ -211,32 +232,9 @@ static long copy_ccw_from_iova(struct channel_program *cp, struct ccw1 *to, u64 iova, unsigned long len) { - struct ccw0 ccw0; -struct ccw1 *pccw1; int ret; -int i; ret =copy_from_iova (cp->mdev, to, iova, len *sizeof (struct ccw1 )); -if (ret) -return ret; - -if (!cp->orb.cmd.fmt) { - pccw1 = to; -for (i =0 ; i < len; i++) { - ccw0 = *(struct ccw0 *)pccw1; -if ((pccw1->cmd_code &0x0f ) ==CCW_CMD_TIC ) { - pccw1->cmd_code =CCW_CMD_TIC ; - pccw1->flags =0 ; - pccw1->count =0 ; - }else { - pccw1->cmd_code = ccw0.cmd_code; - pccw1->flags = ccw0.flags; - pccw1->count = ccw0.count; - } - pccw1->cda = ccw0.cda; - pccw1++; - } - }return ret; }
@@ -441,6 +439,10 @@ static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)if (len)return len; +/* Convert any Format-0 CCWs to Format-1 */ +if (!cp->orb.cmd.fmt) +convert_ccw0_to_ccw1 (cp->guest_cp, len); +/* Count the CCWs in the current chain */ len =ccwchain_calc_length (cda, cp);if (len <0 )
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 4/5] vfio-ccw: Factor out the ccw0-to-ccw1 transition
Quoted 7 lines · On Tue, 18 Jun 2019 22:23:51 +0200 Eric Farman <[email protected]> wrote:
This is a really useful function, but it's buried in the
copy_ccw_from_iova()routine so thatccwchain_calc_length()can just work with Format-1 CCWs while doing its counting. But it means we're translating a full 2K of "CCWs" to Format-1, when in reality there's probably far fewer in that space.Let's factor it out, so maybe we can do something with it later.
Agreed, this looks sensible.
Quoted 5 lines
Signed-off-by Eric Farman <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 48 ++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 23 deletions(-)
Eric Farman
maintainerRFC PATCH 5/5 vfio-ccw: Remove copy_ccw_from_iova()
In reply to Eric FarmanJust to keep things tidy.
--- drivers/s390/cio/vfio_ccw_cp.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
drivers/s390/cio/vfio_ccw_cp.c +2 -12
@@ -228,17 +228,6 @@ static long copy_from_iova(struct device *mdev,return l; } -static long copy_ccw_from_iova (struct channel_program *cp, -struct ccw1 *to,u64 iova, -unsigned long len) -{ -int ret; - - ret =copy_from_iova (cp->mdev, to, iova, len *sizeof (struct ccw1 )); - -return ret; -} -/* * Helpers to operate ccwchain. */
@@ -435,7 +424,8 @@ static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)int len;/* Copy 2K (the most we support today) of possible CCWs */ - len =copy_ccw_from_iova (cp, cp->guest_cp, cda,CCWCHAIN_LEN_MAX ); + len =copy_from_iova (cp->mdev, cp->guest_cp, cda, +CCWCHAIN_LEN_MAX *sizeof (struct ccw1 ));if (len)return len;
Signature · 1 line
2.17.1
Re: [RFC PATCH v1 5/5] vfio-ccw: Remove copy_ccw_from_iova()
Quoted 6 lines · On Tue, 18 Jun 2019 22:23:52 +0200 Eric Farman <[email protected]> wrote:
Just to keep things tidy.
Signed-off-by Eric Farman <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
Re: [RFC PATCH v1 5/5] vfio-ccw: Remove copy_ccw_from_iova()
In reply to Eric FarmanQuoted 31 lines · Eric Farman
Just to keep things tidy.
Signed-off-by Eric Farman <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-11@@ -228,17 +228,6 @@ static long copy_from_iova(struct device *mdev,return l; } -static long copy_ccw_from_iova (struct channel_program *cp, -struct ccw1 *to,u64 iova, -unsigned long len) -{ -int ret; - - ret =copy_from_iova (cp->mdev, to, iova, len *sizeof (struct ccw1 )); - -return ret; -} -/* * Helpers to operate ccwchain. */
drivers/s390/cio/vfio_ccw_cp.c static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp) · line 431
Quoted 9 lines
drivers/s390/cio/vfio_ccw_cp.c +2
-1@@ -435,7 +424,8 @@ static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)int len;/* Copy 2K (the most we support today) of possible CCWs */ - len =copy_ccw_from_iova (cp, cp->guest_cp, cda,CCWCHAIN_LEN_MAX ); + len =copy_from_iova (cp->mdev, cp->guest_cp, cda, +CCWCHAIN_LEN_MAX *sizeof (struct ccw1 ));if (len)return len;
This patch probably could be squashed with patch 4. Not a big deal though.
Quoted 39 lines · On Tue, 18 Jun 2019 22:23:47 +0200 Eric Farman <[email protected]> wrote:
A couple little improvements to the malloc load in vfio-ccw. Really, there were just (the first) two patches, but then I got excited and added a few stylistic ones to the end.
The routine
ccwchain_calc_length()has this basic structure:
ccwchain_calc_lengtha0 = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1)) copy_ccw_from_iova(a0, src) copy_from_iova pfn_array_alloc b = kcalloc(len, sizeof(*pa_iova_pfn + *pa_pfn) pfn_array_pin vfio_pin_pages memcpy(a0, src) pfn_array_unpin_free vfio_unpin_pages kfree(b) kfree(a0)We do this EVERY time we process a new channel program chain, meaning at least once per SSCH and more if TICs are involved, to figure out how many CCWs are chained together. Once that is determined, a new piece of memory is allocated (call it a1) and then passed to
copy_ccw_from_iova()again, but for the value calculated byccwchain_calc_length().This seems inefficient.
Patch 1 moves the malloc of a0 from the CCW processor to the initialization of the device. Since only one SSCH can be handled concurrently, we can use this space safely to determine how long the chain being processed actually is.
Patch 2 then removes the second
copy_ccw_from_iova()call entirely, and replaces it with a memcpy from a0 to a1. This is done before we process a TIC and thus a second chain, so there is no overlap in the storage inchannel_program.
Quoted 7 lines
Patches 3-5 clean up some things that aren't as clear as I'd like, but didn't want to pollute the first two changes. For example, patch 3 moves the population of
guest_cpto the same routine that copies from it, rather than in a called function. Meanwhile, patch 4 (and thus, 5) was something I had lying around for quite some time, because it looked to be structured weird. Maybe that's one bridge too far.
I think this is worthwhile.
Quoted 8 lines
Eric Farman (5):
vfio-ccw: Moveguest_cpstorage into common struct
vfio-ccw: Skip second copy of guest cp to host
vfio-ccw: Copy CCW data outside length calculation
vfio-ccw: Factor out the ccw0-to-ccw1 transition
vfio-ccw: Removecopy_ccw_from_iova()
Quoted 5 lines
drivers/s390/cio/vfio_ccw_cp.c | 108 +++++++++++--------------------- drivers/s390/cio/vfio_ccw_cp.h | 7 +++ drivers/s390/cio/vfio_ccw_drv.c | 7 +++ 3 files changed, 52 insertions(+), 70 deletions(-)
Ok, so I just wanted to take a quick look, and then ended up reviewing it all :)
Will give others some time to look at this before I queue.
Eric Farman
maintainerQuoted 60 lines · Cornelia Huck
Quoted 39 lines · On Tue, 18 Jun 2019 22:23:47 +0200 Eric Farman <[email protected]> wrote:
A couple little improvements to the malloc load in vfio-ccw. Really, there were just (the first) two patches, but then I got excited and added a few stylistic ones to the end.
The routine
ccwchain_calc_length()has this basic structure:
ccwchain_calc_lengtha0 = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1)) copy_ccw_from_iova(a0, src) copy_from_iova pfn_array_alloc b = kcalloc(len, sizeof(*pa_iova_pfn + *pa_pfn) pfn_array_pin vfio_pin_pages memcpy(a0, src) pfn_array_unpin_free vfio_unpin_pages kfree(b) kfree(a0)We do this EVERY time we process a new channel program chain, meaning at least once per SSCH and more if TICs are involved, to figure out how many CCWs are chained together. Once that is determined, a new piece of memory is allocated (call it a1) and then passed to
copy_ccw_from_iova()again, but for the value calculated byccwchain_calc_length().This seems inefficient.
Patch 1 moves the malloc of a0 from the CCW processor to the initialization of the device. Since only one SSCH can be handled concurrently, we can use this space safely to determine how long the chain being processed actually is.
Patch 2 then removes the second
copy_ccw_from_iova()call entirely, and replaces it with a memcpy from a0 to a1. This is done before we process a TIC and thus a second chain, so there is no overlap in the storage inchannel_program.Quoted 7 lines
Patches 3-5 clean up some things that aren't as clear as I'd like, but didn't want to pollute the first two changes. For example, patch 3 moves the population of
guest_cpto the same routine that copies from it, rather than in a called function. Meanwhile, patch 4 (and thus, 5) was something I had lying around for quite some time, because it looked to be structured weird. Maybe that's one bridge too far.I think this is worthwhile.
Quoted 8 lines
Eric Farman (5):
vfio-ccw: Moveguest_cpstorage into common struct
vfio-ccw: Skip second copy of guest cp to host
vfio-ccw: Copy CCW data outside length calculation
vfio-ccw: Factor out the ccw0-to-ccw1 transition
vfio-ccw: Removecopy_ccw_from_iova()
Quoted 8 lines
Quoted 5 lines
drivers/s390/cio/vfio_ccw_cp.c | 108 +++++++++++--------------------- drivers/s390/cio/vfio_ccw_cp.h | 7 +++ drivers/s390/cio/vfio_ccw_drv.c | 7 +++ 3 files changed, 52 insertions(+), 70 deletions(-)Ok, so I just wanted to take a quick look, and then ended up reviewing it all :)
Haha, oops! :) Thank you! That was a nice surprise.
Quoted 3 lines
Will give others some time to look at this before I queue.
Sounds great! I'll get back to my own reviews (notes the gentle reminder on qemu :)
Quoted 54 lines · Eric Farman
A couple little improvements to the malloc load in vfio-ccw. Really, there were just (the first) two patches, but then I got excited and added a few stylistic ones to the end.
The routine
ccwchain_calc_length()has this basic structure:ccwchain_calc_length a0 = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1)) copy_ccw_from_iova(a0, src) copy_from_iova pfn_array_alloc b = kcalloc(len, sizeof(*pa_iova_pfn + *pa_pfn) pfn_array_pin vfio_pin_pages memcpy(a0, src) pfn_array_unpin_free vfio_unpin_pages kfree(b) kfree(a0)We do this EVERY time we process a new channel program chain, meaning at least once per SSCH and more if TICs are involved, to figure out how many CCWs are chained together. Once that is determined, a new piece of memory is allocated (call it a1) and then passed to
copy_ccw_from_iova()again, but for the value calculated byccwchain_calc_length().This seems inefficient.
Patch 1 moves the malloc of a0 from the CCW processor to the initialization of the device. Since only one SSCH can be handled concurrently, we can use this space safely to determine how long the chain being processed actually is.
Patch 2 then removes the second
copy_ccw_from_iova()call entirely, and replaces it with a memcpy from a0 to a1. This is done before we process a TIC and thus a second chain, so there is no overlap in the storage inchannel_program.Patches 3-5 clean up some things that aren't as clear as I'd like, but didn't want to pollute the first two changes. For example, patch 3 moves the population of
guest_cpto the same routine that copies from it, rather than in a called function. Meanwhile, patch 4 (and thus, 5) was something I had lying around for quite some time, because it looked to be structured weird. Maybe that's one bridge too far.Eric Farman (5):
vfio-ccw: Moveguest_cpstorage into common struct
vfio-ccw: Skip second copy of guest cp to host
vfio-ccw: Copy CCW data outside length calculation
vfio-ccw: Factor out the ccw0-to-ccw1 transition
vfio-ccw: Removecopy_ccw_from_iova()
Quoted 5 lines
drivers/s390/cio/vfio_ccw_cp.c | 108 +++++++++++--------------------- drivers/s390/cio/vfio_ccw_cp.h | 7 +++ drivers/s390/cio/vfio_ccw_drv.c | 7 +++ 3 files changed, 52 insertions(+), 70 deletions(-)
For this series:
Cornelia Huck
AppliedQuoted 54 lines · On Tue, 18 Jun 2019 22:23:47 +0200 Eric Farman <[email protected]> wrote:
A couple little improvements to the malloc load in vfio-ccw. Really, there were just (the first) two patches, but then I got excited and added a few stylistic ones to the end.
The routine
ccwchain_calc_length()has this basic structure:
ccwchain_calc_lengtha0 = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1)) copy_ccw_from_iova(a0, src) copy_from_iova pfn_array_alloc b = kcalloc(len, sizeof(*pa_iova_pfn + *pa_pfn) pfn_array_pin vfio_pin_pages memcpy(a0, src) pfn_array_unpin_free vfio_unpin_pages kfree(b) kfree(a0)We do this EVERY time we process a new channel program chain, meaning at least once per SSCH and more if TICs are involved, to figure out how many CCWs are chained together. Once that is determined, a new piece of memory is allocated (call it a1) and then passed to
copy_ccw_from_iova()again, but for the value calculated byccwchain_calc_length().This seems inefficient.
Patch 1 moves the malloc of a0 from the CCW processor to the initialization of the device. Since only one SSCH can be handled concurrently, we can use this space safely to determine how long the chain being processed actually is.
Patch 2 then removes the second
copy_ccw_from_iova()call entirely, and replaces it with a memcpy from a0 to a1. This is done before we process a TIC and thus a second chain, so there is no overlap in the storage inchannel_program.Patches 3-5 clean up some things that aren't as clear as I'd like, but didn't want to pollute the first two changes. For example, patch 3 moves the population of
guest_cpto the same routine that copies from it, rather than in a called function. Meanwhile, patch 4 (and thus, 5) was something I had lying around for quite some time, because it looked to be structured weird. Maybe that's one bridge too far.Eric Farman (5):
vfio-ccw: Moveguest_cpstorage into common struct
vfio-ccw: Skip second copy of guest cp to host
vfio-ccw: Copy CCW data outside length calculation
vfio-ccw: Factor out the ccw0-to-ccw1 transition
vfio-ccw: Removecopy_ccw_from_iova()
Quoted 5 lines
drivers/s390/cio/vfio_ccw_cp.c | 108 +++++++++++--------------------- drivers/s390/cio/vfio_ccw_cp.h | 7 +++ drivers/s390/cio/vfio_ccw_drv.c | 7 +++ 3 files changed, 52 insertions(+), 70 deletions(-)
Thanks, applied.