Re: [libvirt] [PATCH] qemu: Do not reattach PCI device used by other domain when shutdown

On 09/27/2011 12:53 AM, Osier Yang wrote: Apologies on the delayed review. This is some hairy code, and I want to make sure we get it right, so I kind of shelved it knowing it would be a longer review. > When failing on starting a domain, it tries to reattach all the PCI > devices defined in the domain conf, regardless of whether the devices > are still used by other domain. This will cause the devices are deleted s/are deleted/to be deleted/ > from the list qemu_driver->activePciHostdevs, thus the devices will be > thought as usable even if it's not true. And following commands > nodedev-{reattach,reset} will be successful. > > How to reproduce: > 1) Define two domains with same PCI device defined in the confs. > 2) # virsh start domain1 > 3) # virsh start domain2 > 4) # virsh nodedev-reattach $pci_device > > You will see the device will be reattached to host successfully. > As pciDeviceReattach just check if the device is still used by > other domain via checking if the device is in list > driver->activePciHostdevs, > however, the device is deleted from the list by step 2). Ouch, and definitely needs patching. > > This patch is to prohibit the bug by: > 1) Prohibit a domain starting or device attachment right at > preparation period (qemuPrepareHostdevPCIDevices) if the > device is in list driver->activePciHostdevs, which means > it's used by other domain. Off-hand, I'm not sure if this is quite right - it is completely valid to have two persistent domains both using the same hostdev, _as long as_ you guarantee that at most one of those two domains is active at a time. Is qemuPrepareHostdevPCIDevices called at define time (when the persistent xml is stored) or at start time (when actually starting the guest)? [/me goes and looks...] Yay! qemuPrepareHostdevPCIDevices is only called during hot-plug (domain already active) and by qemuPrepareHostDevices(), which in turn is only used in qemuProcessStart. So it does indeed look like this patch merely prohibits starting domain2 if domain1 is running in your above example, but does not prohibit both definitions from existing.

> > 2) Introduces a new field for struct _pciDevice, (char *used_by), > it will be set as the domain name at preparation period, > (qemuPrepareHostdevPCIDevices). Thus we can prohibit deleting > the device from driver->activePciHostdevs if it's still used by > other domain when stopping the domain process. > > * src/pci.h (define two internal functions, pciDeviceSetUsedBy and > pciDevceGetUsedBy) > * src/pci.c (new field "char *used_by" for struct _pciDevice, > implementations for the two new functions) > * src/libvirt_private.syms (Add the two new internal functions) > * src/qemu_hostdev.h (Modify the definition of functions > qemuPrepareHostdevPCIDevices, and qemuDomainReAttachHostdevDevices) > * src/qemu_hostdev.c (Prohibit preparation and don't delete the > device from activePciHostdevs list if it's still used by other domain) > * src/qemu_hotplug.c (Update function usage, as the definitions are > changed) > --- > src/libvirt_private.syms | 2 ++ > src/qemu/qemu_hostdev.c | 31 ++++++++++++++++++++++++++++--- > src/qemu/qemu_hostdev.h | 2 ++ > src/qemu/qemu_hotplug.c | 4 ++-- > src/util/pci.c | 22 ++++++++++++++++++++++ > src/util/pci.h | 3 +++ > 6 files changed, 59 insertions(+), 5 deletions(-) > > @@ -126,7 +127,10 @@ int qemuPrepareHostdevPCIDevices(struct > qemud_driver *driver, > for (i = 0; i< pciDeviceListCount(pcidevs); i++) { > pciDevice *dev = pciDeviceListGet(pcidevs, i); > if (!pciDeviceIsAssignable(dev, !driver->relaxedACS)) > - goto reattachdevs; > + goto cleanup; This doesn't seem right, when there are multiple devices. Pre-patch, suppose we had: dev1 - is assignable, is managed, and detach succeeds (nevermind the spelling error in pciDettachDevice) dev2 - is not assignable goto reattachdevs dev1 - is managed, so it is reattached Now we have: dev1 - is assignable, is not active, and detach succeeds dev2 - is not assignable goto cleanup Oops - we left dev1 detached. I think you _have_ to break this into two separate loops (and adjust the line earlier about 4 loops to now call out 5 loops!). Loop 1 - prove that all desired devices are assignable and not already active Loop 2 - for all managed devices, detach them

