Skip to contentSkip to conversation

qapi/range/memory-device: fixes and cleanups

PATCH Superseded David Hildenbrand 2 participants 8 messages Dec 12, 2018 – Dec 13, 2018

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

David Hildenbrand

author
#0 lore raw

v4 cover qapi/range/memory-device: fixes and cleanups

These are the two leftovers from
[PATCH v3 0/7] qapi/range/memory-device: fixes and cleanups

The remaining patches extent the QEMU range code and rewrite memory-device code to make use of it.

v3 -> v4:
- "memory-device: rewrite address assignment using ranges" -- Use better error messages
-- Fix one theretical overflow

David Hildenbrand (2):
range: add some more functions
memory-device: rewrite address assignment using ranges

 hw/mem/memory-device.c | 54 ++++++++++++++++++++----------------
 include/qemu/range.h   | 62 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+), 23 deletions(-)
Signature · 1 line
2.17.2
author
#1 lore raw

PATCH v4 1/2 range: add some more functions

Add some more functions that will be used in memory-device context.

range_init(): Init using lower bound and size, check for validity range_init_nofail(): Init using lower bound and size, validity asserted range_size(): Extract the size of a range range_overlaps_range(): Check for overlaps of two ranges range_contains_range(): Check if one range is contained in the other

Reviewed-by Igor Mammedov <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 include/qemu/range.h | 62 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
include/qemu/range.h +62 -0
index 7e75f4e655..ba606c6bc0 100644
@@ -112,6 +112,68 @@ static inline uint64_t range_upb(Range *range)
     return range->upb;
 }
 
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. If the range would overflow, returns -ERANGE, otherwise
+ * 0.
+ */
+static inline int QEMU_WARN_UNUSED_RESULT range_init(Range *range, uint64_t lob,
+                                                     uint64_t size)
+{
+    if (lob + size < lob) {
+        return -ERANGE;
+    }
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+    return 0;
+}
+
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. Range must not overflow.
+ */
+static inline void range_init_nofail(Range *range, uint64_t lob, uint64_t size)
+{
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+}
+
+/*
+ * Get the size of @range.
+ */
+static inline uint64_t range_size(const Range *range)
+{
+    return range->upb - range->lob + 1;
+}
+
+/*
+ * Check if @range1 overlaps with @range2. If one of the ranges is empty,
+ * the result is always "false".
+ */
+static inline bool range_overlaps_range(const Range *range1,
+                                        const Range *range2)
+{
+    if (range_is_empty(range1) || range_is_empty(range2)) {
+        return false;
+    }
+    return !(range2->upb < range1->lob || range1->upb < range2->lob);
+}
+
+/*
+ * Check if @range1 contains @range2. If one of the ranges is empty,
+ * the result is always "false".
+ */
+static inline bool range_contains_range(const Range *range1,
+                                        const Range *range2)
+{
+    if (range_is_empty(range1) || range_is_empty(range2)) {
+        return false;
+    }
+    return range1->lob <= range2->lob && range1->upb >= range2->upb;
+}
+
 /*
  * Extend @range to the smallest interval that includes @extend_by, too.
  */
Signature · 1 line
2.17.2
author
#2 lore raw

PATCH v4 2/2 memory-device: rewrite address assignment using ranges

In reply to David Hildenbrand

Let's rewrite it properly using ranges. This fixes certain overflows that are right now possible. E.g.

qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \
    -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G
    -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000

Now properly errors out instead of succeeding. (Note that qapi parsing of huge uint64_t values is broken and fixes are on the way)

"can't add memory device [0xffffffffa0000000:0x80000000], usable range for memory devices [0x140000000:0xe00000000]"

Signed-off-by David Hildenbrand <[email protected]>
---
 hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 23 deletions(-)
hw/mem/memory-device.c +31 -23
index 8be63c8032..28e871f562 100644
@@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
-    uint64_t address_space_start, address_space_end;
     GSList *list = NULL, *item;
