[Qemu-devel] [PATCH v6 0/3] ramblock: add hmp command "info ramblock"

[Qemu-devel] [PATCH v6 0/3] ramblock: add hmp command "info ramblock"

[Peter Xu]

v6
- patch 2: instead of create a new size_to_str(), abstract the logic
  out from print_type_size(), refactor it, to make sure
  print_type_size() dumps exactly the same thing as before. (a simple
  test with info qtree is done)
- let suffixes be an array of strings [Markus]

v5
- add r-b for Dave on first patch (which I forgot in v4, so I got it
  again)
- add one more patch to introduce size_to_str() as patch 2 [Dave]
- let the last patch use the new interface

v4:
- move page_size_to_str() into util/cutil.c [Dave]

v3:
- cast the three PRIx64 addresses using (uint64_t) [Fam]
- add more comment in patch 2 to emphasize that this command is only
  suitable for HMP, not QMP [Markus]

v2:
- replace "lx" with "PRIx64" in three places

Sometimes I would like to know ramblock info for a VM. This command
would help. It provides a way to dump ramblock info. Currently the
list is by default sorted by size, though I think it's good enough.

Please review, thanks.

Peter Xu (3):
  ramblock: add RAMBLOCK_FOREACH()
  utils: provide size_to_str()
  ramblock: add new hmp command "info ramblock"

 exec.c                       | 44 +++++++++++++++++++++++++++++++++-----------
 hmp-commands-info.hx         | 14 ++++++++++++++
 hmp.c                        |  6 ++++++
 hmp.h                        |  1 +
 include/exec/ramlist.h       |  6 ++++++
 include/qemu-common.h        |  1 +
 migration/ram.c              | 15 ++++++++-------
 qapi/string-output-visitor.c | 22 ++++++----------------
 util/cutils.c                | 23 +++++++++++++++++++++++
 9 files changed, 98 insertions(+), 34 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v6 1/3] ramblock: add RAMBLOCK_FOREACH()

[Peter Xu]

So that it can simplifies the iterators.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 exec.c                 | 22 +++++++++++-----------
 include/exec/ramlist.h |  5 +++++
 migration/ram.c        | 15 ++++++++-------
 3 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/exec.c b/exec.c
index eac6085..50519ae 100644
--- a/exec.c
+++ b/exec.c
@@ -978,7 +978,7 @@ static RAMBlock *qemu_get_ram_block(ram_addr_t addr)
     if (block && addr - block->offset < block->max_length) {
         return block;
     }
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         if (addr - block->offset < block->max_length) {
             goto found;
         }
@@ -1578,12 +1578,12 @@ static ram_addr_t find_ram_offset(ram_addr_t size)
         return 0;
     }
 
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         ram_addr_t end, next = RAM_ADDR_MAX;
 
         end = block->offset + block->max_length;
 
-        QLIST_FOREACH_RCU(next_block, &ram_list.blocks, next) {
+        RAMBLOCK_FOREACH(next_block) {
             if (next_block->offset >= end) {
                 next = MIN(next, next_block->offset);
             }
@@ -1609,7 +1609,7 @@ unsigned long last_ram_page(void)
     ram_addr_t last = 0;
 
     rcu_read_lock();
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         last = MAX(last, block->offset + block->max_length);
     }
     rcu_read_unlock();
@@ -1659,7 +1659,7 @@ void qemu_ram_set_idstr(RAMBlock *new_block, const char *name, DeviceState *dev)
     pstrcat(new_block->idstr, sizeof(new_block->idstr), name);
 
     rcu_read_lock();
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         if (block != new_block &&
             !strcmp(block->idstr, new_block->idstr)) {
             fprintf(stderr, "RAMBlock \"%s\" already registered, abort!\n",
@@ -1693,7 +1693,7 @@ size_t qemu_ram_pagesize_largest(void)
     RAMBlock *block;
     size_t largest = 0;
 
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         largest = MAX(largest, qemu_ram_pagesize(block));
     }
 
@@ -1839,7 +1839,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
      * QLIST (which has an RCU-friendly variant) does not have insertion at
      * tail, so save the last element in last_block.
      */
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         last_block = block;
         if (block->max_length < new_block->max_length) {
             break;
@@ -2021,7 +2021,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
     int flags;
     void *area, *vaddr;
 
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         offset = addr - block->offset;
         if (offset < block->max_length) {
             vaddr = ramblock_ptr(block, offset);
@@ -2167,7 +2167,7 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
         goto found;
     }
 
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         /* This case append when the block is not mapped. */
         if (block->host == NULL) {
             continue;
@@ -2200,7 +2200,7 @@ RAMBlock *qemu_ram_block_by_name(const char *name)
 {
     RAMBlock *block;
 
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         if (!strcmp(name, block->idstr)) {
             return block;
         }
@@ -3424,7 +3424,7 @@ int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque)
     int ret = 0;
 
     rcu_read_lock();
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         ret = func(block->idstr, block->host, block->offset,
                    block->used_length, opaque);
         if (ret) {
diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
index c59880d..f1c6b45 100644
--- a/include/exec/ramlist.h
+++ b/include/exec/ramlist.h
@@ -4,6 +4,7 @@
 #include "qemu/queue.h"
 #include "qemu/thread.h"
 #include "qemu/rcu.h"
+#include "qemu/rcu_queue.h"
 
 typedef struct RAMBlockNotifier RAMBlockNotifier;
 
@@ -54,6 +55,10 @@ typedef struct RAMList {
 } RAMList;
 extern RAMList ram_list;
 
+/* Should be holding either ram_list.mutex, or the RCU lock. */
+#define  RAMBLOCK_FOREACH(block)  \
+    QLIST_FOREACH_RCU(block, &ram_list.blocks, next)
+
 void qemu_mutex_lock_ramlist(void);
 void qemu_mutex_unlock_ramlist(void);
 
diff --git a/migration/ram.c b/migration/ram.c
index f48664e..7ba5d7e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -672,7 +672,7 @@ uint64_t ram_pagesize_summary(void)
     RAMBlock *block;
     uint64_t summary = 0;
 
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         summary |= block->page_size;
     }
 
@@ -700,7 +700,7 @@ static void migration_bitmap_sync(RAMState *rs)
 
     qemu_mutex_lock(&rs->bitmap_mutex);
     rcu_read_lock();
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         migration_bitmap_sync_range(rs, block, 0, block->used_length);
     }
     rcu_read_unlock();
@@ -1439,8 +1439,9 @@ uint64_t ram_bytes_total(void)
     uint64_t total = 0;
 
     rcu_read_lock();
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next)
+    RAMBLOCK_FOREACH(block) {
         total += block->used_length;
+    }
     rcu_read_unlock();
     return total;
 }
@@ -1543,7 +1544,7 @@ void ram_postcopy_migrated_memory_release(MigrationState *ms)
     struct RAMBlock *block;
     unsigned long *bitmap = atomic_rcu_read(&rs->ram_bitmap)->bmap;
 
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         unsigned long first = block->offset >> TARGET_PAGE_BITS;
         unsigned long range = first + (block->used_length >> TARGET_PAGE_BITS);
         unsigned long run_start = find_next_zero_bit(bitmap, range, first);
@@ -1624,7 +1625,7 @@ static int postcopy_each_ram_send_discard(MigrationState *ms)
     struct RAMBlock *block;
     int ret;
 
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         unsigned long first = block->offset >> TARGET_PAGE_BITS;
         PostcopyDiscardState *pds = postcopy_discard_send_init(ms,
                                                                first,
@@ -1802,7 +1803,7 @@ static int postcopy_chunk_hostpages(MigrationState *ms)
     rs->last_sent_block = NULL;
     rs->last_page = 0;
 
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         unsigned long first = block->offset >> TARGET_PAGE_BITS;
 
         PostcopyDiscardState *pds =
@@ -2021,7 +2022,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
 
     qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
 
-    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+    RAMBLOCK_FOREACH(block) {
         qemu_put_byte(f, strlen(block->idstr));
         qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr));
         qemu_put_be64(f, block->used_length);
-- 
2.7.4

[Qemu-devel] [PATCH v6 2/3] utils: provide size_to_str()

[Peter Xu]

Moving the algorithm from print_type_size() into size_to_str() so that
other component can also leverage it. With that, refactor
print_type_size().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qemu-common.h        |  1 +
 qapi/string-output-visitor.c | 22 ++++++----------------
 util/cutils.c                | 23 +++++++++++++++++++++++
 3 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index d218821..d7d0448 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -145,6 +145,7 @@ void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size);
 int parse_debug_env(const char *name, int max, int initial);
 
 const char *qemu_ether_ntoa(const MACAddr *mac);
+char *size_to_str(double val);
 void page_size_init(void);
 
 /* returns non-zero if dump is in progress, otherwise zero is
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index 94ac821..53c2175 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -211,10 +211,8 @@ static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
                             Error **errp)
 {
     StringOutputVisitor *sov = to_sov(v);
-    static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T', 'P', 'E' };
-    uint64_t div, val;
-    char *out;
-    int i;
+    uint64_t val;
+    char *out, *psize;
 
     if (!sov->human) {
         out = g_strdup_printf("%"PRIu64, *obj);
@@ -223,19 +221,11 @@ static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
     }
 
     val = *obj;
-
-    /* The exponent (returned in i) minus one gives us
-     * floor(log2(val * 1024 / 1000).  The correction makes us
-     * switch to the higher power when the integer part is >= 1000.
-     */
-    frexp(val / (1000.0 / 1024.0), &i);
-    i = (i - 1) / 10;
-    assert(i < ARRAY_SIZE(suffixes));
-    div = 1ULL << (i * 10);
-
-    out = g_strdup_printf("%"PRIu64" (%0.3g %c%s)", val,
-                          (double)val/div, suffixes[i], i ? "iB" : "");
+    psize = size_to_str(val);
+    out = g_strdup_printf("%"PRIu64" (%s)", val, psize);
     string_output_set(sov, out);
+
+    g_free(psize);
 }
 
 static void print_type_bool(Visitor *v, const char *name, bool *obj,
diff --git a/util/cutils.c b/util/cutils.c
index 50ad179..c562d87 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -619,3 +619,26 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
 
     return ret;
 }
+
+/*
+ * Please free the buffer after use.
+ */
+char *size_to_str(double val)
+{
+    static const char *suffixes[] = { "", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei" };
+    unsigned long div;
+    int i;
+
+    /*
+     * The exponent (returned in i) minus one gives us
+     * floor(log2(val * 1024 / 1000).  The correction makes us
+     * switch to the higher power when the integer part is >= 1000.
+     * (see e41b509d68afb1f for more info)
+     */
+    frexp(val / (1000.0 / 1024.0), &i);
+    i = (i - 1) / 10;
+    assert(i < ARRAY_SIZE(suffixes));
+    div = 1ULL << (i * 10);
+
+    return g_strdup_printf("%0.3g %sB", val / div, suffixes[i]);
+}
-- 
2.7.4

[Qemu-devel] [PATCH v6 3/3] ramblock: add new hmp command "info ramblock"

[Peter Xu]

To dump information about ramblocks. It looks like:

(qemu) info ramblock
              Block Name    PSize              Offset               Used              Total
            /objects/mem    2 MiB  0x0000000000000000 0x0000000080000000 0x0000000080000000
                vga.vram    4 KiB  0x0000000080060000 0x0000000001000000 0x0000000001000000
    /rom@etc/acpi/tables    4 KiB  0x00000000810b0000 0x0000000000020000 0x0000000000200000
                 pc.bios    4 KiB  0x0000000080000000 0x0000000000040000 0x0000000000040000
  0000:00:03.0/e1000.rom    4 KiB  0x0000000081070000 0x0000000000040000 0x0000000000040000
                  pc.rom    4 KiB  0x0000000080040000 0x0000000000020000 0x0000000000020000
    0000:00:02.0/vga.rom    4 KiB  0x0000000081060000 0x0000000000010000 0x0000000000010000
   /rom@etc/table-loader    4 KiB  0x00000000812b0000 0x0000000000001000 0x0000000000001000
      /rom@etc/acpi/rsdp    4 KiB  0x00000000812b1000 0x0000000000001000 0x0000000000001000

Ramblock is something hidden internally in QEMU implementation, and this
command should only be used by mostly QEMU developers on RAM stuff. It
is not a command suitable for QMP interface. So only HMP interface is
provided for it.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 exec.c                 | 22 ++++++++++++++++++++++
 hmp-commands-info.hx   | 14 ++++++++++++++
 hmp.c                  |  6 ++++++
 hmp.h                  |  1 +
 include/exec/ramlist.h |  1 +
 5 files changed, 44 insertions(+)

diff --git a/exec.c b/exec.c
index 50519ae..821bef3 100644
--- a/exec.c
+++ b/exec.c
@@ -71,6 +71,8 @@
 #include "qemu/mmap-alloc.h"
 #endif
 
+#include "monitor/monitor.h"
+
 //#define DEBUG_SUBPAGE
 
 #if !defined(CONFIG_USER_ONLY)
@@ -1333,6 +1335,26 @@ void qemu_mutex_unlock_ramlist(void)
     qemu_mutex_unlock(&ram_list.mutex);
 }
 
+void ram_block_dump(Monitor *mon)
+{
+    RAMBlock *block;
+    char *psize;
+
+    rcu_read_lock();
+    monitor_printf(mon, "%24s %8s  %18s %18s %18s\n",
+                   "Block Name", "PSize", "Offset", "Used", "Total");
+    RAMBLOCK_FOREACH(block) {
+        psize = size_to_str(block->page_size);
+        monitor_printf(mon, "%24s %8s  0x%016" PRIx64 " 0x%016" PRIx64
+                       " 0x%016" PRIx64 "\n", block->idstr, psize,
+                       (uint64_t)block->offset,
+                       (uint64_t)block->used_length,
+                       (uint64_t)block->max_length);
+        g_free(psize);
+    }
+    rcu_read_unlock();
+}
+
 #ifdef __linux__
 /*
  * FIXME TOCTTOU: this iterates over memory backends' mem-path, which
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index a53f105..ae16901 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -788,6 +788,20 @@ Display the latest dump status.
 ETEXI
 
     {
+        .name       = "ramblock",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Display system ramblock information",
+        .cmd        = hmp_info_ramblock,
+    },
+
+STEXI
+@item info ramblock
+@findex ramblock
+Dump all the ramblocks of the system.
+ETEXI
+
+    {
         .name       = "hotpluggable-cpus",
         .args_type  = "",
         .params     = "",
diff --git a/hmp.c b/hmp.c
index ab407d6..8369388 100644
--- a/hmp.c
+++ b/hmp.c
@@ -37,6 +37,7 @@
 #include "qemu-io.h"
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
+#include "exec/ramlist.h"
 #include "hw/intc/intc.h"
 
 #ifdef CONFIG_SPICE
@@ -2563,6 +2564,11 @@ void hmp_info_dump(Monitor *mon, const QDict *qdict)
     qapi_free_DumpQueryResult(result);
 }
 
+void hmp_info_ramblock(Monitor *mon, const QDict *qdict)
+{
+    ram_block_dump(mon);
+}
+
 void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
diff --git a/hmp.h b/hmp.h
index 799fd37..7353b67 100644
--- a/hmp.h
+++ b/hmp.h
@@ -136,6 +136,7 @@ void hmp_rocker_ports(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
 void hmp_info_dump(Monitor *mon, const QDict *qdict);
+void hmp_info_ramblock(Monitor *mon, const QDict *qdict);
 void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
 void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict);
 
diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
index f1c6b45..2e2ac6c 100644
--- a/include/exec/ramlist.h
+++ b/include/exec/ramlist.h
@@ -73,5 +73,6 @@ void ram_block_notifier_remove(RAMBlockNotifier *n);
 void ram_block_notify_add(void *host, size_t size);
 void ram_block_notify_remove(void *host, size_t size);
 
+void ram_block_dump(Monitor *mon);
 
 #endif /* RAMLIST_H */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 3/3] ramblock: add new hmp command "info ramblock"

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> To dump information about ramblocks. It looks like:
> 
> (qemu) info ramblock
>               Block Name    PSize              Offset               Used              Total
>             /objects/mem    2 MiB  0x0000000000000000 0x0000000080000000 0x0000000080000000
>                 vga.vram    4 KiB  0x0000000080060000 0x0000000001000000 0x0000000001000000
>     /rom@etc/acpi/tables    4 KiB  0x00000000810b0000 0x0000000000020000 0x0000000000200000
>                  pc.bios    4 KiB  0x0000000080000000 0x0000000000040000 0x0000000000040000
>   0000:00:03.0/e1000.rom    4 KiB  0x0000000081070000 0x0000000000040000 0x0000000000040000
>                   pc.rom    4 KiB  0x0000000080040000 0x0000000000020000 0x0000000000020000
>     0000:00:02.0/vga.rom    4 KiB  0x0000000081060000 0x0000000000010000 0x0000000000010000
>    /rom@etc/table-loader    4 KiB  0x00000000812b0000 0x0000000000001000 0x0000000000001000
>       /rom@etc/acpi/rsdp    4 KiB  0x00000000812b1000 0x0000000000001000 0x0000000000001000
> 
> Ramblock is something hidden internally in QEMU implementation, and this
> command should only be used by mostly QEMU developers on RAM stuff. It
> is not a command suitable for QMP interface. So only HMP interface is
> provided for it.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  exec.c                 | 22 ++++++++++++++++++++++
>  hmp-commands-info.hx   | 14 ++++++++++++++
>  hmp.c                  |  6 ++++++
>  hmp.h                  |  1 +
>  include/exec/ramlist.h |  1 +
>  5 files changed, 44 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index 50519ae..821bef3 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -71,6 +71,8 @@
>  #include "qemu/mmap-alloc.h"
>  #endif
>  
> +#include "monitor/monitor.h"
> +
>  //#define DEBUG_SUBPAGE
>  
>  #if !defined(CONFIG_USER_ONLY)
> @@ -1333,6 +1335,26 @@ void qemu_mutex_unlock_ramlist(void)
>      qemu_mutex_unlock(&ram_list.mutex);
>  }
>  
> +void ram_block_dump(Monitor *mon)
> +{
> +    RAMBlock *block;
> +    char *psize;
> +
> +    rcu_read_lock();
> +    monitor_printf(mon, "%24s %8s  %18s %18s %18s\n",
> +                   "Block Name", "PSize", "Offset", "Used", "Total");
> +    RAMBLOCK_FOREACH(block) {
> +        psize = size_to_str(block->page_size);
> +        monitor_printf(mon, "%24s %8s  0x%016" PRIx64 " 0x%016" PRIx64
> +                       " 0x%016" PRIx64 "\n", block->idstr, psize,
> +                       (uint64_t)block->offset,
> +                       (uint64_t)block->used_length,
> +                       (uint64_t)block->max_length);
> +        g_free(psize);
> +    }
> +    rcu_read_unlock();
> +}
> +
>  #ifdef __linux__
>  /*
>   * FIXME TOCTTOU: this iterates over memory backends' mem-path, which
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index a53f105..ae16901 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -788,6 +788,20 @@ Display the latest dump status.
>  ETEXI
>  
>      {
> +        .name       = "ramblock",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "Display system ramblock information",
> +        .cmd        = hmp_info_ramblock,
> +    },
> +
> +STEXI
> +@item info ramblock
> +@findex ramblock
> +Dump all the ramblocks of the system.
> +ETEXI
> +
> +    {
>          .name       = "hotpluggable-cpus",
>          .args_type  = "",
>          .params     = "",
> diff --git a/hmp.c b/hmp.c
> index ab407d6..8369388 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -37,6 +37,7 @@
>  #include "qemu-io.h"
>  #include "qemu/cutils.h"
>  #include "qemu/error-report.h"
> +#include "exec/ramlist.h"
>  #include "hw/intc/intc.h"
>  
>  #ifdef CONFIG_SPICE
> @@ -2563,6 +2564,11 @@ void hmp_info_dump(Monitor *mon, const QDict *qdict)
>      qapi_free_DumpQueryResult(result);
>  }
>  
> +void hmp_info_ramblock(Monitor *mon, const QDict *qdict)
> +{
> +    ram_block_dump(mon);
> +}
> +
>  void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> diff --git a/hmp.h b/hmp.h
> index 799fd37..7353b67 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -136,6 +136,7 @@ void hmp_rocker_ports(Monitor *mon, const QDict *qdict);
>  void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
>  void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
>  void hmp_info_dump(Monitor *mon, const QDict *qdict);
> +void hmp_info_ramblock(Monitor *mon, const QDict *qdict);
>  void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
>  void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict);
>  
> diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
> index f1c6b45..2e2ac6c 100644
> --- a/include/exec/ramlist.h
> +++ b/include/exec/ramlist.h
> @@ -73,5 +73,6 @@ void ram_block_notifier_remove(RAMBlockNotifier *n);
>  void ram_block_notify_add(void *host, size_t size);
>  void ram_block_notify_remove(void *host, size_t size);
>  
> +void ram_block_dump(Monitor *mon);
>  
>  #endif /* RAMLIST_H */
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 2/3] utils: provide size_to_str()

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> Moving the algorithm from print_type_size() into size_to_str() so that
> other component can also leverage it. With that, refactor
> print_type_size().
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qemu-common.h        |  1 +
>  qapi/string-output-visitor.c | 22 ++++++----------------
>  util/cutils.c                | 23 +++++++++++++++++++++++
>  3 files changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index d218821..d7d0448 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -145,6 +145,7 @@ void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size);
>  int parse_debug_env(const char *name, int max, int initial);
>  
>  const char *qemu_ether_ntoa(const MACAddr *mac);
> +char *size_to_str(double val);
>  void page_size_init(void);
>  
>  /* returns non-zero if dump is in progress, otherwise zero is
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index 94ac821..53c2175 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -211,10 +211,8 @@ static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
>                              Error **errp)
>  {
>      StringOutputVisitor *sov = to_sov(v);
> -    static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T', 'P', 'E' };
> -    uint64_t div, val;
> -    char *out;
> -    int i;
> +    uint64_t val;
> +    char *out, *psize;
>  
>      if (!sov->human) {
>          out = g_strdup_printf("%"PRIu64, *obj);
> @@ -223,19 +221,11 @@ static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
>      }
>  
>      val = *obj;
> -
> -    /* The exponent (returned in i) minus one gives us
> -     * floor(log2(val * 1024 / 1000).  The correction makes us
> -     * switch to the higher power when the integer part is >= 1000.
> -     */
> -    frexp(val / (1000.0 / 1024.0), &i);
> -    i = (i - 1) / 10;
> -    assert(i < ARRAY_SIZE(suffixes));
> -    div = 1ULL << (i * 10);
> -
> -    out = g_strdup_printf("%"PRIu64" (%0.3g %c%s)", val,
> -                          (double)val/div, suffixes[i], i ? "iB" : "");
> +    psize = size_to_str(val);
> +    out = g_strdup_printf("%"PRIu64" (%s)", val, psize);
>      string_output_set(sov, out);
> +
> +    g_free(psize);
>  }
>  
>  static void print_type_bool(Visitor *v, const char *name, bool *obj,
> diff --git a/util/cutils.c b/util/cutils.c
> index 50ad179..c562d87 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -619,3 +619,26 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
>  
>      return ret;
>  }
> +
> +/*
> + * Please free the buffer after use.
> + */
> +char *size_to_str(double val)
> +{
> +    static const char *suffixes[] = { "", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei" };
> +    unsigned long div;
> +    int i;
> +
> +    /*
> +     * The exponent (returned in i) minus one gives us
> +     * floor(log2(val * 1024 / 1000).  The correction makes us
> +     * switch to the higher power when the integer part is >= 1000.
> +     * (see e41b509d68afb1f for more info)
> +     */
> +    frexp(val / (1000.0 / 1024.0), &i);
> +    i = (i - 1) / 10;
> +    assert(i < ARRAY_SIZE(suffixes));
> +    div = 1ULL << (i * 10);
> +
> +    return g_strdup_printf("%0.3g %sB", val / div, suffixes[i]);
> +}

Right, I think I'm happy with that;  Paolo, Markus - make sense to you?

Dave

> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 2/3] utils: provide size_to_str()

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Moving the algorithm from print_type_size() into size_to_str() so that
> other component can also leverage it. With that, refactor
> print_type_size().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qemu-common.h        |  1 +
>  qapi/string-output-visitor.c | 22 ++++++----------------
>  util/cutils.c                | 23 +++++++++++++++++++++++
>  3 files changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index d218821..d7d0448 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -145,6 +145,7 @@ void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size);
>  int parse_debug_env(const char *name, int max, int initial);
>  
>  const char *qemu_ether_ntoa(const MACAddr *mac);
> +char *size_to_str(double val);
>  void page_size_init(void);
>  
>  /* returns non-zero if dump is in progress, otherwise zero is
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index 94ac821..53c2175 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -211,10 +211,8 @@ static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
>                              Error **errp)
>  {
>      StringOutputVisitor *sov = to_sov(v);
> -    static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T', 'P', 'E' };
> -    uint64_t div, val;
> -    char *out;
> -    int i;
> +    uint64_t val;
> +    char *out, *psize;
>  
>      if (!sov->human) {
>          out = g_strdup_printf("%"PRIu64, *obj);
> @@ -223,19 +221,11 @@ static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
>      }
>  
>      val = *obj;
> -
> -    /* The exponent (returned in i) minus one gives us
> -     * floor(log2(val * 1024 / 1000).  The correction makes us
> -     * switch to the higher power when the integer part is >= 1000.
> -     */
> -    frexp(val / (1000.0 / 1024.0), &i);
> -    i = (i - 1) / 10;
> -    assert(i < ARRAY_SIZE(suffixes));
> -    div = 1ULL << (i * 10);
> -
> -    out = g_strdup_printf("%"PRIu64" (%0.3g %c%s)", val,
> -                          (double)val/div, suffixes[i], i ? "iB" : "");
> +    psize = size_to_str(val);
> +    out = g_strdup_printf("%"PRIu64" (%s)", val, psize);
>      string_output_set(sov, out);
> +
> +    g_free(psize);
>  }
>  
>  static void print_type_bool(Visitor *v, const char *name, bool *obj,
> diff --git a/util/cutils.c b/util/cutils.c
> index 50ad179..c562d87 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -619,3 +619,26 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
>  
>      return ret;
>  }
> +
> +/*
> + * Please free the buffer after use.
> + */

A proper function comment would be nice.  Something like

/*
 * Return human readable string for size @val.
 * @val must be between 0 inclusive and 1000EiB exclusive.
 * Use IEC binary units like KiB, MiB, and so forth
 * Caller is responsible for passing it to g_free().
 */

> +char *size_to_str(double val)
> +{
> +    static const char *suffixes[] = { "", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei" };
> +    unsigned long div;
> +    int i;
> +
> +    /*
> +     * The exponent (returned in i) minus one gives us
> +     * floor(log2(val * 1024 / 1000).  The correction makes us
> +     * switch to the higher power when the integer part is >= 1000.
> +     * (see e41b509d68afb1f for more info)
> +     */
> +    frexp(val / (1000.0 / 1024.0), &i);
> +    i = (i - 1) / 10;
> +    assert(i < ARRAY_SIZE(suffixes));
> +    div = 1ULL << (i * 10);
> +
> +    return g_strdup_printf("%0.3g %sB", val / div, suffixes[i]);
> +}

The function happily accepts negative sizes.  You can reject them with
assert(), adapt my function comment accordingly, or do nothing.

The function asserts on very large sizes.  I'm fine with that, and
worded my function comment accordingly.  If you want to avoid the
constraint, format them in the biggest unit, e.g. 3.1415 * (1ull<70) as
"3142 EiB", in followup patch.

The function formats 999.9 * (1ull<<60) as "1e+03 EiB".  Oops.  No big
deal, but you might want to fix it anyway, possibly in a followup patch.

Preferably with a function comment:

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v6 2/3] utils: provide size_to_str()

