From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52483) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUHRQ-0004PK-Df for qemu-devel@nongnu.org; Tue, 25 Aug 2015 12:51:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUHRM-0004j9-PD for qemu-devel@nongnu.org; Tue, 25 Aug 2015 12:51:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47618) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUHRM-0004ip-Fe for qemu-devel@nongnu.org; Tue, 25 Aug 2015 12:51:16 -0400 References: <1440519050-17986-1-git-send-email-cornelia.huck@de.ibm.com> <1440519050-17986-5-git-send-email-cornelia.huck@de.ibm.com> From: Eric Blake Message-ID: <55DC9CFB.3080101@redhat.com> Date: Tue, 25 Aug 2015 10:51:07 -0600 MIME-Version: 1.0 In-Reply-To: <1440519050-17986-5-git-send-email-cornelia.huck@de.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="5SOWbPRpvNxxVOgMnWhogE2Xwg7A91uuS" Subject: Re: [Qemu-devel] [PATCH v2 4/8] s390x: Dump storage keys qmp command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck , qemu-devel@nongnu.org Cc: borntraeger@de.ibm.com, jfrei@linux.vnet.ibm.com, agraf@suse.de, jjherne@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --5SOWbPRpvNxxVOgMnWhogE2Xwg7A91uuS Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 08/25/2015 10:10 AM, Cornelia Huck wrote: > From: "Jason J. Herne" >=20 > 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 supp= ort > within Qemu and for guest operating system developers. >=20 > Reviewed-by: Thomas Huth > Reviewed-by: David Hildenbrand > Signed-off-by: Jason J. Herne > Signed-off-by: Cornelia Huck > --- > =20 > +static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn, > + uint64_t count, Error **errp) > +{ > + uint64_t curpage =3D startgfn; > + uint64_t maxpage =3D curpage + count - 1; > + const char *fmt =3D "page=3D%03" PRIx64 ": key(%d) =3D> ACC=3D%X, = FP=3D%d, REF=3D%d," > + " ch=3D%d, reserved=3D%d\n"; > + char *buf =3D 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 <=3D maxpage; curpage++) { > + uint8_t acc =3D (*keys & 0xF0) >> 4; > + int fp =3D (*keys & 0x08); > + int ref =3D (*keys & 0x04); > + int ch =3D (*keys & 0x02); > + int res =3D (*keys & 0x01); > + > + len =3D 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 =3D s390_get_skeys_device(); > + S390SKeysClass *skeyclass =3D S390_SKEYS_GET_CLASS(ss); > + const uint64_t total_count =3D ram_size / TARGET_PAGE_SIZE; > + uint64_t handled_count =3D 0, cur_count; > + Error *lerr =3D NULL; > + vaddr cur_gfn =3D 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 =3D 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 =3D 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 +=3D cur_count; > + handled_count +=3D 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' } > =20 > ## > +# @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: > =20 > EQMP > =20 > +#if defined TARGET_S390X > + { > + .name =3D "dump-skeys", > + .args_type =3D "filename:F", > + .mhandler.cmd_new =3D 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --5SOWbPRpvNxxVOgMnWhogE2Xwg7A91uuS Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJV3J0CAAoJEKeha0olJ0NqicIH/Rp7TwwT/QU/QeFGC33gp8pO gutzTaIggRoxhc0ciyJ9P+4XqzUNbG75ay16UuMkeMcTL7un1ldAkD7Vc+gUqkzZ GcWFxbe7A6LZdklv79s97wkdTLL2rS9ypAB336bwuqphKvRihfGbKcx0Qs8bX6gK Q2K7QKjcbMtvT8hEku5vbiH/kviZzJ9FSYoD+pQkPHNUNoSZNf3oAVBaAiIbokp+ RwZOFG5wKJQ+gBO1N9eA085t54X1ofmu4rC1EKrUM9VQ50pm05vrq+32/qJeCudk tvzDP4f0CesDS1FRJ9nlRpUXnr9/qqCm0cVYIco7mdlCRXXxsBCRlbOv6wKR4tI= =MSby -----END PGP SIGNATURE----- --5SOWbPRpvNxxVOgMnWhogE2Xwg7A91uuS--