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

2016-09-22 Thread 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  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 variantInterrupt  Interrupt Pin  MSI-X cap.
> > * ivshmem-plain none0 no
> > * ivshmem-doorbell MSI-X1yes(1)
> > * ivshmem,msi=off   INTx1 no
> > * ivshmem,msi=on   MSI-X1(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-

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

2016-09-22 Thread Martin Kletzander 

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  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 variantInterrupt  Interrupt Pin  MSI-X cap.
* ivshmem-plain none0 no
* ivshmem-doorbell MSI-X1yes(1)
* ivshmem,msi=off   INTx1 no
* ivshmem,msi=on   MSI-X1(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.


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


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-09-21 Thread 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  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 variantInterrupt  Interrupt Pin  MSI-X cap.
 * ivshmem-plain none0 no
 * ivshmem-doorbell MSI-X1yes(1)
 * ivshmem,msi=off   INTx1 no
 * ivshmem,msi=on   MSI-X1(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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-09-21 Thread Martin Kletzander 

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


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


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-09-21 Thread Daniel P. Berrange 
On Wed, Sep 21, 2016 at 03:30:55PM +0200, Martin Kletzander wrote:
> QEMU added support for ivshmem-plain and ivshmem-doorbell.  Those are
> reworked varians of legacy ivshmem that are compatible from the guest
> POV, but not from host's POV and have sane specification and handling.
> 
> Details about the newer device type can be found in qemu's commit
> 5400c02b90bb:
> 
>   http://git.qemu.org/?p=qemu.git;a=commit;h=5400c02b90bb
> 
> Signed-off-by: Martin Kletzander 
> ---
>  docs/formatdomain.html.in  |   5 +-
>  src/qemu/qemu_command.c| 109 
> -
>  src/qemu/qemu_command.h|  10 ++
>  .../qemuxml2argv-shmem-plain-doorbell.args |  46 +
>  .../qemuxml2argv-shmem-plain-doorbell.xml  |   1 +
>  tests/qemuxml2argvtest.c   |   3 +
>  6 files changed, 172 insertions(+), 2 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args
>  create mode 12 
> tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index b9163584d9f5..c614caf5f47f 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6740,7 +6740,10 @@ qemu-kvm -net nic,model=? /dev/null
> 
>  
>A shared memory device allows to share a memory region between
> -  different virtual machines and the host.
> +  different virtual machines and the host.  Note that memory in
> +  this region will be migrated, if that's not desired, the device
> +  needs to be unplugged before migration and plugged back again
> +  after that.
>Since 1.2.10, QEMU and KVM only
>  
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2130a7e2e8cf..3f7dfc4a46b9 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8550,6 +8550,44 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def,
>  return NULL;
>  }
> 
> +char *
> +qemuBuildShmemDevStr(virDomainDefPtr def,
> + virDomainShmemDefPtr shmem,
> + virQEMUCapsPtr qemuCaps)
> +{
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +if (shmem->server.enabled) {
> +virBufferAddLit(, "ivshmem-doorbell");
> +virBufferAsprintf(, ",id=%s,chardev=char%s",
> +  shmem->info.alias, shmem->info.alias);
> +if (shmem->msi.vectors)
> +virBufferAsprintf(, ",vectors=%u", shmem->msi.vectors);
> +if (shmem->msi.ioeventfd)
> +virBufferAsprintf(, ",ioeventfd=%s",
> +  
> virTristateSwitchTypeToString(shmem->msi.ioeventfd));
> +} else {
> +virBufferAddLit(, "ivshmem-plain");
> +virBufferAsprintf(, ",id=%s,memdev=shmmem-%s",
> +  shmem->info.alias, shmem->info.alias);
> +
> +/* For now we default to master=on, users are adviced to
> + * unplug the ivshmem if they don't want to migrate the data
> + * in the shmem (see docs). */
> +virBufferAddLit(, ",master=on");
> +}
> +
> +if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0) {
> +virBufferFreeAndReset();
> +return NULL;
> +}
> +
> +if (virBufferCheckError() < 0)
> +return NULL;
> +
> +return virBufferContentAndReset();
> +}


> @@ -8579,6 +8662,18 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
>virQEMUCapsPtr qemuCaps)
>  {
>  char *devstr = NULL;
> +bool legacy = false;
> +
> +if (!qemuDomainSupportsNonLegacyShmem(qemuCaps, shmem)) {
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IVSHMEM)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("shmem device is not supported with this "
> + "QEMU binary"));
> +return -1;
> +}
> +
> +legacy = true;
> +}
> 
>  if (shmem->size) {
>  /*
> @@ -8606,8 +8701,14 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
>  return -1;
>  }
> 
> -if (!(devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps)))
> +if (legacy)
> +devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps);
> +else
> +devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps);

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