From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2] xen/arm: Allow balooning working with 1:1 memory mapping Date: Mon, 16 Dec 2013 14:30:53 +0000 Message-ID: <52AF0E9D.5020405@linaro.org> References: <1386965881-12151-1-git-send-email-julien.grall@linaro.org> <52AED12C020000780010D832@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VsZCE-0003op-KD for xen-devel@lists.xenproject.org; Mon, 16 Dec 2013 14:30:58 +0000 Received: by mail-wi0-f177.google.com with SMTP id cc10so2177506wib.4 for ; Mon, 16 Dec 2013 06:30:57 -0800 (PST) In-Reply-To: <52AED12C020000780010D832@nat28.tlf.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: Keir Fraser , ian.campbell@citrix.com, patches@linaro.org, tim@xen.org, stefano.stabellini@citrix.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 12/16/2013 09:08 AM, Jan Beulich wrote: >>>> On 13.12.13 at 21:18, Julien Grall wrote: >> --- a/xen/common/memory.c >> +++ b/xen/common/memory.c >> @@ -90,7 +90,7 @@ static void increase_reservation(struct memop_args *a) >> >> static void populate_physmap(struct memop_args *a) >> { >> - struct page_info *page; >> + struct page_info *page = NULL; > > Why? Spurious line from a previous version. I will remove it. > >> @@ -122,7 +122,29 @@ static void populate_physmap(struct memop_args *a) >> } >> else >> { >> - page = alloc_domheap_pages(d, a->extent_order, a->memflags); >> + if ( d == dom0 && is_dom0_mapped_11() ) >> + { >> + mfn = gpfn; >> + if (!mfn_valid(mfn)) > > Coding style. > >> + { >> + gdprintk(XENLOG_INFO, "Invalid mfn 0x%"PRI_xen_pfn"\n", >> + mfn); >> + goto out; >> + } >> + >> + page = mfn_to_page(mfn); >> + if ( !get_page(page, d) ) >> + { >> + gdprintk(XENLOG_INFO, >> + "mfn 0x%"PRI_xen_pfn" doesn't belong to dom0\n", >> + mfn); >> + goto out; >> + } >> + put_page(page); >> + } > > I think this hack doesn't belong into a common file. Rather than > having the (anyway oddly named) is_dom0_mapped_11(), it > would seem more clean to have this implemented by an inline > function for ARM, returning the struct page_info * (and getting > #define-d to NULL for all other cases, perhaps not even in an > x86 header, but right in the source file). > > And even if we were to stay with the implementation here, for > being at least half way sane the checking macro should take a > domain argument rather than requiring the extra "d == dom0" > check up front. Plus the "11" there is bogus - I first read this as > "eleven" rather than "1:1", wondering what eleven here was > about. I'd suggest something like is_domain_direct_mapped() or > is_domain_identity_mapped(). > > Jan > -- Julien Grall