Some vfio-ccw fixes
Subsystems (4)
cover Some vfio-ccw fixes
Hi,
While trying to chase down the problem regarding the stacktraces, I have also found some minor problems in the code.
Would appreciate any review or feedback regarding them.
Thanks
Farhan
Farhan Ali (4):
vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw
vfio-ccw: No need to call cp_free on an error in cp_init
vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn
vfio-ccw: Don't call cp_free if we are processing a channel program
drivers/s390/cio/vfio_ccw_cp.c | 12 ++++++------ drivers/s390/cio/vfio_ccw_drv.c | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-)
Signature · 1 line
2.7.4
RFC 1/4 vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw
Because ccwchain_handle_ccw calls ccwchain_calc_length and as per the comment we should set orb.cmd.c64 before calling ccwchanin_calc_length.
--- drivers/s390/cio/vfio_ccw_cp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/s390/cio/vfio_ccw_cp.c +5 -5
@@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)memcpy (&cp->orb, orb,sizeof (*orb)); cp->mdev = mdev; -/* 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. */ cp->orb.cmd.c64 =1 ; +/* Build a ccwchain for the first CCW segment */ + ret =ccwchain_handle_ccw (orb->cmd.cpa, cp); +if (ret) +cp_free (cp); +if (!ret) cp->initialized = true;
Signature · 1 line
2.7.4
Re: [RFC v1 1/4] vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw
Quoted 25 lines · On Mon, 1 Jul 2019 12:23:43 -0400 Farhan Ali <[email protected]> wrote:
Because
ccwchain_handle_ccwcallsccwchain_calc_lengthand as per the comment we should set orb.cmd.c64 before callingccwchanin_calc_length.Signed-off-by Farhan Ali <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-5@@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)memcpy (&cp->orb, orb,sizeof (*orb)); cp->mdev = mdev; -/* 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. */
drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) · line 655
Quoted 10 lines
drivers/s390/cio/vfio_ccw_cp.c +5
-0cp->orb.cmd.c64 =1 ; +/* Build a ccwchain for the first CCW segment */ + ret =ccwchain_handle_ccw (orb->cmd.cpa, cp); +if (ret) +cp_free (cp); +if (!ret) cp->initialized = true;
Hm... has this ever been correct, or did this break only with the recent refactorings?
(IOW, what should Fixes: point to?)
Re: [RFC v1 1/4] vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw
Quoted 42 lines · Cornelia Huck
Quoted 25 lines · On Mon, 1 Jul 2019 12:23:43 -0400 Farhan Ali <[email protected]> wrote:
Because
ccwchain_handle_ccwcallsccwchain_calc_lengthand as per the comment we should set orb.cmd.c64 before callingccwchanin_calc_length.Signed-off-by Farhan Ali <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-5@@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)memcpy (&cp->orb, orb,sizeof (*orb)); cp->mdev = mdev; -/* 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. */ drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) · line 655
Quoted 10 lines
drivers/s390/cio/vfio_ccw_cp.c +5
-0cp->orb.cmd.c64 =1 ; +/* Build a ccwchain for the first CCW segment */ + ret =ccwchain_handle_ccw (orb->cmd.cpa, cp); +if (ret) +cp_free (cp); +if (!ret) cp->initialized = true;Hm... has this ever been correct, or did this break only with the recent refactorings?
Quoted 3 lines
(IOW, what should Fixes: point to?)
I think it was correct before some of the new refactoring we did. But we do need to set before calling ccwchain_calc_length, because the function does have a check for orb.cmd.64. I will see which exact commit did it.
Thanks
Farhan
Eric Farman
maintainerRe: [RFC v1 1/4] vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw
Quoted 45 lines · Farhan Ali
Quoted 42 lines · Cornelia Huck
Quoted 27 lines · On Mon, 1 Jul 2019 12:23:43 -0400 Farhan Ali <[email protected]> wrote:
Because
ccwchain_handle_ccwcallsccwchain_calc_lengthand as per the comment we should set orb.cmd.c64 before callingccwchanin_calc_length.Signed-off-by Farhan Ali <[email protected]>---
drivers/s390/cio/vfio_ccw_cp.c| 10 +++++-----1 file changed, 5 insertions(+), 5 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-0@@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) memcpy(&cp->orb, orb, sizeof(*orb)); cp->mdev = mdev;- /* 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_lengthreturns an error. */
cp->orb.cmd.c64= 1;Quoted 8 lines
+ /* Build a ccwchain for the first CCW segment */ + ret =
ccwchain_handle_ccw(orb->cmd.cpa, cp); + if (ret)
+cp_free(cp);
+
if (!ret)
cp->initialized= true;Hm... has this ever been correct, or did this break only with the recent refactorings?
Quoted 1 line
Quoted 1 line
(IOW, what should Fixes: point to?)
Yeah, that looks like it should blame my refactoring.
Quoted 6 lines
Quoted 2 lines
I think it was correct before some of the new refactoring we did. But we do need to set before calling
ccwchain_calc_length, because the function does have a check for orb.cmd.64. I will see which exact commit did it.
I get why that check exists, but does anyone know why it's buried in ccwchain_calc_length()? Is it simply because ccwchain_calc_length() assumes to be working on Format-1 CCWs? I don't think that routine cares if it's an IDA or not, an it'd be nice if we could put a check for the supported IDA formats somewhere up front.
Quoted 3 lines
Thanks
Farhan
Re: [RFC v1 1/4] vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw
Quoted 57 lines · On Tue, 2 Jul 2019 11:11:47 -0400 Eric Farman <[email protected]> wrote:
Quoted 45 lines · Farhan Ali
Quoted 42 lines · Cornelia Huck
Quoted 27 lines · On Mon, 1 Jul 2019 12:23:43 -0400 Farhan Ali <[email protected]> wrote:
Because
ccwchain_handle_ccwcallsccwchain_calc_lengthand as per the comment we should set orb.cmd.c64 before callingccwchanin_calc_length.Signed-off-by Farhan Ali <[email protected]>---
drivers/s390/cio/vfio_ccw_cp.c| 10 +++++-----1 file changed, 5 insertions(+), 5 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-0@@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) memcpy(&cp->orb, orb, sizeof(*orb)); cp->mdev = mdev;- /* 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_lengthreturns an error. */
cp->orb.cmd.c64= 1;Quoted 8 lines
+ /* Build a ccwchain for the first CCW segment */ + ret =
ccwchain_handle_ccw(orb->cmd.cpa, cp); + if (ret)
+cp_free(cp);
+
if (!ret)
cp->initialized= true;Hm... has this ever been correct, or did this break only with the recent refactorings?
Quoted 1 line
Quoted 1 line
(IOW, what should Fixes: point to?)
Yeah, that looks like it should blame my refactoring.
Quoted 6 lines
Quoted 2 lines
I think it was correct before some of the new refactoring we did. But we do need to set before calling
ccwchain_calc_length, because the function does have a check for orb.cmd.64. I will see which exact commit did it.
Quoted 5 lines
I get why that check exists, but does anyone know why it's buried in
ccwchain_calc_length()? Is it simply becauseccwchain_calc_length()assumes to be working on Format-1 CCWs? I don't think that routine cares if it's an IDA or not, an it'd be nice if we could put a check for the supported IDA formats somewhere up front.
The more I stare at this code, the more confused I get :(
Apparently we want to allow the guest to specify an orb without cmd.c64 set, as this is fine as long as the channel program does not use idals. However, we _do_ want to reject it if cmd.c64 is not set, but idals are used; so we actually _don't_ want to force this before the processing. We just want the flag in the orb to be set when we do the ssch.
So it seems that the comment does not really talk about what ccwchain_calc_length _will_ do, but what it _generally_ does (and, in this case, already would have done.)
If my understanding is correct, maybe we should reword the comment instead? i.e. s/returns/would have returned/
Re: [RFC v1 1/4] vfio-ccw: Set orb.cmd.c64 before calling ccwchain_handle_ccw
Quoted 60 lines · Cornelia Huck
Quoted 57 lines · On Tue, 2 Jul 2019 11:11:47 -0400 Eric Farman <[email protected]> wrote:
Quoted 45 lines · Farhan Ali
Quoted 42 lines · Cornelia Huck
Quoted 25 lines · On Mon, 1 Jul 2019 12:23:43 -0400 Farhan Ali <[email protected]> wrote:
Because
ccwchain_handle_ccwcallsccwchain_calc_lengthand as per the comment we should set orb.cmd.c64 before callingccwchanin_calc_length.Signed-off-by Farhan Ali <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-4@@ -640,16 +640,16 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) memcpy(&cp->orb, orb, sizeof(*orb)); cp->mdev = mdev; - /* 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. drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct · line 655
Quoted 10 lines
drivers/s390/cio/vfio_ccw_cp.c +4
-0*/ cp->orb.cmd.c64 = 1; + /* Build a ccwchain for the first CCW segment */ + ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); + if (ret) + cp_free(cp); + if (!ret) cp->initialized = true;Hm... has this ever been correct, or did this break only with the recent refactorings?
Quoted 1 line
Quoted 1 line
(IOW, what should Fixes: point to?)
Yeah, that looks like it should blame my refactoring.
Quoted 6 lines
Quoted 2 lines
I think it was correct before some of the new refactoring we did. But we do need to set before calling
ccwchain_calc_length, because the function does have a check for orb.cmd.64. I will see which exact commit did it.
Quoted 7 lines
Quoted 5 lines
I get why that check exists, but does anyone know why it's buried in
ccwchain_calc_length()? Is it simply becauseccwchain_calc_length()assumes to be working on Format-1 CCWs? I don't think that routine cares if it's an IDA or not, an it'd be nice if we could put a check for the supported IDA formats somewhere up front.The more I stare at this code, the more confused I get :(
That makes 2 of us :(
Quoted 7 lines
Apparently we want to allow the guest to specify an orb without cmd.c64 set, as this is fine as long as the channel program does not use idals. However, we _do_ want to reject it if cmd.c64 is not set, but idals are used; so we actually _don't_ want to force this before the processing. We just want the flag in the orb to be set when we do the ssch.
Quoted 8 lines
So it seems that the comment does not really talk about what
ccwchain_calc_length_will_ do, but what it _generally_ does (and, in this case, already would have done.)If my understanding is correct, maybe we should reword the comment instead? i.e. s/returns/would have returned/
I think you are right. But then should we move this to ccw_fetch_direct? It might be easier to understand the code, since that's where we are converting guest ccws to host idals?
Thanks
Farhan
RFC 2/4 vfio-ccw: No need to call cp_free on an error in cp_init
In reply to Farhan AliWe don't set cp->initialized to true so calling cp_free will just return and not do anything.
--- drivers/s390/cio/vfio_ccw_cp.c | 2 -- 1 file changed, 2 deletions(-)
drivers/s390/cio/vfio_ccw_cp.c +0 -2
@@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)/* Build a ccwchain for the first CCW segment */ ret =ccwchain_handle_ccw (orb->cmd.cpa, cp); -if (ret) -cp_free (cp);if (!ret) cp->initialized = true;
Signature · 1 line
2.7.4
Re: [RFC v1 2/4] vfio-ccw: No need to call cp_free on an error in cp_init
Quoted 13 lines · On Mon, 1 Jul 2019 12:23:44 -0400 Farhan Ali <[email protected]> wrote:
We don't set
cp->initializedto true so callingcp_freewill just return and not do anything.Signed-off-by Farhan Ali <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 2 -- 1 file changed, 2 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-0
drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) · line 651
Quoted 5 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-2@@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)/* Build a ccwchain for the first CCW segment */ ret =ccwchain_handle_ccw (orb->cmd.cpa, cp); -if (ret) -cp_free (cp);
Makes sense; hopefully ccwchain_handle_ccw() cleans up correctly on error :) (I think it does)
Maybe add a comment
/* ccwchain_handle_ccw() already cleans up on error */
so we don't stumble over this in the future?
(Also, does this want a Fixes: tag?)
Quoted 3 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-0if (!ret) cp->initialized = true;
Re: [RFC v1 2/4] vfio-ccw: No need to call cp_free on an error in cp_init
Quoted 17 lines · Cornelia Huck
Quoted 14 lines · On Mon, 1 Jul 2019 12:23:44 -0400 Farhan Ali <[email protected]> wrote:
We don't set
cp->initializedto true so callingcp_freewill just return and not do anything.Signed-off-by Farhan Ali <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 2 -- 1 file changed, 2 deletions(-)drivers/s390/cio/vfio_ccw_cp.c +0
-0@@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
Quoted 8 lines
drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) · line 651
Quoted 4 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-2/* Build a ccwchain for the first CCW segment */ ret =ccwchain_handle_ccw (orb->cmd.cpa, cp); -if (ret) -cp_free (cp);Makes sense; hopefully
ccwchain_handle_ccw()cleans up correctly on error :) (I think it does)
I have checked that it does as well, but wouldn't hurt if someone else also glances over once again :)
Quoted 5 lines
Maybe add a comment
/*
ccwchain_handle_ccw()already cleans up on error */so we don't stumble over this in the future?
Sure.
Quoted 2 lines
(Also, does this want a Fixes: tag?)
This might warrant a fixes tag as well.
Quoted 6 lines
Quoted 4 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-0if (!ret) cp->initialized = true;
Eric Farman
maintainerRe: [RFC v1 2/4] vfio-ccw: No need to call cp_free on an error in cp_init
Quoted 30 lines · Farhan Ali
Quoted 11 lines · Cornelia Huck
Quoted 8 lines · On Mon, 1 Jul 2019 12:23:44 -0400 Farhan Ali <[email protected]> wrote:
We don't set
cp->initializedto true so callingcp_freewill just return and not do anything.Signed-off-by Farhan Ali <[email protected]>---
drivers/s390/cio/vfio_ccw_cp.c| 2 --1 file changed, 2 deletions(-)Quoted 15 lines
drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct · line 651
Quoted 11 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-2@@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) /* Build a ccwchain for the first CCW segment */ ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); - if (ret) - cp_free(cp);Makes sense; hopefully
ccwchain_handle_ccw()cleans up correctly on error :) (I think it does)
Quoted 2 lines
I have checked that it does as well, but wouldn't hurt if someone else also glances over once again :)
Oh noes. What happens once we start encountering TICs? If we do:
ccwchain_handle_ccw() (OK)ccwchain_loop_tic() (OK)ccwchain_handle_ccw() (FAIL)
The first _handle_ccw() will have added a ccwchain to the cp list, which doesn't appear to get cleaned up now. That used to be done in cp_init() until I squashed cp_free and cp_unpin_free. :(
Quoted 18 lines
Quoted 6 lines
Maybe add a comment
/*
ccwchain_handle_ccw()already cleans up on error */so we don't stumble over this in the future?
Sure.
Quoted 2 lines
(Also, does this want a Fixes: tag?)
This might warrant a fixes tag as well.
Quoted 5 lines
Quoted 3 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-0if (!ret) cp->initialized = true;
Re: [RFC v1 2/4] vfio-ccw: No need to call cp_free on an error in cp_init
Quoted 38 lines · Eric Farman
Quoted 30 lines · Farhan Ali
Quoted 11 lines · Cornelia Huck
Quoted 8 lines · On Mon, 1 Jul 2019 12:23:44 -0400 Farhan Ali <[email protected]> wrote:
We don't set
cp->initializedto true so callingcp_freewill just return and not do anything.Signed-off-by Farhan Ali <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 2 -- 1 file changed, 2 deletions(-)Quoted 15 lines
drivers/s390/cio/vfio_ccw_cp.c int cp_init(struct channel_program *cp, struct · line 651
Quoted 11 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-2@@ -647,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) /* Build a ccwchain for the first CCW segment */ ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); - if (ret) - cp_free(cp);Makes sense; hopefully
ccwchain_handle_ccw()cleans up correctly on error :) (I think it does)Quoted 2 lines
I have checked that it does as well, but wouldn't hurt if someone else also glances over once again :)
Oh noes. What happens once we start encountering TICs? If we do:
Quoted 7 lines
ccwchain_handle_ccw()(OK)ccwchain_loop_tic()(OK)ccwchain_handle_ccw()(FAIL)The first
_handle_ccw()will have added a ccwchain to the cp list, which doesn't appear to get cleaned up now. That used to be done incp_init()until I squashedcp_freeandcp_unpin_free. :(
Yup, you are right we are not freeing the chain correctly. Will fix it in v2.
Quoted 20 lines
Quoted 19 lines
Quoted 7 lines
Maybe add a comment
/*
ccwchain_handle_ccw()already cleans up on error */so we don't stumble over this in the future?
Sure.
Quoted 2 lines
(Also, does this want a Fixes: tag?)
This might warrant a fixes tag as well.
Quoted 5 lines
Quoted 3 lines
drivers/s390/cio/vfio_ccw_cp.c +0
-0if (!ret) cp->initialized = true;
RFC 3/4 vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn
In reply to Farhan AliSo we clean up correctly.
--- drivers/s390/cio/vfio_ccw_cp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/s390/cio/vfio_ccw_cp.c +3 -1
@@ -72,8 +72,10 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len)sizeof (*pa->pa_iova_pfn) +sizeof (*pa->pa_pfn), GFP_KERNEL); -if (unlikely (!pa->pa_iova_pfn)) +if (unlikely (!pa->pa_iova_pfn)) { + pa->pa_nr =0 ;return -ENOMEM ; + } pa->pa_pfn = pa->pa_iova_pfn + pa->pa_nr; pa->pa_iova_pfn[0 ] = pa->pa_iova >>PAGE_SHIFT ;
Signature · 1 line
2.7.4
Re: [RFC v1 3/4] vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn
Quoted 13 lines · On Mon, 1 Jul 2019 12:23:45 -0400 Farhan Ali <[email protected]> wrote:
So we clean up correctly.
Signed-off-by Farhan Ali <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/s390/cio/vfio_ccw_cp.c +0
-0@@ -72,8 +72,10 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len) sizeof(*pa->pa_iova_pfn) +
drivers/s390/cio/vfio_ccw_cp.c static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len) · line 81
Quoted 10 lines
drivers/s390/cio/vfio_ccw_cp.c +3
-1sizeof (*pa->pa_pfn), GFP_KERNEL); -if (unlikely (!pa->pa_iova_pfn)) +if (unlikely (!pa->pa_iova_pfn)) { + pa->pa_nr =0 ;return -ENOMEM ; + } pa->pa_pfn = pa->pa_iova_pfn + pa->pa_nr; pa->pa_iova_pfn[0 ] = pa->pa_iova >>PAGE_SHIFT ;
This looks like an older error -- can you give a Fixes: tag? (Yeah, I know I sound like a broken record wrt that tag... :)
Re: [RFC v1 3/4] vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn
Quoted 25 lines · Cornelia Huck
Quoted 22 lines · On Mon, 1 Jul 2019 12:23:45 -0400 Farhan Ali <[email protected]> wrote:
So we clean up correctly.
Signed-off-by Farhan Ali <[email protected]>--- drivers/s390/cio/vfio_ccw_cp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/s390/cio/vfio_ccw_cp.c +3
-1@@ -72,8 +72,10 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len)sizeof (*pa->pa_iova_pfn) +sizeof (*pa->pa_pfn), GFP_KERNEL); -if (unlikely (!pa->pa_iova_pfn)) +if (unlikely (!pa->pa_iova_pfn)) { + pa->pa_nr =0 ;return -ENOMEM ; + } pa->pa_pfn = pa->pa_iova_pfn + pa->pa_nr;
Quoted 5 lines
drivers/s390/cio/vfio_ccw_cp.c static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len) · line 81
Quoted 1 line
drivers/s390/cio/vfio_ccw_cp.c +0
-0pa->pa_iova_pfn[0 ] = pa->pa_iova >>PAGE_SHIFT ;This looks like an older error -- can you give a Fixes: tag? (Yeah, I know I sound like a broken record wrt that tag... :)
Yes, this is an older error. And yup I will add a fixes tag :)
Re: [RFC v1 3/4] vfio-ccw: Set pa_nr to 0 if memory allocation fails for pa_iova_pfn
Quoted 7 lines · Farhan Ali
Quoted 4 lines · Cornelia Huck
Quoted 1 line · On Mon, 1 Jul 2019 12:23:45 -0400 Farhan Ali <[email protected]> wrote:
So we clean up correctly.
You mean, "so we don't try to call vfio_unpin_pages()" ?
Quoted 27 lines
Quoted 18 lines
Quoted 18 lines
Signed-off-by Farhan Ali <[email protected]>---
drivers/s390/cio/vfio_ccw_cp.c| 4 +++-1 file changed, 3 insertions(+), 1 deletion(-)drivers/s390/cio/vfio_ccw_cp.c +0
-0@@ -72,8 +72,10 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len)
sizeof(*pa->pa_iova_pfn)+
sizeof(*pa->pa_pfn),
GFP_KERNEL);
- if (unlikely(!pa->pa_iova_pfn))
+ if (unlikely(!pa->pa_iova_pfn)) {Quoted 9 lines
Quoted 5 lines
+
pa->pa_nr= 0;
return-ENOMEM;
+ }
pa->pa_pfn=pa->pa_iova_pfn+pa->pa_nr; pa->pa_iova_pfn[0] =pa->pa_iova>>PAGE_SHIFT;This looks like an older error -- can you give a Fixes: tag? (Yeah, I know I sound like a broken record wrt that tag... :)
Quoted 1 line
Yes, this is an older error. And yup I will add a fixes tag :)
Seems okay.
RFC 4/4 vfio-ccw: Don't call cp_free if we are processing a channel program
In reply to Farhan AliThere is a small window where it's possible that we could be working on an interrupt (queued in the workqueue) and setting up a channel program (i.e allocating memory, pinning pages, translating address). This can lead to allocating and freeing the channel program at the same time and can cause memory corruption.
Let's not call cp_free if we are currently processing a channel program. The only way we know for sure that we don't have a thread setting up a channel program is when the state is set to VFIO_CCW_STATE_CP_PENDING.
--- drivers/s390/cio/vfio_ccw_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/s390/cio/vfio_ccw_drv.c +1 -1
@@ -92,7 +92,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));if (scsw_is_solicited (&irb->scsw)) {cp_update_scsw (&private->cp, &irb->scsw); -if (is_final) +if (is_final && private->state ==VFIO_CCW_STATE_CP_PENDING )cp_free (&private->cp); }mutex_lock (&private->io_mutex);
Signature · 1 line
2.7.4
Re: [RFC v1 4/4] vfio-ccw: Don't call cp_free if we are processing a channel program
Quoted 5 lines · On Mon, 1 Jul 2019 12:23:46 -0400 Farhan Ali <[email protected]> wrote:
There is a small window where it's possible that we could be working on an interrupt (queued in the workqueue) and setting up a channel program (i.e allocating memory, pinning pages, translating address). This can lead to allocating and freeing the channel program at the same time and can cause memory corruption.
This can only happen if the interrupt is for a halt/clear operation, right?
Quoted 4 lines
Let's not call
cp_freeif we are currently processing a channel program. The only way we know for sure that we don't have a thread setting up a channel program is when the state is set toVFIO_CCW_STATE_CP_PENDING.
I have looked through the code again and I think you are right.
Quoted 11 lines
Signed-off-by Farhan Ali <[email protected]>--- drivers/s390/cio/vfio_ccw_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/s390/cio/vfio_ccw_drv.c +0
-0
drivers/s390/cio/vfio_ccw_drv.c static void vfio_ccw_sch_io_todo(struct work_struct *work) · line 95
Quoted 5 lines
drivers/s390/cio/vfio_ccw_drv.c +1
-1@@ -92,7 +92,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work) (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));if (scsw_is_solicited (&irb->scsw)) {cp_update_scsw (&private->cp, &irb->scsw); -if (is_final) +if (is_final && private->state ==VFIO_CCW_STATE_CP_PENDING )
Do we actually want to call cp_update_scsw() unconditionally?
At this point, we know that we have a solicited interrupt; that may be for several reasons:
- Interrupt for something we issued via ssch; it makes sense to update the scsw with the cpa address.
- Interrupt for a csch; the cpa address will be unpredictable, even if we did a ssch before. cp_update_scsw() hopefully can deal with that? Given that its purpose is to translate the cpa back, any unpredictable value in the scsw should be fine in the end.
- Interrupt for a hsch after we did a ssch; the cpa might be valid (see figure 16-6).
- Interrupt for a hsch without a prior ssch; we'll end up with an unpredictable cpa, again.
So I *think* we're fine with calling cp_update_scsw() in all cases, even if there's junk in the cpa of the scsw we get from the hardware. Opinions?
Quoted 3 lines
drivers/s390/cio/vfio_ccw_drv.c +0
-0cp_free (&private->cp); }mutex_lock (&private->io_mutex);