pvh dom0: memory leak from iomem map

pvh dom0: memory leak from iomem map

[Mukesh Rathor]

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

Thanks,
Mukesh

Re: pvh dom0: memory leak from iomem map

[Jan Beulich]

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

Jan

Re: pvh dom0: memory leak from iomem map

[Mukesh Rathor]

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

Re: pvh dom0: memory leak from iomem map

[Jan Beulich]

>>> On 05.06.14 at 01:32, <mukesh.rathor@oracle.com> wrote:
> 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. 

That would be bad - I understood (or maybe it was just "hoped")
that holes wouldn't get populated anymore in the first place. Roger?

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

Adding it to page_list would result in the page getting freed when
the domain dies, but not earlier (unless PGC_allocated wouldn't
get cleared explicitly somewhere along with doing the respective
put_page()).

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

Yes, I agree with this part.

Jan

Re: pvh dom0: memory leak from iomem map

[Tim Deegan]

At 18:29 -0700 on 03 Jun (1401816588), Mukesh Rathor 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. 

Right.  So, er, don't do that then? :)  You have all the information
you need available at the time that you build dom0's p2m, so why not
just do it right the first time instead of fixing up afterwards?  It's
not like the pvh dom0 p2m building code is shared with anything else.

The change you suggest doesn't really DTRT anyway: it just drops dom0
memory on the floor.  A sufficiently inconvenient host memory map
would lead to freeing most of dom0's memory and failing the boot.

Tim.

> 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..
> 
> Thanks,
> Mukesh
> 

Re: pvh dom0: memory leak from iomem map

[Roger Pau Monné]

On 05/06/14 01:32, Mukesh Rathor wrote:
> 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'm quite sure I'm missing something here, but I don't see were those 
pages are removed from the domheap page list (d->page_list). In fact 
I've created a small debug patch to show that the pages removed by the 
MMIO holes are still in the domheap list:

---
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index ba42fc9..d54929c 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -312,13 +312,29 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
                                        unsigned long mfn, unsigned long nr_mfns)
 {
     unsigned long i;
+    unsigned long omfn;
+    struct page_info *page, *pg;
+    p2m_type_t t;
     int rc;
 
     for ( i = 0; i < nr_mfns; i++ )
     {
+        page = NULL;
+        omfn = mfn_x(get_gfn_query_unlocked(d, gfn + i, &t));
+        if ( mfn_valid(omfn) )
+            page = mfn_to_page(omfn);
         if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i))) )
             panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
                   gfn, mfn, i, rc);
+        if ( !page )
+            goto done;
+        page_list_for_each( pg, &d->page_list )
+        {
+            if ( pg == page )
+                goto done;
+        }
+        panic("Unable to find page: %p in d->page_list\n", page);
+ done:
         if ( !(i & 0xfffff) )
                 process_pending_softirqs();
     }
---

What I tried to do in af06d66e is to reuse the pages 
previously removed from the MMIO holes to populate the end of the 
memory map.

Roger.

Re: pvh dom0: memory leak from iomem map

[Jan Beulich]

>>> On 05.06.14 at 12:17, <roger.pau@citrix.com> wrote:
> What I tried to do in af06d66e is to reuse the pages 
> previously removed from the MMIO holes to populate the end of the 
> memory map.

Which would be equivalent to not populating the holes, and
allocating pages instead. Indeed there's no alloc_domheap_pages()
in that change, so unless some of the pages don't get taken care
of I don't see where a leak would be. Mukesh?

Jan

Re: pvh dom0: memory leak from iomem map

[Mukesh Rathor]

On Thu, 5 Jun 2014 12:17:54 +0200
Roger Pau Monné <roger.pau@citrix.com> wrote:

> On 05/06/14 01:32, Mukesh Rathor wrote:
> > 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():
> >>> ...
.....
> 
> I'm quite sure I'm missing something here, but I don't see were those 
> pages are removed from the domheap page list (d->page_list). In fact 
> I've created a small debug patch to show that the pages removed by
> the MMIO holes are still in the domheap list:

Ah, I see, you are reusing the pages by snooping into the M2P... 

            if ( get_gpfn_from_mfn(mfn) != INVALID_M2P_ENTRY )
                continue;

