Re: [PATCH 3/5] xen: allow balloon driver to use more than one memory region

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Aug 19, 2011 at 03:57:18PM +0100, David Vrabel wrote:
> Allow the xen balloon driver to populate its list of extra pages from
> more than one region of memory.  This will allow platforms to provide
> (for example) a region of low memory and a region of high memory.
> 
> The maximum possible number of extra regions is 128 (== E820MAX) which
> is quite large so xen_extra_mem is placed in __initdata.  This is safe
> as both xen_memeory_setup() and balloon_init() are in __init.
              ^^^^^^ memory.

> 
> The balloon regions themselves are not altered (i.e., there is still
> only the one region).
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  arch/x86/xen/setup.c  |   20 +++++++++---------
>  drivers/xen/balloon.c |   51 ++++++++++++++++++++++++++++--------------------
>  include/xen/page.h    |    9 +++++++-
>  3 files changed, 48 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index c3b8d44..4720c2d 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -37,7 +37,7 @@ extern void xen_syscall_target(void);
>  extern void xen_syscall32_target(void);
>  
>  /* Amount of extra memory space we add to the e820 ranges */
> -phys_addr_t xen_extra_mem_start, xen_extra_mem_size;
> +struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
>  
>  /* 
>   * The maximum amount of extra memory compared to the base size.  The
> @@ -56,7 +56,7 @@ static void __init xen_add_extra_mem(unsigned long pages)
>  	unsigned long pfn;
>  
>  	u64 size = (u64)pages * PAGE_SIZE;
> -	u64 extra_start = xen_extra_mem_start + xen_extra_mem_size;
> +	u64 extra_start = xen_extra_mem[0].start + xen_extra_mem[0].size;
>  
>  	if (!pages)
>  		return;
> @@ -66,7 +66,7 @@ static void __init xen_add_extra_mem(unsigned long pages)
>  
>  	memblock_x86_reserve_range(extra_start, extra_start + size, "XEN EXTRA");
>  
> -	xen_extra_mem_size += size;
> +	xen_extra_mem[0].size += size;
>  
>  	xen_max_p2m_pfn = PFN_DOWN(extra_start + size);
>  
> @@ -239,7 +239,7 @@ char * __init xen_memory_setup(void)
>  
>  	memcpy(map_raw, map, sizeof(map));
>  	e820.nr_map = 0;
> -	xen_extra_mem_start = mem_end;
> +	xen_extra_mem[0].start = mem_end;
>  	for (i = 0; i < memmap.nr_entries; i++) {
>  		unsigned long long end;
>  
> @@ -267,8 +267,8 @@ char * __init xen_memory_setup(void)
>  				e820_add_region(end, delta, E820_UNUSABLE);
>  		}
>  
> -		if (map[i].size > 0 && end > xen_extra_mem_start)
> -			xen_extra_mem_start = end;
> +		if (map[i].size > 0 && end > xen_extra_mem[0].start)
> +			xen_extra_mem[0].start = end;
>  
>  		/* Add region if any remains */
>  		if (map[i].size > 0)
> @@ -276,10 +276,10 @@ char * __init xen_memory_setup(void)
>  	}
>  	/* Align the balloon area so that max_low_pfn does not get set
>  	 * to be at the _end_ of the PCI gap at the far end (fee01000).
> -	 * Note that xen_extra_mem_start gets set in the loop above to be
> -	 * past the last E820 region. */
> -	if (xen_initial_domain() && (xen_extra_mem_start < (1ULL<<32)))
> -		xen_extra_mem_start = (1ULL<<32);
> +	 * Note that the start of balloon area gets set in the loop above
> +	 * to be past the last E820 region. */
> +	if (xen_initial_domain() && (xen_extra_mem[0].start < (1ULL<<32)))
> +		xen_extra_mem[0].start = (1ULL<<32);
>  
>  	/*
>  	 * In domU, the ISA region is normal, usable memory, but we
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 5814022..996cf65 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -555,11 +555,35 @@ void free_xenballooned_pages(int nr_pages, struct page** pages)
>  }
>  EXPORT_SYMBOL(free_xenballooned_pages);
>  
> -static int __init balloon_init(void)
> +static void __init balloon_add_memory_region(unsigned long start_pfn, unsigned long pages)

That looks suspiciously like it has more than 80 lines. You ran the
_all_ of the patches through scripts/checkpath.pl right?

>  {
> -	domid_t domid = DOMID_SELF;
>  	unsigned long pfn, extra_pfn_end;
>  	struct page *page;
> +
> +	/*
> +	 * Initialise the balloon with excess memory space.  We need
> +	 * to make sure we don't add memory which doesn't exist or
> +	 * logically exist.  The E820 map can be trimmed to be smaller
> +	 * than the amount of physical memory due to the mem= command
> +	 * line parameter.  And if this is a 32-bit non-HIGHMEM kernel
> +	 * on a system with memory which requires highmem to access,
> +	 * don't try to use it.


That whole comment is just confusing.. But I do know that you
just moved it, but I wonder if it makes sense to remove it or
alter it in the future.

> +	 */
> +	extra_pfn_end = min(min(max_pfn, e820_end_of_ram_pfn()),
> +			    start_pfn + pages);
> +	for (pfn = start_pfn; pfn < extra_pfn_end; pfn++) {
> +		page = pfn_to_page(pfn);
> +		/* totalram_pages and totalhigh_pages do not
> +		   include the boot-time balloon extension, so
> +		   don't subtract from it. */
> +		__balloon_append(page);
> +	}
> +}
> +
> +static int __init balloon_init(void)
> +{
> +	domid_t domid = DOMID_SELF;
> +	int i;
>  	int ret;
>  
>  	if (!xen_domain())
> @@ -588,25 +612,10 @@ static int __init balloon_init(void)
>  	register_memory_notifier(&xen_memory_nb);
>  #endif
>  
> -	/*
> -	 * Initialise the balloon with excess memory space.  We need
> -	 * to make sure we don't add memory which doesn't exist or
> -	 * logically exist.  The E820 map can be trimmed to be smaller
> -	 * than the amount of physical memory due to the mem= command
> -	 * line parameter.  And if this is a 32-bit non-HIGHMEM kernel
> -	 * on a system with memory which requires highmem to access,
> -	 * don't try to use it.
> -	 */
> -	extra_pfn_end = min(min(max_pfn, e820_end_of_ram_pfn()),
> -			    (unsigned long)PFN_DOWN(xen_extra_mem_start + xen_extra_mem_size));
> -	for (pfn = PFN_UP(xen_extra_mem_start);
> -	     pfn < extra_pfn_end;
> -	     pfn++) {
> -		page = pfn_to_page(pfn);
> -		/* totalram_pages and totalhigh_pages do not include the boot-time
> -		   balloon extension, so don't subtract from it. */
> -		__balloon_append(page);
> -	}
> +	for (i = 0; i < XEN_EXTRA_MEM_MAX_REGIONS; i++)
> +		if (xen_extra_mem[i].size)
> +			balloon_add_memory_region(PFN_UP(xen_extra_mem[i].start),
> +						  PFN_DOWN(xen_extra_mem[i].size));
>  
>  	return 0;
>  }
> diff --git a/include/xen/page.h b/include/xen/page.h
> index 0be36b9..b432c03 100644
> --- a/include/xen/page.h
> +++ b/include/xen/page.h
> @@ -3,6 +3,13 @@
>  
>  #include <asm/xen/page.h>
>  
> -extern phys_addr_t xen_extra_mem_start, xen_extra_mem_size;
> +struct xen_memory_region {
> +	phys_addr_t start;
> +	phys_addr_t size;
> +};
> +
> +#define XEN_EXTRA_MEM_MAX_REGIONS 128 /* == E820MAX */
> +
> +extern struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS];

__initdata

>  
>  #endif	/* _XEN_PAGE_H */
> -- 
> 1.7.2.5