From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 2/6] tools: arm: allocate superpages to guests. Date: Tue, 10 Jun 2014 16:16:50 +0100 Message-ID: <1402413410.7541.5.camel@kazak.uk.xensource.com> References: <1402394127.29980.52.camel@kazak.uk.xensource.com> <1402394278-9850-2-git-send-email-ian.campbell@citrix.com> <5396EACE.6070308@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5396EACE.6070308@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Tue, 2014-06-10 at 12:23 +0100, Julien Grall wrote: > 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). Yes, good idea. I'll add a precursor patch. > > > +/* >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? It was in the original as a backstop on allocsz. It would correspond to 4GB worth of 4K page I suppose > > + 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. userspace stack isn't all that precious but 8MB does seem a little excessive. Previously this was using the already allocated host p2m so it wasn't an issue, but that doesn't work for allocations of page >4K. The tradeoff is that smaller batches mean it will take longer. I don't think it would be unreasonable to reduce this to be e.g. 1GB worth of entries (256*1024) or 2MB of 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. I can't remember what I was doing here, but it is just debug so I'll nuke it. Ian.