qdevices: refreshes the device_add params for QMP hotplug #4053

mcasquer · 2025-01-14T09:49:56Z

qdevices: refreshes the device_add params for QMP hotplug

device_add no longer accept string values in QMP command.
Casts the node and requested-size values to integer before
doing the QMP hotplug.

Signed-off-by: mcasquer mcasquer@redhat.com
ID: 3253

virttest/qemu_devices/qcontainer.py

yanan-fu · 2025-01-14T11:15:11Z

The dimm device need the node parameter while virtio-mem be updated in the this patch only.

mcasquer · 2025-01-14T11:55:52Z

The dimm device need the node parameter while virtio-mem be updated in the this patch only.

@yanan-fu so you mean I should only send the patch for the 'node' option?

mcasquer · 2025-01-14T11:57:24Z

Here the affected test cases now pass (hotplug_virtio_mem includes the needed changes in tp-qemu)

 (1/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.nvdimm.nvdimm_hotplug.q35: STARTED
 (1/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.nvdimm.nvdimm_hotplug.q35: PASS (128.44 s)
 (2/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.hotplug_memory.after.guest_reboot.hotplug.backend_file.policy_default.two.q35: STARTED
 (2/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.hotplug_memory.after.guest_reboot.hotplug.backend_file.policy_default.two.q35: PASS (173.89 s)
 (3/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.hotplug_virtio_mem.q35: STARTED
 (3/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.hotplug_virtio_mem.q35: PASS (142.71 s)
RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0

yanan-fu · 2025-01-15T03:34:18Z

The dimm device need the node parameter while virtio-mem be updated in the this patch only.

@yanan-fu so you mean I should only send the patch for the 'node' option?

No, i do not mean that, i missed the L3507~3508 in the pre review, it already be added for dimm device, that's good ^^

yanan-fu · 2025-01-15T06:03:43Z

IMO, it is more reasonable to change it in the qdevices instead of the qcontainer.
Currently, the qemu command line style is correct, but the hotplug parameters need be updated, I prefer to use the _refresh_hotplug_props

mcasquer · 2025-01-15T07:31:19Z

IMO, it is more reasonable to change it in the qdevices instead of the qcontainer.
Currently, the qemu command line style is correct, but the hotplug parameters need be updated, I prefer to use the _refresh_hotplug_props

@yanan-fu but should I keep the Flags implementation? Or just cast to int the 'node' and 'requested-size' values?

yanan-fu · 2025-01-16T06:09:28Z

IMO, it is more reasonable to change it in the qdevices instead of the qcontainer.
Currently, the qemu command line style is correct, but the hotplug parameters need be updated, I prefer to use the _refresh_hotplug_props

@yanan-fu but should I keep the Flags implementation? Or just cast to int the 'node' and 'requested-size' values?

Yes, but not the Flag implementation in this PR, you can refer the approach for QObject that introduce different qmp method based on qemu version.

mcasquer · 2025-01-16T12:52:12Z

@yanan-fu I see the functions using _refresh_hotplug_props, the one from Memory class and also the one from QObject are for the object_add, but not device_add, what am I missing here?

PaulYuuu · 2025-01-17T01:04:20Z

FYI, this is not the only change for mem, QEMU aligns the create_cli_devices with qmp_device_add, for most opts, we should follow json format, see: [PULL,7/8] qdev-monitor: avoid QemuOpts in QMP device_add and [PULL,8/8] vl: use qmp_device_add() in qemu_create_cli_devices().

@yanglei-rh ping me that the NIC device opt mq also from OnOffAuto to Bool

yanan-fu · 2025-01-17T02:50:23Z

@yanan-fu I see the functions using _refresh_hotplug_props, the one from Memory class and also the one from QObject are for the object_add, but not device_add, what am I missing here?

I means you can refer the approach, and QDevice need such logic.

yanan-fu · 2025-01-17T02:52:45Z

FYI, this is not the only change for mem, QEMU aligns the create_cli_devices with qmp_device_add, for most opts, we should follow json format, see: [PULL,7/8] qdev-monitor: avoid QemuOpts in QMP device_add and [PULL,8/8] vl: use qmp_device_add() in qemu_create_cli_devices().

@yanglei-rh ping me that the NIC device opt mq also from OnOffAuto to Bool

Yes, that's the reason that i think it need be changed in the QDevice side for the qmp_hotplug instead of current approach.

YongxueHong · 2025-01-17T02:54:10Z

Hi @nickzhq
CC you since this change may affect the JSON format implementation you worked on.
Thanks.

yanan-fu · 2025-01-17T02:57:39Z

Hi @nickzhq CC you since this change may affect the JSON format implementation you worked on. Thanks.

IMO, no effect for the json format which be used to generate qemu command line, this one is for the hotplug.

YongxueHong · 2025-01-17T03:25:29Z

Hi @nickzhq CC you since this change may affect the JSON format implementation you worked on. Thanks.

IMO, no effect for the json format which be used to generate qemu command line, this one is for the hotplug.

Hi,
Checked the changed code, We changed the def memory_define_by_params(self, params, name): this part will be used for creating the memory device of the VM and then we could use it to create the command line(e.g: json format) and hotplug it.
The related code snippets:

avocado-vt/virttest/qemu_vm.py

Lines 1146 to 1158 in fef8380

    
           for name in params.objects("mem_devs"): 
        
               dev = devices.memory_define_by_params(params, name) 
        
               for ele in dev: 
        
                   if ( 
        
                       isinstance(ele, qdevices.Memory) 
        
                       and ele.params["backend"] == "memory-backend-epc" 
        
                   ): 
        
                       sgx_epc_memdev_id[name] = ele.get_qid() 
        
                   set_cmdline_format_by_cfg( 
        
                       ele, self._get_cmdline_format_cfg(), "mem_devs" 
        
                   ) 
        
               devs.extend(dev)

Please correct me if I am wrong. CC @nickzhq
Thanks.

nickzhq · 2025-01-17T05:45:07Z

Hi @nickzhq CC you since this change may affect the JSON format implementation you worked on. Thanks.

IMO, no effect for the json format which be used to generate qemu command line, this one is for the hotplug.

Hi, Checked the changed code, We changed the def memory_define_by_params(self, params, name): this part will be used for creating the memory device of the VM and then we could use it to create the command line(e.g: json format) and hotplug it. The related code snippets:

avocado-vt/virttest/qemu_vm.py

Lines 1146 to 1158 in fef8380

for name in params.objects("mem_devs"):

dev = devices.memory_define_by_params(params, name)

for ele in dev:

if (

isinstance(ele, qdevices.Memory)

and ele.params["backend"] == "memory-backend-epc"

):

sgx_epc_memdev_id[name] = ele.get_qid()

set_cmdline_format_by_cfg(

ele, self._get_cmdline_format_cfg(), "mem_devs"

)

devs.extend(dev)

Please correct me if I am wrong. CC @nickzhq
Thanks.

Hello @YongxueHong @yanan-fu ,
After I checked the code in framework and based on my understanding, the function memory_define_by_params is called by both qemu command line and hotplug via QMP.
The key issue is that the current framework does NOT distinguish the render of qemu-command-line from definition of qemu-command-line. ( This issue may be noticed by us on pervious meeting. )
So far, the framework handling the render of qemu-command-line is based on the scenarios, such as "the _cmdline_json handles the qemu-command-line render". Therefore, the hotplug scenario should be handled in hotplug_qmp?

#4050 has the same issue with this patch.
Thanks!

yanan-fu · 2025-01-17T06:52:46Z

@YongxueHong When i talking about it is not related with json format, i do not mean the solution in the current MR now. I suggest change QDevice, for qmp hotplug, it is not related with the json format.

And json_format works well with both test result and code review, take node as an example, refer:

avocado-vt/virttest/qemu_devices/qdevices.py

Line 1280 in fef8380

"node",

device_add no longer accept string values in QMP command. Casts the node and requested-size values to integer before doing the QMP hotplug. Signed-off-by: mcasquer <mcasquer@redhat.com>

mcasquer · 2025-01-17T09:35:55Z

I means you can refer the approach, and QDevice need such logic.

@yanan-fu I was misunderstanding the idea, code updated ! :)

mcasquer · 2025-01-17T10:29:52Z

 (1/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.nvdimm.nvdimm_hotplug.q35: STARTED
 (1/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.nvdimm.nvdimm_hotplug.q35: PASS (126.47 s)
 (2/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.hotplug_memory.after.guest_reboot.hotplug.backend_file.policy_default.two.q35: STARTED
 (2/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.hotplug_memory.after.guest_reboot.hotplug.backend_file.policy_default.two.q35: PASS (235.49 s)
 (3/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.hotplug_virtio_mem.q35: STARTED
 (3/3) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.hotplug_virtio_mem.q35: PASS (143.17 s)
RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0

mcasquer · 2025-01-17T10:30:10Z

@yanan-fu please could you review again this PR? Thanks !

yanan-fu · 2025-01-17T11:16:47Z

virttest/qemu_devices/qdevices.py

+        for option in ["node", "requested-size"]:
+            value = params.get(option)
+            if value:
+                params[option] = int(value)


The logic with L1165~L1168 belong to the refresh_hotplug_procs function. And for node, it already be covered in the Dimm class.

yanan-fu · 2025-01-17T11:31:53Z

@mcasquer could you please also considering the numa_queues as i commented in #4050

And, to be more broadly, I guess all the conversion in the _cmdline_json can be covered.

By now, I do not have a very clear idea abut if it reasonable to combine the parameter conversion in _cmdline_json and hotplug_qmp yet. Feel free to share your opinion.

mcasquer · 2025-01-17T11:51:30Z

@mcasquer could you please also considering the numa_queues as i commented in #4050

And, to be more broadly, I guess all the conversion in the _cmdline_json can be covered.

By now, I do not have a very clear idea abut if it reasonable to combine the parameter conversion in _cmdline_json and hotplug_qmp yet. Feel free to share your opinion.

So @yanan-fu all the following params should be covered right?:

avocado-vt/virttest/qemu_devices/qdevices.py

Lines 1263 to 1287 in 44d5e2b

    
           elif key in ( 
        
               "physical_block_size", 
        
               "logical_block_size", 
        
               "bootindex", 
        
               "max_sectors", 
        
               "num_queues", 
        
               "virtqueue_size", 
        
               "discard_granularity", 
        
               "period", 
        
               "max-bytes", 
        
               "max-write-zeroes-sectors", 
        
               "queue-size", 
        
               "max-discard-sectors", 
        
               "num-queues", 
        
               "host_mtu", 
        
               "speed", 
        
               "vectors", 
        
               "node", 
        
               "events", 
        
               "min_io_size", 
        
               "opt_io_size", 
        
               "guest-stats-polling-interval", 
        
               "acpi-index", 
        
               "aw-bits", 
        
           ):

About the _cmdline_json discussion I think I am not getting the main point, the idea is to do only the conversion in one place? Or what's the idea? Thanks !

yanan-fu · 2025-01-17T13:22:01Z

@mcasquer Let me make myself more clear:

refer to the product change, qmp_device_add use the same opt type with the json format cli. So i think we can apply this change to all the properties in the _cmdline_json, need verify it with the actual test result
If _cmdline_json and the hotplug_qmp use the same properties type, I think it is better to redesign the automation structure. Because, automation use two separate functions now, convert parameter in two function are duplicate.

mcasquer · 2025-01-20T12:01:09Z

@yanan-fu ok, doing the change with all the properties could be a valid change, let me know with the final decision

mcasquer marked this pull request as ready for review January 14, 2025 10:34

yanan-fu reviewed Jan 14, 2025

View reviewed changes

virttest/qemu_devices/qcontainer.py Outdated Show resolved Hide resolved

mcasquer force-pushed the 3253_node_uint_device_hotplug branch from f0fe314 to 7cad78b Compare January 14, 2025 11:46

yanan-fu mentioned this pull request Jan 16, 2025

num_queues: correct num-queues to accept int value #4050

Open

mcasquer force-pushed the 3253_node_uint_device_hotplug branch from 7cad78b to e4e648f Compare January 17, 2025 09:33

qdevices: refreshes the device_add params for QMP hotplug

0d91c38

device_add no longer accept string values in QMP command. Casts the node and requested-size values to integer before doing the QMP hotplug. Signed-off-by: mcasquer <mcasquer@redhat.com>

mcasquer force-pushed the 3253_node_uint_device_hotplug branch from e4e648f to 0d91c38 Compare January 17, 2025 09:34

yanan-fu reviewed Jan 17, 2025

View reviewed changes

mcasquer changed the title ~~qcontainer: adds new flag that converts device_add opts to int~~ qdevices: refreshes the device_add params for QMP hotplug Jan 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qdevices: refreshes the device_add params for QMP hotplug #4053

qdevices: refreshes the device_add params for QMP hotplug #4053

mcasquer commented Jan 14, 2025 •

edited

Loading

yanan-fu commented Jan 14, 2025

mcasquer commented Jan 14, 2025

mcasquer commented Jan 14, 2025

yanan-fu commented Jan 15, 2025

yanan-fu commented Jan 15, 2025

mcasquer commented Jan 15, 2025

yanan-fu commented Jan 16, 2025

mcasquer commented Jan 16, 2025

PaulYuuu commented Jan 17, 2025

yanan-fu commented Jan 17, 2025

yanan-fu commented Jan 17, 2025

YongxueHong commented Jan 17, 2025

yanan-fu commented Jan 17, 2025

YongxueHong commented Jan 17, 2025

nickzhq commented Jan 17, 2025

yanan-fu commented Jan 17, 2025 •

edited

Loading

mcasquer commented Jan 17, 2025

mcasquer commented Jan 17, 2025

mcasquer commented Jan 17, 2025

yanan-fu Jan 17, 2025

yanan-fu commented Jan 17, 2025

mcasquer commented Jan 17, 2025

yanan-fu commented Jan 17, 2025

mcasquer commented Jan 20, 2025

qdevices: refreshes the device_add params for QMP hotplug #4053

Are you sure you want to change the base?

qdevices: refreshes the device_add params for QMP hotplug #4053

Conversation

mcasquer commented Jan 14, 2025 • edited Loading

yanan-fu commented Jan 14, 2025

mcasquer commented Jan 14, 2025

mcasquer commented Jan 14, 2025

yanan-fu commented Jan 15, 2025

yanan-fu commented Jan 15, 2025

mcasquer commented Jan 15, 2025

yanan-fu commented Jan 16, 2025

mcasquer commented Jan 16, 2025

PaulYuuu commented Jan 17, 2025

yanan-fu commented Jan 17, 2025

yanan-fu commented Jan 17, 2025

YongxueHong commented Jan 17, 2025

yanan-fu commented Jan 17, 2025

YongxueHong commented Jan 17, 2025

nickzhq commented Jan 17, 2025

yanan-fu commented Jan 17, 2025 • edited Loading

mcasquer commented Jan 17, 2025

mcasquer commented Jan 17, 2025

mcasquer commented Jan 17, 2025

yanan-fu Jan 17, 2025

Choose a reason for hiding this comment

yanan-fu commented Jan 17, 2025

mcasquer commented Jan 17, 2025

yanan-fu commented Jan 17, 2025

mcasquer commented Jan 20, 2025

mcasquer commented Jan 14, 2025 •

edited

Loading

yanan-fu commented Jan 17, 2025 •

edited

Loading