|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/4] xen: arm: support up to (almost) 1TB of guest RAM
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@xxxxxxxxxx>
> ---
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |