Re: [Xen-devel] [RFC PATCH] PVH: cleanup of p2m upon p2m destroy

[Tim Deegan <tim@xxxxxxx>]

At 18:38 -0800 on 31 Jan (1391189918), Mukesh Rathor wrote:
> On Wed, 18 Dec 2013 17:51:52 +0100
> Tim Deegan <tim@xxxxxxx> wrote:
> 
> > At 11:09 +0100 on 18 Dec (1387361398), Tim Deegan wrote:
> > > > An alternative might be to just create a link list then and walk
> > > > it. In general, foreign mappings should be very small, so the
> > > > overhead of 16 bytes per page for the link list might not be too
> > > > bad. I will code it if there is no disagreement from any
> > > > maintainer... everyone has different ideas :)...
> > > 
> > > I think it would be best to walk the p2m trie (i.e. bounded by
> > > amount of RAM, rather than max GFN) and do it preemptably.  I'll
> > > look into something like that for the mem_sharing loop today, and
> > > foreign mapping code can reuse it.
> > 
> > What I've ended up with is making p2m_change_entry_type_global()
> > preemptible (which is a bigger task but will be needed as domains get
> > bigger).  Do you think that using that function to switch all mappings
> > from p2m_foreign to p2m_invalid, appropriately late in the teardown,
> > will be good enough for what you need?
> > 
> > Cheers,
> > 
> > Tim.
> 
> Finally, coming back to this, the answer is yes. Looks like all I need
> to do is:
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 9faa663..268a8a2 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -470,6 +470,10 @@ void p2m_teardown(struct p2m_domain *p2m)
>  
>      p2m_lock(p2m);
>  
> +    /* pvh: we must release refcnt on all foreign pages */
> +    if ( is_pvh_domain(d) )
> +        p2m_change_entry_type_global(d, p2m_map_foreign, p2m_invalid);
> +
>      /* Try to unshare any remaining shared p2m entries. Safeguard
>       * Since relinquish_shared_pages should have done the work. */ 
>      for ( gfn=0; gfn < p2m->max_mapped_pfn; gfn++ )

That looks right.  Sorting out how to make it restartable is on my
TODO list, along with other similar code.

> In this call, the new atomic_write_ept_entry() will DTRT:
> 
> static inline void atomic_write_ept_entry(ept_entry_t *entryptr,
>                                           const ept_entry_t *new)
> {
>     if ( p2m_is_foreign(new->sa_p2mt) )
>     {
>         struct page_info *page;
>         struct domain *fdom;
> 
>         ASSERT(mfn_valid(new->mfn));
>         page = mfn_to_page(new->mfn);
>         fdom = page_get_owner(page);
>         get_page(page, fdom);
>     }
>     if ( p2m_is_foreign(entryptr->sa_p2mt) )
>         put_page(mfn_to_page(entryptr->mfn));
> 
>     write_atomic(&entryptr->epte, new->epte);
> }

Yep.  The write_atomic() should happen before the put_page(), so we
don't need to think about race conditions (see, e.g. shadow_set_l1e()
for the idiom), but otherwise that looks fine. 

Cheers,

Tim.

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