[03/18] qemu: Support disk model=virtio-{non-}transitional

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

Commit Message

Cole Robinson Jan. 17, 2019, 5:52 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_capabilities.c                  |  4 +
 src/qemu/qemu_capabilities.h                  |  2 +
 src/qemu/qemu_command.c                       | 90 ++++++++++++++++++-
 src/qemu/qemu_domain_address.c                |  2 +
 .../caps_4.0.0.x86_64.xml                     |  2 +
 .../virtio-non-transitional.x86_64-3.1.0.args |  4 +-
 ...virtio-non-transitional.x86_64-latest.args |  4 +-
 .../virtio-transitional.x86_64-3.1.0.args     |  5 +-
 .../virtio-transitional.x86_64-latest.args    |  7 +-
 .../virtio-transitional.xml                   | 10 ++-
 10 files changed, 117 insertions(+), 13 deletions(-)

-- 
2.20.1

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

Comments

Andrea Bolognani Jan. 18, 2019, 12:33 p.m. | #1
On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
[...]
> @@ -1108,6 +1110,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {

>      { "vfio-ap", QEMU_CAPS_DEVICE_VFIO_AP },

>      { "zpci", QEMU_CAPS_DEVICE_ZPCI },

>      { "memory-backend-memfd", QEMU_CAPS_OBJECT_MEMORY_MEMFD },

> +    {"virtio-blk-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL},

> +    {"virtio-blk-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL},


There should be whitespace before and after curly braces, for
consistency with existing entries.

[...]

> +++ b/src/qemu/qemu_capabilities.h

> @@ -504,6 +504,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */

>      /* 325 */

>      QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM, /* -object memory-backend-file,pmem= */

>      QEMU_CAPS_DEVICE_NVDIMM_UNARMED, /* -device nvdimm,unarmed= */

> +    QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL, /* -device virtio-blk-pci-transitional */

> +    QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL, /* -device virtio-blk-pci-non-transitional */


A bit more verbose, but I think we should go for

  QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_{,NON_}TRANSITIONAL

since there are several non-PCI variants of VirtIO devices.

It'd also be nice if adding the capabilities and wiring up the
command line generation bits happened in separate patches.

[...]

> +static int

> +qemuBuildVirtioTransitional(virBufferPtr buf,

> +                            const char *baseName,

> +                            virQEMUCapsPtr qemuCaps,

> +                            virDomainDeviceAddressType type,

> +                            int model,

> +                            virDomainDeviceType devtype)

> +{

> +    int tmodel_cap, ntmodel_cap;

> +    bool has_tmodel, has_ntmodel;

> +

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

> +        return -1;

> +

> +    switch (devtype) {

> +        case VIR_DOMAIN_DEVICE_DISK:

> +            has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;

> +            has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;

> +            tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL;

> +            ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL;

> +            break;


I like this approach much better than the one used in the RFC,
eg. passing two booleans to the function. Nice!

What I don't like is that you're building this fairly thin wrapper
around qemuBuildVirtioDevStr() when IMHO you should rather be
agumenting the original function - mostly because the new name is
not nearly as good :) Do you think you could make that happen?

[...]
> +    if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&

> +        (has_tmodel || has_ntmodel)) {

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,

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

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


s/transitional/(non-)transitional/

[...]
> +    if (has_tmodel) {

> +        if (virQEMUCapsGet(qemuCaps, tmodel_cap))

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

> +

> +        /* No error for if -transitional is not supported: our address

> +         * allocation will force the device into plain PCI bus, which

> +         * is functionally identical to standard 'virtio-XXX' behavior

> +         */

> +    } else if (has_ntmodel) {

> +        if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) {

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

> +        } else if (virQEMUCapsGet(qemuCaps,

> +                                  QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {

> +            virBufferAddLit(buf, ",disable-legacy=on");

> +        } else {

> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

> +                           _("virtio non-transitional model not supported "

> +                             "for this qemu"));

> +            return -1;

> +        }

> +    }


Would it make sense to be more explicit here? Current versions of
QEMU default to disable-modern=off,disable-legacy=off for virtio-pci
devices plugged into conventional PCI slots, but unless I'm mistaken
that was not always the case, so it would perhaps be preferrable to
not rely on that behavior and always explicitly set both disable-*
options when the new devices are not available; if the options
themselves are not available, then we should error out.

[...]
> @@ -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_TRANSITIONAL)

> +                return pciFlags;


Perhaps a short comment about how transitional VirtIO devices can
only be plugged into conventional PCI slots would be appropriate
here, for the benefit of those looking at the code years from now :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 21, 2019, 10:40 p.m. | #2
On 01/18/2019 07:33 AM, Andrea Bolognani wrote:
> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

> [...]

>> @@ -1108,6 +1110,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {

>>      { "vfio-ap", QEMU_CAPS_DEVICE_VFIO_AP },

>>      { "zpci", QEMU_CAPS_DEVICE_ZPCI },

>>      { "memory-backend-memfd", QEMU_CAPS_OBJECT_MEMORY_MEMFD },

>> +    {"virtio-blk-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL},

>> +    {"virtio-blk-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL},

> 

> There should be whitespace before and after curly braces, for

> consistency with existing entries.

> 


Indeed, I'll fix that in all the patches

> [...]

> 

>> +++ b/src/qemu/qemu_capabilities.h

>> @@ -504,6 +504,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */

>>      /* 325 */

>>      QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM, /* -object memory-backend-file,pmem= */

>>      QEMU_CAPS_DEVICE_NVDIMM_UNARMED, /* -device nvdimm,unarmed= */

>> +    QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL, /* -device virtio-blk-pci-transitional */

>> +    QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL, /* -device virtio-blk-pci-non-transitional */

> 

> A bit more verbose, but I think we should go for

> 

>   QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_{,NON_}TRANSITIONAL

> 

> since there are several non-PCI variants of VirtIO devices.

> 


Sure sounds good

> It'd also be nice if adding the capabilities and wiring up the

> command line generation bits happened in separate patches.

> 

> [...]

> 


OK

>> +static int

>> +qemuBuildVirtioTransitional(virBufferPtr buf,

>> +                            const char *baseName,

>> +                            virQEMUCapsPtr qemuCaps,

>> +                            virDomainDeviceAddressType type,

>> +                            int model,

>> +                            virDomainDeviceType devtype)

>> +{

>> +    int tmodel_cap, ntmodel_cap;

>> +    bool has_tmodel, has_ntmodel;

>> +

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

>> +        return -1;

>> +

>> +    switch (devtype) {

>> +        case VIR_DOMAIN_DEVICE_DISK:

>> +            has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;

>> +            has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;

>> +            tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL;

>> +            ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL;

>> +            break;

> 

> I like this approach much better than the one used in the RFC,

> eg. passing two booleans to the function. Nice!

> 

> What I don't like is that you're building this fairly thin wrapper

> around qemuBuildVirtioDevStr() when IMHO you should rather be

> agumenting the original function - mostly because the new name is

> not nearly as good :) Do you think you could make that happen?

> 


Hmm, seems weird to make call sites that will never support the
transitional naming (virtio-keyboard/mouse/tablet/gpu) have to call into
this function, passing DEVICE_TYPE etc. I can split the transitional
bits into their own function and not have it wrap BuildVirtioDevStr but
be a follow on, so for example:

if (qemuBuildVirtioDevStr(...) < 0)
    goto error;
if (qemuBuildVirtioTransitionalStr(...) < )
    goto error;

Does that work?

> [...]

>> +    if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&

>> +        (has_tmodel || has_ntmodel)) {

>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,

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

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

> 

> s/transitional/(non-)transitional/

> 

> [...]

>> +    if (has_tmodel) {

>> +        if (virQEMUCapsGet(qemuCaps, tmodel_cap))

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

>> +

>> +        /* No error for if -transitional is not supported: our address

>> +         * allocation will force the device into plain PCI bus, which

>> +         * is functionally identical to standard 'virtio-XXX' behavior

>> +         */

>> +    } else if (has_ntmodel) {

>> +        if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) {

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

>> +        } else if (virQEMUCapsGet(qemuCaps,

>> +                                  QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {

>> +            virBufferAddLit(buf, ",disable-legacy=on");

>> +        } else {

>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

>> +                           _("virtio non-transitional model not supported "

>> +                             "for this qemu"));

>> +            return -1;

>> +        }

>> +    }

> 

> Would it make sense to be more explicit here? Current versions of

> QEMU default to disable-modern=off,disable-legacy=off for virtio-pci

> devices plugged into conventional PCI slots, but unless I'm mistaken

> that was not always the case, so it would perhaps be preferrable to

> not rely on that behavior and always explicitly set both disable-*

> options when the new devices are not available; if the options

> themselves are not available, then we should error out.

> 


I'll respond separately

> [...]

>> @@ -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_TRANSITIONAL)

>> +                return pciFlags;

> 

> Perhaps a short comment about how transitional VirtIO devices can

> only be plugged into conventional PCI slots would be appropriate

> here, for the benefit of those looking at the code years from now :)

> 


This same pattern is duplicated in the rest of the series, seems weird
to comment one site, but commenting all of them is overkill. I guess
commenting one site is the middle ground unless you have a better idea
of where to put the comment

Thanks for the review
- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 21, 2019, 10:41 p.m. | #3
On 01/18/2019 07:33 AM, Andrea Bolognani wrote:
> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

>> +    if (has_tmodel) {

>> +        if (virQEMUCapsGet(qemuCaps, tmodel_cap))

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

>> +

>> +        /* No error for if -transitional is not supported: our address

>> +         * allocation will force the device into plain PCI bus, which

>> +         * is functionally identical to standard 'virtio-XXX' behavior

>> +         */

>> +    } else if (has_ntmodel) {

>> +        if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) {

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

>> +        } else if (virQEMUCapsGet(qemuCaps,

>> +                                  QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {

>> +            virBufferAddLit(buf, ",disable-legacy=on");

>> +        } else {

>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

>> +                           _("virtio non-transitional model not supported "

>> +                             "for this qemu"));

>> +            return -1;

>> +        }

>> +    }

> 

> Would it make sense to be more explicit here? Current versions of

> QEMU default to disable-modern=off,disable-legacy=off for virtio-pci

> devices plugged into conventional PCI slots, but unless I'm mistaken

> that was not always the case, so it would perhaps be preferrable to

> not rely on that behavior and always explicitly set both disable-*

> options when the new devices are not available; if the options

> themselves are not available, then we should error out.

> 


I don't know enough to say, CCing ehabkost and danpb for more eyes

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel P. Berrangé Jan. 22, 2019, 9:31 a.m. | #4
On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote:
> On 01/18/2019 07:33 AM, Andrea Bolognani wrote:

> > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

> >> +    if (has_tmodel) {

> >> +        if (virQEMUCapsGet(qemuCaps, tmodel_cap))

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

> >> +

> >> +        /* No error for if -transitional is not supported: our address

> >> +         * allocation will force the device into plain PCI bus, which

> >> +         * is functionally identical to standard 'virtio-XXX' behavior

> >> +         */

> >> +    } else if (has_ntmodel) {

> >> +        if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) {

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

> >> +        } else if (virQEMUCapsGet(qemuCaps,

> >> +                                  QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {

> >> +            virBufferAddLit(buf, ",disable-legacy=on");

> >> +        } else {

> >> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

> >> +                           _("virtio non-transitional model not supported "

> >> +                             "for this qemu"));

> >> +            return -1;

> >> +        }

> >> +    }

> > 

> > Would it make sense to be more explicit here? Current versions of

> > QEMU default to disable-modern=off,disable-legacy=off for virtio-pci

> > devices plugged into conventional PCI slots, but unless I'm mistaken

> > that was not always the case, so it would perhaps be preferrable to

> > not rely on that behavior and always explicitly set both disable-*

> > options when the new devices are not available; if the options

> > themselves are not available, then we should error out.

> > 

> 

> I don't know enough to say, CCing ehabkost and danpb for more eyes


The QEMU code for the devices has

 - Original devs:  disable-modern=off disable-legacy=auto
 - Transitional devs: disable-modern=off disable-legacy=off
 - Non-transitional devs: disable-modern=off disable-legacy=on

IOW, in the case that -transitional is not available, we could
set disable-legacy=off. Provided that we always place -transitional
devices into PCI slots, never PCI-e slots, whether we set
disable-legacy=off or not doesn't have any effect. None the less it
would make sense to set it explicitly though as that would cause us
to catch bugs if we mistakenly had a -transitional dev in a PCI-e
slot.

I don't see a need to set disable-modern at all, since it is
defaulting to "off" in all cases.

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. 22, 2019, 11:45 a.m. | #5
On Tue, 2019-01-22 at 09:31 +0000, Daniel P. Berrangé wrote:
> On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote:
> > On 01/18/2019 07:33 AM, Andrea Bolognani wrote:
> > > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
> > > > +    if (has_tmodel) {
> > > > +        if (virQEMUCapsGet(qemuCaps, tmodel_cap))
> > > > +            virBufferAddLit(buf, "-transitional");
> > > > +
> > > > +        /* No error for if -transitional is not supported: our address
> > > > +         * allocation will force the device into plain PCI bus, which
> > > > +         * is functionally identical to standard 'virtio-XXX' behavior
> > > > +         */
> > > > +    } else if (has_ntmodel) {
> > > > +        if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) {
> > > > +            virBufferAddLit(buf, "-non-transitional");
> > > > +        } else if (virQEMUCapsGet(qemuCaps,
> > > > +                                  QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {
> > > > +            virBufferAddLit(buf, ",disable-legacy=on");
> > > > +        } else {
> > > > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > > +                           _("virtio non-transitional model not supported "
> > > > +                             "for this qemu"));
> > > > +            return -1;
> > > > +        }
> > > > +    }
> > > 
> > > Would it make sense to be more explicit here? Current versions of
> > > QEMU default to disable-modern=off,disable-legacy=off for virtio-pci
> > > devices plugged into conventional PCI slots, but unless I'm mistaken
> > > that was not always the case, so it would perhaps be preferrable to
> > > not rely on that behavior and always explicitly set both disable-*
> > > options when the new devices are not available; if the options
> > > themselves are not available, then we should error out.
> > 
> > I don't know enough to say, CCing ehabkost and danpb for more eyes
> 
> The QEMU code for the devices has
> 
>  - Original devs:  disable-modern=off disable-legacy=auto
>  - Transitional devs: disable-modern=off disable-legacy=off
>  - Non-transitional devs: disable-modern=off disable-legacy=on
> 
> IOW, in the case that -transitional is not available, we could
> set disable-legacy=off. Provided that we always place -transitional
> devices into PCI slots, never PCI-e slots, whether we set
> disable-legacy=off or not doesn't have any effect. None the less it
> would make sense to set it explicitly though as that would cause us
> to catch bugs if we mistakenly had a -transitional dev in a PCI-e
> slot.
> 
> I don't see a need to set disable-modern at all, since it is
> defaulting to "off" in all cases.

Wasn't there some old QEMU release where disable-modern didn't
default to off? I seem to remember that.

More generally, I don't see a reason *not* to specify disable-modern
along with disable-legacy. Why be implicit when you can very easily
be explicit instead?
Andrea Bolognani Jan. 22, 2019, 11:53 a.m. | #6
On Mon, 2019-01-21 at 17:40 -0500, Cole Robinson wrote:
> On 01/18/2019 07:33 AM, Andrea Bolognani wrote:

> > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

> > > +static int

> > > +qemuBuildVirtioTransitional(virBufferPtr buf,

> > > +                            const char *baseName,

> > > +                            virQEMUCapsPtr qemuCaps,

> > > +                            virDomainDeviceAddressType type,

> > > +                            int model,

> > > +                            virDomainDeviceType devtype)

> > > +{

> > > +    int tmodel_cap, ntmodel_cap;

> > > +    bool has_tmodel, has_ntmodel;

> > > +

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

> > > +        return -1;

> > > +

> > > +    switch (devtype) {

> > > +        case VIR_DOMAIN_DEVICE_DISK:

> > > +            has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;

> > > +            has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;

> > > +            tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL;

> > > +            ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL;

> > > +            break;

> > 

> > I like this approach much better than the one used in the RFC,

> > eg. passing two booleans to the function. Nice!

> > 

> > What I don't like is that you're building this fairly thin wrapper

> > around qemuBuildVirtioDevStr() when IMHO you should rather be

> > agumenting the original function - mostly because the new name is

> > not nearly as good :) Do you think you could make that happen?

> 

> Hmm, seems weird to make call sites that will never support the

> transitional naming (virtio-keyboard/mouse/tablet/gpu) have to call into

> this function, passing DEVICE_TYPE etc. I can split the transitional

> bits into their own function and not have it wrap BuildVirtioDevStr but

> be a follow on, so for example:

> 

> if (qemuBuildVirtioDevStr(...) < 0)

>     goto error;

> if (qemuBuildVirtioTransitionalStr(...) < )

>     goto error;

> 

> Does that work?


The split version is more likely to end up being misused, so I
wouldn't go down that road.

As for the naming, my suggestion was to stick with the original
qemuBuildVirtioDevStr() name, add parameters to it, and not
introduce qemuBuildVirtioTransitional() at all, so it wouldn't look
weird when called for devices that are modern only.

I don't see a problem with passing devType even when you're not
ultimately going to make decisions based on it for certain devices.

> > [...]

> > > @@ -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_TRANSITIONAL)

> > > +                return pciFlags;

> > 

> > Perhaps a short comment about how transitional VirtIO devices can

> > only be plugged into conventional PCI slots would be appropriate

> > here, for the benefit of those looking at the code years from now :)

> 

> This same pattern is duplicated in the rest of the series, seems weird

> to comment one site, but commenting all of them is overkill. I guess

> commenting one site is the middle ground unless you have a better idea

> of where to put the comment


I don't think having a short comment such as

  /* Transitional devices only work in conventional PCI slots */

repeated a few times really counts as overkill :) So I'd go that way.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Eduardo Habkost Jan. 22, 2019, 12:44 p.m. | #7
On Tue, Jan 22, 2019 at 12:45:59PM +0100, Andrea Bolognani wrote:
> On Tue, 2019-01-22 at 09:31 +0000, Daniel P. Berrangé wrote:

> > On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote:

> > > On 01/18/2019 07:33 AM, Andrea Bolognani wrote:

> > > > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

> > > > > +    if (has_tmodel) {

> > > > > +        if (virQEMUCapsGet(qemuCaps, tmodel_cap))

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

> > > > > +

> > > > > +        /* No error for if -transitional is not supported: our address

> > > > > +         * allocation will force the device into plain PCI bus, which

> > > > > +         * is functionally identical to standard 'virtio-XXX' behavior

> > > > > +         */

> > > > > +    } else if (has_ntmodel) {

> > > > > +        if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) {

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

> > > > > +        } else if (virQEMUCapsGet(qemuCaps,

> > > > > +                                  QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {

> > > > > +            virBufferAddLit(buf, ",disable-legacy=on");

> > > > > +        } else {

> > > > > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

> > > > > +                           _("virtio non-transitional model not supported "

> > > > > +                             "for this qemu"));

> > > > > +            return -1;

> > > > > +        }

> > > > > +    }

> > > > 

> > > > Would it make sense to be more explicit here? Current versions of

> > > > QEMU default to disable-modern=off,disable-legacy=off for virtio-pci

> > > > devices plugged into conventional PCI slots, but unless I'm mistaken

> > > > that was not always the case, so it would perhaps be preferrable to

> > > > not rely on that behavior and always explicitly set both disable-*

> > > > options when the new devices are not available; if the options

> > > > themselves are not available, then we should error out.

> > > 

> > > I don't know enough to say, CCing ehabkost and danpb for more eyes

> > 

> > The QEMU code for the devices has

> > 

> >  - Original devs:  disable-modern=off disable-legacy=auto

> >  - Transitional devs: disable-modern=off disable-legacy=off

> >  - Non-transitional devs: disable-modern=off disable-legacy=on

> > 

> > IOW, in the case that -transitional is not available, we could

> > set disable-legacy=off. Provided that we always place -transitional

> > devices into PCI slots, never PCI-e slots, whether we set

> > disable-legacy=off or not doesn't have any effect. None the less it

> > would make sense to set it explicitly though as that would cause us

> > to catch bugs if we mistakenly had a -transitional dev in a PCI-e

> > slot.

> > 

> > I don't see a need to set disable-modern at all, since it is

> > defaulting to "off" in all cases.

> 

> Wasn't there some old QEMU release where disable-modern didn't

> default to off? I seem to remember that.


Yes, disable-modern is "on" on 2.6 and older machine-types.

> 

> More generally, I don't see a reason *not* to specify disable-modern

> along with disable-legacy. Why be implicit when you can very easily

> be explicit instead?


Agreed.

-- 
Eduardo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel P. Berrangé Jan. 22, 2019, 2 p.m. | #8
On Tue, Jan 22, 2019 at 10:44:23AM -0200, Eduardo Habkost wrote:
> On Tue, Jan 22, 2019 at 12:45:59PM +0100, Andrea Bolognani wrote:
> > On Tue, 2019-01-22 at 09:31 +0000, Daniel P. Berrangé wrote:
> > > On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote:
> > > > On 01/18/2019 07:33 AM, Andrea Bolognani wrote:
> > > > > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
> > > > > > +    if (has_tmodel) {
> > > > > > +        if (virQEMUCapsGet(qemuCaps, tmodel_cap))
> > > > > > +            virBufferAddLit(buf, "-transitional");
> > > > > > +
> > > > > > +        /* No error for if -transitional is not supported: our address
> > > > > > +         * allocation will force the device into plain PCI bus, which
> > > > > > +         * is functionally identical to standard 'virtio-XXX' behavior
> > > > > > +         */
> > > > > > +    } else if (has_ntmodel) {
> > > > > > +        if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) {
> > > > > > +            virBufferAddLit(buf, "-non-transitional");
> > > > > > +        } else if (virQEMUCapsGet(qemuCaps,
> > > > > > +                                  QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {
> > > > > > +            virBufferAddLit(buf, ",disable-legacy=on");
> > > > > > +        } else {
> > > > > > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > > > > +                           _("virtio non-transitional model not supported "
> > > > > > +                             "for this qemu"));
> > > > > > +            return -1;
> > > > > > +        }
> > > > > > +    }
> > > > > 
> > > > > Would it make sense to be more explicit here? Current versions of
> > > > > QEMU default to disable-modern=off,disable-legacy=off for virtio-pci
> > > > > devices plugged into conventional PCI slots, but unless I'm mistaken
> > > > > that was not always the case, so it would perhaps be preferrable to
> > > > > not rely on that behavior and always explicitly set both disable-*
> > > > > options when the new devices are not available; if the options
> > > > > themselves are not available, then we should error out.
> > > > 
> > > > I don't know enough to say, CCing ehabkost and danpb for more eyes
> > > 
> > > The QEMU code for the devices has
> > > 
> > >  - Original devs:  disable-modern=off disable-legacy=auto
> > >  - Transitional devs: disable-modern=off disable-legacy=off
> > >  - Non-transitional devs: disable-modern=off disable-legacy=on
> > > 
> > > IOW, in the case that -transitional is not available, we could
> > > set disable-legacy=off. Provided that we always place -transitional
> > > devices into PCI slots, never PCI-e slots, whether we set
> > > disable-legacy=off or not doesn't have any effect. None the less it
> > > would make sense to set it explicitly though as that would cause us
> > > to catch bugs if we mistakenly had a -transitional dev in a PCI-e
> > > slot.
> > > 
> > > I don't see a need to set disable-modern at all, since it is
> > > defaulting to "off" in all cases.
> > 
> > Wasn't there some old QEMU release where disable-modern didn't
> > default to off? I seem to remember that.
> 
> Yes, disable-modern is "on" on 2.6 and older machine-types.
> 
> > 
> > More generally, I don't see a reason *not* to specify disable-modern
> > along with disable-legacy. Why be implicit when you can very easily
> > be explicit instead?
> 
> Agreed.

Ok, I've no objection to an explicit disable-modern=off based on this
info.

Regards,
Daniel
Cole Robinson Jan. 22, 2019, 3:29 p.m. | #9
On 01/22/2019 09:00 AM, Daniel P. Berrangé wrote:
> On Tue, Jan 22, 2019 at 10:44:23AM -0200, Eduardo Habkost wrote:
>> On Tue, Jan 22, 2019 at 12:45:59PM +0100, Andrea Bolognani wrote:
>>> On Tue, 2019-01-22 at 09:31 +0000, Daniel P. Berrangé wrote:
>>>> On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote:
>>>>> On 01/18/2019 07:33 AM, Andrea Bolognani wrote:
>>>>>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
>>>>>>> +    if (has_tmodel) {
>>>>>>> +        if (virQEMUCapsGet(qemuCaps, tmodel_cap))
>>>>>>> +            virBufferAddLit(buf, "-transitional");
>>>>>>> +
>>>>>>> +        /* No error for if -transitional is not supported: our address
>>>>>>> +         * allocation will force the device into plain PCI bus, which
>>>>>>> +         * is functionally identical to standard 'virtio-XXX' behavior
>>>>>>> +         */
>>>>>>> +    } else if (has_ntmodel) {
>>>>>>> +        if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) {
>>>>>>> +            virBufferAddLit(buf, "-non-transitional");
>>>>>>> +        } else if (virQEMUCapsGet(qemuCaps,
>>>>>>> +                                  QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {
>>>>>>> +            virBufferAddLit(buf, ",disable-legacy=on");
>>>>>>> +        } else {
>>>>>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>>>>>> +                           _("virtio non-transitional model not supported "
>>>>>>> +                             "for this qemu"));
>>>>>>> +            return -1;
>>>>>>> +        }
>>>>>>> +    }
>>>>>>
>>>>>> Would it make sense to be more explicit here? Current versions of
>>>>>> QEMU default to disable-modern=off,disable-legacy=off for virtio-pci
>>>>>> devices plugged into conventional PCI slots, but unless I'm mistaken
>>>>>> that was not always the case, so it would perhaps be preferrable to
>>>>>> not rely on that behavior and always explicitly set both disable-*
>>>>>> options when the new devices are not available; if the options
>>>>>> themselves are not available, then we should error out.
>>>>>
>>>>> I don't know enough to say, CCing ehabkost and danpb for more eyes
>>>>
>>>> The QEMU code for the devices has
>>>>
>>>>  - Original devs:  disable-modern=off disable-legacy=auto
>>>>  - Transitional devs: disable-modern=off disable-legacy=off
>>>>  - Non-transitional devs: disable-modern=off disable-legacy=on
>>>>
>>>> IOW, in the case that -transitional is not available, we could
>>>> set disable-legacy=off. Provided that we always place -transitional
>>>> devices into PCI slots, never PCI-e slots, whether we set
>>>> disable-legacy=off or not doesn't have any effect. None the less it
>>>> would make sense to set it explicitly though as that would cause us
>>>> to catch bugs if we mistakenly had a -transitional dev in a PCI-e
>>>> slot.
>>>>
>>>> I don't see a need to set disable-modern at all, since it is
>>>> defaulting to "off" in all cases.
>>>
>>> Wasn't there some old QEMU release where disable-modern didn't
>>> default to off? I seem to remember that.
>>
>> Yes, disable-modern is "on" on 2.6 and older machine-types.
>>
>>>
>>> More generally, I don't see a reason *not* to specify disable-modern
>>> along with disable-legacy. Why be implicit when you can very easily
>>> be explicit instead?
>>
>> Agreed.
> 
> Ok, I've no objection to an explicit disable-modern=off based on this
> info.

Cool I'll make this change for the next round

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 22, 2019, 4:38 p.m. | #10
On 01/22/2019 06:53 AM, Andrea Bolognani wrote:
> On Mon, 2019-01-21 at 17:40 -0500, Cole Robinson wrote:

>> On 01/18/2019 07:33 AM, Andrea Bolognani wrote:

>>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

>>>> +static int

>>>> +qemuBuildVirtioTransitional(virBufferPtr buf,

>>>> +                            const char *baseName,

>>>> +                            virQEMUCapsPtr qemuCaps,

>>>> +                            virDomainDeviceAddressType type,

>>>> +                            int model,

>>>> +                            virDomainDeviceType devtype)

>>>> +{

>>>> +    int tmodel_cap, ntmodel_cap;

>>>> +    bool has_tmodel, has_ntmodel;

>>>> +

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

>>>> +        return -1;

>>>> +

>>>> +    switch (devtype) {

>>>> +        case VIR_DOMAIN_DEVICE_DISK:

>>>> +            has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;

>>>> +            has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;

>>>> +            tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL;

>>>> +            ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL;

>>>> +            break;

>>>

>>> I like this approach much better than the one used in the RFC,

>>> eg. passing two booleans to the function. Nice!

>>>

>>> What I don't like is that you're building this fairly thin wrapper

>>> around qemuBuildVirtioDevStr() when IMHO you should rather be

>>> agumenting the original function - mostly because the new name is

>>> not nearly as good :) Do you think you could make that happen?

>>

>> Hmm, seems weird to make call sites that will never support the

>> transitional naming (virtio-keyboard/mouse/tablet/gpu) have to call into

>> this function, passing DEVICE_TYPE etc. I can split the transitional

>> bits into their own function and not have it wrap BuildVirtioDevStr but

>> be a follow on, so for example:

>>

>> if (qemuBuildVirtioDevStr(...) < 0)

>>     goto error;

>> if (qemuBuildVirtioTransitionalStr(...) < )

>>     goto error;

>>

>> Does that work?

> 

> The split version is more likely to end up being misused, so I

> wouldn't go down that road.

> 

> As for the naming, my suggestion was to stick with the original

> qemuBuildVirtioDevStr() name, add parameters to it, and not

> introduce qemuBuildVirtioTransitional() at all, so it wouldn't look

> weird when called for devices that are modern only.

> 

> I don't see a problem with passing devType even when you're not

> ultimately going to make decisions based on it for certain devices.

> 


Okay I'll go that route

>>> [...]

>>>> @@ -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_TRANSITIONAL)

>>>> +                return pciFlags;

>>>

>>> Perhaps a short comment about how transitional VirtIO devices can

>>> only be plugged into conventional PCI slots would be appropriate

>>> here, for the benefit of those looking at the code years from now :)

>>

>> This same pattern is duplicated in the rest of the series, seems weird

>> to comment one site, but commenting all of them is overkill. I guess

>> commenting one site is the middle ground unless you have a better idea

>> of where to put the comment

> 

> I don't think having a short comment such as

> 

>   /* Transitional devices only work in conventional PCI slots */

> 

> repeated a few times really counts as overkill :) So I'd go that way.

> 


Cool, I'll add it to the series

Thanks,
Cole

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

Patch

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index f504db7d05..b7c0387f8e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -520,6 +520,8 @@  VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
               /* 325 */
               "memory-backend-file.pmem",
               "nvdimm.unarmed",
+              "virtio-blk-pci-transitional",
+              "virtio-blk-pci-non-transitional",
     );
 
 
