Re: [Xen-devel] [PATCH v3 12/19] hvmloader: retrieve vNUMA information from hypervisor

2015-01-13 Thread Wei Liu
On Tue, Jan 13, 2015 at 04:50:11PM +, Jan Beulich wrote:
> >>> On 13.01.15 at 13:11,  wrote:
> > +void init_vnuma_info(void)
> > +{
> > +int rc, retry = 0;
> > +struct xen_vnuma_topology_info vnuma_topo;
> > +
> > +vcpu_to_vnode = scratch_alloc(sizeof(uint32_t) * hvm_info->nr_vcpus, 
> > 0);
> 
> sizeof(*vcpu_to_vnode) please.
> 

Done.

> > +rc = -EAGAIN;
> > +while ( rc == -EAGAIN && retry < 10 )
> 
> What's the justification for 10 here? A sane tool stack shouldn't alter
> the values while starting the domain.
> 

I wasn't sure if a toolstack will change the values whilst domain is
running. But you now confirm that a sane toolstack shouldn't do that I
can just remove this loop.

> > +{
> > +vnuma_topo.domid = DOMID_SELF;
> > +vnuma_topo.pad = 0;
> > +vnuma_topo.nr_vcpus = 0;
> > +vnuma_topo.nr_vnodes = 0;
> > +vnuma_topo.nr_vmemranges = 0;
> > +
> > +set_xen_guest_handle(vnuma_topo.vdistance.h, NULL);
> > +set_xen_guest_handle(vnuma_topo.vcpu_to_vnode.h, NULL);
> > +set_xen_guest_handle(vnuma_topo.vmemrange.h, NULL);
> > +
> > +rc = hypercall_memory_op(XENMEM_get_vnumainfo, &vnuma_topo);
> > +
> > +if ( rc == -EOPNOTSUPP )
> > +return;
> > +
> > +if ( rc != -ENOBUFS )
> > +break;
> > +
> > +ASSERT(vnuma_topo.nr_vcpus == hvm_info->nr_vcpus);
> 
> I also wonder whether we shouldn't make the hypervisor return
> back the (modified) values in the -EAGAIN error case, so that you
> could move above first half of the loop body out of the loop.
> 

I don't think hypercall modifies values in -EAGAIN case. The first half
of that loop is to prepare hypercall structure so that we can retrieve
the new size.

But since you say no sane toolstack should do that this issue becomes
moot.

Wei.

> Jan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel


Re: [Xen-devel] [PATCH v3 12/19] hvmloader: retrieve vNUMA information from hypervisor

2015-01-13 Thread Jan Beulich
>>> On 13.01.15 at 13:11,  wrote:
> +void init_vnuma_info(void)
> +{
> +int rc, retry = 0;
> +struct xen_vnuma_topology_info vnuma_topo;
> +
> +vcpu_to_vnode = scratch_alloc(sizeof(uint32_t) * hvm_info->nr_vcpus, 0);

sizeof(*vcpu_to_vnode) please.

> +rc = -EAGAIN;
> +while ( rc == -EAGAIN && retry < 10 )

What's the justification for 10 here? A sane tool stack shouldn't alter
the values while starting the domain.

> +{
> +vnuma_topo.domid = DOMID_SELF;
> +vnuma_topo.pad = 0;
> +vnuma_topo.nr_vcpus = 0;
> +vnuma_topo.nr_vnodes = 0;
> +vnuma_topo.nr_vmemranges = 0;
> +
> +set_xen_guest_handle(vnuma_topo.vdistance.h, NULL);
> +set_xen_guest_handle(vnuma_topo.vcpu_to_vnode.h, NULL);
> +set_xen_guest_handle(vnuma_topo.vmemrange.h, NULL);
> +
> +rc = hypercall_memory_op(XENMEM_get_vnumainfo, &vnuma_topo);
> +
> +if ( rc == -EOPNOTSUPP )
> +return;
> +
> +if ( rc != -ENOBUFS )
> +break;
> +
> +ASSERT(vnuma_topo.nr_vcpus == hvm_info->nr_vcpus);

I also wonder whether we shouldn't make the hypervisor return
back the (modified) values in the -EAGAIN error case, so that you
could move above first half of the loop body out of the loop.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel


[Xen-devel] [PATCH v3 12/19] hvmloader: retrieve vNUMA information from hypervisor

2015-01-13 Thread Wei Liu
Hvmloader issues XENMEM_get_vnumainfo hypercall and stores the
information retrieved in scratch space for later use.

Signed-off-by: Wei Liu 
Cc: Jan Beulich 
---
Changes in v3:
1. Move init_vnuma_info before ACPI stuff.
2. Fix errno.h inclusion.
3. Remove upper limits and use loop.
---
 tools/firmware/hvmloader/Makefile|2 +-
 tools/firmware/hvmloader/hvmloader.c |3 +
 tools/firmware/hvmloader/vnuma.c |  102 ++
 tools/firmware/hvmloader/vnuma.h |   52 +
 4 files changed, 158 insertions(+), 1 deletion(-)
 create mode 100644 tools/firmware/hvmloader/vnuma.c
 create mode 100644 tools/firmware/hvmloader/vnuma.h

diff --git a/tools/firmware/hvmloader/Makefile 
b/tools/firmware/hvmloader/Makefile
index ef2337b..8bf119d 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -29,7 +29,7 @@ LOADADDR = 0x10
 CFLAGS += $(CFLAGS_xeninclude)
 
 OBJS  = hvmloader.o mp_tables.o util.o smbios.o 
-OBJS += smp.o cacheattr.o xenbus.o
+OBJS += smp.o cacheattr.o xenbus.o vnuma.o
 OBJS += e820.o pci.o pir.o ctype.o
 OBJS += hvm_param.o
 ifeq ($(debug),y)
diff --git a/tools/firmware/hvmloader/hvmloader.c 
b/tools/firmware/hvmloader/hvmloader.c
index 7b0da38..25b7f08 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -26,6 +26,7 @@
 #include "pci_regs.h"
 #include "apic_regs.h"
 #include "acpi/acpi2_0.h"
+#include "vnuma.h"
 #include 
 #include 
 
@@ -310,6 +311,8 @@ int main(void)
 
 if ( acpi_enabled )
 {
+init_vnuma_info();
+
 if ( bios->acpi_build_tables )
 {
 printf("Loading ACPI ...\n");
diff --git a/tools/firmware/hvmloader/vnuma.c b/tools/firmware/hvmloader/vnuma.c
new file mode 100644
index 000..1f3d4a5
--- /dev/null
+++ b/tools/firmware/hvmloader/vnuma.c
@@ -0,0 +1,102 @@
+/*
+ * vnuma.c: obtain vNUMA information from hypervisor
+ *
+ * Copyright (c) 2014 Wei Liu, Citrix Systems (R&D) Ltd.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include "util.h"
+#include "hypercall.h"
+#include "vnuma.h"
+#include "errno.h"
+
+unsigned int nr_vnodes, nr_vmemranges;
+unsigned int *vcpu_to_vnode, *vdistance;
+xen_vmemrange_t *vmemrange;
+
+void init_vnuma_info(void)
+{
+int rc, retry = 0;
+struct xen_vnuma_topology_info vnuma_topo;
+
+vcpu_to_vnode = scratch_alloc(sizeof(uint32_t) * hvm_info->nr_vcpus, 0);
+
+rc = -EAGAIN;
+while ( rc == -EAGAIN && retry < 10 )
+{
+vnuma_topo.domid = DOMID_SELF;
+vnuma_topo.pad = 0;
+vnuma_topo.nr_vcpus = 0;
+vnuma_topo.nr_vnodes = 0;
+vnuma_topo.nr_vmemranges = 0;
+
+set_xen_guest_handle(vnuma_topo.vdistance.h, NULL);
+set_xen_guest_handle(vnuma_topo.vcpu_to_vnode.h, NULL);
+set_xen_guest_handle(vnuma_topo.vmemrange.h, NULL);
+
+rc = hypercall_memory_op(XENMEM_get_vnumainfo, &vnuma_topo);
+
+if ( rc == -EOPNOTSUPP )
+return;
+
+if ( rc != -ENOBUFS )
+break;
+
+ASSERT(vnuma_topo.nr_vcpus == hvm_info->nr_vcpus);
+
+vdistance = scratch_alloc(sizeof(uint32_t) * vnuma_topo.nr_vnodes *
+  vnuma_topo.nr_vnodes, 0);
+vmemrange = scratch_alloc(sizeof(xen_vmemrange_t) *
+  vnuma_topo.nr_vmemranges, 0);
+
+set_xen_guest_handle(vnuma_topo.vdistance.h, vdistance);
+set_xen_guest_handle(vnuma_topo.vcpu_to_vnode.h, vcpu_to_vnode);
+set_xen_guest_handle(vnuma_topo.vmemrange.h, vmemrange);
+
+rc = hypercall_memory_op(XENMEM_get_vnumainfo, &vnuma_topo);
+
+retry++;
+}
+
+if ( rc < 0 )
+{
+printf("Faile