Add support for binding guest numa nodes to host numa nodes

From: Wanlong Gao <[email protected]>

If the total number of the assigned numa nodes memory is not equal to the assigned ram size, it will write the wrong data to ACPI talb, then the guest will ignore the wrong ACPI table

and recognize all memory to one node. It's buggy, we should check it to ensure that we write the right data to ACPI table.

Signed-off-by Wanlong Gao <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

Signed-off-by Hu Tao <[email protected]>

---

From: Wanlong Gao <[email protected]>

Add the numa_info structure to contain the numa nodes memory, VCPUs information and the future added numa nodes host memory policies.

Reviewed-by Eduardo Habkost <[email protected]>

Signed-off-by Andre Przywara <[email protected]>

Signed-off-by Wanlong Gao <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

Signed-off-by Hu Tao <[email protected]>

---
 hw/i386/pc.c            | 12 ++++++++----
 include/sysemu/sysemu.h |  8 ++++++--
 monitor.c               |  2 +-
 numa.c                  | 23 ++++++++++++-----------
 vl.c                    |  7 +++----
 5 files changed, 30 insertions(+), 22 deletions(-)

hw/i386/pc.c +8 -4

index e715a33..a464e48 100644
@@ -674,14 +674,14 @@ static FWCfgState *bochs_bios_init(void)
         unsigned int apic_id = x86_cpu_apic_id_from_index(i);
         assert(apic_id < apic_id_limit);
         for (j = 0; j < nb_numa_nodes; j++) {
-            if (test_bit(i, node_cpumask[j])) {
+            if (test_bit(i, numa_info[j].node_cpu)) {
                 numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);
                 break;
             }
         }
     }
     for (i = 0; i < nb_numa_nodes; i++) {
-        numa_fw_cfg[apic_id_limit + 1 + i] = cpu_to_le64(node_mem[i]);
+        numa_fw_cfg[apic_id_limit + 1 + i] = cpu_to_le64(numa_info[i].node_mem);
     }
     fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg,
                      (1 + apic_id_limit + nb_numa_nodes) *

@@ -1077,8 +1077,12 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
     guest_info->apic_id_limit = pc_apic_id_limit(max_cpus);
     guest_info->apic_xrupt_override = kvm_allows_irq0_override();
     guest_info->numa_nodes = nb_numa_nodes;
-    guest_info->node_mem = g_memdup(node_mem, guest_info->numa_nodes *
+    guest_info->node_mem = g_malloc0(guest_info->numa_nodes *
                                     sizeof *guest_info->node_mem);
+    for (i = 0; i < nb_numa_nodes; i++) {
+        guest_info->node_mem[i] = numa_info[i].node_mem;
+    }
+
     guest_info->node_cpu = g_malloc0(guest_info->apic_id_limit *
                                      sizeof *guest_info->node_cpu);
 

@@ -1086,7 +1090,7 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
         unsigned int apic_id = x86_cpu_apic_id_from_index(i);
         assert(apic_id < guest_info->apic_id_limit);
         for (j = 0; j < nb_numa_nodes; j++) {
-            if (test_bit(i, node_cpumask[j])) {
+            if (test_bit(i, numa_info[j].node_cpu)) {
                 guest_info->node_cpu[apic_id] = j;
                 break;
             }

include/sysemu/sysemu.h +6 -2

index 2509649..d873b42 100644
@@ -9,6 +9,7 @@
 #include "qapi-types.h"
 #include "qemu/notify.h"
 #include "qemu/main-loop.h"
+#include "qemu/bitmap.h"
 
 /* vl.c */
 

@@ -134,8 +135,11 @@ extern QEMUClockType rtc_clock;
 #define MAX_NODES 64
 #define MAX_CPUMASK_BITS 255
 extern int nb_numa_nodes;
-extern uint64_t node_mem[MAX_NODES];
-extern unsigned long *node_cpumask[MAX_NODES];
+typedef struct node_info {
+    uint64_t node_mem;
+    DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
+} NodeInfo;
+extern NodeInfo numa_info[MAX_NODES];
 void numa_add(const char *optarg);
 void set_numa_nodes(void);
 void set_numa_modes(void);

monitor.c +1 -1

index 690c152..0284735 100644
@@ -2004,7 +2004,7 @@ static void do_info_numa(Monitor *mon, const QDict *qdict)
         }
         monitor_printf(mon, "\n");
         monitor_printf(mon, "node %d size: %" PRId64 " MB\n", i,
-            node_mem[i] >> 20);
+            numa_info[i].node_mem >> 20);
     }
 }
 

numa.c +12 -11

index a06e2d1..1f413a0 100644
@@ -61,7 +61,7 @@ static void numa_node_parse_cpus(int nodenr, const char *cpus)
         goto error;
     }
 
-    bitmap_set(node_cpumask[nodenr], value, endvalue-value+1);
+    bitmap_set(numa_info[nodenr].node_cpu, value, endvalue-value+1);
     return;
 
 error:

@@ -101,7 +101,7 @@ void numa_add(const char *optarg)
         }
 
         if (get_param_value(option, 128, "mem", optarg) == 0) {
-            node_mem[nodenr] = 0;
+            numa_info[nodenr].node_mem = 0;
         } else {
             int64_t sval;
             sval = strtosz(option, &endptr);

@@ -109,7 +109,7 @@ void numa_add(const char *optarg)
                 fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
                 exit(1);
             }
-            node_mem[nodenr] = sval;
+            numa_info[nodenr].node_mem = sval;
         }
         if (get_param_value(option, 128, "cpus", optarg) != 0) {
             numa_node_parse_cpus(nodenr, option);

@@ -134,7 +134,7 @@ void set_numa_nodes(void)
          * and distribute the available memory equally across all nodes
          */
         for (i = 0; i < nb_numa_nodes; i++) {
-            if (node_mem[i] != 0) {
+            if (numa_info[i].node_mem != 0) {
                 break;
             }
         }

@@ -145,15 +145,16 @@ void set_numa_nodes(void)
              * the final node gets the rest.
              */
             for (i = 0; i < nb_numa_nodes - 1; i++) {
-                node_mem[i] = (ram_size / nb_numa_nodes) & ~((1 << 23UL) - 1);
-                usedmem += node_mem[i];
+                numa_info[i].node_mem = (ram_size / nb_numa_nodes) &
+                                        ~((1 << 23UL) - 1);
+                usedmem += numa_info[i].node_mem;
             }
-            node_mem[i] = ram_size - usedmem;
+            numa_info[i].node_mem = ram_size - usedmem;
         }
 
         uint64_t numa_total = 0;
         for (i = 0; i < nb_numa_nodes; i++) {
-            numa_total += node_mem[i];
+            numa_total += numa_info[i].node_mem;
         }
         if (numa_total != ram_size) {
             fprintf(stderr, "qemu: numa nodes total memory size "

@@ -162,7 +163,7 @@ void set_numa_nodes(void)
         }
 
         for (i = 0; i < nb_numa_nodes; i++) {
-            if (!bitmap_empty(node_cpumask[i], MAX_CPUMASK_BITS)) {
+            if (!bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
                 break;
             }
         }

@@ -172,7 +173,7 @@ void set_numa_nodes(void)
          */
         if (i == nb_numa_nodes) {
             for (i = 0; i < max_cpus; i++) {
-                set_bit(i, node_cpumask[i % nb_numa_nodes]);
+                set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
             }
         }
     }

@@ -185,7 +186,7 @@ void set_numa_modes(void)
 
     CPU_FOREACH(cpu) {
         for (i = 0; i < nb_numa_nodes; i++) {
-            if (test_bit(cpu->cpu_index, node_cpumask[i])) {
+            if (test_bit(cpu->cpu_index, numa_info[i].node_cpu)) {
                 cpu->numa_node = i;
             }
         }

vl.c +3 -4

index 0adac0c..915f8b7 100644
@@ -196,8 +196,7 @@ static QTAILQ_HEAD(, FWBootEntry) fw_boot_order =
     QTAILQ_HEAD_INITIALIZER(fw_boot_order);
 
 int nb_numa_nodes;
-uint64_t node_mem[MAX_NODES];
-unsigned long *node_cpumask[MAX_NODES];
+NodeInfo numa_info[MAX_NODES];
 
 uint8_t qemu_uuid[16];
 bool qemu_uuid_set;

@@ -2787,8 +2786,8 @@ int main(int argc, char **argv, char **envp)
     translation = BIOS_ATA_TRANSLATION_AUTO;
 
     for (i = 0; i < MAX_NODES; i++) {
-        node_mem[i] = 0;
-        node_cpumask[i] = bitmap_new(MAX_CPUMASK_BITS);
+        numa_info[i].node_mem = 0;
+        bitmap_zero(numa_info[i].node_cpu, MAX_CPUMASK_BITS);
     }
 
     nb_numa_nodes = 0;

Quoted 2 lines · On Wed, 19 Feb 2014 15:53:54 +0800 Hu Tao <[email protected]> wrote:

From: Wanlong Gao <[email protected]>

Quoted 3 lines

Add the numa_info structure to contain the numa nodes memory, VCPUs information and the future added numa nodes host memory policies.

this is old version that breaks spar build which Wanlong already fixed.

You can replace patches 1-5 with more recent ones posted recently:
http://www.mail-archive.com/[email protected]/msg216404.html

From: Igor Mammedov <[email protected]>

Signed-off-by Igor Mammedov <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

Signed-off-by Hu Tao <[email protected]>

---
 qapi/string-input-visitor.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

qapi/string-input-visitor.c +18 -0

index 8f1bc41..a152f5d 100644
@@ -97,6 +97,23 @@ static void parse_type_number(Visitor *v, double *obj, const char *name,
     *obj = val;
 }
 
+static void parse_type_size(Visitor *v, uint64_t *obj, const char *name,
+                            Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    int64_t val;
+    char *endp;
+
+    val = strtosz_suffix(siv->string ? siv->string : "", &endp,
+                         STRTOSZ_DEFSUFFIX_B);
+    if (val < 0 || *endp != '\0') {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, name,
+                  "a size value representible as a non-negative int64");
+        return;
+    }
+    *obj = val;
+}
+
 static void parse_start_optional(Visitor *v, bool *present,
                                  const char *name, Error **errp)
 {

@@ -131,6 +148,7 @@ StringInputVisitor *string_input_visitor_new(const char *str)
     v->visitor.type_bool = parse_type_bool;
     v->visitor.type_str = parse_type_str;
     v->visitor.type_number = parse_type_number;
+    v->visitor.type_size = parse_type_size;
     v->visitor.start_optional = parse_start_optional;
 
     v->string = str;

Provides framework for splitting host RAM allocation/ policies into a separate backend that could be used by devices.

Initially only legacy RAM backend is provided, which uses memory_region_init_ram() allocator and compatible with every CLI option that affects memory_region_init_ram().

Signed-off-by Igor Mammedov <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

Signed-off-by Hu Tao <[email protected]>

Signed-off-by Hu Tao <[email protected]>

---

 backends/Makefile.objs   |   2 +
 backends/hostmem-ram.c   |  48 ++++++++++++++++++
 backends/hostmem.c       | 125 +++++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/hostmem.h |  63 ++++++++++++++++++++++++
 4 files changed, 238 insertions(+)
 create mode 100644 backends/hostmem-ram.c
 create mode 100644 backends/hostmem.c
 create mode 100644 include/sysemu/hostmem.h

backends/Makefile.objs +2 -0

index 42557d5..e6bdc11 100644
@@ -6,3 +6,5 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
 $(obj)/baum.o: QEMU_CFLAGS += $(SDL_CFLAGS) 
 
 common-obj-$(CONFIG_TPM) += tpm.o
+
+common-obj-y += hostmem.o hostmem-ram.o

backends/hostmem-ram.c new file +48 -0

new file mode 100644
index 0000000..a496dbd
@@ -0,0 +1,48 @@
+/*
+ * QEMU Host Memory Backend
+ *
+ * Copyright (C) 2013 Red Hat Inc
+ *
+ * Authors:
+ *   Igor Mammedov <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "sysemu/hostmem.h"
+
+#define TYPE_MEMORY_BACKEND_RAM "memory-ram"
+
+
+static int
+ram_backend_memory_init(HostMemoryBackend *backend, Error **errp)
+{
+    if (!memory_region_size(&backend->mr)) {
+        memory_region_init_ram(&backend->mr, OBJECT(backend),
+                               object_get_canonical_path(OBJECT(backend)),
+                               backend->size);
+    }
+
+    return 0;
+}
+
+static void
+ram_backend_class_init(ObjectClass *oc, void *data)
+{
+    HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
+
+    bc->memory_init = ram_backend_memory_init;
+}
+
+static const TypeInfo ram_backend_info = {
+    .name = TYPE_MEMORY_BACKEND_RAM,
+    .parent = TYPE_MEMORY_BACKEND,
+    .class_init = ram_backend_class_init,
+};
+
+static void register_types(void)
+{
+    type_register_static(&ram_backend_info);
+}
+
+type_init(register_types);

backends/hostmem.c new file +125 -0

new file mode 100644
index 0000000..4b8fd8d
@@ -0,0 +1,125 @@
+/*
+ * QEMU Host Memory Backend
+ *
+ * Copyright (C) 2013 Red Hat Inc
+ *
+ * Authors:
+ *   Igor Mammedov <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "sysemu/hostmem.h"
+#include "sysemu/sysemu.h"
+#include "qapi/visitor.h"
+#include "qapi/qmp/qerror.h"
+#include "qemu/config-file.h"
+#include "qom/object_interfaces.h"
+
+static void
+hostmemory_backend_get_size(Object *obj, Visitor *v, void *opaque,
+                            const char *name, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+    uint64_t value = backend->size;
+
+    visit_type_size(v, &value, name, errp);
+}
+
+static void
+hostmemory_backend_set_size(Object *obj, Visitor *v, void *opaque,
+                            const char *name, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+    uint64_t value;
+
+    if (memory_region_size(&backend->mr)) {
+        error_setg(errp, "cannot change property value\n");
+        return;
+    }
+
+    visit_type_size(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    if (!value) {
+        error_setg(errp, "Property '%s.%s' doesn't take value '%" PRIu64 "'",
+                   object_get_typename(obj), name , value);
+        return;
+    }
+    backend->size = value;
+}
+
+static void hostmemory_backend_initfn(Object *obj)
+{
+    object_property_add(obj, "size", "int",
+                        hostmemory_backend_get_size,
+                        hostmemory_backend_set_size, NULL, NULL, NULL);
+}
+
+static void hostmemory_backend_finalize(Object *obj)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+
+    if (memory_region_size(&backend->mr)) {
+        memory_region_destroy(&backend->mr);
+    }
+}
+
+static int
+hostmemory_backend_memory_init(HostMemoryBackend *backend, Error **errp)
+{
+    error_setg(errp, "memory_init is not implemented for type [%s]",
+               object_get_typename(OBJECT(backend)));
+
+    return -1;
+}
+
+MemoryRegion *
+host_memory_backend_get_memory(HostMemoryBackend *backend, Error **errp)
+{
+    HostMemoryBackendClass *bc = MEMORY_BACKEND_GET_CLASS(backend);
+    Object *obj = OBJECT(backend);
+
+    if (!backend->size) {
+        error_setg(errp, "Invalid property [%s.size] value: %" PRIu64,
+                   object_get_typename(obj), backend->size);
+        return NULL;
+    }
+
+    if (bc->memory_init(backend, errp) < 0) {
+        return NULL;
+    }
+
+    return memory_region_size(&backend->mr) ? &backend->mr : NULL;
+}
+
+static void
+hostmemory_backend_class_init(ObjectClass *oc, void *data)
+{
+    HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
+
+    bc->memory_init = hostmemory_backend_memory_init;
+}
+
+static const TypeInfo hostmemory_backend_info = {
+    .name = TYPE_MEMORY_BACKEND,
+    .parent = TYPE_OBJECT,
+    .abstract = true,
+    .class_size = sizeof(HostMemoryBackendClass),
+    .class_init = hostmemory_backend_class_init,
+    .instance_size = sizeof(HostMemoryBackend),
+    .instance_init = hostmemory_backend_initfn,
+    .instance_finalize = hostmemory_backend_finalize,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
+static void register_types(void)
+{
+    type_register_static(&hostmemory_backend_info);
+}
+
+type_init(register_types);

include/sysemu/hostmem.h new file +63 -0

new file mode 100644
index 0000000..5847943
@@ -0,0 +1,63 @@
+/*
+ * QEMU Host Memory Backend
+ *
+ * Copyright (C) 2013 Red Hat Inc
+ *
+ * Authors:
+ *   Igor Mammedov <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef QEMU_RAM_H
+#define QEMU_RAM_H
+
+#include "qom/object.h"
+#include "qapi/error.h"
+#include "exec/memory.h"
+#include "qemu/option.h"
+
+#define TYPE_MEMORY_BACKEND "memory"
+#define MEMORY_BACKEND(obj) \
+    OBJECT_CHECK(HostMemoryBackend, (obj), TYPE_MEMORY_BACKEND)
+#define MEMORY_BACKEND_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(HostMemoryBackendClass, (obj), TYPE_MEMORY_BACKEND)
+#define MEMORY_BACKEND_CLASS(klass) \
+    OBJECT_CLASS_CHECK(HostMemoryBackendClass, (klass), TYPE_MEMORY_BACKEND)
+
+typedef struct HostMemoryBackend HostMemoryBackend;
+typedef struct HostMemoryBackendClass HostMemoryBackendClass;
+
+/**
+ * HostMemoryBackendClass:
+ * @parent_class: opaque parent class container
+ * @memory_init: hook for derived classes to perform memory allocation
+ */
+struct HostMemoryBackendClass {
+    ObjectClass parent_class;
+
+    int (*memory_init)(HostMemoryBackend *backend, Error **errp);
+};
+
+/**
+ * @HostMemoryBackend
+ *
+ * @parent: opaque parent object container
+ * @size: amount of memory backend provides
+ * @id: unique identification string in memdev namespace
+ * @mr: MemoryRegion representing host memory belonging to backend
+ */
+struct HostMemoryBackend {
+    /* private */
+    Object parent;
+
+    /* protected */
+    uint64_t size;
+
+    MemoryRegion mr;
+};
+
+MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend,
+                                             Error **errp);
+
+#endif

From: Paolo Bonzini <[email protected]>

This option provides the infrastructure for binding guest NUMA nodes to host NUMA nodes. For example:

-object memory-ram,size=1024M,policy=membind,host-nodes=0,id=ram-node0 \
-numa node,nodeid=0,cpus=0,memdev=ram-node0 \
-object memory-ram,size=1024M,policy=interleave,host-nodes=1-3,id=ram-node1 \
-numa node,nodeid=1,cpus=1,memdev=ram-node1

The option replaces "-numa mem".

Signed-off-by Paolo Bonzini <[email protected]>

Conflicts:
include/sysemu/sysemu.h
numa.c

Signed-off-by Hu Tao <[email protected]>

---
 include/sysemu/sysemu.h |  2 ++
 numa.c                  | 64 +++++++++++++++++++++++++++++++++++++++++++++++--
 qapi-schema.json        |  6 ++++-
 3 files changed, 69 insertions(+), 3 deletions(-)

include/sysemu/sysemu.h +2 -0

index e9da760..acfc0c7 100644
@@ -12,6 +12,7 @@
 #include "qemu/bitmap.h"
 #include "qom/object.h"
 #include "hw/boards.h"
+#include "sysemu/hostmem.h"
 
 /* vl.c */
 

@@ -140,6 +141,7 @@ extern int nb_numa_nodes;
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
+    HostMemoryBackend *node_memdev;
 } NodeInfo;
 extern NodeInfo numa_info[MAX_NODES];
 void set_numa_nodes(void);

numa.c +20 -0

index 403b08b..ca55ad7 100644
@@ -27,6 +27,8 @@
 #include "qapi-visit.h"
 #include "qapi/opts-visitor.h"
 #include "qapi/dealloc-visitor.h"
+#include "qapi/qmp/qerror.h"
+
 QemuOptsList qemu_numa_opts = {
     .name = "numa",
     .implied_opt_name = "type",

@@ -34,10 +36,13 @@ QemuOptsList qemu_numa_opts = {
     .desc = { { 0 } } /* validated with OptsVisitor */
 };
 
+static int have_memdevs = -1;
+
 static int numa_node_parse(NumaNodeOptions *opts)
 {
     uint16_t nodenr;
     uint16List *cpus = NULL;
+    Error *local_err = NULL;
 
     if (opts->has_nodeid) {
         nodenr = opts->nodeid;

@@ -60,6 +65,19 @@ static int numa_node_parse(NumaNodeOptions *opts)
         bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
     }
 
+    if (opts->has_mem && opts->has_memdev) {
+        fprintf(stderr, "qemu: cannot specify both mem= and memdev=\n");
+        return -1;
+    }
+
+    if (have_memdevs == -1) {
+        have_memdevs = opts->has_memdev;
+    }
+    if (opts->has_memdev != have_memdevs) {
+        fprintf(stderr, "qemu: memdev option must be specified for either "
+                "all or no nodes\n");
+    }
+
     if (opts->has_mem) {
         int64_t mem_size;
         char *endptr;

@@ -70,7 +88,19 @@ static int numa_node_parse(NumaNodeOptions *opts)
         }
         numa_info[nodenr].node_mem = mem_size;
     }
+    if (opts->has_memdev) {
+        Object *o;

numa.c +33 -2

+    }
     return 0;
 }
 

@@ -189,12 +219,42 @@ void set_numa_modes(void)
     }
 }
 
