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>
---
xen/common/domain.c | 10 ++++++
xen/common/domctl.c | 82 +++++++++++++++++++++++++++++++++++++++++++
xen/common/memory.c | 36 +++++++++++++++++++
xen/include/public/domctl.h | 24 +++++++++++++
xen/include/public/memory.h | 8 +++++
xen/include/public/vnuma.h | 44 +++++++++++++++++++++++
xen/include/xen/domain.h | 10 ++++++
xen/include/xen/sched.h | 1 +
8 files changed, 215 insertions(+)
create mode 100644 xen/include/public/vnuma.h
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8c9b813..6433383 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -539,6 +539,7 @@ int domain_kill(struct domain *d)
tmem_destroy(d->tmem);
domain_set_outstanding_pages(d, 0);
d->tmem = NULL;
+ domain_vnuma_destroy(&d->vnuma);
/* fallthrough */
case DOMDYING_dying:
rc = domain_relinquish_resources(d);
@@ -1297,6 +1298,15 @@ int continue_hypercall_on_cpu(
return 0;
}
+void domain_vnuma_destroy(struct domain_vnuma_info *v)
+{
+ v->nr_vnodes = 0;
+ xfree(v->vmemrange);
+ xfree(v->vcpu_to_vnode);
+ xfree(v->vdistance);
+ xfree(v->vnode_numamap);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 870eef1..57154d5 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 <public/vnuma.h>
static DEFINE_SPINLOCK(domctl_lock);
DEFINE_SPINLOCK(vcpu_alloc_lock);
@@ -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);
+
+ if ( d->vnuma.vdistance == NULL ||
+ d->vnuma.vmemrange == NULL ||
+ d->vnuma.vcpu_to_vnode == NULL ||
+ d->vnuma.vnode_numamap == NULL )
+ {
+ ret = -ENOMEM;
+ goto err_dom;
+ }
+ if ( unlikely(copy_from_guest(d->vnuma.vdistance,
+ op->u.vnuma.vdistance,
+ dist_size)) )
+ goto err_dom;
+ if ( unlikely(copy_from_guest(d->vnuma.vmemrange,
+ op->u.vnuma.vmemrange,
+ nr_vnodes)) )
+ goto err_dom;
+ if ( unlikely(copy_from_guest(d->vnuma.vcpu_to_vnode,
+ op->u.vnuma.vcpu_to_vnode,
+ d->max_vcpus)) )
+ goto err_dom;
+ if ( !guest_handle_is_null(op->u.vnuma.vnode_numamap) )
+ {
+ if ( unlikely(copy_from_guest(d->vnuma.vnode_numamap,
+ op->u.vnuma.vnode_numamap,
+ nr_vnodes)) )
+ goto err_dom;
+ }
+ else
+ for ( i = 0; i < nr_vnodes; i++ )
+ d->vnuma.vnode_numamap[i] = NUMA_NO_NODE;
+
+ /* Everything is good, lets set the number of vnodes */
+ d->vnuma.nr_vnodes = nr_vnodes;
+ ret = 0;
+err_dom:
+ if ( ret != 0 )
+ {
+ d->vnuma.nr_vnodes = 0;
+ xfree(d->vnuma.vdistance);
+ xfree(d->vnuma.vmemrange);
+ xfree(d->vnuma.vcpu_to_vnode);
+ xfree(d->vnuma.vnode_numamap);
+ }
+ }
+ break;
+
default:
ret = arch_do_domctl(op, d, u_domctl);
break;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 50b740f..38108ce 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 <public/vnuma.h>
struct memop_args {
/* INPUT */
@@ -733,6 +734,41 @@ long do_memory_op(unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
break;
+ case XENMEM_get_vnuma_info:
+ {
+ vnuma_topology_info_t mtopology;
+ struct domain *d;
+
+ rc = -EFAULT;
+ if ( copy_from_guest(&mtopology, arg, 1) )
+ return -EFAULT;
+ if ( (d = rcu_lock_domain_by_any_id(mtopology.domid)) == NULL )
+ return -ESRCH;
+
+ if ( (d->vnuma.nr_vnodes == 0) || (d->vnuma.nr_vnodes >
d->max_vcpus) )
+ return EOPNOTSUPP;
+
+ if ( __copy_to_guest(mtopology.vmemrange,
+ d->vnuma.vmemrange,
+ d->vnuma.nr_vnodes) != 0 )
+ goto vnumaout;
+ if ( __copy_to_guest(mtopology.vdistance,
+ d->vnuma.vdistance,
+ d->vnuma.nr_vnodes * d->vnuma.nr_vnodes)
!= 0 )
+ goto vnumaout;
+ if ( __copy_to_guest(mtopology.vcpu_to_vnode,
+ d->vnuma.vcpu_to_vnode,
+ d->max_vcpus) != 0 )
+ goto vnumaout;
+
+ if ( __copy_to_guest(mtopology.nr_vnodes, &d->vnuma.nr_vnodes,
1) != 0 )
+ goto vnumaout;
+ rc = 0;
+vnumaout:
+ rcu_unlock_domain(d);
+ break;
+ }
+
default:
rc = arch_memory_op(op, arg);
break;
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;
+ /*
+ * memory rages that vNUMA node can represent
+ * If more than one, its a linked list.
+ */
+ XEN_GUEST_HANDLE_64(vmemrange_t) vmemrange;
+};
+
+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
@@ -932,6 +954,7 @@ struct xen_domctl {
#define XEN_DOMCTL_setnodeaffinity 68
#define XEN_DOMCTL_getnodeaffinity 69
#define XEN_DOMCTL_set_max_evtchn 70
+#define XEN_DOMCTL_setvnumainfo 71
#define XEN_DOMCTL_gdbsx_guestmemio 1000
#define XEN_DOMCTL_gdbsx_pausevcpu 1001
#define XEN_DOMCTL_gdbsx_unpausevcpu 1002
@@ -992,6 +1015,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;
};
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 7a26dee..75a88b6 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -459,6 +459,14 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
* The zero value is appropiate.
*/
+/*
+ * XENMEM_get_vnuma_info used by caller to retrieve
+ * vNUMA topology constructed for particular domain.
+ *
+ * The data exchanged is presented by vnuma_topology_info.
+ */
+#define XENMEM_get_vnuma_info 25
+
#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
#endif /* __XEN_PUBLIC_MEMORY_H__ */
diff --git a/xen/include/public/vnuma.h b/xen/include/public/vnuma.h
new file mode 100644
index 0000000..36ee387
--- /dev/null
+++ b/xen/include/public/vnuma.h
@@ -0,0 +1,44 @@
+#ifndef _XEN_PUBLIC_VNUMA_H
+#define _XEN_PUBLIC_VNUMA_H
+#include "memory.h"
+#include "xen.h"
+
+/*
+ * Following structures are used to represent vNUMA
+ * topology to guest if requested.
+ */
+
+/*
+ * Memory ranges can be used to define
+ * vNUMA memory node boundaries by the
+ * linked list. As of now, only one range
+ * per domain is suported.
+ */
+
+struct vmemrange {
+ uint64_t start, end;
+ struct vmemrange *next;
+};
+typedef struct vmemrange vmemrange_t;
+DEFINE_XEN_GUEST_HANDLE(vmemrange_t);
+
+/*
+ * vNUMA topology specifies vNUMA node
+ * number, distance table, memory ranges and
+ * vcpu mapping provided for guests.
+ */
+
+struct vnuma_topology_info {
+ /* IN */
+ domid_t domid;
+ uint32_t _pad;
+ /* OUT */
+ XEN_GUEST_HANDLE(uint) nr_vnodes;
+ XEN_GUEST_HANDLE(uint) vdistance;
+ XEN_GUEST_HANDLE(uint) vcpu_to_vnode;
+ XEN_GUEST_HANDLE(vmemrange_t) vmemrange;
+};
+typedef struct vnuma_topology_info vnuma_topology_info_t;
+DEFINE_XEN_GUEST_HANDLE(vnuma_topology_info_t);
+
+#endif
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index a057069..bc61bab 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -89,4 +89,14 @@ extern unsigned int xen_processor_pmbits;
extern bool_t opt_dom0_vcpus_pin;
+struct domain_vnuma_info {
+ uint nr_vnodes;
+ uint *vdistance;
+ uint *vcpu_to_vnode;
+ uint *vnode_numamap;
+ struct vmemrange *vmemrange;
+};
+
+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 2397537..9638780 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -409,6 +409,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
--
1.7.10.4
On mer, 2013-11-13 at 22:26 -0500, Elena Ufimtseva wrote:> Defines interface, structures and hypercalls for guests that wish > to retreive vNUMA topology from Xen. >Well, not only "for guests that wish to retrieve" the vNUMA topology, also for toolstacks that wish to configure it.> 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> > ---> diff --git a/xen/common/domain.c b/xen/common/domain.c > @@ -539,6 +539,7 @@ int domain_kill(struct domain *d) > tmem_destroy(d->tmem); > domain_set_outstanding_pages(d, 0); > d->tmem = NULL; > + domain_vnuma_destroy(&d->vnuma); > /* fallthrough */ > case DOMDYING_dying: > rc = domain_relinquish_resources(d); > @@ -1297,6 +1298,15 @@ int continue_hypercall_on_cpu( > return 0; > } > > +void domain_vnuma_destroy(struct domain_vnuma_info *v) > +{I think vnuma_destroy() is ok, as it is tmem_destroy(), evtchn_destory(), etc.> + v->nr_vnodes = 0; > + xfree(v->vmemrange); > + xfree(v->vcpu_to_vnode); > + xfree(v->vdistance); > + xfree(v->vnode_numamap); > +} > +> diff --git a/xen/common/domctl.c b/xen/common/domctl.c > @@ -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; > +Mmm... I think this three ''return''s ought all to be ''break''s, or you''ll never get to execute the common exit path from do_domctl().> + /* > + * 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; > +Sorry, I''m not sure I fully understand the comment: you''re saying that it is ok for vnuma_nodemap to be NULL, right?> + 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); > + > + if ( d->vnuma.vdistance == NULL || > + d->vnuma.vmemrange == NULL || > + d->vnuma.vcpu_to_vnode == NULL || > + d->vnuma.vnode_numamap == NULL ) > + { > + ret = -ENOMEM; > + goto err_dom; > + } >Well, in general, things like just ''err'' or ''out'' are fine as labels. However, in this case, since we''re inside quite a big switch{}, a bit more of context could be helpful. What about killing the ''_dom'' part (which does not really say much) and putting a meaningful prefix? Also, it''s not like the code you''re jumping at is executed only on error, so even the ''err_'' part looks incorrect. Personally, I''d go for something like ''setvnumainfo_out'' (see XEN_DOMCTL_getdomaininfo for reference).> + if ( unlikely(copy_from_guest(d->vnuma.vdistance, > + op->u.vnuma.vdistance, > + dist_size)) ) > + goto err_dom; > + if ( unlikely(copy_from_guest(d->vnuma.vmemrange, > + op->u.vnuma.vmemrange, > + nr_vnodes)) ) > + goto err_dom; > + if ( unlikely(copy_from_guest(d->vnuma.vcpu_to_vnode, > + op->u.vnuma.vcpu_to_vnode, > + d->max_vcpus)) ) > + goto err_dom; > + if ( !guest_handle_is_null(op->u.vnuma.vnode_numamap) ) > + { > + if ( unlikely(copy_from_guest(d->vnuma.vnode_numamap, > + op->u.vnuma.vnode_numamap, > + nr_vnodes)) ) > + goto err_dom; > + } > + else > + for ( i = 0; i < nr_vnodes; i++ ) > + d->vnuma.vnode_numamap[i] = NUMA_NO_NODE; > + > + /* Everything is good, lets set the number of vnodes */ > + d->vnuma.nr_vnodes = nr_vnodes;Put a blank line here.> + ret = 0; > +err_dom: > + if ( ret != 0 ) > + { > + d->vnuma.nr_vnodes = 0; > + xfree(d->vnuma.vdistance); > + xfree(d->vnuma.vmemrange); > + xfree(d->vnuma.vcpu_to_vnode); > + xfree(d->vnuma.vnode_numamap); > + } > + } > + break; > +> diff --git a/xen/common/memory.c b/xen/common/memory.c > @@ -733,6 +734,41 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > break; > > + case XENMEM_get_vnuma_info: > + { > + vnuma_topology_info_t mtopology; > + struct domain *d; > + > + rc = -EFAULT; > + if ( copy_from_guest(&mtopology, arg, 1) ) > + return -EFAULT; > + if ( (d = rcu_lock_domain_by_any_id(mtopology.domid)) == NULL ) > + return -ESRCH; > + > + if ( (d->vnuma.nr_vnodes == 0) || (d->vnuma.nr_vnodes > d->max_vcpus) ) > + return EOPNOTSUPP; >I think you need to rcu_unlock_xxx() here. Also, -EONOTSUPP (note the ''-'') ?> + > + if ( __copy_to_guest(mtopology.vmemrange, > + d->vnuma.vmemrange, > + d->vnuma.nr_vnodes) != 0 ) > + goto vnumaout; > + if ( __copy_to_guest(mtopology.vdistance, > + d->vnuma.vdistance, > + d->vnuma.nr_vnodes * d->vnuma.nr_vnodes) != 0 ) > + goto vnumaout; > + if ( __copy_to_guest(mtopology.vcpu_to_vnode, > + d->vnuma.vcpu_to_vnode, > + d->max_vcpus) != 0 ) > + goto vnumaout; > + > + if ( __copy_to_guest(mtopology.nr_vnodes, &d->vnuma.nr_vnodes, 1) != 0 ) > + goto vnumaout; > + rc = 0; > +vnumaout: > vnumainfo_out ?> + rcu_unlock_domain(d); > + break; > + } > + > default: > rc = arch_memory_op(op, arg); > break;> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > +/* > + * 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; > + /* > + * memory rages that vNUMA node can represent^ranges> + * If more than one, its a linked list. > + */ > + XEN_GUEST_HANDLE_64(vmemrange_t) vmemrange; > +};> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h > index a057069..bc61bab 100644 > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -89,4 +89,14 @@ extern unsigned int xen_processor_pmbits; > > extern bool_t opt_dom0_vcpus_pin; > > +struct domain_vnuma_info { > + uint nr_vnodes; > + uint *vdistance; > + uint *vcpu_to_vnode; > + uint *vnode_numamap; > + struct vmemrange *vmemrange; > +}; > +I think you can kill the ''domain_'' prefix. It''s pretty clear this is a per-domain thing, from the fact that it lives inside struct domain.> +void domain_vnuma_destroy(struct domain_vnuma_info *v); > +Why do you need to declare this function here? Isn''t this used only in domain.c ? Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[ This series hasn''t been threaded correctly, please ensure you use the --compose option to git send-email to write the cover letter. ] On 14/11/13 03:26, Elena Ufimtseva wrote:> --- /dev/null > +++ b/xen/include/public/vnuma.h > @@ -0,0 +1,44 @@ > +#ifndef _XEN_PUBLIC_VNUMA_H > +#define _XEN_PUBLIC_VNUMA_H > +#include "memory.h" > +#include "xen.h" > + > +/* > + * Following structures are used to represent vNUMA > + * topology to guest if requested. > + */ > + > +/* > + * Memory ranges can be used to define > + * vNUMA memory node boundaries by the > + * linked list. As of now, only one range > + * per domain is suported. > + */ > + > +struct vmemrange { > + uint64_t start, end; > + struct vmemrange *next;I think this probably wants to be an index into the vmemrange array in struct vnuma_topology_info. It certainly cannot be a bare pointer like this.> +}; > +typedef struct vmemrange vmemrange_t; > +DEFINE_XEN_GUEST_HANDLE(vmemrange_t); > + > +/* > + * vNUMA topology specifies vNUMA node > + * number, distance table, memory ranges and > + * vcpu mapping provided for guests. > + */ > + > +struct vnuma_topology_info { > + /* IN */ > + domid_t domid; > + uint32_t _pad; > + /* OUT */ > + XEN_GUEST_HANDLE(uint) nr_vnodes; > + XEN_GUEST_HANDLE(uint) vdistance; > + XEN_GUEST_HANDLE(uint) vcpu_to_vnode; > + XEN_GUEST_HANDLE(vmemrange_t) vmemrange; > +};XEN_GUEST_HANDLE() has different size in 32-bit and 64-bit x86 but you have no compat code to translate the structure. Using: union { XEN_GUEST_HANDLE(uint) h; uint64_t _pad; } nr_vnodes; for each field would produce a struct with a uniform ABI. See the recently added struct xen_kexec_segment for an example. David
On gio, 2013-11-14 at 11:48 +0000, David Vrabel wrote:> On 14/11/13 03:26, Elena Ufimtseva wrote: > > +/* > > + * Memory ranges can be used to define > > + * vNUMA memory node boundaries by the > > + * linked list. As of now, only one range > > + * per domain is suported. > > + */ > > + > > +struct vmemrange { > > + uint64_t start, end; > > + struct vmemrange *next; > > I think this probably wants to be an index into the vmemrange array in > struct vnuma_topology_info. It certainly cannot be a bare pointer like > this. >I think Elena is aiming at a liked list, and so there really is no such thing as a vmemrange array. This is (probably) because right now only one memory range is supported, and thus it sounds a bit too much to have an array there already (e.g., how big?), but at the same time she wanted to be sure to leave room for future extensions (namely, multiple memory ranges in each node). So, if we can ask, with that in mind, what do you think the best ABI would be? Thanks and Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Thu, Nov 14, 2013 at 7:11 AM, Dario Faggioli <dario.faggioli@citrix.com> wrote:> On gio, 2013-11-14 at 11:48 +0000, David Vrabel wrote: >> On 14/11/13 03:26, Elena Ufimtseva wrote: >> > +/* >> > + * Memory ranges can be used to define >> > + * vNUMA memory node boundaries by the >> > + * linked list. As of now, only one range >> > + * per domain is suported. >> > + */ >> > + >> > +struct vmemrange { >> > + uint64_t start, end; >> > + struct vmemrange *next; >>Thank you for reviewing. Will take into account the comments above.>> I think this probably wants to be an index into the vmemrange array in >> struct vnuma_topology_info. It certainly cannot be a bare pointer like >> this. >> > I think Elena is aiming at a liked list, and so there really is no such > thing as a vmemrange array. This is (probably) because right now only > one memory range is supported, and thus it sounds a bit too much to have > an array there already (e.g., how big?), but at the same time she wanted > to be sure to leave room for future extensions (namely, multiple memory > ranges in each node).Right, that was my idea of a linked list of memory ranges.> > So, if we can ask, with that in mind, what do you think the best ABI > would be? > > Thanks and Regards, > Dario > > -- > <<This happens because I choose it to happen!>> (Raistlin Majere) > ----------------------------------------------------------------- > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) >-- Elena
On 11/14/2013 11:18 AM, Dario Faggioli wrote:>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h >> index a057069..bc61bab 100644 >> --- a/xen/include/xen/domain.h >> +++ b/xen/include/xen/domain.h >> @@ -89,4 +89,14 @@ extern unsigned int xen_processor_pmbits; >> >> extern bool_t opt_dom0_vcpus_pin; >> >> +struct domain_vnuma_info { >> + uint nr_vnodes; >> + uint *vdistance; >> + uint *vcpu_to_vnode; >> + uint *vnode_numamap; >> + struct vmemrange *vmemrange; >> +}; >> + > I think you can kill the ''domain_'' prefix. It''s pretty clear this is a > per-domain thing, from the fact that it lives inside struct domain.Still, it doesn''t hurt to have a little bit of extra context. If you look above in this file, for example, it has "vcpu_guest_context" -- even though it should be obvious that guest context is per-vcpu. :-)> >> +void domain_vnuma_destroy(struct domain_vnuma_info *v); >> + > Why do you need to declare this function here? Isn''t this used only in > domain.c ?In fact, it should probably be static. -George
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
On Thu, Nov 14, 2013 at 4:59 PM, George Dunlap <george.dunlap@eu.citrix.com> wrote:> 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: >Hi George, good to hear )> >> @@ -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?In order to run through xmalloc again, d->vnuma.nr_vnodes should be equal to zero. The only way to set it to zero (for domain) is to exit from this call with error (wich sets it to zero and releases memory) or to call vnuma_destroy for domain. Zero as a new value for nr_vnodes is not accepted so it cant be set by issuing do_domctl.> > >> 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.This is used to set domain vnode to pnode mapping, do allocations later and keep it on per-domain basis for later use. For example, as vnuma aware ballooning will request this information.> > -George-- Elena
On 11/14/2013 10:51 PM, Elena Ufimtseva wrote:> On Thu, Nov 14, 2013 at 4:59 PM, George Dunlap > <george.dunlap@eu.citrix.com> wrote: >> 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: >> > Hi George, good to hear ) >> >>> @@ -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? > > In order to run through xmalloc again, d->vnuma.nr_vnodes should be > equal to zero. > The only way to set it to zero (for domain) is to exit from this call > with error (wich sets it to zero and releases memory) or to call > vnuma_destroy for domain. Zero as a new value for nr_vnodes is not > accepted so it cant be set by issuing do_domctl.Oh, right -- sorry, I missed that. In that case, can I suggest changing that comment to, "Don''t initialize again if we''ve already been initialized once" or something like that. You should probably add "ASSERT(d->vnuma.$FOO==NULL);" as well, as a precaution (and as a hint to people who are reading the code too quickly). Also, that should return an error (-EBUSY? -EINVAL?), rather than returning success.> >> >> >>> 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. > > This is used to set domain vnode to pnode mapping, do allocations > later and keep it on per-domain > basis for later use. For example, as vnuma aware ballooning will > request this information.Oh, right -- we wanted the HV to be able to translate the vnuma ballooning requests from vnode to pnode without the guest knowing about it. Sorry, forgot about that. It''s probably worth noting that, at least in the changelog, and probably in a comment somewhere. -George
>>> On 14.11.13 at 12:18, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On mer, 2013-11-13 at 22:26 -0500, Elena Ufimtseva wrote: >> @@ -1297,6 +1298,15 @@ int continue_hypercall_on_cpu( >> return 0; >> } >> >> +void domain_vnuma_destroy(struct domain_vnuma_info *v) >> +{ > I think vnuma_destroy() is ok, as it is tmem_destroy(), > evtchn_destory(), etc.And the parameter would be very nice to be named other than "v" (which we commonly use for struct vcpu * instances - in fact I got misled to think you would be freeing per-vCPU information here... Jan
>>> On 14.11.13 at 04:26, Elena Ufimtseva <ufimtseva@gmail.com> wrote: > + case XEN_DOMCTL_setvnumainfo: > + { > + unsigned int i = 0, dist_size; > + uint nr_vnodes;"unsigned int" or "uint" consistently please (preferably the former). Also the initializer for "i" seems pointless.> + ret = -EFAULT; > + > + /* Already set? */ > + if ( d->vnuma.nr_vnodes > 0 ) > + return 0;This surely needs to be some -E (or else how would the caller know that the function didn''t do what was asked for).> + > + nr_vnodes = op->u.vnuma.nr_vnodes; > + > + if ( nr_vnodes == 0 ) > + return ret; > + if ( nr_vnodes * nr_vnodes > UINT_MAX ) > + return ret;Neither of these are -EFAULT (and I''m pretty certain I commented on the need to use suitable error indicators before). Further, the last check is insufficient: The product can appear to be smaller than UINT_MAX due to truncation. You really need to compare one of the values to the quotient of UINT_MAX and the other.> + /* Everything is good, lets set the number of vnodes */ > + d->vnuma.nr_vnodes = nr_vnodes; > + ret = 0; > +err_dom:Labels indented by one space please (to make diff''s -p option not pick them up as context for a hunk).> + if ( ret != 0 ) > + { > + d->vnuma.nr_vnodes = 0;Now that you (properly) set the field only once everything else was successful, this seems unnecessary.> + case XENMEM_get_vnuma_info: > + { > + vnuma_topology_info_t mtopology; > + struct domain *d; > + > + rc = -EFAULT; > + if ( copy_from_guest(&mtopology, arg, 1) ) > + return -EFAULT; > + if ( (d = rcu_lock_domain_by_any_id(mtopology.domid)) == NULL ) > + return -ESRCH; > + > + if ( (d->vnuma.nr_vnodes == 0) || (d->vnuma.nr_vnodes > d->max_vcpus) ) > + return EOPNOTSUPP;-EOPNOTSUPP. And you need to "goto vnumaout" here.> + > + if ( __copy_to_guest(mtopology.vmemrange, > + d->vnuma.vmemrange, > + d->vnuma.nr_vnodes) != 0 ) > + goto vnumaout; > + if ( __copy_to_guest(mtopology.vdistance, > + d->vnuma.vdistance, > + d->vnuma.nr_vnodes * d->vnuma.nr_vnodes) != 0 ) > + goto vnumaout; > + if ( __copy_to_guest(mtopology.vcpu_to_vnode, > + d->vnuma.vcpu_to_vnode, > + d->max_vcpus) != 0 ) > + goto vnumaout;You can''t use __copy_* without previously having validated the passed in address range (or you risk corrupting hypervisor memory).> + > + if ( __copy_to_guest(mtopology.nr_vnodes, &d->vnuma.nr_vnodes, 1) != 0 )I think you could use __copy_field_to_guest() here.> --- a/xen/include/public/memory.h > +++ b/xen/include/public/memory.h > @@ -459,6 +459,14 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t); > * The zero value is appropiate. > */ > > +/* > + * XENMEM_get_vnuma_info used by caller to retrieve > + * vNUMA topology constructed for particular domain. > + * > + * The data exchanged is presented by vnuma_topology_info. > + */ > +#define XENMEM_get_vnuma_info 25 > + > #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */Isn''t that call supposed to be used by guests? If so, it must not sit in a hypervisor-and-tools-only section.> --- /dev/null > +++ b/xen/include/public/vnuma.h > @@ -0,0 +1,44 @@ > +#ifndef _XEN_PUBLIC_VNUMA_H > +#define _XEN_PUBLIC_VNUMA_H > +#include "memory.h"This seems backwards - if anything, I''d expect memory.h to include vnuma.h (if we need a new header here at all).> +#include "xen.h" > + > +/* > + * Following structures are used to represent vNUMA > + * topology to guest if requested. > + */ > + > +/* > + * Memory ranges can be used to define > + * vNUMA memory node boundaries by the > + * linked list. As of now, only one range > + * per domain is suported. > + */ > + > +struct vmemrange { > + uint64_t start, end; > + struct vmemrange *next;ISTR having commented on this before: No pointers in public headers. Only guest handles are permitted here. Jan
On Fri, Nov 15, 2013 at 3:50 AM, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 14.11.13 at 04:26, Elena Ufimtseva <ufimtseva@gmail.com> wrote: >> + case XEN_DOMCTL_setvnumainfo: >> + { >> + unsigned int i = 0, dist_size; >> + uint nr_vnodes; > > "unsigned int" or "uint" consistently please (preferably the former). > > Also the initializer for "i" seems pointless. > >> + ret = -EFAULT; >> + >> + /* Already set? */ >> + if ( d->vnuma.nr_vnodes > 0 ) >> + return 0; > > This surely needs to be some -E (or else how would the caller know > that the function didn''t do what was asked for). > >> + >> + nr_vnodes = op->u.vnuma.nr_vnodes; >> + >> + if ( nr_vnodes == 0 ) >> + return ret; >> + if ( nr_vnodes * nr_vnodes > UINT_MAX ) >> + return ret; > > Neither of these are -EFAULT (and I''m pretty certain I commented > on the need to use suitable error indicators before). > > Further, the last check is insufficient: The product can appear to > be smaller than UINT_MAX due to truncation. You really need to > compare one of the values to the quotient of UINT_MAX and the > other. > >> + /* Everything is good, lets set the number of vnodes */ >> + d->vnuma.nr_vnodes = nr_vnodes; >> + ret = 0; >> +err_dom: > > Labels indented by one space please (to make diff''s -p option not > pick them up as context for a hunk). > >> + if ( ret != 0 ) >> + { >> + d->vnuma.nr_vnodes = 0; > > Now that you (properly) set the field only once everything else > was successful, this seems unnecessary. > >> + case XENMEM_get_vnuma_info: >> + { >> + vnuma_topology_info_t mtopology; >> + struct domain *d; >> + >> + rc = -EFAULT; >> + if ( copy_from_guest(&mtopology, arg, 1) ) >> + return -EFAULT; >> + if ( (d = rcu_lock_domain_by_any_id(mtopology.domid)) == NULL ) >> + return -ESRCH; >> + >> + if ( (d->vnuma.nr_vnodes == 0) || (d->vnuma.nr_vnodes > d->max_vcpus) ) >> + return EOPNOTSUPP; > > -EOPNOTSUPP. And you need to "goto vnumaout" here. > >> + >> + if ( __copy_to_guest(mtopology.vmemrange, >> + d->vnuma.vmemrange, >> + d->vnuma.nr_vnodes) != 0 ) >> + goto vnumaout; >> + if ( __copy_to_guest(mtopology.vdistance, >> + d->vnuma.vdistance, >> + d->vnuma.nr_vnodes * d->vnuma.nr_vnodes) != 0 ) >> + goto vnumaout; >> + if ( __copy_to_guest(mtopology.vcpu_to_vnode, >> + d->vnuma.vcpu_to_vnode, >> + d->max_vcpus) != 0 ) >> + goto vnumaout; > > You can''t use __copy_* without previously having validated the > passed in address range (or you risk corrupting hypervisor > memory). > >> + >> + if ( __copy_to_guest(mtopology.nr_vnodes, &d->vnuma.nr_vnodes, 1) != 0 ) > > I think you could use __copy_field_to_guest() here. > >> --- a/xen/include/public/memory.h >> +++ b/xen/include/public/memory.h >> @@ -459,6 +459,14 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t); >> * The zero value is appropiate. >> */ >> >> +/* >> + * XENMEM_get_vnuma_info used by caller to retrieve >> + * vNUMA topology constructed for particular domain. >> + * >> + * The data exchanged is presented by vnuma_topology_info. >> + */ >> +#define XENMEM_get_vnuma_info 25 >> + >> #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */ > > Isn''t that call supposed to be used by guests? If so, it must not sit > in a hypervisor-and-tools-only section. > >> --- /dev/null >> +++ b/xen/include/public/vnuma.h >> @@ -0,0 +1,44 @@ >> +#ifndef _XEN_PUBLIC_VNUMA_H >> +#define _XEN_PUBLIC_VNUMA_H >> +#include "memory.h" > > This seems backwards - if anything, I''d expect memory.h to > include vnuma.h (if we need a new header here at all). > >> +#include "xen.h" >> + >> +/* >> + * Following structures are used to represent vNUMA >> + * topology to guest if requested. >> + */ >> + >> +/* >> + * Memory ranges can be used to define >> + * vNUMA memory node boundaries by the >> + * linked list. As of now, only one range >> + * per domain is suported. >> + */ >> + >> +struct vmemrange { >> + uint64_t start, end; >> + struct vmemrange *next; > > ISTR having commented on this before: No pointers in public headers. > Only guest handles are permitted here.Thanks Jan, will work on these.> > Jan-- Elena