Re: [Xen-devel] [PATCH V2] x86/ioreq server: Fix XenGT couldn't reboot when XenGT use p2m_ioreq_server p2m_type

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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@xxxxxxxxx>
> Signed-off-by: Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>
> ---
>  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


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