Josh Junon <[email protected]> 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.
