[4/6] qemu: Wire up disk model=virtio-{non-}transitional

Message ID 1ac81ba920b03d153f80236fea5aa81321e054fa.1547420060.git.crobinso@redhat.com
State New
Headers show
Series
  • RFC: qemu: virtio-{non-}transitional support
Related show

Commit Message

Cole Robinson Jan. 13, 2019, 11:12 p.m.
Add new <disk> model values for virtio transitional devices. When
combined with bus='virtio':

* "virtio-transitional" maps to qemu "virtio-blk-pci-transitional"
* "virtio-non-transitional" maps to qemu "virtio-blk-pci-non-transitional"

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 src/qemu/qemu_command.c                       | 31 ++++++++++++++++++-
 src/qemu/qemu_domain_address.c                |  2 ++
 ...virtio-non-transitional.x86_64-latest.args |  7 +++--
 .../virtio-transitional.x86_64-latest.args    |  4 +--
 .../virtio-non-transitional.xml               | 10 ++++--
 5 files changed, 45 insertions(+), 9 deletions(-)

-- 
2.20.1

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

Comments

Daniel P. Berrangé Jan. 15, 2019, 4:56 p.m. | #1
On Sun, Jan 13, 2019 at 06:12:06PM -0500, Cole Robinson wrote:
> Add new <disk> model values for virtio transitional devices. When

> combined with bus='virtio':

> 

> * "virtio-transitional" maps to qemu "virtio-blk-pci-transitional"

> * "virtio-non-transitional" maps to qemu "virtio-blk-pci-non-transitional"

> 

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---

>  src/qemu/qemu_command.c                       | 31 ++++++++++++++++++-

>  src/qemu/qemu_domain_address.c                |  2 ++

>  ...virtio-non-transitional.x86_64-latest.args |  7 +++--

>  .../virtio-transitional.x86_64-latest.args    |  4 +--

>  .../virtio-non-transitional.xml               | 10 ++++--

>  5 files changed, 45 insertions(+), 9 deletions(-)

> 

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c

> index 822d5f8669..ca6abea227 100644

> --- a/src/qemu/qemu_command.c

> +++ b/src/qemu/qemu_command.c

> @@ -443,6 +443,33 @@ qemuBuildVirtioDevStr(virBufferPtr buf,

>      return 0;

>  }

>  

> +static int

> +qemuBuildVirtioTransitional(virBufferPtr buf,

> +                            const char *baseName,

> +                            virDomainDeviceAddressType type,

> +                            bool transitional,

> +                            bool nontransitional)

> +{

> +    if (qemuBuildVirtioDevStr(buf, baseName, type) < 0)

> +        return -1;

> +

> +    if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&

> +        (transitional || nontransitional)) {

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,

> +                       _("virtio transitional models are not supported "

> +                         "for address type=%s"),

> +                       virDomainDeviceAddressTypeToString(type));

> +        return -1;

> +    }

> +

> +    if (transitional) {

> +        virBufferAddLit(buf, "-transitional");

> +    } else if (nontransitional) {

> +        virBufferAddLit(buf, "-non-transitional");

> +    }

> +    return 0;



So this only works on QEMU >= 4.0.0 - earlier versions will
fail to start.

We can, however, make it work correctly with old QEMU.

A transitional device is 100% identical to the existing device
types, so we can simply not add the "-transitional" suffix for
old QEMU. The only difference is the way libvirt does PCI bus
placement of the transitional device - we'd never use PCIe.

A non-transitional device is identical to the existing device
types, but with  disable-legacy=true set.


