[Qemu-devel] [PATCH 1/2] cutils: Proactively fix pow2floor(), switch to unsigned

[Markus Armbruster]

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 <address@hidden>
---
 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);
 }
 
 /*
-- 
1.9.3