-    uint64_t new_addr = 0;
+    Range as, new = range_empty;
 
     if (!ms->device_memory) {
         error_setg(errp, "memory devices (e.g. for memory hotplug) are not "

@@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                          "enabled, please specify the maxmem option");
         return 0;
     }
-    address_space_start = ms->device_memory->base;
-    address_space_end = address_space_start +
-                        memory_region_size(&ms->device_memory->mr);
-    g_assert(address_space_end >= address_space_start);
+    range_init_nofail(&as, ms->device_memory->base,
+                      memory_region_size(&ms->device_memory->mr));
 
-    /* address_space_start indicates the maximum alignment we expect */
-    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
+    /* start of address space indicates the maximum alignment we expect */
+    if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;

@@ -145,20 +142,25 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     }
 
     if (hint) {
-        new_addr = *hint;
-        if (new_addr < address_space_start) {
+        if (range_init(&new, *hint, size)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] before 0x%" PRIx64, new_addr, size,
-                       address_space_start);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", *hint, size, range_lob(&as),
+                       range_size(&as));
             return 0;
-        } else if ((new_addr + size) > address_space_end) {
+        }
+        if (!range_contains_range(&as, &new)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] beyond 0x%" PRIx64, new_addr, size,
-                       address_space_end);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", range_lob(&new), range_size(&new),
+                       range_lob(&as), range_size(&as));
             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        if (range_init(&new, range_lob(&as), size)) {
+            error_setg(errp, "can't add memory device, device too big");
+            return 0;
+        }
     }
 
     /* find address range that will fit new memory device */

@@ -166,30 +168,36 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     for (item = list; item; item = g_slist_next(item)) {
         const MemoryDeviceState *md = item->data;
         const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
-        uint64_t md_size, md_addr;
+        uint64_t next_addr;
+        Range tmp;
 
-        md_addr = mdc->get_addr(md);
-        md_size = memory_device_get_region_size(md, &error_abort);
+        range_init_nofail(&tmp, mdc->get_addr(md),
+                          memory_device_get_region_size(md, &error_abort));
 
-        if (ranges_overlap(md_addr, md_size, new_addr, size)) {
+        if (range_overlaps_range(&tmp, &new)) {
             if (hint) {
                 const DeviceState *d = DEVICE(md);
                 error_setg(errp, "address range conflicts with memory device"
                            " id='%s'", d->id ? d->id : "(unnamed)");
                 goto out;
             }
-            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
+
+            next_addr = QEMU_ALIGN_UP(range_upb(&tmp) + 1, align);
+            if (!next_addr || range_init(&new, next_addr, range_size(&new))) {
+                range_make_empty(&new);
+                break;
+            }
         }
     }
 
-    if (new_addr + size > address_space_end) {
+    if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "
                    "memory device - memory fragmented due to alignments");
         goto out;
     }
 out:
     g_slist_free(list);
-    return new_addr;
+    return range_lob(&new);
 }
 
 MemoryDeviceInfoList *qmp_memory_device_list(void)
Signature · 1 line
2.17.2
#3 lore raw

Re: [Qemu-devel] [PATCH v4 2/2] memory-device: rewrite address assignment using ranges

Quoted 54 lines · On Wed, 12 Dec 2018 10:28:21 +0100 David Hildenbrand <[email protected]> wrote:

Let's rewrite it properly using ranges. This fixes certain overflows that are right now possible. E.g.

qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \
    -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G
    -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000

Now properly errors out instead of succeeding. (Note that qapi parsing of huge uint64_t values is broken and fixes are on the way)

"can't add memory device [0xffffffffa0000000:0x80000000], usable range for memory devices [0x140000000:0xe00000000]"

Signed-off-by David Hildenbrand <[email protected]>
---
 hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 23 deletions(-)
hw/mem/memory-device.c +5 -8
index 8be63c8032..28e871f562 100644
@@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
-    uint64_t address_space_start, address_space_end;
     GSList *list = NULL, *item;
-    uint64_t new_addr = 0;
+    Range as, new = range_empty;
 
     if (!ms->device_memory) {
         error_setg(errp, "memory devices (e.g. for memory hotplug) are not "

@@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                          "enabled, please specify the maxmem option");
         return 0;
     }
