[Qemu-devel] [PATCH v4 0/2] qapi/range/memory-device: fixes and cleanups

[Qemu-devel] [PATCH v4 0/2] qapi/range/memory-device: fixes and cleanups

[David Hildenbrand]

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(-)

-- 
2.17.2

[Qemu-devel] [PATCH v4 1/2] range: add some more functions

[David Hildenbrand]

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 <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qemu/range.h | 62 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/include/qemu/range.h b/include/qemu/range.h
index 7e75f4e655..ba606c6bc0 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -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.
  */
-- 
2.17.2

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

[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 <david@redhat.com>
---
 hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 8be63c8032..28e871f562 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -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)
-- 
2.17.2

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

[Igor Mammedov]

On Wed, 12 Dec 2018 10:28:21 +0100
David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
> ---
>  hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
>  1 file changed, 31 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 8be63c8032..28e871f562 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -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));
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.

>              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));
is it possible to make QEMU abort here during hotplug here? (range_init_nofail)

>  
> -        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");
it could happen due to fragmentation but also in case remaining free space is no enough

>          goto out;
>      }
>  out:
>      g_slist_free(list);
> -    return new_addr;
> +    return range_lob(&new);
>  }
>  
>  MemoryDeviceInfoList *qmp_memory_device_list(void)

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

[David Hildenbrand]

On 13.12.18 13:28, Igor Mammedov wrote:
> On Wed, 12 Dec 2018 10:28:21 +0100
> David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
>> ---
>>  hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
>>  1 file changed, 31 insertions(+), 23 deletions(-)
>>
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index 8be63c8032..28e871f562 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -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));
> 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.

> 
>>              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));
> 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.

> 
>>  
>> -        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");
> 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!

-- 

Thanks,

David / dhildenb

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

[Igor Mammedov]

On Thu, 13 Dec 2018 13:35:28 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 13.12.18 13:28, Igor Mammedov wrote:
> > On Wed, 12 Dec 2018 10:28:21 +0100
> > David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
> >> ---
> >>  hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
> >>  1 file changed, 31 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> >> index 8be63c8032..28e871f562 100644
> >> --- a/hw/mem/memory-device.c
> >> +++ b/hw/mem/memory-device.c
> >> @@ -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));  
> > 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.
either that or better make errors different to avoid confusion.

[...]
> >> -            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");  
> > 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?
   
> 
> Thanks!
> 

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

[David Hildenbrand]

On 13.12.18 15:48, Igor Mammedov wrote:
> On Thu, 13 Dec 2018 13:35:28 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 13.12.18 13:28, Igor Mammedov wrote:
>>> On Wed, 12 Dec 2018 10:28:21 +0100
>>> David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
>>>> ---
>>>>  hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
>>>>  1 file changed, 31 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>>>> index 8be63c8032..28e871f562 100644
>>>> --- a/hw/mem/memory-device.c
>>>> +++ b/hw/mem/memory-device.c
>>>> @@ -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));  
>>> 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.
> 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.

> [...]
>>>> -            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");  
>>> 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?


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.


>    
>>
>> Thanks!
>>
> 


-- 

Thanks,

David / dhildenb

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

[Igor Mammedov]

On Thu, 13 Dec 2018 15:54:47 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 13.12.18 15:48, Igor Mammedov wrote:
> > On Thu, 13 Dec 2018 13:35:28 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 13.12.18 13:28, Igor Mammedov wrote:  
> >>> On Wed, 12 Dec 2018 10:28:21 +0100
> >>> David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
> >>>> ---
> >>>>  hw/mem/memory-device.c | 54 ++++++++++++++++++++++++------------------
> >>>>  1 file changed, 31 insertions(+), 23 deletions(-)
> >>>>
> >>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> >>>> index 8be63c8032..28e871f562 100644
> >>>> --- a/hw/mem/memory-device.c
> >>>> +++ b/hw/mem/memory-device.c
> >>>> @@ -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));    
> >>> 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.  
> > 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

> 
> > [...]  
> >>>> -            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");    
> >>> 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?  
> 
> 
> 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.
Thanks, I see it now.

> 
> 
> >      
> >>
> >> Thanks!
> >>  
> >   
> 
>