From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 2/6] tools: arm: allocate superpages to guests. Date: Tue, 10 Jun 2014 12:23:58 +0100 Message-ID: <5396EACE.6070308@linaro.org> References: <1402394127.29980.52.camel@kazak.uk.xensource.com> <1402394278-9850-2-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1402394278-9850-2-git-send-email-ian.campbell@citrix.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: Ian Campbell , xen-devel@lists.xen.org Cc: tim@xen.org, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org Hi Ian, On 06/10/2014 10:57 AM, Ian Campbell wrote: > Tries to allocate as many large (1G or 2M) pages as it can to the guest and tries > to align to the next larger size on each attempt so that we can attempt to > allocate even larger pages on the next iteration (this is currently only > exercised via a compile time debug option, which is left in place under a #if > 0). > > Since ARM page tables are consistent at each level there is a common helper > function which tries to allocate a levels worth of pages. The exception to this > consistency is level 0 which does not support table mappings (0.5TB > superpages!). > > Signed-off-by: Ian Campbell > --- > tools/libxc/xc_dom_arm.c | 103 +++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 92 insertions(+), 11 deletions(-) > > diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c > index 75f8363..da68ec3 100644 > --- a/tools/libxc/xc_dom_arm.c > +++ b/tools/libxc/xc_dom_arm.c > @@ -30,6 +30,13 @@ > #define CONSOLE_PFN_OFFSET 0 > #define XENSTORE_PFN_OFFSET 1 > > +#define LPAE_SHIFT 9 > + > +#define PFN_4K_SHIFT (0) > +#define PFN_2M_SHIFT (PFN_4K_SHIFT+LPAE_SHIFT) > +#define PFN_1G_SHIFT (PFN_2M_SHIFT+LPAE_SHIFT) > +#define PFN_512G_SHIFT (PFN_1G_SHIFT+LPAE_SHIFT) > + > /* get guest IO ABI protocol */ > const char *xc_domain_get_native_protocol(xc_interface *xch, > uint32_t domid) > @@ -249,11 +256,57 @@ static int set_mode(xc_interface *xch, domid_t domid, char *guest_type) > return rc; > } > > +#define min_t(type,x,y) \ > + ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; }) min_t is also defined in xc_dom_decompress_unsafe_xz.c. It might be interesting to define it in a common header (such as xc_private.h). > +/* >0: success, *nr_pfns set to number actually populated > + * 0: didn't try with this pfn shift (e.g. misaligned base etc) > + * <0: ERROR > + */ > +static int populate_one_size(struct xc_dom_image *dom, int pfn_shift, > + xen_pfn_t base_pfn, xen_pfn_t *nr_pfns) > +{ > + uint64_t mask = ((uint64_t)1<<(pfn_shift))-1; > + uint64_t next_mask = ((uint64_t)1<<(LPAE_SHIFT))-1; > + int nr, i, count = min_t(int, 1024*1024, *nr_pfns >> pfn_shift); Stupid question, where does come from the 1024*1024? > + xen_pfn_t extents[count]; If I follow the count definition, it's possible to allocate 1024*1024 xen_pfn_t (about 8MB) on the stack. [..] > static int populate_guest_memory(struct xc_dom_image *dom, > xen_pfn_t base_pfn, xen_pfn_t nr_pfns) > { > - int rc; > + int rc = 0; > xen_pfn_t allocsz, pfn; > + int debug_iters = 0; > > DOMPRINTF("%s: populating RAM @ %016"PRIx64"-%016"PRIx64" (%"PRId64"MB)", > __FUNCTION__, > @@ -261,21 +314,49 @@ static int populate_guest_memory(struct xc_dom_image *dom, > (uint64_t)(base_pfn + nr_pfns) << XC_PAGE_SHIFT, > (uint64_t)nr_pfns >> (20-XC_PAGE_SHIFT)); > > - for ( pfn = 0; pfn < nr_pfns; pfn++ ) > - dom->p2m_host[pfn] = base_pfn + pfn; > - > - for ( pfn = rc = allocsz = 0; (pfn < nr_pfns) && !rc; pfn += allocsz ) > + for ( pfn = 0; pfn < nr_pfns; pfn += allocsz ) > { > allocsz = nr_pfns - pfn; > - if ( allocsz > 1024*1024 ) > - allocsz = 1024*1024; > > - rc = xc_domain_populate_physmap_exact( > - dom->xch, dom->guest_domid, allocsz, > - 0, 0, &dom->p2m_host[pfn]); > + if ( debug_iters++ > 4 ) > + abort(); IIRC, abort doesn't give any stack trace. I'm not sure if you intended to keep it in your patch. If so, I would add an error message. Regards, -- Julien Grall