Re: pvh dom0: memory leak from iomem map

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

On Wed, 04 Jun 2014 08:33:59 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 04.06.14 at 03:29, <mukesh.rathor@oracle.com> wrote:
> > Hi Tim,
> > 
> > When building a dom0 pvh, we populate the p2m with 0..N pfns
> > upfront. Then in pvh_map_all_iomem, we walk the e820 and map all
> > iomem 1:1. As such any iomem range below N would cause those ram
> > frames to be silently dropped. 
> > 
> > Since the holes could be pretty big, I am concenred this could
> > result in significant loss of frames. 
> > 
> > In my very early patches I had:
> > 
> > set_typed_p2m_entry():
> > ...
> >     else if ( p2m_is_ram(ot) )
> >     {
> >          if ( is_pvh_domain(d) )                    <---
> >              free_domheap_page(mfn_to_page(omfn));  <---
> > 
> >          ASSERT(mfn_valid(omfn));
> >          set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
> > ..
> > 
> > I'd like you to reconsider it. Since there is a dislike using
> > is_pvh, I suppose one alternative could be, 'if ( gfn_p2mt ==
> > p2m_mmio_direct)'.
> > 
> > If you have any other suggestions, I'm open to them. LMK your
> > thoughts..
> 
> Isn't Roger's af06d66e ("x86: fix setup of PVH Dom0 memory map")
> already taking care of this?

Not quite. He is adding N pages from domheap (d->page_list) to the end
of memory map, where N is the number of pages freed during walking holes.
When walking holes, I call set_mmio_p2m_entry to do 1:1 mapping. In that
path I don't see the old ram page being put back to the domheap. 

I realized looking into free_domheap_page, it is not appropriate to call
it above. Instead, we just need to add page to the page_list. We can do that 
in set_typed_p2m_entry, or in pvh_map_all_iomem. But latter would result in 
extra get_entry call. Please lmk what you think.


BTW, looking at set_typed_p2m_entry just now I realized that it prematurely
updates M2P. If p2m_set_entry fails, IMO, we should leave it as is. IOW:

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 642ec28..bce904a 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -829,11 +829,6 @@ static int set_typed_p2m_entry(struct domain *d, unsigned l
         domain_crash(d);
         return -ENOENT;
     }
-    else if ( p2m_is_ram(ot) )
-    {
-        ASSERT(mfn_valid(omfn));
-        set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
-    }
 
     P2M_DEBUG("set %d %lx %lx\n", gfn_p2mt, gfn, mfn_x(mfn));
     rc = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, gfn_p2mt,
@@ -843,6 +838,11 @@ static int set_typed_p2m_entry(struct domain *d, unsigned l
         gdprintk(XENLOG_ERR,
                  "p2m_set_entry failed! mfn=%08lx rc:%d\n",
                  mfn_x(get_gfn_query_unlocked(p2m->domain, gfn, &ot)), rc);
+    else if ( p2m_is_ram(ot) )
+    {
+        ASSERT(mfn_valid(omfn));
+        set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
+    }
     return rc;
 }
 

If you agree, I can submit officially.

thanks,
Mukesh