Re: [Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

Re: [Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

[Vadim Galitsyn]

Hi Guys,

This thread is a continuation of discussion from:
  http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg01565.html

I will post changes list here in cover letter.

v5:
 * hmp: Updated description and '.help' message for
   'info memory-size-summary' command.

 * hmp: Removed '-' characters from
   'info memory-size-summary' output.

 * Dropped ballooned memory information.

 * get_existing_hotpluggable_memory_size() assumed
   to never fail; routine now has no arguments and
   returns uint64_t; in case if target does not support
   memory hotplug, (uint64_t)-1 is returned.

 * MemoryInfo structure:
   * Removed @balloon-actual-memory field.
   * Field @hotpluggable-memory renamed
     to @hotunpluggable-memory.
   * Updated description for fields.

 * qmp: Updated description for
   query-memory-size-summary.

 * Patch v4 splitted into series.

v4:
 * Commands "info memory" and "query-memory" were renamed
   to "info memory-size-summary" and "query-memory-size-summary"
   correspondingly.
 * Descriptions for both commands as well as MemoryInfo structure
   fields were updated/renamed according to
   http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg05972.html.
 * In MemoryInfo structure following fields are now optional:
   hotpluggable-memory and balloon-actual-memory.
 * Field "hotpluggable-memory" now not displayed in HMP if target
   has no CONFIG_MEM_HOTPLUG enabled.
 * Field "balloon-actual-memory" now not displayed in HMP if
   ballooning not enabled.
 * qapi_free_MemoryInfo() used in order to free corresponding memory
   instead of g_free().
 * #ifdef CONFIG_MEM_HOTPLUG was removed and replaced with stubs/ approach.
   get_exiting_hotpluggable_memory_size() function was introduced in
   hw/mem/pc-dimm.c (available for all targets which have CONFIG_MEM_HOTPLUG
   enabled). For other targets, there is a stub in stubs/qmp_pc_dimm.c.
   In addition, stubs/qmp_pc_dimm_device_list.c was renamed to
   stubs/qmp_pc_dimm.c in order to reflect actual source file content.
 * Commit message was updated in order to reflect what was changed.

v3:
 * Use PRIu64 instead of 'lu' when printing results via HMP.
 * Report zero hot-plugged memory instead of reporting error
   when target architecture has no CONFIG_MEM_HOTPLUG enabled.

v2:
 * Fixed build for targets which do not have CONFIG_MEM_HOTPLUG
   enabled.

Best regards,
Vadim

[Qemu-devel] [PATCH v5 1/3] Extend "info numa" with hotplugged memory information

[Vadim Galitsyn]

Report amount of hotplugged memory in addition to total
amount per NUMA node.

Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: qemu-devel@nongnu.org
---
 include/qemu/typedefs.h |  1 +
 include/sysemu/numa.h   |  7 ++++++-
 monitor.c               |  9 ++++++---
 numa.c                  | 18 +++++++++++++-----
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 39bc8351a3..e5f5347e01 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -100,6 +100,7 @@ typedef struct uWireSlave uWireSlave;
 typedef struct VirtIODevice VirtIODevice;
 typedef struct Visitor Visitor;
 typedef struct node_info NodeInfo;
+typedef struct numa_node_mem NumaNodeMem;
 typedef void SaveStateHandler(QEMUFile *f, void *opaque);
 typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
 
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 610eece211..af2e2d3af2 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -24,9 +24,14 @@ struct node_info {
     uint8_t distance[MAX_NODES];
 };
 
+struct numa_node_mem {
+    uint64_t node_mem;
+    uint64_t node_hotpluggable_mem;
+};
+
 extern NodeInfo numa_info[MAX_NODES];
 void parse_numa_opts(MachineState *ms);
-void query_numa_node_mem(uint64_t node_mem[]);
+void query_numa_node_mem(NumaNodeMem node_mem[]);
 extern QemuOptsList qemu_numa_opts;
 void numa_set_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
 void numa_unset_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
diff --git a/monitor.c b/monitor.c
index 6d040e620f..ba1f81e88e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1710,11 +1710,12 @@ static void hmp_info_mtree(Monitor *mon, const QDict *qdict)
 static void hmp_info_numa(Monitor *mon, const QDict *qdict)
 {
     int i;
-    uint64_t *node_mem;
+    NumaNodeMem *node_mem;
     CpuInfoList *cpu_list, *cpu;
 
     cpu_list = qmp_query_cpus(&error_abort);
-    node_mem = g_new0(uint64_t, nb_numa_nodes);
+    node_mem = g_new0(NumaNodeMem, nb_numa_nodes);
+
     query_numa_node_mem(node_mem);
     monitor_printf(mon, "%d nodes\n", nb_numa_nodes);
     for (i = 0; i < nb_numa_nodes; i++) {
@@ -1727,7 +1728,9 @@ static void hmp_info_numa(Monitor *mon, const QDict *qdict)
         }
         monitor_printf(mon, "\n");
         monitor_printf(mon, "node %d size: %" PRId64 " MB\n", i,
-                       node_mem[i] >> 20);
+                       node_mem[i].node_mem >> 20);
+        monitor_printf(mon, "node %d hotplugged: %" PRId64 " MB\n", i,
+                       node_mem[i].node_hotpluggable_mem >> 20);
     }
     qapi_free_CpuInfoList(cpu_list);
     g_free(node_mem);
diff --git a/numa.c b/numa.c
index e32af04cd2..e3b4013f99 100644
--- a/numa.c
+++ b/numa.c
@@ -591,11 +591,12 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
     }
 }
 
-static void numa_stat_memory_devices(uint64_t node_mem[])
+static void numa_stat_memory_devices(NumaNodeMem node_mem[])
 {
     MemoryDeviceInfoList *info_list = NULL;
     MemoryDeviceInfoList **prev = &info_list;
     MemoryDeviceInfoList *info;
+    PCDIMMDeviceInfo     *pcdimm_info;
 
     qmp_pc_dimm_device_list(qdev_get_machine(), &prev);
     for (info = info_list; info; info = info->next) {
@@ -603,9 +604,16 @@ static void numa_stat_memory_devices(uint64_t node_mem[])
 
         if (value) {
             switch (value->type) {
-            case MEMORY_DEVICE_INFO_KIND_DIMM:
-                node_mem[value->u.dimm.data->node] += value->u.dimm.data->size;
+            case MEMORY_DEVICE_INFO_KIND_DIMM: {
+                pcdimm_info = value->u.dimm.data;
+                node_mem[pcdimm_info->node].node_mem += pcdimm_info->size;
+                if (pcdimm_info->hotpluggable && pcdimm_info->hotplugged) {
+                    node_mem[pcdimm_info->node].node_hotpluggable_mem +=
+                        pcdimm_info->size;
+                }
                 break;
+            }
+
             default:
                 break;
             }
@@ -614,7 +622,7 @@ static void numa_stat_memory_devices(uint64_t node_mem[])
     qapi_free_MemoryDeviceInfoList(info_list);
 }
 
-void query_numa_node_mem(uint64_t node_mem[])
+void query_numa_node_mem(NumaNodeMem node_mem[])
 {
     int i;
 
@@ -624,7 +632,7 @@ void query_numa_node_mem(uint64_t node_mem[])
 
     numa_stat_memory_devices(node_mem);
     for (i = 0; i < nb_numa_nodes; i++) {
-        node_mem[i] += numa_info[i].node_mem;
+        node_mem[i].node_mem += numa_info[i].node_mem;
     }
 }
 
-- 
2.13.1.394.g41dd433

[Qemu-devel] [PATCH v5 2/3] qmp: introduce query-memory-size-summary command

[Vadim Galitsyn]

Command above provides the following memory information in bytes:

  * base-memory - size of "base" memory specified with command line option -m.

  * hotunpluggable-memory - amount of memory that was hot-plugged.
    If target does not have CONFIG_MEM_HOTPLUG enabled, no
    value is reported.

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org
---
 hw/mem/pc-dimm.c                                   |  5 +++++
 include/hw/mem/pc-dimm.h                           |  1 +
 qapi-schema.json                                   | 25 ++++++++++++++++++++++
 qmp.c                                              | 13 +++++++++++
 stubs/Makefile.objs                                |  2 +-
 stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 +++++
 6 files changed, 50 insertions(+), 1 deletion(-)
 rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (64%)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index ea67b461c2..1df8b7ee57 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -159,6 +159,11 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
     return cap.size;
 }
 
+uint64_t get_existing_hotpluggable_memory_size(void)
+{
+    return pc_existing_dimms_capacity(&error_abort);
+}
+
 int qmp_pc_dimm_device_list(Object *obj, void *opaque)
 {
     MemoryDeviceInfoList ***prev = opaque;
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 1e483f2670..52c6b5e641 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -95,6 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
 int qmp_pc_dimm_device_list(Object *obj, void *opaque);
 uint64_t pc_existing_dimms_capacity(Error **errp);
+uint64_t get_existing_hotpluggable_memory_size(void);
 void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
                          MemoryRegion *mr, uint64_t align, Error **errp);
 void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
diff --git a/qapi-schema.json b/qapi-schema.json
index 9c6c3e1a53..bbedf1a7bc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4407,6 +4407,31 @@
   'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
             '*unavailable-features': [ 'str' ], 'typename': 'str' } }
 
+##
+# @MemoryInfo:
+#
+# Actual memory information in bytes.
+#
+# @base-memory: size of "base" memory specified with command line
+#               option -m.
+#
+# @hotunpluggable-memory: size memory that can be hot-unplugged.
+#
+# Since: 2.10.0
+##
+{ 'struct': 'MemoryInfo',
+  'data'  : { 'base-memory': 'size', '*hotunpluggable-memory': 'size' } }
+
+##
+# @query-memory-size-summary:
+#
+# Return the amount of initially allocated and hot-plugged (if
+# enabled) memory in bytes.
+#
+# Since: 2.10.0
+##
+{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
+
 ##
 # @query-cpu-definitions:
 #
diff --git a/qmp.c b/qmp.c
index b86201e349..682d950440 100644
--- a/qmp.c
+++ b/qmp.c
@@ -709,3 +709,16 @@ ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error **errp)
 
     return head;
 }
+
+MemoryInfo *qmp_query_memory_size_summary(Error **errp)
+{
+    MemoryInfo *mem_info = g_malloc0(sizeof(MemoryInfo));
+
+    mem_info->base_memory = ram_size;
+
+    mem_info->hotunpluggable_memory = get_existing_hotpluggable_memory_size();
+    mem_info->has_hotunpluggable_memory =
+        (mem_info->hotunpluggable_memory != (uint64_t)-1);
+
+    return mem_info;
+}
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f5b47bfd74..f7cab5b11c 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -32,7 +32,7 @@ stub-obj-y += uuid.o
 stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
-stub-obj-y += qmp_pc_dimm_device_list.o
+stub-obj-y += qmp_pc_dimm.o
 stub-obj-y += target-monitor-defs.o
 stub-obj-y += target-get-monitor-def.o
 stub-obj-y += pc_madt_cpu_entry.o
diff --git a/stubs/qmp_pc_dimm_device_list.c b/stubs/qmp_pc_dimm.c
similarity index 64%
rename from stubs/qmp_pc_dimm_device_list.c
rename to stubs/qmp_pc_dimm.c
index def211564d..1d1e008b58 100644
--- a/stubs/qmp_pc_dimm_device_list.c
+++ b/stubs/qmp_pc_dimm.c
@@ -6,3 +6,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
 {
    return 0;
 }