+static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
+                                           const char *name,
+                                           QEMUMachineInitArgs *args)
+{
+    uint64_t ram_size = args->ram_size;
+
+    memory_region_init_ram(mr, owner, name, ram_size);
+    vmstate_register_ram_global(mr);
+}
+
 void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
                                           const char *name,
                                           QEMUMachineInitArgs *args)
 {
     uint64_t ram_size = args->ram_size;
+    uint64_t addr = 0;
+    int i;
 
-    memory_region_init_ram(mr, owner, name, ram_size);
-    vmstate_register_ram_global(mr);
+    if (nb_numa_nodes == 0 || !have_memdevs) {
+        allocate_system_memory_nonnuma(mr, owner, name, args);
+        return;
+    }
+
+    memory_region_init(mr, owner, name, ram_size);
+    for (i = 0; i < nb_numa_nodes; i++) {
+        Error *local_err = NULL;
+        uint64_t size = numa_info[i].node_mem;
+        HostMemoryBackend *backend = numa_info[i].node_memdev;
+        MemoryRegion *seg = host_memory_backend_get_memory(backend, &local_err);
+        if (local_err) {
+            qerror_report_err(local_err);
+            exit(1);
+        }
+
+        memory_region_add_subregion(mr, addr, seg);
+        vmstate_register_ram_global(seg);
+        addr += size;
+    }
 }

qapi-schema.json +5 -1

index a2839b8..498ea9b 100644
@@ -4441,7 +4441,10 @@
 #
 # @cpus: #optional VCPUs belong to this node
 #
-# @mem: #optional memory size of this node
+# @memdev: #optional memory backend object.  If specified for one node,
+#          it must be specified for all nodes.
+#
+# @mem: #optional memory size of this node; mutually exclusive with @memdev.
 #
 # Since: 2.0
 ##

@@ -4449,4 +4452,5 @@
   'data': {
    '*nodeid': 'uint16',
    '*cpus':   ['uint16'],
+   '*memdev': 'str',
    '*mem':    'str' }}

Quoted 2 lines

numa.c static int numa_node_parse(NumaNodeOptions *opts) · line 102

Quoted 2 lines

numa.c +2 -0

+        numa_info[nodenr].node_mem = object_property_get_int(o, "size", NULL);
+        numa_info[nodenr].node_memdev = MEMORY_BACKEND(o);

if you make numa_info QOM object node_memdev link<> property, then above hunk could be replaced with just setting link. And node_mem could be replaced with readonly property that reads size directly from memdev avoiding data duplication.

As side-effect it numa_info will also become accessible for introspection using QOM interface. Something like:
qom-list /machine/memory-node[X]
qom-get /machine/memory-node[X]/memory_size