-    address_space_start = ms->device_memory->base;
-    address_space_end = address_space_start +
-                        memory_region_size(&ms->device_memory->mr);
-    g_assert(address_space_end >= address_space_start);
+    range_init_nofail(&as, ms->device_memory->base,
+                      memory_region_size(&ms->device_memory->mr));
 
-    /* address_space_start indicates the maximum alignment we expect */
-    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
+    /* start of address space indicates the maximum alignment we expect */
+    if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;

@@ -145,20 +142,25 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     }

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 149

Quoted 10 lines
hw/mem/memory-device.c +4 -4
     if (hint) {
-        new_addr = *hint;
-        if (new_addr < address_space_start) {
+        if (range_init(&new, *hint, size)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] before 0x%" PRIx64, new_addr, size,
-                       address_space_start);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", *hint, size, range_lob(&as),
+                       range_size(&as));

this changes error message to be the same as the next one and looses 'before' meaning so if you'd like to have the same error message, then prbably merging both branches would be better.

Quoted 23 lines
hw/mem/memory-device.c +9 -4
             return 0;
-        } else if ((new_addr + size) > address_space_end) {
+        }
+        if (!range_contains_range(&as, &new)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] beyond 0x%" PRIx64, new_addr, size,
-                       address_space_end);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", range_lob(&new), range_size(&new),
+                       range_lob(&as), range_size(&as));
             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        if (range_init(&new, range_lob(&as), size)) {
+            error_setg(errp, "can't add memory device, device too big");
+            return 0;
+        }
     }
 
     /* find address range that will fit new memory device */

@@ -166,30 +168,36 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     for (item = list; item; item = g_slist_next(item)) {

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 175

Quoted 10 lines
hw/mem/memory-device.c +4 -3
         const MemoryDeviceState *md = item->data;
         const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
-        uint64_t md_size, md_addr;
+        uint64_t next_addr;
+        Range tmp;
 
-        md_addr = mdc->get_addr(md);
-        md_size = memory_device_get_region_size(md, &error_abort);
+        range_init_nofail(&tmp, mdc->get_addr(md),
+                          memory_device_get_region_size(md, &error_abort));

is it possible to make QEMU abort here during hotplug here? (range_init_nofail)

Quoted 13 lines
hw/mem/memory-device.c +4 -2
-        if (ranges_overlap(md_addr, md_size, new_addr, size)) {
+        if (range_overlaps_range(&tmp, &new)) {
             if (hint) {
                 const DeviceState *d = DEVICE(md);
                 error_setg(errp, "address range conflicts with memory device"
                            " id='%s'", d->id ? d->id : "(unnamed)");
                 goto out;
             }
-            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
+
+            next_addr = QEMU_ALIGN_UP(range_upb(&tmp) + 1, align);
+            if (!next_addr || range_init(&new, next_addr, range_size(&new))) {

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 195

Quoted 10 lines
hw/mem/memory-device.c +4 -1
+                range_make_empty(&new);
+                break;
+            }
         }
     }
 
-    if (new_addr + size > address_space_end) {
+    if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "
                    "memory device - memory fragmented due to alignments");

it could happen due to fragmentation but also in case remaining free space is no enough

Quoted 9 lines
hw/mem/memory-device.c +1 -1
         goto out;
     }
 out:
     g_slist_free(list);
-    return new_addr;
+    return range_lob(&new);
 }
 
 MemoryDeviceInfoList *qmp_memory_device_list(void)
author
#4 lore raw

Re: [Qemu-devel] [PATCH v4 2/2] memory-device: rewrite address assignment using ranges

Quoted 57 lines · Igor Mammedov
Quoted 54 lines · On Wed, 12 Dec 2018 10:28:21 +0100 David Hildenbrand <[email protected]> wrote:

Let's rewrite it properly using ranges. This fixes certain overflows that are right now possible. E.g.

qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \
    -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G
    -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000