[Peter Xu]

On Wed, May 10, 2017 at 01:34:18PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Moving the algorithm from print_type_size() into size_to_str() so that
> > other component can also leverage it. With that, refactor
> > print_type_size().
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/qemu-common.h        |  1 +
> >  qapi/string-output-visitor.c | 22 ++++++----------------
> >  util/cutils.c                | 23 +++++++++++++++++++++++
> >  3 files changed, 30 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/qemu-common.h b/include/qemu-common.h
> > index d218821..d7d0448 100644
> > --- a/include/qemu-common.h
> > +++ b/include/qemu-common.h
> > @@ -145,6 +145,7 @@ void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size);
> >  int parse_debug_env(const char *name, int max, int initial);
> >  
> >  const char *qemu_ether_ntoa(const MACAddr *mac);
> > +char *size_to_str(double val);
> >  void page_size_init(void);
> >  
> >  /* returns non-zero if dump is in progress, otherwise zero is
> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> > index 94ac821..53c2175 100644
> > --- a/qapi/string-output-visitor.c
> > +++ b/qapi/string-output-visitor.c
> > @@ -211,10 +211,8 @@ static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
> >                              Error **errp)
> >  {
> >      StringOutputVisitor *sov = to_sov(v);
> > -    static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T', 'P', 'E' };
> > -    uint64_t div, val;
> > -    char *out;
> > -    int i;
> > +    uint64_t val;
> > +    char *out, *psize;
> >  
> >      if (!sov->human) {
> >          out = g_strdup_printf("%"PRIu64, *obj);
> > @@ -223,19 +221,11 @@ static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
> >      }
> >  
> >      val = *obj;
> > -
> > -    /* The exponent (returned in i) minus one gives us
> > -     * floor(log2(val * 1024 / 1000).  The correction makes us
> > -     * switch to the higher power when the integer part is >= 1000.
> > -     */
> > -    frexp(val / (1000.0 / 1024.0), &i);
> > -    i = (i - 1) / 10;
> > -    assert(i < ARRAY_SIZE(suffixes));
> > -    div = 1ULL << (i * 10);
> > -
> > -    out = g_strdup_printf("%"PRIu64" (%0.3g %c%s)", val,
> > -                          (double)val/div, suffixes[i], i ? "iB" : "");
> > +    psize = size_to_str(val);
> > +    out = g_strdup_printf("%"PRIu64" (%s)", val, psize);
> >      string_output_set(sov, out);
> > +
> > +    g_free(psize);
> >  }
> >  
> >  static void print_type_bool(Visitor *v, const char *name, bool *obj,
> > diff --git a/util/cutils.c b/util/cutils.c
> > index 50ad179..c562d87 100644
> > --- a/util/cutils.c
> > +++ b/util/cutils.c
> > @@ -619,3 +619,26 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
> >  
> >      return ret;
> >  }
> > +
> > +/*
> > + * Please free the buffer after use.
> > + */
> 
> A proper function comment would be nice.  Something like
> 
> /*
>  * Return human readable string for size @val.
>  * @val must be between 0 inclusive and 1000EiB exclusive.
>  * Use IEC binary units like KiB, MiB, and so forth
>  * Caller is responsible for passing it to g_free().
>  */