From: Paolo Bonzini <[email protected]>

This option provides the infrastructure for binding guest NUMA nodes to host NUMA nodes. For example:

-object memory-ram,size=1024M,policy=membind,host-nodes=0,id=ram-node0 \
-numa node,nodeid=0,cpus=0,memdev=ram-node0 \
-object memory-ram,size=1024M,policy=interleave,host-nodes=1-3,id=ram-node1 \
-numa node,nodeid=1,cpus=1,memdev=ram-node1

The option replaces "-numa mem".

Signed-off-by Paolo Bonzini <[email protected]>

Conflicts:
include/sysemu/sysemu.h
numa.c

Signed-off-by Hu Tao <[email protected]>

---
 include/sysemu/sysemu.h |  2 ++
 numa.c                  | 64 +++++++++++++++++++++++++++++++++++++++++++++++--
 qapi-schema.json        |  6 ++++-
 3 files changed, 69 insertions(+), 3 deletions(-)

Quoted 14 lines · Hu Tao

From: Paolo Bonzini <[email protected]>

This option provides the infrastructure for binding guest NUMA nodes to host NUMA nodes. For example:

-object memory-ram,size=1024M,policy=membind,host-nodes=0,id=ram-node0 \
-numa node,nodeid=0,cpus=0,memdev=ram-node0 \
-object memory-ram,size=1024M,policy=interleave,host-nodes=1-3,id=ram-node1 \
-numa node,nodeid=1,cpus=1,memdev=ram-node1

The option replaces "-numa mem".

Signed-off-by Paolo Bonzini <[email protected]>

Quoted 3 lines

Conflicts:
include/sysemu/sysemu.h
numa.c

Until this patch is taken upstream, a 'Conflicts:' section in your commit message doesn't make sense. That is useful for downstream or stable branch backports, but doesn't belong on the mainline branch.

Cc Laszlo Ersek <[email protected]>

Signed-off-by Hu Tao <[email protected]>

---
 qapi/string-input-visitor.c       | 160 ++++++++++++++++++++++++++++++++++++--
 tests/test-string-input-visitor.c |  22 ++++++
 2 files changed, 176 insertions(+), 6 deletions(-)

qapi/string-input-visitor.c +142 -6

index a152f5d..4540ca3 100644
@@ -15,30 +15,175 @@
 #include "qapi/visitor-impl.h"
 #include "qapi/qmp/qerror.h"
 
+enum ListMode {
+    LM_NONE,             /* not traversing a list of repeated options */
+    LM_STARTED,          /* start_list() succeeded */
+
+    LM_IN_PROGRESS,      /* next_list() has been called.
+                          *
+                          * Generating the next list link will consume the most
+                          * recently parsed QemuOpt instance of the repeated
+                          * option.
+                          *
+                          * Parsing a value into the list link will examine the
+                          * next QemuOpt instance of the repeated option, and
+                          * possibly enter LM_SIGNED_INTERVAL or
+                          * LM_UNSIGNED_INTERVAL.
+                          */
+
+    LM_SIGNED_INTERVAL,  /* next_list() has been called.
+                          *
+                          * Generating the next list link will consume the most
+                          * recently stored element from the signed interval,
+                          * parsed from the most recent QemuOpt instance of the
+                          * repeated option. This may consume QemuOpt itself
+                          * and return to LM_IN_PROGRESS.
+                          *
+                          * Parsing a value into the list link will store the
+                          * next element of the signed interval.
+                          */
+
+    LM_UNSIGNED_INTERVAL,/* Same as above, only for an unsigned interval. */
+
+    LM_END
+};
+
+typedef enum ListMode ListMode;
+
 struct StringInputVisitor
 {
     Visitor visitor;
+
+    ListMode list_mode;
+
+    /* When parsing a list of repeating options as integers, values of the form
+     * "a-b", representing a closed interval, are allowed. Elements in the
+     * range are generated individually.
+     */
+    union {
+        int64_t s;
+        uint64_t u;
+    } range_next, range_limit;
+
     const char *string;
 };
 