I guess that works since the M2P gets invalidated in set_mmio_p2m_entry,
and I think it's guaranteed that INVALID_M2P_ENTRY frame is always free
to be used. 

Ok, we are fine then.

thanks Roger,
Mukesh


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

Re: pvh dom0: memory leak from iomem map

[Mukesh Rathor]

On Thu, 5 Jun 2014 11:20:32 +0200
Tim Deegan <tim@xen.org> wrote:

> At 18:29 -0700 on 03 Jun (1401816588), Mukesh Rathor 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. 
> 
> Right.  So, er, don't do that then? :)  You have all the information
> you need available at the time that you build dom0's p2m, so why not


I thought about that, and wasn't sure how easy it would be to change
construct_dom0 for that purpose. It's common for both PV and PVH, and is pretty
messy as is. Anyways, Roger has already done the work of reusing the frames 
via snooping into M2P, and repopulating those frames that got invalidated
when mapping iomem 1:1. 

We don't have any case where a frame is used for some special purpose and 
does not have a mapping in the M2P, right? Otherwise, his code would be 
broken....

thanks,
Mukesh

Re: pvh dom0: memory leak from iomem map

[Tim Deegan]

At 19:12 -0700 on 05 Jun (1401991969), Mukesh Rathor wrote:
> On Thu, 5 Jun 2014 11:20:32 +0200
> Tim Deegan <tim@xen.org> wrote:
> 
> > At 18:29 -0700 on 03 Jun (1401816588), Mukesh Rathor 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. 
> > 
> > Right.  So, er, don't do that then? :)  You have all the information
> > you need available at the time that you build dom0's p2m, so why not
> 
> 
> I thought about that, and wasn't sure how easy it would be to change
> construct_dom0 for that purpose. It's common for both PV and PVH, and is pretty
> messy as is. Anyways, Roger has already done the work of reusing the frames 
> via snooping into M2P, and repopulating those frames that got invalidated
> when mapping iomem 1:1. 
> 
> We don't have any case where a frame is used for some special purpose and 
> does not have a mapping in the M2P, right? Otherwise, his code would be 
> broken....

Not that I know of.  But I can't believe that any path that builds a
p2m, punches holes in it, and then uses the _m2p_ to figure out where
the memory went could be the easy option. 

If doing it right the first time is too fiddly, we do have code that
moves a frame from one gfn to another (for the xatp call) -- perhaps
you could do that when you want to make a hole for mmio?

Tim.

Re: pvh dom0: memory leak from iomem map

[Mukesh Rathor]

On Fri, 6 Jun 2014 11:53:34 +0200
Tim Deegan <tim@xen.org> wrote:

> At 19:12 -0700 on 05 Jun (1401991969), Mukesh Rathor wrote:
> > On Thu, 5 Jun 2014 11:20:32 +0200
> > Tim Deegan <tim@xen.org> wrote:
> > 
> > > At 18:29 -0700 on 03 Jun (1401816588), Mukesh Rathor 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. 
> > > 
> > > Right.  So, er, don't do that then? :)  You have all the
> > > information you need available at the time that you build dom0's
> > > p2m, so why not
> > 
> > 
> > I thought about that, and wasn't sure how easy it would be to change
> > construct_dom0 for that purpose. It's common for both PV and PVH,
> > and is pretty messy as is. Anyways, Roger has already done the work
> > of reusing the frames via snooping into M2P, and repopulating those
> > frames that got invalidated when mapping iomem 1:1. 
> > 
> > We don't have any case where a frame is used for some special
> > purpose and does not have a mapping in the M2P, right? Otherwise,
> > his code would be broken....
> 
> Not that I know of.  But I can't believe that any path that builds a
> p2m, punches holes in it, and then uses the _m2p_ to figure out where
> the memory went could be the easy option. 
> 
> If doing it right the first time is too fiddly, we do have code that
> moves a frame from one gfn to another (for the xatp call) -- perhaps
> you could do that when you want to make a hole for mmio?

Well, doing it right the first time, meaning in construct_dom0 when we
populate the p2m would be the cleanest IMO. JFYI.. the reason it didn't
happen that way is because initially it didn't need to happen, because
linux guest did the repopulating. But then Roger noticed it could 
be done in xen itself, and the patch got done quickly for pvh, linux
still has the old code for pv fwiw.

Anyways, I'll add to my list.

thanks
Mukesh