| Message ID | 1494635688-3855-1-git-send-email-xiong.y.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, May 13, 2017 at 1:34 AM, Xiong Zhang <xiong.y.zhang@intel.com> 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")' > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > Reviewed-by: George Dunlap <george.dunlap@ctrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.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) > Keep the name of p2m-specific recal function unchanged. (Jan) > v4: Move version info below S-o-B and handle return value of > p2m->recalc. (Jan) > v5: Fix coding style. (Julien) > > The target of this patch is Xen 4.9. > --- > xen/arch/x86/hvm/dm.c | 5 +++-- > xen/arch/x86/mm/p2m-ept.c | 1 + > xen/arch/x86/mm/p2m-pt.c | 1 + > xen/arch/x86/mm/p2m.c | 35 +++++++++++++++++++++++------------ > xen/include/asm-x86/p2m.h | 9 +++++---- > 5 files changed, 33 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c > index d72b7bd..99bf66a 100644 > --- a/xen/arch/x86/hvm/dm.c > +++ b/xen/arch/x86/hvm/dm.c > @@ -412,8 +412,9 @@ 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); > + rc = p2m_finish_type_change(d, _gfn(first_gfn), 256); > + if ( rc < 0 ) > + break; > > 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..1600422 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1011,33 +1011,44 @@ void p2m_change_type_range(struct domain *d, > p2m_unlock(p2m); > } > > -/* 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) > +/* > + * Finish p2m type change for gfns which are marked as need_recalc in a range. > + * Returns: 0/1 for success, negative for failure > + */ > +int p2m_finish_type_change(struct domain *d, > + 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)); > + int rc = 0; > > 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); > + rc = p2m->recalc(p2m, gfn); > + /* > + * 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. > + */ Hmm, now that these functions are being called externally it would be good if their semantics was the same. But since we're a bit late in the cycle to do that kind of rework: Acked-by: George Dunlap <george.dunlap@citrix.com>
Hi, On 15/05/17 14:47, George Dunlap wrote: > On Sat, May 13, 2017 at 1:34 AM, Xiong Zhang <xiong.y.zhang@intel.com> 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")' >> >> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> >> Reviewed-by: George Dunlap <george.dunlap@ctrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.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) >> Keep the name of p2m-specific recal function unchanged. (Jan) >> v4: Move version info below S-o-B and handle return value of >> p2m->recalc. (Jan) >> v5: Fix coding style. (Julien) >> >> The target of this patch is Xen 4.9. >> --- >> xen/arch/x86/hvm/dm.c | 5 +++-- >> xen/arch/x86/mm/p2m-ept.c | 1 + >> xen/arch/x86/mm/p2m-pt.c | 1 + >> xen/arch/x86/mm/p2m.c | 35 +++++++++++++++++++++++------------ >> xen/include/asm-x86/p2m.h | 9 +++++---- >> 5 files changed, 33 insertions(+), 18 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c >> index d72b7bd..99bf66a 100644 >> --- a/xen/arch/x86/hvm/dm.c >> +++ b/xen/arch/x86/hvm/dm.c >> @@ -412,8 +412,9 @@ 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); >> + rc = p2m_finish_type_change(d, _gfn(first_gfn), 256); >> + if ( rc < 0 ) >> + break; >> >> 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..1600422 100644 >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -1011,33 +1011,44 @@ void p2m_change_type_range(struct domain *d, >> p2m_unlock(p2m); >> } >> >> -/* 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) >> +/* >> + * Finish p2m type change for gfns which are marked as need_recalc in a range. >> + * Returns: 0/1 for success, negative for failure >> + */ >> +int p2m_finish_type_change(struct domain *d, >> + 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)); >> + int rc = 0; >> >> 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); >> + rc = p2m->recalc(p2m, gfn); >> + /* >> + * 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. >> + */ > > Hmm, now that these functions are being called externally it would be > good if their semantics was the same. But since we're a bit late in > the cycle to do that kind of rework: I guess this is a call for a rework after the tree has opened? > > Acked-by: George Dunlap <george.dunlap@citrix.com> > Release-acked-by: Julien Grall <julien.grall@arm.com> Cheers,
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c index d72b7bd..99bf66a 100644 --- a/xen/arch/x86/hvm/dm.c +++ b/xen/arch/x86/hvm/dm.c @@ -412,8 +412,9 @@ 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); + rc = p2m_finish_type_change(d, _gfn(first_gfn), 256); + if ( rc < 0 ) + break; 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..1600422 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1011,33 +1011,44 @@ void p2m_change_type_range(struct domain *d, p2m_unlock(p2m); } -/* 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) +/* + * Finish p2m type change for gfns which are marked as need_recalc in a range. + * Returns: 0/1 for success, negative for failure + */ +int p2m_finish_type_change(struct domain *d, + 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)); + int rc = 0; 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); + rc = p2m->recalc(p2m, gfn); + /* + * 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. + */ + if ( rc == -ENOENT ) + rc = 0; + else if ( rc < 0 ) + { + gdprintk(XENLOG_ERR, "p2m->recalc failed! Dom%d gfn=%lx\n", + d->domain_id, gfn); + break; + } gfn++; } p2m_unlock(p2m); + + return rc; } /* diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 7574a9b..408f7da 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); @@ -607,10 +609,9 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn, p2m_type_t ot, p2m_type_t nt); /* 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); +int p2m_finish_type_change(struct domain *d, + gfn_t first_gfn, + 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")' Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> Reviewed-by: George Dunlap <george.dunlap@ctrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.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) Keep the name of p2m-specific recal function unchanged. (Jan) v4: Move version info below S-o-B and handle return value of p2m->recalc. (Jan) v5: Fix coding style. (Julien) The target of this patch is Xen 4.9. --- xen/arch/x86/hvm/dm.c | 5 +++-- xen/arch/x86/mm/p2m-ept.c | 1 + xen/arch/x86/mm/p2m-pt.c | 1 + xen/arch/x86/mm/p2m.c | 35 +++++++++++++++++++++++------------ xen/include/asm-x86/p2m.h | 9 +++++---- 5 files changed, 33 insertions(+), 18 deletions(-)