| Message ID | 1494364964-3775-1-git-send-email-xiong.y.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/05/17 22:22, Xiong Zhang wrote: > 'commit 1679e0df3df6 ("x86/ioreq server: asynchronously reset > outstanding p2m_ioreq_server entries")' will call > p2m_change_entry_type_global() which set entry.recalc=1. Then > the following get_entry(p2m_ioreq_server) will return > p2m_ram_rw type. > But 'commit 6d774a951696 ("x86/ioreq server: synchronously reset > outstanding p2m_ioreq_server entries when an ioreq server unmaps")' > assume get_entry(p2m_ioreq_server) will return p2m_ioreq_server > type, then reset p2m_ioreq_server entries. The fact is the assumption > isn't true, and sysnchronously reset function couldn't work. Then > ioreq.entry_count is larger than zero after an ioreq server unmaps. > During XenGT domU reboot, it will unmap, map and unmap ioreq > server with old domid, the map will fail as ioreq.entry_count > 0 and > reboot process is terminated. > > This patch add p2m->recalc() hook which use the existing implementation > specific function as ept resolve_misconfig and pt do_recalc, so > p2m_finish_type_change() could call p2m->recalc() directly to > change gfn p2m_type which need recalc. This looks a lot nicer! Two things: I think I'd rewrite the changelog this way: --- x86/ioreq_server: Make p2m_finish_type_change actually work Commit 6d774a951696 ("x86/ioreq server: synchronously reset outstanding p2m_ioreq_server entries when an ioreq server unmaps") introduced p2m_finish_type_change(), which was meant to synchronously finish a previously initiated type change over a gpfn range. It did this by calling get_entry(), checking if it was the appropriate type, and then calling set_entry(). Unfortunately, a previous commit (1679e0df3df6 "x86/ioreq server: asynchronously reset outstanding p2m_ioreq_server entries") modified get_entry() to always return the new type after the type change, meaning that p2m_finish_type_change() never changed any entries. Which means when an ioreq server was detached and then re-attached (as happens in XenGT on reboot) the re-attach failed. Fix this by using the existing p2m-specific recalculation logic instead of doing a read-check-write loop. --- Also... > > Fix: 'commit 6d774a951696 ("x86/ioreq server: synchronously reset > outstanding p2m_ioreq_server entries when an ioreq server unmaps")' > > v1: Add ioreq_pre_recalc query flag to get the old p2m_type.(Jan) > v2: Add p2m->recalc() hook to change gfn p2m_type. (George) > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > --- > xen/arch/x86/hvm/dm.c | 3 +-- > xen/arch/x86/mm/p2m-ept.c | 7 ++++--- > xen/arch/x86/mm/p2m-pt.c | 7 ++++--- > xen/arch/x86/mm/p2m.c | 12 ++---------- > xen/include/asm-x86/p2m.h | 5 +++-- > 5 files changed, 14 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c > index d72b7bd..c1627ec 100644 > --- a/xen/arch/x86/hvm/dm.c > +++ b/xen/arch/x86/hvm/dm.c > @@ -412,8 +412,7 @@ static int dm_op(domid_t domid, > first_gfn <= p2m->max_mapped_pfn ) > { > /* Iterate p2m table for 256 gfns each time. */ > - p2m_finish_type_change(d, _gfn(first_gfn), 256, > - p2m_ioreq_server, p2m_ram_rw); > + p2m_finish_type_change(d, _gfn(first_gfn), 256); > > first_gfn += 256; > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c > index f37a1f2..f96bd3b 100644 > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m, > * - zero if no adjustment was done, > * - a positive value if at least one adjustment was done. > */ > -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) > +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) I think while we're renaming this I'd rename this to ept_do_recalc(). Thanks, -George
>>> On 09.05.17 at 11:44, <george.dunlap@citrix.com> wrote: > On 09/05/17 22:22, Xiong Zhang wrote: >> --- a/xen/arch/x86/mm/p2m-ept.c >> +++ b/xen/arch/x86/mm/p2m-ept.c >> @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m, >> * - zero if no adjustment was done, >> * - a positive value if at least one adjustment was done. >> */ >> -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) >> +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) > > I think while we're renaming this I'd rename this to ept_do_recalc(). Which gets me to ask (once again) what purpose the ept_ prefix has for a static function. I'd rather see this called do_recalc(), and the p2m-pt variant could be left unchanged altogether. Jan
On 09/05/17 11:08, Jan Beulich wrote: >>>> On 09.05.17 at 11:44, <george.dunlap@citrix.com> wrote: >> On 09/05/17 22:22, Xiong Zhang wrote: >>> --- a/xen/arch/x86/mm/p2m-ept.c >>> +++ b/xen/arch/x86/mm/p2m-ept.c >>> @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m, >>> * - zero if no adjustment was done, >>> * - a positive value if at least one adjustment was done. >>> */ >>> -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) >>> +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) >> >> I think while we're renaming this I'd rename this to ept_do_recalc(). > > Which gets me to ask (once again) what purpose the ept_ prefix > has for a static function. I'd rather see this called do_recalc(), and > the p2m-pt variant could be left unchanged altogether. Well we should have them both named do_recalc() (no prefix), or have them both tagged to specify which version they're for. ISTR people complaining about duplicate static symbols making things harder to debug (i.e., is this do_recalc() in the stack trace the p2m-pt version or the p2m-ept version?), so the latter is probably preferable. "p2m_pt_" does seem like kind of a long prefix, but that seems to be what the rest of the p2m_pt.c functions are called, so at this point it's probably best to follow suit. -George
> >>> On 09.05.17 at 11:44, <george.dunlap@citrix.com> wrote: > > On 09/05/17 22:22, Xiong Zhang wrote: > >> --- a/xen/arch/x86/mm/p2m-ept.c > >> +++ b/xen/arch/x86/mm/p2m-ept.c > >> @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct > p2m_domain *p2m, > >> * - zero if no adjustment was done, > >> * - a positive value if at least one adjustment was done. > >> */ > >> -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) > >> +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long > gfn) > > > > I think while we're renaming this I'd rename this to ept_do_recalc(). > > Which gets me to ask (once again) what purpose the ept_ prefix > has for a static function. I'd rather see this called do_recalc(), and > the p2m-pt variant could be left unchanged altogether. > [Zhang, Xiong Y] As all the functions with p2m have ept_ prefix in p2m-ept.c and have p2m_pt_ prefix in p2m-pt.c, then I guess there may be a potential rule to name these functions. If there isn't such rule, I will keep their name unchanged. Thanks. > Jan
>>> On 09.05.17 at 12:21, <george.dunlap@citrix.com> wrote: > On 09/05/17 11:08, Jan Beulich wrote: >>>>> On 09.05.17 at 11:44, <george.dunlap@citrix.com> wrote: >>> On 09/05/17 22:22, Xiong Zhang wrote: >>>> --- a/xen/arch/x86/mm/p2m-ept.c >>>> +++ b/xen/arch/x86/mm/p2m-ept.c >>>> @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain > *p2m, >>>> * - zero if no adjustment was done, >>>> * - a positive value if at least one adjustment was done. >>>> */ >>>> -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) >>>> +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) >>> >>> I think while we're renaming this I'd rename this to ept_do_recalc(). >> >> Which gets me to ask (once again) what purpose the ept_ prefix >> has for a static function. I'd rather see this called do_recalc(), and >> the p2m-pt variant could be left unchanged altogether. > > Well we should have them both named do_recalc() (no prefix), or have > them both tagged to specify which version they're for. ISTR people > complaining about duplicate static symbols making things harder to debug > (i.e., is this do_recalc() in the stack trace the p2m-pt version or the > p2m-ept version?), so the latter is probably preferable. But that's the reason I had done d37d63d4b5 ("symbols: prefix static symbols with their source file names") - they are distinguishable in stack traces nowadays. Jan
On 09/05/17 11:51, Jan Beulich wrote: >>>> On 09.05.17 at 12:21, <george.dunlap@citrix.com> wrote: >> On 09/05/17 11:08, Jan Beulich wrote: >>>>>> On 09.05.17 at 11:44, <george.dunlap@citrix.com> wrote: >>>> On 09/05/17 22:22, Xiong Zhang wrote: >>>>> --- a/xen/arch/x86/mm/p2m-ept.c >>>>> +++ b/xen/arch/x86/mm/p2m-ept.c >>>>> @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain >> *p2m, >>>>> * - zero if no adjustment was done, >>>>> * - a positive value if at least one adjustment was done. >>>>> */ >>>>> -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) >>>>> +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) >>>> >>>> I think while we're renaming this I'd rename this to ept_do_recalc(). >>> >>> Which gets me to ask (once again) what purpose the ept_ prefix >>> has for a static function. I'd rather see this called do_recalc(), and >>> the p2m-pt variant could be left unchanged altogether. >> >> Well we should have them both named do_recalc() (no prefix), or have >> them both tagged to specify which version they're for. ISTR people >> complaining about duplicate static symbols making things harder to debug >> (i.e., is this do_recalc() in the stack trace the p2m-pt version or the >> p2m-ept version?), so the latter is probably preferable. > > But that's the reason I had done d37d63d4b5 ("symbols: prefix static > symbols with their source file names") - they are distinguishable in > stack traces nowadays. Ah, right. In that case, Xiong, go ahead and leave the two functions named as the are. I plan at some point in the future to do a wider renaming if "resolve_misconfig" in p2m-ept.c, so I can change the name of all the related functions at the same time. -George
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c index d72b7bd..c1627ec 100644 --- a/xen/arch/x86/hvm/dm.c +++ b/xen/arch/x86/hvm/dm.c @@ -412,8 +412,7 @@ static int dm_op(domid_t domid, first_gfn <= p2m->max_mapped_pfn ) { /* Iterate p2m table for 256 gfns each time. */ - p2m_finish_type_change(d, _gfn(first_gfn), 256, - p2m_ioreq_server, p2m_ram_rw); + p2m_finish_type_change(d, _gfn(first_gfn), 256); first_gfn += 256; diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index f37a1f2..f96bd3b 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m, * - zero if no adjustment was done, * - a positive value if at least one adjustment was done. */ -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) { struct ept_data *ept = &p2m->ept; unsigned int level = ept->wl; @@ -659,7 +659,7 @@ bool_t ept_handle_misconfig(uint64_t gpa) p2m_lock(p2m); spurious = curr->arch.hvm_vmx.ept_spurious_misconfig; - rc = resolve_misconfig(p2m, PFN_DOWN(gpa)); + rc = ept_resolve_misconfig(p2m, PFN_DOWN(gpa)); curr->arch.hvm_vmx.ept_spurious_misconfig = 0; p2m_unlock(p2m); @@ -707,7 +707,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, return -EINVAL; /* Carry out any eventually pending earlier changes first. */ - ret = resolve_misconfig(p2m, gfn); + ret = ept_resolve_misconfig(p2m, gfn); if ( ret < 0 ) return ret; @@ -1238,6 +1238,7 @@ int ept_p2m_init(struct p2m_domain *p2m) p2m->set_entry = ept_set_entry; p2m->get_entry = ept_get_entry; + p2m->recalc = ept_resolve_misconfig; p2m->change_entry_type_global = ept_change_entry_type_global; p2m->change_entry_type_range = ept_change_entry_type_range; p2m->memory_type_changed = ept_memory_type_changed; diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index 5079b59..b0f6aa0 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -367,7 +367,7 @@ static int p2m_pt_set_recalc_range(struct p2m_domain *p2m, * GFN. Propagate the re-calculation flag down to the next page table level * for entries not involved in the translation of the given GFN. */ -static int do_recalc(struct p2m_domain *p2m, unsigned long gfn) +static int p2m_pt_do_recalc(struct p2m_domain *p2m, unsigned long gfn) { void *table; unsigned long gfn_remainder = gfn; @@ -493,7 +493,7 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa) int rc; p2m_lock(p2m); - rc = do_recalc(p2m, PFN_DOWN(gpa)); + rc = p2m_pt_do_recalc(p2m, PFN_DOWN(gpa)); p2m_unlock(p2m); return rc; @@ -555,7 +555,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, } /* Carry out any eventually pending earlier changes first. */ - rc = do_recalc(p2m, gfn); + rc = p2m_pt_do_recalc(p2m, gfn); if ( rc < 0 ) return rc; @@ -1153,6 +1153,7 @@ void p2m_pt_init(struct p2m_domain *p2m) { p2m->set_entry = p2m_pt_set_entry; p2m->get_entry = p2m_pt_get_entry; + p2m->recalc = p2m_pt_do_recalc; p2m->change_entry_type_global = p2m_pt_change_entry_type_global; p2m->change_entry_type_range = p2m_pt_change_entry_type_range; p2m->write_p2m_entry = paging_write_p2m_entry; diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 1d57e5c..2bad2e1 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1013,26 +1013,18 @@ void p2m_change_type_range(struct domain *d, /* Synchronously modify the p2m type for a range of gfns from ot to nt. */ void p2m_finish_type_change(struct domain *d, - gfn_t first_gfn, unsigned long max_nr, - p2m_type_t ot, p2m_type_t nt) + gfn_t first_gfn, unsigned long max_nr) { struct p2m_domain *p2m = p2m_get_hostp2m(d); - p2m_type_t t; unsigned long gfn = gfn_x(first_gfn); unsigned long last_gfn = gfn + max_nr - 1; - ASSERT(ot != nt); - ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt)); - p2m_lock(p2m); last_gfn = min(last_gfn, p2m->max_mapped_pfn); while ( gfn <= last_gfn ) { - get_gfn_query_unlocked(d, gfn, &t); - - if ( t == ot ) - p2m_change_type_one(d, gfn, t, nt); + p2m->recalc(p2m, gfn); gfn++; } diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 7574a9b..081639c 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -246,6 +246,8 @@ struct p2m_domain { p2m_query_t q, unsigned int *page_order, bool_t *sve); + int (*recalc)(struct p2m_domain *p2m, + unsigned long gfn); void (*enable_hardware_log_dirty)(struct p2m_domain *p2m); void (*disable_hardware_log_dirty)(struct p2m_domain *p2m); void (*flush_hardware_cached_dirty)(struct p2m_domain *p2m); @@ -609,8 +611,7 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn, /* Synchronously change the p2m type for a range of gfns */ void p2m_finish_type_change(struct domain *d, gfn_t first_gfn, - unsigned long max_nr, - p2m_type_t ot, p2m_type_t nt); + unsigned long max_nr); /* Report a change affecting memory types. */ void p2m_memory_type_changed(struct domain *d);
'commit 1679e0df3df6 ("x86/ioreq server: asynchronously reset outstanding p2m_ioreq_server entries")' will call p2m_change_entry_type_global() which set entry.recalc=1. Then the following get_entry(p2m_ioreq_server) will return p2m_ram_rw type. But 'commit 6d774a951696 ("x86/ioreq server: synchronously reset outstanding p2m_ioreq_server entries when an ioreq server unmaps")' assume get_entry(p2m_ioreq_server) will return p2m_ioreq_server type, then reset p2m_ioreq_server entries. The fact is the assumption isn't true, and sysnchronously reset function couldn't work. Then ioreq.entry_count is larger than zero after an ioreq server unmaps. During XenGT domU reboot, it will unmap, map and unmap ioreq server with old domid, the map will fail as ioreq.entry_count > 0 and reboot process is terminated. This patch add p2m->recalc() hook which use the existing implementation specific function as ept resolve_misconfig and pt do_recalc, so p2m_finish_type_change() could call p2m->recalc() directly to change gfn p2m_type which need recalc. Fix: 'commit 6d774a951696 ("x86/ioreq server: synchronously reset outstanding p2m_ioreq_server entries when an ioreq server unmaps")' v1: Add ioreq_pre_recalc query flag to get the old p2m_type.(Jan) v2: Add p2m->recalc() hook to change gfn p2m_type. (George) Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> --- xen/arch/x86/hvm/dm.c | 3 +-- xen/arch/x86/mm/p2m-ept.c | 7 ++++--- xen/arch/x86/mm/p2m-pt.c | 7 ++++--- xen/arch/x86/mm/p2m.c | 12 ++---------- xen/include/asm-x86/p2m.h | 5 +++-- 5 files changed, 14 insertions(+), 20 deletions(-)