Re: [Xen-devel] pvh dom0: memory leak from iomem map

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.06.14 at 01:32, <mukesh.rathor@xxxxxxxxxx> wrote:
> On Wed, 04 Jun 2014 08:33:59 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 04.06.14 at 03:29, <mukesh.rathor@xxxxxxxxxx> 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


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