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

[Julien Grall <julien.grall@linaro.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 <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).

> +/*  >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