| Message ID | 20190322120848.31528-1-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] x86/mm: Clean up p2m_finish_type_change return value | expand |
> -----Original Message----- > From: Alexandru Stefan ISAILA [mailto:aisaila@bitdefender.com] > Sent: 22 March 2019 12:09 > To: xen-devel@lists.xenproject.org > Cc: Paul Durrant <Paul.Durrant@citrix.com>; jbeulich@suse.com; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; > George Dunlap <George.Dunlap@citrix.com>; Alexandru Stefan ISAILA <aisaila@bitdefender.com> > Subject: [PATCH v3] x86/mm: Clean up p2m_finish_type_change return value > > 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(). Sorry, I don't think I was cc-ed on the original and I managed to miss George cc-ing me on his response to v2. Digging into the code a bit I can't honestly see what the point of returning anything other than 0/-errno out of p2m->recalc is. The only use of rc > 0 from p2m-ept.c:resolve_misconfig() is in ept_handle_misconfig() AFAICT so it would make more sense to me to tighten up the semantics of recalc (which I believe Jan suggested in response to v1) and turn any > 0 return from resolve_misconfig() into 0. So, the code below looks fine but the patch just needs to do a little more (and then your rc < 0 test can also be simplified to !rc too). Paul > > Suggested-by: George Dunlap <george.dunlap@eu.citrix.com> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > > --- > Changes since V2: > - Update commit msg. > --- > xen/arch/x86/mm/p2m.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index b9bbb8f485..bcf8cad423 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1213,22 +1213,17 @@ 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 > + rc = 0; > > out: > p2m_unlock(hostp2m); > -- > 2.17.1
>>> On 25.03.19 at 10:09, <Paul.Durrant@citrix.com> wrote: >> From: Alexandru Stefan ISAILA [mailto:aisaila@bitdefender.com] >> Sent: 22 March 2019 12:09 >> >> 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(). > > Sorry, I don't think I was cc-ed on the original and I managed to miss George > cc-ing me on his response to v2. Digging into the code a bit I can't honestly > see what the point of returning anything other than 0/-errno out of p2m->recalc > is. The only use of rc > 0 from p2m-ept.c:resolve_misconfig() is in > ept_handle_misconfig() AFAICT so it would make more sense to me to tighten up > the semantics of recalc (which I believe Jan suggested in response to v1) and > turn any > 0 return from resolve_misconfig() into 0. So, the code below looks > fine but the patch just needs to do a little more (and then your rc < 0 test > can also be simplified to !rc too). Yes, indeed I'd prefer everything to be cleaned up in one go. Jan
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index b9bbb8f485..bcf8cad423 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1213,22 +1213,17 @@ 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 + rc = 0; out: p2m_unlock(hostp2m);
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 V2: - Update commit msg. --- xen/arch/x86/mm/p2m.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)