Re: [libvirt] [PATCH v5 11/13] qemu: Add hotpluging support for PCI devices on S390 guests
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
+static int
+qemuDomainAttachExtensionDevice(qemuMonitorPtr mon,
+ virDomainDeviceInfoPtr info)
+{
+ if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci))
+ return qemuDomainAttachZPCIDevice(mon, info);
+
+ return 0;
Same comment as with qemuBuildExtensionCommandLine().
[...]
+static int
+qemuDomainDetachExtensionDevice(qemuMonitorPtr mon,
+ virDomainDeviceInfoPtr info)
+{
+ if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci))
+ return qemuDomainDetachZPCIDevice(mon, info);
+
+ return 0;
Here, too.
[...]
@@ -805,8 +869,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr
driver,
if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0)
goto exit_monitor;
- if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
+ if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0)
+ goto exit_monitor;
+
Since the zpci device refers to the underlying device through the
target= option, does it make sense to attach the zpci device first
and the target device second? I would expect it to fail unless you
attach them the other way around...
[...]
@@ -913,7 +982,15 @@ int
qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
goto cleanup;
qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorAddDevice(priv->mon, devstr);
+
+ if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0)
{
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ goto cleanup;
+ }
I'm not sure this is entirely safe. Perhaps you should introduce an
exit_monitor label that ensures failure to exit the monitor is also
taken into account, and jump to that one instead?
[...]
+ if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
+ configfd, configfd_name)) < 0)
+ ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));
Parentheses around here, please.
[...]
@@ -3236,8 +3347,17 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr
driver,
goto cleanup;
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
+
+ if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) {
+ if (qemuDomainAttachExtensionDevice(priv->mon, &input->info) < 0)
+ goto exit_monitor;
+ }
+
+ if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
+ if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO)
+ ignore_value(qemuDomainDetachExtensionDevice(priv->mon,
&input->info));
goto exit_monitor;
+ }
Shouldn't you rather check for the address type here? IIUC an input
device with BUS_VIRTIO could be virtio-ccw or virtio-mmio, for
example, and you don't want to try adding a zpci device in those
cases.
[...]
@@ -5301,6 +5427,11 @@ int
qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
qemuDomainMarkDeviceForRemoval(vm, &detach->info);
qemuDomainObjEnterMonitor(driver, vm);
+ if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
+ qemuDomainDetachExtensionDevice(priv->mon, &detach->info)) {
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ goto cleanup;
+ }
if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) {
ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto cleanup;
This one too looks like it would benefit from an exit_monitor lable.
--
Andrea Bolognani / Red Hat / Virtualization