QEMU guarantees this compatibility of the different devices,
but only for machine types < pc-i440fx-4.0.0 / pc-q35-4.0.0.
So we should none the less make sure we use the modern device
names for any QEMU which genuinely supports them.



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani Jan. 16, 2019, 11:31 a.m. | #2
On Tue, 2019-01-15 at 16:56 +0000, Daniel P. Berrangé wrote:
> On Sun, Jan 13, 2019 at 06:12:06PM -0500, Cole Robinson wrote:
> > Add new <disk> model values for virtio transitional devices. When
> > combined with bus='virtio':
> > 
> > * "virtio-transitional" maps to qemu "virtio-blk-pci-transitional"
> > * "virtio-non-transitional" maps to qemu "virtio-blk-pci-non-transitional"
> > 
> > Signed-off-by: Cole Robinson <crobinso@redhat.com>
> > ---
> >  src/qemu/qemu_command.c                       | 31 ++++++++++++++++++-
> >  src/qemu/qemu_domain_address.c                |  2 ++
> >  ...virtio-non-transitional.x86_64-latest.args |  7 +++--
> >  .../virtio-transitional.x86_64-latest.args    |  4 +--
> >  .../virtio-non-transitional.xml               | 10 ++++--
> >  5 files changed, 45 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 822d5f8669..ca6abea227 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -443,6 +443,33 @@ qemuBuildVirtioDevStr(virBufferPtr buf,
> >      return 0;
> >  }
> >  
> > +static int
> > +qemuBuildVirtioTransitional(virBufferPtr buf,
> > +                            const char *baseName,
> > +                            virDomainDeviceAddressType type,
> > +                            bool transitional,
> > +                            bool nontransitional)
> > +{
> > +    if (qemuBuildVirtioDevStr(buf, baseName, type) < 0)
> > +        return -1;
> > +
> > +    if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> > +        (transitional || nontransitional)) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("virtio transitional models are not supported "
> > +                         "for address type=%s"),
> > +                       virDomainDeviceAddressTypeToString(type));
> > +        return -1;
> > +    }
> > +
> > +    if (transitional) {
> > +        virBufferAddLit(buf, "-transitional");
> > +    } else if (nontransitional) {
> > +        virBufferAddLit(buf, "-non-transitional");
> > +    }
> > +    return 0;
> 
> So this only works on QEMU >= 4.0.0 - earlier versions will
> fail to start.
> 
> We can, however, make it work correctly with old QEMU.
> 
> A transitional device is 100% identical to the existing device
> types, so we can simply not add the "-transitional" suffix for
> old QEMU. The only difference is the way libvirt does PCI bus
> placement of the transitional device - we'd never use PCIe.
> 
> A non-transitional device is identical to the existing device
> types, but with  disable-legacy=true set.

Again, the relationship between existing and new devices is not
quite this straighforward because of the reasons I outlined in

  https://www.redhat.com/archives/libvir-list/2019-January/msg00514.html

But the idea of using disable-{legacy,modern} instead of the new
virtio-*-{non,}-transitional devices is one I had already suggested
in

  https://bugzilla.redhat.com/show_bug.cgi?id=1614127

so I'm obviously on board with it :)

> QEMU guarantees this compatibility of the different devices,
> but only for machine types < pc-i440fx-4.0.0 / pc-q35-4.0.0.
> So we should none the less make sure we use the modern device
> names for any QEMU which genuinely supports them.

As I already mentioned in the bug report linked above, I'm not
quite convinced that's the case, and I don't see why we wouldn't
just use the options and basically ignore the QEMU-level devices,
as the former approach would work on old QEMU releases as well as
recent ones with no drawback I can think of.
Daniel P. Berrangé Jan. 16, 2019, 11:39 a.m. | #3
On Wed, Jan 16, 2019 at 12:31:04PM +0100, Andrea Bolognani wrote:
> On Tue, 2019-01-15 at 16:56 +0000, Daniel P. Berrangé wrote:
> > On Sun, Jan 13, 2019 at 06:12:06PM -0500, Cole Robinson wrote:
> > > Add new <disk> model values for virtio transitional devices. When
> > > combined with bus='virtio':
> > > 
> > > * "virtio-transitional" maps to qemu "virtio-blk-pci-transitional"
> > > * "virtio-non-transitional" maps to qemu "virtio-blk-pci-non-transitional"
> > > 
> > > Signed-off-by: Cole Robinson <crobinso@redhat.com>
> > > ---
> > >  src/qemu/qemu_command.c                       | 31 ++++++++++++++++++-
> > >  src/qemu/qemu_domain_address.c                |  2 ++
> > >  ...virtio-non-transitional.x86_64-latest.args |  7 +++--
> > >  .../virtio-transitional.x86_64-latest.args    |  4 +--
> > >  .../virtio-non-transitional.xml               | 10 ++++--
> > >  5 files changed, 45 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > index 822d5f8669..ca6abea227 100644
> > > --- a/src/qemu/qemu_command.c
> > > +++ b/src/qemu/qemu_command.c
> > > @@ -443,6 +443,33 @@ qemuBuildVirtioDevStr(virBufferPtr buf,
> > >      return 0;
> > >  }
> > >  
> > > +static int
> > > +qemuBuildVirtioTransitional(virBufferPtr buf,
> > > +                            const char *baseName,
> > > +                            virDomainDeviceAddressType type,
> > > +                            bool transitional,
> > > +                            bool nontransitional)
> > > +{
> > > +    if (qemuBuildVirtioDevStr(buf, baseName, type) < 0)
> > > +        return -1;
> > > +
> > > +    if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> > > +        (transitional || nontransitional)) {
> > > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > +                       _("virtio transitional models are not supported "
> > > +                         "for address type=%s"),
> > > +                       virDomainDeviceAddressTypeToString(type));
> > > +        return -1;
> > > +    }
> > > +
> > > +    if (transitional) {
> > > +        virBufferAddLit(buf, "-transitional");
> > > +    } else if (nontransitional) {
> > > +        virBufferAddLit(buf, "-non-transitional");
> > > +    }
> > > +    return 0;
> > 
> > So this only works on QEMU >= 4.0.0 - earlier versions will
> > fail to start.
> > 
> > We can, however, make it work correctly with old QEMU.
> > 
> > A transitional device is 100% identical to the existing device
> > types, so we can simply not add the "-transitional" suffix for
> > old QEMU. The only difference is the way libvirt does PCI bus
> > placement of the transitional device - we'd never use PCIe.
> > 
> > A non-transitional device is identical to the existing device
> > types, but with  disable-legacy=true set.
> 
> Again, the relationship between existing and new devices is not
> quite this straighforward because of the reasons I outlined in
> 
>   https://www.redhat.com/archives/libvir-list/2019-January/msg00514.html

