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

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

[David Hildenbrand]

While working on memory device code, I noticed that specifiying an uint64_t
on command line does not work in all cases as we always parse an int64_t.
So I fix that and also cleanup the old int64_t parser.

To be able to fix some overflows in memory-device code in a clean way,
I am reusing the range implementation of qemu, for which I need some
more helpers.

This series is based on
    "[PATCH v5 00/16] memory-device: complete refactoring"
which should get merged soon.

David Hildenbrand (7):
  qapi: correctly parse uint64_t values from strings
  qapi: use qemu_strtoi64() in parse_str_int64
  range: pass const pointer where possible
  range: add some more functions
  memory-device: use QEMU_IS_ALIGNED
  memory-device: avoid overflows on very huge devices
  memory-device: rewrite address assignment using ranges

 hw/mem/memory-device.c      |  63 ++++++++++-------
 include/qemu/range.h        |  86 ++++++++++++++++++++++-
 qapi/string-input-visitor.c | 133 ++++++++++++++++++++++++++++++------
 3 files changed, 231 insertions(+), 51 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v1 1/7] qapi: correctly parse uint64_t values from strings

[David Hildenbrand]

Right now, we parse uint64_t values just like int64_t values, resulting
in negative values getting accepted and certain valid large numbers only
being representable as negative numbers. Also, reported errors indicate
that an int64_t is expected.

Parse uin64_t separately. Implementation inspired by original
parse_str() implementation.

E.g. we can now specify
    -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
Instead of going via negative values
    -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000

Resulting in the same values

(qemu) info memory-devices
Memory device [nvdimm]: "nv1"
  addr: 0xffffffffc0000000
  slot: 0
  node: 0

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 qapi/string-input-visitor.c | 117 ++++++++++++++++++++++++++++++++----
 1 file changed, 106 insertions(+), 11 deletions(-)

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index b3fdd0827d..af0a841152 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -19,6 +19,7 @@
 #include "qapi/qmp/qnull.h"
 #include "qemu/option.h"
 #include "qemu/queue.h"
+#include "qemu/cutils.h"
 #include "qemu/range.h"
 
 
@@ -44,7 +45,8 @@ static void free_range(void *range, void *dummy)
     g_free(range);
 }
 
-static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
+static int parse_str_int64(StringInputVisitor *siv, const char *name,
+                           Error **errp)
 {
     char *str = (char *) siv->string;
     long long start, end;
@@ -118,6 +120,75 @@ error:
     return -1;
 }
 
+static int parse_str_uint64(StringInputVisitor *siv, const char *name,
+                            Error **errp)
+{
+    const char *str = (char *) siv->string;
+    uint64_t start, end;
+    const char *endptr;
+    Range *cur;
+
+    if (siv->ranges) {
+        return 0;
+    }
+
+    if (!*str) {
+        return 0;
+    }
+
+    do {
+        if (!qemu_strtou64(str, &endptr, 0, &start)) {
+            if (*endptr == '\0') {
+                cur = g_malloc0(sizeof(*cur));
+                range_set_bounds(cur, start, start);
+                siv->ranges = range_list_insert(siv->ranges, cur);
+                cur = NULL;
+                str = NULL;
+            } else if (*endptr == '-') {
+                str = endptr + 1;
+                if (!qemu_strtou64(str, &endptr, 0, &end) && start <= end) {
+                    if (*endptr == '\0') {
+                        cur = g_malloc0(sizeof(*cur));
+                        range_set_bounds(cur, start, end);
+                        siv->ranges = range_list_insert(siv->ranges, cur);
+                        cur = NULL;
+                        str = NULL;
+                    } else if (*endptr == ',') {
+                        str = endptr + 1;
+                        cur = g_malloc0(sizeof(*cur));
+                        range_set_bounds(cur, start, end);
+                        siv->ranges = range_list_insert(siv->ranges, cur);
+                        cur = NULL;
+                    } else {
+                        goto error;
+                    }
+                } else {
+                    goto error;
+                }
+            } else if (*endptr == ',') {
+                str = endptr + 1;
+                cur = g_malloc0(sizeof(*cur));
+                range_set_bounds(cur, start, start);
+                siv->ranges = range_list_insert(siv->ranges, cur);
+                cur = NULL;
+            } else {
+                goto error;
+            }
+        } else {
+            goto error;
+        }
+    } while (str);
+
+    return 0;
+error:
+    g_list_foreach(siv->ranges, free_range, NULL);
+    g_list_free(siv->ranges);
+    siv->ranges = NULL;
+    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+               "an uint64 value or range");
+    return -1;
+}
+
 static void
 start_list(Visitor *v, const char *name, GenericList **list, size_t size,
            Error **errp)
