Re: [PATCH 2/6] tools: arm: allocate superpages to guests.

[Ian Campbell <Ian.Campbell@citrix.com>]

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 <ian.campbell@citrix.com>
> > ---
> >  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.