When told to use virtio-transitional for a device, libvirt would
only plug it into a PCI slot, never a PCI-X slot. Given this
constraint, it is functionally identical / interchangable with
the existing device.

> But the idea of using disable-{legacy,modern} instead of the new
> virtio-*-{non,}-transitional devices is one I had already suggested
> in
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1614127
> 
> so I'm obviously on board with it :)
> 
> > QEMU guarantees this compatibility of the different devices,
> > but only for machine types < pc-i440fx-4.0.0 / pc-q35-4.0.0.
> > So we should none the less make sure we use the modern device
> > names for any QEMU which genuinely supports them.
> 
> As I already mentioned in the bug report linked above, I'm not
> quite convinced that's the case, and I don't see why we wouldn't
> just use the options and basically ignore the QEMU-level devices,
> as the former approach would work on old QEMU releases as well as
> recent ones with no drawback I can think of.

The QEMU maintainers were against the idea of us doing that. In the
future they may add properties to, or change the defaults on, the
-transitional or -non-transitional devices only, associated with
new machine type versions. If libvirt forever uses the old devices,
then we loose ability to take advantage of that.

Indeed if QEMU maintainers wanted us to use the disable-legacy/modern
features long term, there would be no point in them even adding the
new device types in the first place.

We should only ever use the disable- flags if the new devices do
not exist in QEMU.

Regards,
Daniel
Andrea Bolognani Jan. 16, 2019, 12:29 p.m. | #4
On Wed, 2019-01-16 at 11:39 +0000, Daniel P. Berrangé wrote:
> On Wed, Jan 16, 2019 at 12:31:04PM +0100, Andrea Bolognani wrote:
> > On Tue, 2019-01-15 at 16:56 +0000, Daniel P. Berrangé wrote:
> > > A transitional device is 100% identical to the existing device
> > > types, so we can simply not add the "-transitional" suffix for
> > > old QEMU. The only difference is the way libvirt does PCI bus
> > > placement of the transitional device - we'd never use PCIe.
> > > 
> > > A non-transitional device is identical to the existing device
> > > types, but with  disable-legacy=true set.
> > 
> > Again, the relationship between existing and new devices is not
> > quite this straighforward because of the reasons I outlined in
> > 
> >   https://www.redhat.com/archives/libvir-list/2019-January/msg00514.html
> 
> When told to use virtio-transitional for a device, libvirt would
> only plug it into a PCI slot, never a PCI-X slot. Given this
> constraint, it is functionally identical / interchangable with
> the existing device.

Right, but you didn't spell out the constraint the first time
around, thus making your (broader) statement that a "transitional
device is 100% identical to the existing device" incorrect :)

> > But the idea of using disable-{legacy,modern} instead of the new
> > virtio-*-{non,}-transitional devices is one I had already suggested
> > in
> > 
> >   https://bugzilla.redhat.com/show_bug.cgi?id=1614127
> > 
> > so I'm obviously on board with it :)
> > 
> > > QEMU guarantees this compatibility of the different devices,
> > > but only for machine types < pc-i440fx-4.0.0 / pc-q35-4.0.0.
> > > So we should none the less make sure we use the modern device
> > > names for any QEMU which genuinely supports them.
> > 
> > As I already mentioned in the bug report linked above, I'm not
> > quite convinced that's the case, and I don't see why we wouldn't
> > just use the options and basically ignore the QEMU-level devices,
> > as the former approach would work on old QEMU releases as well as
> > recent ones with no drawback I can think of.
> 
> The QEMU maintainers were against the idea of us doing that.

I don't recall any QEMU developer specifically saying that, but
that might be just a case of my memory sucking :) CC'ing Eduardo
so he can weigh in.

> In the
> future they may add properties to, or change the defaults on, the
> -transitional or -non-transitional devices only, associated with
> new machine type versions. If libvirt forever uses the old devices,
> then we loose ability to take advantage of that.

Regardless of what libvirt ends up doing, from the QEMU user point
of view I think it would be very surprising if eg. virtio-blk-pci
plugged into a PCIe slot behaved differently from
virtio-blk-pci-non-transitional plugged into the very same slot, or
if virtio-net-pci,disable-legacy=false,disable-modern=false behaved
differently from virtio-net-pci-transitional regardless of the slot
it's plugged into, so moving away from that consistency should be a
non-goal IMHO.

> Indeed if QEMU maintainers wanted us to use the disable-legacy/modern
> features long term, there would be no point in them even adding the
> new device types in the first place.

Yeah, after commenting on the bug report mentioned above I indeed
started thinking that we could have gotten away with not adding
those devices. They might still be useful to people running QEMU
directly, though.

> We should only ever use the disable- flags if the new devices do
> not exist in QEMU.

Wouldn't that potentially cause issues when migrating from QEMU
< 4.0.0, where we'd use disable-*, to QEMU >= 4.0.0, where we'd
use *-{,non}transitional instead? I guess not if the changes in
device behavior are gated by the machine type version.
Daniel P. Berrangé Jan. 16, 2019, 12:44 p.m. | #5
On Wed, Jan 16, 2019 at 01:29:13PM +0100, Andrea Bolognani wrote:
> On Wed, 2019-01-16 at 11:39 +0000, Daniel P. Berrangé wrote:
> > On Wed, Jan 16, 2019 at 12:31:04PM +0100, Andrea Bolognani wrote:
> > > On Tue, 2019-01-15 at 16:56 +0000, Daniel P. Berrangé wrote:
> > > > A transitional device is 100% identical to the existing device
> > > > types, so we can simply not add the "-transitional" suffix for
> > > > old QEMU. The only difference is the way libvirt does PCI bus
> > > > placement of the transitional device - we'd never use PCIe.
> > > > 
> > > > A non-transitional device is identical to the existing device
> > > > types, but with  disable-legacy=true set.
> > > 
> > > Again, the relationship between existing and new devices is not
> > > quite this straighforward because of the reasons I outlined in
> > > 
> > >   https://www.redhat.com/archives/libvir-list/2019-January/msg00514.html
> > 
> > When told to use virtio-transitional for a device, libvirt would
> > only plug it into a PCI slot, never a PCI-X slot. Given this
> > constraint, it is functionally identical / interchangable with
> > the existing device.
> 
> Right, but you didn't spell out the constraint the first time
> around, thus making your (broader) statement that a "transitional
> device is 100% identical to the existing device" incorrect :)
> 
> > > But the idea of using disable-{legacy,modern} instead of the new
> > > virtio-*-{non,}-transitional devices is one I had already suggested
> > > in
> > > 
> > >   https://bugzilla.redhat.com/show_bug.cgi?id=1614127
> > > 
> > > so I'm obviously on board with it :)
> > > 
> > > > QEMU guarantees this compatibility of the different devices,
> > > > but only for machine types < pc-i440fx-4.0.0 / pc-q35-4.0.0.
> > > > So we should none the less make sure we use the modern device
> > > > names for any QEMU which genuinely supports them.
> > > 
> > > As I already mentioned in the bug report linked above, I'm not
> > > quite convinced that's the case, and I don't see why we wouldn't
> > > just use the options and basically ignore the QEMU-level devices,
> > > as the former approach would work on old QEMU releases as well as
> > > recent ones with no drawback I can think of.
> > 
> > The QEMU maintainers were against the idea of us doing that.
> 
> I don't recall any QEMU developer specifically saying that, but
> that might be just a case of my memory sucking :) CC'ing Eduardo
> so he can weigh in.

It was somewhere in one of the many mail threads, but I'm not
finding the archive link right now.

> > In the
> > future they may add properties to, or change the defaults on, the
> > -transitional or -non-transitional devices only, associated with
> > new machine type versions. If libvirt forever uses the old devices,
> > then we loose ability to take advantage of that.
> 
> Regardless of what libvirt ends up doing, from the QEMU user point
> of view I think it would be very surprising if eg. virtio-blk-pci
> plugged into a PCIe slot behaved differently from
> virtio-blk-pci-non-transitional plugged into the very same slot, or
> if virtio-net-pci,disable-legacy=false,disable-modern=false behaved
> differently from virtio-net-pci-transitional regardless of the slot
> it's plugged into, so moving away from that consistency should be a
> non-goal IMHO.
> 
> > Indeed if QEMU maintainers wanted us to use the disable-legacy/modern
> > features long term, there would be no point in them even adding the
> > new device types in the first place.
> 
> Yeah, after commenting on the bug report mentioned above I indeed
> started thinking that we could have gotten away with not adding
> those devices. They might still be useful to people running QEMU
> directly, though.
> 
> > We should only ever use the disable- flags if the new devices do
> > not exist in QEMU.
> 
> Wouldn't that potentially cause issues when migrating from QEMU
> < 4.0.0, where we'd use disable-*, to QEMU >= 4.0.0, where we'd
> use *-{,non}transitional instead? I guess not if the changes in
> device behavior are gated by the machine type version.

In this message Eduardo said virtio-blk-pci,disable-legacy and
virtio-blk-pci-non-transitional are only compatible with the
pc-4.0 machine types and earlier. There's no compat guarantee
of compat for future machine types:

  https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03762.html

If we didn't use the new QEMU device models right now, we could end
up trapped forever. The safe futureproof approach is to always use
the new devices models if available, and use disable-legacy for old
QEMU versions only, which we know will have old machine types for
which the compat guarantee is available.

Regards,
Daniel
Andrea Bolognani Jan. 16, 2019, 2:31 p.m. | #6
On Wed, 2019-01-16 at 12:44 +0000, Daniel P. Berrangé wrote:
> On Wed, Jan 16, 2019 at 01:29:13PM +0100, Andrea Bolognani wrote:
> > On Wed, 2019-01-16 at 11:39 +0000, Daniel P. Berrangé wrote:
> > > In the
> > > future they may add properties to, or change the defaults on, the
> > > -transitional or -non-transitional devices only, associated with
> > > new machine type versions. If libvirt forever uses the old devices,
> > > then we loose ability to take advantage of that.
> > 
> > Regardless of what libvirt ends up doing, from the QEMU user point
> > of view I think it would be very surprising if eg. virtio-blk-pci
> > plugged into a PCIe slot behaved differently from
> > virtio-blk-pci-non-transitional plugged into the very same slot, or
> > if virtio-net-pci,disable-legacy=false,disable-modern=false behaved
> > differently from virtio-net-pci-transitional regardless of the slot
> > it's plugged into, so moving away from that consistency should be a
> > non-goal IMHO.
> > 
[...]
> 
> In this message Eduardo said virtio-blk-pci,disable-legacy and
> virtio-blk-pci-non-transitional are only compatible with the
> pc-4.0 machine types and earlier. There's no compat guarantee
> of compat for future machine types:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03762.html
> 
> If we didn't use the new QEMU device models right now, we could end
> up trapped forever. The safe futureproof approach is to always use
> the new devices models if available, and use disable-legacy for old
> QEMU versions only, which we know will have old machine types for
> which the compat guarantee is available.

Well, let's see if Eduardo is willing to reconsider the policy on
compatibility between virtio-*-pci-{,non-}transitional and plain
virtio-*-pci going forward based on the principle-of-least-surprise
rationale outlined above :)
Daniel P. Berrangé Jan. 16, 2019, 2:37 p.m. | #7
On Wed, Jan 16, 2019 at 03:31:49PM +0100, Andrea Bolognani wrote:
> On Wed, 2019-01-16 at 12:44 +0000, Daniel P. Berrangé wrote:
> > On Wed, Jan 16, 2019 at 01:29:13PM +0100, Andrea Bolognani wrote:
> > > On Wed, 2019-01-16 at 11:39 +0000, Daniel P. Berrangé wrote:
> > > > In the
> > > > future they may add properties to, or change the defaults on, the
> > > > -transitional or -non-transitional devices only, associated with
> > > > new machine type versions. If libvirt forever uses the old devices,
> > > > then we loose ability to take advantage of that.
> > > 
> > > Regardless of what libvirt ends up doing, from the QEMU user point
> > > of view I think it would be very surprising if eg. virtio-blk-pci
> > > plugged into a PCIe slot behaved differently from
> > > virtio-blk-pci-non-transitional plugged into the very same slot, or
> > > if virtio-net-pci,disable-legacy=false,disable-modern=false behaved
> > > differently from virtio-net-pci-transitional regardless of the slot
> > > it's plugged into, so moving away from that consistency should be a
> > > non-goal IMHO.
> > > 
> [...]
> > 
> > In this message Eduardo said virtio-blk-pci,disable-legacy and
> > virtio-blk-pci-non-transitional are only compatible with the
> > pc-4.0 machine types and earlier. There's no compat guarantee
> > of compat for future machine types:
> > 
> >   https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03762.html
> > 
> > If we didn't use the new QEMU device models right now, we could end
> > up trapped forever. The safe futureproof approach is to always use
> > the new devices models if available, and use disable-legacy for old
> > QEMU versions only, which we know will have old machine types for
> > which the compat guarantee is available.
> 
> Well, let's see if Eduardo is willing to reconsider the policy on
> compatibility between virtio-*-pci-{,non-}transitional and plain
> virtio-*-pci going forward based on the principle-of-least-surprise
> rationale outlined above :)

I think we should use the new devices no matter what. Libvirt generally
always strives to follow the latest QEMU best practice, even when new &
old way of doing something are functionally identical. Eventually we
would drop support for QEU < 4.0 and the old way would go away entirely.


Regards,
Daniel
Eduardo Habkost Jan. 16, 2019, 2:45 p.m. | #8
On Wed, Jan 16, 2019 at 02:37:22PM +0000, Daniel P. Berrangé wrote:
> On Wed, Jan 16, 2019 at 03:31:49PM +0100, Andrea Bolognani wrote:

> > On Wed, 2019-01-16 at 12:44 +0000, Daniel P. Berrangé wrote:

> > > On Wed, Jan 16, 2019 at 01:29:13PM +0100, Andrea Bolognani wrote:

> > > > On Wed, 2019-01-16 at 11:39 +0000, Daniel P. Berrangé wrote:

> > > > > In the

> > > > > future they may add properties to, or change the defaults on, the

> > > > > -transitional or -non-transitional devices only, associated with

> > > > > new machine type versions. If libvirt forever uses the old devices,

> > > > > then we loose ability to take advantage of that.

> > > > 

> > > > Regardless of what libvirt ends up doing, from the QEMU user point

> > > > of view I think it would be very surprising if eg. virtio-blk-pci

> > > > plugged into a PCIe slot behaved differently from

> > > > virtio-blk-pci-non-transitional plugged into the very same slot, or

> > > > if virtio-net-pci,disable-legacy=false,disable-modern=false behaved

> > > > differently from virtio-net-pci-transitional regardless of the slot

> > > > it's plugged into, so moving away from that consistency should be a

> > > > non-goal IMHO.

> > > > 

> > [...]

> > > 

> > > In this message Eduardo said virtio-blk-pci,disable-legacy and

> > > virtio-blk-pci-non-transitional are only compatible with the

> > > pc-4.0 machine types and earlier. There's no compat guarantee

> > > of compat for future machine types:

> > > 

> > >   https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03762.html

> > > 

> > > If we didn't use the new QEMU device models right now, we could end

> > > up trapped forever. The safe futureproof approach is to always use

> > > the new devices models if available, and use disable-legacy for old

> > > QEMU versions only, which we know will have old machine types for

> > > which the compat guarantee is available.

> > 

> > Well, let's see if Eduardo is willing to reconsider the policy on

> > compatibility between virtio-*-pci-{,non-}transitional and plain

> > virtio-*-pci going forward based on the principle-of-least-surprise

> > rationale outlined above :)

> 

> I think we should use the new devices no matter what. Libvirt generally

> always strives to follow the latest QEMU best practice, even when new &

> old way of doing something are functionally identical. Eventually we

> would drop support for QEU < 4.0 and the old way would go away entirely.


It would also allow us to deprecate the old devices, which would
be welcome.  Always using the new devices when available would be
my recommendation.

But I don't want to create unnecessary obstacles for libvirt, so
if there's a real benefit in promising compatibility between both
device types, we can still promise that on the QEMU side.
Breaking compatibility on purpose is very unlikely, and the most
likely accidents could be detected by
tests/acceptance/virtio_version.py.

