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

[Markus Armbruster]

Vadim Galitsyn <address@hidden> 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 <address@hidden>
> Signed-off-by: Mohammed Gamal <address@hidden>
> Signed-off-by: Eduardo Otubo <address@hidden>
> Signed-off-by: Vadim Galitsyn <address@hidden>
> Reviewed-by: Eugene Crosser <address@hidden>
> Cc: Dr. David Alan Gilbert <address@hidden>
> Cc: Markus Armbruster <address@hidden>
> Cc: Igor Mammedov <address@hidden>
> Cc: Eric Blake <address@hidden>
> Cc: address@hidden
> ---
>  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;
> +}