From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58700) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YR0Vy-0006qT-A4 for qemu-devel@nongnu.org; Thu, 26 Feb 2015 10:38:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YR0Vt-00088U-Ag for qemu-devel@nongnu.org; Thu, 26 Feb 2015 10:38:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52684) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YR0Vt-00088P-3Q for qemu-devel@nongnu.org; Thu, 26 Feb 2015 10:38:09 -0500 Date: Thu, 26 Feb 2015 15:38:04 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20150226153746.GI2371@work-vm> References: <1424873192-3644-1-git-send-email-armbru@redhat.com> <1424873192-3644-2-git-send-email-armbru@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1424873192-3644-2-git-send-email-armbru@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/2] cutils: Proactively fix pow2floor(), switch to unsigned List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: amit.shah@redhat.com, aik@ozlabs.ru, qemu-devel@nongnu.org, quintela@redhat.com * Markus Armbruster (armbru@redhat.com) wrote: > The function's stated contract is simple enough: "round down to the > nearest power of 2". Suggests the domain is the representable numbers > >= 1, because that's the smallest power of two. > > The implementation doesn't check for domain errors, but returns > garbage instead: > > * For negative arguments, pow2floor() returns -2^63, which is not even > a power of two, let alone the nearest one. > > * For a zero argument, pow2floor() shifts right by 64. Undefined > behavior. Common actual behavior is to shift by 0, yielding -2^63. > > Fix by switching to unsigned types and amending the contract to map > zero to zero. > > Callers are fine with that: > > * xbzrle_cache_resize() > > Passes an int64_t argument >= TARGET_PAGE_SIZE. Thus, the argument > value is representable in uint64_t, and not zero. The result is > converted back to int64_t. Safe, because int64_t can represent the > value. > > No change. > > * cache_init() > > Likewise, except > 0 instead of >= TARGET_PAGE_SIZE. No change. > > * cache_resize() > > Unused since commit fd8cec9. Before, its caller passed an int64_t > argumet >= 1. > > * spapr_node0_size() and spapr_populate_memory() > > Argument comes from numa_info[].node_mem. This is uint64_t. Before > this patch, we convert it to int64_t for pow2floor(), and convert > its result to hwaddr, i.e. back to uint64_t. Not obviously safe. > The patch gets rid of the dubious conversions. > > The patch also gets rid of undefined behavior on zero, and maps zero > to zero instead. Beats mapping it to 2^63, which is what we > commonly get from the undefined behavior before the patch. > > Thus, the patch is a strict improvement here. > > Signed-off-by: Markus Armbruster > --- > include/qemu-common.h | 6 ++++-- > util/cutils.c | 11 +++++------ > 2 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/include/qemu-common.h b/include/qemu-common.h > index 644b46d..f3ede45 100644 > --- a/include/qemu-common.h > +++ b/include/qemu-common.h > @@ -415,8 +415,10 @@ static inline bool is_power_of_2(uint64_t value) > return !(value & (value - 1)); > } > > -/* round down to the nearest power of 2*/ > -int64_t pow2floor(int64_t value); > +/** > + * Return @value rounded down to the nearest power of two or zero. > + */ > +uint64_t pow2floor(uint64_t value); > > #include "qemu/module.h" > > diff --git a/util/cutils.c b/util/cutils.c > index dbe7412..4b8c5be 100644 > --- a/util/cutils.c > +++ b/util/cutils.c > @@ -474,13 +474,12 @@ int qemu_parse_fd(const char *param) > return fd; > } > > -/* round down to the nearest power of 2*/ > -int64_t pow2floor(int64_t value) > +/** > + * Return @value rounded down to the nearest power of two or zero. > + */ > +uint64_t pow2floor(uint64_t value) > { > - if (!is_power_of_2(value)) { > - value = 0x8000000000000000ULL >> clz64(value); > - } > - return value; > + return !value ? 0 : 0x8000000000000000ull >> clz64(value); > } Yes (it's odd that clz returns an int rather than unsigned) Reviewed-by: Dr. David Alan Gilbert Dave > > /* > -- > 1.9.3 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK