Re: [PATCH v2 1/7] xen: vNUMA support for guests.

[George Dunlap <george.dunlap@eu.citrix.com>]

On 11/14/2013 03:26 AM, Elena Ufimtseva wrote:
> Defines interface, structures and hypercalls for guests that wish
> to retreive vNUMA topology from Xen.
> Two subop hypercalls introduced by patch:
> XEN_DOMCTL_setvnumainfo to define vNUMA domain topology per domain
> and XENMEM_get_vnuma_info to retreive that topology by guest.
>
> Signed-off-by: Elena Ufimtseva <ufimtseva@gmail.com>

Thanks, Elena -- this looks like it's much closer to being ready to be 
checked in.  A few comments:

> @@ -871,6 +872,87 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>       }
>       break;
>
> +    case XEN_DOMCTL_setvnumainfo:
> +    {
> +        unsigned int i = 0, dist_size;
> +        uint nr_vnodes;
> +        ret = -EFAULT;
> +
> +        /* Already set? */
> +        if ( d->vnuma.nr_vnodes > 0 )
> +            return 0;
> +
> +        nr_vnodes = op->u.vnuma.nr_vnodes;
> +
> +        if ( nr_vnodes == 0 )
> +            return ret;
> +        if ( nr_vnodes * nr_vnodes > UINT_MAX )
> +            return ret;
> +
> +        /*
> +         * If null, vnode_numamap will set default to
> +         * point to allocation mechanism to dont use
> +         * per physical node allocation or this is for
> +         * cases when there is no physical NUMA.
> +         */
> +        if ( guest_handle_is_null(op->u.vnuma.vdistance) ||
> +             guest_handle_is_null(op->u.vnuma.vmemrange) ||
> +             guest_handle_is_null(op->u.vnuma.vcpu_to_vnode) )
> +            goto err_dom;
> +
> +        dist_size = nr_vnodes * nr_vnodes;
> +
> +        d->vnuma.vdistance = xmalloc_array(unsigned int, dist_size);
> +        d->vnuma.vmemrange = xmalloc_array(vmemrange_t, nr_vnodes);
> +        d->vnuma.vcpu_to_vnode = xmalloc_array(unsigned int, d->max_vcpus);
> +        d->vnuma.vnode_numamap = xmalloc_array(unsigned int, nr_vnodes);

Is there a risk here that you'll leak memory if a buggy toolstack calls 
setvnuma_info multiple times?

> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index d4e479f..da458d3 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -35,6 +35,7 @@
>   #include "xen.h"
>   #include "grant_table.h"
>   #include "hvm/save.h"
> +#include "vnuma.h"
>
>   #define XEN_DOMCTL_INTERFACE_VERSION 0x00000009
>
> @@ -863,6 +864,27 @@ struct xen_domctl_set_max_evtchn {
>   typedef struct xen_domctl_set_max_evtchn xen_domctl_set_max_evtchn_t;
>   DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_max_evtchn_t);
>
> +/*
> + * XEN_DOMCTL_setvnumainfo: sets the vNUMA topology
> + * parameters a guest may request.
> + */
> +struct xen_domctl_vnuma {
> +    uint32_t nr_vnodes;
> +    uint32_t __pad;
> +    XEN_GUEST_HANDLE_64(uint) vdistance;
> +    XEN_GUEST_HANDLE_64(uint) vcpu_to_vnode;
> +    /* domain memory mapping map to physical NUMA nodes */
> +    XEN_GUEST_HANDLE_64(uint) vnode_numamap;

What is this for?  We don't pass this to the guest or seem to use it in 
any way.

  -George