| Message ID | 1440519050-17986-5-git-send-email-cornelia.huck@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 08/25/2015 10:10 AM, Cornelia Huck wrote: > From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> > > Provide a dump-skeys qmp command to allow the end user to dump storage > keys. This is useful for debugging problems with guest storage key support > within Qemu and for guest operating system developers. > > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com> > Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com> > Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > > +static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn, > + uint64_t count, Error **errp) > +{ > + uint64_t curpage = startgfn; > + uint64_t maxpage = curpage + count - 1; > + const char *fmt = "page=%03" PRIx64 ": key(%d) => ACC=%X, FP=%d, REF=%d," > + " ch=%d, reserved=%d\n"; > + char *buf = g_try_malloc(128); > + int len; > + > + if (!buf) { > + error_setg(errp, "Out of memory"); > + return; > + } 128 bytes is small enough to just stack-allocate, and forget about malloc(). Even if you insist on malloc'ing, a simple g_malloc() is nicer than g_try_malloc(), as it is unlikely to fail (and if it DOES fail, something else is likely to fail soon) - we tend to reserve g_try_malloc() for potentially large allocations where failure is more likely. > + > + for (; curpage <= maxpage; curpage++) { > + uint8_t acc = (*keys & 0xF0) >> 4; > + int fp = (*keys & 0x08); > + int ref = (*keys & 0x04); > + int ch = (*keys & 0x02); > + int res = (*keys & 0x01); > + > + len = snprintf(buf, 128, fmt, curpage, If you stack-allocate buf, then sizeof(buf) is nicer than hard-coded 128 here. > + *keys, acc, fp, ref, ch, res); > + qemu_put_buffer(f, (uint8_t *)buf, len); Potential bug. snprintf() returns how many bytes WOULD have been printed if the buffer is large enough, and may therefore be larger than 128 if your buffer size guess was wrong or the format string is edited. The only way to safely use snprintf is to first check that the result is no larger than the input, before passing the string on to qemu_put_buffer(). > +void qmp_dump_skeys(const char *filename, Error **errp) > +{ > + S390SKeysState *ss = s390_get_skeys_device(); > + S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss); > + const uint64_t total_count = ram_size / TARGET_PAGE_SIZE; > + uint64_t handled_count = 0, cur_count; > + Error *lerr = NULL; > + vaddr cur_gfn = 0; > + uint8_t *buf; > + int ret; > + QEMUFile *f; > + > + /* Quick check to see if guest is using storage keys*/ > + if (!skeyclass->skeys_enabled(ss)) { > + error_setg(&lerr, "This guest is not using storage keys. " > + "Nothing to dump."); Error messages don't usually end in '.' > + error_propagate(errp, lerr); Instead of setting the local error just to propagate it, just write the error message directly into errp, as in: error_setg(errp, ...) > + return; > + } > + > + f = qemu_fopen(filename, "wb"); > + if (!f) { > + error_setg(&lerr, "Could not open file"); > + error_propagate(errp, lerr); Same story. Also, we have error_setg_file_open() which is more appropriate to use here. > + ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf); > + if (ret < 0) { > + error_setg(&lerr, "get_keys error %d", ret); > + error_propagate(errp, lerr); > + goto out_free; > + } > + > + /* write keys to stream */ > + write_keys(f, buf, cur_gfn, cur_count, &lerr); > + if (lerr) { > + error_propagate(errp, lerr); > + goto out_free; Instead of propagating the error on every caller... > + } > + > + cur_gfn += cur_count; > + handled_count += cur_count; > + } > + > +out_free: > + g_free(buf); you could do it just once here unconditionally (it is safe to call error_propagate(..., NULL) when no error occurred). > +++ b/qapi-schema.json > @@ -2058,6 +2058,19 @@ > 'returns': 'DumpGuestMemoryCapability' } > > ## > +# @dump-skeys > +# > +# Dump guest's storage keys. @filename: the path to the file to dump to. Newline before @filename, please. > +# This command is only supported on s390 architecture. It would be nice if we fixed the qapi generator to allow conditional compilation of the .json files, so that the command is not even exposed on other platforms. Markus mentioned that at KVM Forum as one of the possible followups to pursue after his current pending series on introspection lands. [1] > +# > +# Returns: nothing on success The 'Returns' line adds no information, so it is better omitted. > +# > +# Since: 2.5 > +## > +{ 'command': 'dump-skeys', > + 'data': { 'filename': 'str' } } > + > +## > # @netdev_add: > # > # Add a network backend. > diff --git a/qmp-commands.hx b/qmp-commands.hx > index ba630b1..9848fd8 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -872,6 +872,31 @@ Example: > > EQMP > > +#if defined TARGET_S390X > + { > + .name = "dump-skeys", > + .args_type = "filename:F", > + .mhandler.cmd_new = qmp_marshal_input_dump_skeys, > + }, > +#endif [1] At any rate, as long as we have the .hx file that does support conditional compilation, I think 'query-commands' properly shows whether the command is present, even if Markus' addition of 'query-schema' does not have the same luxury of omitting unused stuff.
On 08/25/2015 12:51 PM, Eric Blake wrote: > On 08/25/2015 10:10 AM, Cornelia Huck wrote: >> From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> >> >> Provide a dump-skeys qmp command to allow the end user to dump storage >> keys. This is useful for debugging problems with guest storage key support >> within Qemu and for guest operating system developers. >> >> Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com> >> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com> >> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> >> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> >> --- > >> >> +static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn, >> + uint64_t count, Error **errp) >> +{ >> + uint64_t curpage = startgfn; >> + uint64_t maxpage = curpage + count - 1; >> + const char *fmt = "page=%03" PRIx64 ": key(%d) => ACC=%X, FP=%d, REF=%d," >> + " ch=%d, reserved=%d\n"; >> + char *buf = g_try_malloc(128); >> + int len; >> + >> + if (!buf) { >> + error_setg(errp, "Out of memory"); >> + return; >> + } > > 128 bytes is small enough to just stack-allocate, and forget about > malloc(). Even if you insist on malloc'ing, a simple g_malloc() is > nicer than g_try_malloc(), as it is unlikely to fail (and if it DOES > fail, something else is likely to fail soon) - we tend to reserve > g_try_malloc() for potentially large allocations where failure is more > likely. > Will do. >> + >> + for (; curpage <= maxpage; curpage++) { >> + uint8_t acc = (*keys & 0xF0) >> 4; >> + int fp = (*keys & 0x08); >> + int ref = (*keys & 0x04); >> + int ch = (*keys & 0x02); >> + int res = (*keys & 0x01); >> + >> + len = snprintf(buf, 128, fmt, curpage, > > If you stack-allocate buf, then sizeof(buf) is nicer than hard-coded 128 > here. > Will use sizeof() >> + *keys, acc, fp, ref, ch, res); >> + qemu_put_buffer(f, (uint8_t *)buf, len); > > Potential bug. snprintf() returns how many bytes WOULD have been printed > if the buffer is large enough, and may therefore be larger than 128 if > your buffer size guess was wrong or the format string is edited. The > only way to safely use snprintf is to first check that the result is no > larger than the input, before passing the string on to qemu_put_buffer(). > I chose 128 because it is large enough to handle even the most extreme cases. I understand that someone could change the format string later on but if that is the case then they should update the buffer size accordingly. I can add a comment to that effect if you think that is good. But I'd like to avoid over engineering something that is fairly simple. >> +void qmp_dump_skeys(const char *filename, Error **errp) >> +{ >> + S390SKeysState *ss = s390_get_skeys_device(); >> + S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss); >> + const uint64_t total_count = ram_size / TARGET_PAGE_SIZE; >> + uint64_t handled_count = 0, cur_count; >> + Error *lerr = NULL; >> + vaddr cur_gfn = 0; >> + uint8_t *buf; >> + int ret; >> + QEMUFile *f; >> + >> + /* Quick check to see if guest is using storage keys*/ >> + if (!skeyclass->skeys_enabled(ss)) { >> + error_setg(&lerr, "This guest is not using storage keys. " >> + "Nothing to dump."); > > Error messages don't usually end in '.' > Will fix. >> + error_propagate(errp, lerr); > > Instead of setting the local error just to propagate it, just write the > error message directly into errp, as in: > > error_setg(errp, ...) > Ok, will fix. >> + return; >> + } >> + >> + f = qemu_fopen(filename, "wb"); >> + if (!f) { >> + error_setg(&lerr, "Could not open file"); >> + error_propagate(errp, lerr); > > Same story. Also, we have error_setg_file_open() which is more > appropriate to use here. > Neat :) Will switch to that. >> + ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf); >> + if (ret < 0) { >> + error_setg(&lerr, "get_keys error %d", ret); >> + error_propagate(errp, lerr); >> + goto out_free; >> + } >> + >> + /* write keys to stream */ >> + write_keys(f, buf, cur_gfn, cur_count, &lerr); >> + if (lerr) { >> + error_propagate(errp, lerr); >> + goto out_free; > > Instead of propagating the error on every caller... > >> + } >> + >> + cur_gfn += cur_count; >> + handled_count += cur_count; >> + } >> + >> +out_free: >> + g_free(buf); > > you could do it just once here unconditionally (it is safe to call > error_propagate(..., NULL) when no error occurred). Awesome, thanks for the tip. > >> +++ b/qapi-schema.json >> @@ -2058,6 +2058,19 @@ >> 'returns': 'DumpGuestMemoryCapability' } >> >> ## >> +# @dump-skeys >> +# >> +# Dump guest's storage keys. @filename: the path to the file to dump to. > > Newline before @filename, please. > Will fix. >> +# This command is only supported on s390 architecture. > > It would be nice if we fixed the qapi generator to allow conditional > compilation of the .json files, so that the command is not even exposed > on other platforms. Markus mentioned that at KVM Forum as one of the > possible followups to pursue after his current pending series on > introspection lands. [1] > >> +# >> +# Returns: nothing on success > > The 'Returns' line adds no information, so it is better omitted. > Will fix. >> +# >> +# Since: 2.5 >> +## >> +{ 'command': 'dump-skeys', >> + 'data': { 'filename': 'str' } } >> + >> +## >> # @netdev_add: >> # >> # Add a network backend. >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index ba630b1..9848fd8 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -872,6 +872,31 @@ Example: >> >> EQMP >> >> +#if defined TARGET_S390X >> + { >> + .name = "dump-skeys", >> + .args_type = "filename:F", >> + .mhandler.cmd_new = qmp_marshal_input_dump_skeys, >> + }, >> +#endif > > [1] At any rate, as long as we have the .hx file that does support > conditional compilation, I think 'query-commands' properly shows whether > the command is present, even if Markus' addition of 'query-schema' does > not have the same luxury of omitting unused stuff. >
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c index 77c42ff..ebf6a54 100644 --- a/hw/s390x/s390-skeys.c +++ b/hw/s390x/s390-skeys.c @@ -10,9 +10,12 @@ */ #include "hw/boards.h" +#include "qmp-commands.h" #include "hw/s390x/storage-keys.h" #include "qemu/error-report.h" +#define S390_SKEYS_BUFFER_SIZE 131072 /* Room for 128k storage keys */ + S390SKeysState *s390_get_skeys_device(void) { S390SKeysState *ss; @@ -38,6 +41,100 @@ void s390_skeys_init(void) qdev_init_nofail(DEVICE(obj)); } +static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn, + uint64_t count, Error **errp) +{ + uint64_t curpage = startgfn; + uint64_t maxpage = curpage + count - 1; + const char *fmt = "page=%03" PRIx64 ": key(%d) => ACC=%X, FP=%d, REF=%d," + " ch=%d, reserved=%d\n"; + char *buf = g_try_malloc(128); + int len; + + if (!buf) { + error_setg(errp, "Out of memory"); + return; + } + + for (; curpage <= maxpage; curpage++) { + uint8_t acc = (*keys & 0xF0) >> 4; + int fp = (*keys & 0x08); + int ref = (*keys & 0x04); + int ch = (*keys & 0x02); + int res = (*keys & 0x01); + + len = snprintf(buf, 128, fmt, curpage, + *keys, acc, fp, ref, ch, res); + qemu_put_buffer(f, (uint8_t *)buf, len); + keys++; + } + + g_free(buf); +} + +void qmp_dump_skeys(const char *filename, Error **errp) +{ + S390SKeysState *ss = s390_get_skeys_device(); + S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss); + const uint64_t total_count = ram_size / TARGET_PAGE_SIZE; + uint64_t handled_count = 0, cur_count; + Error *lerr = NULL; + vaddr cur_gfn = 0; + uint8_t *buf; + int ret; + QEMUFile *f; + + /* Quick check to see if guest is using storage keys*/ + if (!skeyclass->skeys_enabled(ss)) { + error_setg(&lerr, "This guest is not using storage keys. " + "Nothing to dump."); + error_propagate(errp, lerr); + return; + } + + f = qemu_fopen(filename, "wb"); + if (!f) { + error_setg(&lerr, "Could not open file"); + error_propagate(errp, lerr); + return; + } + + buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE); + if (!buf) { + error_setg(&lerr, "Could not allocate memory"); + error_propagate(errp, lerr); + goto out; + } + + /* we'll only dump initial memory for now */ + while (handled_count < total_count) { + /* Calculate how many keys to ask for & handle overflow case */ + cur_count = MIN(total_count - handled_count, S390_SKEYS_BUFFER_SIZE); + + ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf); + if (ret < 0) { + error_setg(&lerr, "get_keys error %d", ret); + error_propagate(errp, lerr); + goto out_free; + } + + /* write keys to stream */ + write_keys(f, buf, cur_gfn, cur_count, &lerr); + if (lerr) { + error_propagate(errp, lerr); + goto out_free; + } + + cur_gfn += cur_count; + handled_count += cur_count; + } + +out_free: + g_free(buf); +out: + qemu_fclose(f); +} + static void qemu_s390_skeys_init(Object *obj) { QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj); diff --git a/monitor.c b/monitor.c index fc32f12..daa3d98 100644 --- a/monitor.c +++ b/monitor.c @@ -5361,3 +5361,10 @@ void qmp_rtc_reset_reinjection(Error **errp) error_setg(errp, QERR_FEATURE_DISABLED, "rtc-reset-reinjection"); } #endif + +#ifndef TARGET_S390X +void qmp_dump_skeys(const char *filename, Error **errp) +{ + error_setg(errp, QERR_FEATURE_DISABLED, "dump-skeys"); +} +#endif diff --git a/qapi-schema.json b/qapi-schema.json index 4342a08..1213b4e 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2058,6 +2058,19 @@ 'returns': 'DumpGuestMemoryCapability' } ## +# @dump-skeys +# +# Dump guest's storage keys. @filename: the path to the file to dump to. +# This command is only supported on s390 architecture. +# +# Returns: nothing on success +# +# Since: 2.5 +## +{ 'command': 'dump-skeys', + 'data': { 'filename': 'str' } } + +## # @netdev_add: # # Add a network backend. diff --git a/qmp-commands.hx b/qmp-commands.hx index ba630b1..9848fd8 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -872,6 +872,31 @@ Example: EQMP +#if defined TARGET_S390X + { + .name = "dump-skeys", + .args_type = "filename:F", + .mhandler.cmd_new = qmp_marshal_input_dump_skeys, + }, +#endif + +SQMP +dump-skeys +---------- + +Save guest storage keys to file. + +Arguments: + +- "filename": file path (json-string) + +Example: + +-> { "execute": "dump-skeys", "arguments": { "filename": "/tmp/skeys" } } +<- { "return": {} } + +EQMP + { .name = "netdev_add", .args_type = "netdev:O",