Skip to contentSkip to conversation

NUMA series v5

PATCH In review Hu Tao 3 participants 47 messages Jun 10, 2014 – Jun 16, 2014 Arm STM32

lore
Share & tools
lore

Hu Tao

author
#0 lore raw

v5 cover NUMA series v5

note: based on MST's numa tree.

changes are error messages and docs tweaks to address comments to v4.

Hu Tao (7):
backend:hostmem: replace hostmemory with host_memory
hostmem: separate allocation from UserCreatable complete method
hostmem: add properties for NUMA memory policy
tests: fix memory leak in test of string input visitor
qapi: make string input visitor parse int list
qapi: make string output visitor parse int list
qmp: add query-memdev

Paolo Bonzini (8):
vl: redo -object parsing
fixup! qmp: improve error reporting for -object and object-add
pc: pass MachineState to pc_memory_init
fixup! numa: add -numa node,memdev= option
hostmem: add file-based HostMemoryBackend
hostmem: add merge and dump properties
hostmem: allow preallocation of any memory region
hostmem: add property to map memory with MAP_SHARED

Wanlong Gao (1):
fixup! NUMA: check if the total numa memory size is equal to ram_size

 backends/Makefile.objs             |   1 +
 backends/hostmem-file.c            | 134 ++++++++++++++++
 backends/hostmem-ram.c             |   7 +-
 backends/hostmem.c                 | 304 +++++++++++++++++++++++++++++++++++--
 exec.c                             |  25 ++-
 hw/i386/pc.c                       |  24 +--
 hw/i386/pc_piix.c                  |   8 +-
 hw/i386/pc_q35.c                   |   4 +-
 include/exec/memory.h              |  12 ++
 include/exec/ram_addr.h            |   4 +-
 include/hw/i386/pc.h               |   7 +-
 include/qemu/osdep.h               |  10 ++
 include/sysemu/hostmem.h           |   8 +
 memory.c                           |  14 +-
 numa.c                             |  78 +++++++++-
 qapi-schema.json                   |  60 ++++++++
 qapi/string-input-visitor.c        | 181 +++++++++++++++++++++-
 qapi/string-output-visitor.c       | 230 ++++++++++++++++++++++++++--
 qemu-options.hx                    |   6 +-
 qmp-commands.hx                    |  38 +++++
 qmp.c                              |   3 +-
 tests/test-string-input-visitor.c  |  39 +++++
 tests/test-string-output-visitor.c |  34 +++++
 vl.c                               |  65 ++++----
 24 files changed, 1191 insertions(+), 105 deletions(-)
 create mode 100644 backends/hostmem-file.c
Signature · 1 line
1.9.3
author
#1 lore raw

PATCH v5 1/16 fixup! NUMA: check if the total numa memory size is equal to ram_size

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 table, 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.

Conflicts:
numa.c

---
 numa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
