hmp-commands.hx | 4 +- hw/core/machine-hmp-cmds.c | 8 +- include/qapi/qmp/qdict.h | 4 + monitor/hmp-expr.inc | 200 ++++++++++++++++++++++++++++++ monitor/hmp.c | 241 ++++++++----------------------------- qapi/machine.json | 4 +- qobject/qdict.c | 38 ++++++ system/cpus.c | 4 +- tests/unit/check-qdict.c | 39 ++++++ 9 files changed, 343 insertions(+), 199 deletions(-) create mode 100644 monitor/hmp-expr.inc
Previously, int64_t's were used when parsing virtual / physical
addresses for the [p]memsave commands over QMP (HMP was unaffected).
This caused issues when the virtual addresses were in the higher
half of a 64-bit address space, as various portions of the parsing
code would consider them out of bounds.
This PR addresses that, creating uint64_t analogs for much of the
parsing/qdict implementations, as well as the expression parser,
to allow for higher-half virtual addresses (or any address with a
MSB=1).
---
hmp-commands.hx | 4 +-
hw/core/machine-hmp-cmds.c | 8 +-
include/qapi/qmp/qdict.h | 4 +
monitor/hmp-expr.inc | 200 ++++++++++++++++++++++++++++++
monitor/hmp.c | 241 ++++++++-----------------------------
qapi/machine.json | 4 +-
qobject/qdict.c | 38 ++++++
system/cpus.c | 4 +-
tests/unit/check-qdict.c | 39 ++++++
9 files changed, 343 insertions(+), 199 deletions(-)
create mode 100644 monitor/hmp-expr.inc
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 06746f0afc..9e0fe980b1 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -801,7 +801,7 @@ ERST
{
.name = "memsave",
- .args_type = "val:l,size:i,filename:s",
+ .args_type = "val:u,size:u,filename:s",
.params = "addr size file",
.help = "save to disk virtual memory dump starting at 'addr' of size 'size'",
.cmd = hmp_memsave,
@@ -814,7 +814,7 @@ ERST
{
.name = "pmemsave",
- .args_type = "val:l,size:i,filename:s",
+ .args_type = "val:u,size:u,filename:s",
.params = "addr size file",
.help = "save to disk physical memory dump starting at 'addr' of size 'size'",
.cmd = hmp_pmemsave,
diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
index 8701f00cc7..10512a034b 100644
--- a/hw/core/machine-hmp-cmds.c
+++ b/hw/core/machine-hmp-cmds.c
@@ -196,9 +196,9 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
void hmp_memsave(Monitor *mon, const QDict *qdict)
{
- uint32_t size = qdict_get_int(qdict, "size");
+ uint64_t size = qdict_get_uint(qdict, "size");
const char *filename = qdict_get_str(qdict, "filename");
- uint64_t addr = qdict_get_int(qdict, "val");
+ uint64_t addr = qdict_get_uint(qdict, "val");
Error *err = NULL;
int cpu_index = monitor_get_cpu_index(mon);
@@ -213,9 +213,9 @@ void hmp_memsave(Monitor *mon, const QDict *qdict)
void hmp_pmemsave(Monitor *mon, const QDict *qdict)
{
- uint32_t size = qdict_get_int(qdict, "size");
+ uint64_t size = qdict_get_uint(qdict, "size");
const char *filename = qdict_get_str(qdict, "filename");
- uint64_t addr = qdict_get_int(qdict, "val");
+ uint64_t addr = qdict_get_uint(qdict, "val");
Error *err = NULL;
qmp_pmemsave(addr, size, filename, &err);
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 82e90fc072..38c310becb 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -52,17 +52,21 @@ const QDictEntry *qdict_next(const QDict *qdict, const QDictEntry *entry);
void qdict_put_bool(QDict *qdict, const char *key, bool value);
void qdict_put_int(QDict *qdict, const char *key, int64_t value);
+void qdict_put_uint(QDict *qdict, const char *key, uint64_t value);
void qdict_put_null(QDict *qdict, const char *key);
void qdict_put_str(QDict *qdict, const char *key, const char *value);
double qdict_get_double(const QDict *qdict, const char *key);
int64_t qdict_get_int(const QDict *qdict, const char *key);
+uint64_t qdict_get_uint(const QDict *qdict, const char *key);
bool qdict_get_bool(const QDict *qdict, const char *key);
QList *qdict_get_qlist(const QDict *qdict, const char *key);
QDict *qdict_get_qdict(const QDict *qdict, const char *key);
const char *qdict_get_str(const QDict *qdict, const char *key);
int64_t qdict_get_try_int(const QDict *qdict, const char *key,
int64_t def_value);
+uint64_t qdict_get_try_uint(const QDict *qdict, const char *key,
+ uint64_t def_value);
bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
const char *qdict_get_try_str(const QDict *qdict, const char *key);
diff --git a/monitor/hmp-expr.inc b/monitor/hmp-expr.inc
new file mode 100644
index 0000000000..789a957ed2
--- /dev/null
+++ b/monitor/hmp-expr.inc
@@ -0,0 +1,200 @@
+#ifndef HMP_EXPR_INC_TY
+#error "missing HMP_EXPR_INC_TY"
+#endif
+
+#ifndef HMP_EXPR_INC_IDENT
+#error "missing HMP_EXPR_INC_IDENT"
+#endif
+
+static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_sum)(Monitor *mon);
+
+static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_unary)(Monitor *mon)
+{
+ HMP_EXPR_INC_TY n;
+ char *p;
+ int ret;
+
+ switch (*pch) {
+ case '+':
+ next();
+ n = HMP_EXPR_INC_IDENT(expr_unary)(mon);
+ break;
+ case '-':
+ next();
+ n = -HMP_EXPR_INC_IDENT(expr_unary)(mon);
+ break;
+ case '~':
+ next();
+ n = ~HMP_EXPR_INC_IDENT(expr_unary)(mon);
+ break;
+ case '(':
+ next();
+ n = HMP_EXPR_INC_IDENT(expr_sum)(mon);
+ if (*pch != ')') {
+ expr_error(mon, "')' expected");
+ }
+ next();
+ break;
+ case '\'':
+ pch++;
+ if (*pch == '\0') {
+ expr_error(mon, "character constant expected");
+ }
+ n = *pch;
+ pch++;
+ if (*pch != '\'') {
+ expr_error(mon, "missing terminating \' character");
+ }
+ next();
+ break;
+ case '$':
+ {
+ char buf[128], *q;
+ int64_t reg = 0;
+
+ pch++;
+ q = buf;
+ while ((*pch >= 'a' && *pch <= 'z') ||
+ (*pch >= 'A' && *pch <= 'Z') ||
+ (*pch >= '0' && *pch <= '9') ||
+ *pch == '_' || *pch == '.') {
+ if ((q - buf) < sizeof(buf) - 1) {
+ *q++ = *pch;
+ }
+ pch++;
+ }
+ while (qemu_isspace(*pch)) {
+ pch++;
+ }
+ *q = 0;
+ ret = get_monitor_def(mon, ®, buf);
+ if (ret < 0) {
+ expr_error(mon, "unknown register");
+ }
+ n = (HMP_EXPR_INC_TY)reg;
+ }
+ break;
+ case '\0':
+ expr_error(mon, "unexpected end of expression");
+ n = 0;
+ break;
+ default:
+ errno = 0;
+ n = strtoull(pch, &p, 0);
+ if (errno == ERANGE) {
+ expr_error(mon, "number too large");
+ }
+ if (pch == p) {
+ expr_error(mon, "invalid char '%c' in expression", *p);
+ }
+ pch = p;
+ while (qemu_isspace(*pch)) {
+ pch++;
+ }
+ break;
+ }
+ return n;
+}
+
+static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_prod)(Monitor *mon)
+{
+ HMP_EXPR_INC_TY val, val2;
+ int op;
+
+ val = HMP_EXPR_INC_IDENT(expr_unary)(mon);
+ for (;;) {
+ op = *pch;
+ if (op != '*' && op != '/' && op != '%') {
+ break;
+ }
+ next();
+ val2 = HMP_EXPR_INC_IDENT(expr_unary)(mon);
+ switch (op) {
+ default:
+ case '*':
+ val *= val2;
+ break;
+ case '/':
+ case '%':
+ if (val2 == 0) {
+ expr_error(mon, "division by zero");
+ }
+ if (op == '/') {
+ val /= val2;
+ } else {
+ val %= val2;
+ }
+ break;
+ }
+ }
+ return val;
+}
+
+static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_logic)(Monitor *mon)
+{
+ HMP_EXPR_INC_TY val, val2;
+ int op;
+
+ val = HMP_EXPR_INC_IDENT(expr_prod)(mon);
+ for (;;) {
+ op = *pch;
+ if (op != '&' && op != '|' && op != '^') {
+ break;
+ }
+ next();
+ val2 = HMP_EXPR_INC_IDENT(expr_prod)(mon);
+ switch (op) {
+ default:
+ case '&':
+ val &= val2;
+ break;
+ case '|':
+ val |= val2;
+ break;
+ case '^':
+ val ^= val2;
+ break;
+ }
+ }
+ return val;
+}
+
+static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_sum)(Monitor *mon)
+{
+ HMP_EXPR_INC_TY val, val2;
+ int op;
+
+ val = HMP_EXPR_INC_IDENT(expr_logic)(mon);
+ for (;;) {
+ op = *pch;
+ if (op != '+' && op != '-') {
+ break;
+ }
+ next();
+ val2 = HMP_EXPR_INC_IDENT(expr_logic)(mon);
+ if (op == '+') {
+ val += val2;
+ } else {
+ val -= val2;
+ }
+ }
+ return val;
+}
+
+static int HMP_EXPR_INC_IDENT(get_expr)(Monitor *mon, HMP_EXPR_INC_TY *pval, const char **pp)
+{
+ pch = *pp;
+ if (sigsetjmp(expr_env, 0)) {
+ *pp = pch;
+ return -1;
+ }
+ while (qemu_isspace(*pch)) {
+ pch++;
+ }
+ *pval = HMP_EXPR_INC_IDENT(expr_sum)(mon);
+ *pp = pch;
+ return 0;
+}
+
+#undef HMP_EXPR_INC_TY
+#undef HMP_EXPR_INC_IDENT
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 460e8832f6..95d965a20a 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -332,195 +332,13 @@ static void next(void)
}
}
-static int64_t expr_sum(Monitor *mon);
+#define HMP_EXPR_INC_TY int64_t
+#define HMP_EXPR_INC_IDENT(name) name ## _int64
+#include "monitor/hmp-expr.inc"
-static int64_t expr_unary(Monitor *mon)
-{
- int64_t n;
- char *p;
- int ret;
-
- switch (*pch) {
- case '+':
- next();
- n = expr_unary(mon);
- break;
- case '-':
- next();
- n = -expr_unary(mon);
- break;
- case '~':
- next();
- n = ~expr_unary(mon);
- break;
- case '(':
- next();
- n = expr_sum(mon);
- if (*pch != ')') {
- expr_error(mon, "')' expected");
- }
- next();
- break;
- case '\'':
- pch++;
- if (*pch == '\0') {
- expr_error(mon, "character constant expected");
- }
- n = *pch;
- pch++;
- if (*pch != '\'') {
- expr_error(mon, "missing terminating \' character");
- }
- next();
- break;
- case '$':
- {
- char buf[128], *q;
- int64_t reg = 0;
-
- pch++;
- q = buf;
- while ((*pch >= 'a' && *pch <= 'z') ||
- (*pch >= 'A' && *pch <= 'Z') ||
- (*pch >= '0' && *pch <= '9') ||
- *pch == '_' || *pch == '.') {
- if ((q - buf) < sizeof(buf) - 1) {
- *q++ = *pch;
- }
- pch++;
- }
- while (qemu_isspace(*pch)) {
- pch++;
- }
- *q = 0;
- ret = get_monitor_def(mon, ®, buf);
- if (ret < 0) {
- expr_error(mon, "unknown register");
- }
- n = reg;
- }
- break;
- case '\0':
- expr_error(mon, "unexpected end of expression");
- n = 0;
- break;
- default:
- errno = 0;
- n = strtoull(pch, &p, 0);
- if (errno == ERANGE) {
- expr_error(mon, "number too large");
- }
- if (pch == p) {
- expr_error(mon, "invalid char '%c' in expression", *p);
- }
- pch = p;
- while (qemu_isspace(*pch)) {
- pch++;
- }
- break;
- }
- return n;
-}
-
-static int64_t expr_prod(Monitor *mon)
-{
- int64_t val, val2;
- int op;
-
- val = expr_unary(mon);
- for (;;) {
- op = *pch;
- if (op != '*' && op != '/' && op != '%') {
- break;
- }
- next();
- val2 = expr_unary(mon);
- switch (op) {
- default:
- case '*':
- val *= val2;
- break;
- case '/':
- case '%':
- if (val2 == 0) {
- expr_error(mon, "division by zero");
- }
- if (op == '/') {
- val /= val2;
- } else {
- val %= val2;
- }
- break;
- }
- }
- return val;
-}
-
-static int64_t expr_logic(Monitor *mon)
-{
- int64_t val, val2;
- int op;
-
- val = expr_prod(mon);
- for (;;) {
- op = *pch;
- if (op != '&' && op != '|' && op != '^') {
- break;
- }
- next();
- val2 = expr_prod(mon);
- switch (op) {
- default:
- case '&':
- val &= val2;
- break;
- case '|':
- val |= val2;
- break;
- case '^':
- val ^= val2;
- break;
- }
- }
- return val;
-}
-
-static int64_t expr_sum(Monitor *mon)
-{
- int64_t val, val2;
- int op;
-
- val = expr_logic(mon);
- for (;;) {
- op = *pch;
- if (op != '+' && op != '-') {
- break;
- }
- next();
- val2 = expr_logic(mon);
- if (op == '+') {
- val += val2;
- } else {
- val -= val2;
- }
- }
- return val;
-}
-
-static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
-{
- pch = *pp;
- if (sigsetjmp(expr_env, 0)) {
- *pp = pch;
- return -1;
- }
- while (qemu_isspace(*pch)) {
- pch++;
- }
- *pval = expr_sum(mon);
- *pp = pch;
- return 0;
-}
+#define HMP_EXPR_INC_TY uint64_t
+#define HMP_EXPR_INC_IDENT(name) name ## _uint64
+#include "monitor/hmp-expr.inc"
static int get_double(Monitor *mon, double *pval, const char **pp)
{
@@ -882,7 +700,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
}
typestr++;
}
- if (get_expr(mon, &val, &p)) {
+ if (get_expr_int64(mon, &val, &p)) {
goto fail;
}
/* Check if 'i' is greater than 32-bit */
@@ -900,6 +718,51 @@ static QDict *monitor_parse_arguments(Monitor *mon,
qdict_put_int(qdict, key, val);
}
break;
+ case 'd':
+ case 'u':
+ case 'm':
+ {
+ uint64_t val;
+
+ while (qemu_isspace(*p)) {
+ p++;
+ }
+ if (*typestr == '?' || *typestr == '.') {
+ if (*typestr == '?') {
+ if (*p == '\0') {
+ typestr++;
+ break;
+ }
+ } else {
+ if (*p == '.') {
+ p++;
+ while (qemu_isspace(*p)) {
+ p++;
+ }
+ } else {
+ typestr++;
+ break;
+ }
+ }
+ typestr++;
+ }
+
+ if (get_expr_uint64(mon, &val, &p)) {
+ goto fail;
+ }
+
+ /* Check if 'd' is greater than 32-bit */
+ if ((c == 'd') && ((val >> 32) & 0xffffffff)) {
+ monitor_printf(mon, "\'%s\' has failed: ", cmd->name);
+ monitor_printf(mon, "integer is for 32-bit values\n");
+ goto fail;
+ } else if (c == 'm') {
+ val *= MiB;
+ }
+
+ qdict_put_uint(qdict, key, val);
+ }
+ break;
case 'o':
{
int ret;
diff --git a/qapi/machine.json b/qapi/machine.json
index fcfd249e2d..90f4ecbfd0 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -852,7 +852,7 @@
# <- { "return": {} }
##
{ 'command': 'memsave',
- 'data': {'val': 'int', 'size': 'int', 'filename': 'str', '*cpu-index': 'int'} }
+ 'data': {'val': 'uint64', 'size': 'uint64', 'filename': 'str', '*cpu-index': 'int'} }
##
# @pmemsave:
@@ -878,7 +878,7 @@
# <- { "return": {} }
##
{ 'command': 'pmemsave',
- 'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
+ 'data': {'val': 'uint64', 'size': 'uint64', 'filename': 'str'} }
##
# @Memdev:
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 8faff230d3..9696eee57d 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -136,6 +136,11 @@ void qdict_put_int(QDict *qdict, const char *key, int64_t value)
qdict_put(qdict, key, qnum_from_int(value));
}
+void qdict_put_uint(QDict *qdict, const char *key, uint64_t value)
+{
+ qdict_put(qdict, key, qnum_from_uint(value));
+}
+
void qdict_put_bool(QDict *qdict, const char *key, bool value)
{
qdict_put(qdict, key, qbool_from_bool(value));
@@ -209,6 +214,19 @@ int64_t qdict_get_int(const QDict *qdict, const char *key)
return qnum_get_int(qobject_to(QNum, qdict_get(qdict, key)));
}
+/**
+ * qdict_get_int(): Get an unsigned integer mapped by 'key'
+ *
+ * This function assumes that 'key' exists and it stores a
+ * QNum representable as uint.
+ *
+ * Return integer mapped by 'key'.
+ */
+uint64_t qdict_get_uint(const QDict *qdict, const char *key)
+{
+ return qnum_get_uint(qobject_to(QNum, qdict_get(qdict, key)));
+}
+
/**
* qdict_get_bool(): Get a bool mapped by 'key'
*
@@ -272,6 +290,26 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key,
return val;
}
+/**
+ * qdict_get_try_uint(): Try to get unsigned integer mapped by 'key'
+ *
+ * Return integer mapped by 'key', if it is not present in the
+ * dictionary or if the stored object is not a QNum representing an
+ * unsigned integer, 'def_value' will be returned.
+ */
+uint64_t qdict_get_try_uint(const QDict *qdict, const char *key,
+ uint64_t def_value)
+{
+ QNum *qnum = qobject_to(QNum, qdict_get(qdict, key));
+ uint64_t val;
+
+ if (!qnum || !qnum_get_try_uint(qnum, &val)) {
+ return def_value;
+ }
+
+ return val;
+}
+
/**
* qdict_get_try_bool(): Try to get a bool mapped by 'key'
*
diff --git a/system/cpus.c b/system/cpus.c
index 5e3a988a0a..daad8852d1 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -792,7 +792,7 @@ int vm_stop_force_state(RunState state)
}
}
-void qmp_memsave(int64_t addr, int64_t size, const char *filename,
+void qmp_memsave(uint64_t addr, uint64_t size, const char *filename,
bool has_cpu, int64_t cpu_index, Error **errp)
{
FILE *f;
@@ -840,7 +840,7 @@ exit:
fclose(f);
}
-void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
+void qmp_pmemsave(uint64_t addr, uint64_t size, const char *filename,
Error **errp)
{
FILE *f;
diff --git a/tests/unit/check-qdict.c b/tests/unit/check-qdict.c
index b5efa859b0..09ebe08900 100644
--- a/tests/unit/check-qdict.c
+++ b/tests/unit/check-qdict.c
@@ -99,6 +99,21 @@ static void qdict_get_int_test(void)
qobject_unref(tests_dict);
}
+static void qdict_get_uint_test(void)
+{
+ int ret;
+ const unsigned int value = 100;
+ const char *key = "int";
+ QDict *tests_dict = qdict_new();
+
+ qdict_put_uint(tests_dict, key, value);
+
+ ret = qdict_get_uint(tests_dict, key);
+ g_assert(ret == value);
+
+ qobject_unref(tests_dict);
+}
+
static void qdict_get_try_int_test(void)
{
int ret;
@@ -121,6 +136,28 @@ static void qdict_get_try_int_test(void)
qobject_unref(tests_dict);
}
+static void qdict_get_try_uint_test(void)
+{
+ int ret;
+ const unsigned int value = 100;
+ const char *key = "int";
+ QDict *tests_dict = qdict_new();
+
+ qdict_put_uint(tests_dict, key, value);
+ qdict_put_str(tests_dict, "string", "test");
+
+ ret = qdict_get_try_uint(tests_dict, key, 0);
+ g_assert(ret == value);
+
+ ret = qdict_get_try_uint(tests_dict, "missing", -42);
+ g_assert_cmpuint(ret, ==, -42);
+
+ ret = qdict_get_try_uint(tests_dict, "string", -42);
+ g_assert_cmpuint(ret, ==, -42);
+
+ qobject_unref(tests_dict);
+}
+
static void qdict_get_str_test(void)
{
const char *p;
@@ -358,7 +395,9 @@ int main(int argc, char **argv)
/* Continue, but now with fixtures */
g_test_add_func("/public/get", qdict_get_test);
g_test_add_func("/public/get_int", qdict_get_int_test);
+ g_test_add_func("/public/get_uint", qdict_get_uint_test);
g_test_add_func("/public/get_try_int", qdict_get_try_int_test);
+ g_test_add_func("/public/get_try_uint", qdict_get_try_uint_test);
g_test_add_func("/public/get_str", qdict_get_str_test);
g_test_add_func("/public/get_try_str", qdict_get_try_str_test);
g_test_add_func("/public/haskey_not", qdict_haskey_not_test);
--
2.34.1Josh Junon <junon@oro.sh> writes:
> Previously, int64_t's were used when parsing virtual / physical
> addresses for the [p]memsave commands over QMP (HMP was unaffected).
> This caused issues when the virtual addresses were in the higher
> half of a 64-bit address space, as various portions of the parsing
> code would consider them out of bounds.
>
> This PR addresses that, creating uint64_t analogs for much of the
> parsing/qdict implementations, as well as the expression parser,
> to allow for higher-half virtual addresses (or any address with a
> MSB=1).
Too many things for a single patch. Let's ignore that for now.
> ---
> hmp-commands.hx | 4 +-
> hw/core/machine-hmp-cmds.c | 8 +-
> include/qapi/qmp/qdict.h | 4 +
> monitor/hmp-expr.inc | 200 ++++++++++++++++++++++++++++++
> monitor/hmp.c | 241 ++++++++-----------------------------
> qapi/machine.json | 4 +-
> qobject/qdict.c | 38 ++++++
> system/cpus.c | 4 +-
> tests/unit/check-qdict.c | 39 ++++++
> 9 files changed, 343 insertions(+), 199 deletions(-)
> create mode 100644 monitor/hmp-expr.inc
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 06746f0afc..9e0fe980b1 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -801,7 +801,7 @@ ERST
>
> {
> .name = "memsave",
> - .args_type = "val:l,size:i,filename:s",
> + .args_type = "val:u,size:u,filename:s",
> .params = "addr size file",
> .help = "save to disk virtual memory dump starting at 'addr' of size 'size'",
> .cmd = hmp_memsave,
> @@ -814,7 +814,7 @@ ERST
>
> {
> .name = "pmemsave",
> - .args_type = "val:l,size:i,filename:s",
> + .args_type = "val:u,size:u,filename:s",
> .params = "addr size file",
> .help = "save to disk physical memory dump starting at 'addr' of size 'size'",
> .cmd = hmp_pmemsave,
Arg type 'u' is new in this patch, and takes quite some code. Sure we
want it? 'l' has been good enough for decades...
If yes: more commands use arg type 'l' for addresses: x, xp, gpa2hva,
gpa2hpa, gva2gpa, dump-guest-memory.
> diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
> index 8701f00cc7..10512a034b 100644
> --- a/hw/core/machine-hmp-cmds.c
> +++ b/hw/core/machine-hmp-cmds.c
> @@ -196,9 +196,9 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
>
> void hmp_memsave(Monitor *mon, const QDict *qdict)
> {
> - uint32_t size = qdict_get_int(qdict, "size");
> + uint64_t size = qdict_get_uint(qdict, "size");
> const char *filename = qdict_get_str(qdict, "filename");
> - uint64_t addr = qdict_get_int(qdict, "val");
> + uint64_t addr = qdict_get_uint(qdict, "val");
> Error *err = NULL;
> int cpu_index = monitor_get_cpu_index(mon);
>
> @@ -213,9 +213,9 @@ void hmp_memsave(Monitor *mon, const QDict *qdict)
>
> void hmp_pmemsave(Monitor *mon, const QDict *qdict)
> {
> - uint32_t size = qdict_get_int(qdict, "size");
> + uint64_t size = qdict_get_uint(qdict, "size");
> const char *filename = qdict_get_str(qdict, "filename");
> - uint64_t addr = qdict_get_int(qdict, "val");
> + uint64_t addr = qdict_get_uint(qdict, "val");
> Error *err = NULL;
>
> qmp_pmemsave(addr, size, filename, &err);
Compare hmp_memory_dump():
void hmp_memory_dump(Monitor *mon, const QDict *qdict)
{
int count = qdict_get_int(qdict, "count");
int format = qdict_get_int(qdict, "format");
int size = qdict_get_int(qdict, "size");
target_long addr = qdict_get_int(qdict, "addr");
memory_dump(mon, count, format, size, addr, 0);
}
void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict)
{
int count = qdict_get_int(qdict, "count");
int format = qdict_get_int(qdict, "format");
int size = qdict_get_int(qdict, "size");
hwaddr addr = qdict_get_int(qdict, "addr");
memory_dump(mon, count, format, size, addr, 1);
}
Both convert argument "addr" from signed to unsigned silently
hmp_gpa2hva(), hmp_gva2gpa(), hmp_gpa2hpa() do the same.
hmp_dump_guest_memory() keeps it signed, because the QMP command takes
its address signed, like memsave and pmemsave do before this patch.
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index 82e90fc072..38c310becb 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -52,17 +52,21 @@ const QDictEntry *qdict_next(const QDict *qdict, const QDictEntry *entry);
>
> void qdict_put_bool(QDict *qdict, const char *key, bool value);
> void qdict_put_int(QDict *qdict, const char *key, int64_t value);
> +void qdict_put_uint(QDict *qdict, const char *key, uint64_t value);
> void qdict_put_null(QDict *qdict, const char *key);
> void qdict_put_str(QDict *qdict, const char *key, const char *value);
>
> double qdict_get_double(const QDict *qdict, const char *key);
> int64_t qdict_get_int(const QDict *qdict, const char *key);
> +uint64_t qdict_get_uint(const QDict *qdict, const char *key);
> bool qdict_get_bool(const QDict *qdict, const char *key);
> QList *qdict_get_qlist(const QDict *qdict, const char *key);
> QDict *qdict_get_qdict(const QDict *qdict, const char *key);
> const char *qdict_get_str(const QDict *qdict, const char *key);
> int64_t qdict_get_try_int(const QDict *qdict, const char *key,
> int64_t def_value);
> +uint64_t qdict_get_try_uint(const QDict *qdict, const char *key,
> + uint64_t def_value);
> bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
> const char *qdict_get_try_str(const QDict *qdict, const char *key);
>
> diff --git a/monitor/hmp-expr.inc b/monitor/hmp-expr.inc
> new file mode 100644
> index 0000000000..789a957ed2
> --- /dev/null
> +++ b/monitor/hmp-expr.inc
> @@ -0,0 +1,200 @@
> +#ifndef HMP_EXPR_INC_TY
> +#error "missing HMP_EXPR_INC_TY"
> +#endif
> +
> +#ifndef HMP_EXPR_INC_IDENT
> +#error "missing HMP_EXPR_INC_IDENT"
> +#endif
> +
> +static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_sum)(Monitor *mon);
> +
> +static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_unary)(Monitor *mon)
> +{
> + HMP_EXPR_INC_TY n;
> + char *p;
> + int ret;
> +
> + switch (*pch) {
> + case '+':
> + next();
> + n = HMP_EXPR_INC_IDENT(expr_unary)(mon);
> + break;
> + case '-':
> + next();
> + n = -HMP_EXPR_INC_IDENT(expr_unary)(mon);
> + break;
> + case '~':
> + next();
> + n = ~HMP_EXPR_INC_IDENT(expr_unary)(mon);
> + break;
> + case '(':
> + next();
> + n = HMP_EXPR_INC_IDENT(expr_sum)(mon);
> + if (*pch != ')') {
> + expr_error(mon, "')' expected");
> + }
> + next();
> + break;
> + case '\'':
> + pch++;
> + if (*pch == '\0') {
> + expr_error(mon, "character constant expected");
> + }
> + n = *pch;
> + pch++;
> + if (*pch != '\'') {
> + expr_error(mon, "missing terminating \' character");
> + }
> + next();
> + break;
> + case '$':
> + {
> + char buf[128], *q;
> + int64_t reg = 0;
> +
> + pch++;
> + q = buf;
> + while ((*pch >= 'a' && *pch <= 'z') ||
> + (*pch >= 'A' && *pch <= 'Z') ||
> + (*pch >= '0' && *pch <= '9') ||
> + *pch == '_' || *pch == '.') {
> + if ((q - buf) < sizeof(buf) - 1) {
> + *q++ = *pch;
> + }
> + pch++;
> + }
> + while (qemu_isspace(*pch)) {
> + pch++;
> + }
> + *q = 0;
> + ret = get_monitor_def(mon, ®, buf);
> + if (ret < 0) {
> + expr_error(mon, "unknown register");
> + }
> + n = (HMP_EXPR_INC_TY)reg;
> + }
> + break;
> + case '\0':
> + expr_error(mon, "unexpected end of expression");
> + n = 0;
> + break;
> + default:
> + errno = 0;
> + n = strtoull(pch, &p, 0);
> + if (errno == ERANGE) {
> + expr_error(mon, "number too large");
> + }
> + if (pch == p) {
> + expr_error(mon, "invalid char '%c' in expression", *p);
> + }
> + pch = p;
> + while (qemu_isspace(*pch)) {
> + pch++;
> + }
> + break;
> + }
> + return n;
> +}
> +
> +static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_prod)(Monitor *mon)
> +{
> + HMP_EXPR_INC_TY val, val2;
> + int op;
> +
> + val = HMP_EXPR_INC_IDENT(expr_unary)(mon);
> + for (;;) {
> + op = *pch;
> + if (op != '*' && op != '/' && op != '%') {
> + break;
> + }
> + next();
> + val2 = HMP_EXPR_INC_IDENT(expr_unary)(mon);
> + switch (op) {
> + default:
> + case '*':
> + val *= val2;
> + break;
> + case '/':
> + case '%':
> + if (val2 == 0) {
> + expr_error(mon, "division by zero");
> + }
> + if (op == '/') {
> + val /= val2;
> + } else {
> + val %= val2;
> + }
> + break;
> + }
> + }
> + return val;
> +}
> +
> +static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_logic)(Monitor *mon)
> +{
> + HMP_EXPR_INC_TY val, val2;
> + int op;
> +
> + val = HMP_EXPR_INC_IDENT(expr_prod)(mon);
> + for (;;) {
> + op = *pch;
> + if (op != '&' && op != '|' && op != '^') {
> + break;
> + }
> + next();
> + val2 = HMP_EXPR_INC_IDENT(expr_prod)(mon);
> + switch (op) {
> + default:
> + case '&':
> + val &= val2;
> + break;
> + case '|':
> + val |= val2;
> + break;
> + case '^':
> + val ^= val2;
> + break;
> + }
> + }
> + return val;
> +}
> +
> +static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_sum)(Monitor *mon)
> +{
> + HMP_EXPR_INC_TY val, val2;
> + int op;
> +
> + val = HMP_EXPR_INC_IDENT(expr_logic)(mon);
> + for (;;) {
> + op = *pch;
> + if (op != '+' && op != '-') {
> + break;
> + }
> + next();
> + val2 = HMP_EXPR_INC_IDENT(expr_logic)(mon);
> + if (op == '+') {
> + val += val2;
> + } else {
> + val -= val2;
> + }
> + }
> + return val;
> +}
> +
> +static int HMP_EXPR_INC_IDENT(get_expr)(Monitor *mon, HMP_EXPR_INC_TY *pval, const char **pp)
> +{
> + pch = *pp;
> + if (sigsetjmp(expr_env, 0)) {
> + *pp = pch;
> + return -1;
> + }
> + while (qemu_isspace(*pch)) {
> + pch++;
> + }
> + *pval = HMP_EXPR_INC_IDENT(expr_sum)(mon);
> + *pp = pch;
> + return 0;
> +}
> +
> +#undef HMP_EXPR_INC_TY
> +#undef HMP_EXPR_INC_IDENT
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 460e8832f6..95d965a20a 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -332,195 +332,13 @@ static void next(void)
> }
> }
>
> -static int64_t expr_sum(Monitor *mon);
> +#define HMP_EXPR_INC_TY int64_t
> +#define HMP_EXPR_INC_IDENT(name) name ## _int64
> +#include "monitor/hmp-expr.inc"
>
> -static int64_t expr_unary(Monitor *mon)
> -{
> - int64_t n;
> - char *p;
> - int ret;
> -
> - switch (*pch) {
> - case '+':
> - next();
> - n = expr_unary(mon);
> - break;
> - case '-':
> - next();
> - n = -expr_unary(mon);
> - break;
> - case '~':
> - next();
> - n = ~expr_unary(mon);
> - break;
> - case '(':
> - next();
> - n = expr_sum(mon);
> - if (*pch != ')') {
> - expr_error(mon, "')' expected");
> - }
> - next();
> - break;
> - case '\'':
> - pch++;
> - if (*pch == '\0') {
> - expr_error(mon, "character constant expected");
> - }
> - n = *pch;
> - pch++;
> - if (*pch != '\'') {
> - expr_error(mon, "missing terminating \' character");
> - }
> - next();
> - break;
> - case '$':
> - {
> - char buf[128], *q;
> - int64_t reg = 0;
> -
> - pch++;
> - q = buf;
> - while ((*pch >= 'a' && *pch <= 'z') ||
> - (*pch >= 'A' && *pch <= 'Z') ||
> - (*pch >= '0' && *pch <= '9') ||
> - *pch == '_' || *pch == '.') {
> - if ((q - buf) < sizeof(buf) - 1) {
> - *q++ = *pch;
> - }
> - pch++;
> - }
> - while (qemu_isspace(*pch)) {
> - pch++;
> - }
> - *q = 0;
> - ret = get_monitor_def(mon, ®, buf);
> - if (ret < 0) {
> - expr_error(mon, "unknown register");
> - }
> - n = reg;
> - }
> - break;
> - case '\0':
> - expr_error(mon, "unexpected end of expression");
> - n = 0;
> - break;
> - default:
> - errno = 0;
> - n = strtoull(pch, &p, 0);
> - if (errno == ERANGE) {
> - expr_error(mon, "number too large");
> - }
> - if (pch == p) {
> - expr_error(mon, "invalid char '%c' in expression", *p);
> - }
> - pch = p;
> - while (qemu_isspace(*pch)) {
> - pch++;
> - }
> - break;
> - }
> - return n;
> -}
> -
> -static int64_t expr_prod(Monitor *mon)
> -{
> - int64_t val, val2;
> - int op;
> -
> - val = expr_unary(mon);
> - for (;;) {
> - op = *pch;
> - if (op != '*' && op != '/' && op != '%') {
> - break;
> - }
> - next();
> - val2 = expr_unary(mon);
> - switch (op) {
> - default:
> - case '*':
> - val *= val2;
> - break;
> - case '/':
> - case '%':
> - if (val2 == 0) {
> - expr_error(mon, "division by zero");
> - }
> - if (op == '/') {
> - val /= val2;
> - } else {
> - val %= val2;
> - }
> - break;
> - }
> - }
> - return val;
> -}
> -
> -static int64_t expr_logic(Monitor *mon)
> -{
> - int64_t val, val2;
> - int op;
> -
> - val = expr_prod(mon);
> - for (;;) {
> - op = *pch;
> - if (op != '&' && op != '|' && op != '^') {
> - break;
> - }
> - next();
> - val2 = expr_prod(mon);
> - switch (op) {
> - default:
> - case '&':
> - val &= val2;
> - break;
> - case '|':
> - val |= val2;
> - break;
> - case '^':
> - val ^= val2;
> - break;
> - }
> - }
> - return val;
> -}
> -
> -static int64_t expr_sum(Monitor *mon)
> -{
> - int64_t val, val2;
> - int op;
> -
> - val = expr_logic(mon);
> - for (;;) {
> - op = *pch;
> - if (op != '+' && op != '-') {
> - break;
> - }
> - next();
> - val2 = expr_logic(mon);
> - if (op == '+') {
> - val += val2;
> - } else {
> - val -= val2;
> - }
> - }
> - return val;
> -}
> -
> -static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
> -{
> - pch = *pp;
> - if (sigsetjmp(expr_env, 0)) {
> - *pp = pch;
> - return -1;
> - }
> - while (qemu_isspace(*pch)) {
> - pch++;
> - }
> - *pval = expr_sum(mon);
> - *pp = pch;
> - return 0;
> -}
> +#define HMP_EXPR_INC_TY uint64_t
> +#define HMP_EXPR_INC_IDENT(name) name ## _uint64
> +#include "monitor/hmp-expr.inc"
You arrange to have the silly integrated desk calculator compiled twice,
once for signed and once for unsigned. Meh.
>
> static int get_double(Monitor *mon, double *pval, const char **pp)
> {
> @@ -882,7 +700,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
> }
> typestr++;
> }
> - if (get_expr(mon, &val, &p)) {
> + if (get_expr_int64(mon, &val, &p)) {
> goto fail;
> }
> /* Check if 'i' is greater than 32-bit */
> @@ -900,6 +718,51 @@ static QDict *monitor_parse_arguments(Monitor *mon,
> qdict_put_int(qdict, key, val);
> }
> break;
> + case 'd':
> + case 'u':
> + case 'm':
> + {
> + uint64_t val;
> +
> + while (qemu_isspace(*p)) {
> + p++;
> + }
> + if (*typestr == '?' || *typestr == '.') {
> + if (*typestr == '?') {
> + if (*p == '\0') {
> + typestr++;
> + break;
> + }
> + } else {
> + if (*p == '.') {
> + p++;
> + while (qemu_isspace(*p)) {
> + p++;
> + }
> + } else {
> + typestr++;
> + break;
> + }
> + }
> + typestr++;
> + }
> +
> + if (get_expr_uint64(mon, &val, &p)) {
> + goto fail;
> + }
> +
> + /* Check if 'd' is greater than 32-bit */
> + if ((c == 'd') && ((val >> 32) & 0xffffffff)) {
> + monitor_printf(mon, "\'%s\' has failed: ", cmd->name);
> + monitor_printf(mon, "integer is for 32-bit values\n");
> + goto fail;
> + } else if (c == 'm') {
> + val *= MiB;
> + }
> +
> + qdict_put_uint(qdict, key, val);
> + }
> + break;
You duplicate the code for signed arg types 'i', l', and 'M' for the new
unsigned arg types 'd', 'u', and 'm'.
Sure we want these?
If yes: add them to the arg type comment in monitor-internal.h
> case 'o':
> {
> int ret;
> diff --git a/qapi/machine.json b/qapi/machine.json
> index fcfd249e2d..90f4ecbfd0 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -852,7 +852,7 @@
> # <- { "return": {} }
> ##
> { 'command': 'memsave',
> - 'data': {'val': 'int', 'size': 'int', 'filename': 'str', '*cpu-index': 'int'} }
> + 'data': {'val': 'uint64', 'size': 'uint64', 'filename': 'str', '*cpu-index': 'int'} }
Let's use type 'size' for member 'size'.
PLease use the opportunity to wrap this line.
>
> ##
> # @pmemsave:
> @@ -878,7 +878,7 @@
> # <- { "return": {} }
> ##
> { 'command': 'pmemsave',
> - 'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
> + 'data': {'val': 'uint64', 'size': 'uint64', 'filename': 'str'} }
Let's use type 'size' for member 'size'.
> ##
> # @Memdev:
Similarly broken: dump-guest-memory.
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index 8faff230d3..9696eee57d 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -136,6 +136,11 @@ void qdict_put_int(QDict *qdict, const char *key, int64_t value)
> qdict_put(qdict, key, qnum_from_int(value));
> }
>
> +void qdict_put_uint(QDict *qdict, const char *key, uint64_t value)
> +{
> + qdict_put(qdict, key, qnum_from_uint(value));
> +}
> +
> void qdict_put_bool(QDict *qdict, const char *key, bool value)
> {
> qdict_put(qdict, key, qbool_from_bool(value));
> @@ -209,6 +214,19 @@ int64_t qdict_get_int(const QDict *qdict, const char *key)
> return qnum_get_int(qobject_to(QNum, qdict_get(qdict, key)));
> }
>
> +/**
> + * qdict_get_int(): Get an unsigned integer mapped by 'key'
Pasto.
> + *
> + * This function assumes that 'key' exists and it stores a
> + * QNum representable as uint.
> + *
> + * Return integer mapped by 'key'.
> + */
> +uint64_t qdict_get_uint(const QDict *qdict, const char *key)
> +{
> + return qnum_get_uint(qobject_to(QNum, qdict_get(qdict, key)));
> +}
> +
> /**
> * qdict_get_bool(): Get a bool mapped by 'key'
> *
> @@ -272,6 +290,26 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key,
> return val;
> }
>
> +/**
> + * qdict_get_try_uint(): Try to get unsigned integer mapped by 'key'
> + *
> + * Return integer mapped by 'key', if it is not present in the
> + * dictionary or if the stored object is not a QNum representing an
> + * unsigned integer, 'def_value' will be returned.
> + */
> +uint64_t qdict_get_try_uint(const QDict *qdict, const char *key,
> + uint64_t def_value)
Indentation is off.
> +{
> + QNum *qnum = qobject_to(QNum, qdict_get(qdict, key));
> + uint64_t val;
> +
> + if (!qnum || !qnum_get_try_uint(qnum, &val)) {
> + return def_value;
> + }
> +
> + return val;
> +}
> +
> /**
> * qdict_get_try_bool(): Try to get a bool mapped by 'key'
> *
[...]
I'd go for a much smaller solution: change the QMP commands to unsigned,
keep the HMP commands signed, with a silent conversion just like
hmp_memory_dump() & friends.
On Fri, Aug 02, 2024 at 01:39:15PM +0200, Markus Armbruster wrote:
> Josh Junon <junon@oro.sh> writes:
>
> > Previously, int64_t's were used when parsing virtual / physical
> > addresses for the [p]memsave commands over QMP (HMP was unaffected).
> > This caused issues when the virtual addresses were in the higher
> > half of a 64-bit address space, as various portions of the parsing
> > code would consider them out of bounds.
> >
> > This PR addresses that, creating uint64_t analogs for much of the
> > parsing/qdict implementations, as well as the expression parser,
> > to allow for higher-half virtual addresses (or any address with a
> > MSB=1).
>
> Too many things for a single patch. Let's ignore that for now.
>
> > ---
> > hmp-commands.hx | 4 +-
> > hw/core/machine-hmp-cmds.c | 8 +-
> > include/qapi/qmp/qdict.h | 4 +
> > monitor/hmp-expr.inc | 200 ++++++++++++++++++++++++++++++
> > monitor/hmp.c | 241 ++++++++-----------------------------
> > qapi/machine.json | 4 +-
> > qobject/qdict.c | 38 ++++++
> > system/cpus.c | 4 +-
> > tests/unit/check-qdict.c | 39 ++++++
> > 9 files changed, 343 insertions(+), 199 deletions(-)
> > create mode 100644 monitor/hmp-expr.inc
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 06746f0afc..9e0fe980b1 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -801,7 +801,7 @@ ERST
> >
> > {
> > .name = "memsave",
> > - .args_type = "val:l,size:i,filename:s",
> > + .args_type = "val:u,size:u,filename:s",
> > .params = "addr size file",
> > .help = "save to disk virtual memory dump starting at 'addr' of size 'size'",
> > .cmd = hmp_memsave,
> > @@ -814,7 +814,7 @@ ERST
> >
> > {
> > .name = "pmemsave",
> > - .args_type = "val:l,size:i,filename:s",
> > + .args_type = "val:u,size:u,filename:s",
> > .params = "addr size file",
> > .help = "save to disk physical memory dump starting at 'addr' of size 'size'",
> > .cmd = hmp_pmemsave,
>
> Arg type 'u' is new in this patch, and takes quite some code. Sure we
> want it? 'l' has been good enough for decades...
>
> If yes: more commands use arg type 'l' for addresses: x, xp, gpa2hva,
> gpa2hpa, gva2gpa, dump-guest-memory.
>
Fair enough, HMP isn't affected by the signedness anyway, QMP is the
only one that doesn't parse things correctly. Just wanted to keep things
unsigned across the board but I'll keep HMP as it is.
> > diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
> > index 8701f00cc7..10512a034b 100644
> > --- a/hw/core/machine-hmp-cmds.c
> > +++ b/hw/core/machine-hmp-cmds.c
> > @@ -196,9 +196,9 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
> >
> > void hmp_memsave(Monitor *mon, const QDict *qdict)
> > {
> > - uint32_t size = qdict_get_int(qdict, "size");
> > + uint64_t size = qdict_get_uint(qdict, "size");
> > const char *filename = qdict_get_str(qdict, "filename");
> > - uint64_t addr = qdict_get_int(qdict, "val");
> > + uint64_t addr = qdict_get_uint(qdict, "val");
> > Error *err = NULL;
> > int cpu_index = monitor_get_cpu_index(mon);
> >
> > @@ -213,9 +213,9 @@ void hmp_memsave(Monitor *mon, const QDict *qdict)
> >
> > void hmp_pmemsave(Monitor *mon, const QDict *qdict)
> > {
> > - uint32_t size = qdict_get_int(qdict, "size");
> > + uint64_t size = qdict_get_uint(qdict, "size");
> > const char *filename = qdict_get_str(qdict, "filename");
> > - uint64_t addr = qdict_get_int(qdict, "val");
> > + uint64_t addr = qdict_get_uint(qdict, "val");
> > Error *err = NULL;
> >
> > qmp_pmemsave(addr, size, filename, &err);
>
> Compare hmp_memory_dump():
>
> void hmp_memory_dump(Monitor *mon, const QDict *qdict)
> {
> int count = qdict_get_int(qdict, "count");
> int format = qdict_get_int(qdict, "format");
> int size = qdict_get_int(qdict, "size");
> target_long addr = qdict_get_int(qdict, "addr");
>
> memory_dump(mon, count, format, size, addr, 0);
> }
>
> void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict)
> {
> int count = qdict_get_int(qdict, "count");
> int format = qdict_get_int(qdict, "format");
> int size = qdict_get_int(qdict, "size");
> hwaddr addr = qdict_get_int(qdict, "addr");
>
> memory_dump(mon, count, format, size, addr, 1);
> }
>
> Both convert argument "addr" from signed to unsigned silently
>
> hmp_gpa2hva(), hmp_gva2gpa(), hmp_gpa2hpa() do the same.
>
> hmp_dump_guest_memory() keeps it signed, because the QMP command takes
> its address signed, like memsave and pmemsave do before this patch.
>
> > diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> > index 82e90fc072..38c310becb 100644
> > --- a/include/qapi/qmp/qdict.h
> > +++ b/include/qapi/qmp/qdict.h
> > @@ -52,17 +52,21 @@ const QDictEntry *qdict_next(const QDict *qdict, const QDictEntry *entry);
> >
> > void qdict_put_bool(QDict *qdict, const char *key, bool value);
> > void qdict_put_int(QDict *qdict, const char *key, int64_t value);
> > +void qdict_put_uint(QDict *qdict, const char *key, uint64_t value);
> > void qdict_put_null(QDict *qdict, const char *key);
> > void qdict_put_str(QDict *qdict, const char *key, const char *value);
> >
> > double qdict_get_double(const QDict *qdict, const char *key);
> > int64_t qdict_get_int(const QDict *qdict, const char *key);
> > +uint64_t qdict_get_uint(const QDict *qdict, const char *key);
> > bool qdict_get_bool(const QDict *qdict, const char *key);
> > QList *qdict_get_qlist(const QDict *qdict, const char *key);
> > QDict *qdict_get_qdict(const QDict *qdict, const char *key);
> > const char *qdict_get_str(const QDict *qdict, const char *key);
> > int64_t qdict_get_try_int(const QDict *qdict, const char *key,
> > int64_t def_value);
> > +uint64_t qdict_get_try_uint(const QDict *qdict, const char *key,
> > + uint64_t def_value);
> > bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
> > const char *qdict_get_try_str(const QDict *qdict, const char *key);
> >
> > diff --git a/monitor/hmp-expr.inc b/monitor/hmp-expr.inc
> > new file mode 100644
> > index 0000000000..789a957ed2
> > --- /dev/null
> > +++ b/monitor/hmp-expr.inc
> > @@ -0,0 +1,200 @@
> > +#ifndef HMP_EXPR_INC_TY
> > +#error "missing HMP_EXPR_INC_TY"
> > +#endif
> > +
> > +#ifndef HMP_EXPR_INC_IDENT
> > +#error "missing HMP_EXPR_INC_IDENT"
> > +#endif
> > +
> > +static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_sum)(Monitor *mon);
> > +
> > +static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_unary)(Monitor *mon)
> > +{
> > + HMP_EXPR_INC_TY n;
> > + char *p;
> > + int ret;
> > +
> > + switch (*pch) {
> > + case '+':
> > + next();
> > + n = HMP_EXPR_INC_IDENT(expr_unary)(mon);
> > + break;
> > + case '-':
> > + next();
> > + n = -HMP_EXPR_INC_IDENT(expr_unary)(mon);
> > + break;
> > + case '~':
> > + next();
> > + n = ~HMP_EXPR_INC_IDENT(expr_unary)(mon);
> > + break;
> > + case '(':
> > + next();
> > + n = HMP_EXPR_INC_IDENT(expr_sum)(mon);
> > + if (*pch != ')') {
> > + expr_error(mon, "')' expected");
> > + }
> > + next();
> > + break;
> > + case '\'':
> > + pch++;
> > + if (*pch == '\0') {
> > + expr_error(mon, "character constant expected");
> > + }
> > + n = *pch;
> > + pch++;
> > + if (*pch != '\'') {
> > + expr_error(mon, "missing terminating \' character");
> > + }
> > + next();
> > + break;
> > + case '$':
> > + {
> > + char buf[128], *q;
> > + int64_t reg = 0;
> > +
> > + pch++;
> > + q = buf;
> > + while ((*pch >= 'a' && *pch <= 'z') ||
> > + (*pch >= 'A' && *pch <= 'Z') ||
> > + (*pch >= '0' && *pch <= '9') ||
> > + *pch == '_' || *pch == '.') {
> > + if ((q - buf) < sizeof(buf) - 1) {
> > + *q++ = *pch;
> > + }
> > + pch++;
> > + }
> > + while (qemu_isspace(*pch)) {
> > + pch++;
> > + }
> > + *q = 0;
> > + ret = get_monitor_def(mon, ®, buf);
> > + if (ret < 0) {
> > + expr_error(mon, "unknown register");
> > + }
> > + n = (HMP_EXPR_INC_TY)reg;
> > + }
> > + break;
> > + case '\0':
> > + expr_error(mon, "unexpected end of expression");
> > + n = 0;
> > + break;
> > + default:
> > + errno = 0;
> > + n = strtoull(pch, &p, 0);
> > + if (errno == ERANGE) {
> > + expr_error(mon, "number too large");
> > + }
> > + if (pch == p) {
> > + expr_error(mon, "invalid char '%c' in expression", *p);
> > + }
> > + pch = p;
> > + while (qemu_isspace(*pch)) {
> > + pch++;
> > + }
> > + break;
> > + }
> > + return n;
> > +}
> > +
> > +static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_prod)(Monitor *mon)
> > +{
> > + HMP_EXPR_INC_TY val, val2;
> > + int op;
> > +
> > + val = HMP_EXPR_INC_IDENT(expr_unary)(mon);
> > + for (;;) {
> > + op = *pch;
> > + if (op != '*' && op != '/' && op != '%') {
> > + break;
> > + }
> > + next();
> > + val2 = HMP_EXPR_INC_IDENT(expr_unary)(mon);
> > + switch (op) {
> > + default:
> > + case '*':
> > + val *= val2;
> > + break;
> > + case '/':
> > + case '%':
> > + if (val2 == 0) {
> > + expr_error(mon, "division by zero");
> > + }
> > + if (op == '/') {
> > + val /= val2;
> > + } else {
> > + val %= val2;
> > + }
> > + break;
> > + }
> > + }
> > + return val;
> > +}
> > +
> > +static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_logic)(Monitor *mon)
> > +{
> > + HMP_EXPR_INC_TY val, val2;
> > + int op;
> > +
> > + val = HMP_EXPR_INC_IDENT(expr_prod)(mon);
> > + for (;;) {
> > + op = *pch;
> > + if (op != '&' && op != '|' && op != '^') {
> > + break;
> > + }
> > + next();
> > + val2 = HMP_EXPR_INC_IDENT(expr_prod)(mon);
> > + switch (op) {
> > + default:
> > + case '&':
> > + val &= val2;
> > + break;
> > + case '|':
> > + val |= val2;
> > + break;
> > + case '^':
> > + val ^= val2;
> > + break;
> > + }
> > + }
> > + return val;
> > +}
> > +
> > +static HMP_EXPR_INC_TY HMP_EXPR_INC_IDENT(expr_sum)(Monitor *mon)
> > +{
> > + HMP_EXPR_INC_TY val, val2;
> > + int op;
> > +
> > + val = HMP_EXPR_INC_IDENT(expr_logic)(mon);
> > + for (;;) {
> > + op = *pch;
> > + if (op != '+' && op != '-') {
> > + break;
> > + }
> > + next();
> > + val2 = HMP_EXPR_INC_IDENT(expr_logic)(mon);
> > + if (op == '+') {
> > + val += val2;
> > + } else {
> > + val -= val2;
> > + }
> > + }
> > + return val;
> > +}
> > +
> > +static int HMP_EXPR_INC_IDENT(get_expr)(Monitor *mon, HMP_EXPR_INC_TY *pval, const char **pp)
> > +{
> > + pch = *pp;
> > + if (sigsetjmp(expr_env, 0)) {
> > + *pp = pch;
> > + return -1;
> > + }
> > + while (qemu_isspace(*pch)) {
> > + pch++;
> > + }
> > + *pval = HMP_EXPR_INC_IDENT(expr_sum)(mon);
> > + *pp = pch;
> > + return 0;
> > +}
> > +
> > +#undef HMP_EXPR_INC_TY
> > +#undef HMP_EXPR_INC_IDENT
> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> > index 460e8832f6..95d965a20a 100644
> > --- a/monitor/hmp.c
> > +++ b/monitor/hmp.c
> > @@ -332,195 +332,13 @@ static void next(void)
> > }
> > }
> >
> > -static int64_t expr_sum(Monitor *mon);
> > +#define HMP_EXPR_INC_TY int64_t
> > +#define HMP_EXPR_INC_IDENT(name) name ## _int64
> > +#include "monitor/hmp-expr.inc"
> >
> > -static int64_t expr_unary(Monitor *mon)
> > -{
> > - int64_t n;
> > - char *p;
> > - int ret;
> > -
> > - switch (*pch) {
> > - case '+':
> > - next();
> > - n = expr_unary(mon);
> > - break;
> > - case '-':
> > - next();
> > - n = -expr_unary(mon);
> > - break;
> > - case '~':
> > - next();
> > - n = ~expr_unary(mon);
> > - break;
> > - case '(':
> > - next();
> > - n = expr_sum(mon);
> > - if (*pch != ')') {
> > - expr_error(mon, "')' expected");
> > - }
> > - next();
> > - break;
> > - case '\'':
> > - pch++;
> > - if (*pch == '\0') {
> > - expr_error(mon, "character constant expected");
> > - }
> > - n = *pch;
> > - pch++;
> > - if (*pch != '\'') {
> > - expr_error(mon, "missing terminating \' character");
> > - }
> > - next();
> > - break;
> > - case '$':
> > - {
> > - char buf[128], *q;
> > - int64_t reg = 0;
> > -
> > - pch++;
> > - q = buf;
> > - while ((*pch >= 'a' && *pch <= 'z') ||
> > - (*pch >= 'A' && *pch <= 'Z') ||
> > - (*pch >= '0' && *pch <= '9') ||
> > - *pch == '_' || *pch == '.') {
> > - if ((q - buf) < sizeof(buf) - 1) {
> > - *q++ = *pch;
> > - }
> > - pch++;
> > - }
> > - while (qemu_isspace(*pch)) {
> > - pch++;
> > - }
> > - *q = 0;
> > - ret = get_monitor_def(mon, ®, buf);
> > - if (ret < 0) {
> > - expr_error(mon, "unknown register");
> > - }
> > - n = reg;
> > - }
> > - break;
> > - case '\0':
> > - expr_error(mon, "unexpected end of expression");
> > - n = 0;
> > - break;
> > - default:
> > - errno = 0;
> > - n = strtoull(pch, &p, 0);
> > - if (errno == ERANGE) {
> > - expr_error(mon, "number too large");
> > - }
> > - if (pch == p) {
> > - expr_error(mon, "invalid char '%c' in expression", *p);
> > - }
> > - pch = p;
> > - while (qemu_isspace(*pch)) {
> > - pch++;
> > - }
> > - break;
> > - }
> > - return n;
> > -}
> > -
> > -static int64_t expr_prod(Monitor *mon)
> > -{
> > - int64_t val, val2;
> > - int op;
> > -
> > - val = expr_unary(mon);
> > - for (;;) {
> > - op = *pch;
> > - if (op != '*' && op != '/' && op != '%') {
> > - break;
> > - }
> > - next();
> > - val2 = expr_unary(mon);
> > - switch (op) {
> > - default:
> > - case '*':
> > - val *= val2;
> > - break;
> > - case '/':
> > - case '%':
> > - if (val2 == 0) {
> > - expr_error(mon, "division by zero");
> > - }
> > - if (op == '/') {
> > - val /= val2;
> > - } else {
> > - val %= val2;
> > - }
> > - break;
> > - }
> > - }
> > - return val;
> > -}
> > -
> > -static int64_t expr_logic(Monitor *mon)
> > -{
> > - int64_t val, val2;
> > - int op;
> > -
> > - val = expr_prod(mon);
> > - for (;;) {
> > - op = *pch;
> > - if (op != '&' && op != '|' && op != '^') {
> > - break;
> > - }
> > - next();
> > - val2 = expr_prod(mon);
> > - switch (op) {
> > - default:
> > - case '&':
> > - val &= val2;
> > - break;
> > - case '|':
> > - val |= val2;
> > - break;
> > - case '^':
> > - val ^= val2;
> > - break;
> > - }
> > - }
> > - return val;
> > -}
> > -
> > -static int64_t expr_sum(Monitor *mon)
> > -{
> > - int64_t val, val2;
> > - int op;
> > -
> > - val = expr_logic(mon);
> > - for (;;) {
> > - op = *pch;
> > - if (op != '+' && op != '-') {
> > - break;
> > - }
> > - next();
> > - val2 = expr_logic(mon);
> > - if (op == '+') {
> > - val += val2;
> > - } else {
> > - val -= val2;
> > - }
> > - }
> > - return val;
> > -}
> > -
> > -static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
> > -{
> > - pch = *pp;
> > - if (sigsetjmp(expr_env, 0)) {
> > - *pp = pch;
> > - return -1;
> > - }
> > - while (qemu_isspace(*pch)) {
> > - pch++;
> > - }
> > - *pval = expr_sum(mon);
> > - *pp = pch;
> > - return 0;
> > -}
> > +#define HMP_EXPR_INC_TY uint64_t
> > +#define HMP_EXPR_INC_IDENT(name) name ## _uint64
> > +#include "monitor/hmp-expr.inc"
>
> You arrange to have the silly integrated desk calculator compiled twice,
> once for signed and once for unsigned. Meh.
>
Yeah I wasn't proud of this either. I was surprised QEMU even supported
this. I'll revert it since HMP isn't going to be part of the patch
anyway, and I'll make sure everything still works.
> >
> > static int get_double(Monitor *mon, double *pval, const char **pp)
> > {
> > @@ -882,7 +700,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
> > }
> > typestr++;
> > }
> > - if (get_expr(mon, &val, &p)) {
> > + if (get_expr_int64(mon, &val, &p)) {
> > goto fail;
> > }
> > /* Check if 'i' is greater than 32-bit */
> > @@ -900,6 +718,51 @@ static QDict *monitor_parse_arguments(Monitor *mon,
> > qdict_put_int(qdict, key, val);
> > }
> > break;
> > + case 'd':
> > + case 'u':
> > + case 'm':
> > + {
> > + uint64_t val;
> > +
> > + while (qemu_isspace(*p)) {
> > + p++;
> > + }
> > + if (*typestr == '?' || *typestr == '.') {
> > + if (*typestr == '?') {
> > + if (*p == '\0') {
> > + typestr++;
> > + break;
> > + }
> > + } else {
> > + if (*p == '.') {
> > + p++;
> > + while (qemu_isspace(*p)) {
> > + p++;
> > + }
> > + } else {
> > + typestr++;
> > + break;
> > + }
> > + }
> > + typestr++;
> > + }
> > +
> > + if (get_expr_uint64(mon, &val, &p)) {
> > + goto fail;
> > + }
> > +
> > + /* Check if 'd' is greater than 32-bit */
> > + if ((c == 'd') && ((val >> 32) & 0xffffffff)) {
> > + monitor_printf(mon, "\'%s\' has failed: ", cmd->name);
> > + monitor_printf(mon, "integer is for 32-bit values\n");
> > + goto fail;
> > + } else if (c == 'm') {
> > + val *= MiB;
> > + }
> > +
> > + qdict_put_uint(qdict, key, val);
> > + }
> > + break;
>
> You duplicate the code for signed arg types 'i', l', and 'M' for the new
> unsigned arg types 'd', 'u', and 'm'.
>
> Sure we want these?
>
> If yes: add them to the arg type comment in monitor-internal.h
>
> > case 'o':
> > {
> > int ret;
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index fcfd249e2d..90f4ecbfd0 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -852,7 +852,7 @@
> > # <- { "return": {} }
> > ##
> > { 'command': 'memsave',
> > - 'data': {'val': 'int', 'size': 'int', 'filename': 'str', '*cpu-index': 'int'} }
> > + 'data': {'val': 'uint64', 'size': 'uint64', 'filename': 'str', '*cpu-index': 'int'} }
>
> Let's use type 'size' for member 'size'.
>
> PLease use the opportunity to wrap this line.
>
> >
> > ##
> > # @pmemsave:
> > @@ -878,7 +878,7 @@
> > # <- { "return": {} }
> > ##
> > { 'command': 'pmemsave',
> > - 'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
> > + 'data': {'val': 'uint64', 'size': 'uint64', 'filename': 'str'} }
>
> Let's use type 'size' for member 'size'.
>
> > ##
> > # @Memdev:
>
> Similarly broken: dump-guest-memory.
>
> > diff --git a/qobject/qdict.c b/qobject/qdict.c
> > index 8faff230d3..9696eee57d 100644
> > --- a/qobject/qdict.c
> > +++ b/qobject/qdict.c
> > @@ -136,6 +136,11 @@ void qdict_put_int(QDict *qdict, const char *key, int64_t value)
> > qdict_put(qdict, key, qnum_from_int(value));
> > }
> >
> > +void qdict_put_uint(QDict *qdict, const char *key, uint64_t value)
> > +{
> > + qdict_put(qdict, key, qnum_from_uint(value));
> > +}
> > +
> > void qdict_put_bool(QDict *qdict, const char *key, bool value)
> > {
> > qdict_put(qdict, key, qbool_from_bool(value));
> > @@ -209,6 +214,19 @@ int64_t qdict_get_int(const QDict *qdict, const char *key)
> > return qnum_get_int(qobject_to(QNum, qdict_get(qdict, key)));
> > }
> >
> > +/**
> > + * qdict_get_int(): Get an unsigned integer mapped by 'key'
>
> Pasto.
>
> > + *
> > + * This function assumes that 'key' exists and it stores a
> > + * QNum representable as uint.
> > + *
> > + * Return integer mapped by 'key'.
> > + */
> > +uint64_t qdict_get_uint(const QDict *qdict, const char *key)
> > +{
> > + return qnum_get_uint(qobject_to(QNum, qdict_get(qdict, key)));
> > +}
> > +
> > /**
> > * qdict_get_bool(): Get a bool mapped by 'key'
> > *
> > @@ -272,6 +290,26 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key,
> > return val;
> > }
> >
> > +/**
> > + * qdict_get_try_uint(): Try to get unsigned integer mapped by 'key'
> > + *
> > + * Return integer mapped by 'key', if it is not present in the
> > + * dictionary or if the stored object is not a QNum representing an
> > + * unsigned integer, 'def_value' will be returned.
> > + */
> > +uint64_t qdict_get_try_uint(const QDict *qdict, const char *key,
> > + uint64_t def_value)
>
> Indentation is off.
>
> > +{
> > + QNum *qnum = qobject_to(QNum, qdict_get(qdict, key));
> > + uint64_t val;
> > +
> > + if (!qnum || !qnum_get_try_uint(qnum, &val)) {
> > + return def_value;
> > + }
> > +
> > + return val;
> > +}
> > +
> > /**
> > * qdict_get_try_bool(): Try to get a bool mapped by 'key'
> > *
>
> [...]
>
> I'd go for a much smaller solution: change the QMP commands to unsigned,
> keep the HMP commands signed, with a silent conversion just like
> hmp_memory_dump() & friends.
>
Sounds good to me, thanks for the review!
- Josh
Josh Junon <junon@oro.sh> writes: > On Fri, Aug 02, 2024 at 01:39:15PM +0200, Markus Armbruster wrote: [...] >> I'd go for a much smaller solution: change the QMP commands to unsigned, >> keep the HMP commands signed, with a silent conversion just like >> hmp_memory_dump() & friends. >> > > Sounds good to me, thanks for the review! You're welcome! Looking forward to v2.
© 2016 - 2024 Red Hat, Inc.