| Message ID | 20190327152107.29288-1-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v4] x86/mm: Clean up p2m_finish_type_change return value | expand |
>>> On 27.03.19 at 16:21, <aisaila@bitdefender.com> wrote: > @@ -621,7 +623,7 @@ bool_t ept_handle_misconfig(uint64_t gpa) > > p2m_unlock(p2m); > > - return spurious ? (rc >= 0) : (rc > 0); > + return spurious && !rc; > } I think you've gone too far now: This is - afaict - the one place where the distinction matters. Looking back at Paul's reply and my subsequent one on v3, I'm afraid I've managed to misguide you by not looking closely enough at what Paul did sketch out. I'm sorry for this. I think you either want to leave EPT code untouched, and zap the positive return value in finish_type_change() instead. Or EPT's resolve_misconfig() would need to gain a wrapper for use as the ->recalc hook, to squash the positive value for the outside world. Iirc I've avoided introducing such a wrapper originally just to limit the number of functions layered on top of one another, while using resolve_misconfig() directly appeared to be fine. > @@ -668,7 +670,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn, > > /* Carry out any eventually pending earlier changes first. */ > ret = resolve_misconfig(p2m, gfn); > - if ( ret < 0 ) > + if ( ret ) > return ret; This would then need undoing as well. > @@ -1201,7 +1201,7 @@ int p2m_finish_type_change(struct domain *d, > > rc = finish_type_change(hostp2m, first_gfn, max_nr); > > - if ( rc < 0 ) > + if ( rc ) > goto out; While I don't really object to this change, I also don't think it's strictly necessary. Jan
On 3/27/19 3:21 PM, 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(). Thanks for putting in the effort to clean this up. One comment: this is the second instance of the patch you posted where this paragraph is not true. What I wrote was meant to be an example of how a good patch description should be written, which includes a sketch of what the patch does technically. You need to make sure the sketch matches the patch. As it happens, it looks like you'll have to modify the patch such that v5 actually matches the description again. :-) Also, you probably should have dropped the RFC at v2. -George
On 27.03.2019 18:07, Jan Beulich wrote: >>>> On 27.03.19 at 16:21, <aisaila@bitdefender.com> wrote: >> @@ -621,7 +623,7 @@ bool_t ept_handle_misconfig(uint64_t gpa) >> >> p2m_unlock(p2m); >> >> - return spurious ? (rc >= 0) : (rc > 0); >> + return spurious && !rc; >> } > > I think you've gone too far now: This is - afaict - the one place > where the distinction matters. Looking back at Paul's reply and > my subsequent one on v3, I'm afraid I've managed to misguide > you by not looking closely enough at what Paul did sketch out. > I'm sorry for this. > > I think you either want to leave EPT code untouched, and zap > the positive return value in finish_type_change() instead. Or > EPT's resolve_misconfig() would need to gain a wrapper for use > as the ->recalc hook, to squash the positive value for the outside > world. Iirc I've avoided introducing such a wrapper originally > just to limit the number of functions layered on top of one > another, while using resolve_misconfig() directly appeared to > be fine. > It's alright, it's an honest mistake and in this case I will go back and have finish_type_change() cut the positive value if that is ok with everyone. Regards, Alex >> @@ -668,7 +670,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn, >> >> /* Carry out any eventually pending earlier changes first. */ >> ret = resolve_misconfig(p2m, gfn); >> - if ( ret < 0 ) >> + if ( ret ) >> return ret; > > This would then need undoing as well. > >> @@ -1201,7 +1201,7 @@ int p2m_finish_type_change(struct domain *d, >> >> rc = finish_type_change(hostp2m, first_gfn, max_nr); >> >> - if ( rc < 0 ) >> + if ( rc ) >> goto out; > > While I don't really object to this change, I also don't think it's > strictly necessary. >
On 27.03.2019 18:07, Jan Beulich wrote: >>>> On 27.03.19 at 16:21, <aisaila@bitdefender.com> wrote: >> @@ -621,7 +623,7 @@ bool_t ept_handle_misconfig(uint64_t gpa) >> >> p2m_unlock(p2m); >> >> - return spurious ? (rc >= 0) : (rc > 0); >> + return spurious && !rc; >> } > > I think you've gone too far now: This is - afaict - the one place > where the distinction matters. Looking back at Paul's reply and > my subsequent one on v3, I'm afraid I've managed to misguide > you by not looking closely enough at what Paul did sketch out. > I'm sorry for this. > > I think you either want to leave EPT code untouched, and zap > the positive return value in finish_type_change() instead. Or > EPT's resolve_misconfig() would need to gain a wrapper for use > as the ->recalc hook, to squash the positive value for the outside > world. Iirc I've avoided introducing such a wrapper originally > just to limit the number of functions layered on top of one > another, while using resolve_misconfig() directly appeared to > be fine. > Sorry the last reply got mixed up this was my reply: It's alright, it's an honest mistake and in this case I will go back and have finish_type_change() cut the positive value if that is ok with everyone. Regards, Alex >> @@ -668,7 +670,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn, >> >> /* Carry out any eventually pending earlier changes first. */ >> ret = resolve_misconfig(p2m, gfn); >> - if ( ret < 0 ) >> + if ( ret ) >> return ret; > > This would then need undoing as well. > >> @@ -1201,7 +1201,7 @@ int p2m_finish_type_change(struct domain *d, >> >> rc = finish_type_change(hostp2m, first_gfn, max_nr); >> >> - if ( rc < 0 ) >> + if ( rc ) >> goto out; > > While I don't really object to this change, I also don't think it's > strictly necessary. >
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index e3044bee2e..d336c138b0 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -459,8 +459,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m, * for entries not involved in the translation of the given GFN. * Returns: * - negative errno values in error, - * - zero if no adjustment was done, - * - a positive value if at least one adjustment was done. + * - zero for ok */ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) { @@ -600,6 +599,9 @@ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) v->arch.hvm.vmx.ept_spurious_misconfig = 1; } + if ( rc > 0 ) + rc = 0; + return rc; } @@ -621,7 +623,7 @@ bool_t ept_handle_misconfig(uint64_t gpa) p2m_unlock(p2m); - return spurious ? (rc >= 0) : (rc > 0); + return spurious && !rc; } /* @@ -668,7 +670,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn, /* Carry out any eventually pending earlier changes first. */ ret = resolve_misconfig(p2m, gfn); - if ( ret < 0 ) + if ( ret ) return ret; ASSERT((target == 2 && hap_has_1gb) || diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index b9bbb8f485..d5690b96bf 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1172,7 +1172,7 @@ static int finish_type_change(struct p2m_domain *p2m, { rc = p2m->recalc(p2m, gfn); /* - * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return + * ept->recalc could return 0/-ENOMEM. pt->recalc could return * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping * gfn here. */ @@ -1201,7 +1201,7 @@ int p2m_finish_type_change(struct domain *d, rc = finish_type_change(hostp2m, first_gfn, max_nr); - if ( rc < 0 ) + if ( rc ) goto out; #ifdef CONFIG_HVM @@ -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 V3: - Remove positive returned values from p2m->recalc. --- xen/arch/x86/mm/p2m-ept.c | 10 ++++++---- xen/arch/x86/mm/p2m.c | 15 +++++---------- 2 files changed, 11 insertions(+), 14 deletions(-)