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

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