+
+uint64_t get_existing_hotpluggable_memory_size(void)
+{
+    return (uint64_t)-1;
+}
-- 
2.13.1.394.g41dd433

[Qemu-devel] [PATCH v5 3/3] hmp: introduce 'info memory-size-summary' command

[Vadim Galitsyn]

This command is an equivalent of QMP command query-memory-size-summary.
It provides the following memory information in bytes:

  * base-memory - size of "base" memory specified with command line option -m.

  * hotunpluggable-memory - amount of memory that was hot-plugged.
    If target does not have CONFIG_MEM_HOTPLUG enabled, no
    value is reported.

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org
---
 hmp-commands-info.hx     | 16 ++++++++++++++++
 hmp.c                    | 16 ++++++++++++++++
 hmp.h                    |  1 +
 hw/mem/pc-dimm.c         |  2 +-
 include/hw/mem/pc-dimm.h |  2 +-
 qmp.c                    |  3 ++-
 stubs/qmp_pc_dimm.c      |  2 +-
 7 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index d9df238a5f..c5a62699ed 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -849,6 +849,22 @@ ETEXI
         .cmd = hmp_info_vm_generation_id,
     },
 
+STEXI
+@item info memory-size-summary
+@findex memory-size-summary
+Display the amount of initially allocated and hot-plugged (if
+enabled) memory in bytes.
+ETEXI
+
+    {
+        .name       = "memory-size-summary",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show the amount of initially allocated and "
+                      "hot-plugged (if enabled) memory in bytes.",
+        .cmd        = hmp_info_memory_size_summary,
+    },
+
 STEXI
 @end table
 ETEXI
diff --git a/hmp.c b/hmp.c
index fd80dce758..0c14ecc454 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2868,3 +2868,19 @@ void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
     qapi_free_GuidInfo(info);
 }
+
+void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
+{
+    MemoryInfo *info = qmp_query_memory_size_summary(&error_abort);
+    if (info) {
+        monitor_printf(mon, "base memory: %" PRIu64 "\n",
+                       info->base_memory);
+
+        if (info->has_hotunpluggable_memory) {
+            monitor_printf(mon, "hotunpluggable memory: %" PRIu64 "\n",
+                           info->hotunpluggable_memory);
+        }
+
+        qapi_free_MemoryInfo(info);
+    }
+}
diff --git a/hmp.h b/hmp.h
index 1ff455295e..3605003e4c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -145,5 +145,6 @@ 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);
+void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 1df8b7ee57..f00c61bb82 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -159,7 +159,7 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
     return cap.size;
 }
 
-uint64_t get_existing_hotpluggable_memory_size(void)
+uint64_t get_existing_hotunpluggable_memory_size(void)
 {
     return pc_existing_dimms_capacity(&error_abort);
 }
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 52c6b5e641..7dd8c3b7c1 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -95,7 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
 int qmp_pc_dimm_device_list(Object *obj, void *opaque);
 uint64_t pc_existing_dimms_capacity(Error **errp);
-uint64_t get_existing_hotpluggable_memory_size(void);
+uint64_t get_existing_hotunpluggable_memory_size(void);
 void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
                          MemoryRegion *mr, uint64_t align, Error **errp);
 void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
diff --git a/qmp.c b/qmp.c
index 682d950440..18a7594b54 100644
--- a/qmp.c
+++ b/qmp.c
@@ -716,7 +716,8 @@ MemoryInfo *qmp_query_memory_size_summary(Error **errp)
 
     mem_info->base_memory = ram_size;
 
-    mem_info->hotunpluggable_memory = get_existing_hotpluggable_memory_size();
+    mem_info->hotunpluggable_memory =
+        get_existing_hotunpluggable_memory_size();
     mem_info->has_hotunpluggable_memory =
         (mem_info->hotunpluggable_memory != (uint64_t)-1);
 
diff --git a/stubs/qmp_pc_dimm.c b/stubs/qmp_pc_dimm.c
index 1d1e008b58..eba97dbbbb 100644
--- a/stubs/qmp_pc_dimm.c
+++ b/stubs/qmp_pc_dimm.c
@@ -7,7 +7,7 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
    return 0;
 }
 
-uint64_t get_existing_hotpluggable_memory_size(void)
+uint64_t get_existing_hotunpluggable_memory_size(void)
 {
     return (uint64_t)-1;
 }
-- 
2.13.1.394.g41dd433

Re: [Qemu-devel] [PATCH v5 2/3] qmp: introduce query-memory-size-summary command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1640 bytes --]

On 07/28/2017 07:10 AM, Vadim Galitsyn wrote:
> Command above provides the following memory information in bytes:
> 
>   * base-memory - size of "base" memory specified with command line option -m.
> 
>   * hotunpluggable-memory - amount of memory that was hot-plugged.
>     If target does not have CONFIG_MEM_HOTPLUG enabled, no
>     value is reported.
> 
> Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
> Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---

> +++ b/qapi-schema.json
> @@ -4407,6 +4407,31 @@
>    'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
>              '*unavailable-features': [ 'str' ], 'typename': 'str' } }
>  
> +##
> +# @MemoryInfo:
> +#
> +# Actual memory information in bytes.
> +#
> +# @base-memory: size of "base" memory specified with command line
> +#               option -m.
> +#
> +# @hotunpluggable-memory: size memory that can be hot-unplugged.
> +#
> +# Since: 2.10.0

At this point, we've missed feature freeze for 2.10; so this would be
better as 2.11.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v5 3/3] hmp: introduce 'info memory-size-summary' command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

On 07/28/2017 07:10 AM, Vadim Galitsyn wrote:
> This command is an equivalent of QMP command query-memory-size-summary.
> It provides the following memory information in bytes:
> 
>   * base-memory - size of "base" memory specified with command line option -m.
> 
>   * hotunpluggable-memory - amount of memory that was hot-plugged.
>     If target does not have CONFIG_MEM_HOTPLUG enabled, no
>     value is reported.

Most of our HMP commands use underscores between words; for consistency,
you might want to name it 'info memory_size_summary'.  Also, between the
new QMP and HMP parameters, do you have any testsuite coverage?  I know
we don't have many existing QMP tests to copy from, but where possible,
we want to avoid adding new QMP features that don't have some sort of
coverage.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v5 2/3] qmp: introduce query-memory-size-summary command

[Markus Armbruster]

Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:

> Command above provides the following memory information in bytes:
>
>   * base-memory - size of "base" memory specified with command line option -m.
>
>   * hotunpluggable-memory - amount of memory that was hot-plugged.
>     If target does not have CONFIG_MEM_HOTPLUG enabled, no
>     value is reported.
>
> Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
> Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  hw/mem/pc-dimm.c                                   |  5 +++++
>  include/hw/mem/pc-dimm.h                           |  1 +
>  qapi-schema.json                                   | 25 ++++++++++++++++++++++
>  qmp.c                                              | 13 +++++++++++
>  stubs/Makefile.objs                                |  2 +-
>  stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 +++++
>  6 files changed, 50 insertions(+), 1 deletion(-)
>  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (64%)
>
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index ea67b461c2..1df8b7ee57 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -159,6 +159,11 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
>      return cap.size;
>  }
>  
> +uint64_t get_existing_hotpluggable_memory_size(void)
> +{
> +    return pc_existing_dimms_capacity(&error_abort);
> +}
> +
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque)
>  {
>      MemoryDeviceInfoList ***prev = opaque;
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 1e483f2670..52c6b5e641 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -95,6 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
>  uint64_t pc_existing_dimms_capacity(Error **errp);
> +uint64_t get_existing_hotpluggable_memory_size(void);
>  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
>                           MemoryRegion *mr, uint64_t align, Error **errp);
>  void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 9c6c3e1a53..bbedf1a7bc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4407,6 +4407,31 @@
>    'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
>              '*unavailable-features': [ 'str' ], 'typename': 'str' } }
>  
> +##
> +# @MemoryInfo:
> +#
> +# Actual memory information in bytes.
> +#
> +# @base-memory: size of "base" memory specified with command line
> +#               option -m.
> +#
> +# @hotunpluggable-memory: size memory that can be hot-unplugged.

"hotunpluggable" is ugly.  What about just "pluggable"?

> +#
> +# Since: 2.10.0
> +##
> +{ 'struct': 'MemoryInfo',
> +  'data'  : { 'base-memory': 'size', '*hotunpluggable-memory': 'size' } }
> +
> +##
> +# @query-memory-size-summary:
> +#
> +# Return the amount of initially allocated and hot-plugged (if
> +# enabled) memory in bytes.
> +#
> +# Since: 2.10.0
> +##
> +{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
> +
>  ##
>  # @query-cpu-definitions:
>  #
> diff --git a/qmp.c b/qmp.c
> index b86201e349..682d950440 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -709,3 +709,16 @@ ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error **errp)
>  
>      return head;
>  }
> +
> +MemoryInfo *qmp_query_memory_size_summary(Error **errp)
> +{
> +    MemoryInfo *mem_info = g_malloc0(sizeof(MemoryInfo));
> +
> +    mem_info->base_memory = ram_size;
> +
> +    mem_info->hotunpluggable_memory = get_existing_hotpluggable_memory_size();

"unpluggable = pluggable" gave me pause, because it looks just like a
bug.  It isn't because pluggable means unpluggable here.

I think this would be more legible with simpler names:

       mem_info->pluggable_memory = get_pluggable_memory_size();

> +    mem_info->has_hotunpluggable_memory =
> +        (mem_info->hotunpluggable_memory != (uint64_t)-1);

Pleas drop the superfluous parenthesis around the comparison.

> +
> +    return mem_info;
> +}
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index f5b47bfd74..f7cab5b11c 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -32,7 +32,7 @@ stub-obj-y += uuid.o
>  stub-obj-y += vm-stop.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_WIN32) += fd-register.o
> -stub-obj-y += qmp_pc_dimm_device_list.o
> +stub-obj-y += qmp_pc_dimm.o
>  stub-obj-y += target-monitor-defs.o
>  stub-obj-y += target-get-monitor-def.o
>  stub-obj-y += pc_madt_cpu_entry.o
> diff --git a/stubs/qmp_pc_dimm_device_list.c b/stubs/qmp_pc_dimm.c
> similarity index 64%
> rename from stubs/qmp_pc_dimm_device_list.c
> rename to stubs/qmp_pc_dimm.c
> index def211564d..1d1e008b58 100644
> --- a/stubs/qmp_pc_dimm_device_list.c
> +++ b/stubs/qmp_pc_dimm.c
> @@ -6,3 +6,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
>  {
>     return 0;
>  }
> +
> +uint64_t get_existing_hotpluggable_memory_size(void)
> +{
> +    return (uint64_t)-1;
> +}

Re: [Qemu-devel] [PATCH v5 3/3] hmp: introduce 'info memory-size-summary' command

[Markus Armbruster]

Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:

> This command is an equivalent of QMP command query-memory-size-summary.
> It provides the following memory information in bytes:
>
>   * base-memory - size of "base" memory specified with command line option -m.
>
>   * hotunpluggable-memory - amount of memory that was hot-plugged.
>     If target does not have CONFIG_MEM_HOTPLUG enabled, no
>     value is reported.
>
> Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
> Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  hmp-commands-info.hx     | 16 ++++++++++++++++
>  hmp.c                    | 16 ++++++++++++++++
>  hmp.h                    |  1 +
>  hw/mem/pc-dimm.c         |  2 +-
>  include/hw/mem/pc-dimm.h |  2 +-
>  qmp.c                    |  3 ++-
>  stubs/qmp_pc_dimm.c      |  2 +-
>  7 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index d9df238a5f..c5a62699ed 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -849,6 +849,22 @@ ETEXI
>          .cmd = hmp_info_vm_generation_id,
>      },
>  
> +STEXI
> +@item info memory-size-summary
> +@findex memory-size-summary
> +Display the amount of initially allocated and hot-plugged (if
> +enabled) memory in bytes.
> +ETEXI
> +
> +    {
> +        .name       = "memory-size-summary",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show the amount of initially allocated and "
> +                      "hot-plugged (if enabled) memory in bytes.",
> +        .cmd        = hmp_info_memory_size_summary,
> +    },
> +
>  STEXI
>  @end table
>  ETEXI
> diff --git a/hmp.c b/hmp.c
> index fd80dce758..0c14ecc454 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2868,3 +2868,19 @@ void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>      qapi_free_GuidInfo(info);
>  }
> +
> +void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
> +{
> +    MemoryInfo *info = qmp_query_memory_size_summary(&error_abort);
> +    if (info) {
> +        monitor_printf(mon, "base memory: %" PRIu64 "\n",
> +                       info->base_memory);
> +
> +        if (info->has_hotunpluggable_memory) {
> +            monitor_printf(mon, "hotunpluggable memory: %" PRIu64 "\n",
> +                           info->hotunpluggable_memory);
> +        }
> +
> +        qapi_free_MemoryInfo(info);
> +    }
> +}
> diff --git a/hmp.h b/hmp.h
> index 1ff455295e..3605003e4c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -145,5 +145,6 @@ 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);
> +void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 1df8b7ee57..f00c61bb82 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -159,7 +159,7 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
>      return cap.size;
>  }
>  
> -uint64_t get_existing_hotpluggable_memory_size(void)
> +uint64_t get_existing_hotunpluggable_memory_size(void)
>  {
>      return pc_existing_dimms_capacity(&error_abort);
>  }
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 52c6b5e641..7dd8c3b7c1 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -95,7 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
>  uint64_t pc_existing_dimms_capacity(Error **errp);
> -uint64_t get_existing_hotpluggable_memory_size(void);
> +uint64_t get_existing_hotunpluggable_memory_size(void);

Introduced in PATCH 2, renamed in PATCH 3.  Try again :)

>  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
>                           MemoryRegion *mr, uint64_t align, Error **errp);
>  void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> diff --git a/qmp.c b/qmp.c
> index 682d950440..18a7594b54 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -716,7 +716,8 @@ MemoryInfo *qmp_query_memory_size_summary(Error **errp)
>  
>      mem_info->base_memory = ram_size;
>  
> -    mem_info->hotunpluggable_memory = get_existing_hotpluggable_memory_size();
> +    mem_info->hotunpluggable_memory =
> +        get_existing_hotunpluggable_memory_size();
>      mem_info->has_hotunpluggable_memory =
>          (mem_info->hotunpluggable_memory != (uint64_t)-1);
>  
> diff --git a/stubs/qmp_pc_dimm.c b/stubs/qmp_pc_dimm.c
> index 1d1e008b58..eba97dbbbb 100644
> --- a/stubs/qmp_pc_dimm.c
> +++ b/stubs/qmp_pc_dimm.c
> @@ -7,7 +7,7 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
>     return 0;
>  }
>  
> -uint64_t get_existing_hotpluggable_memory_size(void)
> +uint64_t get_existing_hotunpluggable_memory_size(void)
>  {
>      return (uint64_t)-1;
>  }

Re: [Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

[Markus Armbruster]

Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:

> Hi Guys,
>
> This thread is a continuation of discussion from:
>   http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg01565.html
>
> I will post changes list here in cover letter.

Looks good from a QMP point of view.  Just clean up the naming issues.

Eduardo, Igor, through which tree should this go?

Re: [Qemu-devel] [PATCH v5 2/3] qmp: introduce query-memory-size-summary command

[Igor Mammedov]

On Fri, 28 Jul 2017 14:10:43 +0200
Vadim Galitsyn <vadim.galitsyn@profitbricks.com> wrote:

> Command above provides the following memory information in bytes:
> 
>   * base-memory - size of "base" memory specified with command line option -m.
> 
>   * hotunpluggable-memory - amount of memory that was hot-plugged.
>     If target does not have CONFIG_MEM_HOTPLUG enabled, no
>     value is reported.
> 
> Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
> Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  hw/mem/pc-dimm.c                                   |  5 +++++
>  include/hw/mem/pc-dimm.h                           |  1 +
>  qapi-schema.json                                   | 25 ++++++++++++++++++++++
>  qmp.c                                              | 13 +++++++++++
>  stubs/Makefile.objs                                |  2 +-
>  stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 +++++
>  6 files changed, 50 insertions(+), 1 deletion(-)
>  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (64%)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index ea67b461c2..1df8b7ee57 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -159,6 +159,11 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
>      return cap.size;
>  }
>  
> +uint64_t get_existing_hotpluggable_memory_size(void)
> +{
> +    return pc_existing_dimms_capacity(&error_abort);
> +}
> +
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque)
>  {
>      MemoryDeviceInfoList ***prev = opaque;
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 1e483f2670..52c6b5e641 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -95,6 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
>  uint64_t pc_existing_dimms_capacity(Error **errp);
> +uint64_t get_existing_hotpluggable_memory_size(void);
>  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
>                           MemoryRegion *mr, uint64_t align, Error **errp);
>  void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 9c6c3e1a53..bbedf1a7bc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4407,6 +4407,31 @@
>    'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
>              '*unavailable-features': [ 'str' ], 'typename': 'str' } }
>  
> +##
> +# @MemoryInfo:
> +#
> +# Actual memory information in bytes.
> +#
> +# @base-memory: size of "base" memory specified with command line
> +#               option -m.
> +#
> +# @hotunpluggable-memory: size memory that can be hot-unplugged.
> +#
> +# Since: 2.10.0
> +##
> +{ 'struct': 'MemoryInfo',
> +  'data'  : { 'base-memory': 'size', '*hotunpluggable-memory': 'size' } }
> +
> +##
> +# @query-memory-size-summary:
> +#
> +# Return the amount of initially allocated and hot-plugged (if
> +# enabled) memory in bytes.
> +#
> +# Since: 2.10.0
> +##
> +{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
> +
>  ##
>  # @query-cpu-definitions:
>  #
> diff --git a/qmp.c b/qmp.c
> index b86201e349..682d950440 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -709,3 +709,16 @@ ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error **errp)
>  
>      return head;
>  }
> +
> +MemoryInfo *qmp_query_memory_size_summary(Error **errp)
> +{
> +    MemoryInfo *mem_info = g_malloc0(sizeof(MemoryInfo));
> +
> +    mem_info->base_memory = ram_size;
> +
> +    mem_info->hotunpluggable_memory = get_existing_hotpluggable_memory_size();
this could be done without wrapper:
mem_info->hotunpluggable_memory = pc_existing_dimms_capacity(&local_error)

> +    mem_info->has_hotunpluggable_memory =
> +        (mem_info->hotunpluggable_memory != (uint64_t)-1);

mem_info->has_hotunpluggable_memory = !local_error;
if (local_error)
  cleanup

and defining stub:

uint64_t pc_existing_dimms_capacity(Error **errp) {
     error_setg(errp, "not implemented");
     return 0;
}

> +
> +    return mem_info;
> +}
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index f5b47bfd74..f7cab5b11c 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -32,7 +32,7 @@ stub-obj-y += uuid.o
>  stub-obj-y += vm-stop.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_WIN32) += fd-register.o
> -stub-obj-y += qmp_pc_dimm_device_list.o
> +stub-obj-y += qmp_pc_dimm.o
>  stub-obj-y += target-monitor-defs.o
>  stub-obj-y += target-get-monitor-def.o
>  stub-obj-y += pc_madt_cpu_entry.o
> diff --git a/stubs/qmp_pc_dimm_device_list.c b/stubs/qmp_pc_dimm.c
> similarity index 64%
> rename from stubs/qmp_pc_dimm_device_list.c
> rename to stubs/qmp_pc_dimm.c
> index def211564d..1d1e008b58 100644
> --- a/stubs/qmp_pc_dimm_device_list.c
> +++ b/stubs/qmp_pc_dimm.c
> @@ -6,3 +6,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
>  {
>     return 0;
>  }
> +
> +uint64_t get_existing_hotpluggable_memory_size(void)
> +{
> +    return (uint64_t)-1;
> +}

Re: [Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

[Igor Mammedov]

On Mon, 14 Aug 2017 16:32:40 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:
> 
> > Hi Guys,
> >
> > This thread is a continuation of discussion from:
> >   http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg01565.html
> >
> > I will post changes list here in cover letter.  
> 
> Looks good from a QMP point of view.  Just clean up the naming issues.
> 
> Eduardo, Igor, through which tree should this go?
It's mostly QMP/HMP interface so it could go via QMP tree.

Re: [Qemu-devel] [PATCH v5 2/3] qmp: introduce query-memory-size-summary command

[Vadim Galitsyn]

Hi Guys,

Thank you for the input!

> "hotunpluggable" is ugly.  What about just "pluggable"?

Markus, I think "pluggable" is a bit misleading here. Some people can take
it like a maximum amount of memory that can be hot-added to a guest (i.e.,
difference between static memory size and value specified with "-m
...,maxmem=XXX"). I would say:

  mem_info->plugged_memory = get_plugged_memory_size();

..since it reflects actual amount which was already hot-added. Agree?

> this could be done without wrapper

Igor, your approach changes command behaviour a little. First of all it
turns it in command which can fail. As far as I understand we wanted to
avoid this.

Wrapper is needed in order to "error_abort" command only in case if (a)
CONFIG_MEM_HOTPLUG  is enabled, but (b) QOM data is somehow corrupted.
If CONFIG_MEM_HOTPLUG
is disabled for target, it does not mean to report an error -- in this case
value simply not reported and no error path triggered.

I agree with the rest of comments and will provide changes with the next
version.
Could you please leave a comment(s) if you agree or not (before I submit
next version)?

Thank you in advance,
Vadim


On Tue, Aug 15, 2017 at 9:51 AM, Igor Mammedov <imammedo@redhat.com> wrote:

> On Fri, 28 Jul 2017 14:10:43 +0200
> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> wrote:
>
> > Command above provides the following memory information in bytes:
> >
> >   * base-memory - size of "base" memory specified with command line
> option -m.
> >
> >   * hotunpluggable-memory - amount of memory that was hot-plugged.
> >     If target does not have CONFIG_MEM_HOTPLUG enabled, no
> >     value is reported.
> >
> > Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com
> >
> > Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
> > Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> > Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> > Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Eric Blake <eblake@redhat.com>
> > Cc: qemu-devel@nongnu.org
> > ---
> >  hw/mem/pc-dimm.c                                   |  5 +++++
> >  include/hw/mem/pc-dimm.h                           |  1 +
> >  qapi-schema.json                                   | 25
> ++++++++++++++++++++++
> >  qmp.c                                              | 13 +++++++++++
> >  stubs/Makefile.objs                                |  2 +-
> >  stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 +++++
> >  6 files changed, 50 insertions(+), 1 deletion(-)
> >  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (64%)
> >
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index ea67b461c2..1df8b7ee57 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -159,6 +159,11 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
> >      return cap.size;
> >  }
> >
> > +uint64_t get_existing_hotpluggable_memory_size(void)
> > +{
> > +    return pc_existing_dimms_capacity(&error_abort);
> > +}
> > +
> >  int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> >  {
> >      MemoryDeviceInfoList ***prev = opaque;
> > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> > index 1e483f2670..52c6b5e641 100644
> > --- a/include/hw/mem/pc-dimm.h
> > +++ b/include/hw/mem/pc-dimm.h
> > @@ -95,6 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int
> max_slots, Error **errp);
> >
> >  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
> >  uint64_t pc_existing_dimms_capacity(Error **errp);
> > +uint64_t get_existing_hotpluggable_memory_size(void);
> >  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> >                           MemoryRegion *mr, uint64_t align, Error
> **errp);
> >  void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 9c6c3e1a53..bbedf1a7bc 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4407,6 +4407,31 @@
> >    'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
> >              '*unavailable-features': [ 'str' ], 'typename': 'str' } }
> >
> > +##
> > +# @MemoryInfo:
> > +#
> > +# Actual memory information in bytes.
> > +#
> > +# @base-memory: size of "base" memory specified with command line
> > +#               option -m.
> > +#
> > +# @hotunpluggable-memory: size memory that can be hot-unplugged.
> > +#
> > +# Since: 2.10.0
> > +##
> > +{ 'struct': 'MemoryInfo',
> > +  'data'  : { 'base-memory': 'size', '*hotunpluggable-memory': 'size' }
> }
> > +
> > +##
> > +# @query-memory-size-summary:
> > +#
> > +# Return the amount of initially allocated and hot-plugged (if
> > +# enabled) memory in bytes.
> > +#
> > +# Since: 2.10.0
> > +##
> > +{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
> > +
> >  ##
> >  # @query-cpu-definitions:
> >  #
> > diff --git a/qmp.c b/qmp.c
> > index b86201e349..682d950440 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -709,3 +709,16 @@ ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error
> **errp)
> >
> >      return head;
> >  }
> > +
> > +MemoryInfo *qmp_query_memory_size_summary(Error **errp)
> > +{
> > +    MemoryInfo *mem_info = g_malloc0(sizeof(MemoryInfo));
> > +
> > +    mem_info->base_memory = ram_size;
> > +
> > +    mem_info->hotunpluggable_memory = get_existing_hotpluggable_
> memory_size();
> this could be done without wrapper:
> mem_info->hotunpluggable_memory = pc_existing_dimms_capacity(&local_error)
>
> > +    mem_info->has_hotunpluggable_memory =
> > +        (mem_info->hotunpluggable_memory != (uint64_t)-1);
>
> mem_info->has_hotunpluggable_memory = !local_error;
> if (local_error)
>   cleanup
>
> and defining stub:
>
> uint64_t pc_existing_dimms_capacity(Error **errp) {
>      error_setg(errp, "not implemented");
>      return 0;
> }
>
> > +
> > +    return mem_info;
> > +}
> > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > index f5b47bfd74..f7cab5b11c 100644
> > --- a/stubs/Makefile.objs
> > +++ b/stubs/Makefile.objs
> > @@ -32,7 +32,7 @@ stub-obj-y += uuid.o
> >  stub-obj-y += vm-stop.o
> >  stub-obj-y += vmstate.o
> >  stub-obj-$(CONFIG_WIN32) += fd-register.o
> > -stub-obj-y += qmp_pc_dimm_device_list.o
> > +stub-obj-y += qmp_pc_dimm.o
> >  stub-obj-y += target-monitor-defs.o
> >  stub-obj-y += target-get-monitor-def.o
> >  stub-obj-y += pc_madt_cpu_entry.o
> > diff --git a/stubs/qmp_pc_dimm_device_list.c b/stubs/qmp_pc_dimm.c
> > similarity index 64%
> > rename from stubs/qmp_pc_dimm_device_list.c
> > rename to stubs/qmp_pc_dimm.c
> > index def211564d..1d1e008b58 100644
> > --- a/stubs/qmp_pc_dimm_device_list.c
> > +++ b/stubs/qmp_pc_dimm.c
> > @@ -6,3 +6,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> >  {
> >     return 0;
> >  }
> > +
> > +uint64_t get_existing_hotpluggable_memory_size(void)
> > +{
> > +    return (uint64_t)-1;
> > +}
>
>

Re: [Qemu-devel] [PATCH v5 3/3] hmp: introduce 'info memory-size-summary' command

[Vadim Galitsyn]

Hi Eric,

Thank you for the input. I will update it with the next version. Btw, most
of HMP "info *" commands use '-' instead of '_' in names =)

Best regards,
Vadim

On Fri, Jul 28, 2017 at 8:27 PM, Eric Blake <eblake@redhat.com> wrote:

> On 07/28/2017 07:10 AM, Vadim Galitsyn wrote:
> > This command is an equivalent of QMP command query-memory-size-summary.
> > It provides the following memory information in bytes:
> >
> >   * base-memory - size of "base" memory specified with command line
> option -m.
> >
> >   * hotunpluggable-memory - amount of memory that was hot-plugged.
> >     If target does not have CONFIG_MEM_HOTPLUG enabled, no
> >     value is reported.
>
> Most of our HMP commands use underscores between words; for consistency,
> you might want to name it 'info memory_size_summary'.  Also, between the
> new QMP and HMP parameters, do you have any testsuite coverage?  I know
> we don't have many existing QMP tests to copy from, but where possible,
> we want to avoid adding new QMP features that don't have some sort of
> coverage.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>
>

Re: [Qemu-devel] [PATCH v5 2/3] qmp: introduce query-memory-size-summary command

[Igor Mammedov]

On Tue, 15 Aug 2017 17:43:00 +0200
Vadim Galitsyn <vadim.galitsyn@profitbricks.com> wrote:

> Hi Guys,
> 
> Thank you for the input!
> 
> > "hotunpluggable" is ugly.  What about just "pluggable"?  
> 
> Markus, I think "pluggable" is a bit misleading here. Some people can take
> it like a maximum amount of memory that can be hot-added to a guest (i.e.,
> difference between static memory size and value specified with "-m
> ...,maxmem=XXX"). I would say:
> 
>   mem_info->plugged_memory = get_plugged_memory_size();
> 
> ..since it reflects actual amount which was already hot-added. Agree?
> 
> > this could be done without wrapper  
> 
> Igor, your approach changes command behaviour a little. First of all it
> turns it in command which can fail. As far as I understand we wanted to
> avoid this.
> 
> Wrapper is needed in order to "error_abort" command only in case if (a)
> CONFIG_MEM_HOTPLUG  is enabled, but (b) QOM data is somehow corrupted.
> If CONFIG_MEM_HOTPLUG
> is disabled for target, it does not mean to report an error -- in this case
> value simply not reported and no error path triggered.
Ok,
keeping current approach is also fine.

> 
> I agree with the rest of comments and will provide changes with the next
> version.
> Could you please leave a comment(s) if you agree or not (before I submit
> next version)?
> 
> Thank you in advance,
> Vadim
> 
> 
> On Tue, Aug 15, 2017 at 9:51 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Fri, 28 Jul 2017 14:10:43 +0200
> > Vadim Galitsyn <vadim.galitsyn@profitbricks.com> wrote:
> >  
> > > Command above provides the following memory information in bytes:
> > >
> > >   * base-memory - size of "base" memory specified with command line  
> > option -m.  
> > >
> > >   * hotunpluggable-memory - amount of memory that was hot-plugged.
> > >     If target does not have CONFIG_MEM_HOTPLUG enabled, no
> > >     value is reported.
> > >
> > > Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com
> > >
> > > Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
> > > Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> > > Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> > > Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
> > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Cc: Markus Armbruster <armbru@redhat.com>
> > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > Cc: Eric Blake <eblake@redhat.com>
> > > Cc: qemu-devel@nongnu.org
> > > ---
> > >  hw/mem/pc-dimm.c                                   |  5 +++++
> > >  include/hw/mem/pc-dimm.h                           |  1 +
> > >  qapi-schema.json                                   | 25  
> > ++++++++++++++++++++++  
> > >  qmp.c                                              | 13 +++++++++++
> > >  stubs/Makefile.objs                                |  2 +-
> > >  stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 +++++
> > >  6 files changed, 50 insertions(+), 1 deletion(-)
> > >  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (64%)
> > >
> > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > index ea67b461c2..1df8b7ee57 100644
> > > --- a/hw/mem/pc-dimm.c
> > > +++ b/hw/mem/pc-dimm.c
> > > @@ -159,6 +159,11 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
> > >      return cap.size;
> > >  }
> > >
> > > +uint64_t get_existing_hotpluggable_memory_size(void)
> > > +{
> > > +    return pc_existing_dimms_capacity(&error_abort);
> > > +}
> > > +
> > >  int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> > >  {
> > >      MemoryDeviceInfoList ***prev = opaque;
> > > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> > > index 1e483f2670..52c6b5e641 100644
> > > --- a/include/hw/mem/pc-dimm.h
> > > +++ b/include/hw/mem/pc-dimm.h
> > > @@ -95,6 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int  
> > max_slots, Error **errp);  
> > >
> > >  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
> > >  uint64_t pc_existing_dimms_capacity(Error **errp);
> > > +uint64_t get_existing_hotpluggable_memory_size(void);
> > >  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> > >                           MemoryRegion *mr, uint64_t align, Error  
> > **errp);  
> > >  void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 9c6c3e1a53..bbedf1a7bc 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -4407,6 +4407,31 @@
> > >    'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
> > >              '*unavailable-features': [ 'str' ], 'typename': 'str' } }
> > >
> > > +##
> > > +# @MemoryInfo:
> > > +#
> > > +# Actual memory information in bytes.
> > > +#
> > > +# @base-memory: size of "base" memory specified with command line
> > > +#               option -m.
> > > +#
> > > +# @hotunpluggable-memory: size memory that can be hot-unplugged.
> > > +#
> > > +# Since: 2.10.0
> > > +##
> > > +{ 'struct': 'MemoryInfo',
> > > +  'data'  : { 'base-memory': 'size', '*hotunpluggable-memory': 'size' }  
> > }  
> > > +
> > > +##
> > > +# @query-memory-size-summary:
> > > +#
> > > +# Return the amount of initially allocated and hot-plugged (if
> > > +# enabled) memory in bytes.
> > > +#
> > > +# Since: 2.10.0
> > > +##
> > > +{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
> > > +
> > >  ##
> > >  # @query-cpu-definitions:
> > >  #
> > > diff --git a/qmp.c b/qmp.c
> > > index b86201e349..682d950440 100644
> > > --- a/qmp.c
> > > +++ b/qmp.c
> > > @@ -709,3 +709,16 @@ ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error  
> > **errp)  
> > >
> > >      return head;
> > >  }
> > > +
> > > +MemoryInfo *qmp_query_memory_size_summary(Error **errp)
> > > +{
> > > +    MemoryInfo *mem_info = g_malloc0(sizeof(MemoryInfo));
> > > +
> > > +    mem_info->base_memory = ram_size;
> > > +
> > > +    mem_info->hotunpluggable_memory = get_existing_hotpluggable_  
> > memory_size();
> > this could be done without wrapper:
> > mem_info->hotunpluggable_memory = pc_existing_dimms_capacity(&local_error)
> >  
> > > +    mem_info->has_hotunpluggable_memory =
> > > +        (mem_info->hotunpluggable_memory != (uint64_t)-1);  
> >
> > mem_info->has_hotunpluggable_memory = !local_error;
> > if (local_error)
> >   cleanup
> >
> > and defining stub:
> >
> > uint64_t pc_existing_dimms_capacity(Error **errp) {
> >      error_setg(errp, "not implemented");
> >      return 0;
> > }
> >  
> > > +
> > > +    return mem_info;
> > > +}
> > > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > > index f5b47bfd74..f7cab5b11c 100644
> > > --- a/stubs/Makefile.objs
> > > +++ b/stubs/Makefile.objs
> > > @@ -32,7 +32,7 @@ stub-obj-y += uuid.o
> > >  stub-obj-y += vm-stop.o
> > >  stub-obj-y += vmstate.o
> > >  stub-obj-$(CONFIG_WIN32) += fd-register.o
> > > -stub-obj-y += qmp_pc_dimm_device_list.o
> > > +stub-obj-y += qmp_pc_dimm.o
> > >  stub-obj-y += target-monitor-defs.o
> > >  stub-obj-y += target-get-monitor-def.o
> > >  stub-obj-y += pc_madt_cpu_entry.o
> > > diff --git a/stubs/qmp_pc_dimm_device_list.c b/stubs/qmp_pc_dimm.c
> > > similarity index 64%
> > > rename from stubs/qmp_pc_dimm_device_list.c
> > > rename to stubs/qmp_pc_dimm.c
> > > index def211564d..1d1e008b58 100644
> > > --- a/stubs/qmp_pc_dimm_device_list.c
> > > +++ b/stubs/qmp_pc_dimm.c
> > > @@ -6,3 +6,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> > >  {
> > >     return 0;
> > >  }
> > > +
> > > +uint64_t get_existing_hotpluggable_memory_size(void)
> > > +{
> > > +    return (uint64_t)-1;
> > > +}  
> >
> >  

Re: [Qemu-devel] [PATCH v5 2/3] qmp: introduce query-memory-size-summary command

[Markus Armbruster]

Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:

> Hi Guys,
>
> Thank you for the input!
>
>> "hotunpluggable" is ugly.  What about just "pluggable"?
>
> Markus, I think "pluggable" is a bit misleading here. Some people can take
> it like a maximum amount of memory that can be hot-added to a guest (i.e.,
> difference between static memory size and value specified with "-m
> ...,maxmem=XXX"). I would say:
>
>   mem_info->plugged_memory = get_plugged_memory_size();
>
> ..since it reflects actual amount which was already hot-added. Agree?

I like it.  Thanks!

[...]

Re: [Qemu-devel] [PATCH v5 2/3] qmp: introduce query-memory-size-summary command

[Vadim Galitsyn]

Hi Guys,

Thanks again! Just sent patch v6 which takes into account comments you left.

Best regards,
Vadim


On Wed, Aug 16, 2017 at 8:01 AM, Markus Armbruster <armbru@redhat.com>
wrote:

> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:
>
> > Hi Guys,
> >
> > Thank you for the input!
> >
> >> "hotunpluggable" is ugly.  What about just "pluggable"?
> >
> > Markus, I think "pluggable" is a bit misleading here. Some people can
> take
> > it like a maximum amount of memory that can be hot-added to a guest
> (i.e.,
> > difference between static memory size and value specified with "-m
> > ...,maxmem=XXX"). I would say:
> >
> >   mem_info->plugged_memory = get_plugged_memory_size();
> >
> > ..since it reflects actual amount which was already hot-added. Agree?
>
> I like it.  Thanks!
>
> [...]
>

Re: [Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

[Vadim Galitsyn]

Hi Guys,

Thank you for the feedback! Unfortunately, I am almost off for vacation and
will not be able to provide next patch in the following couple of weeks.
Nevertheless, will do so as soon as I return back.

I still have a question whether we need to provide NUMA information here?
>From my point of view it is a little bit out of scope of these commands
since it gives a bit more detailed information while the commands aimed to
give a short summary. I have a small patch which extends "info numa" with
hotplugged memory information per NUMA node. Maybe it is better to go this
way? I can include it into this patch series.

In the next patch (2 weeks later unfortunately) I will take into account
all the remaining points from this thread:

1. Turn into patch series by splitting HMP/QMP into separate patches;
2. Drop ballooning information;
3. Include "info numa" patch which extents output with hotplugged memory
info per NUMA node.

Thank you for the feedback once again,
Vadim


On Fri, Jul 7, 2017 at 10:58 AM, Markus Armbruster <armbru@redhat.com>
wrote:

> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> Sorry for the late review, got a bit overwhelmed...
> >>
> >> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:
> >>
> >> > Commands above provide the following memory information in bytes:
> >> >
> >> >   * base-memory - amount of unremovable memory specified
> >> >     with '-m' option at the start of the QEMU process.
> >> >
> >> >   * hotpluggable-memory - amount of memory that was hot-plugged.
> >> >     If target does not have CONFIG_MEM_HOTPLUG enabled, no
> >> >     value is reported.
> >> >
> >> >   * balloon-actual-memory - size of the memory that remains
> >> >     available to the guest after ballooning, as reported by the
> >> >     guest. If the guest has not reported its memory, this value
> >> >     equals to @base-memory + @hot-plug-memory. If ballooning
> >> >     is not enabled, no value is reported.
> >> >
> >> > NOTE:
> >> >
> >> >     Parameter @balloon-actual-memory reports the same as
> >> >     "info balloon" command when ballooning is enabled. The idea
> >> >     to have it in scope of this command(s) comes from
> >> >     https://lists.gnu.org/archive/html/qemu-devel/2012-07/
> msg01472.html.
> [...]
> >> >  hmp-commands-info.hx                               | 17 ++++++++++++
> >> >  hmp.c                                              | 23
> ++++++++++++++++
> >> >  hmp.h                                              |  1 +
> >> >  hw/mem/pc-dimm.c                                   |  6 +++++
> >> >  include/hw/mem/pc-dimm.h                           |  1 +
> >> >  qapi-schema.json                                   | 28
> +++++++++++++++++++
> >> >  qmp.c                                              | 31
> ++++++++++++++++++++++
> >> >  stubs/Makefile.objs                                |  2 +-
> >> >  stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 ++++
> >> >  9 files changed, 113 insertions(+), 1 deletion(-)
> >> >  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (60%)
> >>
> >> No test coverage?
> >>
> >> I prefer to add pairs of QMP / HMP commands in separate patches, QMP
> >> first, for easier review.  This patch seems small enough to tolerate
> >> adding them in a single patch.  But do consider splitting if you have to
> >> respin.
> >
> > The HMP tester scans all 'info' commands so it's not needed for the HMP
> > side.
>
> Correct.  I want a similar test for QMP, but I'm not asking Vadim to
> provide it :)
>

Re: [Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Sorry for the late review, got a bit overwhelmed...
>> 
>> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:
>> 
>> > Commands above provide the following memory information in bytes:
>> >
>> >   * base-memory - amount of unremovable memory specified
>> >     with '-m' option at the start of the QEMU process.
>> >
>> >   * hotpluggable-memory - amount of memory that was hot-plugged.
>> >     If target does not have CONFIG_MEM_HOTPLUG enabled, no
>> >     value is reported.
>> >
>> >   * balloon-actual-memory - size of the memory that remains
>> >     available to the guest after ballooning, as reported by the
>> >     guest. If the guest has not reported its memory, this value
>> >     equals to @base-memory + @hot-plug-memory. If ballooning
>> >     is not enabled, no value is reported.
>> >
>> > NOTE:
>> >
>> >     Parameter @balloon-actual-memory reports the same as
>> >     "info balloon" command when ballooning is enabled. The idea
>> >     to have it in scope of this command(s) comes from
>> >     https://lists.gnu.org/archive/html/qemu-devel/2012-07/msg01472.html.
[...]
>> >  hmp-commands-info.hx                               | 17 ++++++++++++
>> >  hmp.c                                              | 23 ++++++++++++++++
>> >  hmp.h                                              |  1 +
>> >  hw/mem/pc-dimm.c                                   |  6 +++++
>> >  include/hw/mem/pc-dimm.h                           |  1 +
>> >  qapi-schema.json                                   | 28 +++++++++++++++++++
>> >  qmp.c                                              | 31 ++++++++++++++++++++++
>> >  stubs/Makefile.objs                                |  2 +-
>> >  stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 ++++
>> >  9 files changed, 113 insertions(+), 1 deletion(-)
>> >  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (60%)
>> 
>> No test coverage?
>> 
>> I prefer to add pairs of QMP / HMP commands in separate patches, QMP
>> first, for easier review.  This patch seems small enough to tolerate
>> adding them in a single patch.  But do consider splitting if you have to
>> respin.
>
> The HMP tester scans all 'info' commands so it's not needed for the HMP
> side.

Correct.  I want a similar test for QMP, but I'm not asking Vadim to
provide it :)

Re: [Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Sorry for the late review, got a bit overwhelmed...
> 
> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:
> 
> > Commands above provide the following memory information in bytes:
> >
> >   * base-memory - amount of unremovable memory specified
> >     with '-m' option at the start of the QEMU process.
> >
> >   * hotpluggable-memory - amount of memory that was hot-plugged.
> >     If target does not have CONFIG_MEM_HOTPLUG enabled, no
> >     value is reported.
> >
> >   * balloon-actual-memory - size of the memory that remains
> >     available to the guest after ballooning, as reported by the
> >     guest. If the guest has not reported its memory, this value
> >     equals to @base-memory + @hot-plug-memory. If ballooning
> >     is not enabled, no value is reported.
> >
> > NOTE:
> >
> >     Parameter @balloon-actual-memory reports the same as
> >     "info balloon" command when ballooning is enabled. The idea
> >     to have it in scope of this command(s) comes from
> >     https://lists.gnu.org/archive/html/qemu-devel/2012-07/msg01472.html.
> 
> Should we deprecate qmp-query-balloon?  Hmm, see qmp.c below.
> 
> > Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> > Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
> > Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> > Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> > Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Eric Blake <eblake@redhat.com>
> > Cc: qemu-devel@nongnu.org
> > ---
> >
> > v4:
> >  * Commands "info memory" and "query-memory" were renamed
> >    to "info memory-size-summary" and "query-memory-size-summary"
> >    correspondingly.
> >  * Descriptions for both commands as well as MemoryInfo structure
> >    fields were updated/renamed according to
> >    http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg05972.html.
> >  * In MemoryInfo structure following fields are now optional:
> >    hotpluggable-memory and balloon-actual-memory.
> >  * Field "hotpluggable-memory" now not displayed in HMP if target
> >    has no CONFIG_MEM_HOTPLUG enabled.
> >  * Field "balloon-actual-memory" now not displayed in HMP if
> >    ballooning not enabled.
> >  * qapi_free_MemoryInfo() used in order to free corresponding memory
> >    instead of g_free().
> >  * #ifdef CONFIG_MEM_HOTPLUG was removed and replaced with stubs/ approach.
> >    get_exiting_hotpluggable_memory_size() function was introduced in
> >    hw/mem/pc-dimm.c (available for all targets which have CONFIG_MEM_HOTPLUG
> >    enabled). For other targets, there is a stub in stubs/qmp_pc_dimm.c.
> >    In addition, stubs/qmp_pc_dimm_device_list.c was renamed to
> >    stubs/qmp_pc_dimm.c in order to reflect actual source file content.
> >  * Commit message was updated in order to reflect what was changed.
> >
> > v3:
> >  * Use PRIu64 instead of 'lu' when printing results via HMP.
> >  * Report zero hot-plugged memory instead of reporting error
> >    when target architecture has no CONFIG_MEM_HOTPLUG enabled.
> >
> > v2:
> >  * Fixed build for targets which do not have CONFIG_MEM_HOTPLUG
> >    enabled.
> >
> >  hmp-commands-info.hx                               | 17 ++++++++++++
> >  hmp.c                                              | 23 ++++++++++++++++
> >  hmp.h                                              |  1 +
> >  hw/mem/pc-dimm.c                                   |  6 +++++
> >  include/hw/mem/pc-dimm.h                           |  1 +
> >  qapi-schema.json                                   | 28 +++++++++++++++++++
> >  qmp.c                                              | 31 ++++++++++++++++++++++
> >  stubs/Makefile.objs                                |  2 +-
> >  stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 ++++
> >  9 files changed, 113 insertions(+), 1 deletion(-)
> >  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (60%)
> 
> No test coverage?
> 
> I prefer to add pairs of QMP / HMP commands in separate patches, QMP
> first, for easier review.  This patch seems small enough to tolerate
> adding them in a single patch.  But do consider splitting if you have to
> respin.

The HMP tester scans all 'info' commands so it's not needed for the HMP
side.

Dave

> 
> > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > index ba98e581ab..a535960157 100644
> > --- a/hmp-commands-info.hx
> > +++ b/hmp-commands-info.hx
> > @@ -829,6 +829,23 @@ ETEXI
> >          .cmd = hmp_info_vm_generation_id,
> >      },
> >  
> > +STEXI
> > +@item info memory-size-summary
> > +@findex memory-size-summary
> > +Display the amount of initially allocated, hot-plugged (if enabled)
> > +and ballooned (if enabled) memory in bytes.
> > +ETEXI
> > +
> > +    {
> > +        .name       = "memory-size-summary",
> > +        .args_type  = "",
> > +        .params     = "",
> > +        .help       = "show the amount of initially allocated, "
> > +                      "hot-plugged (if enabled) and ballooned (if enabled) "
> > +                      "memory in bytes.",
> > +        .cmd        = hmp_info_memory_size_summary,
> > +    },
> > +
> >  STEXI
> >  @end table
> >  ETEXI
> > diff --git a/hmp.c b/hmp.c
> > index dee40284c1..15f632481c 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -2828,3 +2828,26 @@ void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict)
> >      hmp_handle_error(mon, &err);
> >      qapi_free_GuidInfo(info);
> >  }
> > +
> > +void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *err = NULL;
> > +    MemoryInfo *info = qmp_query_memory_size_summary(&err);
> > +    if (info) {
> > +        monitor_printf(mon, "base-memory: %" PRIu64 "\n",
> > +                       info->base_memory);
> > +
> > +        if (info->has_hotpluggable_memory) {
> > +            monitor_printf(mon, "hotpluggable-memory: %" PRIu64 "\n",
> > +                           info->hotpluggable_memory);
> > +        }
> > +
> > +        if (info->has_balloon_actual_memory) {
> > +            monitor_printf(mon, "balloon-actual-memory: %" PRIu64 "\n",
> > +                           info->balloon_actual_memory);
> > +        }
> 
> Why-do-you-separate-words-by-dashes?  Separating them by spaces has been
> the custom since about the tenth century :)
> 
> According to your cover letter, "balloon actual memory" is the "size of
> the memory that remains available to the guest after ballooning".
> That's not obvious.  It could just as well be the size of the balloon.
> Can we find a more self-explanatory wording that's still short enough?
> 
> > +
> > +        qapi_free_MemoryInfo(info);
> > +    }
> > +    hmp_handle_error(mon, &err);
> > +}
> > diff --git a/hmp.h b/hmp.h
> > index 214b2617e7..8c5398ea7a 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -144,5 +144,6 @@ 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);
> > +void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict);
> >  
> >  #endif
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index b72258e28f..ea81b1384a 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -159,6 +159,12 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
> >      return cap.size;
> >  }
> >  
> > +bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp)
> 
> Why is my hot-pluggable memory exiting, and where's it going?  Or do you
> mean "exciting"?  Or "existing", perhaps?
> 
> > +{
> > +    *mem_size = pc_existing_dimms_capacity(errp);
> > +    return true;
> > +}
> 
> What if pc_existing_dimms_capacity() fails?  Shouldn't you return false
> then?
> 
> Hmm, get_exiting_hotpluggable_memory_size()'s only caller passes
> &error_abort, which means you think it can't fail.  Drop the errp
> parameter and pass &error_abort here then.
> 
> I find "on success store through parameter and return true, on failure
> return false" awkward.  I'd return the size on success and (uint64_t)-1
> on failure.  Matter of taste.
> 
> > +
> >  int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> >  {
> >      MemoryDeviceInfoList ***prev = opaque;
> > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> > index 1e483f2670..738343df32 100644
> > --- a/include/hw/mem/pc-dimm.h
> > +++ b/include/hw/mem/pc-dimm.h
> > @@ -95,6 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
> >  
> >  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
> >  uint64_t pc_existing_dimms_capacity(Error **errp);
> > +bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp);
> >  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> >                           MemoryRegion *mr, uint64_t align, Error **errp);
> >  void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 37c4b95aad..683da8a711 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4327,6 +4327,34 @@
> >    'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
> >              '*unavailable-features': [ 'str' ], 'typename': 'str' } }
> >  
> > +##
> > +# @MemoryInfo:
> > +#
> > +# Actual memory information in bytes.
> > +#
> > +# @base-memory: size of unremovable memory which is specified
> > +#               with '-m size' CLI option.
> 
> The relative clause suggests that there may be other unremovable memory,
> but base-memory reflects only the unremovable memory specified with -m.
> Suggest something like
> 
>      @base-memory: size of "base" memory specified with command line
>                    option -m
> 
> > +#
> > +# @hotpluggable-memory: size of hot-plugged memory.
> 
> I suspect this is actually the size of memory that can be hot-unplugged.
> If true, let's say so:
> 
>    # @hotpluggable-memory: size memory that can be hot-unplugged
> 
> > +#
> > +# @balloon-actual-memory: amount of guest memory available after ballooning.
> > +#
> > +# Since: 2.10.0
> > +##
> > +{ 'struct': 'MemoryInfo',
> > +  'data'  : { 'base-memory': 'int', '*hotpluggable-memory': 'int',
> > +              '*balloon-actual-memory': 'int' } }
> 
> I think these should all be 'size'.
> 
> We suck at using 'size' correctly.  For instance, @BalloonInfo also uses
> 'int' instead of 'size'.  Looks fixable to me.
> 
> Apropos BalloonInfo: you effectively inline it here.  What if we ever
> add something to BalloonInfo?  Wouldn't 'balloon': 'BalloonInfo' be
> cleaner?
> 
> See also my comment after next.
> 
> > +
> > +##
> > +# @query-memory-size-summary:
> > +#
> > +# Return the amount of initially allocated, hot-plugged (if enabled)
> > +# and ballooned (if enabled) memory in bytes.
> > +#
> > +# Since: 2.10.0
> > +##
> > +{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
> > +
> >  ##
> >  # @query-cpu-definitions:
> >  #
> > diff --git a/qmp.c b/qmp.c
> > index 7ee9bcfdcf..a863726ad6 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -712,3 +712,34 @@ ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error **errp)
> >  
> >      return head;
> >  }
> > +
> > +MemoryInfo *qmp_query_memory_size_summary(Error **errp)
> > +{
> > +    MemoryInfo *mem_info = g_malloc0(sizeof(MemoryInfo));
> > +    BalloonInfo *balloon_info;
> > +    uint64_t hotpluggable_memory = 0;
> > +    Error *local_err = NULL;
> > +
> > +    mem_info->base_memory = ram_size;
> > +
> > +    mem_info->has_hotpluggable_memory =
> > +        get_exiting_hotpluggable_memory_size(&hotpluggable_memory,
> > +                                             &error_abort);
> > +    if (mem_info->has_hotpluggable_memory) {
> > +        mem_info->hotpluggable_memory = hotpluggable_memory;
> > +    }
> 
> Why the @hotpluggable_memory middleman?  Why not
> 
>        mem_info->has_hotpluggable_memory =
>            get_exiting_hotpluggable_memory_size(&mem_info->hotpluggable_memory,
>                                                 &error_abort);
> 
> > +
> > +    /* In case if it is not possible to get balloon info, just ignore it. */
> > +    balloon_info = qmp_query_balloon(&local_err);
> > +    if (local_err) {
> > +        mem_info->has_balloon_actual_memory = false;
> > +        error_free(local_err);
> 
> Since we throw away the error, query-memory-size-summary is *not* a
> replacement for query-balloon.
> 
> query-memory-size-summary combines three queries:
> 
> (1) base-memory (query can't fail)
> 
> (2) hotpluggable-memory (query must not fail, i.e. &error_abort)
> 
> (3) balloon-actual-memory (query can fail, and we throw away the error
>     then)
> 
> Including (3) adds a failure mode.  The fact that we sweep the failure
> under the rug doesn't change that.
> 
> Why is this a good idea?
> 
> > +    } else {
> > +        mem_info->has_balloon_actual_memory = true;
> > +        mem_info->balloon_actual_memory = balloon_info->actual;
> > +    }
> > +
> > +    qapi_free_BalloonInfo(balloon_info);
> > +
> > +    return mem_info;
> > +}
> > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > index f5b47bfd74..f7cab5b11c 100644
> > --- a/stubs/Makefile.objs
> > +++ b/stubs/Makefile.objs
> > @@ -32,7 +32,7 @@ stub-obj-y += uuid.o
> >  stub-obj-y += vm-stop.o
> >  stub-obj-y += vmstate.o
> >  stub-obj-$(CONFIG_WIN32) += fd-register.o
> > -stub-obj-y += qmp_pc_dimm_device_list.o
> > +stub-obj-y += qmp_pc_dimm.o
> >  stub-obj-y += target-monitor-defs.o
> >  stub-obj-y += target-get-monitor-def.o
> >  stub-obj-y += pc_madt_cpu_entry.o
> > diff --git a/stubs/qmp_pc_dimm_device_list.c b/stubs/qmp_pc_dimm.c
> > similarity index 60%
> > rename from stubs/qmp_pc_dimm_device_list.c
> > rename to stubs/qmp_pc_dimm.c
> > index def211564d..f50029326e 100644
> > --- a/stubs/qmp_pc_dimm_device_list.c
> > +++ b/stubs/qmp_pc_dimm.c
> > @@ -6,3 +6,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> >  {
> >     return 0;
> >  }
> > +
> > +bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp)
> > +{
> > +    return false;
> > +}
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Sorry for the late review, got a bit overwhelmed...
>
> Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:
>
>> Commands above provide the following memory information in bytes:
>>
>>   * base-memory - amount of unremovable memory specified
>>     with '-m' option at the start of the QEMU process.
>>
>>   * hotpluggable-memory - amount of memory that was hot-plugged.
>>     If target does not have CONFIG_MEM_HOTPLUG enabled, no
>>     value is reported.
>>
>>   * balloon-actual-memory - size of the memory that remains
>>     available to the guest after ballooning, as reported by the
>>     guest. If the guest has not reported its memory, this value
>>     equals to @base-memory + @hot-plug-memory. If ballooning
>>     is not enabled, no value is reported.
>>
>> NOTE:
>>
>>     Parameter @balloon-actual-memory reports the same as
>>     "info balloon" command when ballooning is enabled. The idea
>>     to have it in scope of this command(s) comes from
>>     https://lists.gnu.org/archive/html/qemu-devel/2012-07/msg01472.html.
[...]
>>  9 files changed, 113 insertions(+), 1 deletion(-)
>>  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (60%)
>
> No test coverage?