numa.c +2 -2
index 2c349ae..6ea267e 100644
@@ -187,8 +187,8 @@ void set_numa_nodes(void)
             numa_total += numa_info[i].node_mem;
         }
         if (numa_total != ram_size) {
-            error_report("qemu: total memory size for NUMA nodes (%" PRIu64 ")"
-                         " should equal RAM size (" RAM_ADDR_FMT ")\n",
+            error_report("total memory for NUMA nodes (%" PRIu64 ")"
+                         " should equal RAM size (" RAM_ADDR_FMT ")",
                          numa_total, ram_size);
             exit(1);
         }
Signature · 1 line
1.9.3
author
#2 lore raw

PATCH v5 2/16 vl: redo -object parsing

In reply to Hu Tao

From: Paolo Bonzini <[email protected]>

Follow the lines of the HMP implementation, using OptsVisitor to parse the options. This gives access to OptsVisitor's rich parsing of integer lists.

---
 vl.c | 65 ++++++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 36 insertions(+), 29 deletions(-)
vl.c +36 -29
index 730bd1e..1617013 100644
@@ -116,7 +116,7 @@ int main(int argc, char **argv)
 
 #include "ui/qemu-spice.h"
 #include "qapi/string-input-visitor.h"
-#include "qom/object_interfaces.h"
+#include "qapi/opts-visitor.h"
 
 #define DEFAULT_RAM_SIZE 128
 
@@ -2822,44 +2822,51 @@ static int object_set_property(const char *name, const char *value, void *opaque
 
 static int object_create(QemuOpts *opts, void *opaque)
 {
-    const char *type = qemu_opt_get(opts, "qom-type");
-    const char *id = qemu_opts_id(opts);
-    Error *local_err = NULL;
-    Object *obj;
-
-    g_assert(type != NULL);
-
-    if (id == NULL) {
-        qerror_report(QERR_MISSING_PARAMETER, "id");
-        return -1;
+    Error *err = NULL;
+    char *type = NULL;
+    char *id = NULL;
+    void *dummy = NULL;
+    OptsVisitor *ov;
+    QDict *pdict;
+
+    ov = opts_visitor_new(opts);
+    pdict = qemu_opts_to_qdict(opts, NULL);
+
+    visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
+    if (err) {
+        goto out;
     }
 
-    obj = object_new(type);
-    if (qemu_opt_foreach(opts, object_set_property, obj, 1) < 0) {
-        object_unref(obj);
-        return -1;
+    qdict_del(pdict, "qom-type");
+    visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
+    if (err) {
+        goto out;
     }
 
-    if (!object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
-        error_setg(&local_err, "object '%s' isn't supported by -object",
-                   id);
+    qdict_del(pdict, "id");
+    visit_type_str(opts_get_visitor(ov), &id, "id", &err);
+    if (err) {
         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_property_del(container_get(object_get_root(), "/objects"),
-                            id, &error_abort);
+    object_add(type, id, pdict, opts_get_visitor(ov), &err);
+    if (err) {
         goto out;
     }
+    visit_end_struct(opts_get_visitor(ov), &err);
+    if (err) {
+        qmp_object_del(id, NULL);
+    }
+
 out:
-    object_unref(obj);
-    if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+    opts_visitor_cleanup(ov);
+
+    QDECREF(pdict);
+    g_free(id);
+    g_free(type);
+    g_free(dummy);
+    if (err) {
+        qerror_report_err(err);
         return -1;
     }
     return 0;
Signature · 1 line
1.9.3
author
#3 lore raw

PATCH v5 3/16 fixup! qmp: improve error reporting for -object and object-add

In reply to Hu Tao

From: Paolo Bonzini <[email protected]>

reword error message to make it more understandable.

Conflicts:
qmp.c

---
 qmp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
qmp.c +1 -2
index cef60fb..c3c0229 100644
@@ -540,8 +540,7 @@ void object_add(const char *type, const char *id, const QDict *qdict,
 
     klass = object_class_by_name(type);
     if (!klass) {
-        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
-                  "qom-type", "a valid class name");
+        error_setg(errp, "invalid object type: %s", type);
         return;
     }
 
Signature · 1 line
1.9.3
author
#4 lore raw

PATCH v5 4/16 pc: pass MachineState to pc_memory_init

In reply to Hu Tao

From: Paolo Bonzini <[email protected]>

---
 hw/i386/pc.c         | 24 ++++++++++++------------
 hw/i386/pc_piix.c    |  8 +++-----
 hw/i386/pc_q35.c     |  4 +---
 include/hw/i386/pc.h |  7 +++----
 4 files changed, 19 insertions(+), 24 deletions(-)
hw/i386/pc.c +12 -12
index a035da6..37344ce 100644
@@ -1194,10 +1194,8 @@ void pc_acpi_init(const char *default_dsdt)
     }
 }
 
-FWCfgState *pc_memory_init(MemoryRegion *system_memory,
-                           const char *kernel_filename,
-                           const char *kernel_cmdline,
-                           const char *initrd_filename,
+FWCfgState *pc_memory_init(MachineState *machine,
+                           MemoryRegion *system_memory,
                            ram_addr_t below_4g_mem_size,
                            ram_addr_t above_4g_mem_size,
                            MemoryRegion *rom_memory,
@@ -1208,18 +1206,19 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
     MemoryRegion *ram, *option_rom_mr;
     MemoryRegion *ram_below_4g, *ram_above_4g;
     FWCfgState *fw_cfg;
-    ram_addr_t ram_size = below_4g_mem_size + above_4g_mem_size;
-    MachineState *machine = MACHINE(qdev_get_machine());
     PCMachineState *pcms = PC_MACHINE(machine);
 
-    linux_boot = (kernel_filename != NULL);
+    assert(machine->ram_size == below_4g_mem_size + above_4g_mem_size);
+
+    linux_boot = (machine->kernel_filename != NULL);
 
     /* Allocate RAM.  We allocate it as a single memory region and use
      * aliases to address portions of it, mostly for backwards compatibility
      * with older qemus that used qemu_ram_alloc().
      */
     ram = g_malloc(sizeof(*ram));
-    memory_region_allocate_system_memory(ram, NULL, "pc.ram", ram_size);
+    memory_region_allocate_system_memory(ram, NULL, "pc.ram",
+                                         machine->ram_size);
     *ram_memory = ram;
     ram_below_4g = g_malloc(sizeof(*ram_below_4g));
     memory_region_init_alias(ram_below_4g, NULL, "ram-below-4g", ram,
@@ -1237,7 +1236,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
 
     if (!guest_info->has_reserved_memory &&
         (machine->ram_slots ||
-         (machine->maxram_size > ram_size))) {
+         (machine->maxram_size > machine->ram_size))) {
         MachineClass *mc = MACHINE_GET_CLASS(machine);
 
         error_report("\"-memory 'slots|maxmem'\" is not supported by: %s",
@@ -1247,9 +1246,9 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
 
     /* initialize hotplug memory address space */
     if (guest_info->has_reserved_memory &&
-        (ram_size < machine->maxram_size)) {
+        (machine->ram_size < machine->maxram_size)) {
         ram_addr_t hotplug_mem_size =
-            machine->maxram_size - ram_size;
+            machine->maxram_size - machine->ram_size;
 
         if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
             error_report("unsupported amount of memory slots: %"PRIu64,
@@ -1294,7 +1293,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
     }
 
     if (linux_boot) {
-        load_linux(fw_cfg, kernel_filename, initrd_filename, kernel_cmdline, below_4g_mem_size);
+        load_linux(fw_cfg, machine->kernel_filename, machine->initrd_filename,
+                   machine->kernel_cmdline, below_4g_mem_size);
     }
 
     for (i = 0; i < nb_option_roms; i++) {
hw/i386/pc_piix.c +3 -5
index a13e8d6..3e7524b 100644
@@ -156,11 +156,9 @@ static void pc_init1(MachineState *machine,
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
-        fw_cfg = pc_memory_init(system_memory,
-                       machine->kernel_filename, machine->kernel_cmdline,
-                       machine->initrd_filename,
-                       below_4g_mem_size, above_4g_mem_size,
-                       rom_memory, &ram_memory, guest_info);
+        fw_cfg = pc_memory_init(machine, system_memory,
+                                below_4g_mem_size, above_4g_mem_size,
+                                rom_memory, &ram_memory, guest_info);
     }
 
     gsi_state = g_malloc0(sizeof(*gsi_state));
hw/i386/pc_q35.c +1 -3
index 629eb2d..aa71332 100644
@@ -143,9 +143,7 @@ static void pc_q35_init(MachineState *machine)
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
-        pc_memory_init(get_system_memory(),
-                       machine->kernel_filename, machine->kernel_cmdline,
-                       machine->initrd_filename,
+        pc_memory_init(machine, get_system_memory(),
                        below_4g_mem_size, above_4g_mem_size,
                        rom_memory, &ram_memory, guest_info);
     }
include/hw/i386/pc.h +3 -4
index a2bf22c..2c3d7c4 100644
@@ -3,6 +3,7 @@
 
 #include "qemu-common.h"
 #include "exec/memory.h"
+#include "hw/boards.h"
 #include "hw/isa/isa.h"
 #include "hw/block/fdc.h"
 #include "net/net.h"
@@ -183,10 +184,8 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
 void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
                             MemoryRegion *pci_address_space);
 
-FWCfgState *pc_memory_init(MemoryRegion *system_memory,
-                           const char *kernel_filename,
-                           const char *kernel_cmdline,
-                           const char *initrd_filename,
+FWCfgState *pc_memory_init(MachineState *machine,
+                           MemoryRegion *system_memory,
                            ram_addr_t below_4g_mem_size,
                            ram_addr_t above_4g_mem_size,
                            MemoryRegion *rom_memory,
Signature · 1 line
1.9.3
author
#5 lore raw

PATCH v5 5/16 backend:hostmem: replace hostmemory with host_memory

In reply to Hu Tao

..to keep names consistant.

---
 backends/hostmem.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
backends/hostmem.c +12 -12
index 2f578ac..2d9fd00 100644
@@ -17,8 +17,8 @@
 #include "qom/object_interfaces.h"
 
 static void
-hostmemory_backend_get_size(Object *obj, Visitor *v, void *opaque,
-                            const char *name, Error **errp)
+host_memory_backend_get_size(Object *obj, Visitor *v, void *opaque,
+                             const char *name, Error **errp)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
     uint64_t value = backend->size;
@@ -27,8 +27,8 @@ hostmemory_backend_get_size(Object *obj, Visitor *v, void *opaque,
 }
 
 static void
-hostmemory_backend_set_size(Object *obj, Visitor *v, void *opaque,
-                            const char *name, Error **errp)
+host_memory_backend_set_size(Object *obj, Visitor *v, void *opaque,
+                             const char *name, Error **errp)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
     Error *local_err = NULL;
@@ -53,14 +53,14 @@ out:
     error_propagate(errp, local_err);
 }
 
-static void hostmemory_backend_init(Object *obj)
+static void host_memory_backend_init(Object *obj)
 {
     object_property_add(obj, "size", "int",
-                        hostmemory_backend_get_size,
-                        hostmemory_backend_set_size, NULL, NULL, NULL);
+                        host_memory_backend_get_size,
+                        host_memory_backend_set_size, NULL, NULL, NULL);
 }
 
-static void hostmemory_backend_finalize(Object *obj)
+static void host_memory_backend_finalize(Object *obj)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
 
@@ -75,14 +75,14 @@ host_memory_backend_get_memory(HostMemoryBackend *backend, Error **errp)
     return memory_region_size(&backend->mr) ? &backend->mr : NULL;
 }
 
-static const TypeInfo hostmemory_backend_info = {
+static const TypeInfo host_memory_backend_info = {
     .name = TYPE_MEMORY_BACKEND,
     .parent = TYPE_OBJECT,
     .abstract = true,
     .class_size = sizeof(HostMemoryBackendClass),
     .instance_size = sizeof(HostMemoryBackend),
-    .instance_init = hostmemory_backend_init,
-    .instance_finalize = hostmemory_backend_finalize,
+    .instance_init = host_memory_backend_init,
+    .instance_finalize = host_memory_backend_finalize,
     .interfaces = (InterfaceInfo[]) {
         { TYPE_USER_CREATABLE },
         { }
@@ -91,7 +91,7 @@ static const TypeInfo hostmemory_backend_info = {
 
 static void register_types(void)
 {
-    type_register_static(&hostmemory_backend_info);
+    type_register_static(&host_memory_backend_info);
 }
 
 type_init(register_types);
Signature · 1 line
1.9.3
author
#6 lore raw

PATCH v5 6/16 hostmem: separate allocation from UserCreatable complete method

In reply to Hu Tao

This allows the superclass to set various policies on the memory region that the subclass creates. Drops hostmem-ram's complete method accordingly.

---
 backends/hostmem-ram.c   |  7 +++----
 backends/hostmem.c       | 20 ++++++++++++++++++++
 include/sysemu/hostmem.h |  2 ++
 3 files changed, 25 insertions(+), 4 deletions(-)
backends/hostmem-ram.c +3 -4
index bba2ebc..d9a8290 100644
@@ -16,9 +16,8 @@
 
 
 static void
-ram_backend_memory_init(UserCreatable *uc, Error **errp)
+ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
-    HostMemoryBackend *backend = MEMORY_BACKEND(uc);
     char *path;
 
     if (!backend->size) {
@@ -35,9 +34,9 @@ ram_backend_memory_init(UserCreatable *uc, Error **errp)
 static void
 ram_backend_class_init(ObjectClass *oc, void *data)
 {
-    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+    HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
 
-    ucc->complete = ram_backend_memory_init;
+    bc->alloc = ram_backend_memory_alloc;
 }
 
 static const TypeInfo ram_backend_info = {
backends/hostmem.c +20 -0
index 2d9fd00..1738774 100644
@@ -75,11 +75,31 @@ host_memory_backend_get_memory(HostMemoryBackend *backend, Error **errp)
     return memory_region_size(&backend->mr) ? &backend->mr : NULL;
 }
 
+static void
+host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(uc);
+    HostMemoryBackendClass *bc = MEMORY_BACKEND_GET_CLASS(uc);
+
+    if (bc->alloc) {
+        bc->alloc(backend, errp);
+    }
+}
+
+static void
+host_memory_backend_class_init(ObjectClass *oc, void *data)
+{
+    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+
+    ucc->complete = host_memory_backend_memory_complete;
+}
+
 static const TypeInfo host_memory_backend_info = {
     .name = TYPE_MEMORY_BACKEND,
     .parent = TYPE_OBJECT,
     .abstract = true,
     .class_size = sizeof(HostMemoryBackendClass),
+    .class_init = host_memory_backend_class_init,
     .instance_size = sizeof(HostMemoryBackend),
     .instance_init = host_memory_backend_init,
     .instance_finalize = host_memory_backend_finalize,
include/sysemu/hostmem.h +2 -0
index 4fc081e..923f672 100644
@@ -34,6 +34,8 @@ typedef struct HostMemoryBackendClass HostMemoryBackendClass;
  */
 struct HostMemoryBackendClass {
     ObjectClass parent_class;
+
+    void (*alloc)(HostMemoryBackend *backend, Error **errp);
 };
 
 /**
Signature · 1 line
1.9.3
author
#7 lore raw

PATCH v5 7/16 fixup! numa: add -numa node, memdev= option

In reply to 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=bind,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 node,mem=".

Conflicts:
numa.c
qemu-options.hx

---
 qemu-options.hx | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
qemu-options.hx +4 -2
index 5375a93..73b6f1f 100644
@@ -95,9 +95,11 @@ specifies the maximum number of hotpluggable CPUs.
 ETEXI
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,
-    "-numa node[,mem=size][,memdev=id][,cpus=cpu[-cpu]][,nodeid=node]\n", QEMU_ARCH_ALL)
+    "-numa node[,mem=size][,cpus=cpu[-cpu]][,nodeid=node]\n"
+    "-numa node[,memdev=id][,cpus=cpu[-cpu]][,nodeid=node]\n", QEMU_ARCH_ALL)
 STEXI
-@item -numa node[,mem=@var{size}][,memdev=@var{id}][,cpus=@var{cpu[-cpu]}][,nodeid=@var{node}]
+@item -numa node[,mem=@var{size}][,cpus=@var{cpu[-cpu]}][,nodeid=@var{node}]
+@item -numa node[,memdev=@var{id}][,cpus=@var{cpu[-cpu]}][,nodeid=@var{node}]
 @findex -numa
 Simulate a multi node NUMA system. If @samp{mem}, @samp{memdev}
 and @samp{cpus} are omitted, resources are split equally. Also, note
Signature · 1 line
1.9.3
#8 lore raw

Re: [Qemu-devel] [PATCH v5 07/16] fixup! numa: add -numa node, memdev= option

Quoted 10 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=bind,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

Quoted 8 lines

The option replaces "-numa node,mem=".

Conflicts:
numa.c
qemu-options.hx

Don't keep the Conflicts option around next time pls. I dropped this for now.

Quoted 24 lines
---
 qemu-options.hx | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
qemu-options.hx +4 -2
index 5375a93..73b6f1f 100644
@@ -95,9 +95,11 @@ specifies the maximum number of hotpluggable CPUs.
 ETEXI
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,
-    "-numa node[,mem=size][,memdev=id][,cpus=cpu[-cpu]][,nodeid=node]\n", QEMU_ARCH_ALL)
+    "-numa node[,mem=size][,cpus=cpu[-cpu]][,nodeid=node]\n"
+    "-numa node[,memdev=id][,cpus=cpu[-cpu]][,nodeid=node]\n", QEMU_ARCH_ALL)
 STEXI
-@item -numa node[,mem=@var{size}][,memdev=@var{id}][,cpus=@var{cpu[-cpu]}][,nodeid=@var{node}]
+@item -numa node[,mem=@var{size}][,cpus=@var{cpu[-cpu]}][,nodeid=@var{node}]
+@item -numa node[,memdev=@var{id}][,cpus=@var{cpu[-cpu]}][,nodeid=@var{node}]
 @findex -numa
 Simulate a multi node NUMA system. If @samp{mem}, @samp{memdev}
 and @samp{cpus} are omitted, resources are split equally. Also, note
Signature · 1 line
1.9.3
#9 lore raw

Re: [Qemu-devel] [PATCH v5 07/16] fixup! numa: add -numa node, memdev= option

In reply to Hu Tao

Original subject was:
numa: add -numa node, memdev= option
with space after ,

if you are sending fixups, please make subject match verbatim.
I fixed this up for now.

Quoted 10 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=bind,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

Quoted 8 lines

The option replaces "-numa node,mem=".

Conflicts:
numa.c
qemu-options.hx

would be better to mention what's being changed here instead.

Quoted 24 lines
---
 qemu-options.hx | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
qemu-options.hx +4 -2
index 5375a93..73b6f1f 100644
@@ -95,9 +95,11 @@ specifies the maximum number of hotpluggable CPUs.
 ETEXI
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,
-    "-numa node[,mem=size][,memdev=id][,cpus=cpu[-cpu]][,nodeid=node]\n", QEMU_ARCH_ALL)
+    "-numa node[,mem=size][,cpus=cpu[-cpu]][,nodeid=node]\n"
+    "-numa node[,memdev=id][,cpus=cpu[-cpu]][,nodeid=node]\n", QEMU_ARCH_ALL)
 STEXI
-@item -numa node[,mem=@var{size}][,memdev=@var{id}][,cpus=@var{cpu[-cpu]}][,nodeid=@var{node}]
+@item -numa node[,mem=@var{size}][,cpus=@var{cpu[-cpu]}][,nodeid=@var{node}]
+@item -numa node[,memdev=@var{id}][,cpus=@var{cpu[-cpu]}][,nodeid=@var{node}]
 @findex -numa
 Simulate a multi node NUMA system. If @samp{mem}, @samp{memdev}
 and @samp{cpus} are omitted, resources are split equally. Also, note
Signature · 1 line
1.9.3
author
#10 lore raw

PATCH v5 8/16 hostmem: add file-based HostMemoryBackend

In reply to Hu Tao

From: Paolo Bonzini <[email protected]>

---
 backends/Makefile.objs  |   1 +
 backends/hostmem-file.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+)
 create mode 100644 backends/hostmem-file.c
backends/Makefile.objs +1 -0
index 7fb7acd..506a46c 100644
@@ -8,3 +8,4 @@ baum.o-cflags := $(SDL_CFLAGS)
 common-obj-$(CONFIG_TPM) += tpm.o
 
 common-obj-y += hostmem.o hostmem-ram.o
+common-obj-$(CONFIG_LINUX) += hostmem-file.o
backends/hostmem-file.c new file +107 -0

107 lines not on this page · raw message

Signature · 1 line
1.9.3
#11 lore raw

Re: [Qemu-devel] [PATCH v5 08/16] hostmem: add file-based HostMemoryBackend

Quoted 25 lines · Hu Tao

From: Paolo Bonzini <[email protected]>

---
 backends/Makefile.objs  |   1 +
 backends/hostmem-file.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+)
 create mode 100644 backends/hostmem-file.c
backends/Makefile.objs +1 -0
index 7fb7acd..506a46c 100644
@@ -8,3 +8,4 @@ baum.o-cflags := $(SDL_CFLAGS)
 common-obj-$(CONFIG_TPM) += tpm.o
 
 common-obj-y += hostmem.o hostmem-ram.o
+common-obj-$(CONFIG_LINUX) += hostmem-file.o
backends/hostmem-file.c new file +0 -0
new file mode 100644
index 0000000..9b75314

backends/hostmem-file.c line 4

Quoted 4 lines
backends/hostmem-file.c new file +4 -0
@@ -0,0 +1,107 @@
+/*
+ * QEMU Host Memory Backend for hugetlbfs
+ *
+ * Copyright (C) 2013 Red Hat Inc

I tweaked copyright to 2013-2014 here.
Not too important since the full history is in the git log, but still.

Quoted 105 lines
backends/hostmem-file.c new file +103 -0
+ *
+ * Authors:
+ *   Paolo Bonzini <[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 "qom/object_interfaces.h"
+
+/* hostmem-file.c */
+/**
+ * @TYPE_MEMORY_BACKEND_FILE:
+ * name of backend that uses mmap on a file descriptor
+ */
+#define TYPE_MEMORY_BACKEND_FILE "memory-backend-file"
+
+#define MEMORY_BACKEND_FILE(obj) \
+    OBJECT_CHECK(HostMemoryBackendFile, (obj), TYPE_MEMORY_BACKEND_FILE)
+
+typedef struct HostMemoryBackendFile HostMemoryBackendFile;
+
+struct HostMemoryBackendFile {
+    HostMemoryBackend parent_obj;
+    char *mem_path;
+};
+
+static void
+file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
+{
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
+
+    if (!backend->size) {
+        error_setg(errp, "can't create backend with size 0");
+        return;
+    }
+    if (!fb->mem_path) {
+        error_setg(errp, "mem_path property not set");
+        return;
+    }
+#ifndef CONFIG_LINUX
+    error_setg(errp, "-mem-path not supported on this host");
+#else
+    if (!memory_region_size(&backend->mr)) {
+        memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
+                                 object_get_canonical_path(OBJECT(backend)),
+                                 backend->size,
+                                 fb->mem_path, errp);
+    }
+#endif
+}
+
+static void
+file_backend_class_init(ObjectClass *oc, void *data)
+{
+    HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
+
+    bc->alloc = file_backend_memory_alloc;
+}
+
+static char *get_mem_path(Object *o, Error **errp)
+{
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+
+    return g_strdup(fb->mem_path);
+}
+
+static void set_mem_path(Object *o, const char *str, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+
+    if (memory_region_size(&backend->mr)) {
+        error_setg(errp, "cannot change property value");
+        return;
+    }
+    if (fb->mem_path) {
+        g_free(fb->mem_path);
+    }
+    fb->mem_path = g_strdup(str);
+}
+
+static void
+file_backend_instance_init(Object *o)
+{
+    object_property_add_str(o, "mem-path", get_mem_path,
+                            set_mem_path, NULL);
+}
+
+static const TypeInfo file_backend_info = {
+    .name = TYPE_MEMORY_BACKEND_FILE,
+    .parent = TYPE_MEMORY_BACKEND,
+    .class_init = file_backend_class_init,
+    .instance_init = file_backend_instance_init,
+    .instance_size = sizeof(HostMemoryBackendFile),
+};
+
+static void register_types(void)
+{
+    type_register_static(&file_backend_info);
+}
+
+type_init(register_types);
Signature · 1 line
1.9.3
author
#12 lore raw

PATCH v5 9/16 hostmem: add merge and dump properties

In reply to Hu Tao

From: Paolo Bonzini <[email protected]>

---
 backends/hostmem.c       | 84 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/qemu/osdep.h     | 10 ++++++
 include/sysemu/hostmem.h |  1 +
 3 files changed, 94 insertions(+), 1 deletion(-)
backends/hostmem.c +83 -1
index 1738774..a2550fe 100644
@@ -53,8 +53,73 @@ out:
     error_propagate(errp, local_err);
 }
 
+static bool host_memory_backend_get_merge(Object *obj, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+
+    return backend->merge;
+}
+
+static void host_memory_backend_set_merge(Object *obj, bool value, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+
+    if (!memory_region_size(&backend->mr)) {
+        backend->merge = value;
+        return;
+    }
+
+    if (value != backend->merge) {
+        void *ptr = memory_region_get_ram_ptr(&backend->mr);
+        uint64_t sz = memory_region_size(&backend->mr);
+
+        qemu_madvise(ptr, sz,
+                     value ? QEMU_MADV_MERGEABLE : QEMU_MADV_UNMERGEABLE);
+        backend->merge = value;
+    }
+}
+
+static bool host_memory_backend_get_dump(Object *obj, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+
+    return backend->dump;
+}
+
+static void host_memory_backend_set_dump(Object *obj, bool value, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+
+    if (!memory_region_size(&backend->mr)) {
+        backend->dump = value;
+        return;
+    }
+
+    if (value != backend->dump) {
+        void *ptr = memory_region_get_ram_ptr(&backend->mr);
+        uint64_t sz = memory_region_size(&backend->mr);
+
+        qemu_madvise(ptr, sz,
+                     value ? QEMU_MADV_DODUMP : QEMU_MADV_DONTDUMP);
+        backend->dump = value;
+    }
+}
+
 static void host_memory_backend_init(Object *obj)
 {
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+
+    backend->merge = qemu_opt_get_bool(qemu_get_machine_opts(),
+                                       "mem-merge", true);
+    backend->dump = qemu_opt_get_bool(qemu_get_machine_opts(),
+                                      "dump-guest-core", true);
+
+    object_property_add_bool(obj, "merge",
+                        host_memory_backend_get_merge,
+                        host_memory_backend_set_merge, NULL);
+    object_property_add_bool(obj, "dump",
+                        host_memory_backend_get_dump,
+                        host_memory_backend_set_dump, NULL);
     object_property_add(obj, "size", "int",
                         host_memory_backend_get_size,
                         host_memory_backend_set_size, NULL, NULL, NULL);
@@ -80,9 +145,26 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(uc);
     HostMemoryBackendClass *bc = MEMORY_BACKEND_GET_CLASS(uc);
+    Error *local_err = NULL;
+    void *ptr;
+    uint64_t sz;
 
     if (bc->alloc) {
-        bc->alloc(backend, errp);
+        bc->alloc(backend, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+
+        ptr = memory_region_get_ram_ptr(&backend->mr);
+        sz = memory_region_size(&backend->mr);
+
+        if (backend->merge) {
+            qemu_madvise(ptr, sz, QEMU_MADV_MERGEABLE);
+        }
+        if (!backend->dump) {
+            qemu_madvise(ptr, sz, QEMU_MADV_DONTDUMP);
+        }
     }
 }
 
include/qemu/osdep.h +10 -0
index 9c1a119..820c5d0 100644
@@ -116,6 +116,16 @@ void qemu_anon_ram_free(void *ptr, size_t size);
 #else
 #define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
 #endif
+#ifdef MADV_UNMERGEABLE
+#define QEMU_MADV_UNMERGEABLE MADV_UNMERGEABLE
+#else
+#define QEMU_MADV_UNMERGEABLE QEMU_MADV_INVALID
+#endif
+#ifdef MADV_DODUMP
+#define QEMU_MADV_DODUMP MADV_DODUMP
+#else
+#define QEMU_MADV_DODUMP QEMU_MADV_INVALID
+#endif
 #ifdef MADV_DONTDUMP
 #define QEMU_MADV_DONTDUMP MADV_DONTDUMP
 #else
include/sysemu/hostmem.h +1 -0
index 923f672..ede5ec9 100644
@@ -52,6 +52,7 @@ struct HostMemoryBackend {
 
     /* protected */
     uint64_t size;
+    bool merge, dump;
 
     MemoryRegion mr;
 };
Signature · 1 line
1.9.3
author
#13 lore raw

PATCH v5 10/16 hostmem: allow preallocation of any memory region

In reply to Hu Tao

From: Paolo Bonzini <[email protected]>

And allow preallocation of file-based memory even without -mem-prealloc. Some care is necessary because -mem-prealloc does not allow disabling preallocation for hostmem-file.

---
 backends/hostmem-file.c  |  3 +++
 backends/hostmem.c       | 42 ++++++++++++++++++++++++++++++++++++++++++
 exec.c                   |  7 +++++++
 include/exec/memory.h    | 10 ++++++++++
 include/exec/ram_addr.h  |  1 +
 include/sysemu/hostmem.h |  1 +
 memory.c                 | 11 +++++++++++
 7 files changed, 75 insertions(+)
backends/hostmem-file.c +3 -0
index 9b75314..35e32d1 100644
@@ -9,7 +9,9 @@
  * 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 "qemu-common.h"
 #include "sysemu/hostmem.h"
+#include "sysemu/sysemu.h"
 #include "qom/object_interfaces.h"
 
 /* hostmem-file.c */
@@ -46,6 +48,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     error_setg(errp, "-mem-path not supported on this host");
 #else
     if (!memory_region_size(&backend->mr)) {
+        backend->force_prealloc = mem_prealloc;
         memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
                                  object_get_canonical_path(OBJECT(backend)),
                                  backend->size,
backends/hostmem.c +42 -0

66 lines not on this page · raw message

exec.c +7 -0
index 739f0cf..520d673 100644
@@ -1432,6 +1432,13 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
 }
 #endif /* !_WIN32 */
 
+int qemu_get_ram_fd(ram_addr_t addr)
+{
+    RAMBlock *block = qemu_get_ram_block(addr);
+
+    return block->fd;
+}
+
 /* Return a host pointer to ram allocated with qemu_ram_alloc.
    With the exception of the softmmu code in this file, this should
    only be used for local memory (e.g. video ram) that the device owns,
include/exec/memory.h +10 -0
index 82d7781..36226f7 100644
@@ -534,6 +534,16 @@ bool memory_region_is_logging(MemoryRegion *mr);
 bool memory_region_is_rom(MemoryRegion *mr);
 
 /**
+ * memory_region_get_fd: Get a file descriptor backing a RAM memory region.
+ *
+ * Returns a file descriptor backing a file-based RAM memory region,
+ * or -1 if the region is not a file-based RAM memory region.
+ *
+ * @mr: the RAM or alias memory region being queried.
+ */
+int memory_region_get_fd(MemoryRegion *mr);
+
+/**
  * memory_region_get_ram_ptr: Get a pointer into a RAM memory region.
  *
  * Returns a host pointer to a RAM memory region (created with
include/exec/ram_addr.h +1 -0
index f9518a6..d352f60 100644
@@ -27,6 +27,7 @@ ram_addr_t qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
 ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
                                    MemoryRegion *mr);
 ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr);
+int qemu_get_ram_fd(ram_addr_t addr);
 void *qemu_get_ram_ptr(ram_addr_t addr);
 void qemu_ram_free(ram_addr_t addr);
 void qemu_ram_free_from_ptr(ram_addr_t addr);
include/sysemu/hostmem.h +1 -0
index ede5ec9..4cae673 100644
@@ -53,6 +53,7 @@ struct HostMemoryBackend {
     /* protected */
     uint64_t size;
     bool merge, dump;
+    bool prealloc, force_prealloc;
 
     MemoryRegion mr;
 };
memory.c +11 -0
index 310729a..bcef72b 100644
@@ -1258,6 +1258,17 @@ void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
     cpu_physical_memory_reset_dirty(mr->ram_addr + addr, size, client);
 }
 
+int memory_region_get_fd(MemoryRegion *mr)
+{
+    if (mr->alias) {
+        return memory_region_get_fd(mr->alias);
+    }
+
+    assert(mr->terminates);
+
+    return qemu_get_ram_fd(mr->ram_addr & TARGET_PAGE_MASK);
+}
+
 void *memory_region_get_ram_ptr(MemoryRegion *mr)
 {
     if (mr->alias) {
Signature · 1 line
1.9.3
author
#14 lore raw

PATCH v5 11/16 hostmem: add property to map memory with MAP_SHARED

In reply to Hu Tao

From: Paolo Bonzini <[email protected]>

A new "share" property can be used with the "memory-file" backend to map memory with MAP_SHARED instead of MAP_PRIVATE.

---
 backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
 exec.c                  | 18 ++++++++++--------
 include/exec/memory.h   |  2 ++
 include/exec/ram_addr.h |  3 ++-
 memory.c                |  3 ++-
 numa.c                  |  2 +-
 6 files changed, 42 insertions(+), 12 deletions(-)
backends/hostmem-file.c +25 -1
index 35e32d1..d20f79b 100644
@@ -28,6 +28,8 @@ typedef struct HostMemoryBackendFile HostMemoryBackendFile;
 
 struct HostMemoryBackendFile {
     HostMemoryBackend parent_obj;
+
+    bool share;
     char *mem_path;
 };
 
@@ -51,7 +53,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
         backend->force_prealloc = mem_prealloc;
         memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
                                  object_get_canonical_path(OBJECT(backend)),
-                                 backend->size,
+                                 backend->size, fb->share,
                                  fb->mem_path, errp);
     }
 #endif
@@ -87,9 +89,31 @@ static void set_mem_path(Object *o, const char *str, Error **errp)
     fb->mem_path = g_strdup(str);
 }
 
+static bool file_memory_backend_get_share(Object *o, Error **errp)
+{
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+
+    return fb->share;
+}
+
+static void file_memory_backend_set_share(Object *o, bool value, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+
+    if (memory_region_size(&backend->mr)) {
+        error_setg(errp, "cannot change property value");
+        return;
+    }
+    fb->share = value;
+}
+
 static void
 file_backend_instance_init(Object *o)
 {
+    object_property_add_bool(o, "share",
+                        file_memory_backend_get_share,
+                        file_memory_backend_set_share, NULL);
     object_property_add_str(o, "mem-path", get_mem_path,
                             set_mem_path, NULL);
 }
exec.c +10 -8
index 520d673..8705cc5 100644
@@ -73,6 +73,9 @@ static MemoryRegion io_mem_unassigned;
 /* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */
 #define RAM_PREALLOC   (1 << 0)
 
+/* RAM is mmap-ed with MAP_SHARED */
+#define RAM_SHARED     (1 << 1)
+
 #endif
 
 struct CPUTailQ cpus = QTAILQ_HEAD_INITIALIZER(cpus);
@@ -1074,7 +1077,9 @@ static void *file_ram_alloc(RAMBlock *block,
         perror("ftruncate");
     }
 
-    area = mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+    area = mmap(0, memory, PROT_READ | PROT_WRITE,
+                (block->flags & RAM_SHARED ? MAP_SHARED : MAP_PRIVATE),
+                fd, 0);
     if (area == MAP_FAILED) {
         error_setg_errno(errp, errno,
                          "unable to map backing store for hugepages");
@@ -1270,7 +1275,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block)
 
 #ifdef __linux__
 ram_addr_t qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
-                                    const char *mem_path,
+                                    bool share, const char *mem_path,
                                     Error **errp)
 {
     RAMBlock *new_block;
@@ -1295,6 +1300,7 @@ ram_addr_t qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
     new_block = g_malloc0(sizeof(*new_block));
     new_block->mr = mr;
     new_block->length = size;
+    new_block->flags = share ? RAM_SHARED : 0;
     new_block->host = file_ram_alloc(new_block, size,
                                      mem_path, errp);
     if (!new_block->host) {
@@ -1397,12 +1403,8 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
                 flags = MAP_FIXED;
                 munmap(vaddr, length);
                 if (block->fd >= 0) {
-#ifdef MAP_POPULATE
-                    flags |= mem_prealloc ? MAP_POPULATE | MAP_SHARED :
-                        MAP_PRIVATE;
-#else
-                    flags |= MAP_PRIVATE;
-#endif
+                    flags |= (block->flags & RAM_SHARED ?
+                              MAP_SHARED : MAP_PRIVATE);
                     area = mmap(vaddr, length, PROT_READ | PROT_WRITE,
                                 flags, block->fd, offset);
                 } else {
include/exec/memory.h +2 -0
index 36226f7..f01f623 100644
@@ -321,6 +321,7 @@ void memory_region_init_ram(MemoryRegion *mr,
  * @owner: the object that tracks the region's reference count
  * @name: the name of the region.
  * @size: size of the region.
+ * @share: %true if memory must be mmaped with the MAP_SHARED flag
  * @path: the path in which to allocate the RAM.
  * @errp: pointer to Error*, to store an error if it happens.
  */
@@ -328,6 +329,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
                                       struct Object *owner,
                                       const char *name,
                                       uint64_t size,
+                                      bool share,
                                       const char *path,
                                       Error **errp);
 #endif
include/exec/ram_addr.h +2 -1
index d352f60..1d4ac74 100644
@@ -23,7 +23,8 @@
 #include "hw/xen/xen.h"
 
 ram_addr_t qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
-                                    const char *mem_path, Error **errp);
+                                    bool share, const char *mem_path,
+                                    Error **errp);
 ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
                                    MemoryRegion *mr);
 ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr);
memory.c +2 -1
index bcef72b..203b097 100644
@@ -1025,6 +1025,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
                                       struct Object *owner,
                                       const char *name,
                                       uint64_t size,
+                                      bool share,
                                       const char *path,
                                       Error **errp)
 {
@@ -1032,7 +1033,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
     mr->ram = true;
     mr->terminates = true;
     mr->destructor = memory_region_destructor_ram;
-    mr->ram_addr = qemu_ram_alloc_from_file(size, mr, path, errp);
+    mr->ram_addr = qemu_ram_alloc_from_file(size, mr, share, path, errp);
 }
 #endif
 
numa.c +1 -1
index 6ea267e..8ba5a38 100644
@@ -231,7 +231,7 @@ static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
     if (mem_path) {
 #ifdef __linux__
         Error *err = NULL;
-        memory_region_init_ram_from_file(mr, owner, name, ram_size,
+        memory_region_init_ram_from_file(mr, owner, name, ram_size, false,
                                          mem_path, &err);
 
         /* Legacy behavior: if allocation failed, fall back to
Signature · 1 line
1.9.3
author
#15 lore raw

PATCH v5 12/16 hostmem: add properties for NUMA memory policy

In reply to Hu Tao

[Raise errors on setting properties if !CONFIG_NUMA. Add BUILD_BUG_ON
checks. - Paolo]

---
 backends/hostmem.c       | 136 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/sysemu/hostmem.h |   4 ++
 qapi-schema.json         |  20 +++++++
 3 files changed, 159 insertions(+), 1 deletion(-)
backends/hostmem.c +135 -1
index ebef620..ca10c51 100644
@@ -10,12 +10,21 @@
  * See the COPYING file in the top-level directory.
  */
 #include "sysemu/hostmem.h"
-#include "sysemu/sysemu.h"
 #include "qapi/visitor.h"
+#include "qapi-types.h"
+#include "qapi-visit.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/config-file.h"
 #include "qom/object_interfaces.h"
 
+#ifdef CONFIG_NUMA
+#include <numaif.h>
+QEMU_BUILD_BUG_ON(HOST_MEM_POLICY_DEFAULT != MPOL_DEFAULT);
+QEMU_BUILD_BUG_ON(HOST_MEM_POLICY_PREFERRED != MPOL_PREFERRED);
+QEMU_BUILD_BUG_ON(HOST_MEM_POLICY_BIND != MPOL_BIND);
+QEMU_BUILD_BUG_ON(HOST_MEM_POLICY_INTERLEAVE != MPOL_INTERLEAVE);
+#endif
+
 static void
 host_memory_backend_get_size(Object *obj, Visitor *v, void *opaque,
                              const char *name, Error **errp)
@@ -53,6 +62,84 @@ out:
     error_propagate(errp, local_err);
 }
 
+static void
+host_memory_backend_get_host_nodes(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+    uint16List *host_nodes = NULL;
+    uint16List **node = &host_nodes;
+    unsigned long value;
+
+    value = find_first_bit(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(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
+host_memory_backend_set_host_nodes(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+#ifdef CONFIG_NUMA
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+    uint16List *l = NULL;
+
+    visit_type_uint16List(v, &l, name, errp);
+
+    while (l) {
+        bitmap_set(backend->host_nodes, l->value, 1);
+        l = l->next;
+    }
+#else
+    error_setg(errp, "NUMA node binding are not supported by this QEMU");
+#endif
+}
+
+static void
+host_memory_backend_get_policy(Object *obj, Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+    int policy = backend->policy;
+
+    visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp);
+}
+
+static void
+host_memory_backend_set_policy(Object *obj, Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+    int policy;
+
+    visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp);
+    backend->policy = policy;
+
+#ifndef CONFIG_NUMA
+    if (policy != HOST_MEM_POLICY_DEFAULT) {
+        error_setg(errp, "NUMA policies are not supported by this QEMU");
+    }
+#endif
+}
+
 static bool host_memory_backend_get_merge(Object *obj, Error **errp)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
@@ -162,6 +249,12 @@ static void host_memory_backend_init(Object *obj)
     object_property_add(obj, "size", "int",
                         host_memory_backend_get_size,
                         host_memory_backend_set_size, NULL, NULL, NULL);
+    object_property_add(obj, "host-nodes", "int",
+                        host_memory_backend_get_host_nodes,
+                        host_memory_backend_set_host_nodes, NULL, NULL, NULL);
+    object_property_add(obj, "policy", "str",
+                        host_memory_backend_get_policy,
+                        host_memory_backend_set_policy, NULL, NULL, NULL);
 }
 
 static void host_memory_backend_finalize(Object *obj)
@@ -204,6 +297,47 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
         if (!backend->dump) {
             qemu_madvise(ptr, sz, QEMU_MADV_DONTDUMP);
         }
+#ifdef CONFIG_NUMA
+        unsigned long lastbit = find_last_bit(backend->host_nodes, MAX_NODES);
+        /* lastbit == MAX_NODES means maxnode = 0 */
+        unsigned long maxnode = (lastbit + 1) % (MAX_NODES + 1);
+        /* ensure policy won't be ignored in case memory is preallocated
+         * before mbind(). note: MPOL_MF_STRICT is ignored on hugepages so
+         * this doesn't catch hugepage case. */
+        unsigned flags = MPOL_MF_STRICT;
+
+        /* check for invalid host-nodes and policies and give more verbose
+         * error messages than mbind(). */
+        if (maxnode && backend->policy == MPOL_DEFAULT) {
+            error_setg(errp, "host-nodes must be empty for policy default,"
+                       " or you should explicitly specify a policy other"
+                       " than default");
+            return;
+        } else if (maxnode == 0 && backend->policy != MPOL_DEFAULT) {
+            error_setg(errp, "host-nodes must be set for policy %s",
+                       HostMemPolicy_lookup[backend->policy]);
+            return;
+        }
+
+        /* We can have up to MAX_NODES nodes, but we need to pass maxnode+1
+         * as argument to mbind() due to an old Linux bug (feature?) which
+         * cuts off the last specified node. This means backend->host_nodes
+         * must have MAX_NODES+1 bits available.
+         */
+        assert(sizeof(backend->host_nodes) >=
+               BITS_TO_LONGS(MAX_NODES + 1) * sizeof(unsigned long));
+        assert(maxnode <= MAX_NODES);
+        if (mbind(ptr, sz, backend->policy,
+                  maxnode ? backend->host_nodes : NULL, maxnode + 1, flags)) {
+            error_setg_errno(errp, errno,
+                             "cannot bind memory to host NUMA nodes");
+            return;
+        }
+#endif
+        /* Preallocate memory after the NUMA policy has been instantiated.
+         * This is necessary to guarantee memory is allocated with
+         * specified NUMA policy in place.
+         */
         if (backend->prealloc) {
             os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz);
         }
include/sysemu/hostmem.h +4 -0
index 4cae673..1ce4394 100644
@@ -12,10 +12,12 @@
 #ifndef QEMU_RAM_H
 #define QEMU_RAM_H
 
+#include "sysemu/sysemu.h" /* for MAX_NODES */
 #include "qom/object.h"
 #include "qapi/error.h"
 #include "exec/memory.h"
 #include "qemu/option.h"
+#include "qemu/bitmap.h"
 
 #define TYPE_MEMORY_BACKEND "memory-backend"
 #define MEMORY_BACKEND(obj) \
@@ -54,6 +56,8 @@ struct HostMemoryBackend {
     uint64_t size;
     bool merge, dump;
     bool prealloc, force_prealloc;
+    DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
+    HostMemPolicy policy;
 
     MemoryRegion mr;
 };
qapi-schema.json +20 -0
index d5ab066..0898c00 100644
@@ -4759,3 +4759,23 @@
    '*cpus':   ['uint16'],
    '*mem':    'size',
    '*memdev': 'str' }}
+
+##
+# @HostMemPolicy
+#
+# Host memory policy types
+#
+# @default: restore default policy, remove any nondefault policy
+#
+# @preferred: set the preferred host nodes for allocation
+#
+# @bind: a strict policy that restricts memory allocation to the
+#        host nodes specified
+#
+# @interleave: memory allocations are interleaved across the set
+#              of host nodes specified
+#
+# Since 2.1
+##
+{ 'enum': 'HostMemPolicy',
+  'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
Signature · 1 line
1.9.3
author
#16 lore raw

PATCH v5 13/16 tests: fix memory leak in test of string input visitor

In reply to Hu Tao
---
 tests/test-string-input-visitor.c | 3 +++
 1 file changed, 3 insertions(+)
tests/test-string-input-visitor.c +3 -0
index 877e737..01a78f0 100644
@@ -193,6 +193,7 @@ static void test_visitor_in_fuzz(TestInputVisitorData *data,
 
         v = visitor_input_test_init(data, buf);
         visit_type_int(v, &ires, NULL, NULL);
+        visitor_input_teardown(data, NULL);
 
         v = visitor_input_test_init(data, buf);
         visit_type_bool(v, &bres, NULL, NULL);
@@ -200,11 +201,13 @@ static void test_visitor_in_fuzz(TestInputVisitorData *data,
 
         v = visitor_input_test_init(data, buf);
         visit_type_number(v, &nres, NULL, NULL);
+        visitor_input_teardown(data, NULL);
 
         v = visitor_input_test_init(data, buf);
         sres = NULL;
         visit_type_str(v, &sres, NULL, NULL);
         g_free(sres);
+        visitor_input_teardown(data, NULL);
 
         v = visitor_input_test_init(data, buf);
         visit_type_EnumOne(v, &eres, NULL, NULL);
Signature · 1 line
1.9.3
author
#17 lore raw

PATCH v5 14/16 qapi: make string input visitor parse int list

In reply to Hu Tao
---
 qapi/string-input-visitor.c       | 181 ++++++++++++++++++++++++++++++++++++--
 tests/test-string-input-visitor.c |  36 ++++++++
 2 files changed, 209 insertions(+), 8 deletions(-)
qapi/string-input-visitor.c +173 -8
index 5780944..85ac6a1 100644
@@ -15,31 +15,182 @@
 #include "qapi/visitor-impl.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/option.h"
+#include "qemu/queue.h"
+#include "qemu/range.h"
+
 
 struct StringInputVisitor
 {
     Visitor visitor;
+
+    bool head;
+
+    SignedRangeList *ranges;
+    SignedRange *cur_range;
+    int64_t cur;
+
     const char *string;
 };
 
+static void parse_str(StringInputVisitor *siv, Error **errp)
+{
+    char *str = (char *) siv->string;
+    long long start, end;
+    SignedRange *r, *next;
+    char *endptr;
+
+    if (siv->ranges) {
+        return;
+    }
+
+    siv->ranges = g_malloc0(sizeof(*siv->ranges));
+    QTAILQ_INIT(siv->ranges);
+    errno = 0;
+    do {
+        start = strtoll(str, &endptr, 0);
+        if (errno == 0 && endptr > str && INT64_MIN <= start &&
+            start <= INT64_MAX) {
+            if (*endptr == '\0') {
+                if (!range_list_add(siv->ranges, start, 1)) {
+                    goto error;
+                }
+                str = NULL;
+            } else if (*endptr == '-') {
+                str = endptr + 1;
+                end = strtoll(str, &endptr, 0);
+                if (errno == 0 && endptr > str &&
+                    INT64_MIN <= end && end <= INT64_MAX && start <= end &&
+                    (start > INT64_MAX - 65536 ||
+                     end < start + 65536)) {
+                    if (*endptr == '\0') {
+                        if (!range_list_add(siv->ranges, start,
+                                            end - start + 1)) {
+                            goto error;
+                        }
+                        str = NULL;
+                    } else if (*endptr == ',') {
+                        str = endptr + 1;
+                        if (!range_list_add(siv->ranges, start,
+                                            end - start + 1)) {
+                            goto error;
+                        }
+                    } else {
+                        goto error;
+                    }
+                } else {
+                    goto error;
+                }
+            } else if (*endptr == ',') {
+                str = endptr + 1;
+                if (!range_list_add(siv->ranges, start, 1)) {
+                    goto error;
+                }
+            } else {
+                goto error;
+            }
+        } else {
+            goto error;
+        }
+    } while (str);
+
+    return;
+error:
+    if (siv->ranges) {
+        QTAILQ_FOREACH_SAFE(r, siv->ranges, entry, next) {
+            QTAILQ_REMOVE(siv->ranges, r, entry);
+            g_free(r);
+        }
+        g_free(siv->ranges);
+        siv->ranges = NULL;
+    }
+}
+
+static void
+start_list(Visitor *v, const char *name, Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+
+    parse_str(siv, errp);
+
+    if (siv->ranges) {
+        siv->cur_range = QTAILQ_FIRST(siv->ranges);
+        if (siv->cur_range) {
+            siv->cur = siv->cur_range->start;
+        }
+    }
+}
+
+static GenericList *
+next_list(Visitor *v, GenericList **list, Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    GenericList **link;
+
+    if (!siv->ranges || !siv->cur_range) {
+        return NULL;
+    }
+
+    if (siv->cur < siv->cur_range->start ||
+        siv->cur >= (siv->cur_range->start + siv->cur_range->length)) {
+        siv->cur_range = QTAILQ_NEXT(siv->cur_range, entry);
+        if (siv->cur_range) {
+            siv->cur = siv->cur_range->start;
+        } else {
+            return NULL;
+        }
+    }
+
+    if (siv->head) {
+        link = list;
+        siv->head = false;
+    } else {
+        link = &(*list)->next;
+    }
+
+    *link = g_malloc0(sizeof **link);
+    return *link;
+}
+
+static void
+end_list(Visitor *v, Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    siv->head = true;
+}
+
 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;
-    long long val;
 
-    errno = 0;
-    if (siv->string) {
-        val = strtoll(siv->string, &endp, 0);
-    }
-    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;
+    parse_str(siv, errp);
+
+    if (!siv->ranges) {
+        goto error;
+    }
+
+    if (!siv->cur_range) {
+        siv->cur_range = QTAILQ_FIRST(siv->ranges);
+        if (siv->cur_range) {
+            siv->cur = siv->cur_range->start;
+        } else {
+            goto error;
+        }
+    }
+
+    *obj = siv->cur;
+    siv->cur++;
+    return;
+
+error:
+    error_set(errp, QERR_INVALID_PARAMETER_VALUE, name,
+              "an int64 value or range");
 }
 
 static void parse_type_size(Visitor *v, uint64_t *obj, const char *name,
@@ -140,6 +291,16 @@ Visitor *string_input_get_visitor(StringInputVisitor *v)
 
 void string_input_visitor_cleanup(StringInputVisitor *v)
 {
+    SignedRange *r, *next;
+
+    if (v->ranges) {
+        QTAILQ_FOREACH_SAFE(r, v->ranges, entry, next) {
+            QTAILQ_REMOVE(v->ranges, r, entry);
+            g_free(r);
+        }
+        g_free(v->ranges);
+    }
+
     g_free(v);
 }
 
@@ -155,8 +316,12 @@ 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.start_list = start_list;
+    v->visitor.next_list = next_list;
+    v->visitor.end_list = end_list;
     v->visitor.optional = parse_optional;
 
     v->string = str;
+    v->head = true;
     return v;
 }
tests/test-string-input-visitor.c +36 -0
index 01a78f0..b08a7db 100644
@@ -64,6 +64,35 @@ static void test_visitor_in_int(TestInputVisitorData *data,
     g_assert_cmpint(res, ==, value);
 }
 
+static void test_visitor_in_intList(TestInputVisitorData *data,
+                                    const void *unused)
+{
+    int64_t value[] = {-2, -1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
+    int16List *res = NULL, *tmp;
+    Error *errp = NULL;
+    Visitor *v;
+    int i = 0;
+
+    v = visitor_input_test_init(data, "1,2,-2-1,2-4,20,5-9,1-8");
+
+    visit_type_int16List(v, &res, NULL, &errp);
+    g_assert(errp == NULL);
+    tmp = res;
+    while (i < sizeof(value) / sizeof(value[0])) {
+        g_assert(tmp);
+        g_assert_cmpint(tmp->value, ==, value[i++]);
+        tmp = tmp->next;
+    }
+    g_assert(!tmp);
+
+    tmp = res;
+    while (tmp) {
+        res = res->next;
+        g_free(tmp);
+        tmp = res;
+    }
+}
+
 static void test_visitor_in_bool(TestInputVisitorData *data,
                                  const void *unused)
 {
@@ -170,6 +199,7 @@ static void test_visitor_in_fuzz(TestInputVisitorData *data,
                                  const void *unused)
 {
     int64_t ires;
+    intList *ilres;
     bool bres;
     double nres;
     char *sres;
@@ -196,6 +226,10 @@ static void test_visitor_in_fuzz(TestInputVisitorData *data,
         visitor_input_teardown(data, NULL);
 
         v = visitor_input_test_init(data, buf);
+        visit_type_intList(v, &ilres, NULL, NULL);
+        visitor_input_teardown(data, NULL);
+
+        v = visitor_input_test_init(data, buf);
         visit_type_bool(v, &bres, NULL, NULL);
         visitor_input_teardown(data, NULL);
 
@@ -231,6 +265,8 @@ int main(int argc, char **argv)
 
     input_visitor_test_add("/string-visitor/input/int",
                            &in_visitor_data, test_visitor_in_int);
+    input_visitor_test_add("/string-visitor/input/intList",
+                           &in_visitor_data, test_visitor_in_intList);
     input_visitor_test_add("/string-visitor/input/bool",
                            &in_visitor_data, test_visitor_in_bool);
     input_visitor_test_add("/string-visitor/input/number",
Signature · 1 line
1.9.3
author
#18 lore raw

PATCH v5 15/16 qapi: make string output visitor parse int list

In reply to Hu Tao
---
 qapi/string-output-visitor.c       | 230 +++++++++++++++++++++++++++++++++++--
 tests/test-string-output-visitor.c |  34 ++++++
 2 files changed, 254 insertions(+), 10 deletions(-)
qapi/string-output-visitor.c +220 -10
index fb1d2e8..ccebc7a 100644
@@ -16,32 +16,173 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/host-utils.h"
 #include <math.h>
+#include "qemu/range.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 StringOutputVisitor
 {
     Visitor visitor;
     bool human;
-    char *string;
+    GString *string;
+    bool head;
+    ListMode list_mode;
+    union {
+        int64_t s;
+        uint64_t u;
+    } range_start, range_end;
+    SignedRangeList *ranges;
 };
 
 static void string_output_set(StringOutputVisitor *sov, char *string)
 {
-    g_free(sov->string);
-    sov->string = string;
+    if (sov->string) {
+        g_string_free(sov->string, true);
+    }
+    sov->string = g_string_new(string);
+    g_free(string);
+}
+
+static void string_output_append(StringOutputVisitor *sov, int64_t a)
+{
+    range_list_add(sov->ranges, a, 1);
+}
+
+static void string_output_append_range(StringOutputVisitor *sov,
+                                       int64_t s, int64_t e)
+{
+    range_list_add(sov->ranges, s, e);
 }
 
 static void print_type_int(Visitor *v, int64_t *obj, const char *name,
                            Error **errp)
 {
     StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
-    char *out;
+    SignedRange *r;
+
+    if (!sov->ranges) {
+        sov->ranges = g_malloc0(sizeof(*sov->ranges));
+        QTAILQ_INIT(sov->ranges);
+    }
+
+    switch (sov->list_mode) {
+    case LM_NONE:
+        string_output_append(sov, *obj);
+        break;
+
+    case LM_STARTED:
+        sov->range_start.s = *obj;
+        sov->range_end.s = *obj;
+        sov->list_mode = LM_IN_PROGRESS;
+        return;
+
+    case LM_IN_PROGRESS:
+        if (sov->range_end.s + 1 == *obj) {
+            sov->range_end.s++;
+        } else {
+            if (sov->range_start.s == sov->range_end.s) {
+                string_output_append(sov, sov->range_end.s);
+            } else {
+                assert(sov->range_start.s < sov->range_end.s);
+                string_output_append_range(sov, sov->range_start.s,
+                                           sov->range_end.s -
+                                           sov->range_start.s + 1);
+            }
+
+            sov->range_start.s = *obj;
+            sov->range_end.s = *obj;
+        }
+        return;
+
+    case LM_END:
+        if (sov->range_end.s + 1 == *obj) {
+            sov->range_end.s++;
+            assert(sov->range_start.s < sov->range_end.s);
+            string_output_append_range(sov, sov->range_start.s,
+                                       sov->range_end.s -
+                                       sov->range_start.s + 1);
+        } else {
+            if (sov->range_start.s == sov->range_end.s) {
+                string_output_append(sov, sov->range_end.s);
+            } else {
+                assert(sov->range_start.s < sov->range_end.s);
+
+                string_output_append_range(sov, sov->range_start.s,
+                                           sov->range_end.s -
+                                           sov->range_start.s + 1);
+            }
+            string_output_append(sov, *obj);
+        }
+        break;
+
+    default:
+        abort();
+    }
+
+    QTAILQ_FOREACH(r, sov->ranges, entry) {
+        if (r->length > 1) {
+            g_string_append_printf(sov->string, "%" PRId64 "-%" PRId64,
+                                   r->start,
+                                   s_range_end(r->start, r->length));
+        } else {
+            g_string_append_printf(sov->string, "%" PRId64,
+                                   r->start);
+        }
+        if (r->entry.tqe_next) {
+            g_string_append(sov->string, ",");
+        }
+    }
 
     if (sov->human) {
-        out = g_strdup_printf("%lld (%#llx)", (long long) *obj, (long long) *obj);
-    } else {
-        out = g_strdup_printf("%lld", (long long) *obj);
+        g_string_append(sov->string, " (");
+        QTAILQ_FOREACH(r, sov->ranges, entry) {
+            if (r->length > 1) {
+                g_string_append_printf(sov->string, "%" PRIx64 "-%" PRIx64,
+                                       r->start,
+                                       s_range_end(r->start, r->length));
+            } else {
+                g_string_append_printf(sov->string, "%" PRIx64,
+                                       r->start);
+            }
+            if (r->entry.tqe_next) {
+                g_string_append(sov->string, ",");
+            }
+        }
+        g_string_append(sov->string, ")");
     }
-    string_output_set(sov, out);
 }
 
 static void print_type_size(Visitor *v, uint64_t *obj, const char *name,
@@ -103,9 +244,61 @@ static void print_type_number(Visitor *v, double *obj, const char *name,
     string_output_set(sov, g_strdup_printf("%f", *obj));
 }
 
+static void
+start_list(Visitor *v, const char *name, Error **errp)
+{
+    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
+
+    /* we can't traverse a list in a list */
+    assert(sov->list_mode == LM_NONE);
+    sov->list_mode = LM_STARTED;
+    sov->head = true;
+}
+
+static GenericList *
+next_list(Visitor *v, GenericList **list, Error **errp)
+{
+    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
+    GenericList *ret = NULL;
+    if (*list) {
+        if (sov->head) {
+            ret = *list;
+        } else {
+            ret = (*list)->next;
+        }
+
+        if (sov->head) {
+            if (ret && ret->next == NULL) {
+                sov->list_mode = LM_NONE;
+            }
+            sov->head = false;
+        } else {
+            if (ret && ret->next == NULL) {
+                sov->list_mode = LM_END;
+            }
+        }
+    }
+
+    return ret;
+}
+
+static void
+end_list(Visitor *v, Error **errp)
+{
+    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
+
+    assert(sov->list_mode == LM_STARTED ||
+           sov->list_mode == LM_END ||
+           sov->list_mode == LM_NONE ||
+           sov->list_mode == LM_IN_PROGRESS);
+    sov->list_mode = LM_NONE;
+    sov->head = true;
+
+}
+
 char *string_output_get_string(StringOutputVisitor *sov)
 {
-    char *string = sov->string;
+    char *string = g_string_free(sov->string, false);
     sov->string = NULL;
     return string;
 }
@@ -117,7 +310,20 @@ Visitor *string_output_get_visitor(StringOutputVisitor *sov)
 
 void string_output_visitor_cleanup(StringOutputVisitor *sov)
 {
-    g_free(sov->string);
+    SignedRange *r, *next;
+
+    if (sov->string) {
+        g_string_free(sov->string, true);
+    }
+
+    if (sov->ranges) {
+        QTAILQ_FOREACH_SAFE(r, sov->ranges, entry, next) {
+            QTAILQ_REMOVE(sov->ranges, r, entry);
+            s_range_free(r);
+        }
+        g_free(sov->ranges);
+    }
+
     g_free(sov);
 }
 
@@ -127,6 +333,7 @@ StringOutputVisitor *string_output_visitor_new(bool human)
 
     v = g_malloc0(sizeof(*v));
 
+    v->string = g_string_new(NULL);
     v->human = human;
     v->visitor.type_enum = output_type_enum;
     v->visitor.type_int = print_type_int;
@@ -134,6 +341,9 @@ StringOutputVisitor *string_output_visitor_new(bool human)
     v->visitor.type_bool = print_type_bool;
     v->visitor.type_str = print_type_str;
     v->visitor.type_number = print_type_number;
+    v->visitor.start_list = start_list;
+    v->visitor.next_list = next_list;
+    v->visitor.end_list = end_list;
 
     return v;
 }
tests/test-string-output-visitor.c +34 -0
index 2af5a21..d2ad580 100644
@@ -57,6 +57,38 @@ static void test_visitor_out_int(TestOutputVisitorData *data,
     g_free(str);
 }
 
+static void test_visitor_out_intList(TestOutputVisitorData *data,
+                                     const void *unused)
+{
+    int64_t value[] = {-10, -7, -2, -1, 0, 1, 9, 10, 16, 15, 14,
+        3, 4, 5, 6, 11, 12, 13, 21, 22, INT64_MAX - 1, INT64_MAX};
+    intList *list = NULL, **tmp = &list;
+    int i;
+    Error *errp = NULL;
+    char *str;
+
+    for (i = 0; i < sizeof(value) / sizeof(value[0]); i++) {
+        *tmp = g_malloc0(sizeof(**tmp));
+        (*tmp)->value = value[i];
+        tmp = &(*tmp)->next;
+    }
+
+    visit_type_intList(data->ov, &list, NULL, &errp);
+    g_assert(errp == NULL);
+
+    str = string_output_get_string(data->sov);
+    g_assert(str != NULL);
+    g_assert_cmpstr(str, ==,
+        "-10,-7,-2-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807");
+    g_free(str);
+    while (list) {
+        intList *tmp2;
+        tmp2 = list->next;
+        g_free(list);
+        list = tmp2;
+    }
+}
+
 static void test_visitor_out_bool(TestOutputVisitorData *data,
                                   const void *unused)
 {
@@ -182,6 +214,8 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_enum);
     output_visitor_test_add("/string-visitor/output/enum-errors",
                             &out_visitor_data, test_visitor_out_enum_errors);
+    output_visitor_test_add("/string-visitor/output/intList",
+                            &out_visitor_data, test_visitor_out_intList);
 
     g_test_run();
 
Signature · 1 line
1.9.3
author
#19 lore raw

PATCH v5 16/16 qmp: add query-memdev

In reply to Hu Tao

Add qmp command query-memdev to query for information of memory devices

---
 numa.c           | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 40 +++++++++++++++++++++++++++++++
 qmp-commands.hx  | 38 ++++++++++++++++++++++++++++++
 3 files changed, 150 insertions(+)
numa.c +72 -0
index 8ba5a38..ce9382d 100644
@@ -31,9 +31,14 @@
 #include "qapi-visit.h"
 #include "qapi/opts-visitor.h"
 #include "qapi/dealloc-visitor.h"
+#include "qapi/qmp-output-visitor.h"
+#include "qapi/qmp-input-visitor.h"
+#include "qapi/string-output-visitor.h"
+#include "qapi/string-input-visitor.h"
 #include "qapi/qmp/qerror.h"
 #include "hw/boards.h"
 #include "sysemu/hostmem.h"
+#include "qmp-commands.h"
 
 QemuOptsList qemu_numa_opts = {
     .name = "numa",
@@ -280,3 +285,70 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
         addr += size;
     }
 }
+
+MemdevList *qmp_query_memdev(Error **errp)
+{
+    MemdevList *list = NULL, *m;
+    HostMemoryBackend *backend;
+    Error *err = NULL;
+    int i;
+
+    for (i = 0; i < nb_numa_nodes; i++) {
+        backend = numa_info[i].node_memdev;
+
+        m = g_malloc0(sizeof(*m));
+        m->value = g_malloc0(sizeof(*m->value));
+        m->value->size = object_property_get_int(OBJECT(backend), "size",
+                                                 &err);
+        if (err) {
+            goto error;
+        }
+
+        m->value->merge = object_property_get_bool(OBJECT(backend), "merge",
+                                                   &err);
+        if (err) {
+            goto error;
+        }
+
+        m->value->dump = object_property_get_bool(OBJECT(backend), "dump",
+                                                  &err);
+        if (err) {
+            goto error;
+        }
+
+        m->value->prealloc = object_property_get_bool(OBJECT(backend),
+                                                      "prealloc", &err);
+        if (err) {
+            goto error;
+        }
+
+        m->value->policy = object_property_get_enum(OBJECT(backend),
+                                                    "policy",
+                                                    HostMemPolicy_lookup,
+                                                    &err);
+        if (err) {
+            goto error;
+        }
+
+        object_property_get_uint16List(OBJECT(backend), "host-nodes",
+                                       &m->value->host_nodes, &err);
+        if (err) {
+            goto error;
+        }
+
+        m->next = list;
+        list = m;
+    }
+
+    return list;
+
+error:
+    while (list) {
+        m = list;
+        list = list->next;
+        g_free(m->value);
+        g_free(m);
+    }
+    qerror_report_err(err);
+    return NULL;
+}
qapi-schema.json +40 -0
index 0898c00..c3eafcf 100644
@@ -4779,3 +4779,43 @@
 ##
 { 'enum': 'HostMemPolicy',
   'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
+
+##
+# @Memdev:
+#
+# Information of memory device
+#
+# @size: memory device size
+#
+# @merge: enables or disables memory merge support
+#
+# @dump: includes memory device's memory in a core dump or not
+#
+# @prealloc: enables or disables memory preallocation
+#
+# @host-nodes: host nodes for its memory policy
+#
+# @policy: memory policy of memory device
+#
+# Since: 2.1
+##
+
+{ 'type': 'Memdev',
+  'data': {
+    'size':       'size',
+    'merge':      'bool',
+    'dump':       'bool',
+    'prealloc':   'bool',
+    'host-nodes': ['uint16'],
+    'policy':     'HostMemPolicy' }}
+
+##
+# @query-memdev:
+#
+# Returns information for all memory devices.
+#
+# Returns: a list of @Memdev.
+#
+# Since: 2.1
+##
+{ 'command': 'query-memdev', 'returns': ['Memdev'] }
qmp-commands.hx +38 -0
index d8aa4ed..bb34cd8 100644
@@ -3572,3 +3572,41 @@ Example:
                    } } ] }
 
 EQMP
+
+    {
+        .name       = "query-memdev",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_memdev,
+    },
+
+SQMP
+query-memdev
+------------
+
+Show memory devices information.
+
+
+Example (1):
+
+-> { "execute": "query-memdev" }
+<- { "return": [
+       {
+         "size": 536870912,
+         "merge": false,
+         "dump": true,
+         "prealloc": false,
+         "host-nodes": [0, 1],
+         "policy": "bind"
+       },
+       {
+         "size": 536870912,
+         "merge": false,
+         "dump": true,
+         "prealloc": true,
+         "host-nodes": [2, 3],
+         "policy": "preferred"
+       }
+     ]
+   }
+
+EQMP
Signature · 1 line
1.9.3
#20 lore raw
In reply to Hu Tao
Quoted 4 lines · Hu Tao

note: based on MST's numa tree.

changes are error messages and docs tweaks to address comments to v4.

Applied, numa tree applied to the pci tree as well.
Two points I would like to see addressed before it's going upstream:
- please validate and test the pci branch
there have been lots of changes
- please work to drop the SignedRange: for all current purposes existing Range is sufficient, instead of a list and sort re-implementation, it is better to use g_list_insert_sorted

Thanks everyone.

Quoted 52 lines

Hu Tao (7):
backend:hostmem: replace hostmemory with host_memory
hostmem: separate allocation from UserCreatable complete method
hostmem: add properties for NUMA memory policy
tests: fix memory leak in test of string input visitor
qapi: make string input visitor parse int list
qapi: make string output visitor parse int list
qmp: add query-memdev

Paolo Bonzini (8):
vl: redo -object parsing
fixup! qmp: improve error reporting for -object and object-add
pc: pass MachineState to pc_memory_init
fixup! numa: add -numa node,memdev= option
hostmem: add file-based HostMemoryBackend
hostmem: add merge and dump properties
hostmem: allow preallocation of any memory region
hostmem: add property to map memory with MAP_SHARED

Wanlong Gao (1):
fixup! NUMA: check if the total numa memory size is equal to ram_size

 backends/Makefile.objs             |   1 +
 backends/hostmem-file.c            | 134 ++++++++++++++++
 backends/hostmem-ram.c             |   7 +-
 backends/hostmem.c                 | 304 +++++++++++++++++++++++++++++++++++--
 exec.c                             |  25 ++-
 hw/i386/pc.c                       |  24 +--
 hw/i386/pc_piix.c                  |   8 +-
 hw/i386/pc_q35.c                   |   4 +-
 include/exec/memory.h              |  12 ++
 include/exec/ram_addr.h            |   4 +-
 include/hw/i386/pc.h               |   7 +-
 include/qemu/osdep.h               |  10 ++
 include/sysemu/hostmem.h           |   8 +
 memory.c                           |  14 +-
 numa.c                             |  78 +++++++++-
 qapi-schema.json                   |  60 ++++++++
 qapi/string-input-visitor.c        | 181 +++++++++++++++++++++-
 qapi/string-output-visitor.c       | 230 ++++++++++++++++++++++++++--
 qemu-options.hx                    |   6 +-
 qmp-commands.hx                    |  38 +++++
 qmp.c                              |   3 +-
 tests/test-string-input-visitor.c  |  39 +++++
 tests/test-string-output-visitor.c |  34 +++++
 vl.c                               |  65 ++++----
 24 files changed, 1191 insertions(+), 105 deletions(-)
 create mode 100644 backends/hostmem-file.c
Signature · 1 line
1.9.3
author
#21 lore raw
Quoted 6 lines · Michael S. Tsirkin
Quoted 4 lines · Hu Tao

note: based on MST's numa tree.

changes are error messages and docs tweaks to address comments to v4.

Quoted 8 lines

Applied, numa tree applied to the pci tree as well.
Two points I would like to see addressed before it's going upstream:
- please validate and test the pci branch
there have been lots of changes
- please work to drop the SignedRange: for all current purposes existing Range is sufficient, instead of a list and sort re-implementation, it is better to use g_list_insert_sorted

I see. Thanks for your work!

Quoted 55 lines

Thanks everyone.

Quoted 52 lines

Hu Tao (7):
backend:hostmem: replace hostmemory with host_memory
hostmem: separate allocation from UserCreatable complete method
hostmem: add properties for NUMA memory policy
tests: fix memory leak in test of string input visitor
qapi: make string input visitor parse int list
qapi: make string output visitor parse int list
qmp: add query-memdev

Paolo Bonzini (8):
vl: redo -object parsing
fixup! qmp: improve error reporting for -object and object-add
pc: pass MachineState to pc_memory_init
fixup! numa: add -numa node,memdev= option
hostmem: add file-based HostMemoryBackend
hostmem: add merge and dump properties
hostmem: allow preallocation of any memory region
hostmem: add property to map memory with MAP_SHARED

Wanlong Gao (1):
fixup! NUMA: check if the total numa memory size is equal to ram_size

 backends/Makefile.objs             |   1 +
 backends/hostmem-file.c            | 134 ++++++++++++++++
 backends/hostmem-ram.c             |   7 +-
 backends/hostmem.c                 | 304 +++++++++++++++++++++++++++++++++++--
 exec.c                             |  25 ++-
 hw/i386/pc.c                       |  24 +--
 hw/i386/pc_piix.c                  |   8 +-
 hw/i386/pc_q35.c                   |   4 +-
 include/exec/memory.h              |  12 ++
 include/exec/ram_addr.h            |   4 +-
 include/hw/i386/pc.h               |   7 +-
 include/qemu/osdep.h               |  10 ++
 include/sysemu/hostmem.h           |   8 +
 memory.c                           |  14 +-
 numa.c                             |  78 +++++++++-
 qapi-schema.json                   |  60 ++++++++
 qapi/string-input-visitor.c        | 181 +++++++++++++++++++++-
 qapi/string-output-visitor.c       | 230 ++++++++++++++++++++++++++--
 qemu-options.hx                    |   6 +-
 qmp-commands.hx                    |  38 +++++
 qmp.c                              |   3 +-
 tests/test-string-input-visitor.c  |  39 +++++
 tests/test-string-output-visitor.c |  34 +++++
 vl.c                               |  65 ++++----
 24 files changed, 1191 insertions(+), 105 deletions(-)
 create mode 100644 backends/hostmem-file.c
Signature · 1 line
1.9.3
#22 lore raw
Quoted 16 lines · Hu Tao
Quoted 6 lines · Michael S. Tsirkin
Quoted 4 lines · Hu Tao

note: based on MST's numa tree.

changes are error messages and docs tweaks to address comments to v4.

Quoted 8 lines

Applied, numa tree applied to the pci tree as well.
Two points I would like to see addressed before it's going upstream:
- please validate and test the pci branch
there have been lots of changes
- please work to drop the SignedRange: for all current purposes existing Range is sufficient, instead of a list and sort re-implementation, it is better to use g_list_insert_sorted

Quoted 1 line

I see. Thanks for your work!

Could you confirm you'll be able to fix these by Monday? I'd like to send the pull request then.

Quoted 55 lines
Quoted 55 lines

Thanks everyone.

Quoted 52 lines

Hu Tao (7):
backend:hostmem: replace hostmemory with host_memory
hostmem: separate allocation from UserCreatable complete method
hostmem: add properties for NUMA memory policy
tests: fix memory leak in test of string input visitor
qapi: make string input visitor parse int list
qapi: make string output visitor parse int list
qmp: add query-memdev

Paolo Bonzini (8):
vl: redo -object parsing
fixup! qmp: improve error reporting for -object and object-add
pc: pass MachineState to pc_memory_init
fixup! numa: add -numa node,memdev= option
hostmem: add file-based HostMemoryBackend
hostmem: add merge and dump properties
hostmem: allow preallocation of any memory region
hostmem: add property to map memory with MAP_SHARED

Wanlong Gao (1):
fixup! NUMA: check if the total numa memory size is equal to ram_size

 backends/Makefile.objs             |   1 +
 backends/hostmem-file.c            | 134 ++++++++++++++++
 backends/hostmem-ram.c             |   7 +-
 backends/hostmem.c                 | 304 +++++++++++++++++++++++++++++++++++--
 exec.c                             |  25 ++-
 hw/i386/pc.c                       |  24 +--
 hw/i386/pc_piix.c                  |   8 +-
 hw/i386/pc_q35.c                   |   4 +-
 include/exec/memory.h              |  12 ++
 include/exec/ram_addr.h            |   4 +-
 include/hw/i386/pc.h               |   7 +-
 include/qemu/osdep.h               |  10 ++
 include/sysemu/hostmem.h           |   8 +
 memory.c                           |  14 +-
 numa.c                             |  78 +++++++++-
 qapi-schema.json                   |  60 ++++++++
 qapi/string-input-visitor.c        | 181 +++++++++++++++++++++-
 qapi/string-output-visitor.c       | 230 ++++++++++++++++++++++++++--
 qemu-options.hx                    |   6 +-
 qmp-commands.hx                    |  38 +++++
 qmp.c                              |   3 +-
 tests/test-string-input-visitor.c  |  39 +++++
 tests/test-string-output-visitor.c |  34 +++++
 vl.c                               |  65 ++++----
 24 files changed, 1191 insertions(+), 105 deletions(-)
 create mode 100644 backends/hostmem-file.c
Signature · 1 line
1.9.3
#23 lore raw

Il 13/06/2014 10:03, Michael S. Tsirkin ha scritto:

Quoted 11 lines
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines

Two points I would like to see addressed before it's going upstream:
- please validate and test the pci branch
there have been lots of changes
- please work to drop the SignedRange: for all current purposes existing Range is sufficient, instead of a list and sort re-implementation, it is better to use g_list_insert_sorted

I see. Thanks for your work!

Could you confirm you'll be able to fix these by Monday? I'd like to send the pull request then.

I think SignedRange can be dealt with after the merge.

Paolo

#24 lore raw
Quoted 9 lines · Paolo Bonzini

Il 13/06/2014 10:03, Michael S. Tsirkin ha scritto:

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines

Two points I would like to see addressed before it's going upstream:
- please validate and test the pci branch
there have been lots of changes
- please work to drop the SignedRange: for all current purposes existing Range is sufficient, instead of a list and sort re-implementation, it is better to use g_list_insert_sorted

Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line

I see. Thanks for your work!

Could you confirm you'll be able to fix these by Monday? I'd like to send the pull request then.

I think SignedRange can be dealt with after the merge.

Paolo

Could be, but let's not go there, cleaning this up isn't hard.

Signature · 1 line
MST
author
#25 lore raw
In reply to Paolo Bonzini
Quoted 9 lines · Paolo Bonzini

Il 13/06/2014 10:03, Michael S. Tsirkin ha scritto:

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines

Two points I would like to see addressed before it's going upstream:
- please validate and test the pci branch
there have been lots of changes
- please work to drop the SignedRange: for all current purposes existing Range is sufficient, instead of a list and sort re-implementation, it is better to use g_list_insert_sorted

Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line

I see. Thanks for your work!

Could you confirm you'll be able to fix these by Monday? I'd like to send the pull request then.

I think SignedRange can be dealt with after the merge.

Paolo

I agree. Using existing Range makes string output visitor don't support negative values which it does support before SignedRange.

Besides SignedRange, tests show that there are several problems with the pci tree(core dumps, unexpecting exit of qemu).

Hu

#26 lore raw
Quoted 14 lines · Hu Tao
Quoted 13 lines · Paolo Bonzini

Il 13/06/2014 10:03, Michael S. Tsirkin ha scritto:

Quoted 11 lines
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines

Two points I would like to see addressed before it's going upstream:
- please validate and test the pci branch
there have been lots of changes
- please work to drop the SignedRange: for all current purposes existing Range is sufficient, instead of a list and sort re-implementation, it is better to use g_list_insert_sorted

I see. Thanks for your work!

Could you confirm you'll be able to fix these by Monday? I'd like to send the pull request then.

Quoted 6 lines
Quoted 3 lines

I think SignedRange can be dealt with after the merge.

Paolo

I agree. Using existing Range makes string output visitor don't support negative values which it does support before SignedRange.

Are there any users though? I think it's more a bug than a feature. Also we do want to get rid of rewriting g_list_insert_sorted though, that's for sure.

Quoted 4 lines

Besides SignedRange, tests show that there are several problems with the pci tree(core dumps, unexpecting exit of qemu).

Hu

OK pls share this info, also pls bisect if possible.

Thanks!

Signature · 1 line
MST
author
#27 lore raw

cover fixes for pci tree

Michael,

This is fixes for your pci tree.

patch 1 remove signed range as requested.

There are 3 problems in current pci tree, as follows:

1. pc-dimm specified on command line but only -m size (aka not -m size,maxmem,slots)

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -object memory-backend-ram,size=512M,id=ram-node0,prealloc=y,policy=bind,host-nodes=0 -device pc-dimm,id=d0,memdev=ram-node0 -m 640M -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm

result:

qemu/hw/mem/pc-dimm.c:110: pc_dimm_get_free_addr: Assertion `address_space_end > address_space_size' failed. Aborted (core dumped)

patch 2 fixes this.

2. using qemu monitor command object-add to add a memory-backend-ram object whose's size is too big

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -m 512M -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm

in monitor:
(qemu)object_add memory-backend-ram,size=40960G,id=mem0

result:

qemu just exits with message: Cannot set up guest memory 'mem0': Cannot allocate memory

patch 3 fixes this.

3. specifying a non-existing directory for memory-backend-file

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -m 512M,maxmem=1000G,slots=100 -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm -object memory-backend-file,size=512M,id=mem0,mem-path=/nonexistingdir -device pc-dimm,id=d0,memdev=mem0

result:

/nonexistingdir: No such file or directory
Bad ram offset fffffffffffff000
Aborted (core dumped)

patch 4 fixes this.

please review. Thanks!

Hu Tao (4):
get rid of signed range
check if we have space left for hotplugged memory
exec: don't exit unconditionally if failed to allocate memory
memory-backend-file: error out if failed to allocate memory

 backends/hostmem-file.c            |   3 +
 backends/hostmem-ram.c             |   3 +
 exec.c                             |   6 +-
 hw/mem/pc-dimm.c                   |   7 +-
 include/qemu/range.h               | 144 ++++++++++++-------------------------
 qapi/string-input-visitor.c        | 116 +++++++++++++++++-------------
 qapi/string-output-visitor.c       |  97 +++++++++++++------------
 tests/test-string-input-visitor.c  |   4 +-
 tests/test-string-output-visitor.c |   8 +--
 9 files changed, 182 insertions(+), 206 deletions(-)
Signature · 1 line
1.9.3
author
#28 lore raw

RFC PATCH 1/4 get rid of signed range

---
 include/qemu/range.h               | 144 ++++++++++++-------------------------
 qapi/string-input-visitor.c        | 116 +++++++++++++++++-------------
 qapi/string-output-visitor.c       |  97 +++++++++++++------------
 tests/test-string-input-visitor.c  |   4 +-
 tests/test-string-output-visitor.c |   8 +--
 5 files changed, 165 insertions(+), 204 deletions(-)
include/qemu/range.h +46 -98
index 8879f8a..cfa021f 100644
@@ -61,127 +61,75 @@ static inline int ranges_overlap(uint64_t first1, uint64_t len1,
     return !(last2 < first1 || last1 < first2);
 }
 
-typedef struct SignedRangeList SignedRangeList;
-
-typedef struct SignedRange {
-    int64_t start;
-    int64_t length;
-
-    QTAILQ_ENTRY(SignedRange) entry;
-} SignedRange;
-
-QTAILQ_HEAD(SignedRangeList, SignedRange);
-
-static inline int64_t s_range_end(int64_t start, int64_t length)
-{
-    return start + length - 1;
-}
-
-/* negative length or overflow */
-static inline bool s_range_overflow(int64_t start, int64_t length)
+/* 0,1 can merge with 1,2 but don't overlap */
+static inline bool ranges_can_merge(Range *range1, Range *range2)
 {
-    return s_range_end(start, length) < start;
+    return !(range1->end < range2->begin || range2->end < range1->begin);
 }
 
-static inline SignedRange *s_range_new(int64_t start, int64_t length)
+static inline int range_merge(Range *range1, Range *range2)
 {
-    SignedRange *range = NULL;
-
-    if (s_range_overflow(start, length)) {
-        return NULL;
+    if (ranges_can_merge(range1, range2)) {
+        if (range1->end < range2->end) {
+            range1->end = range2->end;
+        }
+        if (range1->begin > range2->begin) {
+            range1->begin = range2->begin;
+        }
+        return 0;
     }
 
-    range = g_malloc0(sizeof(*range));
-    range->start = start;
-    range->length = length;
-
-    return range;
-}
-
-static inline void s_range_free(SignedRange *range)
-{
-    g_free(range);
+    return -1;
 }
 
-static inline bool s_range_overlap(int64_t start1, int64_t length1,
-                                   int64_t start2, int64_t length2)
+static inline GList *g_list_insert_sorted_merged(GList *list,
+                                                 gpointer data,
+                                                 GCompareFunc func)
 {
-    return !((start1 + length1) < start2 || (start2 + length2) < start1);
-}
+    GList *l, *next = NULL;
+    Range *r, *nextr;
 
-static inline int s_range_join(SignedRange *range,
-                               int64_t start, int64_t length)
-{
-    if (s_range_overflow(start, length)) {
-        return -1;
+    if (!list) {
+        list = g_list_insert_sorted(list, data, func);
+        return list;
     }
 
-    if (s_range_overlap(range->start, range->length, start, length)) {
-        int64_t end = s_range_end(range->start, range->length);
-        if (end < s_range_end(start, length)) {
-            end = s_range_end(start, length);
+    nextr = data;
+    l = list;
+    while (l && l != next && nextr) {
+        r = l->data;
+        if (ranges_can_merge(r, nextr)) {
+            range_merge(r, nextr);
+            l = g_list_remove_link(l, next);
+            next = g_list_next(l);
+            if (next) {
+                nextr = next->data;
+            } else {
+                nextr = NULL;
+            }
+        } else {
+            l = g_list_next(l);
         }
-        if (range->start > start) {
-            range->start = start;
-        }
-        range->length = end - range->start + 1;
-        return 0;
     }
 
-    return -1;
+    if (!l) {
+        list = g_list_insert_sorted(list, data, func);
+    }
+
+    return list;
 }
 
-static inline int s_range_compare(int64_t start1, int64_t length1,
-                                  int64_t start2, int64_t length2)
+static inline gint range_compare(gconstpointer a, gconstpointer b)
 {
-    if (start1 == start2 && length1 == length2) {
+    Range *ra = (Range *)a, *rb = (Range *)b;
+    if (ra->begin == rb->begin && ra->end == rb->end) {
         return 0;
-    } else if (s_range_end(start1, length1) <
-               s_range_end(start2, length2)) {
+    } else if (range_get_last(ra->begin, ra->end) <
+               range_get_last(rb->begin, rb->end)) {
         return -1;
     } else {
         return 1;
     }
 }
 
-/* Add range to list. Keep them sorted, and merge ranges whenever possible */
-static inline bool range_list_add(SignedRangeList *list,
-                                  int64_t start, int64_t length)
-{
-    SignedRange *r, *next, *new_range = NULL, *cur = NULL;
-
-    if (s_range_overflow(start, length)) {
-        return false;
-    }
-
-    QTAILQ_FOREACH_SAFE(r, list, entry, next) {
-        if (s_range_overlap(r->start, r->length, start, length)) {
-            s_range_join(r, start, length);
-            break;
-        } else if (s_range_compare(start, length, r->start, r->length) < 0) {
-            cur = r;
-            break;
-        }
-    }
-
-    if (!r) {
-        new_range = s_range_new(start, length);
-        QTAILQ_INSERT_TAIL(list, new_range, entry);
-    } else if (cur) {
-        new_range = s_range_new(start, length);
-        QTAILQ_INSERT_BEFORE(cur, new_range, entry);
-    } else {
-        SignedRange *next = QTAILQ_NEXT(r, entry);
-        while (next && s_range_overlap(r->start, r->length,
-                                       next->start, next->length)) {
-            s_range_join(r, next->start, next->length);
-            QTAILQ_REMOVE(list, next, entry);
-            s_range_free(next);
-            next = QTAILQ_NEXT(r, entry);
-        }
-    }
-
-    return true;
-}
-
 #endif
qapi/string-input-visitor.c +65 -51

194 lines not on this page · raw message

qapi/string-output-visitor.c +48 -49

158 lines not on this page · raw message

tests/test-string-input-visitor.c +2 -2
index b08a7db..b01e2f2 100644
@@ -67,13 +67,13 @@ static void test_visitor_in_int(TestInputVisitorData *data,
 static void test_visitor_in_intList(TestInputVisitorData *data,
                                     const void *unused)
 {
-    int64_t value[] = {-2, -1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
+    int64_t value[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
     int16List *res = NULL, *tmp;
     Error *errp = NULL;
     Visitor *v;
     int i = 0;
 
-    v = visitor_input_test_init(data, "1,2,-2-1,2-4,20,5-9,1-8");
+    v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
 
     visit_type_int16List(v, &res, NULL, &errp);
     g_assert(errp == NULL);
tests/test-string-output-visitor.c +4 -4
index d2ad580..28e7359 100644
@@ -44,7 +44,7 @@ static void visitor_output_teardown(TestOutputVisitorData *data,
 static void test_visitor_out_int(TestOutputVisitorData *data,
                                  const void *unused)
 {
-    int64_t value = -42;
+    int64_t value = 42;
     Error *err = NULL;
     char *str;
 
@@ -53,14 +53,14 @@ static void test_visitor_out_int(TestOutputVisitorData *data,
 
     str = string_output_get_string(data->sov);
     g_assert(str != NULL);
-    g_assert_cmpstr(str, ==, "-42");
+    g_assert_cmpstr(str, ==, "42");
     g_free(str);
 }
 
 static void test_visitor_out_intList(TestOutputVisitorData *data,
                                      const void *unused)
 {
-    int64_t value[] = {-10, -7, -2, -1, 0, 1, 9, 10, 16, 15, 14,
+    int64_t value[] = {0, 1, 9, 10, 16, 15, 14,
         3, 4, 5, 6, 11, 12, 13, 21, 22, INT64_MAX - 1, INT64_MAX};
     intList *list = NULL, **tmp = &list;
     int i;
@@ -79,7 +79,7 @@ static void test_visitor_out_intList(TestOutputVisitorData *data,
     str = string_output_get_string(data->sov);
     g_assert(str != NULL);
     g_assert_cmpstr(str, ==,
-        "-10,-7,-2-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807");
+        "0-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807");
     g_free(str);
     while (list) {
         intList *tmp2;
Signature · 1 line
1.9.3
#29 lore raw

Re: [Qemu-devel] [PATCH RFC 1/4] get rid of signed range

Quoted 1 line · Hu Tao

This also fixed make check failures that I was seeing on 32 bit systems. Applied, but I split this patch up and applied as fixup
to the original.
In the future you can request such fixes by making
subject be "fixup! <original subject>"
This is possible as long as tree is not merged.

Quoted 623 lines
---
 include/qemu/range.h               | 144 ++++++++++++-------------------------
 qapi/string-input-visitor.c        | 116 +++++++++++++++++-------------
 qapi/string-output-visitor.c       |  97 +++++++++++++------------
 tests/test-string-input-visitor.c  |   4 +-
 tests/test-string-output-visitor.c |   8 +--
 5 files changed, 165 insertions(+), 204 deletions(-)
include/qemu/range.h +46 -98
index 8879f8a..cfa021f 100644
@@ -61,127 +61,75 @@ static inline int ranges_overlap(uint64_t first1, uint64_t len1,
     return !(last2 < first1 || last1 < first2);
 }
 
-typedef struct SignedRangeList SignedRangeList;
-
-typedef struct SignedRange {
-    int64_t start;
-    int64_t length;
-
-    QTAILQ_ENTRY(SignedRange) entry;
-} SignedRange;
-
-QTAILQ_HEAD(SignedRangeList, SignedRange);
-
-static inline int64_t s_range_end(int64_t start, int64_t length)
-{
-    return start + length - 1;
-}
-
-/* negative length or overflow */
-static inline bool s_range_overflow(int64_t start, int64_t length)
+/* 0,1 can merge with 1,2 but don't overlap */
+static inline bool ranges_can_merge(Range *range1, Range *range2)
 {
-    return s_range_end(start, length) < start;
+    return !(range1->end < range2->begin || range2->end < range1->begin);
 }
 
-static inline SignedRange *s_range_new(int64_t start, int64_t length)
+static inline int range_merge(Range *range1, Range *range2)
 {
-    SignedRange *range = NULL;
-
-    if (s_range_overflow(start, length)) {
-        return NULL;
+    if (ranges_can_merge(range1, range2)) {
+        if (range1->end < range2->end) {
+            range1->end = range2->end;
+        }
+        if (range1->begin > range2->begin) {
+            range1->begin = range2->begin;
+        }
+        return 0;
     }
 
-    range = g_malloc0(sizeof(*range));
-    range->start = start;
-    range->length = length;
-
-    return range;
-}
-
-static inline void s_range_free(SignedRange *range)
-{
-    g_free(range);
+    return -1;
 }
 
-static inline bool s_range_overlap(int64_t start1, int64_t length1,
-                                   int64_t start2, int64_t length2)
+static inline GList *g_list_insert_sorted_merged(GList *list,
+                                                 gpointer data,
+                                                 GCompareFunc func)
 {
-    return !((start1 + length1) < start2 || (start2 + length2) < start1);
-}
+    GList *l, *next = NULL;
+    Range *r, *nextr;
 
-static inline int s_range_join(SignedRange *range,
-                               int64_t start, int64_t length)
-{
-    if (s_range_overflow(start, length)) {
-        return -1;
+    if (!list) {
+        list = g_list_insert_sorted(list, data, func);
+        return list;
     }
 
-    if (s_range_overlap(range->start, range->length, start, length)) {
-        int64_t end = s_range_end(range->start, range->length);
-        if (end < s_range_end(start, length)) {
-            end = s_range_end(start, length);
+    nextr = data;
+    l = list;
+    while (l && l != next && nextr) {
+        r = l->data;
+        if (ranges_can_merge(r, nextr)) {
+            range_merge(r, nextr);
+            l = g_list_remove_link(l, next);
+            next = g_list_next(l);
+            if (next) {
+                nextr = next->data;
+            } else {
+                nextr = NULL;
+            }
+        } else {
+            l = g_list_next(l);
         }
-        if (range->start > start) {
-            range->start = start;
-        }
-        range->length = end - range->start + 1;
-        return 0;
     }
 
-    return -1;
+    if (!l) {
+        list = g_list_insert_sorted(list, data, func);
+    }
+
+    return list;
 }
 
-static inline int s_range_compare(int64_t start1, int64_t length1,
-                                  int64_t start2, int64_t length2)
+static inline gint range_compare(gconstpointer a, gconstpointer b)
 {
-    if (start1 == start2 && length1 == length2) {
+    Range *ra = (Range *)a, *rb = (Range *)b;
+    if (ra->begin == rb->begin && ra->end == rb->end) {
         return 0;
-    } else if (s_range_end(start1, length1) <
-               s_range_end(start2, length2)) {
+    } else if (range_get_last(ra->begin, ra->end) <
+               range_get_last(rb->begin, rb->end)) {
         return -1;
     } else {
         return 1;
     }
 }
 
-/* Add range to list. Keep them sorted, and merge ranges whenever possible */
-static inline bool range_list_add(SignedRangeList *list,
-                                  int64_t start, int64_t length)
-{
-    SignedRange *r, *next, *new_range = NULL, *cur = NULL;
-
-    if (s_range_overflow(start, length)) {
-        return false;
-    }
-
-    QTAILQ_FOREACH_SAFE(r, list, entry, next) {
-        if (s_range_overlap(r->start, r->length, start, length)) {
-            s_range_join(r, start, length);
-            break;
-        } else if (s_range_compare(start, length, r->start, r->length) < 0) {
-            cur = r;
-            break;
-        }
-    }
-
-    if (!r) {
-        new_range = s_range_new(start, length);
-        QTAILQ_INSERT_TAIL(list, new_range, entry);
-    } else if (cur) {
-        new_range = s_range_new(start, length);
-        QTAILQ_INSERT_BEFORE(cur, new_range, entry);
-    } else {
-        SignedRange *next = QTAILQ_NEXT(r, entry);
-        while (next && s_range_overlap(r->start, r->length,
-                                       next->start, next->length)) {
-            s_range_join(r, next->start, next->length);
-            QTAILQ_REMOVE(list, next, entry);
-            s_range_free(next);
-            next = QTAILQ_NEXT(r, entry);
-        }
-    }
-
-    return true;
-}
-
 #endif
qapi/string-input-visitor.c +65 -51
index 85ac6a1..0b2490b 100644
@@ -25,8 +25,8 @@ struct StringInputVisitor
 
     bool head;
 
-    SignedRangeList *ranges;
-    SignedRange *cur_range;
+    GList *ranges;
+    GList *cur_range;
     int64_t cur;
 
     const char *string;
@@ -36,24 +36,25 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
 {
     char *str = (char *) siv->string;
     long long start, end;
-    SignedRange *r, *next;
+    Range *cur;
     char *endptr;
 
     if (siv->ranges) {
         return;
     }
 
-    siv->ranges = g_malloc0(sizeof(*siv->ranges));
-    QTAILQ_INIT(siv->ranges);
     errno = 0;
     do {
         start = strtoll(str, &endptr, 0);
         if (errno == 0 && endptr > str && INT64_MIN <= start &&
             start <= INT64_MAX) {
             if (*endptr == '\0') {
-                if (!range_list_add(siv->ranges, start, 1)) {
-                    goto error;
-                }
+                cur = g_malloc0(sizeof(*cur));
+                cur->begin = start;
+                cur->end = start + 1;
+                siv->ranges = g_list_insert_sorted_merged(siv->ranges, cur,
+                                                          range_compare);
+                cur = NULL;
                 str = NULL;
             } else if (*endptr == '-') {
                 str = endptr + 1;
@@ -63,17 +64,25 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
                     (start > INT64_MAX - 65536 ||
                      end < start + 65536)) {
                     if (*endptr == '\0') {
-                        if (!range_list_add(siv->ranges, start,
-                                            end - start + 1)) {
-                            goto error;
-                        }
+                        cur = g_malloc0(sizeof(*cur));
+                        cur->begin = start;
+                        cur->end = end + 1;
+                        siv->ranges =
+                            g_list_insert_sorted_merged(siv->ranges,
+                                                        cur,
+                                                        range_compare);
+                        cur = NULL;
                         str = NULL;
                     } else if (*endptr == ',') {
                         str = endptr + 1;
-                        if (!range_list_add(siv->ranges, start,
-                                            end - start + 1)) {
-                            goto error;
-                        }
+                        cur = g_malloc0(sizeof(*cur));
+                        cur->begin = start;
+                        cur->end = end + 1;
+                        siv->ranges =
+                            g_list_insert_sorted_merged(siv->ranges,
+                                                        cur,
+                                                        range_compare);
+                        cur = NULL;
                     } else {
                         goto error;
                     }
@@ -82,9 +91,13 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
                 }
             } else if (*endptr == ',') {
                 str = endptr + 1;
-                if (!range_list_add(siv->ranges, start, 1)) {
-                    goto error;
-                }
+                cur = g_malloc0(sizeof(*cur));
+                cur->begin = start;
+                cur->end = start + 1;
+                siv->ranges = g_list_insert_sorted_merged(siv->ranges,
+                                                          cur,
+                                                          range_compare);
+                cur = NULL;
             } else {
                 goto error;
             }
@@ -95,14 +108,8 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
 
     return;
 error:
-    if (siv->ranges) {
-        QTAILQ_FOREACH_SAFE(r, siv->ranges, entry, next) {
-            QTAILQ_REMOVE(siv->ranges, r, entry);
-            g_free(r);
-        }
-        g_free(siv->ranges);
-        siv->ranges = NULL;
-    }
+    g_list_free_full(siv->ranges, g_free);
+    assert(siv->ranges == NULL);
 }
 
 static void
@@ -112,10 +119,11 @@ start_list(Visitor *v, const char *name, Error **errp)
 
     parse_str(siv, errp);
 
-    if (siv->ranges) {
-        siv->cur_range = QTAILQ_FIRST(siv->ranges);
-        if (siv->cur_range) {
-            siv->cur = siv->cur_range->start;
+    siv->cur_range = g_list_first(siv->ranges);
+    if (siv->cur_range) {
+        Range *r = siv->cur_range->data;
+        if (r) {
+            siv->cur = r->begin;
         }
     }
 }
@@ -125,19 +133,27 @@ next_list(Visitor *v, GenericList **list, Error **errp)
 {
     StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
     GenericList **link;
+    Range *r;
 
     if (!siv->ranges || !siv->cur_range) {
         return NULL;
     }
 
-    if (siv->cur < siv->cur_range->start ||
-        siv->cur >= (siv->cur_range->start + siv->cur_range->length)) {
-        siv->cur_range = QTAILQ_NEXT(siv->cur_range, entry);
-        if (siv->cur_range) {
-            siv->cur = siv->cur_range->start;
-        } else {
+    r = siv->cur_range->data;
+    if (!r) {
+        return NULL;
+    }
+
+    if (siv->cur < r->begin || siv->cur >= r->end) {
+        siv->cur_range = g_list_next(siv->cur_range);
+        if (!siv->cur_range) {
             return NULL;
         }
+        r = siv->cur_range->data;
+        if (!r) {
+            return NULL;
+        }
+        siv->cur = r->begin;
     }
 
     if (siv->head) {
@@ -176,12 +192,19 @@ static void parse_type_int(Visitor *v, int64_t *obj, const char *name,
     }
 
     if (!siv->cur_range) {
-        siv->cur_range = QTAILQ_FIRST(siv->ranges);
-        if (siv->cur_range) {
-            siv->cur = siv->cur_range->start;
-        } else {
+        Range *r;
+
+        siv->cur_range = g_list_first(siv->ranges);
+        if (!siv->cur_range) {
+            goto error;
+        }
+
+        r = siv->cur_range->data;
+        if (!r) {
             goto error;
         }
+
+        siv->cur = r->begin;
     }
 
     *obj = siv->cur;
@@ -291,16 +314,7 @@ Visitor *string_input_get_visitor(StringInputVisitor *v)
 
 void string_input_visitor_cleanup(StringInputVisitor *v)
 {
-    SignedRange *r, *next;
-
-    if (v->ranges) {
-        QTAILQ_FOREACH_SAFE(r, v->ranges, entry, next) {
-            QTAILQ_REMOVE(v->ranges, r, entry);
-            g_free(r);
-        }
-        g_free(v->ranges);
-    }
-
+    g_list_free_full(v->ranges, g_free);
     g_free(v);
 }
 
qapi/string-output-visitor.c +48 -49
index ccebc7a..1c0834a 100644
@@ -64,7 +64,7 @@ struct StringOutputVisitor
         int64_t s;
         uint64_t u;
     } range_start, range_end;
-    SignedRangeList *ranges;
+    GList *ranges;
 };
 
 static void string_output_set(StringOutputVisitor *sov, char *string)
@@ -78,25 +78,50 @@ static void string_output_set(StringOutputVisitor *sov, char *string)
 
 static void string_output_append(StringOutputVisitor *sov, int64_t a)
 {
-    range_list_add(sov->ranges, a, 1);
+    Range *r = g_malloc0(sizeof(*r));
+    r->begin = a;
+    r->end = a + 1;
+    sov->ranges = g_list_insert_sorted_merged(sov->ranges, r, range_compare);
 }
 
 static void string_output_append_range(StringOutputVisitor *sov,
                                        int64_t s, int64_t e)
 {
-    range_list_add(sov->ranges, s, e);
+    Range *r = g_malloc0(sizeof(*r));
+    r->begin = s;
+    r->end = e + 1;
+    sov->ranges = g_list_insert_sorted_merged(sov->ranges, r, range_compare);
+}
+
+static void format_string(StringOutputVisitor *sov, Range *r, bool next,
+                          bool human)
+{
+    if (r->end - r->begin > 1) {
+        if (human) {
+            g_string_append_printf(sov->string, "%" PRIx64 "-%" PRIx64,
+                                   r->begin, r->end - 1);
+
+        } else {
+            g_string_append_printf(sov->string, "%" PRId64 "-%" PRId64,
+                                   r->begin, r->end - 1);
+        }
+    } else {
+        if (human) {
+            g_string_append_printf(sov->string, "%" PRIx64, r->begin);
+        } else {
+            g_string_append_printf(sov->string, "%" PRId64, r->begin);
+        }
+    }
+    if (next) {
+        g_string_append(sov->string, ",");
+    }
 }
 
 static void print_type_int(Visitor *v, int64_t *obj, const char *name,
                            Error **errp)
 {
     StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
-    SignedRange *r;
-
-    if (!sov->ranges) {
-        sov->ranges = g_malloc0(sizeof(*sov->ranges));
-        QTAILQ_INIT(sov->ranges);
-    }
+    GList *l;
 
     switch (sov->list_mode) {
     case LM_NONE:
@@ -118,8 +143,7 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
             } else {
                 assert(sov->range_start.s < sov->range_end.s);
                 string_output_append_range(sov, sov->range_start.s,
-                                           sov->range_end.s -
-                                           sov->range_start.s + 1);
+                                           sov->range_end.s);
             }
 
             sov->range_start.s = *obj;
@@ -132,8 +156,7 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
             sov->range_end.s++;
             assert(sov->range_start.s < sov->range_end.s);
             string_output_append_range(sov, sov->range_start.s,
-                                       sov->range_end.s -
-                                       sov->range_start.s + 1);
+                                       sov->range_end.s);
         } else {
             if (sov->range_start.s == sov->range_end.s) {
                 string_output_append(sov, sov->range_end.s);
@@ -141,8 +164,7 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
                 assert(sov->range_start.s < sov->range_end.s);
 
                 string_output_append_range(sov, sov->range_start.s,
-                                           sov->range_end.s -
-                                           sov->range_start.s + 1);
+                                           sov->range_end.s);
             }
             string_output_append(sov, *obj);
         }
@@ -152,34 +174,20 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
         abort();
     }
 
-    QTAILQ_FOREACH(r, sov->ranges, entry) {
-        if (r->length > 1) {
-            g_string_append_printf(sov->string, "%" PRId64 "-%" PRId64,
-                                   r->start,
-                                   s_range_end(r->start, r->length));
-        } else {
-            g_string_append_printf(sov->string, "%" PRId64,
-                                   r->start);
-        }
-        if (r->entry.tqe_next) {
-            g_string_append(sov->string, ",");
-        }
+    l = sov->ranges;
+    while (l) {
+        Range *r = l->data;
+        format_string(sov, r, l->next != NULL, false);
+        l = l->next;
     }
 
     if (sov->human) {
+        l = sov->ranges;
         g_string_append(sov->string, " (");
-        QTAILQ_FOREACH(r, sov->ranges, entry) {
-            if (r->length > 1) {
-                g_string_append_printf(sov->string, "%" PRIx64 "-%" PRIx64,
-                                       r->start,
-                                       s_range_end(r->start, r->length));
-            } else {
-                g_string_append_printf(sov->string, "%" PRIx64,
-                                       r->start);
-            }
-            if (r->entry.tqe_next) {
-                g_string_append(sov->string, ",");
-            }
+        while (l) {
+            Range *r = l->data;
+            format_string(sov, r, l->next != NULL, false);
+            l = l->next;
         }
         g_string_append(sov->string, ")");
     }
@@ -310,20 +318,11 @@ Visitor *string_output_get_visitor(StringOutputVisitor *sov)
 
 void string_output_visitor_cleanup(StringOutputVisitor *sov)
 {
-    SignedRange *r, *next;
-
     if (sov->string) {
         g_string_free(sov->string, true);
     }
 
-    if (sov->ranges) {
-        QTAILQ_FOREACH_SAFE(r, sov->ranges, entry, next) {
-            QTAILQ_REMOVE(sov->ranges, r, entry);
-            s_range_free(r);
-        }
-        g_free(sov->ranges);
-    }
-
+    g_list_free_full(sov->ranges, g_free);
     g_free(sov);
 }
 
tests/test-string-input-visitor.c +2 -2
index b08a7db..b01e2f2 100644
@@ -67,13 +67,13 @@ static void test_visitor_in_int(TestInputVisitorData *data,
 static void test_visitor_in_intList(TestInputVisitorData *data,
                                     const void *unused)
 {
-    int64_t value[] = {-2, -1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
+    int64_t value[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
     int16List *res = NULL, *tmp;
     Error *errp = NULL;
     Visitor *v;
     int i = 0;
 
-    v = visitor_input_test_init(data, "1,2,-2-1,2-4,20,5-9,1-8");
+    v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
 
     visit_type_int16List(v, &res, NULL, &errp);
     g_assert(errp == NULL);
tests/test-string-output-visitor.c +4 -4
index d2ad580..28e7359 100644
@@ -44,7 +44,7 @@ static void visitor_output_teardown(TestOutputVisitorData *data,
 static void test_visitor_out_int(TestOutputVisitorData *data,
                                  const void *unused)
 {
-    int64_t value = -42;
+    int64_t value = 42;
     Error *err = NULL;
     char *str;
 
@@ -53,14 +53,14 @@ static void test_visitor_out_int(TestOutputVisitorData *data,
 
     str = string_output_get_string(data->sov);
     g_assert(str != NULL);
-    g_assert_cmpstr(str, ==, "-42");
+    g_assert_cmpstr(str, ==, "42");
     g_free(str);
 }
 
 static void test_visitor_out_intList(TestOutputVisitorData *data,
                                      const void *unused)
 {
-    int64_t value[] = {-10, -7, -2, -1, 0, 1, 9, 10, 16, 15, 14,
+    int64_t value[] = {0, 1, 9, 10, 16, 15, 14,
         3, 4, 5, 6, 11, 12, 13, 21, 22, INT64_MAX - 1, INT64_MAX};
     intList *list = NULL, **tmp = &list;
     int i;
@@ -79,7 +79,7 @@ static void test_visitor_out_intList(TestOutputVisitorData *data,
     str = string_output_get_string(data->sov);
     g_assert(str != NULL);
     g_assert_cmpstr(str, ==,
-        "-10,-7,-2-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807");
+        "0-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807");
     g_free(str);
     while (list) {
         intList *tmp2;
Signature · 1 line
1.9.3
author
#30 lore raw

Re: [Qemu-devel] [PATCH RFC 1/4] get rid of signed range

Quoted 9 lines · Michael S. Tsirkin
Quoted 1 line · Hu Tao

This also fixed make check failures that I was seeing on 32 bit systems. Applied, but I split this patch up and applied as fixup
to the original.
In the future you can request such fixes by making
subject be "fixup! <original subject>"
This is possible as long as tree is not merged.

Thanks!

Quoted 624 lines
Quoted 624 lines
---
 include/qemu/range.h               | 144 ++++++++++++-------------------------
 qapi/string-input-visitor.c        | 116 +++++++++++++++++-------------
 qapi/string-output-visitor.c       |  97 +++++++++++++------------
 tests/test-string-input-visitor.c  |   4 +-
 tests/test-string-output-visitor.c |   8 +--
 5 files changed, 165 insertions(+), 204 deletions(-)
include/qemu/range.h +46 -98
index 8879f8a..cfa021f 100644
@@ -61,127 +61,75 @@ static inline int ranges_overlap(uint64_t first1, uint64_t len1,
     return !(last2 < first1 || last1 < first2);
 }
 
-typedef struct SignedRangeList SignedRangeList;
-
-typedef struct SignedRange {
-    int64_t start;
-    int64_t length;
-
-    QTAILQ_ENTRY(SignedRange) entry;
-} SignedRange;
-
-QTAILQ_HEAD(SignedRangeList, SignedRange);
-
-static inline int64_t s_range_end(int64_t start, int64_t length)
-{
-    return start + length - 1;
-}
-
-/* negative length or overflow */
-static inline bool s_range_overflow(int64_t start, int64_t length)
+/* 0,1 can merge with 1,2 but don't overlap */
+static inline bool ranges_can_merge(Range *range1, Range *range2)
 {
-    return s_range_end(start, length) < start;
+    return !(range1->end < range2->begin || range2->end < range1->begin);
 }
 
-static inline SignedRange *s_range_new(int64_t start, int64_t length)
+static inline int range_merge(Range *range1, Range *range2)
 {
-    SignedRange *range = NULL;
-
-    if (s_range_overflow(start, length)) {
-        return NULL;
+    if (ranges_can_merge(range1, range2)) {
+        if (range1->end < range2->end) {
+            range1->end = range2->end;
+        }
+        if (range1->begin > range2->begin) {
+            range1->begin = range2->begin;
+        }
+        return 0;
     }
 
-    range = g_malloc0(sizeof(*range));
-    range->start = start;
-    range->length = length;
-
-    return range;
-}
-
-static inline void s_range_free(SignedRange *range)
-{
-    g_free(range);
+    return -1;
 }
 
-static inline bool s_range_overlap(int64_t start1, int64_t length1,
-                                   int64_t start2, int64_t length2)
+static inline GList *g_list_insert_sorted_merged(GList *list,
+                                                 gpointer data,
+                                                 GCompareFunc func)
 {
-    return !((start1 + length1) < start2 || (start2 + length2) < start1);
-}
+    GList *l, *next = NULL;
+    Range *r, *nextr;
 
-static inline int s_range_join(SignedRange *range,
-                               int64_t start, int64_t length)
-{
-    if (s_range_overflow(start, length)) {
-        return -1;
+    if (!list) {
+        list = g_list_insert_sorted(list, data, func);
+        return list;
     }
 
-    if (s_range_overlap(range->start, range->length, start, length)) {
-        int64_t end = s_range_end(range->start, range->length);
-        if (end < s_range_end(start, length)) {
-            end = s_range_end(start, length);
+    nextr = data;
+    l = list;
+    while (l && l != next && nextr) {
+        r = l->data;
+        if (ranges_can_merge(r, nextr)) {
+            range_merge(r, nextr);
+            l = g_list_remove_link(l, next);
+            next = g_list_next(l);
+            if (next) {
+                nextr = next->data;
+            } else {
+                nextr = NULL;
+            }
+        } else {
+            l = g_list_next(l);
         }
-        if (range->start > start) {
-            range->start = start;
-        }
-        range->length = end - range->start + 1;
-        return 0;
     }
 
-    return -1;
+    if (!l) {
+        list = g_list_insert_sorted(list, data, func);
+    }
+
+    return list;
 }
 
-static inline int s_range_compare(int64_t start1, int64_t length1,
-                                  int64_t start2, int64_t length2)
+static inline gint range_compare(gconstpointer a, gconstpointer b)
 {
-    if (start1 == start2 && length1 == length2) {
+    Range *ra = (Range *)a, *rb = (Range *)b;
+    if (ra->begin == rb->begin && ra->end == rb->end) {
         return 0;
-    } else if (s_range_end(start1, length1) <
-               s_range_end(start2, length2)) {
+    } else if (range_get_last(ra->begin, ra->end) <
+               range_get_last(rb->begin, rb->end)) {
         return -1;
     } else {
         return 1;
     }
 }
 
-/* Add range to list. Keep them sorted, and merge ranges whenever possible */
-static inline bool range_list_add(SignedRangeList *list,
-                                  int64_t start, int64_t length)
-{
-    SignedRange *r, *next, *new_range = NULL, *cur = NULL;
-
-    if (s_range_overflow(start, length)) {
-        return false;
-    }
-
-    QTAILQ_FOREACH_SAFE(r, list, entry, next) {
-        if (s_range_overlap(r->start, r->length, start, length)) {
-            s_range_join(r, start, length);
-            break;
-        } else if (s_range_compare(start, length, r->start, r->length) < 0) {
-            cur = r;
-            break;
-        }
-    }
-
-    if (!r) {
-        new_range = s_range_new(start, length);
-        QTAILQ_INSERT_TAIL(list, new_range, entry);
-    } else if (cur) {
-        new_range = s_range_new(start, length);
-        QTAILQ_INSERT_BEFORE(cur, new_range, entry);
-    } else {
-        SignedRange *next = QTAILQ_NEXT(r, entry);
-        while (next && s_range_overlap(r->start, r->length,
-                                       next->start, next->length)) {
-            s_range_join(r, next->start, next->length);
-            QTAILQ_REMOVE(list, next, entry);
-            s_range_free(next);
-            next = QTAILQ_NEXT(r, entry);
-        }
-    }
-
-    return true;
-}
-
 #endif
qapi/string-input-visitor.c +65 -51
index 85ac6a1..0b2490b 100644
@@ -25,8 +25,8 @@ struct StringInputVisitor
 
     bool head;
 
-    SignedRangeList *ranges;
-    SignedRange *cur_range;
+    GList *ranges;
+    GList *cur_range;
     int64_t cur;
 
     const char *string;
@@ -36,24 +36,25 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
 {
     char *str = (char *) siv->string;
     long long start, end;
-    SignedRange *r, *next;
+    Range *cur;
     char *endptr;
 
     if (siv->ranges) {
         return;
     }
 
-    siv->ranges = g_malloc0(sizeof(*siv->ranges));
-    QTAILQ_INIT(siv->ranges);
     errno = 0;
     do {
         start = strtoll(str, &endptr, 0);
         if (errno == 0 && endptr > str && INT64_MIN <= start &&
             start <= INT64_MAX) {
             if (*endptr == '\0') {
-                if (!range_list_add(siv->ranges, start, 1)) {
-                    goto error;
-                }
+                cur = g_malloc0(sizeof(*cur));
+                cur->begin = start;
+                cur->end = start + 1;
+                siv->ranges = g_list_insert_sorted_merged(siv->ranges, cur,
+                                                          range_compare);
+                cur = NULL;
                 str = NULL;
             } else if (*endptr == '-') {
                 str = endptr + 1;
@@ -63,17 +64,25 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
                     (start > INT64_MAX - 65536 ||
                      end < start + 65536)) {
                     if (*endptr == '\0') {
-                        if (!range_list_add(siv->ranges, start,
-                                            end - start + 1)) {
-                            goto error;
-                        }
+                        cur = g_malloc0(sizeof(*cur));
+                        cur->begin = start;
+                        cur->end = end + 1;
+                        siv->ranges =
+                            g_list_insert_sorted_merged(siv->ranges,
+                                                        cur,
+                                                        range_compare);
+                        cur = NULL;
                         str = NULL;
                     } else if (*endptr == ',') {
                         str = endptr + 1;
-                        if (!range_list_add(siv->ranges, start,
-                                            end - start + 1)) {
-                            goto error;
-                        }
+                        cur = g_malloc0(sizeof(*cur));
+                        cur->begin = start;
+                        cur->end = end + 1;
+                        siv->ranges =
+                            g_list_insert_sorted_merged(siv->ranges,
+                                                        cur,
+                                                        range_compare);
+                        cur = NULL;
                     } else {
                         goto error;
                     }
@@ -82,9 +91,13 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
                 }
             } else if (*endptr == ',') {
                 str = endptr + 1;
-                if (!range_list_add(siv->ranges, start, 1)) {
-                    goto error;
-                }
+                cur = g_malloc0(sizeof(*cur));
+                cur->begin = start;
+                cur->end = start + 1;
+                siv->ranges = g_list_insert_sorted_merged(siv->ranges,
+                                                          cur,
+                                                          range_compare);
+                cur = NULL;
             } else {
                 goto error;
             }
@@ -95,14 +108,8 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
 
     return;
 error:
-    if (siv->ranges) {
-        QTAILQ_FOREACH_SAFE(r, siv->ranges, entry, next) {
-            QTAILQ_REMOVE(siv->ranges, r, entry);
-            g_free(r);
-        }
-        g_free(siv->ranges);
-        siv->ranges = NULL;
-    }
+    g_list_free_full(siv->ranges, g_free);
+    assert(siv->ranges == NULL);
 }
 
 static void
@@ -112,10 +119,11 @@ start_list(Visitor *v, const char *name, Error **errp)
 
     parse_str(siv, errp);
 
-    if (siv->ranges) {
-        siv->cur_range = QTAILQ_FIRST(siv->ranges);
-        if (siv->cur_range) {
-            siv->cur = siv->cur_range->start;
+    siv->cur_range = g_list_first(siv->ranges);
+    if (siv->cur_range) {
+        Range *r = siv->cur_range->data;
+        if (r) {
+            siv->cur = r->begin;
         }
     }
 }
@@ -125,19 +133,27 @@ next_list(Visitor *v, GenericList **list, Error **errp)
 {
     StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
     GenericList **link;
+    Range *r;
 
     if (!siv->ranges || !siv->cur_range) {
         return NULL;
     }
 
-    if (siv->cur < siv->cur_range->start ||
-        siv->cur >= (siv->cur_range->start + siv->cur_range->length)) {
-        siv->cur_range = QTAILQ_NEXT(siv->cur_range, entry);
-        if (siv->cur_range) {
-            siv->cur = siv->cur_range->start;
-        } else {
+    r = siv->cur_range->data;
+    if (!r) {
+        return NULL;
+    }
+
+    if (siv->cur < r->begin || siv->cur >= r->end) {
+        siv->cur_range = g_list_next(siv->cur_range);
+        if (!siv->cur_range) {
             return NULL;
         }
+        r = siv->cur_range->data;
+        if (!r) {
+            return NULL;
+        }
+        siv->cur = r->begin;
     }
 
     if (siv->head) {
@@ -176,12 +192,19 @@ static void parse_type_int(Visitor *v, int64_t *obj, const char *name,
     }
 
     if (!siv->cur_range) {
-        siv->cur_range = QTAILQ_FIRST(siv->ranges);
-        if (siv->cur_range) {
-            siv->cur = siv->cur_range->start;
-        } else {
+        Range *r;
+
+        siv->cur_range = g_list_first(siv->ranges);
+        if (!siv->cur_range) {
+            goto error;
+        }
+
+        r = siv->cur_range->data;
+        if (!r) {
             goto error;
         }
+
+        siv->cur = r->begin;
     }
 
     *obj = siv->cur;
@@ -291,16 +314,7 @@ Visitor *string_input_get_visitor(StringInputVisitor *v)
 
 void string_input_visitor_cleanup(StringInputVisitor *v)
 {
-    SignedRange *r, *next;
-
-    if (v->ranges) {
-        QTAILQ_FOREACH_SAFE(r, v->ranges, entry, next) {
-            QTAILQ_REMOVE(v->ranges, r, entry);
-            g_free(r);
-        }
-        g_free(v->ranges);
-    }
-
+    g_list_free_full(v->ranges, g_free);
     g_free(v);
 }
 
qapi/string-output-visitor.c +48 -49
index ccebc7a..1c0834a 100644
@@ -64,7 +64,7 @@ struct StringOutputVisitor
         int64_t s;
         uint64_t u;
     } range_start, range_end;
-    SignedRangeList *ranges;
+    GList *ranges;
 };
 
 static void string_output_set(StringOutputVisitor *sov, char *string)
@@ -78,25 +78,50 @@ static void string_output_set(StringOutputVisitor *sov, char *string)
 
 static void string_output_append(StringOutputVisitor *sov, int64_t a)
 {
-    range_list_add(sov->ranges, a, 1);
+    Range *r = g_malloc0(sizeof(*r));
+    r->begin = a;
+    r->end = a + 1;
+    sov->ranges = g_list_insert_sorted_merged(sov->ranges, r, range_compare);
 }
 
 static void string_output_append_range(StringOutputVisitor *sov,
                                        int64_t s, int64_t e)
 {
-    range_list_add(sov->ranges, s, e);
+    Range *r = g_malloc0(sizeof(*r));
+    r->begin = s;
+    r->end = e + 1;
+    sov->ranges = g_list_insert_sorted_merged(sov->ranges, r, range_compare);
+}
+
+static void format_string(StringOutputVisitor *sov, Range *r, bool next,
+                          bool human)
+{
+    if (r->end - r->begin > 1) {
+        if (human) {
+            g_string_append_printf(sov->string, "%" PRIx64 "-%" PRIx64,
+                                   r->begin, r->end - 1);
+
+        } else {
+            g_string_append_printf(sov->string, "%" PRId64 "-%" PRId64,
+                                   r->begin, r->end - 1);
+        }
+    } else {
+        if (human) {
+            g_string_append_printf(sov->string, "%" PRIx64, r->begin);
+        } else {
+            g_string_append_printf(sov->string, "%" PRId64, r->begin);
+        }
+    }
+    if (next) {
+        g_string_append(sov->string, ",");
+    }
 }
 
 static void print_type_int(Visitor *v, int64_t *obj, const char *name,
                            Error **errp)
 {
     StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
-    SignedRange *r;
-
-    if (!sov->ranges) {
-        sov->ranges = g_malloc0(sizeof(*sov->ranges));
-        QTAILQ_INIT(sov->ranges);
-    }
+    GList *l;
 
     switch (sov->list_mode) {
     case LM_NONE:
@@ -118,8 +143,7 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
             } else {
                 assert(sov->range_start.s < sov->range_end.s);
                 string_output_append_range(sov, sov->range_start.s,
-                                           sov->range_end.s -
-                                           sov->range_start.s + 1);
+                                           sov->range_end.s);
             }
 
             sov->range_start.s = *obj;
@@ -132,8 +156,7 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
             sov->range_end.s++;
             assert(sov->range_start.s < sov->range_end.s);
             string_output_append_range(sov, sov->range_start.s,
-                                       sov->range_end.s -
-                                       sov->range_start.s + 1);
+                                       sov->range_end.s);
         } else {
             if (sov->range_start.s == sov->range_end.s) {
                 string_output_append(sov, sov->range_end.s);
@@ -141,8 +164,7 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
                 assert(sov->range_start.s < sov->range_end.s);
 
                 string_output_append_range(sov, sov->range_start.s,
-                                           sov->range_end.s -
-                                           sov->range_start.s + 1);
+                                           sov->range_end.s);
             }
             string_output_append(sov, *obj);
         }
@@ -152,34 +174,20 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
         abort();
     }
 
-    QTAILQ_FOREACH(r, sov->ranges, entry) {
-        if (r->length > 1) {
-            g_string_append_printf(sov->string, "%" PRId64 "-%" PRId64,
-                                   r->start,
-                                   s_range_end(r->start, r->length));
-        } else {
-            g_string_append_printf(sov->string, "%" PRId64,
-                                   r->start);
-        }
-        if (r->entry.tqe_next) {
-            g_string_append(sov->string, ",");
-        }
+    l = sov->ranges;
+    while (l) {
+        Range *r = l->data;
+        format_string(sov, r, l->next != NULL, false);
+        l = l->next;
     }
 
     if (sov->human) {
+        l = sov->ranges;
         g_string_append(sov->string, " (");
-        QTAILQ_FOREACH(r, sov->ranges, entry) {
-            if (r->length > 1) {
-                g_string_append_printf(sov->string, "%" PRIx64 "-%" PRIx64,
-                                       r->start,
-                                       s_range_end(r->start, r->length));
-            } else {
-                g_string_append_printf(sov->string, "%" PRIx64,
-                                       r->start);
-            }
-            if (r->entry.tqe_next) {
-                g_string_append(sov->string, ",");
-            }
+        while (l) {
+            Range *r = l->data;
+            format_string(sov, r, l->next != NULL, false);
+            l = l->next;
         }
         g_string_append(sov->string, ")");
     }
@@ -310,20 +318,11 @@ Visitor *string_output_get_visitor(StringOutputVisitor *sov)
 
 void string_output_visitor_cleanup(StringOutputVisitor *sov)
 {
-    SignedRange *r, *next;
-
     if (sov->string) {
         g_string_free(sov->string, true);
     }
 
-    if (sov->ranges) {
-        QTAILQ_FOREACH_SAFE(r, sov->ranges, entry, next) {
-            QTAILQ_REMOVE(sov->ranges, r, entry);
-            s_range_free(r);
-        }
-        g_free(sov->ranges);
-    }
-
+    g_list_free_full(sov->ranges, g_free);
     g_free(sov);
 }
 
tests/test-string-input-visitor.c +2 -2
index b08a7db..b01e2f2 100644
@@ -67,13 +67,13 @@ static void test_visitor_in_int(TestInputVisitorData *data,
 static void test_visitor_in_intList(TestInputVisitorData *data,
                                     const void *unused)
 {
-    int64_t value[] = {-2, -1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
+    int64_t value[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
     int16List *res = NULL, *tmp;
     Error *errp = NULL;
     Visitor *v;
     int i = 0;
 
-    v = visitor_input_test_init(data, "1,2,-2-1,2-4,20,5-9,1-8");
+    v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
 
     visit_type_int16List(v, &res, NULL, &errp);
     g_assert(errp == NULL);
tests/test-string-output-visitor.c +4 -4
index d2ad580..28e7359 100644
@@ -44,7 +44,7 @@ static void visitor_output_teardown(TestOutputVisitorData *data,
 static void test_visitor_out_int(TestOutputVisitorData *data,
                                  const void *unused)
 {
-    int64_t value = -42;
+    int64_t value = 42;
     Error *err = NULL;
     char *str;
 
@@ -53,14 +53,14 @@ static void test_visitor_out_int(TestOutputVisitorData *data,
 
     str = string_output_get_string(data->sov);
     g_assert(str != NULL);
-    g_assert_cmpstr(str, ==, "-42");
+    g_assert_cmpstr(str, ==, "42");
     g_free(str);
 }
 
 static void test_visitor_out_intList(TestOutputVisitorData *data,
                                      const void *unused)
 {
-    int64_t value[] = {-10, -7, -2, -1, 0, 1, 9, 10, 16, 15, 14,
+    int64_t value[] = {0, 1, 9, 10, 16, 15, 14,
         3, 4, 5, 6, 11, 12, 13, 21, 22, INT64_MAX - 1, INT64_MAX};
     intList *list = NULL, **tmp = &list;
     int i;
@@ -79,7 +79,7 @@ static void test_visitor_out_intList(TestOutputVisitorData *data,
     str = string_output_get_string(data->sov);
     g_assert(str != NULL);
     g_assert_cmpstr(str, ==,
-        "-10,-7,-2-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807");
+        "0-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807");
     g_free(str);
     while (list) {
         intList *tmp2;
Signature · 1 line
1.9.3
#31 lore raw

Re: [Qemu-devel] [PATCH RFC 1/4] get rid of signed range

In reply to Hu Tao
Quoted 1 line · Hu Tao

Breaks build with glib < 2.28.
Replaced free_full with foreach + free.

Quoted 623 lines
---
 include/qemu/range.h               | 144 ++++++++++++-------------------------
 qapi/string-input-visitor.c        | 116 +++++++++++++++++-------------
 qapi/string-output-visitor.c       |  97 +++++++++++++------------
 tests/test-string-input-visitor.c  |   4 +-
 tests/test-string-output-visitor.c |   8 +--
 5 files changed, 165 insertions(+), 204 deletions(-)
include/qemu/range.h +46 -98
index 8879f8a..cfa021f 100644
@@ -61,127 +61,75 @@ static inline int ranges_overlap(uint64_t first1, uint64_t len1,
     return !(last2 < first1 || last1 < first2);
 }
 
-typedef struct SignedRangeList SignedRangeList;
-
-typedef struct SignedRange {
-    int64_t start;
-    int64_t length;
-
-    QTAILQ_ENTRY(SignedRange) entry;
-} SignedRange;
-
-QTAILQ_HEAD(SignedRangeList, SignedRange);
-
-static inline int64_t s_range_end(int64_t start, int64_t length)
-{
-    return start + length - 1;
-}
-
-/* negative length or overflow */
-static inline bool s_range_overflow(int64_t start, int64_t length)
+/* 0,1 can merge with 1,2 but don't overlap */
+static inline bool ranges_can_merge(Range *range1, Range *range2)
 {
-    return s_range_end(start, length) < start;
+    return !(range1->end < range2->begin || range2->end < range1->begin);
 }
 
-static inline SignedRange *s_range_new(int64_t start, int64_t length)
+static inline int range_merge(Range *range1, Range *range2)
 {
-    SignedRange *range = NULL;
-
-    if (s_range_overflow(start, length)) {
-        return NULL;
+    if (ranges_can_merge(range1, range2)) {
+        if (range1->end < range2->end) {
+            range1->end = range2->end;
+        }
+        if (range1->begin > range2->begin) {
+            range1->begin = range2->begin;
+        }
+        return 0;
     }
 
-    range = g_malloc0(sizeof(*range));
-    range->start = start;
-    range->length = length;
-
-    return range;
-}
-
-static inline void s_range_free(SignedRange *range)
-{
-    g_free(range);
+    return -1;
 }
 
-static inline bool s_range_overlap(int64_t start1, int64_t length1,
-                                   int64_t start2, int64_t length2)
+static inline GList *g_list_insert_sorted_merged(GList *list,
+                                                 gpointer data,
+                                                 GCompareFunc func)
 {
-    return !((start1 + length1) < start2 || (start2 + length2) < start1);
-}
+    GList *l, *next = NULL;
+    Range *r, *nextr;
 
-static inline int s_range_join(SignedRange *range,
-                               int64_t start, int64_t length)
-{
-    if (s_range_overflow(start, length)) {
-        return -1;
+    if (!list) {
+        list = g_list_insert_sorted(list, data, func);
+        return list;
     }
 
-    if (s_range_overlap(range->start, range->length, start, length)) {
-        int64_t end = s_range_end(range->start, range->length);
-        if (end < s_range_end(start, length)) {
-            end = s_range_end(start, length);
+    nextr = data;
+    l = list;
+    while (l && l != next && nextr) {
+        r = l->data;
+        if (ranges_can_merge(r, nextr)) {
+            range_merge(r, nextr);
+            l = g_list_remove_link(l, next);
+            next = g_list_next(l);
+            if (next) {
+                nextr = next->data;
+            } else {
+                nextr = NULL;
+            }
+        } else {
+            l = g_list_next(l);
         }
-        if (range->start > start) {
-            range->start = start;
-        }
-        range->length = end - range->start + 1;
-        return 0;
     }
 
-    return -1;
+    if (!l) {
+        list = g_list_insert_sorted(list, data, func);
+    }
+
+    return list;
 }
 
-static inline int s_range_compare(int64_t start1, int64_t length1,
-                                  int64_t start2, int64_t length2)
+static inline gint range_compare(gconstpointer a, gconstpointer b)
 {
-    if (start1 == start2 && length1 == length2) {
+    Range *ra = (Range *)a, *rb = (Range *)b;
+    if (ra->begin == rb->begin && ra->end == rb->end) {
         return 0;
-    } else if (s_range_end(start1, length1) <
-               s_range_end(start2, length2)) {
+    } else if (range_get_last(ra->begin, ra->end) <
+               range_get_last(rb->begin, rb->end)) {
         return -1;
     } else {
         return 1;
     }
 }
 
-/* Add range to list. Keep them sorted, and merge ranges whenever possible */
-static inline bool range_list_add(SignedRangeList *list,
-                                  int64_t start, int64_t length)
-{
-    SignedRange *r, *next, *new_range = NULL, *cur = NULL;
-
-    if (s_range_overflow(start, length)) {
-        return false;
-    }
-
-    QTAILQ_FOREACH_SAFE(r, list, entry, next) {
-        if (s_range_overlap(r->start, r->length, start, length)) {
-            s_range_join(r, start, length);
-            break;
-        } else if (s_range_compare(start, length, r->start, r->length) < 0) {
-            cur = r;
-            break;
-        }
-    }
-
-    if (!r) {
-        new_range = s_range_new(start, length);
-        QTAILQ_INSERT_TAIL(list, new_range, entry);
-    } else if (cur) {
-        new_range = s_range_new(start, length);
-        QTAILQ_INSERT_BEFORE(cur, new_range, entry);
-    } else {
-        SignedRange *next = QTAILQ_NEXT(r, entry);
-        while (next && s_range_overlap(r->start, r->length,
-                                       next->start, next->length)) {
-            s_range_join(r, next->start, next->length);
-            QTAILQ_REMOVE(list, next, entry);
-            s_range_free(next);
-            next = QTAILQ_NEXT(r, entry);
-        }
-    }
-
-    return true;
-}
-
 #endif
qapi/string-input-visitor.c +65 -51
index 85ac6a1..0b2490b 100644
@@ -25,8 +25,8 @@ struct StringInputVisitor
 
     bool head;
 
-    SignedRangeList *ranges;
-    SignedRange *cur_range;
+    GList *ranges;
+    GList *cur_range;
     int64_t cur;
 
     const char *string;
@@ -36,24 +36,25 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
 {
     char *str = (char *) siv->string;
     long long start, end;
-    SignedRange *r, *next;
+    Range *cur;
     char *endptr;
 
     if (siv->ranges) {
         return;
     }
 
-    siv->ranges = g_malloc0(sizeof(*siv->ranges));
-    QTAILQ_INIT(siv->ranges);
     errno = 0;
     do {
         start = strtoll(str, &endptr, 0);
         if (errno == 0 && endptr > str && INT64_MIN <= start &&
             start <= INT64_MAX) {
             if (*endptr == '\0') {
-                if (!range_list_add(siv->ranges, start, 1)) {
-                    goto error;
-                }
+                cur = g_malloc0(sizeof(*cur));
+                cur->begin = start;
+                cur->end = start + 1;
+                siv->ranges = g_list_insert_sorted_merged(siv->ranges, cur,
+                                                          range_compare);
+                cur = NULL;
                 str = NULL;
             } else if (*endptr == '-') {
                 str = endptr + 1;
@@ -63,17 +64,25 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
                     (start > INT64_MAX - 65536 ||
                      end < start + 65536)) {
                     if (*endptr == '\0') {
-                        if (!range_list_add(siv->ranges, start,
-                                            end - start + 1)) {
-                            goto error;
-                        }
+                        cur = g_malloc0(sizeof(*cur));
+                        cur->begin = start;
+                        cur->end = end + 1;
+                        siv->ranges =
+                            g_list_insert_sorted_merged(siv->ranges,
+                                                        cur,
+                                                        range_compare);
+                        cur = NULL;
                         str = NULL;
                     } else if (*endptr == ',') {
                         str = endptr + 1;
-                        if (!range_list_add(siv->ranges, start,
-                                            end - start + 1)) {
-                            goto error;
-                        }
+                        cur = g_malloc0(sizeof(*cur));
+                        cur->begin = start;
+                        cur->end = end + 1;
+                        siv->ranges =
+                            g_list_insert_sorted_merged(siv->ranges,
+                                                        cur,
+                                                        range_compare);
+                        cur = NULL;
                     } else {
                         goto error;
                     }
@@ -82,9 +91,13 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
                 }
             } else if (*endptr == ',') {
                 str = endptr + 1;
-                if (!range_list_add(siv->ranges, start, 1)) {
-                    goto error;
-                }
+                cur = g_malloc0(sizeof(*cur));
+                cur->begin = start;
+                cur->end = start + 1;
+                siv->ranges = g_list_insert_sorted_merged(siv->ranges,
+                                                          cur,
+                                                          range_compare);
+                cur = NULL;
             } else {
                 goto error;
             }
@@ -95,14 +108,8 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
 
     return;
 error:
-    if (siv->ranges) {
-        QTAILQ_FOREACH_SAFE(r, siv->ranges, entry, next) {
-            QTAILQ_REMOVE(siv->ranges, r, entry);
-            g_free(r);
-        }
-        g_free(siv->ranges);
-        siv->ranges = NULL;
-    }
+    g_list_free_full(siv->ranges, g_free);
+    assert(siv->ranges == NULL);
 }
 
 static void
@@ -112,10 +119,11 @@ start_list(Visitor *v, const char *name, Error **errp)
 
     parse_str(siv, errp);
 
-    if (siv->ranges) {
-        siv->cur_range = QTAILQ_FIRST(siv->ranges);
-        if (siv->cur_range) {
-            siv->cur = siv->cur_range->start;
+    siv->cur_range = g_list_first(siv->ranges);
+    if (siv->cur_range) {
+        Range *r = siv->cur_range->data;
+        if (r) {
+            siv->cur = r->begin;
         }
     }
 }
@@ -125,19 +133,27 @@ next_list(Visitor *v, GenericList **list, Error **errp)
 {
     StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
     GenericList **link;
+    Range *r;
 
     if (!siv->ranges || !siv->cur_range) {
         return NULL;
     }
 
-    if (siv->cur < siv->cur_range->start ||
-        siv->cur >= (siv->cur_range->start + siv->cur_range->length)) {
-        siv->cur_range = QTAILQ_NEXT(siv->cur_range, entry);
-        if (siv->cur_range) {
-            siv->cur = siv->cur_range->start;
-        } else {
+    r = siv->cur_range->data;
+    if (!r) {
+        return NULL;
+    }
+
+    if (siv->cur < r->begin || siv->cur >= r->end) {
+        siv->cur_range = g_list_next(siv->cur_range);
+        if (!siv->cur_range) {
             return NULL;
         }
+        r = siv->cur_range->data;
+        if (!r) {
+            return NULL;
+        }
+        siv->cur = r->begin;
     }
 
     if (siv->head) {
@@ -176,12 +192,19 @@ static void parse_type_int(Visitor *v, int64_t *obj, const char *name,
     }
 
     if (!siv->cur_range) {
-        siv->cur_range = QTAILQ_FIRST(siv->ranges);
-        if (siv->cur_range) {
-            siv->cur = siv->cur_range->start;
-        } else {
+        Range *r;
+
+        siv->cur_range = g_list_first(siv->ranges);
+        if (!siv->cur_range) {
+            goto error;
+        }
+
+        r = siv->cur_range->data;
+        if (!r) {
             goto error;
         }
+
+        siv->cur = r->begin;
     }
 
     *obj = siv->cur;
@@ -291,16 +314,7 @@ Visitor *string_input_get_visitor(StringInputVisitor *v)
 
 void string_input_visitor_cleanup(StringInputVisitor *v)
 {
-    SignedRange *r, *next;
-
-    if (v->ranges) {
-        QTAILQ_FOREACH_SAFE(r, v->ranges, entry, next) {
-            QTAILQ_REMOVE(v->ranges, r, entry);
-            g_free(r);
-        }
-        g_free(v->ranges);
-    }
-
+    g_list_free_full(v->ranges, g_free);
     g_free(v);
 }
 
qapi/string-output-visitor.c +48 -49
index ccebc7a..1c0834a 100644
@@ -64,7 +64,7 @@ struct StringOutputVisitor
         int64_t s;
         uint64_t u;
     } range_start, range_end;
-    SignedRangeList *ranges;
+    GList *ranges;
 };
 
 static void string_output_set(StringOutputVisitor *sov, char *string)
@@ -78,25 +78,50 @@ static void string_output_set(StringOutputVisitor *sov, char *string)
 
 static void string_output_append(StringOutputVisitor *sov, int64_t a)
 {
-    range_list_add(sov->ranges, a, 1);
+    Range *r = g_malloc0(sizeof(*r));
+    r->begin = a;
+    r->end = a + 1;
+    sov->ranges = g_list_insert_sorted_merged(sov->ranges, r, range_compare);
 }
 
 static void string_output_append_range(StringOutputVisitor *sov,
                                        int64_t s, int64_t e)
 {
-    range_list_add(sov->ranges, s, e);
+    Range *r = g_malloc0(sizeof(*r));
+    r->begin = s;
+    r->end = e + 1;
+    sov->ranges = g_list_insert_sorted_merged(sov->ranges, r, range_compare);
+}
+
+static void format_string(StringOutputVisitor *sov, Range *r, bool next,
+                          bool human)
+{
+    if (r->end - r->begin > 1) {
+        if (human) {
+            g_string_append_printf(sov->string, "%" PRIx64 "-%" PRIx64,
+                                   r->begin, r->end - 1);
+
+        } else {
+            g_string_append_printf(sov->string, "%" PRId64 "-%" PRId64,
+                                   r->begin, r->end - 1);
+        }
+    } else {
+        if (human) {
+            g_string_append_printf(sov->string, "%" PRIx64, r->begin);
+        } else {
+            g_string_append_printf(sov->string, "%" PRId64, r->begin);
+        }
+    }
+    if (next) {
+        g_string_append(sov->string, ",");
+    }
 }
 
 static void print_type_int(Visitor *v, int64_t *obj, const char *name,
                            Error **errp)
 {
     StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
-    SignedRange *r;
-
-    if (!sov->ranges) {
-        sov->ranges = g_malloc0(sizeof(*sov->ranges));
-        QTAILQ_INIT(sov->ranges);
-    }
+    GList *l;
 
     switch (sov->list_mode) {
     case LM_NONE:
@@ -118,8 +143,7 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
             } else {
                 assert(sov->range_start.s < sov->range_end.s);
                 string_output_append_range(sov, sov->range_start.s,
-                                           sov->range_end.s -
-                                           sov->range_start.s + 1);
+                                           sov->range_end.s);
             }
 
             sov->range_start.s = *obj;
@@ -132,8 +156,7 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
             sov->range_end.s++;
             assert(sov->range_start.s < sov->range_end.s);
             string_output_append_range(sov, sov->range_start.s,
-                                       sov->range_end.s -
-                                       sov->range_start.s + 1);
+                                       sov->range_end.s);
         } else {
             if (sov->range_start.s == sov->range_end.s) {
                 string_output_append(sov, sov->range_end.s);
@@ -141,8 +164,7 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
                 assert(sov->range_start.s < sov->range_end.s);
 
                 string_output_append_range(sov, sov->range_start.s,
-                                           sov->range_end.s -
-                                           sov->range_start.s + 1);
+                                           sov->range_end.s);
             }
             string_output_append(sov, *obj);
         }
@@ -152,34 +174,20 @@ static void print_type_int(Visitor *v, int64_t *obj, const char *name,
         abort();
     }
 
-    QTAILQ_FOREACH(r, sov->ranges, entry) {
-        if (r->length > 1) {
-            g_string_append_printf(sov->string, "%" PRId64 "-%" PRId64,
-                                   r->start,
-                                   s_range_end(r->start, r->length));
-        } else {
-            g_string_append_printf(sov->string, "%" PRId64,
-                                   r->start);
-        }
-        if (r->entry.tqe_next) {
-            g_string_append(sov->string, ",");
-        }
+    l = sov->ranges;
+    while (l) {
+        Range *r = l->data;
+        format_string(sov, r, l->next != NULL, false);
+        l = l->next;
     }
 
     if (sov->human) {
+        l = sov->ranges;
         g_string_append(sov->string, " (");
-        QTAILQ_FOREACH(r, sov->ranges, entry) {
-            if (r->length > 1) {
-                g_string_append_printf(sov->string, "%" PRIx64 "-%" PRIx64,
-                                       r->start,
-                                       s_range_end(r->start, r->length));
-            } else {
-                g_string_append_printf(sov->string, "%" PRIx64,
-                                       r->start);
-            }
-            if (r->entry.tqe_next) {
-                g_string_append(sov->string, ",");
-            }
+        while (l) {
+            Range *r = l->data;
+            format_string(sov, r, l->next != NULL, false);
+            l = l->next;
         }
         g_string_append(sov->string, ")");
     }
@@ -310,20 +318,11 @@ Visitor *string_output_get_visitor(StringOutputVisitor *sov)
 
 void string_output_visitor_cleanup(StringOutputVisitor *sov)
 {
-    SignedRange *r, *next;
-
     if (sov->string) {
         g_string_free(sov->string, true);
     }
 
-    if (sov->ranges) {
-        QTAILQ_FOREACH_SAFE(r, sov->ranges, entry, next) {
-            QTAILQ_REMOVE(sov->ranges, r, entry);
-            s_range_free(r);
-        }
-        g_free(sov->ranges);
-    }
-
+    g_list_free_full(sov->ranges, g_free);
     g_free(sov);
 }
 
tests/test-string-input-visitor.c +2 -2
index b08a7db..b01e2f2 100644
@@ -67,13 +67,13 @@ static void test_visitor_in_int(TestInputVisitorData *data,
 static void test_visitor_in_intList(TestInputVisitorData *data,
                                     const void *unused)
 {
-    int64_t value[] = {-2, -1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
+    int64_t value[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
     int16List *res = NULL, *tmp;
     Error *errp = NULL;
     Visitor *v;
     int i = 0;
 
-    v = visitor_input_test_init(data, "1,2,-2-1,2-4,20,5-9,1-8");
+    v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
 
     visit_type_int16List(v, &res, NULL, &errp);
     g_assert(errp == NULL);
tests/test-string-output-visitor.c +4 -4
index d2ad580..28e7359 100644
@@ -44,7 +44,7 @@ static void visitor_output_teardown(TestOutputVisitorData *data,
 static void test_visitor_out_int(TestOutputVisitorData *data,
                                  const void *unused)
 {
-    int64_t value = -42;
+    int64_t value = 42;
     Error *err = NULL;
     char *str;
 
@@ -53,14 +53,14 @@ static void test_visitor_out_int(TestOutputVisitorData *data,
 
     str = string_output_get_string(data->sov);
     g_assert(str != NULL);
-    g_assert_cmpstr(str, ==, "-42");
+    g_assert_cmpstr(str, ==, "42");
     g_free(str);
 }
 
 static void test_visitor_out_intList(TestOutputVisitorData *data,
                                      const void *unused)
 {
-    int64_t value[] = {-10, -7, -2, -1, 0, 1, 9, 10, 16, 15, 14,
+    int64_t value[] = {0, 1, 9, 10, 16, 15, 14,
         3, 4, 5, 6, 11, 12, 13, 21, 22, INT64_MAX - 1, INT64_MAX};
     intList *list = NULL, **tmp = &list;
     int i;
@@ -79,7 +79,7 @@ static void test_visitor_out_intList(TestOutputVisitorData *data,
     str = string_output_get_string(data->sov);
     g_assert(str != NULL);
     g_assert_cmpstr(str, ==,
-        "-10,-7,-2-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807");
+        "0-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807");
     g_free(str);
     while (list) {
         intList *tmp2;
Signature · 1 line
1.9.3
author
#32 lore raw

RFC PATCH 2/4 check if we have space left for hotplugged memory

In reply to Hu Tao

If pc-dimm is specified on qemu command line, but only with -m size (aka not -m size,maxmem,slots) then qemu will core dump.

This patch fixes the problem.

---
 hw/mem/pc-dimm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
hw/mem/pc-dimm.c +6 -1
index 8c26568..6e8bf43 100644
@@ -107,7 +107,12 @@ uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
     uint64_t new_addr, ret = 0;
     uint64_t address_space_end = address_space_start + address_space_size;
 
-    assert(address_space_end > address_space_size);
+    if (address_space_size == 0) {
+        error_setg(errp, "can't add memory beyond 0x%" PRIx64,
+                   address_space_end);
+        goto out;
+    }
+
     object_child_foreach(qdev_get_machine(), pc_dimm_built_list, &list);
 
     if (hint) {
Signature · 1 line
1.9.3
#33 lore raw

Re: [Qemu-devel] [PATCH RFC 2/4] check if we have space left for hotplugged memory

Quoted 15 lines · Hu Tao

If pc-dimm is specified on qemu command line, but only with -m size (aka not -m size,maxmem,slots) then qemu will core dump.

This patch fixes the problem.

---
 hw/mem/pc-dimm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
hw/mem/pc-dimm.c +0 -0
index 8c26568..6e8bf43 100644

hw/mem/pc-dimm.c uint64_t pc_dimm_get_free_addr(uint64_t address_space_start, · line 112

Quoted 7 lines
hw/mem/pc-dimm.c +3 -1
@@ -107,7 +107,12 @@ uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
     uint64_t new_addr, ret = 0;
     uint64_t address_space_end = address_space_start + address_space_size;
 
-    assert(address_space_end > address_space_size);
+    if (address_space_size == 0) {
+        error_setg(errp, "can't add memory beyond 0x%" PRIx64,
+                   address_space_end);

That's quite an unfriendly error message, isn't it? Why not explain what the problem is to the user?

hw/mem/pc-dimm.c uint64_t pc_dimm_get_free_addr(uint64_t address_space_start, · line 115

Quoted 3 lines
hw/mem/pc-dimm.c +3 -0
+        goto out;
+    }
+

I would move the assert to this point. It protects against integer overflow.

Quoted 7 lines
hw/mem/pc-dimm.c +0 -0
     object_child_foreach(qdev_get_machine(), pc_dimm_built_list, &list);
 
     if (hint) {
Signature · 2 lines
-- 
1.9.3
author
#34 lore raw

Re: [Qemu-devel] [PATCH RFC 2/4] check if we have space left for hotplugged memory

Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Hu Tao

If pc-dimm is specified on qemu command line, but only with -m size (aka not -m size,maxmem,slots) then qemu will core dump.

This patch fixes the problem.

---
 hw/mem/pc-dimm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
hw/mem/pc-dimm.c +0 -0
index 8c26568..6e8bf43 100644
@@ -107,7 +107,12 @@ uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
     uint64_t new_addr, ret = 0;
     uint64_t address_space_end = address_space_start + address_space_size;
 
Quoted 7 lines

hw/mem/pc-dimm.c uint64_t pc_dimm_get_free_addr(uint64_t address_space_start, · line 112

Quoted 4 lines
hw/mem/pc-dimm.c +3 -1
-    assert(address_space_end > address_space_size);
+    if (address_space_size == 0) {
+        error_setg(errp, "can't add memory beyond 0x%" PRIx64,
+                   address_space_end);

That's quite an unfriendly error message, isn't it? Why not explain what the problem is to the user?

Thanks! patch updated.

Quoted 15 lines

hw/mem/pc-dimm.c uint64_t pc_dimm_get_free_addr(uint64_t address_space_start, · line 115

Quoted 4 lines
hw/mem/pc-dimm.c +3 -0
+        goto out;
+    }
+

I would move the assert to this point. It protects against integer overflow.

Quoted 7 lines
hw/mem/pc-dimm.c +0 -0
     object_child_foreach(qdev_get_machine(), pc_dimm_built_list, &list);
 
     if (hint) {
Signature · 2 lines
-- 
1.9.3
author
#35 lore raw

RFC PATCH 3/4 exec: don't exit unconditionally if failed to allocate memory

In reply to Hu Tao

return -1 instead.

Now user can add objects memory-backend-ram on-the-fly, fail it if cannot allocate memory rather than quit qemu.

---
 backends/hostmem-ram.c | 3 +++
 exec.c                 | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)
backends/hostmem-ram.c +3 -0
index d9a8290..afb305d 100644
@@ -28,6 +28,9 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     path = object_get_canonical_path_component(OBJECT(backend));
     memory_region_init_ram(&backend->mr, OBJECT(backend), path,
                            backend->size);
+    if (backend->mr.ram_addr == -1) {
+        error_setg(errp, "can't allocate memory");
+    }
     g_free(path);
 }
 
exec.c +5 -1
index 8705cc5..74560e5 100644
@@ -1228,7 +1228,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block)
             if (!new_block->host) {
                 fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
                         new_block->mr->name, strerror(errno));
-                exit(1);
+                return -1;
             }
             memory_try_enable_merging(new_block->host, new_block->length);
         }
@@ -1356,6 +1356,10 @@ void qemu_ram_free(ram_addr_t addr)
 {
     RAMBlock *block;
 
+    if (addr == -1) {
+        return;
+    }
+
     /* This assumes the iothread lock is taken here too.  */
     qemu_mutex_lock_ramlist();
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
Signature · 1 line
1.9.3
#36 lore raw

Re: [Qemu-devel] [PATCH RFC 3/4] exec: don't exit unconditionally if failed to allocate memory

Il 14/06/2014 06:48, Hu Tao ha scritto:

Quoted 6 lines

return -1 instead.

Now user can add objects memory-backend-ram on-the-fly, fail it if cannot allocate memory rather than quit qemu.

This needs an audit of all callers or, alternatively, we need to add memory_region_init_ram_nofail. Better leave it for after the merge.

Paolo

Quoted 44 lines
---
 backends/hostmem-ram.c | 3 +++
 exec.c                 | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)
backends/hostmem-ram.c +3 -0
index d9a8290..afb305d 100644
@@ -28,6 +28,9 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     path = object_get_canonical_path_component(OBJECT(backend));
     memory_region_init_ram(&backend->mr, OBJECT(backend), path,
                            backend->size);
+    if (backend->mr.ram_addr == -1) {
+        error_setg(errp, "can't allocate memory");
+    }
     g_free(path);
 }
 
exec.c +5 -1
index 8705cc5..74560e5 100644
@@ -1228,7 +1228,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block)
             if (!new_block->host) {
                 fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
                         new_block->mr->name, strerror(errno));
-                exit(1);
+                return -1;
             }
             memory_try_enable_merging(new_block->host, new_block->length);
         }
@@ -1356,6 +1356,10 @@ void qemu_ram_free(ram_addr_t addr)
 {
     RAMBlock *block;
 
+    if (addr == -1) {
+        return;
+    }
+
     /* This assumes the iothread lock is taken here too.  */
     qemu_mutex_lock_ramlist();
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
#37 lore raw

Re: [Qemu-devel] [PATCH RFC 3/4] exec: don't exit unconditionally if failed to allocate memory

Quoted 6 lines · Paolo Bonzini

Il 14/06/2014 06:48, Hu Tao ha scritto:

Quoted 5 lines

return -1 instead.

Now user can add objects memory-backend-ram on-the-fly, fail it if cannot allocate memory rather than quit qemu.

Quoted 6 lines
Quoted 1 line

This needs an audit of all callers or, alternatively, we need to add memory_region_init_ram_nofail. Better leave it for after the merge.

Paolo

Specifically memory_region_init_ram_from_file does not seem to handle failures.

qemu_ram_free chunk also looks weird. Can we not avoid calling free on invalid addresses?

Quoted 44 lines
Quoted 44 lines

---
backends/hostmem-ram.c | 3 +++
exec.c | 6 +++++-

2 files changed, 8 insertions(+), 1 deletion(-)
backends/hostmem-ram.c +3 -0
index d9a8290..afb305d 100644
@@ -28,6 +28,9 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
    path = object_get_canonical_path_component(OBJECT(backend));
    memory_region_init_ram(&backend->mr, OBJECT(backend), path,
                           backend->size);
+    if (backend->mr.ram_addr == -1) {
+        error_setg(errp, "can't allocate memory");
+    }
    g_free(path);
 }
 
exec.c +5 -1
index 8705cc5..74560e5 100644
@@ -1228,7 +1228,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block)
            if (!new_block->host) {
                fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
                        new_block->mr->name, strerror(errno));
-                exit(1);
+                return -1;
            }
            memory_try_enable_merging(new_block->host, new_block->length);
        }
@@ -1356,6 +1356,10 @@ void qemu_ram_free(ram_addr_t addr)
 {
    RAMBlock *block;
 
+    if (addr == -1) {
+        return;
+    }
+
    /* This assumes the iothread lock is taken here too.  */
    qemu_mutex_lock_ramlist();
    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
author
#38 lore raw

Re: [Qemu-devel] [PATCH RFC 3/4] exec: don't exit unconditionally if failed to allocate memory

Quoted 11 lines · Michael S. Tsirkin
Quoted 10 lines · Paolo Bonzini

Il 14/06/2014 06:48, Hu Tao ha scritto:

Quoted 6 lines

return -1 instead.

Now user can add objects memory-backend-ram on-the-fly, fail it if cannot allocate memory rather than quit qemu.

This needs an audit of all callers or, alternatively, we need to add memory_region_init_ram_nofail. Better leave it for after the merge.

Paolo, IIUC you suggested we fix it after merge.

Quoted 5 lines
Quoted 2 lines

Paolo

Specifically memory_region_init_ram_from_file does not seem to handle failures.

memory_region_init_ram_from_file has an errp. Also see my updated patch.

Quoted 3 lines

qemu_ram_free chunk also looks weird. Can we not avoid calling free on invalid addresses?

but we still need to check the address somewhere if not in qemu_ram_free.

Quoted 45 lines
Quoted 45 lines
Quoted 45 lines

---
backends/hostmem-ram.c | 3 +++
exec.c | 6 +++++-

2 files changed, 8 insertions(+), 1 deletion(-)
backends/hostmem-ram.c +3 -0
index d9a8290..afb305d 100644
@@ -28,6 +28,9 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
    path = object_get_canonical_path_component(OBJECT(backend));
    memory_region_init_ram(&backend->mr, OBJECT(backend), path,
                           backend->size);
+    if (backend->mr.ram_addr == -1) {
+        error_setg(errp, "can't allocate memory");
+    }
    g_free(path);
 }
 
exec.c +5 -1
index 8705cc5..74560e5 100644
@@ -1228,7 +1228,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block)
            if (!new_block->host) {
                fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
                        new_block->mr->name, strerror(errno));
-                exit(1);
+                return -1;
            }
            memory_try_enable_merging(new_block->host, new_block->length);
        }
@@ -1356,6 +1356,10 @@ void qemu_ram_free(ram_addr_t addr)
 {
    RAMBlock *block;
 
+    if (addr == -1) {
+        return;
+    }
+
    /* This assumes the iothread lock is taken here too.  */
    qemu_mutex_lock_ramlist();
    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
#39 lore raw

Re: [Qemu-devel] [PATCH RFC 3/4] exec: don't exit unconditionally if failed to allocate memory

Il 16/06/2014 11:54, Hu Tao ha scritto:

Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

This needs an audit of all callers or, alternatively, we need to add memory_region_init_ram_nofail. Better leave it for after the merge.

Paolo, IIUC you suggested we fix it after merge.

Yes.

Quoted 3 lines
Quoted 2 lines
Quoted 2 lines

Specifically memory_region_init_ram_from_file does not seem to handle failures.

memory_region_init_ram_from_file has an errp. Also see my updated patch.

Yes, memory_region_init_ram_from_file is okay. memory_region_init_ram is the one that doesn't handle failures.

Quoted 5 lines
Quoted 2 lines

qemu_ram_free chunk also looks weird. Can we not avoid calling free on invalid addresses?

but we still need to check the address somewhere if not in qemu_ram_free.

If we add an errp to memory_region_init_ram, the qemu_ram_free hunk probably will not be necessary anymore.

Paolo

author
#40 lore raw

RFC PATCH 4/4 memory-backend-file: error out if failed to allocate memory

In reply to Hu Tao

If user adds a memory-backend-file object using object_add command, specifying a non-existing directory for property mem-path, qemu will core dump with message:

/nonexistingdir: No such file or directory
Bad ram offset fffffffffffff000
Aborted (core dumped)

This patch fixes this problem.

---
 backends/hostmem-file.c | 3 +++
 1 file changed, 3 insertions(+)
backends/hostmem-file.c +3 -0
index 5179994..70172d1 100644
@@ -55,6 +55,9 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
                                  object_get_canonical_path(OBJECT(backend)),
                                  backend->size, fb->share,
                                  fb->mem_path, errp);
+        if (backend->mr.ram_addr == -1) {
+            error_setg(errp, "failed to allocate memory");
+        }
     }
 #endif
 }
Signature · 1 line
1.9.3
#41 lore raw

Re: [Qemu-devel] [PATCH RFC 4/4] memory-backend-file: error out if failed to allocate memory

Il 14/06/2014 06:48, Hu Tao ha scritto:

Quoted 20 lines

If user adds a memory-backend-file object using object_add command, specifying a non-existing directory for property mem-path, qemu will core dump with message:

/nonexistingdir: No such file or directory
Bad ram offset fffffffffffff000
Aborted (core dumped)

This patch fixes this problem.

---
 backends/hostmem-file.c | 3 +++
 1 file changed, 3 insertions(+)
backends/hostmem-file.c +0 -0
index 5179994..70172d1 100644

backends/hostmem-file.c file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) · line 60

Quoted 6 lines
backends/hostmem-file.c +3 -0
@@ -55,6 +55,9 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
                                  object_get_canonical_path(OBJECT(backend)),
                                  backend->size, fb->share,
                                  fb->mem_path, errp);
+        if (backend->mr.ram_addr == -1) {
+            error_setg(errp, "failed to allocate memory");
+        }

qemu_ram_alloc_from_file is where this error_setg should be added instead.

Paolo

Quoted 4 lines
backends/hostmem-file.c +0 -0
     }
 #endif
 }
author
#42 lore raw

Re: [Qemu-devel] [PATCH RFC 4/4] memory-backend-file: error out if failed to allocate memory

Quoted 16 lines · Paolo Bonzini

Il 14/06/2014 06:48, Hu Tao ha scritto:

Quoted 15 lines

If user adds a memory-backend-file object using object_add command, specifying a non-existing directory for property mem-path, qemu will core dump with message:

/nonexistingdir: No such file or directory
Bad ram offset fffffffffffff000
Aborted (core dumped)

This patch fixes this problem.

---
backends/hostmem-file.c | 3 +++

1 file changed, 3 insertions(+)
Quoted 13 lines

backends/hostmem-file.c file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) · line 60

Quoted 11 lines
backends/hostmem-file.c +3 -0
index 5179994..70172d1 100644
@@ -55,6 +55,9 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
                                 object_get_canonical_path(OBJECT(backend)),
                                 backend->size, fb->share,
                                 fb->mem_path, errp);
+        if (backend->mr.ram_addr == -1) {
+            error_setg(errp, "failed to allocate memory");
+        }

qemu_ram_alloc_from_file is where this error_setg should be added instead.

Thanks, patch updated.

Quoted 7 lines

Paolo

Quoted 4 lines
backends/hostmem-file.c +0 -0
    }
 #endif
 }
#43 lore raw

Re: [Qemu-devel] [PATCH RFC 0/4] fixes for pci tree

In reply to Hu Tao
Quoted 5 lines · Hu Tao

Michael,

This is fixes for your pci tree.

patch 1 remove signed range as requested.

This also fixes make check failures so I applied this.

Others don't look like regressions to me - this is error handling in new functionality, correct? Thus I'll wait for comments on these to be resolved, and hopefully for some acks.

Quoted 73 lines

There are 3 problems in current pci tree, as follows:

1. pc-dimm specified on command line but only -m size (aka not -m size,maxmem,slots)

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -object memory-backend-ram,size=512M,id=ram-node0,prealloc=y,policy=bind,host-nodes=0 -device pc-dimm,id=d0,memdev=ram-node0 -m 640M -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm

result:

qemu/hw/mem/pc-dimm.c:110: pc_dimm_get_free_addr: Assertion `address_space_end > address_space_size' failed. Aborted (core dumped)

patch 2 fixes this.

2. using qemu monitor command object-add to add a memory-backend-ram object whose's size is too big

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -m 512M -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm

in monitor:
(qemu)object_add memory-backend-ram,size=40960G,id=mem0

result:

qemu just exits with message: Cannot set up guest memory 'mem0': Cannot allocate memory

patch 3 fixes this.

3. specifying a non-existing directory for memory-backend-file

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -m 512M,maxmem=1000G,slots=100 -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm -object memory-backend-file,size=512M,id=mem0,mem-path=/nonexistingdir -device pc-dimm,id=d0,memdev=mem0

result:

/nonexistingdir: No such file or directory
Bad ram offset fffffffffffff000
Aborted (core dumped)

patch 4 fixes this.

please review. Thanks!

Hu Tao (4):
get rid of signed range
check if we have space left for hotplugged memory
exec: don't exit unconditionally if failed to allocate memory
memory-backend-file: error out if failed to allocate memory

 backends/hostmem-file.c            |   3 +
 backends/hostmem-ram.c             |   3 +
 exec.c                             |   6 +-
 hw/mem/pc-dimm.c                   |   7 +-
 include/qemu/range.h               | 144 ++++++++++++-------------------------
 qapi/string-input-visitor.c        | 116 +++++++++++++++++-------------
 qapi/string-output-visitor.c       |  97 +++++++++++++------------
 tests/test-string-input-visitor.c  |   4 +-
 tests/test-string-output-visitor.c |   8 +--
 9 files changed, 182 insertions(+), 206 deletions(-)
Signature · 1 line
1.9.3
author
#44 lore raw

Re: [Qemu-devel] [PATCH RFC 0/4] fixes for pci tree

Quoted 11 lines · Michael S. Tsirkin
Quoted 5 lines · Hu Tao

Michael,

This is fixes for your pci tree.

patch 1 remove signed range as requested.

This also fixes make check failures so I applied this.

Others don't look like regressions to me - this is error handling in new functionality, correct?

Yes.

BTW, thre are two more problems:

1. if numa node number doesn't start from 0 then qemu will core dump.

cmd line:

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 128M,maxmem=2G,slots=3 -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm -object memory-backend-ram,id=m1,size=128M -numa node,nodeid=1,cpus=1,memdev=m1

This problem can be fixed by:

numa.c +4 -1
index ce9382d..b00c5cf 100644
@@ -270,10 +270,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Obj
     }
 
     memory_region_init(mr, owner, name, ram_size);
-    for (i = 0; i < nb_numa_nodes; i++) {
+    for (i = 0; i < MAX_NODES; i++) {
         Error *local_err = NULL;
         uint64_t size = numa_info[i].node_mem;
         HostMemoryBackend *backend = numa_info[i].node_memdev;
+        if (!backend) {
+            continue;
+        }
         MemoryRegion *seg = host_memory_backend_get_memory(backend, &local_err);
         if (local_err) {
             qerror_report_err(local_err);

2. your current pci tree doesn't compile because patch 'qmp: add query-memdev' is dropped while commit 5b517e74ed7825(hmp: add info memdev) depends on it.

but patch 'qmp: add query-memdev' itself has a problem: if memory-backend-ram is on the command line but no numa, info memdev returns nothing:

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 128M,maxmem=2G,slots=3 -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm -object memory-backend-ram,id=m1,size=128M QEMU 2.0.50 monitor - type 'help' for more information
(qemu) info memdev
(nothing returned)

even worse, if with -numa mem=size then qemu will core dump:

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 128M,maxmem=2G,slots=3 -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm -object memory-backend-ram,id=m1,size=128M -numa node,nodeid=0,cpus=0,mem=128M (qemu) info memdev
Segmentation fault (core dumped)

this is because query_memdev searchs for memdev information in numa_info[].node_memdev, which don't have value if there is no numa or numa isn't used with memory-backend-ram.

the solution can be gather memdevs in a list. Or query at /objects?

Quoted 76 lines

Thus I'll wait for comments on these to be resolved, and hopefully for some acks.

Quoted 73 lines

There are 3 problems in current pci tree, as follows:

1. pc-dimm specified on command line but only -m size (aka not -m size,maxmem,slots)

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -object memory-backend-ram,size=512M,id=ram-node0,prealloc=y,policy=bind,host-nodes=0 -device pc-dimm,id=d0,memdev=ram-node0 -m 640M -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm

result:

qemu/hw/mem/pc-dimm.c:110: pc_dimm_get_free_addr: Assertion `address_space_end > address_space_size' failed. Aborted (core dumped)

patch 2 fixes this.

2. using qemu monitor command object-add to add a memory-backend-ram object whose's size is too big

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -m 512M -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm

in monitor:
(qemu)object_add memory-backend-ram,size=40960G,id=mem0

result:

qemu just exits with message: Cannot set up guest memory 'mem0': Cannot allocate memory

patch 3 fixes this.

3. specifying a non-existing directory for memory-backend-file

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -m 512M,maxmem=1000G,slots=100 -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm -object memory-backend-file,size=512M,id=mem0,mem-path=/nonexistingdir -device pc-dimm,id=d0,memdev=mem0

result:

/nonexistingdir: No such file or directory
Bad ram offset fffffffffffff000
Aborted (core dumped)

patch 4 fixes this.

please review. Thanks!

Hu Tao (4):
get rid of signed range
check if we have space left for hotplugged memory
exec: don't exit unconditionally if failed to allocate memory
memory-backend-file: error out if failed to allocate memory

 backends/hostmem-file.c            |   3 +
 backends/hostmem-ram.c             |   3 +
 exec.c                             |   6 +-
 hw/mem/pc-dimm.c                   |   7 +-
 include/qemu/range.h               | 144 ++++++++++++-------------------------
 qapi/string-input-visitor.c        | 116 +++++++++++++++++-------------
 qapi/string-output-visitor.c       |  97 +++++++++++++------------
 tests/test-string-input-visitor.c  |   4 +-
 tests/test-string-output-visitor.c |   8 +--
 9 files changed, 182 insertions(+), 206 deletions(-)
Signature · 1 line
1.9.3
#45 lore raw

Re: [Qemu-devel] [PATCH RFC 0/4] fixes for pci tree

Quoted 49 lines · Hu Tao
Quoted 11 lines · Michael S. Tsirkin
Quoted 5 lines · Hu Tao

Michael,

This is fixes for your pci tree.

patch 1 remove signed range as requested.

This also fixes make check failures so I applied this.

Others don't look like regressions to me - this is error handling in new functionality, correct?

Yes.

BTW, thre are two more problems:

1. if numa node number doesn't start from 0 then qemu will core dump.

cmd line:

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 128M,maxmem=2G,slots=3 -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm -object memory-backend-ram,id=m1,size=128M -numa node,nodeid=1,cpus=1,memdev=m1

This problem can be fixed by:

numa.c +4 -1
index ce9382d..b00c5cf 100644
@@ -270,10 +270,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Obj
     }
 
     memory_region_init(mr, owner, name, ram_size);
-    for (i = 0; i < nb_numa_nodes; i++) {
+    for (i = 0; i < MAX_NODES; i++) {
         Error *local_err = NULL;
         uint64_t size = numa_info[i].node_mem;
         HostMemoryBackend *backend = numa_info[i].node_memdev;
+        if (!backend) {
+            continue;
+        }
         MemoryRegion *seg = host_memory_backend_get_memory(backend, &local_err);
         if (local_err) {
             qerror_report_err(local_err);
Quoted 2 lines

2. your current pci tree doesn't compile because patch 'qmp: add query-memdev' is dropped while commit 5b517e74ed7825(hmp: add info memdev) depends on it.

Hmm it builds for me.
Does it only build if some library is present?

Quoted 10 lines

but patch 'qmp: add query-memdev' itself has a problem: if memory-backend-ram is on the command line but no numa, info memdev returns nothing:

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 128M,maxmem=2G,slots=3 -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm -object memory-backend-ram,id=m1,size=128M QEMU 2.0.50 monitor - type 'help' for more information
(qemu) info memdev
(nothing returned)

even worse, if with -numa mem=size then qemu will core dump:

Quoted 8 lines

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 128M,maxmem=2G,slots=3 -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm -object memory-backend-ram,id=m1,size=128M -numa node,nodeid=0,cpus=0,mem=128M (qemu) info memdev
Segmentation fault (core dumped)

this is because query_memdev searchs for memdev information in numa_info[].node_memdev, which don't have value if there is no numa or numa isn't used with memory-backend-ram.

the solution can be gather memdevs in a list. Or query at /objects?

query QOM seems nicer.

Quoted 76 lines
Quoted 76 lines

Thus I'll wait for comments on these to be resolved, and hopefully for some acks.

Quoted 73 lines

There are 3 problems in current pci tree, as follows:

1. pc-dimm specified on command line but only -m size (aka not -m size,maxmem,slots)

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -object memory-backend-ram,size=512M,id=ram-node0,prealloc=y,policy=bind,host-nodes=0 -device pc-dimm,id=d0,memdev=ram-node0 -m 640M -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm

result:

qemu/hw/mem/pc-dimm.c:110: pc_dimm_get_free_addr: Assertion `address_space_end > address_space_size' failed. Aborted (core dumped)

patch 2 fixes this.

2. using qemu monitor command object-add to add a memory-backend-ram object whose's size is too big

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -m 512M -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm

in monitor:
(qemu)object_add memory-backend-ram,size=40960G,id=mem0

result:

qemu just exits with message: Cannot set up guest memory 'mem0': Cannot allocate memory

patch 3 fixes this.

3. specifying a non-existing directory for memory-backend-file

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -smp 2 -m 512M,maxmem=1000G,slots=100 -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm -object memory-backend-file,size=512M,id=mem0,mem-path=/nonexistingdir -device pc-dimm,id=d0,memdev=mem0

result:

/nonexistingdir: No such file or directory
Bad ram offset fffffffffffff000
Aborted (core dumped)

patch 4 fixes this.

please review. Thanks!

Hu Tao (4):
get rid of signed range
check if we have space left for hotplugged memory
exec: don't exit unconditionally if failed to allocate memory
memory-backend-file: error out if failed to allocate memory

 backends/hostmem-file.c            |   3 +
 backends/hostmem-ram.c             |   3 +
 exec.c                             |   6 +-
 hw/mem/pc-dimm.c                   |   7 +-
 include/qemu/range.h               | 144 ++++++++++++-------------------------
 qapi/string-input-visitor.c        | 116 +++++++++++++++++-------------
 qapi/string-output-visitor.c       |  97 +++++++++++++------------
 tests/test-string-input-visitor.c  |   4 +-
 tests/test-string-output-visitor.c |   8 +--
 9 files changed, 182 insertions(+), 206 deletions(-)
Signature · 1 line
1.9.3
author
#46 lore raw

Re: [Qemu-devel] [PATCH RFC 0/4] fixes for pci tree

Quoted 50 lines · Michael S. Tsirkin
Quoted 49 lines · Hu Tao
Quoted 11 lines · Michael S. Tsirkin
Quoted 5 lines · Hu Tao

Michael,

This is fixes for your pci tree.

patch 1 remove signed range as requested.

This also fixes make check failures so I applied this.

Others don't look like regressions to me - this is error handling in new functionality, correct?

Yes.

BTW, thre are two more problems:

1. if numa node number doesn't start from 0 then qemu will core dump.

cmd line:

./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 128M,maxmem=2G,slots=3 -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm -object memory-backend-ram,id=m1,size=128M -numa node,nodeid=1,cpus=1,memdev=m1

This problem can be fixed by:

numa.c +4 -1
index ce9382d..b00c5cf 100644
@@ -270,10 +270,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Obj
     }
 
     memory_region_init(mr, owner, name, ram_size);
-    for (i = 0; i < nb_numa_nodes; i++) {
+    for (i = 0; i < MAX_NODES; i++) {
         Error *local_err = NULL;
         uint64_t size = numa_info[i].node_mem;
         HostMemoryBackend *backend = numa_info[i].node_memdev;
+        if (!backend) {
+            continue;
+        }
         MemoryRegion *seg = host_memory_backend_get_memory(backend, &local_err);
         if (local_err) {
             qerror_report_err(local_err);
Quoted 5 lines
Quoted 2 lines

2. your current pci tree doesn't compile because patch 'qmp: add query-memdev' is dropped while commit 5b517e74ed7825(hmp: add info memdev) depends on it.

Hmm it builds for me.
Does it only build if some library is present?

No, it always builds.

Failed to build both on Fedora 17 and 20. My commandline to build:

../configure --target-list=x86_64-softmmu && make -j4

These are the error messages:

/mnt/data/kernel/qemu/hmp.c: In function ‘hmp_info_memdev’:
/mnt/data/kernel/qemu/hmp.c:1685:5: error: unknown type name ‘MemdevList’
/mnt/data/kernel/qemu/hmp.c:1685:5: error: implicit declaration of function ‘qmp_query_memdev’ [-Werror=implicit-function-declaration] /mnt/data/kernel/qemu/hmp.c:1685:5: error: nested extern declaration of ‘qmp_query_memdev’ [-Werror=nested-externs] /mnt/data/kernel/qemu/hmp.c:1685:31: error: initialization makes pointer from integer without a cast [-Werror] /mnt/data/kernel/qemu/hmp.c:1686:5: error: unknown type name ‘MemdevList’
/mnt/data/kernel/qemu/hmp.c:1694:33: error: request for member ‘value’ in something not a structure or union /mnt/data/kernel/qemu/hmp.c:1696:56: error: request for member ‘value’ in something not a structure or union /mnt/data/kernel/qemu/hmp.c:1698:25: error: request for member ‘value’ in something not a structure or union /mnt/data/kernel/qemu/hmp.c:1700:25: error: request for member ‘value’ in something not a structure or union /mnt/data/kernel/qemu/hmp.c:1702:25: error: request for member ‘value’ in something not a structure or union /mnt/data/kernel/qemu/hmp.c:1704:46: error: request for member ‘value’ in something not a structure or union /mnt/data/kernel/qemu/hmp.c:1709:14: error: request for member ‘next’ in something not a structure or union
cc1: all warnings being treated as errors
make: *** [hmp.o] Error 1