-- 
Eduardo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel P. Berrangé Jan. 16, 2019, 3:40 p.m. | #9
On Wed, Jan 16, 2019 at 12:45:43PM -0200, Eduardo Habkost wrote:
> On Wed, Jan 16, 2019 at 02:37:22PM +0000, Daniel P. Berrangé wrote:
> > On Wed, Jan 16, 2019 at 03:31:49PM +0100, Andrea Bolognani wrote:
> > > On Wed, 2019-01-16 at 12:44 +0000, Daniel P. Berrangé wrote:
> > > > On Wed, Jan 16, 2019 at 01:29:13PM +0100, Andrea Bolognani wrote:
> > > > > On Wed, 2019-01-16 at 11:39 +0000, Daniel P. Berrangé wrote:
> > > > > > In the
> > > > > > future they may add properties to, or change the defaults on, the
> > > > > > -transitional or -non-transitional devices only, associated with
> > > > > > new machine type versions. If libvirt forever uses the old devices,
> > > > > > then we loose ability to take advantage of that.
> > > > > 
> > > > > Regardless of what libvirt ends up doing, from the QEMU user point
> > > > > of view I think it would be very surprising if eg. virtio-blk-pci
> > > > > plugged into a PCIe slot behaved differently from
> > > > > virtio-blk-pci-non-transitional plugged into the very same slot, or
> > > > > if virtio-net-pci,disable-legacy=false,disable-modern=false behaved
> > > > > differently from virtio-net-pci-transitional regardless of the slot
> > > > > it's plugged into, so moving away from that consistency should be a
> > > > > non-goal IMHO.
> > > > > 
> > > [...]
> > > > 
> > > > In this message Eduardo said virtio-blk-pci,disable-legacy and
> > > > virtio-blk-pci-non-transitional are only compatible with the
> > > > pc-4.0 machine types and earlier. There's no compat guarantee
> > > > of compat for future machine types:
> > > > 
> > > >   https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03762.html
> > > > 
> > > > If we didn't use the new QEMU device models right now, we could end
> > > > up trapped forever. The safe futureproof approach is to always use
> > > > the new devices models if available, and use disable-legacy for old
> > > > QEMU versions only, which we know will have old machine types for
> > > > which the compat guarantee is available.
> > > 
> > > Well, let's see if Eduardo is willing to reconsider the policy on
> > > compatibility between virtio-*-pci-{,non-}transitional and plain
> > > virtio-*-pci going forward based on the principle-of-least-surprise
> > > rationale outlined above :)
> > 
> > I think we should use the new devices no matter what. Libvirt generally
> > always strives to follow the latest QEMU best practice, even when new &
> > old way of doing something are functionally identical. Eventually we
> > would drop support for QEU < 4.0 and the old way would go away entirely.
> 
> It would also allow us to deprecate the old devices, which would
> be welcome.  Always using the new devices when available would be
> my recommendation.

I don't really see QEMU upstream deprecating the old devices any
time. There is sooooo much documentation that refers to them that
will never be fixed. 99% of users won't get any benefit from using
the new devices either, so there's no compelling reason to update
their existing configs or docs. They're not going to be a huge maint
burden to QEMU devs either given its just a toggle of a few props.

I might see a downstream distro deprecating them at some point
though, since they have a much tighter controlled usage scenario
than upstream.

> But I don't want to create unnecessary obstacles for libvirt, so
> if there's a real benefit in promising compatibility between both
> device types, we can still promise that on the QEMU side.

I don't think there's an obstacle for libvirt, as I don't see any
compelling reason to avoid the new devices when we have QEMU >= 4.0.

> Breaking compatibility on purpose is very unlikely, and the most
> likely accidents could be detected by
> tests/acceptance/virtio_version.py.

Regards,
Daniel
Andrea Bolognani Jan. 16, 2019, 4:24 p.m. | #10
On Wed, 2019-01-16 at 15:40 +0000, Daniel P. Berrangé wrote:
> On Wed, Jan 16, 2019 at 12:45:43PM -0200, Eduardo Habkost wrote:
> > But I don't want to create unnecessary obstacles for libvirt, so
> > if there's a real benefit in promising compatibility between both
> > device types, we can still promise that on the QEMU side.
> 
> I don't think there's an obstacle for libvirt, as I don't see any
> compelling reason to avoid the new devices when we have QEMU >= 4.0.

Alright, let's do it that way then.

I still think it's important to maintain the relationship between
old and new devices consistent going forward, because not doing so
will certainly result in confusion for those using QEMU directly.
Eduardo Habkost Jan. 16, 2019, 11:24 p.m. | #11
On Wed, Jan 16, 2019 at 05:24:02PM +0100, Andrea Bolognani wrote:
> On Wed, 2019-01-16 at 15:40 +0000, Daniel P. Berrangé wrote:

> > On Wed, Jan 16, 2019 at 12:45:43PM -0200, Eduardo Habkost wrote:

> > > But I don't want to create unnecessary obstacles for libvirt, so

> > > if there's a real benefit in promising compatibility between both

> > > device types, we can still promise that on the QEMU side.

> > 

> > I don't think there's an obstacle for libvirt, as I don't see any

> > compelling reason to avoid the new devices when we have QEMU >= 4.0.

> 

> Alright, let's do it that way then.

> 

> I still think it's important to maintain the relationship between

> old and new devices consistent going forward, because not doing so

