Re: [PATCH] libxc: refactor memory allocation functions

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-12-01 at 11:39 +0000, Wei Liu wrote:
> There were some problems with the original memory allocation functions:
> 1. xc_dom_alloc_segment and xc_dom_alloc_pad ended up calling
>    xc_dom_chk_alloc_pages while xc_dom_alloc_page open-coded everything.
> 2. xc_dom_alloc_pad didn't call dom->allocate.
> 
> Refactor the code so that:
> 1. xc_dom_alloc_{segment,pad,page} end up calling
>    xc_dom_chk_alloc_pages.
> 2. xc_dom_chk_alloc_pages calls dom->allocate.
> 
> This way we avoid scattering dom->allocate over multiple locations and
> open-coding.
> 
> Also change the return type of xc_dom_alloc_page to xen_pfn_t and return
> an invalid pfn when xc_dom_chk_alloc_pages fails.

Given this presumably the handful of callers ought to gain some error
handling in a followup patch?

xc_dom_chk_alloc_pages does log, so at least the callers needn't bother
with that.

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> 
> We don't have INVALID_PFN, maybe we need one?

We have INVALID_MFN and INVALID_P2M_ENTRY, not sure if the latter fits the
bill (or if not how it is distinct from the former).

> 
> Tested with Jessie with 64 bit pvgrub, Wheezy with 64 bit pvgrub, Wheezy
> pv
> guest, Wheezy HVM guest, Wheezy HVM with qemu stubdom.

Also by me in my previously failing environment =>

Tested-by: Ian Campbell <ian.campbell@citrix.com>

I think there are two more fixes in this space:
    libxc: correct domain builder for 64 bit guest with 32 bit tools
    libxc: use correct return type for do_memory_op()

I intend to pick up all three for commit in a moment, prod me if there are
others.

> 
>  tools/libxc/include/xc_dom.h |  2 +-
>  tools/libxc/xc_dom_core.c    | 16 +++++++---------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 2176216..2d0de8c 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -350,7 +350,7 @@ char *xc_dom_strdup(struct xc_dom_image *dom, const
> char *str);
>  
>  /* --- alloc memory pool ------------------------------------------- */
>  
> -int xc_dom_alloc_page(struct xc_dom_image *dom, char *name);
> +xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, char *name);
>  int xc_dom_alloc_segment(struct xc_dom_image *dom,
>                           struct xc_dom_seg *seg, char *name,
>                           xen_vaddr_t start, xen_vaddr_t size);
> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index 5d6c3ba..8967970 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -555,6 +555,9 @@ static int xc_dom_chk_alloc_pages(struct xc_dom_image
> *dom, char *name,
>      dom->pfn_alloc_end += pages;
>      dom->virt_alloc_end += pages * page_size;
>  
> +    if ( dom->allocate )
> +        dom->allocate(dom);
> +
>      return 0;
>  }
>  
> @@ -602,9 +605,6 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
>      if ( xc_dom_chk_alloc_pages(dom, name, pages) )
>          return -1;
>  
> -    if (dom->allocate)
> -        dom->allocate(dom);
> -
>      /* map and clear pages */
>      ptr = xc_dom_seg_to_ptr(dom, seg);
>      if ( ptr == NULL )
> @@ -621,18 +621,16 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
>      return 0;
>  }
>  
> -int xc_dom_alloc_page(struct xc_dom_image *dom, char *name)
> +xen_pfn_t xc_dom_alloc_page(struct xc_dom_image *dom, char *name)
>  {
> -    unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
>      xen_vaddr_t start;
>      xen_pfn_t pfn;
>  
>      start = dom->virt_alloc_end;
>      pfn = dom->pfn_alloc_end - dom->rambase_pfn;
> -    dom->virt_alloc_end += page_size;
> -    dom->pfn_alloc_end++;
> -    if ( dom->allocate )
> -        dom->allocate(dom);
> +
> +    if ( xc_dom_chk_alloc_pages(dom, name, 1) )
> +        return (xen_pfn_t)-1;
>  
>      DOMPRINTF("%-20s:   %-12s : 0x%" PRIx64 " (pfn 0x%" PRIpfn ")",
>                __FUNCTION__, name, start, pfn);

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel