It eases code review, unit is explicit.

Patch generated using:

  $ git grep -E '(1024|2048|4096|8192|(<<|>>).?(10|20|30))' hw/ include/hw/

and modified manually.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alan Robinson <Alan.Robinson@ts.fujitsu.com>
---
 hw/block/xen_disk.c        |  5 +++--
 hw/i386/xen/xen-mapcache.c |  3 ++-
 hw/xenpv/xen_domainbuild.c | 13 +++++++------
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 9fbc0cdb87..5e7f17ffa4 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -20,6 +20,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include <sys/ioctl.h>
 #include <sys/uio.h>
 
@@ -812,9 +813,9 @@ static int blk_connect(struct XenDevice *xendev)
     }
 
     xen_pv_printf(xendev, 1, "type \"%s\", fileproto \"%s\", filename \"%s\","
-                  " size %" PRId64 " (%" PRId64 " MB)\n",
+                  " size %" PRId64 " (%llu MB)\n",
                   blkdev->type, blkdev->fileproto, blkdev->filename,
-                  blkdev->file_size, blkdev->file_size >> 20);
+                  blkdev->file_size, blkdev->file_size / MiB);
 
     /* Fill in number of sector size and number of sectors */
     xenstore_write_be_int(xendev, "sector-size", blkdev->file_blk);
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 628b813a11..4e4f069a24 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -9,6 +9,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "qemu/error-report.h"
 
 #include <sys/resource.h>
@@ -46,7 +47,7 @@
  * From empirical tests I observed that qemu use 75MB more than the
  * max_mcache_size.
  */
