Re: [libvirt] [PATCH] Add persistence of PCI addresses to QEMU

On Fri, Feb 12, 2010 at 12:05:39PM +0100, Daniel Veillard wrote: > On Thu, Feb 11, 2010 at 04:40:43PM +0000, Daniel P. Berrange wrote: > > Current PCI addresses are allocated at time of VM startup. > > To make them truely persistent, it is neccessary to do this > > at time of virDomainDefine/virDomainCreate. The code in > > qemuStartVMDaemon still remains in order to cope with upgrades > > from older libvirt releases > > > > * src/qemu/qemu_driver.c: Rename existing qemuAssignPCIAddresses > > to qemuDetectPCIAddresses. Add new qemuAssignPCIAddresses which > > does auto-allocation upfront. Call qemuAssignPCIAddresses from > > qemuDomainDefine and qemuDomainCreate to assign PCI addresses that > > can then be persisted. Don't clear PCI addresses at shutdown if > > they are intended to be persistent > > --- > > src/qemu/qemu_driver.c | 84 +++++++++++++++++++++++++++++++++++++++++++++--- > > 1 files changed, 79 insertions(+), 5 deletions(-) > > @@ -857,6 +858,7 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaq > > virDomainObjPtr obj = payload; > > struct qemud_driver *driver = opaque; > > qemuDomainObjPrivatePtr priv; > > + unsigned long long qemuCmdFlags; > > > > virDomainObjLock(obj); > > > > @@ -872,6 +874,15 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaq > > goto error; > > } > > > > + /* XXX we should be persisting the original flags in the XML > > + * not re-detecting them, since the binary may have changed > > + * since launch time */ > > But where would we store them ? It sounds a bit strange, it's emulator > properties, not really domain ones, and I think we sound only store in > the domain what specific flags might be needed from the emulator, not > the full set (and this could change over time as a domain is being > modified). We would put them in the secret status file /var/lib/libvirt/qemu/$GUEST.xml where we already store the $PID and a few other bits of internal state we need to preserve across daemon restarts. The key issue is that you might boot the guest with /usr/bin/qemu pointing to QEMU 0.11, and when we later hotplug a device, or restart libvirtd, that path might be pointing to 0.12. So we'd detect 0.12 features which would then fail because the guest is still running with 0.11. I'm not planning to fix this for this patch, since the problem already existed for a long time. I just wanted to put in this comment to remind me for later :-)

> > @@ -2141,6 +2152,44 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) { > > return -1; > > } > > > > + > > +static int > > +qemuAssignPCIAddresses(virDomainDefPtr def) > > +{ > > + int ret = -1; > > + unsigned long long qemuCmdFlags = 0; > > + qemuDomainPCIAddressSetPtr addrs = NULL; > > + struct stat sb; > > + > > + if (stat(def->emulator, &sb) < 0) { > > + virReportSystemError(errno, > > + _("Cannot find QEMU binary %s"), > > + def->emulator); > > + goto cleanup; > > + } > > do we really need to update that every time ? We can't cache forever > but it's not like the emulator is changing every second. Maybe we need > to put a watch on the emulator at the driver level and keep this in > the driver. The only reason for the 'stat()' call is that the next method invoked qemudExtractVersionInfo() does nto give very nice error messages if the binary is missing. We should probably just move the stat() call into the qemudExtractVersionInfo() itself, rather than duplicating it everywhere that we invoke qemudExtractVersionInfo()