> + > + if (pciDeviceListFind(driver->activePciHostdevs, dev)) > + goto cleanup; This is correct for loop 1, validating that the requested device is not already in use by another active domain. > > if (pciDeviceGetManaged(dev)&& > pciDettachDevice(dev, driver->activePciHostdevs)< 0) > @@ -156,6 +160,14 @@ int qemuPrepareHostdevPCIDevices(struct > qemud_driver *driver, > pciDeviceListSteal(pcidevs, dev); > } > > + /* Now set the used_by_domain of the device in > driver->activePciHostdevs > + * as domain name. > + */ > + for (i = 0; i< pciDeviceListCount(driver->activePciHostdevs); i++) { > + pciDevice * dev = pciDeviceListGet(driver->activePciHostdevs, i); Formatting: s/* dev/*dev/ This iterates over all active pci devices, but it _should_ be iterating only over the pci devices in use by the given domain. Otherwise, you have: dom1 - takes dev1, dev1 in use by "dom1" dom2 - takes dev2, now dev1 and dev2 both in use by "dom2"

> + pciDeviceSetUsedBy(dev, name); Ouch. pciDeviceSetUsedBy is malloc'ing memory, but you aren't checking for failure. More on this below...

> @@ -277,6 +291,17 @@ void qemuDomainReAttachHostdevDevices(struct > qemud_driver *driver, > > for (i = 0; i< pciDeviceListCount(pcidevs); i++) { > pciDevice *dev = pciDeviceListGet(pcidevs, i); > + pciDevice *activeDev = NULL; > + > + /* Never delete the dev from list driver->activePciHostdevs > + * if it's used by other domain. > + */ > + activeDev = pciDeviceListFind(driver->activePciHostdevs, dev); > + if (activeDev&& > + (used_by = pciDeviceGetUsedBy(activeDev))&& > + STRNEQ(used_by, name)) I would have simplified these two conditions to one line: STRNEQ(name, pciDeviceGetUsedBy(activeDev))

> +++ b/src/util/pci.c > @@ -62,6 +62,7 @@ struct _pciDevice { > char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */ > char id[PCI_ID_LEN]; /* product vendor */ > char *path; > + char *used_by; /* The domain which uses the device */ As promised above, why not make this 'const char *used_by', since a domain can only own a device for as long as it is active, and as long as the domain is active, then the domain's name will be in scope. Then, you don't have to strdup() the name, and don't have to worry about malloc failure. But you _do_ have to worry about setting the name back to NULL after a domain goes inactive or the device is hotplugged, to update the fact that no active domain owns the device.

> int fd; > > unsigned initted; > @@ -1312,6 +1313,7 @@ pciGetDevice(unsigned domain, > dev->bus = bus; > dev->slot = slot; > dev->function = function; > + dev->used_by = NULL; > > if (snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x", > dev->domain, dev->bus, dev->slot, > @@ -1374,6 +1376,7 @@ pciFreeDevice(pciDevice *dev) > VIR_DEBUG("%s %s: freeing", dev->id, dev->name); > pciCloseConfig(dev); > VIR_FREE(dev->path); > + VIR_FREE(dev->used_by); If you use my suggestion of pointing to the pre-existing domain name, rather than strdup(), then you don't want this line. > VIR_FREE(dev); > } > > @@ -1387,6 +1390,25 @@ unsigned pciDeviceGetManaged(pciDevice *dev) > return dev->managed; > } > > +int > +pciDeviceSetUsedBy(pciDevice *dev, const char *name) > +{ > + dev->used_by = strdup(name); > + > + if (!dev->used_by) { > + virReportOOMError(); > + return -1; > + } > + > + return 0; and here, you could make the function return void (straight pointer assignment, rather than strdup()). Needs a v2, but we should definitely get this patched.