Maybe no need, because ...

[...]
> query-memory-size-summary combines three queries:
>
> (1) base-memory (query can't fail)
>
> (2) hotpluggable-memory (query must not fail, i.e. &error_abort)
>
> (3) balloon-actual-memory (query can fail, and we throw away the error
>     then)
>
> Including (3) adds a failure mode.  The fact that we sweep the failure
> under the rug doesn't change that.
>
> Why is this a good idea?

... with (3) and its failure mode removed, there's less need for
specific tests.  The (future, hopefully near future) generic smoke test
of queries that can't fail should do.

[...]

Re: [Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

[Markus Armbruster]

Sorry for the late review, got a bit overwhelmed...

Vadim Galitsyn <vadim.galitsyn@profitbricks.com> writes:

> Commands above provide the following memory information in bytes:
>
>   * base-memory - amount of unremovable memory specified
>     with '-m' option at the start of the QEMU process.
>
>   * hotpluggable-memory - amount of memory that was hot-plugged.
>     If target does not have CONFIG_MEM_HOTPLUG enabled, no
>     value is reported.
>
>   * balloon-actual-memory - size of the memory that remains
>     available to the guest after ballooning, as reported by the
>     guest. If the guest has not reported its memory, this value
>     equals to @base-memory + @hot-plug-memory. If ballooning
>     is not enabled, no value is reported.
>
> NOTE:
>
>     Parameter @balloon-actual-memory reports the same as
>     "info balloon" command when ballooning is enabled. The idea
>     to have it in scope of this command(s) comes from
>     https://lists.gnu.org/archive/html/qemu-devel/2012-07/msg01472.html.

Should we deprecate qmp-query-balloon?  Hmm, see qmp.c below.

> Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
> Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
> Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>
> v4:
>  * Commands "info memory" and "query-memory" were renamed
>    to "info memory-size-summary" and "query-memory-size-summary"
>    correspondingly.
>  * Descriptions for both commands as well as MemoryInfo structure
>    fields were updated/renamed according to
>    http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg05972.html.
>  * In MemoryInfo structure following fields are now optional:
>    hotpluggable-memory and balloon-actual-memory.
>  * Field "hotpluggable-memory" now not displayed in HMP if target
>    has no CONFIG_MEM_HOTPLUG enabled.
>  * Field "balloon-actual-memory" now not displayed in HMP if
>    ballooning not enabled.
>  * qapi_free_MemoryInfo() used in order to free corresponding memory
>    instead of g_free().
>  * #ifdef CONFIG_MEM_HOTPLUG was removed and replaced with stubs/ approach.
>    get_exiting_hotpluggable_memory_size() function was introduced in
>    hw/mem/pc-dimm.c (available for all targets which have CONFIG_MEM_HOTPLUG
>    enabled). For other targets, there is a stub in stubs/qmp_pc_dimm.c.
>    In addition, stubs/qmp_pc_dimm_device_list.c was renamed to
>    stubs/qmp_pc_dimm.c in order to reflect actual source file content.
>  * Commit message was updated in order to reflect what was changed.
>
> v3:
>  * Use PRIu64 instead of 'lu' when printing results via HMP.
>  * Report zero hot-plugged memory instead of reporting error
>    when target architecture has no CONFIG_MEM_HOTPLUG enabled.
>
> v2:
>  * Fixed build for targets which do not have CONFIG_MEM_HOTPLUG
>    enabled.
>
>  hmp-commands-info.hx                               | 17 ++++++++++++
>  hmp.c                                              | 23 ++++++++++++++++
>  hmp.h                                              |  1 +
>  hw/mem/pc-dimm.c                                   |  6 +++++
>  include/hw/mem/pc-dimm.h                           |  1 +
>  qapi-schema.json                                   | 28 +++++++++++++++++++
>  qmp.c                                              | 31 ++++++++++++++++++++++
>  stubs/Makefile.objs                                |  2 +-
>  stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 ++++
>  9 files changed, 113 insertions(+), 1 deletion(-)
>  rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (60%)

No test coverage?

I prefer to add pairs of QMP / HMP commands in separate patches, QMP
first, for easier review.  This patch seems small enough to tolerate
adding them in a single patch.  But do consider splitting if you have to
respin.

> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ba98e581ab..a535960157 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -829,6 +829,23 @@ ETEXI
>          .cmd = hmp_info_vm_generation_id,
>      },
>  
> +STEXI
> +@item info memory-size-summary
> +@findex memory-size-summary
> +Display the amount of initially allocated, hot-plugged (if enabled)
> +and ballooned (if enabled) memory in bytes.
> +ETEXI
> +
> +    {
> +        .name       = "memory-size-summary",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show the amount of initially allocated, "
> +                      "hot-plugged (if enabled) and ballooned (if enabled) "
> +                      "memory in bytes.",
> +        .cmd        = hmp_info_memory_size_summary,
> +    },
> +
>  STEXI
>  @end table
>  ETEXI
> diff --git a/hmp.c b/hmp.c
> index dee40284c1..15f632481c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2828,3 +2828,26 @@ void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>      qapi_free_GuidInfo(info);
>  }
> +
> +void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    MemoryInfo *info = qmp_query_memory_size_summary(&err);
> +    if (info) {
> +        monitor_printf(mon, "base-memory: %" PRIu64 "\n",
> +                       info->base_memory);
> +
> +        if (info->has_hotpluggable_memory) {
> +            monitor_printf(mon, "hotpluggable-memory: %" PRIu64 "\n",
> +                           info->hotpluggable_memory);
> +        }
> +
> +        if (info->has_balloon_actual_memory) {
> +            monitor_printf(mon, "balloon-actual-memory: %" PRIu64 "\n",
> +                           info->balloon_actual_memory);
> +        }

Why-do-you-separate-words-by-dashes?  Separating them by spaces has been
the custom since about the tenth century :)

