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

[Daniel P. Berrange]

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()

> 
> > +    if (qemudExtractVersionInfo(def->emulator,
> > +                                NULL,
> > +                                &qemuCmdFlags) < 0)
> > +        goto cleanup;
> > +
> > +    if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) {
> > +        if (!(addrs = qemuDomainPCIAddressSetCreate(def)))
> > +            goto cleanup;
> > +
> > +        if (qemuAssignDevicePCISlots(def, addrs) < 0)
> > +            goto cleanup;
> > +    }
> > +
> > +    ret = 0;
> > +
> > +cleanup:
> > +    qemuDomainPCIAddressSetFree(addrs);
> > +
> > +    return ret;
> > +}
> > +
> > +
> >  static pciDeviceList *
> >  qemuGetPciHostDeviceList(virDomainDefPtr def)
> >  {

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|