> will certainly result in confusion for those using QEMU directly.


Agreed that it's a good thing to have.  I will extend the
existing virtio_version.py test case to be more strict and try to
catch mistakes that would break compatibility between the two
device types in the future.

-- 
Eduardo

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

Patch

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 822d5f8669..ca6abea227 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -443,6 +443,33 @@  qemuBuildVirtioDevStr(virBufferPtr buf,
     return 0;
 }
 
+static int
+qemuBuildVirtioTransitional(virBufferPtr buf,
+                            const char *baseName,
+                            virDomainDeviceAddressType type,
+                            bool transitional,
+                            bool nontransitional)
+{
+    if (qemuBuildVirtioDevStr(buf, baseName, type) < 0)
+        return -1;
+
+    if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+        (transitional || nontransitional)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("virtio transitional models are not supported "
+                         "for address type=%s"),
+                       virDomainDeviceAddressTypeToString(type));
+        return -1;
+    }
+
+    if (transitional) {
+        virBufferAddLit(buf, "-transitional");
+    } else if (nontransitional) {
+        virBufferAddLit(buf, "-non-transitional");
+    }
+    return 0;
+}
+
 
 static int
 qemuBuildVirtioOptionsStr(virBufferPtr buf,
@@ -2049,7 +2076,9 @@  qemuBuildDiskDeviceStr(const virDomainDef *def,
         break;
 
     case VIR_DOMAIN_DISK_BUS_VIRTIO:
-        if (qemuBuildVirtioDevStr(&opt, "virtio-blk", disk->info.type) < 0)
+        if (qemuBuildVirtioTransitional(&opt, "virtio-blk", disk->info.type,
+                                        disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL,
+                                        disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL) < 0)
             goto error;
 
         if (disk->iothread)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index bd6c4031e0..1a77b74ad1 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -723,6 +723,8 @@  qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
     case VIR_DOMAIN_DEVICE_DISK:
         switch ((virDomainDiskBus) dev->data.disk->bus) {
         case VIR_DOMAIN_DISK_BUS_VIRTIO:
+            if (dev->data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL)
+                return pciFlags;
             return virtioFlags; /* only virtio disks use PCI */
 
         case VIR_DOMAIN_DISK_BUS_IDE:
diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
index 070b4b8334..a8f878c99c 100644
--- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
@@ -25,10 +25,11 @@  file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 -boot strict=on \
 -device pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,\
 addr=0x1 \
--device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \
+-device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 \
+-device pcie-root-port,port=0x9,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x1 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
--device virtio-blk-pci,scsi=off,bus=pci.1,addr=0x0,drive=drive-virtio-disk0,\
-id=virtio-disk0,bootindex=1 \
+-device virtio-blk-pci-non-transitional,scsi=off,bus=pci.2,addr=0x1,\
+drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
 -msg timestamp=on
diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
index 070b4b8334..7730b177e7 100644
--- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
@@ -27,8 +27,8 @@  file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 addr=0x1 \
 -device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
--device virtio-blk-pci,scsi=off,bus=pci.1,addr=0x0,drive=drive-virtio-disk0,\
-id=virtio-disk0,bootindex=1 \
+-device virtio-blk-pci-transitional,scsi=off,bus=pci.1,addr=0x0,\
+drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
 -msg timestamp=on
diff --git a/tests/qemuxml2xmloutdata/virtio-non-transitional.xml b/tests/qemuxml2xmloutdata/virtio-non-transitional.xml
index 1c00365edf..a64a84d145 100644
--- a/tests/qemuxml2xmloutdata/virtio-non-transitional.xml
+++ b/tests/qemuxml2xmloutdata/virtio-non-transitional.xml
@@ -18,7 +18,7 @@ 
       <driver name='qemu' type='raw'/>
       <source dev='/dev/HostVG/QEMUGuest1'/>
       <target dev='vda' bus='virtio'/>
-      <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
     </disk>
     <controller type='usb' index='0' model='none'/>
     <controller type='sata' index='0'>
@@ -30,9 +30,13 @@ 
       <target chassis='1' port='0x8'/>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/>
     </controller>
-    <controller type='pci' index='2' model='pcie-root-port'>
+    <controller type='pci' index='2' model='pcie-to-pci-bridge'>
+      <model name='pcie-pci-bridge'/>
+      <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
+    </controller>
+    <controller type='pci' index='3' model='pcie-root-port'>
       <model name='pcie-root-port'/>
-      <target chassis='2' port='0x9'/>
+      <target chassis='3' port='0x9'/>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
     </controller>
     <input type='mouse' bus='ps2'/>