Now properly errors out instead of succeeding. (Note that qapi parsing of huge uint64_t values is broken and fixes are on the way)

"can't add memory device [0xffffffffa0000000:0x80000000], usable range for memory devices [0x140000000:0xe00000000]"

Signed-off-by David Hildenbrand <[email protected]>
---
 hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 23 deletions(-)
hw/mem/memory-device.c +5 -8
index 8be63c8032..28e871f562 100644
@@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
-    uint64_t address_space_start, address_space_end;
     GSList *list = NULL, *item;
-    uint64_t new_addr = 0;
+    Range as, new = range_empty;
 
     if (!ms->device_memory) {
         error_setg(errp, "memory devices (e.g. for memory hotplug) are not "

@@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                          "enabled, please specify the maxmem option");
         return 0;
     }
-    address_space_start = ms->device_memory->base;
-    address_space_end = address_space_start +
-                        memory_region_size(&ms->device_memory->mr);
-    g_assert(address_space_end >= address_space_start);
+    range_init_nofail(&as, ms->device_memory->base,
+                      memory_region_size(&ms->device_memory->mr));
 
-    /* address_space_start indicates the maximum alignment we expect */
-    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
+    /* start of address space indicates the maximum alignment we expect */
+    if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;

@@ -145,20 +142,25 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     }
Quoted 12 lines

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 149

Quoted 10 lines
hw/mem/memory-device.c +4 -4
     if (hint) {
-        new_addr = *hint;
-        if (new_addr < address_space_start) {
+        if (range_init(&new, *hint, size)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] before 0x%" PRIx64, new_addr, size,
-                       address_space_start);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", *hint, size, range_lob(&as),
+                       range_size(&as));

this changes error message to be the same as the next one and looses 'before' meaning so if you'd like to have the same error message, then prbably merging both branches would be better.

I can do that, but I'll have to refer to "*hint" and "size" then instead of range_lob(&new) and range_size(&new), because the range might not be initialized.

Quoted 30 lines
Quoted 30 lines
hw/mem/memory-device.c +11 -5
             return 0;
-        } else if ((new_addr + size) > address_space_end) {
+        }
+        if (!range_contains_range(&as, &new)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] beyond 0x%" PRIx64, new_addr, size,
-                       address_space_end);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", range_lob(&new), range_size(&new),
+                       range_lob(&as), range_size(&as));
             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        if (range_init(&new, range_lob(&as), size)) {
+            error_setg(errp, "can't add memory device, device too big");
+            return 0;
+        }
     }
 
     /* find address range that will fit new memory device */

