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

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Wed, 18 Dec 2013 17:51:52 +0100
Tim Deegan <tim@xen.org> 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?

No, not quite, because I need to know which mfns are foreign and
do put_page on them. 

By changing things around a bit for change ept type, I came up with
following:

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 0ba2365..c996aac 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -838,6 +838,80 @@ void setup_ept_dump(void)
     register_keyhandler('D', &ept_p2m_table);
 }
 
+typedef int (ept_walk_entry_callback_f)(ept_entry_t *, unsigned long);
+
+static int ept_change_entry_type(ept_entry_t *entry, unsigned long data)
+{
+    p2m_type_t new = (p2m_type_t)data;
+
+    entry->sa_p2mt = new;
+    ept_p2m_type_to_flags(entry, new, entry->access);
+    return 1;
+}
+
+static int ept_put_foreign_mfn(ept_entry_t *entry, unsigned long data)
+{
+    put_page(mfn_to_page(entry->mfn));
+    return 0;
+}
+
+static void ept_walk_entry_callback_recurse(
+    mfn_t ept_page_mfn, int ept_page_level, p2m_type_t p2mt, 
+    ept_walk_entry_callback_f *fp, unsigned long data)
+{
+    ept_entry_t e, *epte = map_domain_page(mfn_x(ept_page_mfn));
+
+    for ( int i = 0; i < EPT_PAGETABLE_ENTRIES; i++ )
+    {
+        if ( !is_epte_valid(epte + i) )
+            continue;
+
+        if ( (ept_page_level > 0) && !is_epte_superpage(epte + i) )
+            ept_walk_entry_callback_recurse(_mfn(epte[i].mfn),
+                                            ept_page_level - 1, p2mt, fp, data);
+        else
+        {
+            e = atomic_read_ept_entry(&epte[i]);
+
+            if ( e.sa_p2mt == p2mt && fp(&e, data) )
+                    atomic_write_ept_entry(&epte[i], &e);
+        }
+    }
+
+    unmap_domain_page(epte);
+}
+
+static void ept_walk_entry_callback(struct p2m_domain *p2m, p2m_type_t p2mt,
+                                    ept_walk_entry_callback_f *fp, 
+                                    unsigned long data, int do_flush)
+{
+    struct ept_data *ept = &p2m->ept;
+
+    if ( ept_get_asr(ept) == 0 )
+        return;
+
+    ept_walk_entry_callback_recurse(_mfn(ept_get_asr(ept)), ept_get_wl(ept),
+                                    p2mt, fp, data);
+    if ( do_flush )
+        ept_sync_domain(p2m);
+}
+
+int ept_release_foreign_pages(struct p2m_domain *p2m)
+{
+    ept_walk_entry_callback(p2m, p2m_map_foreign, ept_put_foreign_mfn, 0, 0);
+    return 0;
+}
+
+int ept_change_entry_type_global_mine(struct p2m_domain *p2m,
+                                      p2m_type_t ot, p2m_type_t nt)
+{
+    BUG_ON(p2m_is_grant(ot) || p2m_is_grant(nt));
+    BUG_ON(ot != nt && (ot == p2m_mmio_direct || nt == p2m_mmio_direct));
+
+    ept_walk_entry_callback(p2m, ot, ept_change_entry_type, nt, 1);
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C


I can further make this pre-emptible by returning some "handle"
that the caller must stash somewhere, say, p2m_domain, so that at
ept level, synchronization doesn't need to be concerned about.

what do you think?

thanks
mukesh