On Thu, Mar 21, 2019 at 18:28:41 -0400, Laine Stump wrote: > qemuDomainDetachControllerDevice() calls > qemuDomainDetachExtensionDevice() when the *controller* type is > PCI. This is incorrect in multiple ways: > > * Any code that tears down a device should be in the > qemuDomainRemove*Device() function (which is called after libvirt > gets a DEVICE_DELETED event from qemu indicating that the guest is > finished with the device on its end. The qemuDomainDetach*Device() > functions should only contain code that ensures the requested > operation is valid, and sends the command to qemu to initiate the > unplug. > > * qemuDomainDetachExtensionDevice() is a function that applies to > devices that plug into a PCI slot, *not* necessarily PCI controllers > (which is what's being checked in the offending code). The proper > way to check for this would be to see if the DeviceInfo for the > controller device had a PCI address, not to check if the controller > is a PCI controller (the code being removed was doing the latter). > > * According to commit 1d1e264f1 that added this code (and other > support for hotplugging zPCI devices on s390), it's not necessary to > explicitly detach the zPCI device when unplugging a PCI device. To > quote: > > 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. > > and the evidence bears this out - all the other uses of > qemuDomainDetachExtensionDevice() (except one, which I believe is > also in error, and is being removed in a separate patch) are only to > remove the zPCI extension device in cases where it was successfully > added, but there was some other failure later in the hotplug process > (so there was no regular PCI device to remove and trigger removal of > the zPCI extension device). > > * PCI controllers are not hot pluggable, so this is dead code > anyway. (The only controllers that can currently be > hotplugged/unplugged are SCSI controllers). > > Signed-off-by: Laine Stump <laine@xxxxxxxxx> > --- > src/qemu/qemu_hotplug.c | 12 ------------ > 1 file changed, 12 deletions(-) The code change looks okay to me, but I know nothing of zPCI. ACKing this just because I don't care about zPCI would be wrong :)
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