+static void
+start_list(Visitor *v, const char *name, Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+
+    /* we can't traverse a list in a list */
+    assert(siv->list_mode == LM_NONE);
+    siv->list_mode = LM_STARTED;
+}
+
+static GenericList *
+next_list(Visitor *v, GenericList **list, Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    GenericList **link;
+
+    switch (siv->list_mode) {
+    case LM_STARTED:
+        siv->list_mode = LM_IN_PROGRESS;
+        link = list;
+        break;
+
+    case LM_SIGNED_INTERVAL:
+    case LM_UNSIGNED_INTERVAL:
+        link = &(*list)->next;
+
+        if (siv->list_mode == LM_SIGNED_INTERVAL) {
+            if (siv->range_next.s < siv->range_limit.s) {
+                ++siv->range_next.s;
+                break;
+            }
+        } else if (siv->range_next.u < siv->range_limit.u) {
+            ++siv->range_next.u;
+            break;
+        }
+        siv->list_mode = LM_END;
+        /* range has been completed, fall through */
+
+    case LM_END:
+        return NULL;
+
+    case LM_IN_PROGRESS:
+        link = &(*list)->next;
+        break;
+
+    default:
+        abort();
+    }
+
+    *link = g_malloc0(sizeof **link);
+    return *link;
+}
+
+static void
+end_list(Visitor *v, Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+
+    assert(siv->list_mode == LM_STARTED ||
+           siv->list_mode == LM_END ||
+           siv->list_mode == LM_IN_PROGRESS ||
+           siv->list_mode == LM_SIGNED_INTERVAL ||
+           siv->list_mode == LM_UNSIGNED_INTERVAL);
+    siv->list_mode = LM_NONE;
+}
+
 static void parse_type_int(Visitor *v, int64_t *obj, const char *name,
                            Error **errp)
 {
     StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
-    char *endp = (char *) siv->string;
+    char *str = (char *) siv->string;
     long long val;
+    char *endptr;
 
-    errno = 0;
-    if (siv->string) {
-        val = strtoll(siv->string, &endp, 0);
+    if (siv->list_mode == LM_SIGNED_INTERVAL) {
+        *obj = siv->range_next.s;
+        return;
     }
-    if (!siv->string || errno || endp == siv->string || *endp) {
+
+    if (!siv->string) {
         error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                   "integer");
         return;
     }
 
-    *obj = val;
+    errno = 0;
+    val = strtoll(siv->string, &endptr, 0);
+
+    if (errno == 0 && endptr > str && INT64_MIN <= val && val <= INT64_MAX) {
+        if (*endptr == '\0') {
+            *obj = val;
+            siv->list_mode = LM_END;
+            return;
+        }
+        if (*endptr == '-' && siv->list_mode == LM_IN_PROGRESS) {
+            long long val2;
+
+            str = endptr + 1;
+            val2 = strtoll(str, &endptr, 0);
+            if (errno == 0 && endptr > str && *endptr == '\0' &&
+                INT64_MIN <= val2 && val2 <= INT64_MAX && val <= val2 &&
+                (val > INT64_MAX - 65536 ||
+                 val2 < val + 65536)) {
+                siv->range_next.s = val;
+                siv->range_limit.s = val2;
+                siv->list_mode = LM_SIGNED_INTERVAL;
+

Two problems:

1. the code is mostly copied from OptsVisitor. maybe we can share the code?

2. int list is not implemented in string outout visitor. but there is currently no user of it. Should we implement it or not?

From: Wanlong Gao <[email protected]>

This new enum type will be used to set host memory policy of backend host memory.

Signed-off-by Wanlong Gao <[email protected]>

Signed-off-by Hu Tao <[email protected]>

---
 qapi-schema.json | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

qapi-schema.json +0 -0

index 498ea9b..9d6370f 100644
@@ -4454,3 +4454,23 @@
    '*cpus':   ['uint16'],
    '*memdev': 'str',

From: Wanlong Gao <[email protected]>

This new enum type will be used to set host memory policy of backend host memory.

Signed-off-by Wanlong Gao <[email protected]>

Signed-off-by Hu Tao <[email protected]>

---
 qapi-schema.json | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

qapi-schema.json +20 -0

index 498ea9b..9d6370f 100644
@@ -4454,3 +4454,23 @@
    '*cpus':   ['uint16'],
    '*memdev': 'str',
    '*mem':    'str' }}
+
+##
+# @HostMemPolicy
+#
+# Host memory policy types
+#
+# @default: restore default policy, remove any nondefault policy
+#
+# @preferred: set the preferred host node for allocation
+#
+# @membind: a strict policy that restricts memory allocation to the
+#           host nodes specified
+#
+# @interleave: the page allocations is interleaved across the set
+#              of host nodes specified
+#
+# Since 2.0
+##
+{ 'enum': 'HostMemPolicy',
+  'data': [ 'default', 'preferred', 'membind', 'interleave' ] }

Thus makes user control how to allocate memory for ram backend.

Signed-off-by Hu Tao <[email protected]>

---
 backends/hostmem-ram.c  | 158 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/sysemu.h |   2 +
 2 files changed, 160 insertions(+)

backends/hostmem-ram.c +70 -0

index a496dbd..2da9341 100644
@@ -10,23 +10,179 @@
  * See the COPYING file in the top-level directory.
  */
 #include "sysemu/hostmem.h"
+#include "sysemu/sysemu.h"
+#include "qemu/bitmap.h"
+#include "qapi-visit.h"
+#include "qemu/config-file.h"
+#include "qapi/opts-visitor.h"
 
 #define TYPE_MEMORY_BACKEND_RAM "memory-ram"
+#define MEMORY_BACKEND_RAM(obj) \
+    OBJECT_CHECK(HostMemoryBackendRam, (obj), TYPE_MEMORY_BACKEND_RAM)
 
+typedef struct HostMemoryBackendRam HostMemoryBackendRam;
+
+/**
+ * @HostMemoryBackendRam
+ *
+ * @parent: opaque parent object container
+ * @host_nodes: host nodes bitmap used for memory policy
+ * @policy: host memory policy
+ * @relative: if the host nodes bitmap is relative
+ */
+struct HostMemoryBackendRam {
+    /* private */
+    HostMemoryBackend parent;
+
+    DECLARE_BITMAP(host_nodes, MAX_NODES);
+    HostMemPolicy policy;
+    bool relative;
+};
+
+static void
+get_host_nodes(Object *obj, Visitor *v, void *opaque, const char *name,
+               Error **errp)
+{
+    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(obj);
+    uint16List *host_nodes = NULL;
+    uint16List **node = &host_nodes;
+    unsigned long value;
+
+    value = find_first_bit(ram_backend->host_nodes, MAX_NODES);
+    if (value == MAX_NODES) {
+        return;
+    }
+
+    *node = g_malloc0(sizeof(**node));
+    (*node)->value = value;
+    node = &(*node)->next;
+
+    do {
+        value = find_next_bit(ram_backend->host_nodes, MAX_NODES, value + 1);
+        if (value == MAX_NODES) {
+            break;
+        }
+
+        *node = g_malloc0(sizeof(**node));
+        (*node)->value = value;
+        node = &(*node)->next;
+    } while (true);
+
+    visit_type_uint16List(v, &host_nodes, name, errp);
+}
+
+static void
+set_host_nodes(Object *obj, Visitor *v, void *opaque, const char *name,
+               Error **errp)
+{
+    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(obj);
+    uint16List *l = NULL;
+
+    visit_type_uint16List(v, &l, name, errp);
+
+    while (l) {
+        bitmap_set(ram_backend->host_nodes, l->value, 1);
+        l = l->next;

backends/hostmem-ram.c +9 -0

+static void
+get_policy(Object *obj, Visitor *v, void *opaque, const char *name,
+           Error **errp)
+{
+    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(obj);
+    int policy = ram_backend->policy;
+
+    visit_type_enum(v, &policy, policies, NULL, name, errp);
+}

backends/hostmem-ram.c +6 -0

+}
+
+
+static bool get_relative(Object *obj, Error **errp)
+{
+    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(obj);

backends/hostmem-ram.c +13 -0

+#include <sys/syscall.h>
+#ifndef MPOL_F_RELATIVE_NODES
+#define MPOL_F_RELATIVE_NODES (1 << 14)
+#define MPOL_F_STATIC_NODES   (1 << 15)
+#endif
 
 static int
 ram_backend_memory_init(HostMemoryBackend *backend, Error **errp)
 {
+    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(backend);
+    int mode = ram_backend->policy;
+    void *p;
+    unsigned long maxnode;
+
     if (!memory_region_size(&backend->mr)) {
         memory_region_init_ram(&backend->mr, OBJECT(backend),
                                object_get_canonical_path(OBJECT(backend)),
                                backend->size);
+
+        p = memory_region_get_ram_ptr(&backend->mr);
+        maxnode = find_last_bit(ram_backend->host_nodes, MAX_NODES);

backends/hostmem-ram.c +2 -0

+            return -1;
+        }
     }
 

backends/hostmem-ram.c +9 -0

+                        get_policy,
+                        set_policy, NULL, NULL, NULL);
+    object_property_add_bool(obj, "relative",
+                             get_relative, set_relative, NULL);
+}
+
+static void
 ram_backend_class_init(ObjectClass *oc, void *data)
 {
     HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);

@@ -38,6 +194,8 @@ static const TypeInfo ram_backend_info = {
     .name = TYPE_MEMORY_BACKEND_RAM,
     .parent = TYPE_MEMORY_BACKEND,
     .class_init = ram_backend_class_init,
+    .instance_size = sizeof(HostMemoryBackendRam),
+    .instance_init = ram_backend_initfn,
 };
 
 static void register_types(void)

include/sysemu/sysemu.h +0 -0

index acfc0c7..a3d8c02 100644

include/sysemu/sysemu.h +0 -0

 #define MAX_OPTION_ROMS 16
 typedef struct QEMUOptionRom {
     const char *name;

19/02/2014 08:54, Hu Tao ha scritto:

Quoted 9 lines

Thus makes user control how to allocate memory for ram backend.

Signed-off-by Hu Tao <[email protected]>

---
 backends/hostmem-ram.c  | 158 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/sysemu.h |   2 +
 2 files changed, 160 insertions(+)

backends/hostmem-ram.c +0 -0

Quoted 15 lines

static int
ram_backend_memory_init(HostMemoryBackend *backend, Error **errp) {

backends/hostmem-ram.c +8 -0

+    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(backend);
+    int mode = ram_backend->policy;
+    void *p;
+    unsigned long maxnode;
+
     if (!memory_region_size(&backend->mr)) {
         memory_region_init_ram(&backend->mr, OBJECT(backend),
                                object_get_canonical_path(OBJECT(backend)),
                                backend->size);
+
+        p = memory_region_get_ram_ptr(&backend->mr);
+        maxnode = find_last_bit(ram_backend->host_nodes, MAX_NODES);

backends/hostmem-ram.c line 164

Quoted 10 lines

backends/hostmem-ram.c +10 -0

+
+        mode |= ram_backend->relative ? MPOL_F_RELATIVE_NODES :
+            MPOL_F_STATIC_NODES;
+        /* This is a workaround for a long standing bug in Linux'
+         * mbind implementation, which cuts off the last specified
+         * node. To stay compatible should this bug be fixed, we
+         * specify one more node and zero this one out.
+         */
+        if (syscall(SYS_mbind, p, backend->size, mode,
+                    ram_backend->host_nodes, maxnode + 2, 0)) {

This does not compile on non-Linux; also, does libnuma include the workaround? If so, this is a hint that we should be using libnuma instead...

Finally, all this code should be in hostmem.c, not hostmem-ram.c, because the same policies can be applied to hugepage-backed memory.

Currently host_memory_backend_get_memory is calling bc->memory_init. Probably the call should be replaced by something like

static void
host_memory_backend_alloc(HostMemoryBackend *backend, Error **errp)

{
     Error *local_err = NULL;
     bc->memory_init(backend, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         return;
     }

     ... set policy ...
}

...

     Error *local_err = NULL;
     host_memory_backend_alloc(backend, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         return NULL;
     }

     assert(memory_region_size(&backend->mr) != 0);
     return &backend->mr;
}

Quoted 10 lines · On Wed, 19 Feb 2014 10:03:13 +0100 Paolo Bonzini <[email protected]> wrote:

19/02/2014 08:54, Hu Tao ha scritto:

Quoted 9 lines

Thus makes user control how to allocate memory for ram backend.

Signed-off-by Hu Tao <[email protected]>

---
 backends/hostmem-ram.c  | 158 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/sysemu.h |   2 +
 2 files changed, 160 insertions(+)

backends/hostmem-ram.c +0 -0

[...]

Quoted 33 lines

Quoted 15 lines

static int
ram_backend_memory_init(HostMemoryBackend *backend, Error **errp) {

backends/hostmem-ram.c +8 -0

+    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(backend);
+    int mode = ram_backend->policy;
+    void *p;
+    unsigned long maxnode;
+
     if (!memory_region_size(&backend->mr)) {
         memory_region_init_ram(&backend->mr, OBJECT(backend),
                                object_get_canonical_path(OBJECT(backend)),
                                backend->size);
+
+        p = memory_region_get_ram_ptr(&backend->mr);
+        maxnode = find_last_bit(ram_backend->host_nodes, MAX_NODES);

backends/hostmem-ram.c line 164

Quoted 10 lines

backends/hostmem-ram.c +10 -0

+
+        mode |= ram_backend->relative ? MPOL_F_RELATIVE_NODES :
+            MPOL_F_STATIC_NODES;
+        /* This is a workaround for a long standing bug in Linux'
+         * mbind implementation, which cuts off the last specified
+         * node. To stay compatible should this bug be fixed, we
+         * specify one more node and zero this one out.
+         */
+        if (syscall(SYS_mbind, p, backend->size, mode,
+                    ram_backend->host_nodes, maxnode + 2, 0)) {

This does not compile on non-Linux; also, does libnuma include the workaround? If so, this is a hint that we should be using libnuma instead...

Finally, all this code should be in hostmem.c, not hostmem-ram.c, because the same policies can be applied to hugepage-backed memory.

Quoted 2 lines

Currently host_memory_backend_get_memory is calling bc->memory_init. Probably the call should be replaced by something like

I've pushed to github updated version of memdev, where host_memory_backend_get_memory() is just convenience wrapper to get access to memdev's internal MemoryRegion.

All initialization now is done in user_creatable->complete() method which calls ram_backend_memory_init() so leaving it as is should be fine.

There is a problem that user_creatable_complete() is called before adding object to "/objects" (see object_create()), which triggers a assert failure when calling object_get_canonical_path() in ram_backend_memory_init(). Any ideas?

Il 25/02/2014 11:20, Hu Tao ha scritto:

Quoted 4 lines

There is a problem that user_creatable_complete() is called before adding object to "/objects" (see object_create()), which triggers a assert failure when calling object_get_canonical_path() in ram_backend_memory_init(). Any ideas?

You can call object_property_add_child before calling user_creatable_complete, and then call object_unparent (in addition to object_unref) if creation fails.

Paolo

From 59c999c840e4305bb2b95389bbea32e07c1c14c0 Mon Sep 17 00:00:00 2001

Quoted 11 lines · Paolo Bonzini

Il 25/02/2014 11:20, Hu Tao ha scritto:

Quoted 4 lines

There is a problem that user_creatable_complete() is called before adding object to "/objects" (see object_create()), which triggers a assert failure when calling object_get_canonical_path() in ram_backend_memory_init(). Any ideas?

You can call object_property_add_child before calling user_creatable_complete, and then call object_unparent (in addition to object_unref) if creation fails.

Paolo

Something like this?

From 59c999c840e4305bb2b95389bbea32e07c1c14c0 Mon Sep 17 00:00:00 2001
From: Hu Tao <[email protected]>
Date: Wed, 26 Feb 2014 12:54:34 +0800
Subject: [PATCH] call user_creatable_complete() after adding object to
"/objects"

This makes it possible to get the path of object by calling object_get_canonical_path() in the complete callback if someone wants it.

Signed-off-by Hu Tao <[email protected]>

---
 vl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

vl.c +3 -0

index 1d27b34..30b4297 100644
@@ -2770,14 +2770,15 @@ static int object_create(QemuOpts *opts, void *opaque)
         goto out;
     }
 
+    object_property_add_child(container_get(object_get_root(), "/objects"),
+                              id, obj, &local_err);
+
     user_creatable_complete(obj, &local_err);
     if (local_err) {

Quoted 47 lines · On Wed, 26 Feb 2014 13:00:03 +0800 Hu Tao <[email protected]> wrote:

Quoted 11 lines · Paolo Bonzini

Il 25/02/2014 11:20, Hu Tao ha scritto:

Quoted 4 lines

There is a problem that user_creatable_complete() is called before adding object to "/objects" (see object_create()), which triggers a assert failure when calling object_get_canonical_path() in ram_backend_memory_init(). Any ideas?

You can call object_property_add_child before calling user_creatable_complete, and then call object_unparent (in addition to object_unref) if creation fails.

Paolo

Something like this?

From 59c999c840e4305bb2b95389bbea32e07c1c14c0 Mon Sep 17 00:00:00 2001
From: Hu Tao <[email protected]>
Date: Wed, 26 Feb 2014 12:54:34 +0800
Subject: [PATCH] call user_creatable_complete() after adding object to
"/objects"

This makes it possible to get the path of object by calling object_get_canonical_path() in the complete callback if someone wants it.

Signed-off-by Hu Tao <[email protected]>

---
 vl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

vl.c +4 -0

index 1d27b34..30b4297 100644
@@ -2770,14 +2770,15 @@ static int object_create(QemuOpts *opts, void *opaque)
         goto out;
     }
 
+    object_property_add_child(container_get(object_get_root(), "/objects"),
+                              id, obj, &local_err);
+
     user_creatable_complete(obj, &local_err);
     if (local_err) {
+        object_unparent(obj);
         goto out;
     }
 

vl.c static int object_create(QemuOpts *opts, void *opaque) · line 2784

Quoted 6 lines

vl.c +0 -3

-    object_property_add_child(container_get(object_get_root(), "/objects"),
-                              id, obj, &local_err);
-
 out:
     object_unref(obj);
     if (local_err) {

Failure case is not handled properly,
I'm sorry that I've forgotten to mention prerequisite path
https://github.com/imammedo/qemu/commit/72a079b88e055fc690c8895a99ccbcce36f6fc1f
in stage tree https://github.com/imammedo/qemu/commits/memory-hotplug-v8

Quoted 50 lines · Igor Mammedov

Quoted 47 lines · On Wed, 26 Feb 2014 13:00:03 +0800 Hu Tao <[email protected]> wrote:

Quoted 11 lines · Paolo Bonzini

Il 25/02/2014 11:20, Hu Tao ha scritto:

Quoted 4 lines

There is a problem that user_creatable_complete() is called before adding object to "/objects" (see object_create()), which triggers a assert failure when calling object_get_canonical_path() in ram_backend_memory_init(). Any ideas?

You can call object_property_add_child before calling user_creatable_complete, and then call object_unparent (in addition to object_unref) if creation fails.

Paolo

Something like this?

From 59c999c840e4305bb2b95389bbea32e07c1c14c0 Mon Sep 17 00:00:00 2001
From: Hu Tao <[email protected]>
Date: Wed, 26 Feb 2014 12:54:34 +0800
Subject: [PATCH] call user_creatable_complete() after adding object to
"/objects"

This makes it possible to get the path of object by calling object_get_canonical_path() in the complete callback if someone wants it.

Signed-off-by Hu Tao <[email protected]>

---
 vl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

vl.c +4 -0

index 1d27b34..30b4297 100644
@@ -2770,14 +2770,15 @@ static int object_create(QemuOpts *opts, void *opaque)
         goto out;
     }
 
+    object_property_add_child(container_get(object_get_root(), "/objects"),
+                              id, obj, &local_err);
+
     user_creatable_complete(obj, &local_err);
     if (local_err) {
+        object_unparent(obj);
         goto out;
     }
 

Quoted 10 lines

vl.c static int object_create(QemuOpts *opts, void *opaque) · line 2784

Quoted 6 lines

vl.c +0 -3

-    object_property_add_child(container_get(object_get_root(), "/objects"),
-                              id, obj, &local_err);
-
 out:
     object_unref(obj);
     if (local_err) {

Failure case is not handled properly,
I'm sorry that I've forgotten to mention prerequisite path
https://github.com/imammedo/qemu/commit/72a079b88e055fc690c8895a99ccbcce36f6fc1f
in stage tree https://github.com/imammedo/qemu/commits/memory-hotplug-v8

No problem. I should've noticed the patch when cherry-picking. Will you post it seperately?

I'm sorry that I've forgotten to mention prerequisite path
https://github.com/imammedo/qemu/commit/72a079b88e055fc690c8895a99ccbcce36f6fc1f
in stage tree https://github.com/imammedo/qemu/commits/memory-hotplug-v8

Quoted 10 lines · On Wed, 19 Feb 2014 10:03:13 +0100 Paolo Bonzini <[email protected]> wrote:

19/02/2014 08:54, Hu Tao ha scritto:

Quoted 9 lines

Thus makes user control how to allocate memory for ram backend.

Signed-off-by Hu Tao <[email protected]>

---
 backends/hostmem-ram.c  | 158 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/sysemu.h |   2 +
 2 files changed, 160 insertions(+)

backends/hostmem-ram.c +0 -0

[...]

Quoted 33 lines

Quoted 15 lines

static int
ram_backend_memory_init(HostMemoryBackend *backend, Error **errp) {

backends/hostmem-ram.c +8 -0

+    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(backend);
+    int mode = ram_backend->policy;
+    void *p;
+    unsigned long maxnode;
+
     if (!memory_region_size(&backend->mr)) {
         memory_region_init_ram(&backend->mr, OBJECT(backend),
                                object_get_canonical_path(OBJECT(backend)),
                                backend->size);
+
+        p = memory_region_get_ram_ptr(&backend->mr);
+        maxnode = find_last_bit(ram_backend->host_nodes, MAX_NODES);

backends/hostmem-ram.c line 164

Quoted 10 lines

backends/hostmem-ram.c +10 -0

+
+        mode |= ram_backend->relative ? MPOL_F_RELATIVE_NODES :
+            MPOL_F_STATIC_NODES;
+        /* This is a workaround for a long standing bug in Linux'
+         * mbind implementation, which cuts off the last specified
+         * node. To stay compatible should this bug be fixed, we
+         * specify one more node and zero this one out.
+         */
+        if (syscall(SYS_mbind, p, backend->size, mode,
+                    ram_backend->host_nodes, maxnode + 2, 0)) {

This does not compile on non-Linux; also, does libnuma include the workaround? If so, this is a hint that we should be using libnuma instead...

Finally, all this code should be in hostmem.c, not hostmem-ram.c, because the same policies can be applied to hugepage-backed memory.

Quoted 2 lines

Currently host_memory_backend_get_memory is calling bc->memory_init. Probably the call should be replaced by something like

I've pushed to github updated version of memdev, where host_memory_backend_get_memory() is just convenience wrapper to get access to memdev's internal MemoryRegion.

All initialization now is done in user_creatable->complete() method which calls ram_backend_memory_init() so leaving it as is should be fine.

If lines about memory polices are moved up to hostmem.c, the only thing left in ram_backend_memory_init() is calling memory_region_init_ram() to allocate memory. Then it comes a problem that when to apply memory polices? Choices:

1. apply memory polices in hostmem.c since this is the place user sets memory polices. But user_creatable_complete() seems not to support this.( but fix me)

2. cast to HostMemoryBackend in ram_backend_memory_init() (or in other memory backends) and add lines to apply memory polices.

3. provide an interface in HostMemoryBackendClass to do the thing and call it in subclasses. (this is basically the same as 2 except that we can reuse code)

Quoted 48 lines · Igor Mammedov

Quoted 10 lines · On Wed, 19 Feb 2014 10:03:13 +0100 Paolo Bonzini <[email protected]> wrote:

19/02/2014 08:54, Hu Tao ha scritto:

Quoted 9 lines

Thus makes user control how to allocate memory for ram backend.

Signed-off-by Hu Tao <[email protected]>

---
 backends/hostmem-ram.c  | 158 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/sysemu.h |   2 +
 2 files changed, 160 insertions(+)

backends/hostmem-ram.c +0 -0

[...]

Quoted 33 lines

Quoted 15 lines

static int
ram_backend_memory_init(HostMemoryBackend *backend, Error **errp) {

backends/hostmem-ram.c +8 -0

+    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(backend);
+    int mode = ram_backend->policy;
+    void *p;
+    unsigned long maxnode;
+
     if (!memory_region_size(&backend->mr)) {
         memory_region_init_ram(&backend->mr, OBJECT(backend),
                                object_get_canonical_path(OBJECT(backend)),
                                backend->size);
+
+        p = memory_region_get_ram_ptr(&backend->mr);
+        maxnode = find_last_bit(ram_backend->host_nodes, MAX_NODES);

backends/hostmem-ram.c line 164

Quoted 10 lines

backends/hostmem-ram.c +10 -0

+
+        mode |= ram_backend->relative ? MPOL_F_RELATIVE_NODES :
+            MPOL_F_STATIC_NODES;
+        /* This is a workaround for a long standing bug in Linux'
+         * mbind implementation, which cuts off the last specified
+         * node. To stay compatible should this bug be fixed, we
+         * specify one more node and zero this one out.
+         */
+        if (syscall(SYS_mbind, p, backend->size, mode,
+                    ram_backend->host_nodes, maxnode + 2, 0)) {

This does not compile on non-Linux; also, does libnuma include the workaround? If so, this is a hint that we should be using libnuma instead...

Finally, all this code should be in hostmem.c, not hostmem-ram.c, because the same policies can be applied to hugepage-backed memory.

Quoted 8 lines

Quoted 2 lines

Currently host_memory_backend_get_memory is calling bc->memory_init. Probably the call should be replaced by something like

I've pushed to github updated version of memdev, where host_memory_backend_get_memory() is just convenience wrapper to get access to memdev's internal MemoryRegion.

All initialization now is done in user_creatable->complete() method which calls ram_backend_memory_init() so leaving it as is should be fine.

If lines about memory polices are moved up to hostmem.c, the only thing left in ram_backend_memory_init() is calling memory_region_init_ram() to allocate memory. Then it comes a problem that when to apply memory polices? Choices:

1. apply memory polices in hostmem.c since this is the place user sets memory polices. But user_creatable_complete() seems not to support this.( but fix me)

2. cast to HostMemoryBackend in ram_backend_memory_init() (or in other memory backends) and add lines to apply memory polices.

3. provide an interface in HostMemoryBackendClass to do the thing and call it in subclasses. (this is basically the same as 2 except that we can reuse code)

Opinions?

if we assume that NUMA policies apply to every hostmem derived backend, then we could realize() approach used by DEVICE. i.e.
set NUMA policies in hostmem.c:hostmemory_backend_memory_init()

Add parent_complete field to ram-backend class and store there parent's complete pointer. Then we can do:

ram_backend_memory_init(UserCreatable *uc, Error **errp) {
    memory_region_init_ram();
    ...
    MEMORY_BACKEND_RAM_CLASS(uc)->parent_complete(uc, errp);
    ...
}

Il 26/02/2014 10:10, Igor Mammedov ha scritto:

Quoted 7 lines

if we assume that NUMA policies apply to every hostmem derived backend, then we could realize() approach used by DEVICE. i.e.
set NUMA policies in hostmem.c:hostmemory_backend_memory_init()

Add parent_complete field to ram-backend class and store there parent's complete pointer. Then we can do:

Quoted 7 lines

ram_backend_memory_init(UserCreatable *uc, Error **errp) {
    memory_region_init_ram();
    ...
    MEMORY_BACKEND_RAM_CLASS(uc)->parent_complete(uc, errp);
    ...
}

The problem is that some backends might not be handled the same way. For example, not all backends might produce a single void*/size_t pair for the entire region. Think of a "composite" backend that produces a large memory region from two smaller ones.

Paolo

Quoted 4 lines

Quoted 4 lines

The problem is that some backends might not be handled the same way. For example, not all backends might produce a single void*/size_t pair for the entire region. Think of a "composite" backend that produces a large memory region from two smaller ones.

I'd prefer to keep backends simple, with 1:1 mapping to memory regions.

Is there a need in composite one or something similar?

Il 26/02/2014 13:31, Igor Mammedov ha scritto:

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

The problem is that some backends might not be handled the same way. For example, not all backends might produce a single void*/size_t pair for the entire region. Think of a "composite" backend that produces a large memory region from two smaller ones.

I'd prefer to keep backends simple, with 1:1 mapping to memory regions.

I agree. However not all backends may have a mapping to a RAM memory region. A composite backend could create a container memory region whose children are other HostMemoryBackend objects.

Quoted 1 line

Is there a need in composite one or something similar?

I've heard of users that want a node backed partially by hugetlbfs and partially by regular RAM. Not sure why.

Paolo

Quoted 10 lines

Quoted 1 line

I'd prefer to keep backends simple, with 1:1 mapping to memory regions.

I agree. However not all backends may have a mapping to a RAM memory region. A composite backend could create a container memory region whose children are other HostMemoryBackend objects.

Quoted 1 line

Is there a need in composite one or something similar?

I've heard of users that want a node backed partially by hugetlbfs and partially by regular RAM. Not sure why.

To overcommit the non hugetlbfs backed guest RAM (think guest pagecache on that non hugetlbfs backed memory, swappable and KSM-able).

The problem is, you have to in someway guarantee the guest allocates 1GB pages out of the hugetlb backed GPA ranges. Some thoughts (honestly, dislike all of them):

1) Boot guest with hugepages, allocate hugepages in guest, later on hotplug 4K backed ranges. HV-unaware reboot might fail, though.

2) Communicate hugepage GPAs to guest.

3) Create holes in non hugepage backed GPA range.

Il 26/02/2014 13:31, Igor Mammedov ha scritto:

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

The problem is that some backends might not be handled the same way. For example, not all backends might produce a single void*/size_t pair for the entire region. Think of a "composite" backend that produces a large memory region from two smaller ones.

I'd prefer to keep backends simple, with 1:1 mapping to memory regions.

I agree. However not all backends may have a mapping to a RAM memory region. A composite backend could create a container memory region whose children are other HostMemoryBackend objects.

Quoted 1 line

Is there a need in composite one or something similar?

I've heard of users that want a node backed partially by hugetlbfs and partially by regular RAM. Not sure why.

Paolo

Quoted 7 lines · On Wed, 26 Feb 2014 13:45:38 +0100 Paolo Bonzini <[email protected]> wrote:

Il 26/02/2014 13:31, Igor Mammedov ha scritto:

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

The problem is that some backends might not be handled the same way. For example, not all backends might produce a single void*/size_t pair for the entire region. Think of a "composite" backend that produces a large memory region from two smaller ones.

I'd prefer to keep backends simple, with 1:1 mapping to memory regions.

Quoted 8 lines

I agree. However not all backends may have a mapping to a RAM memory region. A composite backend could create a container memory region whose children are other HostMemoryBackend objects.

Quoted 1 line

Is there a need in composite one or something similar?

I've heard of users that want a node backed partially by hugetlbfs and partially by regular RAM. Not sure why.

Isn't issue here in how backend is mapped into GPA? Well that is not backend's job.

Once one starts to put layout (alignment, noncontinuously mapped memory regions inside of container, ...), mapping HPA->GPA gets complicated.

It would be better to use simple building blocks and model as:
2 separate backends (ram + hugetlbfs) and 2 corresponding DIMM devices.

Il 26/02/2014 14:43, Igor Mammedov ha scritto:

Quoted 18 lines

Quoted 7 lines · On Wed, 26 Feb 2014 13:45:38 +0100 Paolo Bonzini <[email protected]> wrote:

Il 26/02/2014 13:31, Igor Mammedov ha scritto:

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

The problem is that some backends might not be handled the same way. For example, not all backends might produce a single void*/size_t pair for the entire region. Think of a "composite" backend that produces a large memory region from two smaller ones.

I'd prefer to keep backends simple, with 1:1 mapping to memory regions.

Quoted 8 lines

I agree. However not all backends may have a mapping to a RAM memory region. A composite backend could create a container memory region whose children are other HostMemoryBackend objects.

Quoted 1 line

Is there a need in composite one or something similar?

I've heard of users that want a node backed partially by hugetlbfs and partially by regular RAM. Not sure why.

Quoted 8 lines

Isn't issue here in how backend is mapped into GPA? Well that is not backend's job.

Once one starts to put layout (alignment, noncontinuously mapped memory regions inside of container, ...), mapping HPA->GPA gets complicated.

It would be better to use simple building blocks and model as:
2 separate backends (ram + hugetlbfs) and 2 corresponding DIMM devices.

Right, I had forgotten that you can have cold-plugged DIMM devices. That's a nice solution, also because it simplifies passing the GPA configuration down to the guest.

How would that translate to sharing HostMemoryBackend code for memory policies? Which of Hu Tao's proposals do you like best?

Paolo

1: 'realize' approach I suggested
drawback is: assumption that all backends derived from HostMemoryBackend
will inherit NUMA controls even if backend shouldn't have one (for example: fictional remote memory backend)
plus: derived types from HostMemoryBackend, don't need to know anything
about NUMA.

Quoted 21 lines

static int
ram_backend_memory_init(HostMemoryBackend *backend, Error **errp) {

backends/hostmem-ram.c +14 -0

+    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(backend);
+    int mode = ram_backend->policy;
+    void *p;
+    unsigned long maxnode;
+
    if (!memory_region_size(&backend->mr)) {
        memory_region_init_ram(&backend->mr, OBJECT(backend),
                               object_get_canonical_path(OBJECT(backend)),
                               backend->size);
+
+        p = memory_region_get_ram_ptr(&backend->mr);
+        maxnode = find_last_bit(ram_backend->host_nodes, MAX_NODES);
+
+        mode |= ram_backend->relative ? MPOL_F_RELATIVE_NODES :
+            MPOL_F_STATIC_NODES;
+        /* This is a workaround for a long standing bug in Linux'
+         * mbind implementation, which cuts off the last specified
+         * node. To stay compatible should this bug be fixed, we

backends/hostmem-ram.c line 164

Quoted 4 lines

backends/hostmem-ram.c +4 -0

+         * specify one more node and zero this one out.
+         */
+        if (syscall(SYS_mbind, p, backend->size, mode,
+                    ram_backend->host_nodes, maxnode + 2, 0)) {

This does not compile on non-Linux; also, does libnuma include the workaround? If so, this is a hint that we should be using libnuma instead...

On Wed, Feb 19, 2014 at 10:03:13AM +0100, Paolo Bonzini wrote:

<...>

Quoted 21 lines

Quoted 21 lines

static int
ram_backend_memory_init(HostMemoryBackend *backend, Error **errp) {

backends/hostmem-ram.c +14 -0

+    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(backend);
+    int mode = ram_backend->policy;
+    void *p;
+    unsigned long maxnode;
+
    if (!memory_region_size(&backend->mr)) {
        memory_region_init_ram(&backend->mr, OBJECT(backend),
                               object_get_canonical_path(OBJECT(backend)),
                               backend->size);
+
+        p = memory_region_get_ram_ptr(&backend->mr);
+        maxnode = find_last_bit(ram_backend->host_nodes, MAX_NODES);
+
+        mode |= ram_backend->relative ? MPOL_F_RELATIVE_NODES :
+            MPOL_F_STATIC_NODES;
+        /* This is a workaround for a long standing bug in Linux'
+         * mbind implementation, which cuts off the last specified
+         * node. To stay compatible should this bug be fixed, we

Quoted 8 lines

backends/hostmem-ram.c line 164

Quoted 4 lines

backends/hostmem-ram.c +4 -0

+         * specify one more node and zero this one out.
+         */
+        if (syscall(SYS_mbind, p, backend->size, mode,
+                    ram_backend->host_nodes, maxnode + 2, 0)) {

This does not compile on non-Linux; also, does libnuma include the workaround? If so, this is a hint that we should be using libnuma instead...

Tested with libnuma and works fine without the workaround. Will use libnuma in v19.