According to your cover letter, "balloon actual memory" is the "size of
the memory that remains available to the guest after ballooning".
That's not obvious.  It could just as well be the size of the balloon.
Can we find a more self-explanatory wording that's still short enough?

> +
> +        qapi_free_MemoryInfo(info);
> +    }
> +    hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 214b2617e7..8c5398ea7a 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -144,5 +144,6 @@ 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);
> +void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index b72258e28f..ea81b1384a 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -159,6 +159,12 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
>      return cap.size;
>  }
>  
> +bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp)

Why is my hot-pluggable memory exiting, and where's it going?  Or do you
mean "exciting"?  Or "existing", perhaps?

> +{
> +    *mem_size = pc_existing_dimms_capacity(errp);
> +    return true;
> +}

What if pc_existing_dimms_capacity() fails?  Shouldn't you return false
then?

Hmm, get_exiting_hotpluggable_memory_size()'s only caller passes
&error_abort, which means you think it can't fail.  Drop the errp
parameter and pass &error_abort here then.

I find "on success store through parameter and return true, on failure
return false" awkward.  I'd return the size on success and (uint64_t)-1
on failure.  Matter of taste.

> +
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque)
>  {
>      MemoryDeviceInfoList ***prev = opaque;
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 1e483f2670..738343df32 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -95,6 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
>  uint64_t pc_existing_dimms_capacity(Error **errp);
> +bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp);
>  void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
>                           MemoryRegion *mr, uint64_t align, Error **errp);
>  void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 37c4b95aad..683da8a711 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4327,6 +4327,34 @@
>    'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
>              '*unavailable-features': [ 'str' ], 'typename': 'str' } }
>  
> +##
> +# @MemoryInfo:
> +#
> +# Actual memory information in bytes.
> +#
> +# @base-memory: size of unremovable memory which is specified
> +#               with '-m size' CLI option.

The relative clause suggests that there may be other unremovable memory,
but base-memory reflects only the unremovable memory specified with -m.
Suggest something like

     @base-memory: size of "base" memory specified with command line
                   option -m

> +#
> +# @hotpluggable-memory: size of hot-plugged memory.

I suspect this is actually the size of memory that can be hot-unplugged.
If true, let's say so:

   # @hotpluggable-memory: size memory that can be hot-unplugged

> +#
> +# @balloon-actual-memory: amount of guest memory available after ballooning.
> +#
> +# Since: 2.10.0
> +##
> +{ 'struct': 'MemoryInfo',
> +  'data'  : { 'base-memory': 'int', '*hotpluggable-memory': 'int',
> +              '*balloon-actual-memory': 'int' } }

I think these should all be 'size'.

We suck at using 'size' correctly.  For instance, @BalloonInfo also uses
'int' instead of 'size'.  Looks fixable to me.

Apropos BalloonInfo: you effectively inline it here.  What if we ever
add something to BalloonInfo?  Wouldn't 'balloon': 'BalloonInfo' be
cleaner?

See also my comment after next.

