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

[Daniel P. Berrange]

On Thu, Sep 22, 2016 at 01:09:20PM +0200, Martin Kletzander wrote:
> On Wed, Sep 21, 2016 at 04:26:31PM +0100, Daniel P. Berrange wrote:
> > 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.
> > 
> 
> OK, we can do that.  But before I go and do this variant, I would like
> to clarify two more things (so that I can hope that's the last variant I
> have to post) =)  Should we support setting the role to 'peer'/'master'
> (with ivshmem that maps to role=peer/master and with ivshmem-plain that
> maps to master=on/off)?  Basically master means that the domain can
> migrate (with the data being copied) and peer (or master=off) has
> migration disabled in qemu, so we would disable that as well.  That's
> why hotplug is being implemented, basically.  Currently we don't use
> that parameter, which means role=auto.  That is special value that ends
> up being 'master' for non-server, for server it tries to pick correctly.
> Shouldn't be used, but rather explicitly stated (or just peer for
> everyone and copy the data yourself).  New ivshmem defaults to master=off.

Yep, given that the choice of master/peer impacts whether you can migrate
or not, I think it is important to give applications the ability to
set that, to override the potentially incorrect defaults.

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 :|