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

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.