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

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

[Mukesh Rathor]

When a controlling domain is destroyed, any p2m_is_foreign pages must
release the refcnt gotten when the page was added to the p2m.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/domain.c     |    3 +++
 xen/arch/x86/mm/p2m.c     |   30 ++++++++++++++++++++++++++++++
 xen/include/asm-x86/p2m.h |    4 +++-
 3 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index c0ac5d6..7f15cdd 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1872,6 +1872,9 @@ int domain_relinquish_resources(struct domain *d)
     case RELMEM_not_started:
         pci_release_devices(d);
 
+        if ( (ret = p2m_cleanup(d)) )
+            return ret;
+
         /* Tear down paging-assistance stuff. */
         paging_teardown(d);
 
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 441d151..a91deb2 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -499,6 +499,36 @@ void p2m_final_teardown(struct domain *d)
     p2m_teardown_hostp2m(d);
 }
 
+/* This function to do any cleanup while walking the entire p2m */
+int p2m_cleanup(struct domain *d)
+{
+    int rc = 0;
+    unsigned long gfn, count = 0;
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+    if ( !is_pvh_domain(d) )
+        return 0;
+
+    for ( gfn = p2m->next_foreign_gfn_to_check;
+          gfn <= p2m->max_mapped_pfn; gfn++, count++ )
+    {
+        p2m_type_t p2mt;
+        mfn_t mfn = get_gfn_query(d, gfn, &p2mt);
+
+        if ( unlikely(p2m_is_foreign(p2mt)) )
+            put_page(mfn_to_page(mfn));
+        put_gfn(d, gfn);
+        
+        /* Preempt every 10k pages, arbritary */
+        if ( count == 10000 && hypercall_preempt_check() )
+        {
+            p2m->next_foreign_gfn_to_check = gfn + 1;
+            rc = -EAGAIN;
+            break;
+        }
+    }
+    return rc;
+}
 
 static void
 p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 6371705..d32d103 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -254,9 +254,10 @@ struct p2m_domain {
     /* Highest guest frame that's ever been mapped in the p2m */
     unsigned long max_mapped_pfn;
 
-    /* When releasing shared gfn's in a preemptible manner, recall where
+    /* When releasing gfn's in a preemptible manner, recall where
      * to resume the search */
     unsigned long next_shared_gfn_to_relinquish;
+    unsigned long next_foreign_gfn_to_check;
 
     /* Populate-on-demand variables
      * All variables are protected with the pod lock. We cannot rely on
@@ -471,6 +472,7 @@ int p2m_alloc_table(struct p2m_domain *p2m);
 /* Return all the p2m resources to Xen. */
 void p2m_teardown(struct p2m_domain *p2m);
 void p2m_final_teardown(struct domain *d);
+int p2m_cleanup(struct domain *d);
 
 /* Add a page to a domain's p2m table */
 int guest_physmap_add_entry(struct domain *d, unsigned long gfn,
-- 
1.7.2.3

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

[Jan Beulich]

>>> On 17.12.13 at 02:47, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> When a controlling domain is destroyed, any p2m_is_foreign pages must
> release the refcnt gotten when the page was added to the p2m.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>

First of all, you leave open what the relationship of this is with
your 7-patch v7 series. Why is this not part of it? Obviously any
necessary cleanup would logically have to come no later than
where the first creation of pages of this type happens...

> +/* This function to do any cleanup while walking the entire p2m */
> +int p2m_cleanup(struct domain *d)
> +{
> +    int rc = 0;
> +    unsigned long gfn, count = 0;
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +
> +    if ( !is_pvh_domain(d) )
> +        return 0;
> +
> +    for ( gfn = p2m->next_foreign_gfn_to_check;
> +          gfn <= p2m->max_mapped_pfn; gfn++, count++ )
> +    {
> +        p2m_type_t p2mt;
> +        mfn_t mfn = get_gfn_query(d, gfn, &p2mt);
> +
> +        if ( unlikely(p2m_is_foreign(p2mt)) )
> +            put_page(mfn_to_page(mfn));
> +        put_gfn(d, gfn);
> +        
> +        /* Preempt every 10k pages, arbritary */
> +        if ( count == 10000 && hypercall_preempt_check() )
> +        {
> +            p2m->next_foreign_gfn_to_check = gfn + 1;
> +            rc = -EAGAIN;
> +            break;
> +        }

So it looks like you copied one of the two obvious bugs from
relinquish_shared_pages() _and_ deferred the preemption point
by quite a bit - 10,000 pages is quite a lot, the 512 used there
seems much more reasonable.

As to the copied bug: Should hypercall_preempt_check() return
false, you'd never again try to preempt.

Jan

> +    }
> +    return rc;
> +}

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

[Tim Deegan]

At 08:42 +0000 on 17 Dec (1387266152), Jan Beulich wrote:
> >>> On 17.12.13 at 02:47, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > When a controlling domain is destroyed, any p2m_is_foreign pages must
> > release the refcnt gotten when the page was added to the p2m.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> 
> First of all, you leave open what the relationship of this is with
> your 7-patch v7 series. Why is this not part of it? Obviously any
> necessary cleanup would logically have to come no later than
> where the first creation of pages of this type happens...
> 
> > +/* This function to do any cleanup while walking the entire p2m */
> > +int p2m_cleanup(struct domain *d)
> > +{
> > +    int rc = 0;
> > +    unsigned long gfn, count = 0;
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +
> > +    if ( !is_pvh_domain(d) )
> > +        return 0;
> > +
> > +    for ( gfn = p2m->next_foreign_gfn_to_check;
> > +          gfn <= p2m->max_mapped_pfn; gfn++, count++ )
> > +    {
> > +        p2m_type_t p2mt;
> > +        mfn_t mfn = get_gfn_query(d, gfn, &p2mt);
> > +
> > +        if ( unlikely(p2m_is_foreign(p2mt)) )
> > +            put_page(mfn_to_page(mfn));
> > +        put_gfn(d, gfn);
> > +        
> > +        /* Preempt every 10k pages, arbritary */
> > +        if ( count == 10000 && hypercall_preempt_check() )
> > +        {
> > +            p2m->next_foreign_gfn_to_check = gfn + 1;
> > +            rc = -EAGAIN;
> > +            break;
> > +        }
> 
> So it looks like you copied one of the two obvious bugs from
> relinquish_shared_pages() _and_ deferred the preemption point
> by quite a bit - 10,000 pages is quite a lot, the 512 used there
> seems much more reasonable.
> 
> As to the copied bug: Should hypercall_preempt_check() return
> false, you'd never again try to preempt.

Further, looping from 0 to max_mapped_pfn, even preemptibly, is not a good
way to do this: the guest can set its own max_mapped_pfn, and we don't
want Xen to spend its time counting to 2^63.

Further further, it occurs to me that the refcounting change might
interact badly with nested EPT, which creates and destroys p2m tables
quite regularly.

Tim.

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

[Mukesh Rathor]

On Tue, 17 Dec 2013 11:19:57 +0100
Tim Deegan <tim@xen.org> wrote:

> At 08:42 +0000 on 17 Dec (1387266152), Jan Beulich wrote:
> > >>> On 17.12.13 at 02:47, Mukesh Rathor <mukesh.rathor@oracle.com>
> > >>> wrote:
> > > When a controlling domain is destroyed, any p2m_is_foreign pages
> > > must release the refcnt gotten when the page was added to the p2m.
> > > 
> > > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > 
......
> > 
> > So it looks like you copied one of the two obvious bugs from
> > relinquish_shared_pages() _and_ deferred the preemption point
> > by quite a bit - 10,000 pages is quite a lot, the 512 used there
> > seems much more reasonable.
> > 
> > As to the copied bug: Should hypercall_preempt_check() return
> > false, you'd never again try to preempt.
> 
> Further, looping from 0 to max_mapped_pfn, even preemptibly, is not a
> good way to do this: the guest can set its own max_mapped_pfn, and we
> don't want Xen to spend its time counting to 2^63.

Ok, some p2m code is setting a bad precedent. 

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 :)...

Or better, if I add a count of foreign mappings and hang it off the 
p2m_domain, then this code would only execute in case of control
domain going away... it seems in that case walking the p2m would be
tolerable. That should be acceptable?

> Further further, it occurs to me that the refcounting change might
> interact badly with nested EPT, which creates and destroys p2m tables
> quite regularly.

nested ept is not supported on pvh.

thanks
mukesh

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

[Jan Beulich]

>>> On 18.12.13 at 03:44, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> An alternative might be to just create a link list then and walk it. In
> general, foreign mappings should be very small,

That's according to the usage you're aware of. I think Fujitsu's
tools to control BS2000 guests map huge parts of guest memory
(if not all of it).

> 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 :)...
> 
> Or better, if I add a count of foreign mappings and hang it off the 
> p2m_domain, then this code would only execute in case of control
> domain going away... it seems in that case walking the p2m would be
> tolerable. That should be acceptable?

That would at least be a first step (following the shared page
count maintained for a domain). I don't think this is going to be
good enough though considering disaggregation.

Jan

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

[Dietmar Hahn]

Am Mittwoch 18 Dezember 2013, 10:03:00 schrieb Jan Beulich:
> >>> On 18.12.13 at 03:44, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > An alternative might be to just create a link list then and walk it. In
> > general, foreign mappings should be very small,
> 
> That's according to the usage you're aware of. I think Fujitsu's
> tools to control BS2000 guests map huge parts of guest memory
> (if not all of it).

Yes, that's true, think  of dimensions in GB.

Dietmar.

> 
> > 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 :)...
> > 
> > Or better, if I add a count of foreign mappings and hang it off the 
> > p2m_domain, then this code would only execute in case of control
> > domain going away... it seems in that case walking the p2m would be
> > tolerable. That should be acceptable?
> 
> That would at least be a first step (following the shared page
> count maintained for a domain). I don't think this is going to be
> good enough though considering disaggregation.
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

-- 
Company details: http://ts.fujitsu.com/imprint.html

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

[Tim Deegan]

At 18:44 -0800 on 17 Dec (1387302252), Mukesh Rathor wrote:
> On Tue, 17 Dec 2013 11:19:57 +0100
> Tim Deegan <tim@xen.org> wrote:
> 
> > At 08:42 +0000 on 17 Dec (1387266152), Jan Beulich wrote:
> > > >>> On 17.12.13 at 02:47, Mukesh Rathor <mukesh.rathor@oracle.com>
> > > >>> wrote:
> > > > When a controlling domain is destroyed, any p2m_is_foreign pages
> > > > must release the refcnt gotten when the page was added to the p2m.
> > > > 
> > > > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > > 
> ......
> > > 
> > > So it looks like you copied one of the two obvious bugs from
> > > relinquish_shared_pages() _and_ deferred the preemption point
> > > by quite a bit - 10,000 pages is quite a lot, the 512 used there
> > > seems much more reasonable.
> > > 
> > > As to the copied bug: Should hypercall_preempt_check() return
> > > false, you'd never again try to preempt.
> > 
> > Further, looping from 0 to max_mapped_pfn, even preemptibly, is not a
> > good way to do this: the guest can set its own max_mapped_pfn, and we
> > don't want Xen to spend its time counting to 2^63.
> 
> Ok, some p2m code is setting a bad precedent. 

Yes, indeed. :(  As I pointed out before, and has been the subject of
several XSAs.

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

> Or better, if I add a count of foreign mappings and hang it off the 
> p2m_domain, then this code would only execute in case of control
> domain going away...

That's a good idea as an optimisation, but it doesn't prevent the bad
case: this teardown has to execute for any domain that has a p2m and
at least one foreign mapping.  Since that domain chooses the GFN of
the mapping, it can still cause the hypervisor to to a lot of work.

> > Further further, it occurs to me that the refcounting change might
> > interact badly with nested EPT, which creates and destroys p2m tables
> > quite regularly.
> 
> nested ept is not supported on pvh.

What, never?  Or as a 'fimxe' for later?  We should avoid painting
ourselves into any more corners here if we can.

Tim.

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

[Tim Deegan]

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.

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

[Mukesh Rathor]

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

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

[Tim Deegan]

At 18:01 -0800 on 18 Dec (1387386115), Mukesh Rathor wrote:
> 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. 

Oh right, but you need to do that anyway -- once you've got refcounts
held by ept entries, you need to do _all_ entry writes through the
function that preserves the refcount invariant.  So
p2m_change_entry_type_global() has to use that function too, in case
either of the types it's called with is p2m_foreign.  And once that's
fixed, a call to s/foreign/invalid/ DTRT.

Also, this:

> +static int ept_put_foreign_mfn(ept_entry_t *entry, unsigned long data)
> +{
> +    put_page(mfn_to_page(entry->mfn));
> +    return 0;
> +}

gives me the heebie-jeebies, because it explcitly _breaks_ the
invariant, leaving the foreign mapping in place without its refcount.

Tim.

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

[Mukesh Rathor]

On Thu, 19 Dec 2013 11:50:58 +0100
Tim Deegan <tim@xen.org> wrote:

> At 18:01 -0800 on 18 Dec (1387386115), Mukesh Rathor wrote:
> > 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. 
> 
> Oh right, but you need to do that anyway -- once you've got refcounts
> held by ept entries, you need to do _all_ entry writes through the
> function that preserves the refcount invariant.  So
> p2m_change_entry_type_global() has to use that function too, in case
> either of the types it's called with is p2m_foreign.  And once that's
> fixed, a call to s/foreign/invalid/ DTRT.

Right, right... i keep not realizing, it's there now and it's use
will scope beyond the current PVH usage. So rather than deferring to
when someone actually finds a use case for p2m_change_entry_type_global
using foreign, i should be handled now. 

I'll have next, un-rushed this time, patch prob first week of jan 
given the upcoming holidays.

> Also, this:
> 
> > +static int ept_put_foreign_mfn(ept_entry_t *entry, unsigned long
> > data) +{
> > +    put_page(mfn_to_page(entry->mfn));
> > +    return 0;
> > +}
> 
> gives me the heebie-jeebies, because it explcitly _breaks_ the
> invariant, leaving the foreign mapping in place without its refcount.

I was thinking the case of multiple mappings, but re-thinking about it,
multiple mappings should just not be allowed IMO, so as soon as
put_page is done, the entry be invalidated. I'll redo it.

thanks a lot,
mukesh

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

[Tim Deegan]

Hi,

At 18:00 -0800 on 19 Dec (1387472409), Mukesh Rathor wrote:
> On Thu, 19 Dec 2013 11:50:58 +0100 Tim Deegan <tim@xen.org> wrote:
> > At 18:01 -0800 on 18 Dec (1387386115), Mukesh Rathor wrote:
> > > No, not quite, because I need to know which mfns are foreign and
> > > do put_page on them. 
> > 
> > Oh right, but you need to do that anyway -- once you've got refcounts
> > held by ept entries, you need to do _all_ entry writes through the
> > function that preserves the refcount invariant.  So
> > p2m_change_entry_type_global() has to use that function too, in case
> > either of the types it's called with is p2m_foreign.  And once that's
> > fixed, a call to s/foreign/invalid/ DTRT.
> 
> Right, right... i keep not realizing, it's there now and it's use
> will scope beyond the current PVH usage. So rather than deferring to
> when someone actually finds a use case for p2m_change_entry_type_global
> using foreign, i should be handled now. 
> 
> I'll have next, un-rushed this time, patch prob first week of jan 
> given the upcoming holidays.

Great!  Looking forward to it.  Happy holidays in the meantime. :)

Tim.

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

[Mukesh Rathor]

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?
> 
> 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++ )


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);
}