@@ -128,7 +199,7 @@ start_list(Visitor *v, const char *name, GenericList **list, size_t size,
     assert(list);
     siv->list = list;
 
-    if (parse_str(siv, name, errp) < 0) {
+    if (parse_str_int64(siv, name, errp) < 0) {
         *list = NULL;
         return;
     }
@@ -216,7 +287,7 @@ static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
 {
     StringInputVisitor *siv = to_siv(v);
 
-    if (parse_str(siv, name, errp) < 0) {
+    if (parse_str_int64(siv, name, errp) < 0) {
         return;
     }
 
@@ -252,15 +323,39 @@ error:
 static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
                               Error **errp)
 {
-    /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
-    int64_t i;
-    Error *err = NULL;
-    parse_type_int64(v, name, &i, &err);
-    if (err) {
-        error_propagate(errp, err);
-    } else {
-        *obj = i;
+    StringInputVisitor *siv = to_siv(v);
+
+    if (parse_str_uint64(siv, name, errp) < 0) {
+        return;
+    }
+
+    if (!siv->ranges) {
+        goto error;
+    }
+
+    if (!siv->cur_range) {
+        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 = range_lob(r);
     }
+
+    *obj = siv->cur;
+    siv->cur++;
+    return;
+
+error:
+    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+               "an uint64 value or range");
 }
 
 static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
-- 
2.17.1

[Qemu-devel] [PATCH v1 2/7] qapi: use qemu_strtoi64() in parse_str_int64

[David Hildenbrand]

The qemu api claims to be easier to use, and the resulting code
shows that.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 qapi/string-input-visitor.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index af0a841152..ee38ab1d10 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -48,10 +48,10 @@ static void free_range(void *range, void *dummy)
 static int parse_str_int64(StringInputVisitor *siv, const char *name,
                            Error **errp)
 {
-    char *str = (char *) siv->string;
-    long long start, end;
+    const char *str = (char *) siv->string;
+    const char *endptr;
+    int64_t start, end;
     Range *cur;
-    char *endptr;
 
     if (siv->ranges) {
         return 0;
@@ -62,9 +62,7 @@ static int parse_str_int64(StringInputVisitor *siv, const char *name,
     }
 
     do {
-        errno = 0;
-        start = strtoll(str, &endptr, 0);
-        if (errno == 0 && endptr > str) {
+        if (!qemu_strtoi64(str, &endptr, 0, &start)) {
             if (*endptr == '\0') {
                 cur = g_malloc0(sizeof(*cur));
                 range_set_bounds(cur, start, start);
@@ -73,11 +71,7 @@ static int parse_str_int64(StringInputVisitor *siv, const char *name,
                 str = NULL;
             } else if (*endptr == '-') {
                 str = endptr + 1;
-                errno = 0;
-                end = strtoll(str, &endptr, 0);
-                if (errno == 0 && endptr > str && start <= end &&
-                    (start > INT64_MAX - 65536 ||
-                     end < start + 65536)) {
+                if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
                     if (*endptr == '\0') {
                         cur = g_malloc0(sizeof(*cur));
                         range_set_bounds(cur, start, end);
-- 
2.17.1

[Qemu-devel] [PATCH v1 3/7] range: pass const pointer where possible

[David Hildenbrand]

If there are no changes, let's use a const pointer.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qemu/range.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/qemu/range.h b/include/qemu/range.h
index f28f0c1825..7e75f4e655 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -39,7 +39,7 @@ struct Range {
     uint64_t upb;        /* inclusive upper bound */
 };
 
-static inline void range_invariant(Range *range)
+static inline void range_invariant(const Range *range)
 {
     assert(range->lob <= range->upb || range->lob == range->upb + 1);
 }
@@ -48,14 +48,14 @@ static inline void range_invariant(Range *range)
 #define range_empty ((Range){ .lob = 1, .upb = 0 })
 
 /* Is @range empty? */
-static inline bool range_is_empty(Range *range)
+static inline bool range_is_empty(const Range *range)
 {
     range_invariant(range);
     return range->lob > range->upb;
 }
 
 /* Does @range contain @val? */
-static inline bool range_contains(Range *range, uint64_t val)
+static inline bool range_contains(const Range *range, uint64_t val)
 {
     return val >= range->lob && val <= range->upb;
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 3/7] range: pass const pointer where possible

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> If there are no changes, let's use a const pointer.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  include/qemu/range.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qemu/range.h b/include/qemu/range.h
> index f28f0c1825..7e75f4e655 100644
> --- a/include/qemu/range.h
> +++ b/include/qemu/range.h
> @@ -39,7 +39,7 @@ struct Range {
>      uint64_t upb;        /* inclusive upper bound */
>  };
>  
> -static inline void range_invariant(Range *range)
> +static inline void range_invariant(const Range *range)
>  {
>      assert(range->lob <= range->upb || range->lob == range->upb + 1);
>  }
> @@ -48,14 +48,14 @@ static inline void range_invariant(Range *range)
>  #define range_empty ((Range){ .lob = 1, .upb = 0 })
>  
>  /* Is @range empty? */
> -static inline bool range_is_empty(Range *range)
> +static inline bool range_is_empty(const Range *range)
>  {
>      range_invariant(range);
>      return range->lob > range->upb;
>  }
>  
>  /* Does @range contain @val? */
> -static inline bool range_contains(Range *range, uint64_t val)
> +static inline bool range_contains(const Range *range, uint64_t val)
>  {
>      return val >= range->lob && val <= range->upb;
>  }
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v1 4/7] 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
range_valid(): Check if there would be an overflow when initializin
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
range_starts_before_range(): Check if one range starts before another
range_ends_after_range(): Check if one range ends after another

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

diff --git a/include/qemu/range.h b/include/qemu/range.h
index 7e75f4e655..18e8acf22f 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -112,6 +112,86 @@ 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.
+ */
+static inline void range_init(Range *range, uint64_t lob, uint64_t size)
+{
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+}
+
+/*
+ * Check if the interval [@lob,@lob + @size - 1] would be valid or not
+ * (result in an overflow).
+ */
+static inline bool range_valid(uint64_t lob, uint64_t size)
+{
+    return lob + size >= lob;
+}
+
+/*
+ * 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;
+}
+
+/*
+ * Check if @range1 starts before @range2. If one of the ranges is empty,
+ * the result is alsways "false".
+ */
+static inline bool range_starts_before_range(const Range *range1,
+                                             const Range *range2)
+{
+    if (range_is_empty(range1) || range_is_empty(range2)) {
+        return false;
+    }
+    return range1->lob < range2->lob;
+}
+
+/*
+ * Check if @range1 ends after @range2. If one of the ranges is empty,
+ * the result is alsways "false".
+ */
+static inline bool range_ends_after_range(const Range *range1,
+                                          const Range *range2)
+{
+    if (range_is_empty(range1) || range_is_empty(range2)) {
+        return false;
+    }
+    return range1->upb > range2->upb;
+}
+
 /*
  * Extend @range to the smallest interval that includes @extend_by, too.
  */
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> Add some more functions that will be used in memory-device context.
> 
> range_init(): Init using lower bound and size
> range_valid(): Check if there would be an overflow when initializin
> 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
> range_starts_before_range(): Check if one range starts before another
> range_ends_after_range(): Check if one range ends after another
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/include/qemu/range.h b/include/qemu/range.h
> index 7e75f4e655..18e8acf22f 100644
> --- a/include/qemu/range.h
> +++ b/include/qemu/range.h
> @@ -112,6 +112,86 @@ 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.
> + */
> +static inline void range_init(Range *range, uint64_t lob, uint64_t size)
> +{
> +    range->lob = lob;
> +    range->upb = lob + size - 1;
> +    range_invariant(range);
> +}
> +
> +/*
> + * Check if the interval [@lob,@lob + @size - 1] would be valid or not
> + * (result in an overflow).
> + */
> +static inline bool range_valid(uint64_t lob, uint64_t size)
> +{
> +    return lob + size >= lob;
> +}

That name confused me, I'd expected that to have taken a range and check
it for something (like a non-asserting version of the invariant).

> +/*
> + * 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;
> +}
> +
> +/*
> + * Check if @range1 starts before @range2. If one of the ranges is empty,
> + * the result is alsways "false".
> + */
> +static inline bool range_starts_before_range(const Range *range1,
> +                                             const Range *range2)
> +{
> +    if (range_is_empty(range1) || range_is_empty(range2)) {
> +        return false;
> +    }
> +    return range1->lob < range2->lob;
> +}
> +
> +/*
> + * Check if @range1 ends after @range2. If one of the ranges is empty,
> + * the result is alsways "false".
> + */
> +static inline bool range_ends_after_range(const Range *range1,
> +                                          const Range *range2)
> +{
> +    if (range_is_empty(range1) || range_is_empty(range2)) {
> +        return false;
> +    }
> +    return range1->upb > range2->upb;
> +}
> +
>  /*
>   * Extend @range to the smallest interval that includes @extend_by, too.
>   */
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions

[David Hildenbrand]

On 11/10/2018 11:08, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> Add some more functions that will be used in memory-device context.
>>
>> range_init(): Init using lower bound and size
>> range_valid(): Check if there would be an overflow when initializin
>> 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
>> range_starts_before_range(): Check if one range starts before another
>> range_ends_after_range(): Check if one range ends after another
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 80 insertions(+)
>>
>> diff --git a/include/qemu/range.h b/include/qemu/range.h
>> index 7e75f4e655..18e8acf22f 100644
>> --- a/include/qemu/range.h
>> +++ b/include/qemu/range.h
>> @@ -112,6 +112,86 @@ 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.
>> + */
>> +static inline void range_init(Range *range, uint64_t lob, uint64_t size)
>> +{
>> +    range->lob = lob;
>> +    range->upb = lob + size - 1;
>> +    range_invariant(range);
>> +}
>> +
>> +/*
>> + * Check if the interval [@lob,@lob + @size - 1] would be valid or not
>> + * (result in an overflow).
>> + */
>> +static inline bool range_valid(uint64_t lob, uint64_t size)
>> +{
>> +    return lob + size >= lob;
>> +}
> 
> That name confused me, I'd expected that to have taken a range and check
> it for something (like a non-asserting version of the invariant).

Then we have to remove all the variant asserts from the initializer
functions (well, because then it is no longer an invariant then). Other
ideas?

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> On 11/10/2018 11:08, Dr. David Alan Gilbert wrote:
> > * David Hildenbrand (david@redhat.com) wrote:
> >> Add some more functions that will be used in memory-device context.
> >>
> >> range_init(): Init using lower bound and size
> >> range_valid(): Check if there would be an overflow when initializin
> >> 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
> >> range_starts_before_range(): Check if one range starts before another
> >> range_ends_after_range(): Check if one range ends after another
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 80 insertions(+)
> >>
> >> diff --git a/include/qemu/range.h b/include/qemu/range.h
> >> index 7e75f4e655..18e8acf22f 100644
> >> --- a/include/qemu/range.h
> >> +++ b/include/qemu/range.h
> >> @@ -112,6 +112,86 @@ 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.
> >> + */
> >> +static inline void range_init(Range *range, uint64_t lob, uint64_t size)
> >> +{
> >> +    range->lob = lob;
> >> +    range->upb = lob + size - 1;
> >> +    range_invariant(range);
> >> +}
> >> +
> >> +/*
> >> + * Check if the interval [@lob,@lob + @size - 1] would be valid or not
> >> + * (result in an overflow).
> >> + */
> >> +static inline bool range_valid(uint64_t lob, uint64_t size)
> >> +{
> >> +    return lob + size >= lob;
> >> +}
> > 
> > That name confused me, I'd expected that to have taken a range and check
> > it for something (like a non-asserting version of the invariant).
> 
> Then we have to remove all the variant asserts from the initializer
> functions (well, because then it is no longer an invariant then). Other
> ideas?

My worry here is just the name 'range_valid'.

Dave

> -- 
> 
> Thanks,
> 
> David / dhildenb
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions

[David Hildenbrand]

On 11/10/2018 11:21, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> On 11/10/2018 11:08, Dr. David Alan Gilbert wrote:
>>> * David Hildenbrand (david@redhat.com) wrote:
>>>> Add some more functions that will be used in memory-device context.
>>>>
>>>> range_init(): Init using lower bound and size
>>>> range_valid(): Check if there would be an overflow when initializin
>>>> 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
>>>> range_starts_before_range(): Check if one range starts before another
>>>> range_ends_after_range(): Check if one range ends after another
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 80 insertions(+)
>>>>
>>>> diff --git a/include/qemu/range.h b/include/qemu/range.h
>>>> index 7e75f4e655..18e8acf22f 100644
>>>> --- a/include/qemu/range.h
>>>> +++ b/include/qemu/range.h
>>>> @@ -112,6 +112,86 @@ 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.
>>>> + */
>>>> +static inline void range_init(Range *range, uint64_t lob, uint64_t size)
>>>> +{
>>>> +    range->lob = lob;
>>>> +    range->upb = lob + size - 1;
>>>> +    range_invariant(range);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Check if the interval [@lob,@lob + @size - 1] would be valid or not
>>>> + * (result in an overflow).
>>>> + */
>>>> +static inline bool range_valid(uint64_t lob, uint64_t size)
>>>> +{
>>>> +    return lob + size >= lob;
>>>> +}
>>>
>>> That name confused me, I'd expected that to have taken a range and check
>>> it for something (like a non-asserting version of the invariant).
>>
>> Then we have to remove all the variant asserts from the initializer
>> functions (well, because then it is no longer an invariant then). Other
>> ideas?
> 
> My worry here is just the name 'range_valid'.
> 

hmm "range_would_overflow()" ?

> Dave

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> On 11/10/2018 11:21, Dr. David Alan Gilbert wrote:
> > * David Hildenbrand (david@redhat.com) wrote:
> >> On 11/10/2018 11:08, Dr. David Alan Gilbert wrote:
> >>> * David Hildenbrand (david@redhat.com) wrote:
> >>>> Add some more functions that will be used in memory-device context.
> >>>>
> >>>> range_init(): Init using lower bound and size
> >>>> range_valid(): Check if there would be an overflow when initializin
> >>>> 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
> >>>> range_starts_before_range(): Check if one range starts before another
> >>>> range_ends_after_range(): Check if one range ends after another
> >>>>
> >>>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>>> ---
> >>>>  include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 80 insertions(+)
> >>>>
> >>>> diff --git a/include/qemu/range.h b/include/qemu/range.h
> >>>> index 7e75f4e655..18e8acf22f 100644
> >>>> --- a/include/qemu/range.h
> >>>> +++ b/include/qemu/range.h
> >>>> @@ -112,6 +112,86 @@ 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.
> >>>> + */
> >>>> +static inline void range_init(Range *range, uint64_t lob, uint64_t size)
> >>>> +{
> >>>> +    range->lob = lob;
> >>>> +    range->upb = lob + size - 1;
> >>>> +    range_invariant(range);
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * Check if the interval [@lob,@lob + @size - 1] would be valid or not
> >>>> + * (result in an overflow).
> >>>> + */
> >>>> +static inline bool range_valid(uint64_t lob, uint64_t size)
> >>>> +{
> >>>> +    return lob + size >= lob;
> >>>> +}
> >>>
> >>> That name confused me, I'd expected that to have taken a range and check
> >>> it for something (like a non-asserting version of the invariant).
> >>
> >> Then we have to remove all the variant asserts from the initializer
> >> functions (well, because then it is no longer an invariant then). Other
> >> ideas?
> > 
> > My worry here is just the name 'range_valid'.
> > 
> 
> hmm "range_would_overflow()" ?

Yes, a bit long but OK.

But another observation; in the following patch, you're tending to do:

  if (!range_valid(...))
     moan


  range_init(...)

would it make more sense to change range_init so it was:

static inline bool range_init(Range *range, uint64_t lob, uint64_t size)
{
    range->lob = lob;
    range->upb = lob + size - 1;
    return ob + size >= lob;
}


and then in the places you use it, you could do:

  if (!range_init(...)
    moan


Dave

> > Dave
> 
> -- 
> 
> Thanks,
> 
> David / dhildenb
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions

[David Hildenbrand]

On 11/10/2018 12:27, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> On 11/10/2018 11:21, Dr. David Alan Gilbert wrote:
>>> * David Hildenbrand (david@redhat.com) wrote:
>>>> On 11/10/2018 11:08, Dr. David Alan Gilbert wrote:
>>>>> * David Hildenbrand (david@redhat.com) wrote:
>>>>>> Add some more functions that will be used in memory-device context.
>>>>>>
>>>>>> range_init(): Init using lower bound and size
>>>>>> range_valid(): Check if there would be an overflow when initializin
>>>>>> 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
>>>>>> range_starts_before_range(): Check if one range starts before another
>>>>>> range_ends_after_range(): Check if one range ends after another
>>>>>>
>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>> ---
>>>>>>  include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 80 insertions(+)
>>>>>>
>>>>>> diff --git a/include/qemu/range.h b/include/qemu/range.h
>>>>>> index 7e75f4e655..18e8acf22f 100644
>>>>>> --- a/include/qemu/range.h
>>>>>> +++ b/include/qemu/range.h
>>>>>> @@ -112,6 +112,86 @@ 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.
>>>>>> + */
>>>>>> +static inline void range_init(Range *range, uint64_t lob, uint64_t size)
>>>>>> +{
>>>>>> +    range->lob = lob;
>>>>>> +    range->upb = lob + size - 1;
>>>>>> +    range_invariant(range);
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * Check if the interval [@lob,@lob + @size - 1] would be valid or not
>>>>>> + * (result in an overflow).
>>>>>> + */
>>>>>> +static inline bool range_valid(uint64_t lob, uint64_t size)
>>>>>> +{
>>>>>> +    return lob + size >= lob;
>>>>>> +}
>>>>>
>>>>> That name confused me, I'd expected that to have taken a range and check
>>>>> it for something (like a non-asserting version of the invariant).
>>>>
>>>> Then we have to remove all the variant asserts from the initializer
>>>> functions (well, because then it is no longer an invariant then). Other
>>>> ideas?
>>>
>>> My worry here is just the name 'range_valid'.
>>>
>>
>> hmm "range_would_overflow()" ?
> 
> Yes, a bit long but OK.
> 
> But another observation; in the following patch, you're tending to do:
> 
>   if (!range_valid(...))
>      moan
> 
> 
>   range_init(...)
> 
> would it make more sense to change range_init so it was:
> 
> static inline bool range_init(Range *range, uint64_t lob, uint64_t size)
> {
>     range->lob = lob;
>     range->upb = lob + size - 1;
>     return ob + size >= lob;
> }
> 
> 
> and then in the places you use it, you could do:
> 
>   if (!range_init(...)
>     moan
> 

Yes, that makes sense.


-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v1 5/7] memory-device: use QEMU_IS_ALIGNED

[David Hildenbrand]

Shorter and easier to read.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 7de1ccd497..996ad1490f 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -120,7 +120,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     g_assert(address_space_end >= address_space_start);
 
     /* address_space_start indicates the maximum alignment we expect */
-    if (QEMU_ALIGN_UP(address_space_start, align) != address_space_start) {
+    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;
@@ -131,13 +131,13 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
         return 0;
     }
 
-    if (hint && QEMU_ALIGN_UP(*hint, align) != *hint) {
+    if (hint && !QEMU_IS_ALIGNED(*hint, align)) {
         error_setg(errp, "address must be aligned to 0x%" PRIx64 " bytes",
                    align);
         return 0;
     }
 
-    if (QEMU_ALIGN_UP(size, align) != size) {
+    if (!QEMU_IS_ALIGNED(size, align)) {
         error_setg(errp, "backend memory size must be multiple of 0x%"
                    PRIx64, align);
         return 0;
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 5/7] memory-device: use QEMU_IS_ALIGNED

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> Shorter and easier to read.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/mem/memory-device.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 7de1ccd497..996ad1490f 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -120,7 +120,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>      g_assert(address_space_end >= address_space_start);
>  
>      /* address_space_start indicates the maximum alignment we expect */
> -    if (QEMU_ALIGN_UP(address_space_start, align) != address_space_start) {
> +    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
>          error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
>                     align);
>          return 0;
> @@ -131,13 +131,13 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>          return 0;
>      }
>  
> -    if (hint && QEMU_ALIGN_UP(*hint, align) != *hint) {
> +    if (hint && !QEMU_IS_ALIGNED(*hint, align)) {
>          error_setg(errp, "address must be aligned to 0x%" PRIx64 " bytes",
>                     align);
>          return 0;
>      }
>  
> -    if (QEMU_ALIGN_UP(size, align) != size) {
> +    if (!QEMU_IS_ALIGNED(size, align)) {
>          error_setg(errp, "backend memory size must be multiple of 0x%"
>                     PRIx64, align);
>          return 0;
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v1 6/7] memory-device: avoid overflows on very huge devices

[David Hildenbrand]

Should not be a problem right now, but it could theoretically happen
in the future.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 996ad1490f..8be63c8032 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -85,7 +85,8 @@ static void memory_device_check_addable(MachineState *ms, uint64_t size,
 
     /* will we exceed the total amount of memory specified */
     memory_device_used_region_size(OBJECT(ms), &used_region_size);
-    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
+    if (used_region_size + size < used_region_size ||
+        used_region_size + size > ms->maxram_size - ms->ram_size) {
         error_setg(errp, "not enough space, currently 0x%" PRIx64
                    " in use of total space for memory devices 0x" RAM_ADDR_FMT,
                    used_region_size, ms->maxram_size - ms->ram_size);
-- 
2.17.1

[Qemu-devel] [PATCH v1 7/7] 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 reports an error instead of succeeding.

"can't add memory device [0xffffffffc0000000:0x80000000], range overflow"

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 56 +++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 8be63c8032..1ca9c8e410 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(&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,26 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     }
 
     if (hint) {
-        new_addr = *hint;
-        if (new_addr < address_space_start) {
+        if (!range_valid(*hint, size)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] before 0x%" PRIx64, new_addr, size,
-                       address_space_start);
+                       "], range overflow", *hint, size);
             return 0;
-        } else if ((new_addr + size) > address_space_end) {
+        }
+        range_init(&new, *hint, size);
+        if (range_starts_before_range(&new, &as)) {
+            error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
+                       "] before 0x%" PRIx64, range_lob(&new), range_size(&new),
+                       range_lob(&as));
+            return 0;
+        } else if (range_ends_after_range(&new, &as)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] beyond 0x%" PRIx64, new_addr, size,
-                       address_space_end);
+                       "] beyond 0x%" PRIx64, range_lob(&new), range_size(&new),
+                       range_upb(&as));
             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        /* our previous size checks make sure that this never overflows */
+        range_init(&new, range_lob(&as), size);
     }
 
     /* find address range that will fit new memory device */
@@ -166,30 +169,37 @@ 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(&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 (!range_valid(next_addr, size)) {
+                range_make_empty(&new);
+                break;
+            }
+            range_init(&new, next_addr, size);
         }
     }
 
-    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.1

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

[Dr. David Alan Gilbert]

* 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 reports an error instead of succeeding.
> 
> "can't add memory device [0xffffffffc0000000:0x80000000], range overflow"
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/memory-device.c | 56 +++++++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 8be63c8032..1ca9c8e410 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(&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,26 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>      }
>  
>      if (hint) {
> -        new_addr = *hint;
> -        if (new_addr < address_space_start) {
> +        if (!range_valid(*hint, size)) {
>              error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
> -                       "] before 0x%" PRIx64, new_addr, size,
> -                       address_space_start);
> +                       "], range overflow", *hint, size);
>              return 0;
> -        } else if ((new_addr + size) > address_space_end) {
> +        }
> +        range_init(&new, *hint, size);
> +        if (range_starts_before_range(&new, &as)) {
> +            error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
> +                       "] before 0x%" PRIx64, range_lob(&new), range_size(&new),
> +                       range_lob(&as));
> +            return 0;
> +        } else if (range_ends_after_range(&new, &as)) {
>              error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
> -                       "] beyond 0x%" PRIx64, new_addr, size,
> -                       address_space_end);
> +                       "] beyond 0x%" PRIx64, range_lob(&new), range_size(&new),
> +                       range_upb(&as));

Would !range_contains_range be easier here?

>              return 0;
>          }
>      } else {
> -        new_addr = address_space_start;
> +        /* our previous size checks make sure that this never overflows */

Hmm, which one exactly?

> +        range_init(&new, range_lob(&as), size);
>      }
>  
>      /* find address range that will fit new memory device */
> @@ -166,30 +169,37 @@ 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(&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 (!range_valid(next_addr, size)) {
> +                range_make_empty(&new);
> +                break;
> +            }
> +            range_init(&new, next_addr, size);
>          }
>      }
>  
> -    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.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[David Hildenbrand]

>>      if (hint) {
>> -        new_addr = *hint;
>> -        if (new_addr < address_space_start) {
>> +        if (!range_valid(*hint, size)) {
>>              error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
>> -                       "] before 0x%" PRIx64, new_addr, size,
>> -                       address_space_start);
>> +                       "], range overflow", *hint, size);
>>              return 0;
>> -        } else if ((new_addr + size) > address_space_end) {
>> +        }
>> +        range_init(&new, *hint, size);
>> +        if (range_starts_before_range(&new, &as)) {
>> +            error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
>> +                       "] before 0x%" PRIx64, range_lob(&new), range_size(&new),
>> +                       range_lob(&as));
>> +            return 0;
>> +        } else if (range_ends_after_range(&new, &as)) {
>>              error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
>> -                       "] beyond 0x%" PRIx64, new_addr, size,
>> -                       address_space_end);
>> +                       "] beyond 0x%" PRIx64, range_lob(&new), range_size(&new),
>> +                       range_upb(&as));
> 
> Would !range_contains_range be easier here?

Yes, but then we have to change the error message, e.g. to

can't add memory device [...:...], usable range [...:...]

> 
>>              return 0;
>>          }
>>      } else {
>> -        new_addr = address_space_start;
>> +        /* our previous size checks make sure that this never overflows */
> 
> Hmm, which one exactly?

Previous patch in this series, via memory_device_check_addable().

But we can add an explicit check here.

> 
>> +        range_init(&new, range_lob(&as), size);
>>      }



-- 

Thanks,

David / dhildenb