Yes this looks much better. :)

> 
> > +char *size_to_str(double val)
> > +{
> > +    static const char *suffixes[] = { "", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei" };
> > +    unsigned long div;
> > +    int i;
> > +
> > +    /*
> > +     * The exponent (returned in i) minus one gives us
> > +     * floor(log2(val * 1024 / 1000).  The correction makes us
> > +     * switch to the higher power when the integer part is >= 1000.
> > +     * (see e41b509d68afb1f for more info)
> > +     */
> > +    frexp(val / (1000.0 / 1024.0), &i);
> > +    i = (i - 1) / 10;
> > +    assert(i < ARRAY_SIZE(suffixes));
> > +    div = 1ULL << (i * 10);
> > +
> > +    return g_strdup_printf("%0.3g %sB", val / div, suffixes[i]);
> > +}
> 
> The function happily accepts negative sizes.  You can reject them with
> assert(), adapt my function comment accordingly, or do nothing.
> 
> The function asserts on very large sizes.  I'm fine with that, and
> worded my function comment accordingly.  If you want to avoid the
> constraint, format them in the biggest unit, e.g. 3.1415 * (1ull<70) as
> "3142 EiB", in followup patch.

For above two points, actually I would prefer we don't have assertion
in this function. The reason is when this helper is used in dynamic
environment (e.g., converting user input into size strings), it can
crash the VM when assertion is triggered. To avoid that, I would like
val to be any value (very big, or negative). For very big one, I'd
like to take your suggestion to have a limit on maximum unit; for
negative values, let's just do nothing, since frexp() supports that,
and finally we'll just see something like "-20 MiB" and imho that's
better than a crash.

> 
> The function formats 999.9 * (1ull<<60) as "1e+03 EiB".  Oops.  No big
> deal, but you might want to fix it anyway, possibly in a followup patch.

I didn't really find a good way to solve this, considering that it is
related to how "%g" is implemented in g_strdup_printf() (or say, it
decided to dump "1e+03" rather than "999.9" for some reason... and I
found that this condition goes away if we specify something <999.5).
So let me just keep it as it is.

This also helps to make sure we won't dump different strings after
this patch when used by print_type_size().

> 
> Preferably with a function comment:

Will add.

> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

-- 
Peter Xu