From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53207) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUfDe-0008BX-Ir for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:14:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUfDb-0007hl-7s for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:14:42 -0400 Received: from e35.co.us.ibm.com ([32.97.110.153]:59360) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUfDa-0007gE-Ud for qemu-devel@nongnu.org; Wed, 26 Aug 2015 14:14:39 -0400 Received: from /spool/local by e35.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 26 Aug 2015 12:14:37 -0600 Received: from b03cxnp07029.gho.boulder.ibm.com (b03cxnp07029.gho.boulder.ibm.com [9.17.130.16]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id C31843E4003F for ; Wed, 26 Aug 2015 12:14:33 -0600 (MDT) Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by b03cxnp07029.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t7QIEXWn34537682 for ; Wed, 26 Aug 2015 11:14:33 -0700 Received: from d03av03.boulder.ibm.com (localhost [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t7QIEXlM018444 for ; Wed, 26 Aug 2015 12:14:33 -0600 Message-ID: <55DE0207.3000503@linux.vnet.ibm.com> Date: Wed, 26 Aug 2015 14:14:31 -0400 From: "Jason J. Herne" MIME-Version: 1.0 References: <1440519050-17986-1-git-send-email-cornelia.huck@de.ibm.com> <1440519050-17986-5-git-send-email-cornelia.huck@de.ibm.com> <55DC9CFB.3080101@redhat.com> In-Reply-To: <55DC9CFB.3080101@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 4/8] s390x: Dump storage keys qmp command Reply-To: jjherne@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Cornelia Huck , qemu-devel@nongnu.org Cc: borntraeger@de.ibm.com, jfrei@linux.vnet.ibm.com, agraf@suse.de On 08/25/2015 12:51 PM, Eric Blake wrote: > On 08/25/2015 10:10 AM, Cornelia Huck wrote: >> From: "Jason J. Herne" >> >> 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 >> Reviewed-by: David Hildenbrand >> Signed-off-by: Jason J. Herne >> Signed-off-by: Cornelia Huck >> --- > >> >> +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. > -- -- Jason J. Herne (jjherne@linux.vnet.ibm.com)