Re: [libvirt] [PATCH v3 10/12] qemu: Add hotpluging support for PCI devices on S390 guests
在 2018/8/20 下午7:34, Andrea Bolognani 写道: So, I'm very much not familiar with the hotplug code and seeing changes to stuff like qemuDomainAttachDiskGeneric() makes me a bit uncomfortable:) I can't spot anything obviously wrong in your changes, but I think perhaps you might want to enter and exit the monitor separately for the zpci device and for the virtio device? I'm not sure that's useful at all, but network devices for example seems to follow that pattern... It would be great if someone with more experience in this area could provide a review. We have to add zpci device firstly and add corresponding pci device secondly. Do you think it's redundant to call monitor twice to add two devices? Again, I'm not familiar at all with this code, but entering (and exiting) the monitor once for each device you're dealing with seems to be a pattern. So let's see other reviewers' comment. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/12] qemu: Add hotpluging support for PCI devices on S390 guests
On Mon, 2018-08-20 at 16:04 +0800, Yi Min Zhao wrote: > 在 2018/8/18 上午12:10, Andrea Bolognani 写道: > > On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: > > > This commit adds hotplug support for PCI devices on S390 guests. > > > There's no need to implement hot unplug for zPCI as QEMU implements > > > an unplug callback which will unplug both PCI and zPCI device in a > > > cascaded way. > > > > It looks like you ended up implementing explicit hot unplug at > > least for controllers. I think perhaps it would be a good idea > > to implement it for all devices instead of relying on QEMU's > > own unplug cascading so that we have more control over the whole > > process. > > It's different between controller and device. And we only hot unplug pci > controller, not for > all controller types. In addition, we only could do hot-unplug one time, > either zpci device > or corresponding pci device. It's due to Qemu logic. Qemu will > hot-unplug zpci device > automatically while doing hotplug pci device, and vice versa. Ouch, that's kinda nasty... And I wonder why that doesn't work for controllers too? They're not *that* special :) > > So, I'm very much not familiar with the hotplug code and seeing > > changes to stuff like qemuDomainAttachDiskGeneric() makes me a bit > > uncomfortable :) > > > > I can't spot anything obviously wrong in your changes, but I think > > perhaps you might want to enter and exit the monitor separately > > for the zpci device and for the virtio device? I'm not sure that's > > useful at all, but network devices for example seems to follow > > that pattern... It would be great if someone with more experience > > in this area could provide a review. > > We have to add zpci device firstly and add corresponding pci device > secondly. > Do you think it's redundant to call monitor twice to add two devices? Again, I'm not familiar at all with this code, but entering (and exiting) the monitor once for each device you're dealing with seems to be a pattern. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/12] qemu: Add hotpluging support for PCI devices on S390 guests
在 2018/8/18 上午12:10, Andrea Bolognani 写道: On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: This commit adds hotplug support for PCI devices on S390 guests. There's no need to implement hot unplug for zPCI as QEMU implements an unplug callback which will unplug both PCI and zPCI device in a cascaded way. It looks like you ended up implementing explicit hot unplug at least for controllers. I think perhaps it would be a good idea to implement it for all devices instead of relying on QEMU's own unplug cascading so that we have more control over the whole process. It's different between controller and device. And we only hot unplug pci controller, not for all controller types. In addition, we only could do hot-unplug one time, either zpci device or corresponding pci device. It's due to Qemu logic. Qemu will hot-unplug zpci device automatically while doing hotplug pci device, and vice versa. @@ -669,8 +737,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0) goto exit_monitor; -if (qemuMonitorAddDevice(priv->mon, devstr) < 0) +if (qemuDomainAttachExtensionDevice(priv->mon, >info) < 0) +goto exit_monitor; + +if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { +ignore_value(qemuDomainDetachExtensionDevice(priv->mon, >info)); goto exit_monitor; +} if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -2; So, I'm very much not familiar with the hotplug code and seeing changes to stuff like qemuDomainAttachDiskGeneric() makes me a bit uncomfortable :) I can't spot anything obviously wrong in your changes, but I think perhaps you might want to enter and exit the monitor separately for the zpci device and for the virtio device? I'm not sure that's useful at all, but network devices for example seems to follow that pattern... It would be great if someone with more experience in this area could provide a review. We have to add zpci device firstly and add corresponding pci device secondly. Do you think it's redundant to call monitor twice to add two devices? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/12] qemu: Add hotpluging support for PCI devices on S390 guests
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: > This commit adds hotplug support for PCI devices on S390 guests. > There's no need to implement hot unplug for zPCI as QEMU implements > an unplug callback which will unplug both PCI and zPCI device in a > cascaded way. It looks like you ended up implementing explicit hot unplug at least for controllers. I think perhaps it would be a good idea to implement it for all devices instead of relying on QEMU's own unplug cascading so that we have more control over the whole process. > @@ -669,8 +737,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, > if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0) > goto exit_monitor; > > -if (qemuMonitorAddDevice(priv->mon, devstr) < 0) > +if (qemuDomainAttachExtensionDevice(priv->mon, >info) < 0) > +goto exit_monitor; > + > +if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { > +ignore_value(qemuDomainDetachExtensionDevice(priv->mon, > >info)); > goto exit_monitor; > +} > > if (qemuDomainObjExitMonitor(driver, vm) < 0) { > ret = -2; So, I'm very much not familiar with the hotplug code and seeing changes to stuff like qemuDomainAttachDiskGeneric() makes me a bit uncomfortable :) I can't spot anything obviously wrong in your changes, but I think perhaps you might want to enter and exit the monitor separately for the zpci device and for the virtio device? I'm not sure that's useful at all, but network devices for example seems to follow that pattern... It would be great if someone with more experience in this area could provide a review. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 10/12] qemu: Add hotpluging support for PCI devices on S390 guests
This commit adds hotplug support for PCI devices on S390 guests. There's no need to implement hot unplug for zPCI as QEMU implements an unplug callback which will unplug both PCI and zPCI device in a cascaded way. Currently, the following PCI devices are supported: virtio-blk-pci virtio-net-pci virtio-rng-pci virtio-input-host-pci virtio-keyboard-pci virtio-mouse-pci virtio-tablet-pci vfio-pci SCSIVhost device Signed-off-by: Yi Min Zhao Reviewed-by: Boris Fiuczynski Reviewed-by: Stefan Zimmermann Reviewed-by: Bjoern Walk Reviewed-by: Ján Tomko --- src/qemu/qemu_hotplug.c | 155 1 file changed, 145 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1488f0a7c2..d7f427597c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -154,6 +154,74 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, } +static int +qemuDomainAttachZPCIDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ +int ret = -1; +char *devstr_zpci = NULL; + +if (!(devstr_zpci = qemuBuildZPCIDevStr(info))) +goto cleanup; + +if (qemuMonitorAddDevice(mon, devstr_zpci) < 0) +goto cleanup; + +ret = 0; + + cleanup: +VIR_FREE(devstr_zpci); +return ret; +} + + +static int +qemuDomainDetachZPCIDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ +char *zpciAlias = NULL; +int ret = -1; + +if (virAsprintf(, "zpci%d", info->addr.pci.zpci->zpci_uid) < 0) +goto cleanup; + +if (qemuMonitorDelDevice(mon, zpciAlias) < 0) +goto cleanup; + +ret = 0; + + cleanup: +VIR_FREE(zpciAlias); +return ret; +} + + +static int +qemuDomainAttachExtensionDevice(qemuMonitorPtr mon, +virDomainDeviceInfoPtr info) +{ +int ret; + +if ((ret = qemuCheckDeviceIsZPCI(info)) == 1) +return qemuDomainAttachZPCIDevice(mon, info); + +return ret; +} + + +static int +qemuDomainDetachExtensionDevice(qemuMonitorPtr mon, +virDomainDeviceInfoPtr info) +{ +int ret; + +if ((ret = qemuCheckDeviceIsZPCI(info)) == 1) +return qemuDomainDetachZPCIDevice(mon, info); + +return ret; +} + + static int qemuHotplugWaitForTrayEject(virDomainObjPtr vm, virDomainDiskDefPtr disk) @@ -669,8 +737,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0) goto exit_monitor; -if (qemuMonitorAddDevice(priv->mon, devstr) < 0) +if (qemuDomainAttachExtensionDevice(priv->mon, >info) < 0) +goto exit_monitor; + +if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { +ignore_value(qemuDomainDetachExtensionDevice(priv->mon, >info)); goto exit_monitor; +} if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -2; @@ -778,7 +851,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); -ret = qemuMonitorAddDevice(priv->mon, devstr); + +if (qemuDomainAttachExtensionDevice(priv->mon, >info) < 0) { +ignore_value(qemuDomainObjExitMonitor(driver, vm)); +goto cleanup; +} + +if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) +ignore_value(qemuDomainDetachExtensionDevice(priv->mon, >info)); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { releaseaddr = false; ret = -1; @@ -1224,7 +1305,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; } -if (qemuDomainIsS390CCW(vm->def) && +if (net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && +qemuDomainIsS390CCW(vm->def) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CCW)) { net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; if (!(ccwaddrs = virDomainCCWAddressSetCreateFromDomain(vm->def))) @@ -1294,7 +1376,15 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto try_remove; qemuDomainObjEnterMonitor(driver, vm); + +if (qemuDomainAttachExtensionDevice(priv->mon, >info) < 0) { +ignore_value(qemuDomainObjExitMonitor(driver, vm)); +virDomainAuditNet(vm, NULL, net, "attach", false); +goto try_remove; +} + if (qemuMonitorAddDevice(priv->mon, nicstr) < 0) { +ignore_value(qemuDomainDetachExtensionDevice(priv->mon, >info)); ignore_value(qemuDomainObjExitMonitor(driver, vm)); virDomainAuditNet(vm, NULL, net, "attach", false); goto try_remove; @@ -1512,8 +1602,15 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, goto error; qemuDomainObjEnterMonitor(driver, vm); -ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, - configfd, configfd_name); + +if ((ret =