Re: 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 03:21:03PM +0100, David Vrabel wrote:
> 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.

Ok, I ask again - what does this patch solve?

> 
> 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
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel