| Message ID | 20190329124940.24091-1-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] x86/mm: Clean up p2m_finish_type_change return value | expand |
>>> 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
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 >
> -----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
>>> 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
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>
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
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(-)