* [Qemu-devel] [PATCH v1 0/7] qapi/range/memory-device: fixes and cleanups
@ 2018-10-09 20:56 David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 1/7] qapi: correctly parse uint64_t values from strings David Hildenbrand
` (6 more replies)
0 siblings, 7 replies; 18+ messages in thread
From: David Hildenbrand @ 2018-10-09 20:56 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, Igor Mammedov, Markus Armbruster,
Michael Roth, David Gibson, Eduardo Habkost,
Dr . David Alan Gilbert, 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 1/7] qapi: correctly parse uint64_t values from strings
2018-10-09 20:56 [Qemu-devel] [PATCH v1 0/7] qapi/range/memory-device: fixes and cleanups David Hildenbrand
@ 2018-10-09 20:56 ` David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 2/7] qapi: use qemu_strtoi64() in parse_str_int64 David Hildenbrand
` (5 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2018-10-09 20:56 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, Igor Mammedov, Markus Armbruster,
Michael Roth, David Gibson, Eduardo Habkost,
Dr . David Alan Gilbert, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 2/7] qapi: use qemu_strtoi64() in parse_str_int64
2018-10-09 20:56 [Qemu-devel] [PATCH v1 0/7] qapi/range/memory-device: fixes and cleanups David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 1/7] qapi: correctly parse uint64_t values from strings David Hildenbrand
@ 2018-10-09 20:56 ` David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 3/7] range: pass const pointer where possible David Hildenbrand
` (4 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2018-10-09 20:56 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, Igor Mammedov, Markus Armbruster,
Michael Roth, David Gibson, Eduardo Habkost,
Dr . David Alan Gilbert, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 3/7] range: pass const pointer where possible
2018-10-09 20:56 [Qemu-devel] [PATCH v1 0/7] qapi/range/memory-device: fixes and cleanups David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 1/7] qapi: correctly parse uint64_t values from strings David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 2/7] qapi: use qemu_strtoi64() in parse_str_int64 David Hildenbrand
@ 2018-10-09 20:56 ` David Hildenbrand
2018-10-11 8:39 ` Dr. David Alan Gilbert
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 4/7] range: add some more functions David Hildenbrand
` (3 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: David Hildenbrand @ 2018-10-09 20:56 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, Igor Mammedov, Markus Armbruster,
Michael Roth, David Gibson, Eduardo Habkost,
Dr . David Alan Gilbert, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 4/7] range: add some more functions
2018-10-09 20:56 [Qemu-devel] [PATCH v1 0/7] qapi/range/memory-device: fixes and cleanups David Hildenbrand
` (2 preceding siblings ...)
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 3/7] range: pass const pointer where possible David Hildenbrand
@ 2018-10-09 20:56 ` David Hildenbrand
2018-10-11 9:08 ` Dr. David Alan Gilbert
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 5/7] memory-device: use QEMU_IS_ALIGNED David Hildenbrand
` (2 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: David Hildenbrand @ 2018-10-09 20:56 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, Igor Mammedov, Markus Armbruster,
Michael Roth, David Gibson, Eduardo Habkost,
Dr . David Alan Gilbert, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 5/7] memory-device: use QEMU_IS_ALIGNED
2018-10-09 20:56 [Qemu-devel] [PATCH v1 0/7] qapi/range/memory-device: fixes and cleanups David Hildenbrand
` (3 preceding siblings ...)
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 4/7] range: add some more functions David Hildenbrand
@ 2018-10-09 20:56 ` David Hildenbrand
2018-10-11 8:47 ` Dr. David Alan Gilbert
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 6/7] memory-device: avoid overflows on very huge devices David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 7/7] memory-device: rewrite address assignment using ranges David Hildenbrand
6 siblings, 1 reply; 18+ messages in thread
From: David Hildenbrand @ 2018-10-09 20:56 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, Igor Mammedov, Markus Armbruster,
Michael Roth, David Gibson, Eduardo Habkost,
Dr . David Alan Gilbert, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 6/7] memory-device: avoid overflows on very huge devices
2018-10-09 20:56 [Qemu-devel] [PATCH v1 0/7] qapi/range/memory-device: fixes and cleanups David Hildenbrand
` (4 preceding siblings ...)
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 5/7] memory-device: use QEMU_IS_ALIGNED David Hildenbrand
@ 2018-10-09 20:56 ` David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 7/7] memory-device: rewrite address assignment using ranges David Hildenbrand
6 siblings, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2018-10-09 20:56 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, Igor Mammedov, Markus Armbruster,
Michael Roth, David Gibson, Eduardo Habkost,
Dr . David Alan Gilbert, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v1 7/7] memory-device: rewrite address assignment using ranges
2018-10-09 20:56 [Qemu-devel] [PATCH v1 0/7] qapi/range/memory-device: fixes and cleanups David Hildenbrand
` (5 preceding siblings ...)
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 6/7] memory-device: avoid overflows on very huge devices David Hildenbrand
@ 2018-10-09 20:56 ` David Hildenbrand
2018-10-11 10:20 ` Dr. David Alan Gilbert
6 siblings, 1 reply; 18+ messages in thread
From: David Hildenbrand @ 2018-10-09 20:56 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, Igor Mammedov, Markus Armbruster,
Michael Roth, David Gibson, Eduardo Habkost,
Dr . David Alan Gilbert, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 3/7] range: pass const pointer where possible
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 3/7] range: pass const pointer where possible David Hildenbrand
@ 2018-10-11 8:39 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-11 8:39 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Michael S . Tsirkin, Igor Mammedov,
Markus Armbruster, Michael Roth, David Gibson, Eduardo Habkost
* 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 5/7] memory-device: use QEMU_IS_ALIGNED
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 5/7] memory-device: use QEMU_IS_ALIGNED David Hildenbrand
@ 2018-10-11 8:47 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-11 8:47 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Michael S . Tsirkin, Igor Mammedov,
Markus Armbruster, Michael Roth, David Gibson, Eduardo Habkost
* 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 4/7] range: add some more functions David Hildenbrand
@ 2018-10-11 9:08 ` Dr. David Alan Gilbert
2018-10-11 9:10 ` David Hildenbrand
0 siblings, 1 reply; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-11 9:08 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Michael S . Tsirkin, Igor Mammedov,
Markus Armbruster, Michael Roth, David Gibson, Eduardo Habkost
* 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions
2018-10-11 9:08 ` Dr. David Alan Gilbert
@ 2018-10-11 9:10 ` David Hildenbrand
2018-10-11 9:21 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 18+ messages in thread
From: David Hildenbrand @ 2018-10-11 9:10 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, Michael S . Tsirkin, Igor Mammedov,
Markus Armbruster, Michael Roth, David Gibson, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions
2018-10-11 9:10 ` David Hildenbrand
@ 2018-10-11 9:21 ` Dr. David Alan Gilbert
2018-10-11 9:27 ` David Hildenbrand
0 siblings, 1 reply; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-11 9:21 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Michael S . Tsirkin, Igor Mammedov,
Markus Armbruster, Michael Roth, David Gibson, Eduardo Habkost
* 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions
2018-10-11 9:21 ` Dr. David Alan Gilbert
@ 2018-10-11 9:27 ` David Hildenbrand
2018-10-11 10:27 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 18+ messages in thread
From: David Hildenbrand @ 2018-10-11 9:27 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, Michael S . Tsirkin, Igor Mammedov,
Markus Armbruster, Michael Roth, David Gibson, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 7/7] memory-device: rewrite address assignment using ranges
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 7/7] memory-device: rewrite address assignment using ranges David Hildenbrand
@ 2018-10-11 10:20 ` Dr. David Alan Gilbert
2018-10-11 10:26 ` David Hildenbrand
0 siblings, 1 reply; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-11 10:20 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Michael S . Tsirkin, Igor Mammedov,
Markus Armbruster, Michael Roth, David Gibson, Eduardo Habkost
* 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 7/7] memory-device: rewrite address assignment using ranges
2018-10-11 10:20 ` Dr. David Alan Gilbert
@ 2018-10-11 10:26 ` David Hildenbrand
0 siblings, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2018-10-11 10:26 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, Michael S . Tsirkin, Igor Mammedov,
Markus Armbruster, Michael Roth, David Gibson, Eduardo Habkost
>> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions
2018-10-11 9:27 ` David Hildenbrand
@ 2018-10-11 10:27 ` Dr. David Alan Gilbert
2018-10-11 10:28 ` David Hildenbrand
0 siblings, 1 reply; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-11 10:27 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Michael S . Tsirkin, Igor Mammedov,
Markus Armbruster, Michael Roth, David Gibson, Eduardo Habkost
* 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions
2018-10-11 10:27 ` Dr. David Alan Gilbert
@ 2018-10-11 10:28 ` David Hildenbrand
0 siblings, 0 replies; 18+ messages in thread
From: David Hildenbrand @ 2018-10-11 10:28 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, Michael S . Tsirkin, Igor Mammedov,
Markus Armbruster, Michael Roth, David Gibson, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-10-11 10:28 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-09 20:56 [Qemu-devel] [PATCH v1 0/7] qapi/range/memory-device: fixes and cleanups David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 1/7] qapi: correctly parse uint64_t values from strings David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 2/7] qapi: use qemu_strtoi64() in parse_str_int64 David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 3/7] range: pass const pointer where possible David Hildenbrand
2018-10-11 8:39 ` Dr. David Alan Gilbert
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 4/7] range: add some more functions David Hildenbrand
2018-10-11 9:08 ` Dr. David Alan Gilbert
2018-10-11 9:10 ` David Hildenbrand
2018-10-11 9:21 ` Dr. David Alan Gilbert
2018-10-11 9:27 ` David Hildenbrand
2018-10-11 10:27 ` Dr. David Alan Gilbert
2018-10-11 10:28 ` David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 5/7] memory-device: use QEMU_IS_ALIGNED David Hildenbrand
2018-10-11 8:47 ` Dr. David Alan Gilbert
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 6/7] memory-device: avoid overflows on very huge devices David Hildenbrand
2018-10-09 20:56 ` [Qemu-devel] [PATCH v1 7/7] memory-device: rewrite address assignment using ranges David Hildenbrand
2018-10-11 10:20 ` Dr. David Alan Gilbert
2018-10-11 10:26 ` David Hildenbrand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.