* [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
@ 2019-03-29 12:50 Alexandru Stefan ISAILA
2019-03-29 13:01 ` Jan Beulich
2019-04-10 8:19 ` [Xen-devel] " Alexandru Stefan ISAILA
0 siblings, 2 replies; 10+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-03-29 12:50 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, george.dunlap, andrew.cooper3, Paul.Durrant, jbeulich,
Alexandru Stefan ISAILA, roger.pau
In the case of any errors, finish_type_change() passes values returned
from p2m->recalc() up the stack (with some exceptions in the case where
an error is expected); this eventually ends up being returned to the
XEN_DOMOP_map_mem_type_to_ioreq_server hypercall.
However, on Intel processors (but not on AMD processor), p2m->recalc()
can also return '1' as well as '0'. This case is handled very
inconsistently: finish_type_change() will return the value of the final
entry it attempts, discarding results for other entries;
p2m_finish_type_change() will attempt to accumulate '1's, so that it
returns '1' if any of the calls to finish_type_change() returns '1'; and
dm_op() will again return '1' only if the very last call to
p2m_finish_type_change() returns '1'. The result is that the
XEN_DMOP_map_mem_type_to_ioreq_server() hypercall will sometimes return
0 and sometimes return 1 on success, in an unpredictable manner.
The hypercall documentation doesn't mention return values; but it's not
clear what the caller could do with the information about whether
entries had been changed or not. At the moment it's always 0 on AMD
boxes, and *usually* 1 on Intel boxes; so nothing can be relying on a
'1' return value for correctness (or if it is, it's broken).
Make the return value on success consistently '0' by only returning
0/-ERROR from finish_type_change(). Also remove the accumulation code
from p2m_finish_type_change().
Suggested-by: George Dunlap <george.dunlap@eu.citrix.com>
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
Changes since V4:
- Undo changes in p2m->recalc() and have 1 turn into 0 in
finish_type_change().
---
xen/arch/x86/mm/p2m.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b9bbb8f485..5508cedc55 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1158,7 +1158,7 @@ void p2m_change_type_range(struct domain *d,
* Finish p2m type change for gfns which are marked as need_recalc in a range.
* Uses the current p2m's max_mapped_pfn to further clip the invalidation
* range for alternate p2ms.
- * Returns: 0/1 for success, negative for failure
+ * Returns: 0 for success, negative for failure
*/
static int finish_type_change(struct p2m_domain *p2m,
gfn_t first_gfn, unsigned long max_nr)
@@ -1174,9 +1174,9 @@ static int finish_type_change(struct p2m_domain *p2m,
/*
* ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
* 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
- * gfn here.
+ * gfn here. If rc is 1 we need to have it 0 for success.
*/
- if ( rc == -ENOENT )
+ if ( rc == -ENOENT || rc == 1 )
rc = 0;
else if ( rc < 0 )
{
@@ -1213,19 +1213,13 @@ int p2m_finish_type_change(struct domain *d,
if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
{
struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
- int rc1;
p2m_lock(altp2m);
- rc1 = finish_type_change(altp2m, first_gfn, max_nr);
+ rc = finish_type_change(altp2m, first_gfn, max_nr);
p2m_unlock(altp2m);
- if ( rc1 < 0 )
- {
- rc = rc1;
+ if ( rc < 0 )
goto out;
- }
-
- rc |= rc1;
}
}
#endif
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
2019-03-29 12:50 [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value Alexandru Stefan ISAILA
@ 2019-03-29 13:01 ` Jan Beulich
2019-04-10 10:05 ` [Xen-devel] " George Dunlap
2019-04-10 8:19 ` [Xen-devel] " Alexandru Stefan ISAILA
1 sibling, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2019-03-29 13:01 UTC (permalink / raw)
To: aisaila
Cc: Wei Liu, George Dunlap, Andrew Cooper, Paul Durrant, xen-devel,
Roger Pau Monne
>>> On 29.03.19 at 13:50, <aisaila@bitdefender.com> wrote:
> @@ -1174,9 +1174,9 @@ static int finish_type_change(struct p2m_domain *p2m,
> /*
> * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
> * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
> - * gfn here.
> + * gfn here. If rc is 1 we need to have it 0 for success.
> */
> - if ( rc == -ENOENT )
> + if ( rc == -ENOENT || rc == 1 )
I guess you mean "rc > 0" here? This and the comment are of course
easy enough to adjust while committing, and with the adjustments
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> @@ -1213,19 +1213,13 @@ int p2m_finish_type_change(struct domain *d,
> if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> {
> struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> - int rc1;
>
> p2m_lock(altp2m);
> - rc1 = finish_type_change(altp2m, first_gfn, max_nr);
> + rc = finish_type_change(altp2m, first_gfn, max_nr);
> p2m_unlock(altp2m);
>
> - if ( rc1 < 0 )
> - {
> - rc = rc1;
> + if ( rc < 0 )
Along the lines of an earlier comment - I think it is generally
better to not special case negative return values when positive
ones don't have special meaning. But I know the code base is
very inconsistent about this, so I don't really mind leaving it as
is.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
@ 2019-04-10 8:19 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 10+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-04-10 8:19 UTC (permalink / raw)
To: xen-devel, george.dunlap, Paul.Durrant
Cc: andrew.cooper3, wei.liu2, jbeulich, roger.pau
Roger/Paul are you ok with the latest changes? Can this go in?
Regards,
Alex
On 29.03.2019 14:50, Alexandru Stefan ISAILA wrote:
> In the case of any errors, finish_type_change() passes values returned
> from p2m->recalc() up the stack (with some exceptions in the case where
> an error is expected); this eventually ends up being returned to the
> XEN_DOMOP_map_mem_type_to_ioreq_server hypercall.
>
> However, on Intel processors (but not on AMD processor), p2m->recalc()
> can also return '1' as well as '0'. This case is handled very
> inconsistently: finish_type_change() will return the value of the final
> entry it attempts, discarding results for other entries;
> p2m_finish_type_change() will attempt to accumulate '1's, so that it
> returns '1' if any of the calls to finish_type_change() returns '1'; and
> dm_op() will again return '1' only if the very last call to
> p2m_finish_type_change() returns '1'. The result is that the
> XEN_DMOP_map_mem_type_to_ioreq_server() hypercall will sometimes return
> 0 and sometimes return 1 on success, in an unpredictable manner.
>
> The hypercall documentation doesn't mention return values; but it's not
> clear what the caller could do with the information about whether
> entries had been changed or not. At the moment it's always 0 on AMD
> boxes, and *usually* 1 on Intel boxes; so nothing can be relying on a
> '1' return value for correctness (or if it is, it's broken).
>
> Make the return value on success consistently '0' by only returning
> 0/-ERROR from finish_type_change(). Also remove the accumulation code
> from p2m_finish_type_change().
>
> Suggested-by: George Dunlap <george.dunlap@eu.citrix.com>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>
> ---
> Changes since V4:
> - Undo changes in p2m->recalc() and have 1 turn into 0 in
> finish_type_change().
> ---
> xen/arch/x86/mm/p2m.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b9bbb8f485..5508cedc55 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1158,7 +1158,7 @@ void p2m_change_type_range(struct domain *d,
> * Finish p2m type change for gfns which are marked as need_recalc in a range.
> * Uses the current p2m's max_mapped_pfn to further clip the invalidation
> * range for alternate p2ms.
> - * Returns: 0/1 for success, negative for failure
> + * Returns: 0 for success, negative for failure
> */
> static int finish_type_change(struct p2m_domain *p2m,
> gfn_t first_gfn, unsigned long max_nr)
> @@ -1174,9 +1174,9 @@ static int finish_type_change(struct p2m_domain *p2m,
> /*
> * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
> * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
> - * gfn here.
> + * gfn here. If rc is 1 we need to have it 0 for success.
> */
> - if ( rc == -ENOENT )
> + if ( rc == -ENOENT || rc == 1 )
> rc = 0;
> else if ( rc < 0 )
> {
> @@ -1213,19 +1213,13 @@ int p2m_finish_type_change(struct domain *d,
> if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> {
> struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> - int rc1;
>
> p2m_lock(altp2m);
> - rc1 = finish_type_change(altp2m, first_gfn, max_nr);
> + rc = finish_type_change(altp2m, first_gfn, max_nr);
> p2m_unlock(altp2m);
>
> - if ( rc1 < 0 )
> - {
> - rc = rc1;
> + if ( rc < 0 )
> goto out;
> - }
> -
> - rc |= rc1;
> }
> }
> #endif
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
@ 2019-04-10 8:19 ` Alexandru Stefan ISAILA
0 siblings, 0 replies; 10+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-04-10 8:19 UTC (permalink / raw)
To: xen-devel, george.dunlap, Paul.Durrant
Cc: andrew.cooper3, wei.liu2, jbeulich, roger.pau
Roger/Paul are you ok with the latest changes? Can this go in?
Regards,
Alex
On 29.03.2019 14:50, Alexandru Stefan ISAILA wrote:
> In the case of any errors, finish_type_change() passes values returned
> from p2m->recalc() up the stack (with some exceptions in the case where
> an error is expected); this eventually ends up being returned to the
> XEN_DOMOP_map_mem_type_to_ioreq_server hypercall.
>
> However, on Intel processors (but not on AMD processor), p2m->recalc()
> can also return '1' as well as '0'. This case is handled very
> inconsistently: finish_type_change() will return the value of the final
> entry it attempts, discarding results for other entries;
> p2m_finish_type_change() will attempt to accumulate '1's, so that it
> returns '1' if any of the calls to finish_type_change() returns '1'; and
> dm_op() will again return '1' only if the very last call to
> p2m_finish_type_change() returns '1'. The result is that the
> XEN_DMOP_map_mem_type_to_ioreq_server() hypercall will sometimes return
> 0 and sometimes return 1 on success, in an unpredictable manner.
>
> The hypercall documentation doesn't mention return values; but it's not
> clear what the caller could do with the information about whether
> entries had been changed or not. At the moment it's always 0 on AMD
> boxes, and *usually* 1 on Intel boxes; so nothing can be relying on a
> '1' return value for correctness (or if it is, it's broken).
>
> Make the return value on success consistently '0' by only returning
> 0/-ERROR from finish_type_change(). Also remove the accumulation code
> from p2m_finish_type_change().
>
> Suggested-by: George Dunlap <george.dunlap@eu.citrix.com>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>
> ---
> Changes since V4:
> - Undo changes in p2m->recalc() and have 1 turn into 0 in
> finish_type_change().
> ---
> xen/arch/x86/mm/p2m.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b9bbb8f485..5508cedc55 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1158,7 +1158,7 @@ void p2m_change_type_range(struct domain *d,
> * Finish p2m type change for gfns which are marked as need_recalc in a range.
> * Uses the current p2m's max_mapped_pfn to further clip the invalidation
> * range for alternate p2ms.
> - * Returns: 0/1 for success, negative for failure
> + * Returns: 0 for success, negative for failure
> */
> static int finish_type_change(struct p2m_domain *p2m,
> gfn_t first_gfn, unsigned long max_nr)
> @@ -1174,9 +1174,9 @@ static int finish_type_change(struct p2m_domain *p2m,
> /*
> * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
> * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
> - * gfn here.
> + * gfn here. If rc is 1 we need to have it 0 for success.
> */
> - if ( rc == -ENOENT )
> + if ( rc == -ENOENT || rc == 1 )
> rc = 0;
> else if ( rc < 0 )
> {
> @@ -1213,19 +1213,13 @@ int p2m_finish_type_change(struct domain *d,
> if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> {
> struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> - int rc1;
>
> p2m_lock(altp2m);
> - rc1 = finish_type_change(altp2m, first_gfn, max_nr);
> + rc = finish_type_change(altp2m, first_gfn, max_nr);
> p2m_unlock(altp2m);
>
> - if ( rc1 < 0 )
> - {
> - rc = rc1;
> + if ( rc < 0 )
> goto out;
> - }
> -
> - rc |= rc1;
> }
> }
> #endif
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
@ 2019-04-10 8:43 ` Paul Durrant
0 siblings, 0 replies; 10+ messages in thread
From: Paul Durrant @ 2019-04-10 8:43 UTC (permalink / raw)
To: 'Alexandru Stefan ISAILA', xen-devel, George Dunlap
Cc: Andrew Cooper, Wei Liu, jbeulich, Roger Pau Monne
> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Alexandru Stefan ISAILA
> Sent: 10 April 2019 09:19
> To: xen-devel@lists.xenproject.org; George Dunlap <George.Dunlap@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; jbeulich@suse.com; Roger
> Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
>
> Roger/Paul are you ok with the latest changes? Can this go in?
>
Yes, I am happy for my R-b to be added if so desired.
Paul
> Regards,
> Alex
>
> On 29.03.2019 14:50, Alexandru Stefan ISAILA wrote:
> > In the case of any errors, finish_type_change() passes values returned
> > from p2m->recalc() up the stack (with some exceptions in the case where
> > an error is expected); this eventually ends up being returned to the
> > XEN_DOMOP_map_mem_type_to_ioreq_server hypercall.
> >
> > However, on Intel processors (but not on AMD processor), p2m->recalc()
> > can also return '1' as well as '0'. This case is handled very
> > inconsistently: finish_type_change() will return the value of the final
> > entry it attempts, discarding results for other entries;
> > p2m_finish_type_change() will attempt to accumulate '1's, so that it
> > returns '1' if any of the calls to finish_type_change() returns '1'; and
> > dm_op() will again return '1' only if the very last call to
> > p2m_finish_type_change() returns '1'. The result is that the
> > XEN_DMOP_map_mem_type_to_ioreq_server() hypercall will sometimes return
> > 0 and sometimes return 1 on success, in an unpredictable manner.
> >
> > The hypercall documentation doesn't mention return values; but it's not
> > clear what the caller could do with the information about whether
> > entries had been changed or not. At the moment it's always 0 on AMD
> > boxes, and *usually* 1 on Intel boxes; so nothing can be relying on a
> > '1' return value for correctness (or if it is, it's broken).
> >
> > Make the return value on success consistently '0' by only returning
> > 0/-ERROR from finish_type_change(). Also remove the accumulation code
> > from p2m_finish_type_change().
> >
> > Suggested-by: George Dunlap <george.dunlap@eu.citrix.com>
> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> >
> > ---
> > Changes since V4:
> > - Undo changes in p2m->recalc() and have 1 turn into 0 in
> > finish_type_change().
> > ---
> > xen/arch/x86/mm/p2m.c | 16 +++++-----------
> > 1 file changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> > index b9bbb8f485..5508cedc55 100644
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -1158,7 +1158,7 @@ void p2m_change_type_range(struct domain *d,
> > * Finish p2m type change for gfns which are marked as need_recalc in a range.
> > * Uses the current p2m's max_mapped_pfn to further clip the invalidation
> > * range for alternate p2ms.
> > - * Returns: 0/1 for success, negative for failure
> > + * Returns: 0 for success, negative for failure
> > */
> > static int finish_type_change(struct p2m_domain *p2m,
> > gfn_t first_gfn, unsigned long max_nr)
> > @@ -1174,9 +1174,9 @@ static int finish_type_change(struct p2m_domain *p2m,
> > /*
> > * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
> > * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
> > - * gfn here.
> > + * gfn here. If rc is 1 we need to have it 0 for success.
> > */
> > - if ( rc == -ENOENT )
> > + if ( rc == -ENOENT || rc == 1 )
> > rc = 0;
> > else if ( rc < 0 )
> > {
> > @@ -1213,19 +1213,13 @@ int p2m_finish_type_change(struct domain *d,
> > if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> > {
> > struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> > - int rc1;
> >
> > p2m_lock(altp2m);
> > - rc1 = finish_type_change(altp2m, first_gfn, max_nr);
> > + rc = finish_type_change(altp2m, first_gfn, max_nr);
> > p2m_unlock(altp2m);
> >
> > - if ( rc1 < 0 )
> > - {
> > - rc = rc1;
> > + if ( rc < 0 )
> > goto out;
> > - }
> > -
> > - rc |= rc1;
> > }
> > }
> > #endif
> >
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
@ 2019-04-10 8:43 ` Paul Durrant
0 siblings, 0 replies; 10+ messages in thread
From: Paul Durrant @ 2019-04-10 8:43 UTC (permalink / raw)
To: 'Alexandru Stefan ISAILA', xen-devel, George Dunlap
Cc: Andrew Cooper, Wei Liu, jbeulich, Roger Pau Monne
> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Alexandru Stefan ISAILA
> Sent: 10 April 2019 09:19
> To: xen-devel@lists.xenproject.org; George Dunlap <George.Dunlap@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; jbeulich@suse.com; Roger
> Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
>
> Roger/Paul are you ok with the latest changes? Can this go in?
>
Yes, I am happy for my R-b to be added if so desired.
Paul
> Regards,
> Alex
>
> On 29.03.2019 14:50, Alexandru Stefan ISAILA wrote:
> > In the case of any errors, finish_type_change() passes values returned
> > from p2m->recalc() up the stack (with some exceptions in the case where
> > an error is expected); this eventually ends up being returned to the
> > XEN_DOMOP_map_mem_type_to_ioreq_server hypercall.
> >
> > However, on Intel processors (but not on AMD processor), p2m->recalc()
> > can also return '1' as well as '0'. This case is handled very
> > inconsistently: finish_type_change() will return the value of the final
> > entry it attempts, discarding results for other entries;
> > p2m_finish_type_change() will attempt to accumulate '1's, so that it
> > returns '1' if any of the calls to finish_type_change() returns '1'; and
> > dm_op() will again return '1' only if the very last call to
> > p2m_finish_type_change() returns '1'. The result is that the
> > XEN_DMOP_map_mem_type_to_ioreq_server() hypercall will sometimes return
> > 0 and sometimes return 1 on success, in an unpredictable manner.
> >
> > The hypercall documentation doesn't mention return values; but it's not
> > clear what the caller could do with the information about whether
> > entries had been changed or not. At the moment it's always 0 on AMD
> > boxes, and *usually* 1 on Intel boxes; so nothing can be relying on a
> > '1' return value for correctness (or if it is, it's broken).
> >
> > Make the return value on success consistently '0' by only returning
> > 0/-ERROR from finish_type_change(). Also remove the accumulation code
> > from p2m_finish_type_change().
> >
> > Suggested-by: George Dunlap <george.dunlap@eu.citrix.com>
> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> >
> > ---
> > Changes since V4:
> > - Undo changes in p2m->recalc() and have 1 turn into 0 in
> > finish_type_change().
> > ---
> > xen/arch/x86/mm/p2m.c | 16 +++++-----------
> > 1 file changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> > index b9bbb8f485..5508cedc55 100644
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -1158,7 +1158,7 @@ void p2m_change_type_range(struct domain *d,
> > * Finish p2m type change for gfns which are marked as need_recalc in a range.
> > * Uses the current p2m's max_mapped_pfn to further clip the invalidation
> > * range for alternate p2ms.
> > - * Returns: 0/1 for success, negative for failure
> > + * Returns: 0 for success, negative for failure
> > */
> > static int finish_type_change(struct p2m_domain *p2m,
> > gfn_t first_gfn, unsigned long max_nr)
> > @@ -1174,9 +1174,9 @@ static int finish_type_change(struct p2m_domain *p2m,
> > /*
> > * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
> > * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
> > - * gfn here.
> > + * gfn here. If rc is 1 we need to have it 0 for success.
> > */
> > - if ( rc == -ENOENT )
> > + if ( rc == -ENOENT || rc == 1 )
> > rc = 0;
> > else if ( rc < 0 )
> > {
> > @@ -1213,19 +1213,13 @@ int p2m_finish_type_change(struct domain *d,
> > if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> > {
> > struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> > - int rc1;
> >
> > p2m_lock(altp2m);
> > - rc1 = finish_type_change(altp2m, first_gfn, max_nr);
> > + rc = finish_type_change(altp2m, first_gfn, max_nr);
> > p2m_unlock(altp2m);
> >
> > - if ( rc1 < 0 )
> > - {
> > - rc = rc1;
> > + if ( rc < 0 )
> > goto out;
> > - }
> > -
> > - rc |= rc1;
> > }
> > }
> > #endif
> >
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
@ 2019-04-10 9:54 ` Jan Beulich
0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2019-04-10 9:54 UTC (permalink / raw)
To: aisaila
Cc: Wei Liu, George Dunlap, Andrew Cooper, Paul Durrant, xen-devel,
Roger Pau Monne
>>> On 10.04.19 at 10:19, <aisaila@bitdefender.com> wrote:
> Roger/Paul are you ok with the latest changes? Can this go in?
The thing keeping it from going in is the lack of George's ack. Judging
from the To: list of your mail you may even have meant him instead
of Roger.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
@ 2019-04-10 9:54 ` Jan Beulich
0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2019-04-10 9:54 UTC (permalink / raw)
To: aisaila
Cc: Wei Liu, George Dunlap, Andrew Cooper, Paul Durrant, xen-devel,
Roger Pau Monne
>>> On 10.04.19 at 10:19, <aisaila@bitdefender.com> wrote:
> Roger/Paul are you ok with the latest changes? Can this go in?
The thing keeping it from going in is the lack of George's ack. Judging
from the To: list of your mail you may even have meant him instead
of Roger.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
@ 2019-04-10 10:05 ` George Dunlap
0 siblings, 0 replies; 10+ messages in thread
From: George Dunlap @ 2019-04-10 10:05 UTC (permalink / raw)
To: Jan Beulich, aisaila
Cc: Wei Liu, George Dunlap, Andrew Cooper, Paul Durrant, xen-devel,
Roger Pau Monne
On 3/29/19 1:01 PM, Jan Beulich wrote:
>>>> On 29.03.19 at 13:50, <aisaila@bitdefender.com> wrote:
>> @@ -1174,9 +1174,9 @@ static int finish_type_change(struct p2m_domain *p2m,
>> /*
>> * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
>> * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
>> - * gfn here.
>> + * gfn here. If rc is 1 we need to have it 0 for success.
>> */
>> - if ( rc == -ENOENT )
>> + if ( rc == -ENOENT || rc == 1 )
>
> I guess you mean "rc > 0" here? This and the comment are of course
> easy enough to adjust while committing, and with the adjustments
+1
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value
@ 2019-04-10 10:05 ` George Dunlap
0 siblings, 0 replies; 10+ messages in thread
From: George Dunlap @ 2019-04-10 10:05 UTC (permalink / raw)
To: Jan Beulich, aisaila
Cc: Wei Liu, George Dunlap, Andrew Cooper, Paul Durrant, xen-devel,
Roger Pau Monne
On 3/29/19 1:01 PM, Jan Beulich wrote:
>>>> On 29.03.19 at 13:50, <aisaila@bitdefender.com> wrote:
>> @@ -1174,9 +1174,9 @@ static int finish_type_change(struct p2m_domain *p2m,
>> /*
>> * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
>> * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
>> - * gfn here.
>> + * gfn here. If rc is 1 we need to have it 0 for success.
>> */
>> - if ( rc == -ENOENT )
>> + if ( rc == -ENOENT || rc == 1 )
>
> I guess you mean "rc > 0" here? This and the comment are of course
> easy enough to adjust while committing, and with the adjustments
+1
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-10 10:07 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-29 12:50 [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value Alexandru Stefan ISAILA
2019-03-29 13:01 ` Jan Beulich
2019-04-10 10:05 ` George Dunlap
2019-04-10 10:05 ` [Xen-devel] " George Dunlap
2019-04-10 8:19 ` Alexandru Stefan ISAILA
2019-04-10 8:19 ` [Xen-devel] " Alexandru Stefan ISAILA
2019-04-10 8:43 ` Paul Durrant
2019-04-10 8:43 ` [Xen-devel] " Paul Durrant
2019-04-10 9:54 ` Jan Beulich
2019-04-10 9:54 ` [Xen-devel] " Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.