| Message ID | 1494457022-5199-1-git-send-email-xiong.y.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/05/17 23:57, Xiong Zhang wrote: > 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. > > 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) > v3: Make commit message clearer. (George) These changes should be put like this.... > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> --- 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) v3: Make commit message clearer. (George) That is, below the S-o-B, you should add a line with '---', and then after that add any comments that you want to aim at the reviewers (such as changes between the versions) but not to end up checked into the tree. This can be fixed up on check-in. Other than that: Reviewed-by: George Dunlap <george.dunlap@citrix.com>
>>> On 11.05.17 at 00:57, <xiong.y.zhang@intel.com> wrote: As a general note: Please send patches _to_ the list, _cc_-ing maintainers and other relevant people. > 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) > v3: Make commit message clearer. (George) This doesn't belong into the commit message, so ought to go ... > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > --- ... below the first separation marker. (As a side note, v3 had more changes than just an adjustment to the commit message.) > --- 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); You shouldn't ignore the return value here. It being positive may be of no interest for the moment, but it being negative certainly is. Jan
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..09efba7 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -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 = 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..2eddeee 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -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 = 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 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. 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) v3: Make commit message clearer. (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 | 1 + xen/arch/x86/mm/p2m-pt.c | 1 + xen/arch/x86/mm/p2m.c | 12 ++---------- xen/include/asm-x86/p2m.h | 5 +++-- 5 files changed, 8 insertions(+), 14 deletions(-)