From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: pvh dom0: memory leak from iomem map Date: Wed, 4 Jun 2014 16:32:16 -0700 Message-ID: <20140604163216.74f5ea15@mantra.us.oracle.com> References: <20140603182948.073180cf@mantra.us.oracle.com> <538EE80702000078000179F3@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WsKfT-0006pH-0K for xen-devel@lists.xenproject.org; Wed, 04 Jun 2014 23:32:27 +0000 In-Reply-To: <538EE80702000078000179F3@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Tim Deegan List-Id: xen-devel@lists.xenproject.org On Wed, 04 Jun 2014 08:33:59 +0100 "Jan Beulich" wrote: > >>> On 04.06.14 at 03:29, 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