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

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

On Tue, Aug 16, 2011 at 11:00:36AM +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.

What does this solve? Is this a requirement for another patch? If so
please specify the name of it.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
> Is there a better way of passing the memory information to the balloon
> driver?

I think the way you have it is OK.

> ---
>  arch/x86/xen/setup.c  |   20 ++++++++++----------
>  drivers/xen/balloon.c |   48 ++++++++++++++++++++++++++++--------------------
>  include/xen/page.h    |    9 ++++++++-
>  3 files changed, 46 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index df118a8..30d0015 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];
>  
>  /* 
>   * 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;

Wouldn't this be for [1]?

>  
>  	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);
>  
> @@ -226,7 +226,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;
>  
> @@ -254,8 +254,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)
> @@ -263,10 +263,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);

So what about the highmem memory? Should there a be a check to move
the lowmem to highmem count?

>  
>  	/*
>  	 * In domU, the ISA region is normal, usable memory, but we
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 5dfd8f8..de7278a 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)
>  {
>  	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.
> +	 */
> +	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)
> +{
> +	int r;
> +
>  	if (!xen_domain())
>  		return -ENODEV;
>  
> @@ -583,25 +607,9 @@ 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 (r = 0; r < XEN_EXTRA_MEM_MAX_REGIONS; r++)

You probably should also check to make sure that the values are actually valid.
Like
        if (!xedn_extra_mem[r].start)
            continue;
> + 		balloon_add_memory_region(PFN_UP(xen_extra_mem[r].start),
> +					  PFN_DOWN(xen_extra_mem[r].size));
>  
>  	return 0;
>  }
> diff --git a/include/xen/page.h b/include/xen/page.h
> index 0be36b9..b01f6ac 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 1
> +
> +extern struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS];
>  
>  #endif	/* _XEN_PAGE_H */
> -- 
> 1.7.4.1