Re: [PATCH v6 4/6] qapi: Add HvBalloonDeviceInfo sub-type to MemoryDeviceInfo

[Markus Armbruster <armbru@redhat.com>]

"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> Used by the hv-balloon driver to report its provided memory state
> information.
>
> Co-developed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> ---
>  hw/core/machine-hmp-cmds.c | 15 +++++++++++++++
>  qapi/machine.json          | 39 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
> index c3e55ef9e9cd..7b06ed35decb 100644
> --- a/hw/core/machine-hmp-cmds.c
> +++ b/hw/core/machine-hmp-cmds.c
> @@ -247,6 +247,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
>      MemoryDeviceInfo *value;
>      PCDIMMDeviceInfo *di;
>      SgxEPCDeviceInfo *se;
> +    HvBalloonDeviceInfo *hi;
>  
>      for (info = info_list; info; info = info->next) {
>          value = info->value;
> @@ -304,6 +305,20 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
>                  monitor_printf(mon, "  node: %" PRId64 "\n", se->node);
>                  monitor_printf(mon, "  memdev: %s\n", se->memdev);
>                  break;
> +            case MEMORY_DEVICE_INFO_KIND_HV_BALLOON:
> +                hi = value->u.hv_balloon.data;
> +                monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
> +                               MemoryDeviceInfoKind_str(value->type),
> +                               hi->id ? hi->id : "");
> +                if (hi->has_memaddr) {
> +                    monitor_printf(mon, "  memaddr: 0x%" PRIx64 "\n",
> +                                   hi->memaddr);
> +                }
> +                monitor_printf(mon, "  max-size: %" PRIu64 "\n", hi->max_size);
> +                if (hi->memdev) {
> +                    monitor_printf(mon, "  memdev: %s\n", hi->memdev);
> +                }
> +                break;
>              default:
>                  g_assert_not_reached();
>              }
> diff --git a/qapi/machine.json b/qapi/machine.json
> index a08b6576cac6..5ede977cf2bc 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1265,6 +1265,29 @@
>            }
>  }
>  
> +##
> +# @HvBalloonDeviceInfo:
> +#
> +# hv-balloon provided memory state information
> +#
> +# @id: device's ID
> +#
> +# @memaddr: physical address in memory, where device is mapped
> +#
> +# @max-size: the maximum size of memory that the device can provide
> +#
> +# @memdev: memory backend linked with device
> +#
> +# Since: TBD

I understand why you put in TBD here (aiming for a moving target is a
hassle), but patches not marked RFC should have no known issues that
should be fixed before merging them.

> +##
> +{ 'struct': 'HvBalloonDeviceInfo',
> +  'data': { '*id': 'str',
> +            '*memaddr': 'size',
> +            'max-size': 'size',
> +            '*memdev': 'str'
> +          }
> +}
> +
>  ##
>  # @MemoryDeviceInfoKind:
>  #
> @@ -1276,10 +1299,13 @@
>  #
>  # @sgx-epc: since 6.2.
>  #
> +# @hv-balloon: since TBD.
> +#
>  # Since: 2.1
>  ##
>  { 'enum': 'MemoryDeviceInfoKind',
> -  'data': [ 'dimm', 'nvdimm', 'virtio-pmem', 'virtio-mem', 'sgx-epc' ] }
> +  'data': [ 'dimm', 'nvdimm', 'virtio-pmem', 'virtio-mem', 'sgx-epc',
> +            'hv-balloon' ] }
>  
>  ##
>  # @PCDIMMDeviceInfoWrapper:
> @@ -1313,6 +1339,14 @@
>  { 'struct': 'SgxEPCDeviceInfoWrapper',
>    'data': { 'data': 'SgxEPCDeviceInfo' } }
>  
> +##
> +# @HvBalloonDeviceInfoWrapper:
> +#
> +# Since: TBD
> +##
> +{ 'struct': 'HvBalloonDeviceInfoWrapper',
> +  'data': { 'data': 'HvBalloonDeviceInfo' } }
> +
>  ##
>  # @MemoryDeviceInfo:
>  #
> @@ -1327,7 +1361,8 @@
>              'nvdimm': 'PCDIMMDeviceInfoWrapper',
>              'virtio-pmem': 'VirtioPMEMDeviceInfoWrapper',
>              'virtio-mem': 'VirtioMEMDeviceInfoWrapper',
> -            'sgx-epc': 'SgxEPCDeviceInfoWrapper'
> +            'sgx-epc': 'SgxEPCDeviceInfoWrapper',
> +            'hv-balloon': 'HvBalloonDeviceInfoWrapper'
>            }
>  }
>  

The organization of the series feels a bit awkward.

In this patch, you define QAPI types and add a bit of code reading them,
but the code creating them is left for later.

In the next patch, you define a QMP event, but the code sending it is
left for later.

In the final, huge patch, you fill in the blanks.

Adding definitions before their uses can be the least awkward solution.
But then the commit messages should point out that uses come later.
Describing these future uses briefly may be necessary to help the reader
understand the patch on its own.

Perhaps you can restructure the series instead.