Re: [PATCH v4 08/21] libxl: functions to build vmemranges for PV guest

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

On Fri, 2015-01-23 at 11:13 +0000, Wei Liu wrote:
> +    /* Generate one vmemrange for each virtual node. */
> +    next = 0;
> +    for (i = 0; i < b_info->num_vnuma_nodes; i++) {
> +        libxl_vnode_info *p = &b_info->vnuma_nodes[i];
> +
> +        GCREALLOC_ARRAY(v, i+1);

Can we not just allocate it outside the loop using
b_info->num_vnuma_nodes?

> +
> +        v[i].start = next;
> +        v[i].end = next + (p->memkb << 10);
> +        v[i].flags = 0;
> +        v[i].nid = i;
> +
> +        next = v[i].end;
> +    }
> +
> +    state->vmemranges = v;
> +    state->num_vmemranges = i;
> +
> +    return libxl__arch_vnuma_build_vmemrange(gc, domid, b_info, state);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index d012b4d..4841fef 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -339,6 +339,82 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>      return 0;
>  }
>  
> +int libxl__arch_vnuma_build_vmemrange(libxl__gc *gc,

This isn't so much "build" as "adjust" or "make it fit arch policy" or
something?

> +                                      uint32_t domid,
> +                                      libxl_domain_build_info *b_info,
> +                                      libxl__domain_build_state *state)
> +{
> +    int nid, nr_vmemrange, rc;
> +    uint32_t nr_e820, e820_count;
> +    struct e820entry map[E820MAX];
> +    xen_vmemrange_t *vmemranges;
> +
> +    /* Only touch vmemranges if it's PV guest and e820_host is true */

I take it that obeying e820_host here is only WRT where the memory is
placed, not also trying to match the underlying hosts numa layout?

> +    if (!(b_info->type == LIBXL_DOMAIN_TYPE_PV &&
> +          libxl_defbool_val(b_info->u.pv.e820_host))) {
> +        rc = 0;
> +        goto out;
> +    }
> +
> +    rc = e820_host_sanitize(gc, b_info, map, &nr_e820);
> +    if (rc) goto out;
> +
> +    /* Ditch old vmemranges and start with host E820 map. Note, memory
> +     * was gc allocated.

This makes me thing something is backwards in the layering here.

Wouldn't it make more sense for libxl__arch_vnuma_build_vmemrange to be
the entry point which starts with:
    if (nothing unusual like e820_host going on)
        return libxl__vnuma_build_pv_generic(...)

    ...the special stuff.

> +    e820_count = 0;
> +    nr_vmemrange = 0;
> +    vmemranges = NULL;
> +    for (nid = 0; nid < b_info->num_vnuma_nodes; nid++) {

This fill-loop looked ok to me.