@@ -166,30 +168,36 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     for (item = list; item; item = g_slist_next(item)) {
         const MemoryDeviceState *md = item->data;
         const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
-        uint64_t md_size, md_addr;
+        uint64_t next_addr;
+        Range tmp;
Quoted 5 lines

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 175

Quoted 4 lines
hw/mem/memory-device.c +2 -2
-        md_addr = mdc->get_addr(md);
-        md_size = memory_device_get_region_size(md, &error_abort);
+        range_init_nofail(&tmp, mdc->get_addr(md),
+                          memory_device_get_region_size(md, &error_abort));

is it possible to make QEMU abort here during hotplug here? (range_init_nofail)

No, as we've handled (and effectively assigned the addresses of) these devices when they were effectively added/cold/hotplugged. We only have to be careful with the new device we're adding.

Quoted 20 lines
Quoted 20 lines
hw/mem/memory-device.c +7 -2
-        if (ranges_overlap(md_addr, md_size, new_addr, size)) {
+        if (range_overlaps_range(&tmp, &new)) {
             if (hint) {
                 const DeviceState *d = DEVICE(md);
                 error_setg(errp, "address range conflicts with memory device"
                            " id='%s'", d->id ? d->id : "(unnamed)");
                 goto out;
             }
-            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
+
+            next_addr = QEMU_ALIGN_UP(range_upb(&tmp) + 1, align);
+            if (!next_addr || range_init(&new, next_addr, range_size(&new))) {
+                range_make_empty(&new);
+                break;
+            }
         }
     }
Quoted 5 lines

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 195

Quoted 4 lines
hw/mem/memory-device.c +1 -1
-    if (new_addr + size > address_space_end) {
+    if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "
                    "memory device - memory fragmented due to alignments");

it could happen due to fragmentation but also in case remaining free space is no enough

That should be handled via memory_device_check_addable(), which is called at the beginning of the function. It checks for general size availability.

Thanks!

Signature · 3 lines
Thanks,

David / dhildenb
#5 lore raw

Re: [Qemu-devel] [PATCH v4 2/2] memory-device: rewrite address assignment using ranges

Quoted 71 lines · On Thu, 13 Dec 2018 13:35:28 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 57 lines · Igor Mammedov
Quoted 54 lines · On Wed, 12 Dec 2018 10:28:21 +0100 David Hildenbrand <[email protected]> wrote:

Let's rewrite it properly using ranges. This fixes certain overflows that are right now possible. E.g.

qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \
    -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G
    -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000

Now properly errors out instead of succeeding. (Note that qapi parsing of huge uint64_t values is broken and fixes are on the way)

"can't add memory device [0xffffffffa0000000:0x80000000], usable range for memory devices [0x140000000:0xe00000000]"

Signed-off-by David Hildenbrand <[email protected]>
---
 hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 23 deletions(-)
hw/mem/memory-device.c +5 -8
index 8be63c8032..28e871f562 100644
@@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
-    uint64_t address_space_start, address_space_end;
     GSList *list = NULL, *item;
-    uint64_t new_addr = 0;
+    Range as, new = range_empty;
 
     if (!ms->device_memory) {
         error_setg(errp, "memory devices (e.g. for memory hotplug) are not "

@@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                          "enabled, please specify the maxmem option");
         return 0;
     }
-    address_space_start = ms->device_memory->base;
-    address_space_end = address_space_start +
-                        memory_region_size(&ms->device_memory->mr);
-    g_assert(address_space_end >= address_space_start);
+    range_init_nofail(&as, ms->device_memory->base,
+                      memory_region_size(&ms->device_memory->mr));
 
-    /* address_space_start indicates the maximum alignment we expect */
-    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
+    /* start of address space indicates the maximum alignment we expect */
+    if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;

@@ -145,20 +142,25 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     }
Quoted 12 lines

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 149

Quoted 10 lines
hw/mem/memory-device.c +4 -4
     if (hint) {
-        new_addr = *hint;
-        if (new_addr < address_space_start) {
+        if (range_init(&new, *hint, size)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] before 0x%" PRIx64, new_addr, size,
-                       address_space_start);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", *hint, size, range_lob(&as),
+                       range_size(&as));

this changes error message to be the same as the next one and looses 'before' meaning so if you'd like to have the same error message, then prbably merging both branches would be better.

Quoted 3 lines

I can do that, but I'll have to refer to "*hint" and "size" then instead of range_lob(&new) and range_size(&new), because the range might not be initialized.

either that or better make errors different to avoid confusion.

[...]

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
hw/mem/memory-device.c +7 -2
-            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
+
+            next_addr = QEMU_ALIGN_UP(range_upb(&tmp) + 1, align);
+            if (!next_addr || range_init(&new, next_addr, range_size(&new))) {
+                range_make_empty(&new);
+                break;
+            }
         }
     }
 
-    if (new_addr + size > address_space_end) {
+    if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "
                    "memory device - memory fragmented due to alignments");
Quoted 5 lines
Quoted 1 line

it could happen due to fragmentation but also in case remaining free space is no enough

That should be handled via memory_device_check_addable(), which is called at the beginning of the function. It checks for general size availability.

I've meant
AS_LOB AS_UPB
100 1000
MEM1_LOB MEM1_UPB
100 900
then hotplugging MEM2 with size 200 would fail with this message, which could be a bit confusing.
Maybe "not enough space to plug device of size %d" would be better?

Quoted 3 lines

Thanks!

author
#6 lore raw

