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

Commit Message

Maciej S. Szmigiero July 20, 2023, 10:13 a.m. UTC

From: "Maciej S. Szmigiero" <[e-mail]>

Used by the hv-balloon driver to report its provided memory state
information.

Co-developed-by: David Hildenbrand <[e-mail]>
Signed-off-by: Maciej S. Szmigiero <[e-mail]>
---
 hw/core/machine-hmp-cmds.c | 15 +++++++++++++++
 qapi/machine.json          | 39 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 52 insertions(+), 2 deletions(-)

Comments

Markus Armbruster July 24, 2023, 11:37 a.m. UTC | #1

"Maciej S. Szmigiero" <[e-mail]> writes:

> From: "Maciej S. Szmigiero" <[e-mail]>
>
> Used by the hv-balloon driver to report its provided memory state
> information.
>
> Co-developed-by: David Hildenbrand <[e-mail]>
> Signed-off-by: Maciej S. Szmigiero <[e-mail]>
> ---
>  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:

This is the only occurence of MEMORY_DEVICE_INFO_KIND_HV_BALLOON at the
end of the series.  Where are MemoryDeviceInfo with this union tag
created?

> +                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();
>              }

[...]

Maciej S. Szmigiero July 24, 2023, 11:43 a.m. UTC | #2

On 24.07.2023 13:37, Markus Armbruster wrote:
> "Maciej S. Szmigiero" <[e-mail]> writes:
> 
>> From: "Maciej S. Szmigiero" <[e-mail]>
>>
>> Used by the hv-balloon driver to report its provided memory state
>> information.
>>
>> Co-developed-by: David Hildenbrand <[e-mail]>
>> Signed-off-by: Maciej S. Szmigiero <[e-mail]>
>> ---
>>   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:
> 
> This is the only occurence of MEMORY_DEVICE_INFO_KIND_HV_BALLOON at the
> end of the series.  Where are MemoryDeviceInfo with this union tag
> created?
> 

 From patch 6:
+static void hv_balloon_md_fill_device_info(const MemoryDeviceState *md,
+                                           MemoryDeviceInfo *info)
+{
+    HvBalloonDeviceInfo *hi = g_new0(HvBalloonDeviceInfo, 1);
+    const HvBalloon *balloon = HV_BALLOON(md);
+    DeviceState *dev = DEVICE(md);
+
+    if (dev->id) {
+        hi->id = g_strdup(dev->id);
+    }
+
+    if (balloon->hostmem) {
+        hi->memdev = object_get_canonical_path(OBJECT(balloon->hostmem));
+        hi->memaddr = balloon->addr;
+        hi->has_memaddr = true;
+        hi->max_size = memory_region_size(balloon->mr);
+        /* TODO: expose current provided size or something else? */
+    } else {
+        hi->max_size = 0;
+    }
+
+    info->u.hv_balloon.data = hi;
+    info->type = MEMORY_DEVICE_INFO_KIND_HV_BALLOON;

                   ^ here.

Thanks,
Maciej

Markus Armbruster July 25, 2023, 8:25 a.m. UTC | #3

"Maciej S. Szmigiero" <[e-mail]> writes:

> From: "Maciej S. Szmigiero" <[e-mail]>
>
> Used by the hv-balloon driver to report its provided memory state
> information.
>
> Co-developed-by: David Hildenbrand <[e-mail]>
> Signed-off-by: Maciej S. Szmigiero <[e-mail]>
> ---
>  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.

Markus Armbruster July 25, 2023, 8:26 a.m. UTC | #4

"Maciej S. Szmigiero" <[e-mail]> writes:

> On 24.07.2023 13:37, Markus Armbruster wrote:
>> "Maciej S. Szmigiero" <[e-mail]> writes:
>> 
>>> From: "Maciej S. Szmigiero" <[e-mail]>
>>>
>>> Used by the hv-balloon driver to report its provided memory state
>>> information.
>>>
>>> Co-developed-by: David Hildenbrand <[e-mail]>
>>> Signed-off-by: Maciej S. Szmigiero <[e-mail]>
>>> ---
>>>   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:
>>
>> This is the only occurence of MEMORY_DEVICE_INFO_KIND_HV_BALLOON at the
>> end of the series.  Where are MemoryDeviceInfo with this union tag
>> created?
>
> From patch 6:
[...]

I messed around with git, and managed to confuse myself.  Thanks for
your help!

Maciej S. Szmigiero July 25, 2023, 6:13 p.m. UTC | #5

On 25.07.2023 10:25, Markus Armbruster wrote:
> "Maciej S. Szmigiero" <[e-mail]> writes:
> 
>> From: "Maciej S. Szmigiero" <[e-mail]>
>>
>> Used by the hv-balloon driver to report its provided memory state
>> information.
>>
>> Co-developed-by: David Hildenbrand <[e-mail]>
>> Signed-off-by: Maciej S. Szmigiero <[e-mail]>
>> ---
>>   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.

Will change TBD to 8.2 then.

>> +##
>> +{ '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.

Will make use of your suggestion in the other e-mail to refactor
it like this:
> 0. The driver with QMP stuff omitted / stubbed out
> 
> 1. Enable query-memory-devices
> 
> 2. Add HV_BALLOON_STATUS_REPORT event

Thanks,
Maciej

Patch

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
+##
+{ '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'
           }
 }