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!
>
> [...]
>