Re: [Qemu-devel] [PATCH v4 2/2] memory-device: rewrite address assignment using ranges

Quoted 74 lines · Igor Mammedov
Quoted 71 lines · On Thu, 13 Dec 2018 13:35:28 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 69 lines · Igor Mammedov
Quoted 54 lines · On Wed, 12 Dec 2018 10:28:21 +0100 David Hildenbrand <[email protected]> wrote:

Let's rewrite it properly using ranges. This fixes certain overflows that are right now possible. E.g.

qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \
    -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G
    -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000

Now properly errors out instead of succeeding. (Note that qapi parsing of huge uint64_t values is broken and fixes are on the way)

"can't add memory device [0xffffffffa0000000:0x80000000], usable range for memory devices [0x140000000:0xe00000000]"

Signed-off-by David Hildenbrand <[email protected]>
---
 hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 23 deletions(-)
hw/mem/memory-device.c +5 -8
index 8be63c8032..28e871f562 100644
@@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
-    uint64_t address_space_start, address_space_end;
     GSList *list = NULL, *item;
-    uint64_t new_addr = 0;
+    Range as, new = range_empty;
 
     if (!ms->device_memory) {
         error_setg(errp, "memory devices (e.g. for memory hotplug) are not "

@@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                          "enabled, please specify the maxmem option");
         return 0;
     }
-    address_space_start = ms->device_memory->base;
-    address_space_end = address_space_start +
-                        memory_region_size(&ms->device_memory->mr);
-    g_assert(address_space_end >= address_space_start);
+    range_init_nofail(&as, ms->device_memory->base,
+                      memory_region_size(&ms->device_memory->mr));
 
-    /* address_space_start indicates the maximum alignment we expect */
-    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
+    /* start of address space indicates the maximum alignment we expect */
+    if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;

@@ -145,20 +142,25 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     }

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 149

Quoted 10 lines
hw/mem/memory-device.c +4 -4
     if (hint) {
-        new_addr = *hint;
-        if (new_addr < address_space_start) {
+        if (range_init(&new, *hint, size)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] before 0x%" PRIx64, new_addr, size,
-                       address_space_start);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", *hint, size, range_lob(&as),
+                       range_size(&as));

this changes error message to be the same as the next one and looses 'before' meaning so if you'd like to have the same error message, then prbably merging both branches would be better.

Quoted 5 lines
Quoted 3 lines

I can do that, but I'll have to refer to "*hint" and "size" then instead of range_lob(&new) and range_size(&new), because the range might not be initialized.

either that or better make errors different to avoid confusion.

Will see what turns out better. As we indicate the ranges the user can figure out what is going wrong.

Quoted 21 lines

[...]

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
hw/mem/memory-device.c +7 -2
-            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
+
+            next_addr = QEMU_ALIGN_UP(range_upb(&tmp) + 1, align);
+            if (!next_addr || range_init(&new, next_addr, range_size(&new))) {
+                range_make_empty(&new);
+                break;
+            }
         }
     }
 
-    if (new_addr + size > address_space_end) {
+    if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "
                    "memory device - memory fragmented due to alignments");
Quoted 5 lines
Quoted 1 line

it could happen due to fragmentation but also in case remaining free space is no enough

That should be handled via memory_device_check_addable(), which is called at the beginning of the function. It checks for general size availability.

Quoted 8 lines

I've meant
AS_LOB AS_UPB
100 1000
MEM1_LOB MEM1_UPB
100 900
then hotplugging MEM2 with size 200 would fail with this message, which could be a bit confusing.
Maybe "not enough space to plug device of size %d" would be better?

That should be covered by memory_device_check_addable() if I am not wrong.

used_region_size + size > ms->maxram_size - ms->ram_size

