| Message ID | 1493849741-13355-1-git-send-email-xiong.y.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 04.05.17 at 00:15, <xiong.y.zhang@intel.com> 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, > finally this results DomU reboot couldn't work. > > This patch will let get_entry(p2m_ioreq_server) return > p2m_ioreq_server type instead of p2m_ram_rw type when the type of > ioreq_server entries havn't been written. The actual type change > happens in recalc funciton. I think this is the wrong solution to the problem: get_entry() is supposed to return the new type, when a type change was done but hasn't got pushed through the page table hierarchy. One option I can see would be to add a new flag to p2m_query_t, allowing to retrieve the currently recorded type instead of the mandated active one. Another might be to relax p2m_finish_type_change()'s old type check, accepting that this would lead to unnecessary calls to p2m_change_type_one(). It may be possible to avoid some of the extra overhead by e.g. also looking at the retrieved order - p2m_ioreq_server pages can only be order-0 right now, so higher order pages could be skipped. Jan
> >>> On 04.05.17 at 00:15, <xiong.y.zhang@intel.com> 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, > > finally this results DomU reboot couldn't work. > > > > This patch will let get_entry(p2m_ioreq_server) return > > p2m_ioreq_server type instead of p2m_ram_rw type when the type of > > ioreq_server entries havn't been written. The actual type change > > happens in recalc funciton. > > I think this is the wrong solution to the problem: get_entry() is > supposed to return the new type, when a type change was done > but hasn't got pushed through the page table hierarchy. One > option I can see would be to add a new flag to p2m_query_t, > allowing to retrieve the currently recorded type instead of the > mandated active one. Another might be to relax > p2m_finish_type_change()'s old type check, accepting that this > would lead to unnecessary calls to p2m_change_type_one(). It > may be possible to avoid some of the extra overhead by e.g. > also looking at the retrieved order - p2m_ioreq_server pages > can only be order-0 right now, so higher order pages could be > skipped. [Zhang, Xiong Y] Thanks for your good propose, I will follow them and report them later. > > Jan
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index f37a1f2..6178046 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -546,6 +546,10 @@ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn) e.ipat = ipat; nt = p2m_recalc_type(e.recalc, e.sa_p2mt, p2m, gfn + i); + if ( e.sa_p2mt == p2m_ioreq_server && + p2m->ioreq.server == NULL ) + nt = p2m_ram_rw; + if ( nt != e.sa_p2mt ) { if ( e.sa_p2mt == p2m_ioreq_server ) diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index 5079b59..4de500c 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -445,6 +445,9 @@ static int do_recalc(struct p2m_domain *p2m, unsigned long gfn) p2m->domain->domain_id, gfn, level); ot = p2m_flags_to_type(l1e_get_flags(e)); nt = p2m_recalc_type_range(true, ot, p2m, gfn & mask, gfn | ~mask); + if ( ot == p2m_ioreq_server && p2m->ioreq.server == NULL ) + nt = p2m_ram_rw; + if ( nt != ot ) { unsigned long mfn = l1e_get_pfn(e); diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 7574a9b..dde516c 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -760,7 +760,7 @@ static inline p2m_type_t p2m_recalc_type_range(bool recalc, p2m_type_t t, if ( !recalc || !p2m_is_changeable(t) ) return t; - if ( t == p2m_ioreq_server && p2m->ioreq.server != NULL ) + if ( t == p2m_ioreq_server ) return t; return p2m_is_logdirty_range(p2m, gfn_start, gfn_end) ? p2m_ram_logdirty
'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, finally this results DomU reboot couldn't work. This patch will let get_entry(p2m_ioreq_server) return p2m_ioreq_server type instead of p2m_ram_rw type when the type of ioreq_server entries havn't been written. The actual type change happens in recalc funciton. 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> --- xen/arch/x86/mm/p2m-ept.c | 4 ++++ xen/arch/x86/mm/p2m-pt.c | 3 +++ xen/include/asm-x86/p2m.h | 2 +- 3 files changed, 8 insertions(+), 1 deletion(-)