@@ -1108,6 +1110,8 @@  struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
     { "vfio-ap", QEMU_CAPS_DEVICE_VFIO_AP },
     { "zpci", QEMU_CAPS_DEVICE_ZPCI },
     { "memory-backend-memfd", QEMU_CAPS_OBJECT_MEMORY_MEMFD },
+    {"virtio-blk-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL},
+    {"virtio-blk-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL},
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 6d5ed8a3cc..34265d7cc0 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -504,6 +504,8 @@  typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
     /* 325 */
     QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM, /* -object memory-backend-file,pmem= */
     QEMU_CAPS_DEVICE_NVDIMM_UNARMED, /* -device nvdimm,unarmed= */
+    QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL, /* -device virtio-blk-pci-transitional */
+    QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL, /* -device virtio-blk-pci-non-transitional */
 
     QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 822d5f8669..608cd65806 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -443,6 +443,91 @@  qemuBuildVirtioDevStr(virBufferPtr buf,
     return 0;
 }
 
+static int
+qemuBuildVirtioTransitional(virBufferPtr buf,
+                            const char *baseName,
+                            virQEMUCapsPtr qemuCaps,
+                            virDomainDeviceAddressType type,
+                            int model,
+                            virDomainDeviceType devtype)
+{
+    int tmodel_cap, ntmodel_cap;
+    bool has_tmodel, has_ntmodel;
+
+    if (qemuBuildVirtioDevStr(buf, baseName, type) < 0)
+        return -1;
+
+    switch (devtype) {
+        case VIR_DOMAIN_DEVICE_DISK:
+            has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;
+            has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;
+            tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL;
+            ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL;
+            break;
+
+        case VIR_DOMAIN_DEVICE_LEASE:
+        case VIR_DOMAIN_DEVICE_FS:
+        case VIR_DOMAIN_DEVICE_NET:
+        case VIR_DOMAIN_DEVICE_INPUT:
+        case VIR_DOMAIN_DEVICE_SOUND:
+        case VIR_DOMAIN_DEVICE_VIDEO:
+        case VIR_DOMAIN_DEVICE_HOSTDEV:
+        case VIR_DOMAIN_DEVICE_WATCHDOG:
+        case VIR_DOMAIN_DEVICE_CONTROLLER:
+        case VIR_DOMAIN_DEVICE_GRAPHICS:
+        case VIR_DOMAIN_DEVICE_HUB:
+        case VIR_DOMAIN_DEVICE_REDIRDEV:
+        case VIR_DOMAIN_DEVICE_NONE:
+        case VIR_DOMAIN_DEVICE_SMARTCARD:
+        case VIR_DOMAIN_DEVICE_CHR:
+        case VIR_DOMAIN_DEVICE_MEMBALLOON:
+        case VIR_DOMAIN_DEVICE_NVRAM:
+        case VIR_DOMAIN_DEVICE_SHMEM:
+        case VIR_DOMAIN_DEVICE_TPM:
+        case VIR_DOMAIN_DEVICE_PANIC:
+        case VIR_DOMAIN_DEVICE_RNG:
+        case VIR_DOMAIN_DEVICE_MEMORY:
+        case VIR_DOMAIN_DEVICE_IOMMU:
+        case VIR_DOMAIN_DEVICE_VSOCK:
+        case VIR_DOMAIN_DEVICE_LAST:
+        default:
+            return 0;
+    }
+
+    if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+        (has_tmodel || has_ntmodel)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("virtio transitional models are not supported "
+                         "for address type=%s"),
+                       virDomainDeviceAddressTypeToString(type));
+        return -1;
+    }
+
+    if (has_tmodel) {
+        if (virQEMUCapsGet(qemuCaps, tmodel_cap))
+            virBufferAddLit(buf, "-transitional");
+
+        /* No error for if -transitional is not supported: our address
+         * allocation will force the device into plain PCI bus, which
+         * is functionally identical to standard 'virtio-XXX' behavior
+         */
+    } else if (has_ntmodel) {
+        if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) {
+            virBufferAddLit(buf, "-non-transitional");
+        } else if (virQEMUCapsGet(qemuCaps,
+                                  QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {
+            virBufferAddLit(buf, ",disable-legacy=on");
+        } else {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("virtio non-transitional model not supported "
+                             "for this qemu"));
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
 
 static int
 qemuBuildVirtioOptionsStr(virBufferPtr buf,
@@ -2049,7 +2134,10 @@  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", qemuCaps,
+                                        disk->info.type,
+                                        disk->model,
+                                        VIR_DOMAIN_DEVICE_DISK) < 0)
             goto error;
 
         if (disk->iothread)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index bd6c4031e0..4a7c71d76d 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_TRANSITIONAL)
+                return pciFlags;
             return virtioFlags; /* only virtio disks use PCI */
 
         case VIR_DOMAIN_DISK_BUS_IDE:
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
index c2db392e83..8cf9083035 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
@@ -212,6 +212,8 @@ 
   <flag name='memory-backend-file.align'/>
   <flag name='memory-backend-file.pmem'/>
   <flag name='nvdimm.unarmed'/>
+  <flag name='virtio-blk-pci-transitional'/>
+  <flag name='virtio-blk-pci-non-transitional'/>
   <version>3001050</version>
   <kvmVersion>0</kvmVersion>
   <microcodeVersion>446361</microcodeVersion>
diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args
index 9e11e900da..9c5d553077 100644
--- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args
+++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.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,disable-legacy=on,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/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
index 070b4b8334..37078765bc 100644
--- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-non-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-non-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/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args
index 9e11e900da..356e8fdf4c 100644
--- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args
+++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args
@@ -25,9 +25,10 @@  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,\
+-device virtio-blk-pci,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 \
diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
index 070b4b8334..e78223eac8 100644
--- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-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-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/qemuxml2xmloutdata/virtio-transitional.xml b/tests/qemuxml2xmloutdata/virtio-transitional.xml
index 1d28af9abb..c19e133bb3 100644
--- a/tests/qemuxml2xmloutdata/virtio-transitional.xml
+++ b/tests/qemuxml2xmloutdata/virtio-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'/>