For your example (if I don't mess up the numbers):

ms->maxram_size - ms->ram_size = 900
used_region_size = 800

So trying to add anything > 100 will bail out.

Quoted 5 lines
Quoted 4 lines

Thanks!

Signature · 3 lines
Thanks,

David / dhildenb
#7 lore raw

Re: [Qemu-devel] [PATCH v4 2/2] memory-device: rewrite address assignment using ranges

Quoted 75 lines · On Thu, 13 Dec 2018 15:54:47 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 74 lines · Igor Mammedov
Quoted 71 lines · On Thu, 13 Dec 2018 13:35:28 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 69 lines · Igor Mammedov
Quoted 54 lines · On Wed, 12 Dec 2018 10:28:21 +0100 David Hildenbrand <[email protected]> wrote:

Let's rewrite it properly using ranges. This fixes certain overflows that are right now possible. E.g.

qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \
    -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G
    -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000

Now properly errors out instead of succeeding. (Note that qapi parsing of huge uint64_t values is broken and fixes are on the way)

"can't add memory device [0xffffffffa0000000:0x80000000], usable range for memory devices [0x140000000:0xe00000000]"

Signed-off-by David Hildenbrand <[email protected]>
---
 hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 23 deletions(-)
hw/mem/memory-device.c +5 -8
index 8be63c8032..28e871f562 100644
@@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
-    uint64_t address_space_start, address_space_end;
     GSList *list = NULL, *item;
-    uint64_t new_addr = 0;
+    Range as, new = range_empty;
 
     if (!ms->device_memory) {
         error_setg(errp, "memory devices (e.g. for memory hotplug) are not "

@@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                          "enabled, please specify the maxmem option");
         return 0;
     }
-    address_space_start = ms->device_memory->base;
-    address_space_end = address_space_start +
-                        memory_region_size(&ms->device_memory->mr);
-    g_assert(address_space_end >= address_space_start);
+    range_init_nofail(&as, ms->device_memory->base,
+                      memory_region_size(&ms->device_memory->mr));
 
-    /* address_space_start indicates the maximum alignment we expect */
-    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
+    /* start of address space indicates the maximum alignment we expect */
+    if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;

@@ -145,20 +142,25 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     }

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 149

Quoted 10 lines
hw/mem/memory-device.c +4 -4
     if (hint) {
-        new_addr = *hint;
-        if (new_addr < address_space_start) {
+        if (range_init(&new, *hint, size)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] before 0x%" PRIx64, new_addr, size,
-                       address_space_start);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", *hint, size, range_lob(&as),
+                       range_size(&as));

this changes error message to be the same as the next one and looses 'before' meaning so if you'd like to have the same error message, then prbably merging both branches would be better.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

I can do that, but I'll have to refer to "*hint" and "size" then instead of range_lob(&new) and range_size(&new), because the range might not be initialized.

either that or better make errors different to avoid confusion.

Will see what turns out better. As we indicate the ranges the user can figure out what is going wrong.

ok

Quoted 34 lines
Quoted 22 lines

[...]

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
hw/mem/memory-device.c +7 -2
-            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
+
+            next_addr = QEMU_ALIGN_UP(range_upb(&tmp) + 1, align);
+            if (!next_addr || range_init(&new, next_addr, range_size(&new))) {
+                range_make_empty(&new);
+                break;
+            }
         }
     }
 
-    if (new_addr + size > address_space_end) {
+    if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "
                    "memory device - memory fragmented due to alignments");
Quoted 5 lines
Quoted 1 line

it could happen due to fragmentation but also in case remaining free space is no enough

That should be handled via memory_device_check_addable(), which is called at the beginning of the function. It checks for general size availability.

Quoted 8 lines

I've meant
AS_LOB AS_UPB
100 1000
MEM1_LOB MEM1_UPB
100 900
then hotplugging MEM2 with size 200 would fail with this message, which could be a bit confusing.
Maybe "not enough space to plug device of size %d" would be better?

That should be covered by memory_device_check_addable() if I am not wrong.

Quoted 8 lines

used_region_size + size > ms->maxram_size - ms->ram_size

For your example (if I don't mess up the numbers):

ms->maxram_size - ms->ram_size = 900
used_region_size = 800

So trying to add anything > 100 will bail out.

Thanks, I see it now.

Quoted 9 lines
Quoted 7 lines
Quoted 6 lines

Thanks!