Re: [Xen-devel] [PATCH] libxc: refactor memory allocation functions

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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

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

> ---
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> 
> 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@xxxxxxxxxx>

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel