qapi/range/memory-device: fixes and cleanups
v2 cover qapi/range/memory-device: fixes and cleanups
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.
v1 -> v2:
- "range: add some more functions"
-- Reduce number of functions
-- make range_init() return an error in case of overflow -- provide range_init_nofail()
- "memory-device: rewrite address assignment using ranges" -- Use new functions range_init/range_init_nofail
-- Use range_contains_range instead of starts_before/ends_after
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 | 60 +++++++++------- include/qemu/range.h | 68 +++++++++++++++++- qapi/string-input-visitor.c | 133 ++++++++++++++++++++++++++++++------ 3 files changed, 210 insertions(+), 51 deletions(-)
Signature · 1 line
2.17.1
PATCH v2 1/7 qapi: correctly parse uint64_t values from strings
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=-0x40000000Resulting in the same values
(qemu) info memory-devices
Memory device [nvdimm]: "nv1"
addr: 0xffffffffc0000000
slot: 0
node: 0
--- qapi/string-input-visitor.c | 117 ++++++++++++++++++++++++++++++++---- 1 file changed, 106 insertions(+), 11 deletions(-)
qapi/string-input-visitor.c +106 -11
@@ -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,
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v2 1/7] qapi: correctly parse uint64_t values from strings
Quick peek only for now.
Quoted 15 lines · David Hildenbrand
Right now, we parse
uint64_tvalues just likeint64_tvalues, resulting in negative values getting accepted and certain valid large numbers only being representable as negative numbers. Also, reported errors indicate that anint64_tis expected.Parse
uin64_tseparately. Implementation inspired by originalparse_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=-0x40000000Resulting in the same values
Quoted 7 lines
(qemu) info memory-devices
Memory device [nvdimm]: "nv1"
addr:0xffffffffc0000000
slot: 0
node: 0Signed-off-by David Hildenbrand <[email protected]>
Related work on the QObject input visitor:
commit 5923f85fb82df7c8c60a89458a5ae856045e5ab1
Author: Marc-André Lureau <[email protected]> Date: Wed Jun 7 20:36:03 2017 +0400
qapi: update the qobject visitor to use QNUM_U64
Switch to use QNum/uint where appropriate to remove i64 limitation.
The input visitor will cast i64 input to u64 for compatibility reasons (existing json QMP client already use negative i64 for large u64, and expect an implicit cast in qemu).
Note: before the patch, uint64_t values above INT64_MAX are sent over json QMP as negative values, e.g. UINT64_MAX is sent as -1. After the patch, they are sent unmodified. Clearly a bug fix, but we have to consider compatibility issues anyway. libvirt should cope fine, because its parsing of unsigned integers accepts negative values modulo 2^64. There's hope that other clients will, too.
Message-Id: <[email protected]>
Note who it considers backward compatibility. Have you done that for the string input visitor? The commit message should tell.
Quoted 89 lines
--- qapi/string-input-visitor.c | 117 ++++++++++++++++++++++++++++++++---- 1 file changed, 106 insertions(+), 11 deletions(-)qapi/string-input-visitor.c +62
-1@@ -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); +
qapi/string-input-visitor.c error: · line 191
Quoted 10 lines
qapi/string-input-visitor.c +10
-0+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 ; +} +
Do we actually need unsigned ranges? I'm asking because I hate this code, and duplicating can only make it worse.
[...]
Re: [Qemu-devel] [PATCH v2 1/7] qapi: correctly parse uint64_t values from strings
Quoted 48 lines · Markus Armbruster
Quick peek only for now.
Quoted 15 lines · David Hildenbrand
Right now, we parse
uint64_tvalues just likeint64_tvalues, resulting in negative values getting accepted and certain valid large numbers only being representable as negative numbers. Also, reported errors indicate that anint64_tis expected.Parse
uin64_tseparately. Implementation inspired by originalparse_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=-0x40000000Resulting in the same values
Quoted 7 lines
(qemu) info memory-devices
Memory device [nvdimm]: "nv1"
addr:0xffffffffc0000000
slot: 0
node: 0Signed-off-by David Hildenbrand <[email protected]>Related work on the QObject input visitor:
commit
5923f85fb82df7c8c60a89458a5ae856045e5ab1Author: Marc-André Lureau <[email protected]> Date: Wed Jun 7 20:36:03 2017 +0400qapi: update the qobject visitor to use
QNUM_U64Switch to use QNum/uint where appropriate to remove i64 limitation.
The input visitor will cast i64 input to u64 for compatibility reasons (existing json QMP client already use negative i64 for large u64, and expect an implicit cast in qemu).
Note: before the patch,
uint64_tvalues aboveINT64_MAXare sent over json QMP as negative values, e.g.UINT64_MAXis sent as -1. After the patch, they are sent unmodified. Clearly a bug fix, but we have to consider compatibility issues anyway. libvirt should cope fine, because its parsing of unsigned integers accepts negative values modulo 2^64. There's hope that other clients will, too.
Quoted 8 lines
Signed-off-by Marc-André Lureau <[email protected]>Reviewed-by Markus Armbruster <[email protected]>Message-Id: <[email protected]>
[check_native_list() tweaked for consistency with signed case]Signed-off-by Markus Armbruster <[email protected]>Note who it considers backward compatibility. Have you done that for the string input visitor? The commit message should tell.
There should be no compat issues, negative values are still accepted. At least I can't think of any :) We simply allow accepting bigger values.
Quoted 104 lines
Quoted 90 lines
--- qapi/string-input-visitor.c | 117 ++++++++++++++++++++++++++++++++---- 1 file changed, 106 insertions(+), 11 deletions(-)qapi/string-input-visitor.c +62
-1@@ -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); +qapi/string-input-visitor.c error: · line 191
Quoted 10 lines
qapi/string-input-visitor.c +10
-0+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 ; +} +Do we actually need unsigned ranges? I'm asking because I hate this code, and duplicating can only make it worse.
Quoted 1 line
[...]
I don't think we need unsigned ranges BUT I am concerned about backwards compatibility. I'll have to check all users to make sure no property flagged as uint64_t will actually expect ranges. Then we can drop it. (and simplify this code)
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v2 1/7] qapi: correctly parse uint64_t values from strings
Quoted 165 lines · David Hildenbrand
Quoted 48 lines · Markus Armbruster
Quick peek only for now.
Quoted 15 lines · David Hildenbrand
Right now, we parse
uint64_tvalues just likeint64_tvalues, resulting in negative values getting accepted and certain valid large numbers only being representable as negative numbers. Also, reported errors indicate that anint64_tis expected.Parse
uin64_tseparately. Implementation inspired by originalparse_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=-0x40000000Resulting in the same values
Quoted 7 lines
(qemu) info memory-devices
Memory device [nvdimm]: "nv1"
addr:0xffffffffc0000000
slot: 0
node: 0Signed-off-by David Hildenbrand <[email protected]>Related work on the QObject input visitor:
commit
5923f85fb82df7c8c60a89458a5ae856045e5ab1Author: Marc-André Lureau <[email protected]> Date: Wed Jun 7 20:36:03 2017 +0400qapi: update the qobject visitor to use
QNUM_U64Switch to use QNum/uint where appropriate to remove i64 limitation.
The input visitor will cast i64 input to u64 for compatibility reasons (existing json QMP client already use negative i64 for large u64, and expect an implicit cast in qemu).
Note: before the patch,
uint64_tvalues aboveINT64_MAXare sent over json QMP as negative values, e.g.UINT64_MAXis sent as -1. After the patch, they are sent unmodified. Clearly a bug fix, but we have to consider compatibility issues anyway. libvirt should cope fine, because its parsing of unsigned integers accepts negative values modulo 2^64. There's hope that other clients will, too.Quoted 8 lines
Signed-off-by Marc-André Lureau <[email protected]>Reviewed-by Markus Armbruster <[email protected]>Message-Id: <[email protected]>
[check_native_list() tweaked for consistency with signed case]Signed-off-by Markus Armbruster <[email protected]>Note who it considers backward compatibility. Have you done that for the string input visitor? The commit message should tell.
There should be no compat issues, negative values are still accepted. At least I can't think of any :) We simply allow accepting bigger values.
Quoted 104 lines
Quoted 90 lines
--- qapi/string-input-visitor.c | 117 ++++++++++++++++++++++++++++++++---- 1 file changed, 106 insertions(+), 11 deletions(-)qapi/string-input-visitor.c +62
-1@@ -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); +qapi/string-input-visitor.c error: · line 191
Quoted 10 lines
qapi/string-input-visitor.c +10
-0+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 ; +} +Do we actually need unsigned ranges? I'm asking because I hate this code, and duplicating can only make it worse.
Quoted 8 lines
Quoted 1 line
[...]
I don't think we need unsigned ranges BUT I am concerned about backwards compatibility. I'll have to check all users to make sure no property flagged as
uint64_twill actually expect ranges. Then we can drop it. (and simplify this code)
... looking at the details, I think you are right, we should not need that range code at all. I will drop it. Makes things a lot simpler :)
Signature · 3 lines
Thanks, David / dhildenb
PATCH v2 2/7 qapi: use qemu_strtoi64() in parse_str_int64
In reply to David HildenbrandThe qemu api claims to be easier to use, and the resulting code shows that.
--- qapi/string-input-visitor.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
qapi/string-input-visitor.c +5 -11
@@ -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);
Signature · 1 line
2.17.1
PATCH v2 3/7 range: pass const pointer where possible
In reply to David HildenbrandIf there are no changes, let's use a const pointer.
--- include/qemu/range.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
include/qemu/range.h +3 -3
@@ -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; }
Signature · 1 line
2.17.1
PATCH v2 4/7 range: add some more functions
In reply to David HildenbrandAdd some more functions that will be used in memory-device context.
range_init(): Init using lower bound and size, check for validity range_init_nofail(): Init using lower bound and size, validity asserted range_size(): Extract the size of a range range_overlaps_range(): Check for overlaps of two ranges range_contains_range(): Check if one range is contained in the other
--- include/qemu/range.h | 62 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)
include/qemu/range.h +62 -0
@@ -112,6 +112,68 @@ static inline uint64_t range_upb(Range *range)return range->upb; } +/* +* Initialize @range to span the interval [@lob,@lob + @size - 1]. +* @size may be 0. If the range would overflow, returns -ERANGE, otherwise +* 0. +*/ +static inline int QEMU_WARN_UNUSED_RESULT range_init(Range *range, uint64_t lob, +uint64_t size) +{ +if (lob + size < lob) { +return -ERANGE ; + } + range->lob = lob; + range->upb = lob + size -1 ; +range_invariant (range); +return 0 ; +} + +/* +* Initialize @range to span the interval [@lob,@lob + @size - 1]. +* @size may be 0. Range must not overflow. +*/ +static inline void range_init_nofail (Range *range,uint64_t lob,uint64_t size) +{ + range->lob = lob; + range->upb = lob + size -1 ; +range_invariant (range); +} + +/* +* Get the size of @range. +*/ +static inline uint64_t range_size (const Range *range) +{ +return range->upb - range->lob +1 ; +} + +/* +* Check if @range1 overlaps with @range2. If one of the ranges is empty, +* the result is always "false". +*/ +static inline bool range_overlaps_range (const Range *range1, +const Range *range2) +{ +if (range_is_empty (range1) ||range_is_empty (range2)) { +return false; + } +return !(range2->upb < range1->lob || range1->upb < range2->lob); +} + +/* +* Check if @range1 contains @range2. If one of the ranges is empty, +* the result is always "false". +*/ +static inline bool range_contains_range (const Range *range1, +const Range *range2) +{ +if (range_is_empty (range1) ||range_is_empty (range2)) { +return false; + } +return range1->lob <= range2->lob && range1->upb >= range2->upb; +} +/* * Extend @range to the smallest interval that includes @extend_by, too. */
Signature · 1 line
2.17.1
PATCH v2 5/7 memory-device: use QEMU_IS_ALIGNED
In reply to David HildenbrandShorter and easier to read.
--- hw/mem/memory-device.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
hw/mem/memory-device.c +3 -3
@@ -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 ;
Signature · 1 line
2.17.1
PATCH v2 6/7 memory-device: avoid overflows on very huge devices
In reply to David HildenbrandShould not be a problem right now, but it could theoretically happen in the future.
--- hw/mem/memory-device.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
hw/mem/memory-device.c +2 -1
@@ -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);
Signature · 1 line
2.17.1
PATCH v2 7/7 memory-device: rewrite address assignment using ranges
In reply to David HildenbrandLet'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=-0x40000000Now properly reports an error instead of succeeding.
"can't add memory device [0xffffffffc0000000:0x80000000], range overflow"
--- hw/mem/memory-device.c | 53 ++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 23 deletions(-)
hw/mem/memory-device.c +30 -23
@@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms, uint64_t align, uint64_t size, Error **errp) { - uint64_t address_space_start, address_space_end; GSList *list = NULL, *item; -uint64_t new_addr =0 ; +Range as, new = range_empty;if (!ms->device_memory) { error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
@@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms, "enabled, please specify the maxmem option");return 0 ; } - address_space_start = ms->device_memory->base; - address_space_end = address_space_start + -memory_region_size (&ms->device_memory->mr); -g_assert (address_space_end >= address_space_start); +range_init_nofail (&as, ms->device_memory->base, +memory_region_size (&ms->device_memory->mr)); -/* address_space_start indicates the maximum alignment we expect */ -if (!QEMU_IS_ALIGNED (address_space_start, align)) { +/* start of address space indicates the maximum alignment we expect */ +if (!QEMU_IS_ALIGNED (range_lob (&as), align)) {error_setg (errp,"the alignment (0x%" PRIx64") is not supported" , align);return 0 ;
@@ -145,20 +142,24 @@ static uint64_t memory_device_get_free_addr(MachineState *ms, }if (hint) { - new_addr = *hint; -if (new_addr < address_space_start) { +if (range_init (&new, *hint, size)) {error_setg (errp,"can't add memory device [0x%" PRIx64":0x%" PRIx64 -"] before 0x%" PRIx64, new_addr, size, - address_space_start); +"], range overflow" , *hint, size);return 0 ; - }else if ((new_addr + size) > address_space_end) { + } +if (!range_contains_range (&as, &new)) {error_setg (errp,"can't add memory device [0x%" PRIx64":0x%" PRIx64 -"] beyond 0x%" PRIx64, new_addr, size, - address_space_end); +"], usable range for memory devices [0x%" PRIx64":0x%" + PRIx64"]" ,range_lob (&new),range_size (&new), +range_lob (&as),range_size (&as));return 0 ; } }else { - new_addr = address_space_start; +if (range_init (&new,range_lob (&as), size)) { +error_setg (errp,"can't add memory device [0x%" PRIx64":0x%" PRIx64 +"], range overflow" , *hint, size); +return 0 ; + } }/* find address range that will fit new memory device */
@@ -166,30 +167,36 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,for (item = list; item; item =g_slist_next (item)) {const MemoryDeviceState *md = item->data;const MemoryDeviceClass *mdc =MEMORY_DEVICE_GET_CLASS (OBJECT (md)); -uint64_t md_size, md_addr; +uint64_t next_addr; +Range tmp; - md_addr = mdc->get_addr (md); - md_size =memory_device_get_region_size (md, &error_abort); +range_init_nofail (&tmp, mdc->get_addr (md), +memory_device_get_region_size (md, &error_abort)); -if (ranges_overlap (md_addr, md_size, new_addr, size)) { +if (range_overlaps_range (&tmp, &new)) {if (hint) {const DeviceState *d =DEVICE (md);error_setg (errp,"address range conflicts with memory device" " id='%s'" , d->id ? d->id :"(unnamed)" ); goto out; } - new_addr =QEMU_ALIGN_UP (md_addr + md_size, align); + + next_addr =QEMU_ALIGN_UP (range_upb (&tmp) +1 , align); +if (range_init (&new, next_addr,range_size (&new))) { +range_make_empty (&new); +break ; + } } } -if (new_addr + size > address_space_end) { +if (!range_contains_range (&as, &new)) {error_setg (errp,"could not find position in guest address space for " "memory device - memory fragmented due to alignments" ); goto out; } out:g_slist_free (list); -return new_addr; +return range_lob (&new); }MemoryDeviceInfoList *qmp_memory_device_list (void )
Signature · 1 line
2.17.1