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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/09/2013 09:49, Elena Ufimtseva wrote:
> Defines XENMEM subop hypercall for PV vNUMA
> enabled guests and provides vNUMA topology
> information from per-domain vnuma topology build info.
>
> Signed-off-by: Elena Ufimtseva <ufimtseva@gmail.com>
>
> ---
> Changes since v1:
> * changed type from int to uint and unsigned in vNUMA structures;
> * removed unecessary file vnuma.h as requested in review
> * added domain_vnuma_destroy;
> * fixed usage of rcu_lock_domain_by_any_id;
> * removed unecessary guest_handle_cast calls;
> * coding style fixes;
> ---
>  xen/common/domain.c         |   25 +++++++++++++++-
>  xen/common/domctl.c         |   68 ++++++++++++++++++++++++++++++++++++++++++-
>  xen/common/memory.c         |   56 +++++++++++++++++++++++++++++++++++
>  xen/include/public/domctl.h |   15 +++++++++-
>  xen/include/public/memory.h |    9 +++++-
>  xen/include/xen/domain.h    |   11 +++++++
>  xen/include/xen/sched.h     |    1 +
>  xen/include/xen/vnuma.h     |   27 +++++++++++++++++
>  8 files changed, 208 insertions(+), 4 deletions(-)
>  create mode 100644 xen/include/xen/vnuma.h
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 9390a22..bb414cf 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -227,6 +227,7 @@ struct domain *domain_create(
>      spin_lock_init(&d->node_affinity_lock);
>      d->node_affinity = NODE_MASK_ALL;
>      d->auto_node_affinity = 1;
> +    d->vnuma.nr_vnodes = 0;

alloc_domain_struct() clears d, so this assignment of 0 is not needed.

>  
>      spin_lock_init(&d->shutdown_lock);
>      d->shutdown_code = -1;
> @@ -530,8 +531,9 @@ int domain_kill(struct domain *d)
>          evtchn_destroy(d);
>          gnttab_release_mappings(d);
>          tmem_destroy(d->tmem);
> -        domain_set_outstanding_pages(d, 0);

Any particular reason for this function call to move?  I cant see any
reason.

>          d->tmem = NULL;
> +        domain_set_outstanding_pages(d, 0);
> +        domain_vnuma_destroy(&d->vnuma);
>          /* fallthrough */
>      case DOMDYING_dying:
>          rc = domain_relinquish_resources(d);
> @@ -1279,6 +1281,27 @@ int continue_hypercall_on_cpu(
>      return 0;
>  }
>  
> +void domain_vnuma_destroy(struct domain_vnuma_info *v)
> +{
> +    if (v->vnuma_memblks) {
> +        xfree(v->vnuma_memblks);
> +        v->vnuma_memblks = NULL;
> +    }

Coding style (see CODING_STYLE in the root of the tree) so this would
become:

if ( v->vnuma_memblks )
{
  xfree(v->vnuma_memblks);
  v->vnuma_memblks = NULL;
}

However, xfree() (like regular free()) is happy with NULL pointers, so
you can drop the if altogether.


> +    if (v->vcpu_to_vnode) {
> +        xfree(v->vcpu_to_vnode);
> +        v->vcpu_to_vnode = NULL;
> +    }
> +    if (v->vdistance) {
> +        xfree(v->vdistance);
> +        v->vdistance = NULL;
> +    }
> +    if (v->vnode_to_pnode) {
> +        xfree(v->vnode_to_pnode);
> +        v->vnode_to_pnode = NULL;
> +    }
> +    v->nr_vnodes = 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 9760d50..e5d05c7 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -29,6 +29,7 @@
>  #include <asm/page.h>
>  #include <public/domctl.h>
>  #include <xsm/xsm.h>
> +#include <xen/vnuma.h>
>  
>  static DEFINE_SPINLOCK(domctl_lock);
>  DEFINE_SPINLOCK(vcpu_alloc_lock);
> @@ -862,7 +863,72 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          ret = set_global_virq_handler(d, virq);
>      }
>      break;
> -

Please leave this blank line in here.

> +    case XEN_DOMCTL_setvnumainfo:
> +    {
> +        unsigned int i, j, nr_vnodes, dist_size;
> +        unsigned int dist, vmap, vntop;
> +        vnuma_memblk_t vmemblk;
> +        
> +        ret = -EINVAL;
> +        dist = i = j = 0;
> +        nr_vnodes = op->u.vnuma.nr_vnodes; 
> +        if (nr_vnodes <= 0 || nr_vnodes > NR_CPUS)
> +            break;

nr_vnodes is unsigned.  It cannot possibly be less than 0.

> +        d->vnuma.nr_vnodes = nr_vnodes;
> +        dist_size = nr_vnodes * nr_vnodes;
> +        if (
> +        (d->vnuma.vdistance = xmalloc_bytes(
> +                 sizeof(*d->vnuma.vdistance) * dist_size)) == NULL ||

sizeof in an operator not a function.  As you are not passing a type,
the brackets are not required.

> +        (d->vnuma.vnuma_memblks = xmalloc_bytes(
> +                 sizeof(*d->vnuma.vnuma_memblks) * nr_vnodes)) == NULL ||
> +        (d->vnuma.vcpu_to_vnode = xmalloc_bytes(
> +                 sizeof(*d->vnuma.vcpu_to_vnode) * d->max_vcpus)) == NULL ||
> +        (d->vnuma.vnode_to_pnode = xmalloc_bytes(
> +                 sizeof(*d->vnuma.vnode_to_pnode) * nr_vnodes)) == NULL )
> +            goto err_dom;
> +        for ( i = 0; i < nr_vnodes; i++ )
> +            for ( j = 0; j < nr_vnodes; j++ )
> +            {
> +                if ( unlikely(__copy_from_guest_offset(&dist,
> +                                        op->u.vnuma.vdistance,
> +                                        __vnode_distance_offset(d, i, j), 1)) )
> +                    goto err_dom;

This error logic is broken.  A failure in __copy_from_guest_offset()
should result in -EFAULT, but will result in -EINVAL after following
err_dom;

> +                __vnode_distance_set(d, i, j, dist);
> +            }
> +        for ( i = 0; i < nr_vnodes; i++ )
> +        {
> +            if ( unlikely(__copy_from_guest_offset(&vmemblk,
> +                                    op->u.vnuma.vnuma_memblks, i, 1)) )
> +                goto err_dom;
> +            d->vnuma.vnuma_memblks[i].start = vmemblk.start;
> +            d->vnuma.vnuma_memblks[i].end = vmemblk.end;
> +        }
> +        for ( i = 0; i < d->max_vcpus; i++ )
> +        {
> +            if ( unlikely(__copy_from_guest_offset(&vmap,
> +                           op->u.vnuma.vcpu_to_vnode, i, 1)) )
> +                goto err_dom;
> +            d->vnuma.vcpu_to_vnode[i] = vmap;
> +        }
> +        if ( !guest_handle_is_null(op->u.vnuma.vnode_to_pnode) )
> +        {
> +            for ( i = 0; i < nr_vnodes; i++ )
> +            {
> +                if ( unlikely(__copy_from_guest_offset(&vntop,
> +                                        op->u.vnuma.vnode_to_pnode, i, 1)) )
> +                    goto err_dom;
> +                d->vnuma.vnode_to_pnode[i] = vntop;
> +            }
> +        }
> +        else
> +            for(i = 0; i < nr_vnodes; i++)
> +                d->vnuma.vnode_to_pnode[i] = NUMA_NO_NODE;
> +        ret = 0;
> +        break;
> +err_dom:
> +        ret = -EINVAL;
> +    }
> +    break;

And an extra line in here please.

>      default:
>          ret = arch_do_domctl(op, d, u_domctl);
>          break;
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 50b740f..6dc2452 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -28,6 +28,7 @@
>  #include <public/memory.h>
>  #include <xsm/xsm.h>
>  #include <xen/trace.h>
> +#include <xen/vnuma.h>
>  
>  struct memop_args {
>      /* INPUT */
> @@ -732,7 +733,62 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          rcu_unlock_domain(d);
>  
>          break;
> +    case XENMEM_get_vnuma_info:
> +    {
> +        int i, j;
> +        struct vnuma_topology_info mtopology;
> +        struct vnuma_topology_info touser_topo;
> +        struct domain *d;
> +        unsigned int max_pages, max_vcpus, nr_vnodes;
> +        vnuma_memblk_t *vblks;
>  
> +        rc = -EINVAL;
> +        if ( guest_handle_is_null(arg) )
> +            return rc;
> +        if( copy_from_guest(&mtopology, arg, 1) )
> +            return -EINVAL;

-EFAULT;

> +        if ( (d = rcu_lock_domain_by_any_id(mtopology.domid)) == NULL )
> +            return -EINVAL;

-ESRCH ( I think? )

> +        touser_topo.nr_vnodes = d->vnuma.nr_vnodes;
> +        max_pages = d->max_pages;
> +        max_vcpus = d->max_vcpus;
> +        rcu_unlock_domain(d);
> +    
> +        nr_vnodes = touser_topo.nr_vnodes;
> +        rc = copy_to_guest(arg, &touser_topo, 1);
> +        if ( rc )
> +            return -EFAULT;
> +        if ( nr_vnodes == 0 || nr_vnodes > max_vcpus )
> +            return -EFAULT;

-EINVAL;

> +        vblks = xmalloc_array(struct vnuma_memblk, nr_vnodes);
> +        if ( vblks == NULL )
> +            return -EFAULT;

-ENOMEM;


Now, if you have an unconditional rc = -EFAULT then all your goto out
logic will be correct

> +        for ( i = 0; i < nr_vnodes; i++ )
> +        {
> +                if ( copy_to_guest_offset(mtopology.vnuma_memblks, i,
> +                           &d->vnuma.vnuma_memblks[i], 1) < 0 )
> +                    goto out;
> +        }
> +        for ( i = 0; i < touser_topo.nr_vnodes; i++ )
> +            for ( j = 0; j < touser_topo.nr_vnodes; j++ )
> +            {
> +                if ( copy_to_guest_offset(mtopology.vdistance,
> +                                            __vnode_distance_offset(d, i, j),
> +                                            &__vnode_distance(d, i, j), 1) )
> +                    goto out;
> +            }
> +        for ( i = 0; i < d->max_vcpus ; i++ )
> +        {
> +            if ( copy_to_guest_offset(mtopology.vcpu_to_vnode, i,
> +                                        &d->vnuma.vcpu_to_vnode[i], 1) )
> +                    goto out;
> +        }
> +        return rc;

But this would need to turn into rc = 0; to avoid return -EFAULT in the
success case, and unconditionally leaking vblks

> +out:
> +        if ( vblks ) xfree(vblks);
> +        return rc;
> +        break;
> +    }
>      default:
>          rc = arch_memory_op(op, arg);
>          break;
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 4c5b2bb..3574d0a 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 "memory.h"
>  
>  #define XEN_DOMCTL_INTERFACE_VERSION 0x00000009
>  
> @@ -852,6 +853,17 @@ struct xen_domctl_set_broken_page_p2m {
>  typedef struct xen_domctl_set_broken_page_p2m xen_domctl_set_broken_page_p2m_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_broken_page_p2m_t);
>  
> +struct xen_domctl_vnuma {
> +    uint16_t nr_vnodes;
> +    XEN_GUEST_HANDLE_64(uint) vdistance;
> +    XEN_GUEST_HANDLE_64(uint) vcpu_to_vnode;
> +    XEN_GUEST_HANDLE_64(uint) vnode_to_pnode;
> +    XEN_GUEST_HANDLE_64(vnuma_memblk_t) vnuma_memblks;
> +};
> +
> +typedef struct xen_domctl_vnuma xen_domctl_vnuma_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_vnuma_t);
> +
>  struct xen_domctl {
>      uint32_t cmd;
>  #define XEN_DOMCTL_createdomain                   1
> @@ -920,6 +932,7 @@ struct xen_domctl {
>  #define XEN_DOMCTL_set_broken_page_p2m           67
>  #define XEN_DOMCTL_setnodeaffinity               68
>  #define XEN_DOMCTL_getnodeaffinity               69
> +#define XEN_DOMCTL_setvnumainfo                  70
>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> @@ -979,6 +992,7 @@ struct xen_domctl {
>          struct xen_domctl_set_broken_page_p2m set_broken_page_p2m;
>          struct xen_domctl_gdbsx_pauseunp_vcpu gdbsx_pauseunp_vcpu;
>          struct xen_domctl_gdbsx_domstatus   gdbsx_domstatus;
> +        struct xen_domctl_vnuma             vnuma;
>          uint8_t                             pad[128];
>      } u;
>  };
> @@ -986,7 +1000,6 @@ typedef struct xen_domctl xen_domctl_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_t);
>  
>  #endif /* __XEN_PUBLIC_DOMCTL_H__ */
> -

Spurious whitespace change.  Please remove

>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 7a26dee..28f6aaf 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -453,12 +453,19 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>   * Caller must be privileged or the hypercall fails.
>   */
>  #define XENMEM_claim_pages                  24
> -

And here

>  /*
>   * XENMEM_claim_pages flags - the are no flags at this time.
>   * The zero value is appropiate.
>   */
>  
> +struct vnuma_memblk {
> +              uint64_t start, end;
> +};
> +typedef struct vnuma_memblk vnuma_memblk_t;
> +DEFINE_XEN_GUEST_HANDLE(vnuma_memblk_t);
> +
> +#define XENMEM_get_vnuma_info               25
> +
>  #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>  
>  #endif /* __XEN_PUBLIC_MEMORY_H__ */
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index a057069..c9d53e3 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -4,6 +4,7 @@
>  
>  #include <public/xen.h>
>  #include <asm/domain.h>
> +#include <public/memory.h>
>  
>  typedef union {
>      struct vcpu_guest_context *nat;
> @@ -89,4 +90,14 @@ extern unsigned int xen_processor_pmbits;
>  
>  extern bool_t opt_dom0_vcpus_pin;
>  
> +struct domain_vnuma_info {
> +    uint16_t nr_vnodes;
> +    uint *vdistance;
> +    uint *vcpu_to_vnode;
> +    uint *vnode_to_pnode;
> +    vnuma_memblk_t *vnuma_memblks;
> +};
> +
> +void domain_vnuma_destroy(struct domain_vnuma_info *v);
> +
>  #endif /* __XEN_DOMAIN_H__ */
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ae6a3b8..cb023cf 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -377,6 +377,7 @@ struct domain
>      nodemask_t node_affinity;
>      unsigned int last_alloc_node;
>      spinlock_t node_affinity_lock;
> +    struct domain_vnuma_info vnuma;
>  };
>  
>  struct domain_setup_info
> diff --git a/xen/include/xen/vnuma.h b/xen/include/xen/vnuma.h
> new file mode 100644
> index 0000000..0b41da0
> --- /dev/null
> +++ b/xen/include/xen/vnuma.h
> @@ -0,0 +1,27 @@
> +#ifndef _VNUMA_H
> +#define _VNUMA_H
> +#include <public/memory.h>
> +
> +/* DEFINE_XEN_GUEST_HANDLE(vnuma_memblk_t); */
> +
> +struct vnuma_topology_info {
> +    domid_t domid;
> +    uint16_t nr_vnodes;
> +    uint32_t _pad;
> +    XEN_GUEST_HANDLE_64(uint) vdistance;
> +    XEN_GUEST_HANDLE_64(uint) vcpu_to_vnode;
> +    XEN_GUEST_HANDLE_64(vnuma_memblk_t) vnuma_memblks;
> +};
> +typedef struct vnuma_topology_info vnuma_topology_info_t;
> +DEFINE_XEN_GUEST_HANDLE(vnuma_topology_info_t);
> +
> +#define __vnode_distance_offset(_dom, _i, _j) \
> +        ( ((_j) * ((_dom)->vnuma.nr_vnodes)) + (_i) )
> +
> +#define __vnode_distance(_dom, _i, _j) \
> +        ( (_dom)->vnuma.vdistance[__vnode_distance_offset((_dom), (_i), (_j))] )

You expand _dom here twice, which is bad practice for macros.

I suggest static inline functions as a better alternative.

~Andrew

> +
> +#define __vnode_distance_set(_dom, _i, _j, _v) \
> +        do { __vnode_distance((_dom), (_i), (_j)) = (_v); } while(0)
> +
> +#endif