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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list