> +
> +##
> +# @query-memory-size-summary:
> +#
> +# Return the amount of initially allocated, hot-plugged (if enabled)
> +# and ballooned (if enabled) memory in bytes.
> +#
> +# Since: 2.10.0
> +##
> +{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
> +
>  ##
>  # @query-cpu-definitions:
>  #
> diff --git a/qmp.c b/qmp.c
> index 7ee9bcfdcf..a863726ad6 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -712,3 +712,34 @@ ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error **errp)
>  
>      return head;
>  }
> +
> +MemoryInfo *qmp_query_memory_size_summary(Error **errp)
> +{
> +    MemoryInfo *mem_info = g_malloc0(sizeof(MemoryInfo));
> +    BalloonInfo *balloon_info;
> +    uint64_t hotpluggable_memory = 0;
> +    Error *local_err = NULL;
> +
> +    mem_info->base_memory = ram_size;
> +
> +    mem_info->has_hotpluggable_memory =
> +        get_exiting_hotpluggable_memory_size(&hotpluggable_memory,
> +                                             &error_abort);
> +    if (mem_info->has_hotpluggable_memory) {
> +        mem_info->hotpluggable_memory = hotpluggable_memory;
> +    }

Why the @hotpluggable_memory middleman?  Why not

       mem_info->has_hotpluggable_memory =
           get_exiting_hotpluggable_memory_size(&mem_info->hotpluggable_memory,
                                                &error_abort);

> +
> +    /* In case if it is not possible to get balloon info, just ignore it. */
> +    balloon_info = qmp_query_balloon(&local_err);
> +    if (local_err) {
> +        mem_info->has_balloon_actual_memory = false;
> +        error_free(local_err);

Since we throw away the error, query-memory-size-summary is *not* a
replacement for query-balloon.

query-memory-size-summary combines three queries:

(1) base-memory (query can't fail)

(2) hotpluggable-memory (query must not fail, i.e. &error_abort)

(3) balloon-actual-memory (query can fail, and we throw away the error
    then)

Including (3) adds a failure mode.  The fact that we sweep the failure
under the rug doesn't change that.

Why is this a good idea?

> +    } else {
> +        mem_info->has_balloon_actual_memory = true;
> +        mem_info->balloon_actual_memory = balloon_info->actual;
> +    }
> +
> +    qapi_free_BalloonInfo(balloon_info);
> +
> +    return mem_info;
> +}
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index f5b47bfd74..f7cab5b11c 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -32,7 +32,7 @@ stub-obj-y += uuid.o
>  stub-obj-y += vm-stop.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_WIN32) += fd-register.o
> -stub-obj-y += qmp_pc_dimm_device_list.o
> +stub-obj-y += qmp_pc_dimm.o
>  stub-obj-y += target-monitor-defs.o
>  stub-obj-y += target-get-monitor-def.o
>  stub-obj-y += pc_madt_cpu_entry.o
> diff --git a/stubs/qmp_pc_dimm_device_list.c b/stubs/qmp_pc_dimm.c
> similarity index 60%
> rename from stubs/qmp_pc_dimm_device_list.c
> rename to stubs/qmp_pc_dimm.c
> index def211564d..f50029326e 100644
> --- a/stubs/qmp_pc_dimm_device_list.c
> +++ b/stubs/qmp_pc_dimm.c
> @@ -6,3 +6,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
>  {
>     return 0;
>  }
> +
> +bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp)
> +{
> +    return false;
> +}

[Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

[Vadim Galitsyn]

Commands above provide the following memory information in bytes:

  * base-memory - amount of unremovable memory specified
    with '-m' option at the start of the QEMU process.

  * hotpluggable-memory - amount of memory that was hot-plugged.
    If target does not have CONFIG_MEM_HOTPLUG enabled, no
    value is reported.

  * balloon-actual-memory - size of the memory that remains
    available to the guest after ballooning, as reported by the
    guest. If the guest has not reported its memory, this value
    equals to @base-memory + @hot-plug-memory. If ballooning
    is not enabled, no value is reported.

NOTE:

    Parameter @balloon-actual-memory reports the same as
    "info balloon" command when ballooning is enabled. The idea
    to have it in scope of this command(s) comes from
    https://lists.gnu.org/archive/html/qemu-devel/2012-07/msg01472.html.

Signed-off-by: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com>
Signed-off-by: Mohammed Gamal <mohammed.gamal@profitbricks.com>
Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
Reviewed-by: Eugene Crosser <evgenii.cherkashin@profitbricks.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org
---

v4:
 * Commands "info memory" and "query-memory" were renamed
   to "info memory-size-summary" and "query-memory-size-summary"
   correspondingly.
 * Descriptions for both commands as well as MemoryInfo structure
   fields were updated/renamed according to
   http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg05972.html.
 * In MemoryInfo structure following fields are now optional:
   hotpluggable-memory and balloon-actual-memory.
 * Field "hotpluggable-memory" now not displayed in HMP if target
   has no CONFIG_MEM_HOTPLUG enabled.
 * Field "balloon-actual-memory" now not displayed in HMP if
   ballooning not enabled.
 * qapi_free_MemoryInfo() used in order to free corresponding memory
   instead of g_free().
 * #ifdef CONFIG_MEM_HOTPLUG was removed and replaced with stubs/ approach.
   get_exiting_hotpluggable_memory_size() function was introduced in
   hw/mem/pc-dimm.c (available for all targets which have CONFIG_MEM_HOTPLUG
   enabled). For other targets, there is a stub in stubs/qmp_pc_dimm.c.
   In addition, stubs/qmp_pc_dimm_device_list.c was renamed to
   stubs/qmp_pc_dimm.c in order to reflect actual source file content.
 * Commit message was updated in order to reflect what was changed.

v3:
 * Use PRIu64 instead of 'lu' when printing results via HMP.
 * Report zero hot-plugged memory instead of reporting error
   when target architecture has no CONFIG_MEM_HOTPLUG enabled.

v2:
 * Fixed build for targets which do not have CONFIG_MEM_HOTPLUG
   enabled.

 hmp-commands-info.hx                               | 17 ++++++++++++
 hmp.c                                              | 23 ++++++++++++++++
 hmp.h                                              |  1 +
 hw/mem/pc-dimm.c                                   |  6 +++++
 include/hw/mem/pc-dimm.h                           |  1 +
 qapi-schema.json                                   | 28 +++++++++++++++++++
 qmp.c                                              | 31 ++++++++++++++++++++++
 stubs/Makefile.objs                                |  2 +-
 stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} |  5 ++++
 9 files changed, 113 insertions(+), 1 deletion(-)
 rename stubs/{qmp_pc_dimm_device_list.c => qmp_pc_dimm.c} (60%)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ba98e581ab..a535960157 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -829,6 +829,23 @@ ETEXI
         .cmd = hmp_info_vm_generation_id,
     },
 
+STEXI
+@item info memory-size-summary
+@findex memory-size-summary
+Display the amount of initially allocated, hot-plugged (if enabled)
+and ballooned (if enabled) memory in bytes.
+ETEXI
+
+    {
+        .name       = "memory-size-summary",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show the amount of initially allocated, "
+                      "hot-plugged (if enabled) and ballooned (if enabled) "
+                      "memory in bytes.",
+        .cmd        = hmp_info_memory_size_summary,
+    },
+
 STEXI
 @end table
 ETEXI
diff --git a/hmp.c b/hmp.c
index dee40284c1..15f632481c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2828,3 +2828,26 @@ void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
     qapi_free_GuidInfo(info);
 }
+
+void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    MemoryInfo *info = qmp_query_memory_size_summary(&err);
+    if (info) {
+        monitor_printf(mon, "base-memory: %" PRIu64 "\n",
+                       info->base_memory);
+
+        if (info->has_hotpluggable_memory) {
+            monitor_printf(mon, "hotpluggable-memory: %" PRIu64 "\n",
+                           info->hotpluggable_memory);
+        }
+
+        if (info->has_balloon_actual_memory) {
+            monitor_printf(mon, "balloon-actual-memory: %" PRIu64 "\n",
+                           info->balloon_actual_memory);
+        }
+
+        qapi_free_MemoryInfo(info);
+    }
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 214b2617e7..8c5398ea7a 100644
--- a/hmp.h
+++ b/hmp.h
@@ -144,5 +144,6 @@ 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);
+void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index b72258e28f..ea81b1384a 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -159,6 +159,12 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
     return cap.size;
 }
 
+bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp)
+{
+    *mem_size = pc_existing_dimms_capacity(errp);
+    return true;
+}
+
 int qmp_pc_dimm_device_list(Object *obj, void *opaque)
 {
     MemoryDeviceInfoList ***prev = opaque;
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 1e483f2670..738343df32 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -95,6 +95,7 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
 int qmp_pc_dimm_device_list(Object *obj, void *opaque);
 uint64_t pc_existing_dimms_capacity(Error **errp);
+bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp);
 void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
                          MemoryRegion *mr, uint64_t align, Error **errp);
 void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
diff --git a/qapi-schema.json b/qapi-schema.json
index 37c4b95aad..683da8a711 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4327,6 +4327,34 @@
   'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
             '*unavailable-features': [ 'str' ], 'typename': 'str' } }
 
+##
+# @MemoryInfo:
+#
+# Actual memory information in bytes.
+#
+# @base-memory: size of unremovable memory which is specified
+#               with '-m size' CLI option.
+#
+# @hotpluggable-memory: size of hot-plugged memory.
+#
+# @balloon-actual-memory: amount of guest memory available after ballooning.
+#
+# Since: 2.10.0
+##
+{ 'struct': 'MemoryInfo',
+  'data'  : { 'base-memory': 'int', '*hotpluggable-memory': 'int',
+              '*balloon-actual-memory': 'int' } }
+
+##
+# @query-memory-size-summary:
+#
+# Return the amount of initially allocated, hot-plugged (if enabled)
+# and ballooned (if enabled) memory in bytes.
+#
+# Since: 2.10.0
+##
+{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
+
 ##
 # @query-cpu-definitions:
 #
diff --git a/qmp.c b/qmp.c
index 7ee9bcfdcf..a863726ad6 100644
--- a/qmp.c
+++ b/qmp.c
@@ -712,3 +712,34 @@ ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error **errp)
 
     return head;
 }
+
+MemoryInfo *qmp_query_memory_size_summary(Error **errp)
+{
+    MemoryInfo *mem_info = g_malloc0(sizeof(MemoryInfo));
+    BalloonInfo *balloon_info;
+    uint64_t hotpluggable_memory = 0;
+    Error *local_err = NULL;
+
+    mem_info->base_memory = ram_size;
+
+    mem_info->has_hotpluggable_memory =
+        get_exiting_hotpluggable_memory_size(&hotpluggable_memory,
+                                             &error_abort);
+    if (mem_info->has_hotpluggable_memory) {
+        mem_info->hotpluggable_memory = hotpluggable_memory;
+    }
+
+    /* In case if it is not possible to get balloon info, just ignore it. */
+    balloon_info = qmp_query_balloon(&local_err);
+    if (local_err) {
+        mem_info->has_balloon_actual_memory = false;
+        error_free(local_err);
+    } else {
+        mem_info->has_balloon_actual_memory = true;
+        mem_info->balloon_actual_memory = balloon_info->actual;
+    }
+
+    qapi_free_BalloonInfo(balloon_info);
+
+    return mem_info;
+}
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f5b47bfd74..f7cab5b11c 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -32,7 +32,7 @@ stub-obj-y += uuid.o
 stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
-stub-obj-y += qmp_pc_dimm_device_list.o
+stub-obj-y += qmp_pc_dimm.o
 stub-obj-y += target-monitor-defs.o
 stub-obj-y += target-get-monitor-def.o
 stub-obj-y += pc_madt_cpu_entry.o
diff --git a/stubs/qmp_pc_dimm_device_list.c b/stubs/qmp_pc_dimm.c
similarity index 60%
rename from stubs/qmp_pc_dimm_device_list.c
rename to stubs/qmp_pc_dimm.c
index def211564d..f50029326e 100644
--- a/stubs/qmp_pc_dimm_device_list.c
+++ b/stubs/qmp_pc_dimm.c
@@ -6,3 +6,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
 {
    return 0;
 }
+
+bool get_exiting_hotpluggable_memory_size(uint64_t *mem_size, Error **errp)
+{
+    return false;
+}
-- 
2.13.1.394.g41dd433

[Qemu-devel] [PATCH v4] hmp, qmp: introduce memory-size-summary commands

[Vadim Galitsyn]

Hi Guys,

This thread is a continuation of discussion started at http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg05972.html.

Vadim