If that sounds ok, I'm ready to submit dom0 PVH patches. Pl lmk.

thanks
Mukesh

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

[Tim Deegan]

At 18:38 -0800 on 31 Jan (1391189918), Mukesh Rathor wrote:
> 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?
> > 
> > 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.

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

[George Dunlap]

On Wed, Dec 18, 2013 at 10:09 AM, Tim Deegan <tim@xen.org> wrote:
> At 18:44 -0800 on 17 Dec (1387302252), Mukesh Rathor wrote:
>> On Tue, 17 Dec 2013 11:19:57 +0100
>> Tim Deegan <tim@xen.org> wrote:
>>
>> > At 08:42 +0000 on 17 Dec (1387266152), Jan Beulich wrote:
>> > > >>> On 17.12.13 at 02:47, Mukesh Rathor <mukesh.rathor@oracle.com>
>> > > >>> wrote:
>> > > > When a controlling domain is destroyed, any p2m_is_foreign pages
>> > > > must release the refcnt gotten when the page was added to the p2m.
>> > > >
>> > > > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
>> > >
>> ......
>> > >
>> > > So it looks like you copied one of the two obvious bugs from
>> > > relinquish_shared_pages() _and_ deferred the preemption point
>> > > by quite a bit - 10,000 pages is quite a lot, the 512 used there
>> > > seems much more reasonable.
>> > >
>> > > As to the copied bug: Should hypercall_preempt_check() return
>> > > false, you'd never again try to preempt.
>> >
>> > Further, looping from 0 to max_mapped_pfn, even preemptibly, is not a
>> > good way to do this: the guest can set its own max_mapped_pfn, and we
>> > don't want Xen to spend its time counting to 2^63.
>>
>> Ok, some p2m code is setting a bad precedent.
>
> Yes, indeed. :(  As I pointed out before, and has been the subject of
> several XSAs.
>
>> 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.
>
>> Or better, if I add a count of foreign mappings and hang it off the
>> p2m_domain, then this code would only execute in case of control
>> domain going away...
>
> That's a good idea as an optimisation, but it doesn't prevent the bad
> case: this teardown has to execute for any domain that has a p2m and
> at least one foreign mapping.  Since that domain chooses the GFN of
> the mapping, it can still cause the hypervisor to to a lot of work.
>
>> > Further further, it occurs to me that the refcounting change might
>> > interact badly with nested EPT, which creates and destroys p2m tables
>> > quite regularly.
>>
>> nested ept is not supported on pvh.
>
> What, never?  Or as a 'fimxe' for later?  We should avoid painting
> ourselves into any more corners here if we can.

I think most uses of nested virt would want a fully virtualized
environment to run on, wouldn't they?

 -George

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

[Tim Deegan]

At 13:58 +0000 on 20 Dec (1387544306), George Dunlap wrote:
> On Wed, Dec 18, 2013 at 10:09 AM, Tim Deegan <tim@xen.org> wrote:
> > At 18:44 -0800 on 17 Dec (1387302252), Mukesh Rathor wrote:
> >> nested ept is not supported on pvh.
> >
> > What, never?  Or as a 'fimxe' for later?  We should avoid painting
> > ourselves into any more corners here if we can.
> 
> I think most uses of nested virt would want a fully virtualized
> environment to run on, wouldn't they?

If, as I hope, we eventually make PVH a special case of HVM with fewer
emulated devices, that line will be blurred.  And maybe a PVH guest
would want to run KVM or VirtualBox?

Tim.

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

[Mukesh Rathor]

On Tue, 17 Dec 2013 08:42:32 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 17.12.13 at 02:47, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > When a controlling domain is destroyed, any p2m_is_foreign pages
> > must release the refcnt gotten when the page was added to the p2m.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> 
> First of all, you leave open what the relationship of this is with
> your 7-patch v7 series. Why is this not part of it? Obviously any
> necessary cleanup would logically have to come no later than
> where the first creation of pages of this type happens...

I was intending this for 4.5, and the series for 4.4, but now i can
combine it since the series is out of 4.4.


> > +/* This function to do any cleanup while walking the entire p2m */
> > +int p2m_cleanup(struct domain *d)
> > +{
> > +    int rc = 0;
> > +    unsigned long gfn, count = 0;
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +
> > +    if ( !is_pvh_domain(d) )
> > +        return 0;
> > +
> > +    for ( gfn = p2m->next_foreign_gfn_to_check;
> > +          gfn <= p2m->max_mapped_pfn; gfn++, count++ )
> > +    {
> > +        p2m_type_t p2mt;
> > +        mfn_t mfn = get_gfn_query(d, gfn, &p2mt);
> > +
> > +        if ( unlikely(p2m_is_foreign(p2mt)) )
> > +            put_page(mfn_to_page(mfn));
> > +        put_gfn(d, gfn);
> > +        
> > +        /* Preempt every 10k pages, arbritary */
> > +        if ( count == 10000 && hypercall_preempt_check() )
> > +        {
> > +            p2m->next_foreign_gfn_to_check = gfn + 1;
> > +            rc = -EAGAIN;
> > +            break;
> > +        }
> 
> So it looks like you copied one of the two obvious bugs from
> relinquish_shared_pages() _and_ deferred the preemption point

LOL...

> by quite a bit - 10,000 pages is quite a lot, the 512 used there
> seems much more reasonable.

Because the foreign pages are very small amount, so the loop is
mostly not doing anything. ok, will reduce it to 1k.

thanks
Mukesh

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

[Jan Beulich]

>>> On 18.12.13 at 02:01, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> On Tue, 17 Dec 2013 08:42:32 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:
>> > +/* This function to do any cleanup while walking the entire p2m */
>> > +int p2m_cleanup(struct domain *d)
>> > +{
>> > +    int rc = 0;
>> > +    unsigned long gfn, count = 0;
>> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> > +
>> > +    if ( !is_pvh_domain(d) )
>> > +        return 0;
>> > +
>> > +    for ( gfn = p2m->next_foreign_gfn_to_check;
>> > +          gfn <= p2m->max_mapped_pfn; gfn++, count++ )
>> > +    {
>> > +        p2m_type_t p2mt;
>> > +        mfn_t mfn = get_gfn_query(d, gfn, &p2mt);
>> > +
>> > +        if ( unlikely(p2m_is_foreign(p2mt)) )
>> > +            put_page(mfn_to_page(mfn));
>> > +        put_gfn(d, gfn);
>> > +        
>> > +        /* Preempt every 10k pages, arbritary */
>> > +        if ( count == 10000 && hypercall_preempt_check() )
>> > +        {
>> > +            p2m->next_foreign_gfn_to_check = gfn + 1;
>> > +            rc = -EAGAIN;
>> > +            break;
>> > +        }
>> 
>> So it looks like you copied one of the two obvious bugs from
>> relinquish_shared_pages() _and_ deferred the preemption point
> 
> LOL...

If that makes you laugh, I would imply you should have seen the
(as said quite obvious) bugs and not only hyve avoided copying
one of them, but also submit a fix to the original code.

You did neither, and hence I assume you didn't pay too much
attention to how what you copy actually works...

> > by quite a bit - 10,000 pages is quite a lot, the 512 used there
> > seems much more reasonable.
> 
> Because the foreign pages are very small amount, so the loop is
> mostly not doing anything. ok, will reduce it to 1k.

Right - in which case biasing just like done in the fix to the original
code is probably the right approach here too.

Jan