On 08/25/2015 10:10 AM, Cornelia Huck wrote: > From: "Jason J. Herne" <jjhe...@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 <th...@linux.vnet.ibm.com> > Reviewed-by: David Hildenbrand <d...@linux.vnet.ibm.com> > Signed-off-by: Jason J. Herne <jjhe...@linux.vnet.ibm.com> > Signed-off-by: Cornelia Huck <cornelia.h...@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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature