[libvirt] [PATCH 6/7] qemu: Support newer ivshmem device variants

[Daniel P. Berrange]

On Wed, Sep 21, 2016 at 05:10:09PM +0200, Martin Kletzander wrote:
> On Wed, Sep 21, 2016 at 04:01:23PM +0100, Daniel P. Berrange wrote:
> > I'm not a fan of the idea of silently picking a different device
> > for the guest behind the applications back. By not exposing the
> > different device types with a "model" attribute, we miss a way
> > to report to the application which models are supported by the
> > QEMU they're using - eg via domain capabilities.
> > 
> > This in turn means the application doesn't know whether they're
> > getting the new or old version, and so don't know if they're going
> > to have working migration or not.
> > 
> > If we expanded the XML to include model, then application can
> > explicitly request the new model (which supports migration) and
> > know that they'll get a startup failure if not supported, as
> > opposed to silently falling back to the non-migratable version.
> > 
> > Also, it makes life hard for us if the ivshmem-plain device wants
> > to support use of the 'server' attribute in the future, as we will
> > then not know which to create.
> > 
> > We've often been burnt in the past by trying todo magic like this,
> > instead of explicitly representing stuff in the XML, so I think we
> > should be being explicit about ivshmem models here.
> > 
> > Of course, if we do add <model> support, we have to allow for it
> > to be missing for sake of upgrades. So there's a question of which
> > model we should select as the default, if not seen in the XML.
> > 
> 
> If selecting the newest one whenever the element is missing is fine,
> then I'm OK with that.  But that would change the device when upgrading
> libvirt (without user intervention), which you didn't like IIUC.

Yes, I don't think we can do that - at least note exactly in the way
you do it in this patch. eg Looking at the ivshmem code in QEMU there
is this comment about guest interupt setup:

     * Do nothing unless the device actually uses INTx.  Here's how
     * the device variants signal interrupts, what they put in PCI
     * config space:
     * Device variant    Interrupt  Interrupt Pin  MSI-X cap.
     * ivshmem-plain         none            0         no
     * ivshmem-doorbell     MSI-X            1        yes(1)
     * ivshmem,msi=off       INTx            1         no
     * ivshmem,msi=on       MSI-X            1(2)     yes(1)
     * (1) if guest enabled MSI-X
     * (2) the device lies

Note that neither ivshmem-plain or ivshmem-doorbell support use of
INTx for interupts. I'm also concerned about the footnote (2) there,
as that seems to imply that ivshmem,msi=on, is not in fact the
same as ivshmem-doorbell, because ivshmem lies about the interrupt
pin (whatever that means).

I'm inclined so that that we should do

  if (ivshmem exists) {
     use ivshmem
  } else {
     if (server) {
        if(msi) {
	   use ivshmem-doorbell
	} else {
	   error config unsupported
	}
     } else {
        use ivshmem-plain
     }
  }

That way if a distro compiles-out 'ivshmem' we'll use one of the
new devices if they're available, otherwise we'll stick with the
conversative behaviour of using the legacy device for guaranteed
bug compatibility.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|