-#define NON_MCACHE_MEMORY_SIZE (80 * 1024 * 1024)
+#define NON_MCACHE_MEMORY_SIZE (80 * MiB)
 
 typedef struct MapCacheEntry {
     hwaddr paddr_index;
diff --git a/hw/xenpv/xen_domainbuild.c b/hw/xenpv/xen_domainbuild.c
index 027f76fad1..188acaca16 100644
--- a/hw/xenpv/xen_domainbuild.c
+++ b/hw/xenpv/xen_domainbuild.c
@@ -1,4 +1,5 @@
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "hw/xen/xen_backend.h"
 #include "xen_domainbuild.h"
 #include "qemu/timer.h"
@@ -75,9 +76,9 @@ int xenstore_domain_init1(const char *kernel, const char *ramdisk,
     xenstore_write_str(dom, "vm",     vm);
 
     /* memory */
-    xenstore_write_int(dom, "memory/target", ram_size >> 10);  // kB
-    xenstore_write_int(vm, "memory",         ram_size >> 20);  // MB
-    xenstore_write_int(vm, "maxmem",         ram_size >> 20);  // MB
+    xenstore_write_int(dom, "memory/target", ram_size / KiB);
+    xenstore_write_int(vm, "memory",         ram_size / MiB);
+    xenstore_write_int(vm, "maxmem",         ram_size / MiB);
 
     /* cpus */
     for (i = 0; i < smp_cpus; i++) {
@@ -113,7 +114,7 @@ int xenstore_domain_init2(int xenstore_port, int xenstore_mfn,
 
     /* console */
     xenstore_write_str(dom, "console/type",     "ioemu");
-    xenstore_write_int(dom, "console/limit",    128 * 1024);
+    xenstore_write_int(dom, "console/limit",    128 * KiB);
     xenstore_write_int(dom, "console/ring-ref", console_mfn);
     xenstore_write_int(dom, "console/port",     console_port);
     xen_config_dev_console(0);
@@ -260,7 +261,7 @@ int xen_domain_build_pv(const char *kernel, const char *ramdisk,
     }
 #endif
 
-    rc = xc_domain_setmaxmem(xen_xc, xen_domid, ram_size >> 10);
+    rc = xc_domain_setmaxmem(xen_xc, xen_domid, ram_size / KiB);
     if (rc < 0) {
         fprintf(stderr, "xen: xc_domain_setmaxmem() failed\n");
         goto err;
@@ -269,7 +270,7 @@ int xen_domain_build_pv(const char *kernel, const char *ramdisk,
     xenstore_port = xc_evtchn_alloc_unbound(xen_xc, xen_domid, 0);
     console_port = xc_evtchn_alloc_unbound(xen_xc, xen_domid, 0);
 
-    rc = xc_linux_build(xen_xc, xen_domid, ram_size >> 20,
+    rc = xc_linux_build(xen_xc, xen_domid, ram_size / MiB,
                         kernel, ramdisk, cmdline,
                         0, flags,
                         xenstore_port, &xenstore_mfn,
-- 
2.17.1

On 06/10/2018 03:14 PM, Philippe Mathieu-Daudé wrote:
>      xen_pv_printf(xendev, 1, "type \"%s\", fileproto \"%s\", filename \"%s\","
> -                  " size %" PRId64 " (%" PRId64 " MB)\n",
> +                  " size %" PRId64 " (%llu MB)\n",
>                    blkdev->type, blkdev->fileproto, blkdev->filename,
> -                  blkdev->file_size, blkdev->file_size >> 20);
> +                  blkdev->file_size, blkdev->file_size / MiB);

Having to change printf markup is exactly why you shouldn't use ULL in MiB.


r~

On 06/12/2018 03:51 PM, Richard Henderson wrote:
> On 06/10/2018 03:14 PM, Philippe Mathieu-Daudé wrote:
>>       xen_pv_printf(xendev, 1, "type \"%s\", fileproto \"%s\", filename \"%s\","
>> -                  " size %" PRId64 " (%" PRId64 " MB)\n",
>> +                  " size %" PRId64 " (%llu MB)\n",
>>                     blkdev->type, blkdev->fileproto, blkdev->filename,
>> -                  blkdev->file_size, blkdev->file_size >> 20);
>> +                  blkdev->file_size, blkdev->file_size / MiB);
> 
> Having to change printf markup is exactly why you shouldn't use ULL in MiB.

Conversely, M_BYTE was already ULL, so if you don't use it in MiB, 
you'll have to change other printf markup where you were changing those 
uses.

One benefit of using the widest possible type: we avoid risk of silent 
truncation.  Potential downsides: wasted processing time (when 32 bits 
was sufficient), and compilers might start warning when we narrow a 
64-bit value into a 32-bit variable (but I think we already ignore that).

One benefit of using the natural type that holds the value: use of 
64-bit math is explicit based on the type of what else is being 
multiplied by the macro.  Potential downside: 32*32 assigned to a 64-bit 
result may be botched (but hopefully Coverity will flag it).

So there's tradeoffs either way, and you at least need to document in 
your commit messages what auditing you have done that any type changes 
introduced by your changes are safe.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 06/12/2018 11:04 AM, Eric Blake wrote:
> On 06/12/2018 03:51 PM, Richard Henderson wrote:
>> On 06/10/2018 03:14 PM, Philippe Mathieu-Daudé wrote:
>>>       xen_pv_printf(xendev, 1, "type \"%s\", fileproto \"%s\", filename
>>> \"%s\","
>>> -                  " size %" PRId64 " (%" PRId64 " MB)\n",
>>> +                  " size %" PRId64 " (%llu MB)\n",
>>>                     blkdev->type, blkdev->fileproto, blkdev->filename,
>>> -                  blkdev->file_size, blkdev->file_size >> 20);
>>> +                  blkdev->file_size, blkdev->file_size / MiB);
>>
>> Having to change printf markup is exactly why you shouldn't use ULL in MiB.
> 
> Conversely, M_BYTE was already ULL, so if you don't use it in MiB, you'll have
> to change other printf markup where you were changing those uses.
> 
> One benefit of using the widest possible type: we avoid risk of silent
> truncation.  Potential downsides: wasted processing time (when 32 bits was
> sufficient), and compilers might start warning when we narrow a 64-bit value
> into a 32-bit variable (but I think we already ignore that).
> 
> One benefit of using the natural type that holds the value: use of 64-bit math
> is explicit based on the type of what else is being multiplied by the macro. 
> Potential downside: 32*32 assigned to a 64-bit result may be botched (but
> hopefully Coverity will flag it).
> 
> So there's tradeoffs either way, and you at least need to document in your
> commit messages what auditing you have done that any type changes introduced by
> your changes are safe.

I'm more concerned about unnecessary or unintended signed vs unsigned changes
than I am about width.  But if we're going to force a 64-bit type, use
(int64_t)1 not 1LL.  That way the type will match the existing PRId64 printf
markup.


r~

On 06/12/2018 04:10 PM, Richard Henderson wrote:
>> So there's tradeoffs either way, and you at least need to document in your
>> commit messages what auditing you have done that any type changes introduced by
>> your changes are safe.
> 
> I'm more concerned about unnecessary or unintended signed vs unsigned changes
> than I am about width.  But if we're going to force a 64-bit type, use
> (int64_t)1 not 1LL.  That way the type will match the existing PRId64 printf
> markup.

Or spell it UINT64_C(1) if you don't want a cast.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 06/13/2018 02:13 AM, Eric Blake wrote:
> Or spell it UINT64_C(1) if you don't want a cast.

Not unsigned is what I want most.


r~

On 06/13/2018 04:31 PM, Richard Henderson wrote:
> On 06/13/2018 02:13 AM, Eric Blake wrote:
>> Or spell it UINT64_C(1) if you don't want a cast.
> 
> Not unsigned is what I want most.

I used both of your suggestions, but now new format string errors
appeared due to ram_addr_t being unsigned, so code cleaned using
MachineState->ram_size now complains.

include/exec/cpu-common.h:53:typedef uintptr_t ram_addr_t;
include/hw/boards.h:259:    ram_addr_t ram_size;

Is the following snippet OK?

     /* allocate RAM */
-    if (ram_size > (2048u << 20)) {
-        error_report("Too much memory for this machine: %dMB, maximum
2048MB",
-                     ((unsigned int)ram_size / (1 << 20)));
+    if (ram_size > 2 * GiB) {
+        error_report("Too much memory for this machine: %luMB, maximum
2048MB",
+                     ram_size / MiB);
         exit(1);
     }