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

[David Vrabel <david.vrabel@citrix.com>]

On 16/08/11 14:38, Konrad Rzeszutek Wilk wrote:
> 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]?

No. I probably should have made it clear in the description but this
patch doesn't change the number of regions.  It only changes the pair of
variables to a single element array of a structure.

See XEN_EXTRA_MEM_MAX_REGIONS in include/xen/page.h:

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

>> @@ -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?

Again, the patch isn't adding any additional regions.


>> +	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() is a nop if size == 0.  But I can an
explicit check (of size) here if that is preferred.

>> + 		balloon_add_memory_region(PFN_UP(xen_extra_mem[r].start),
>> +					  PFN_DOWN(xen_extra_mem[r].size));
>>  
>>  	return 0;
>>  }

David