Re: [Xen-devel] [PATCH] x86/ioreq server: Fix DomU reboot couldn't work when using p2m_ioreq_server p2m_type

[Zhang, Xiong Y]

> >>> On 04.05.17 at 00:15,  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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/ioreq server: Fix DomU reboot couldn't work when using p2m_ioreq_server p2m_type

[Jan Beulich]

>>> On 04.05.17 at 00:15,  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


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH] x86/ioreq server: Fix DomU reboot couldn't work when using p2m_ioreq_server p2m_type

[Xiong Zhang]

'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 
Signed-off-by: Yu Zhang 
---
 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(-)

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
-- 
2.7.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel