Re: [PATCH 4/4] xen: arm: support up to (almost) 1TB of guest RAM

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 04/08/2014 03:19 PM, Ian Campbell wrote:
> This creates a second bank of RAM starting at 8GB and potentially extending to
> the 1TB boundary, which is the limit imposed by our current use of a 3 level
> p2m with 2 pages at level 0 (2^40 bits).
> 
> I've deliberately left a gap between the two banks just to exercise those code paths.
> 
> The second bank is 1016GB in size which plus the 3GB below 4GB is 1019GB
> maximum guest RAM. At the point where the fact that this is slightly less than
> a full TB starts to become an issue for people then we can switch to a 4 level
> p2m, which would be needed to support guests larger than 1TB anyhow.
> 
> Tested on 32-bit with 1, 4 and 6GB guests. Anything more than ~3GB requires an
> LPAE enabled kernel, or a 64-bit guest.

What happen if you try to boot a non-LPAE guest with more than ~3GB?
Does it boot or crash?

> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxc/xc_dom_arm.c      |   95 +++++++++++++++++++++++++++++------------
>  tools/libxl/libxl_arm.c       |   27 +++++++++---
>  xen/include/public/arch-arm.h |   13 +++++-
>  3 files changed, 100 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index 36b1487..65464c7 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -18,6 +18,7 @@
>   * Copyright (c) 2011, Citrix Systems
>   */
>  #include <inttypes.h>
> +#include <assert.h>
>  
>  #include <xen/xen.h>
>  #include <xen/io/protocols.h>
> @@ -245,28 +246,73 @@ static int set_mode(xc_interface *xch, domid_t domid, char *guest_type)
>      return rc;
>  }
>  
> +static int populate_guest_memory(struct xc_dom_image *dom,
> +                                 xen_pfn_t base_pfn, xen_pfn_t nr_pfns)
> +{
> +    int rc;
> +    xen_pfn_t allocsz, pfn;
> +
> +    if (!nr_pfns)

if ( !nr_pfns )

> +        return 0;
> +
> +    DOMPRINTF("%s: populating RAM @ %016"PRIx64"-%016"PRIx64" (%"PRId64"MB)",
> +              __FUNCTION__,
> +              (uint64_t)base_pfn << XC_PAGE_SHIFT,
> +              (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 )

It's very confusing to see pfn = rc = allocsz = 0 and each are not
related. Any reason to not initialized rc/allocsz earlier?

> +    {
> +        allocsz = nr_pfns - pfn;

In fact you are using allocsz here... so you don't need to initialize it.

> +        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]);
> +    }
> +
> +    return rc;
> +}
> +
>  int arch_setup_meminit(struct xc_dom_image *dom)
>  {
>      int rc;
> -    xen_pfn_t pfn, allocsz, i;
> +    xen_pfn_t pfn;
>      uint64_t modbase;
>  
>      /* Convenient */
> -    const uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
> -    const uint64_t ramend = rambase + ( dom->total_pages << XC_PAGE_SHIFT );
> +    const uint64_t ramsize = (uint64_t)dom->total_pages << XC_PAGE_SHIFT;
> +
> +    const uint64_t ram0size =
> +        ramsize > GUEST_RAM0_SIZE ? GUEST_RAM0_SIZE : ramsize;
> +    const uint64_t ram0end = GUEST_RAM0_BASE + ram0size;
> +    const uint64_t ram1size =
> +        ramsize > ram0size ? ramsize - ram0size : 0;
> +    const uint64_t ram1end = GUEST_RAM1_BASE + ram1size;
> +
> +    const xen_pfn_t p2m_size = ram1size ?
> +        (ram1end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT :
> +        (ram0end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT;
> +
>      const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, 21/*2MB*/);
>      const uint64_t dtb_size = dom->devicetree_blob ?
>          ROUNDUP(dom->devicetree_size, XC_PAGE_SHIFT) : 0;
>      const uint64_t ramdisk_size = dom->ramdisk_blob ?
>          ROUNDUP(dom->ramdisk_size, XC_PAGE_SHIFT) : 0;
>      const uint64_t modsize = dtb_size + ramdisk_size;
> -    const uint64_t ram128mb = rambase + (128<<20);
> +    const uint64_t ram128mb = GUEST_RAM0_BASE + (128<<20);
>  
> -    if ( ramend - 1 > GUEST_RAM_END )
> +    assert(dom->rambase_pfn << XC_PAGE_SHIFT == GUEST_RAM0_BASE);

Can you add parenthesis here for readability?

[..]

> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 4f0f0e2..6170454 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -255,9 +255,9 @@ static int make_psci_node(libxl__gc *gc, void *fdt)
>      return 0;
>  }
>  
> -static int make_memory_node(libxl__gc *gc, void *fdt,
> -                            unsigned long long base,
> -                            unsigned long long size)> 
> +static int make_one_memory_node(libxl__gc *gc, void *fdt,
> +                                unsigned long long base,
> +                                unsigned long long size)

I would use uint64_t rather unsigned long long. It's more clear the
base/size are 64 bits.

>  {
>      int res;
>      const char *name = GCSPRINTF("memory@%08llx", base);
> @@ -269,7 +269,7 @@ static int make_memory_node(libxl__gc *gc, void *fdt,
>      if (res) return res;
>  
>      res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
> -                            1, (uint64_t)base, (uint64_t)size);
> +                            1, base, size);
>      if (res) return res;
>  
>      res = fdt_end_node(fdt);
> @@ -278,6 +278,24 @@ static int make_memory_node(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>  
> +static int make_memory_node(libxl__gc *gc, void *fdt,
> +                            unsigned long long size)

Same here.

> +{
> +    int res;
> +    /* This had better match libxc's arch_setup_meminit... */
> +    const uint64_t size0 = size > GUEST_RAM0_SIZE ? GUEST_RAM0_SIZE : size;
> +    const uint64_t size1 = size > GUEST_RAM0_SIZE ? size - size0 : 0;
> +
> +    res = make_one_memory_node(gc, fdt, GUEST_RAM0_BASE, size0);
> +    if (res) return res;
> +    if (size1) {
> +        res = make_one_memory_node(gc, fdt, GUEST_RAM1_BASE, size1);
> +        if (res) return res;
> +    }

Any reason to not gather the both bank in one memory node?

Regards,

-- 
Julien Grall