diff mbox series

[16/18] qemu: Support scsi controller model=virtio-{non-}transitional

Message ID 4d8c3b45d1e5a667908ab2596b59ef1881625449.1547746868.git.crobinso@redhat.com
State New
Headers show
Series qemu: virtio-{non-}transitional support | expand

Commit Message

Cole Robinson Jan. 17, 2019, 5:52 p.m. UTC
Add <controller type='scsi' model handling for virtio transitional
devices. Ex:

  <controller type='scsi' model='virtio-transitional'/>

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

The naming here doesn't match the pre-existing model=virtio-scsi.
The prescence of '-scsi' there seems kind of redundant as we have
type='scsi' already, so I decided to follow the pattern of other
patches and use virtio-transitional etc.

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

---
 docs/formatdomain.html.in                     |  4 ++--
 docs/schemas/domaincommon.rng                 |  2 ++
 src/conf/domain_conf.c                        |  4 +++-
 src/conf/domain_conf.h                        |  2 ++
 src/qemu/qemu_capabilities.c                  |  4 ++++
 src/qemu/qemu_capabilities.h                  |  2 ++
 src/qemu/qemu_command.c                       | 18 +++++++++++++--
 src/qemu/qemu_domain.c                        |  8 ++++++-
 src/qemu/qemu_domain_address.c                |  2 ++
 src/vbox/vbox_common.c                        |  2 ++
 src/vmx/vmx.c                                 |  4 +++-
 .../caps_4.0.0.x86_64.xml                     |  2 ++
 .../virtio-non-transitional.x86_64-3.1.0.args | 17 ++++++++------
 ...virtio-non-transitional.x86_64-latest.args | 17 ++++++++------
 .../virtio-non-transitional.xml               |  1 +
 .../virtio-transitional.x86_64-3.1.0.args     | 13 ++++++-----
 .../virtio-transitional.x86_64-latest.args    | 13 ++++++-----
 .../qemuxml2argvdata/virtio-transitional.xml  |  1 +
 .../virtio-non-transitional.xml               | 22 +++++++++++++------
 .../virtio-transitional.xml                   | 15 ++++++++-----
 tests/qemuxml2xmltest.c                       |  6 +++--
 21 files changed, 111 insertions(+), 48 deletions(-)

-- 
2.20.1

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

Comments

Andrea Bolognani Jan. 21, 2019, 4:49 p.m. UTC | #1
On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
> Add <controller type='scsi' model handling for virtio transitional

> devices. Ex:

> 

>   <controller type='scsi' model='virtio-transitional'/>

> 

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

> * "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional"

> 

> The naming here doesn't match the pre-existing model=virtio-scsi.

> The prescence of '-scsi' there seems kind of redundant as we have

> type='scsi' already, so I decided to follow the pattern of other

> patches and use virtio-transitional etc.


Completely agree with the rationale here; however, in order to
really match the way other devices are handled, I think we should
make it so you can use

  <controller type='scsi' model='virtio'/>

as well, which of course would behave the same as the currently
available version. What do you think?

[...]
> +++ b/src/conf/domain_conf.c

> @@ -359,7 +359,9 @@ VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAS

>                "vmpvscsi",

>                "ibmvscsi",

>                "virtio-scsi",

> -              "lsisas1078");

> +              "lsisas1078",

> +              "virtio-transitional",

> +              "virtio-non-transitional");


Same comment as always for VIR_ENUM_IMPL().

[...]
> @@ -1146,6 +1148,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {

>      {"vhost-vsock-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL},

>      {"virtio-input-host-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL},

>      {"virtio-input-host-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL},

> +    {"virtio-scsi-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL},

> +    {"virtio-scsi-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL},

>  };


Same comment as always for capabilities.

[...]
> @@ -507,12 +507,20 @@ qemuBuildVirtioTransitional(virBufferPtr buf,

>              tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL;

>              ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL;

>              break;


Empty line here.

> +        case VIR_DOMAIN_DEVICE_CONTROLLER:

> +            if (strstr(baseName, "scsi")) {

> +                has_tmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL;

> +                has_ntmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL;

> +                tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL;

> +                ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL;

> +                break;

> +            }

> +            return 0;


Using strstr() here is kinda crude, especially since the caller has
enough information to pass the appropriate virDomainControllerType
value, both in this case and later on for serial controllers.

I'd say just add yet another argument to the function. Yeah, it
starts to get quite unsightly, but I'd really rather not resort to
string comparison when a nice, type safe enum will do.

[...]
> +++ b/tests/qemuxml2xmltest.c

> @@ -1271,14 +1271,16 @@ mymain(void)

>              QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,

>              QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,

>              QEMU_CAPS_DEVICE_VHOST_VSOCK,

> -            QEMU_CAPS_VIRTIO_INPUT_HOST);

> +            QEMU_CAPS_VIRTIO_INPUT_HOST,

> +            QEMU_CAPS_VIRTIO_SCSI);

>      DO_TEST("virtio-non-transitional",

>              QEMU_CAPS_DEVICE_VIDEO_PRIMARY,

>              QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE,

>              QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,

>              QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,

>              QEMU_CAPS_DEVICE_VHOST_VSOCK,

> -            QEMU_CAPS_VIRTIO_INPUT_HOST);

> +            QEMU_CAPS_VIRTIO_INPUT_HOST,

> +            QEMU_CAPS_VIRTIO_SCSI);


This too could go in patch 2/18.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 22, 2019, 5:39 p.m. UTC | #2
On 01/21/2019 11:49 AM, Andrea Bolognani wrote:
> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

>> Add <controller type='scsi' model handling for virtio transitional

>> devices. Ex:

>>

>>   <controller type='scsi' model='virtio-transitional'/>

>>

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

>> * "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional"

>>

>> The naming here doesn't match the pre-existing model=virtio-scsi.

>> The prescence of '-scsi' there seems kind of redundant as we have

>> type='scsi' already, so I decided to follow the pattern of other

>> patches and use virtio-transitional etc.

> 

> Completely agree with the rationale here; however, in order to

> really match the way other devices are handled, I think we should

> make it so you can use

> 

>   <controller type='scsi' model='virtio'/>

> 

> as well, which of course would behave the same as the currently

> available version. What do you think?

> 


Yes I agree it would be nice to add that option. I suggest we make it a
separate issue from this series though incase it's a contentious idea,
v2 is already shaping up to be ~30 patches...

> 

>> +        case VIR_DOMAIN_DEVICE_CONTROLLER:

>> +            if (strstr(baseName, "scsi")) {

>> +                has_tmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL;

>> +                has_ntmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL;

>> +                tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL;

>> +                ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL;

>> +                break;

>> +            }

>> +            return 0;

> 

> Using strstr() here is kinda crude, especially since the caller has

> enough information to pass the appropriate virDomainControllerType

> value, both in this case and later on for serial controllers.

> 

> I'd say just add yet another argument to the function. Yeah, it

> starts to get quite unsightly, but I'd really rather not resort to

> string comparison when a nice, type safe enum will do.

> 


Yeah it's hacky. Adding another arg here is going to add extra pain if
when this is merged into qemuBuildVirtioStr, most callers are going have
NULL arguments, and an increased chance of new future callers passing in
incorrect values and accidentally triggering a wrong code path. I feel
like this is another argument for the separated BuildTransitional or
whatever, but I'm not sold either way so I'll stick with your suggestion

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 22, 2019, 9:27 p.m. UTC | #3
On 01/22/2019 12:39 PM, Cole Robinson wrote:
> On 01/21/2019 11:49 AM, Andrea Bolognani wrote:

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

>>> Add <controller type='scsi' model handling for virtio transitional

>>> devices. Ex:

>>>

>>>   <controller type='scsi' model='virtio-transitional'/>

>>>

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

>>> * "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional"

>>>

>>> The naming here doesn't match the pre-existing model=virtio-scsi.

>>> The prescence of '-scsi' there seems kind of redundant as we have

>>> type='scsi' already, so I decided to follow the pattern of other

>>> patches and use virtio-transitional etc.

>>

>> Completely agree with the rationale here; however, in order to

>> really match the way other devices are handled, I think we should

>> make it so you can use

>>

>>   <controller type='scsi' model='virtio'/>

>>

>> as well, which of course would behave the same as the currently

>> available version. What do you think?

>>

> 

> Yes I agree it would be nice to add that option. I suggest we make it a

> separate issue from this series though incase it's a contentious idea,

> v2 is already shaping up to be ~30 patches...

> 

>>

>>> +        case VIR_DOMAIN_DEVICE_CONTROLLER:

>>> +            if (strstr(baseName, "scsi")) {

>>> +                has_tmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL;

>>> +                has_ntmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL;

>>> +                tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL;

>>> +                ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL;

>>> +                break;

>>> +            }

>>> +            return 0;

>>

>> Using strstr() here is kinda crude, especially since the caller has

>> enough information to pass the appropriate virDomainControllerType

>> value, both in this case and later on for serial controllers.

>>

>> I'd say just add yet another argument to the function. Yeah, it

>> starts to get quite unsightly, but I'd really rather not resort to

>> string comparison when a nice, type safe enum will do.

>>

> 

> Yeah it's hacky. Adding another arg here is going to add extra pain if

> when this is merged into qemuBuildVirtioStr, most callers are going have

> NULL arguments, and an increased chance of new future callers passing in

> incorrect values and accidentally triggering a wrong code path. I feel

> like this is another argument for the separated BuildTransitional or

> whatever, but I'm not sold either way so I'll stick with your suggestion

> 


What do you think of this approach? See the attached two patches. It
adds a domain_conf.c function virDomainDeviceSetData which makes it
easier to create a virDomainDeviceDef. qemuBuildVirtioDevStr callers now
pass in a virDomainDeviceType and the specific DefPtr for their device
(virDomainDiskDefPtr, virDomainNetDefPtr etc). qemuBuildVirtioDevStr
then turns that into a virDomainDeviceDef and acts on that locally.

Saves having to extend the argument list several times (like this
example above, and the net->model string example). Seperately I think
virDomainDeviceSetData can be used to clean up some device interactions
elsewhere too

Thanks,
Cole
>From 26a76becc0297a4fbae572ac328e9068a0141174 Mon Sep 17 00:00:00 2001

Message-Id: <26a76becc0297a4fbae572ac328e9068a0141174.1548192116.git.crobinso@redhat.com>
In-Reply-To: <b477d397deac304be61d60b4fc4592b4faa957ab.1548192116.git.crobinso@redhat.com>
References: <b477d397deac304be61d60b4fc4592b4faa957ab.1548192116.git.crobinso@redhat.com>
From: Cole Robinson <crobinso@redhat.com>

Date: Tue, 22 Jan 2019 16:15:03 -0500
Subject: [PATCH 2/2] qemu: command: Make BuildVirtioDevStr more generic

Switch qemuBuildVirtioDevStr to use virDomainDeviceSetData: callers
pass in the virDomainDeviceType and the void * DefPtr. This will
save us from having to repeatedly extend the function argument
list in subsequent patches.

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

---
 src/qemu/qemu_command.c | 141 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 124 insertions(+), 17 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bb4eb97da4..ef0b6399ad 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -396,12 +396,93 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
 }
 
 
+/**
+ * qemuBuildVirtioDevStr
+ * @buf: virBufferPtr to append the built string
+ * @baseName: qemu virtio device basename string. Ex: virtio-rng for <rng>
+ * @qemuCaps: virQEMUCapPtr
+ * @devtype: virDomainDeviceType of the device. Ex: VIR_DOMAIN_DEVICE_TYPE_RNG
+ * @devdata: *DefPtr of the device definition
+ *
+ * Build the qemu virtio -device name from the passed parameters. Currently
+ * this is mostly about attaching the correct string prefix to @baseName for
+ * the passed @type. So for @baseName "virtio-rng" and devdata->info.type
+ * VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, generate "virtio-rng-pci"
+ *
+ * Returns: -1 on failure, 0 on success
+ */
 static int
 qemuBuildVirtioDevStr(virBufferPtr buf,
                       const char *baseName,
-                      virDomainDeviceAddressType type)
+                      virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED,
+                      virDomainDeviceType devtype,
+                      void *devdata)
 {
     const char *implName = NULL;
+    virDomainDeviceAddressType type = 0;
+    virDomainDeviceDef device = { .type = devtype };
+
+    virDomainDeviceSetData(&device, devdata);
+
+    switch ((virDomainDeviceType) device.type) {
+    case VIR_DOMAIN_DEVICE_DISK:
+        type = device.data.disk->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_NET:
+        type = device.data.net->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_FS:
+        type = device.data.fs->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_INPUT:
+        type = device.data.input->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_VIDEO:
+        type = device.data.video->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_HOSTDEV:
+        type = device.data.hostdev->info->type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_CONTROLLER:
+        type = device.data.controller->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_MEMBALLOON:
+        type = device.data.memballoon->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_RNG:
+        type = device.data.rng->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_VSOCK:
+        type = device.data.vsock->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_CHR:
+    case VIR_DOMAIN_DEVICE_LEASE:
+    case VIR_DOMAIN_DEVICE_SOUND:
+    case VIR_DOMAIN_DEVICE_WATCHDOG:
+    case VIR_DOMAIN_DEVICE_GRAPHICS:
+    case VIR_DOMAIN_DEVICE_HUB:
+    case VIR_DOMAIN_DEVICE_REDIRDEV:
+    case VIR_DOMAIN_DEVICE_SMARTCARD:
+    case VIR_DOMAIN_DEVICE_NVRAM:
+    case VIR_DOMAIN_DEVICE_SHMEM:
+    case VIR_DOMAIN_DEVICE_TPM:
+    case VIR_DOMAIN_DEVICE_PANIC:
+    case VIR_DOMAIN_DEVICE_MEMORY:
+    case VIR_DOMAIN_DEVICE_IOMMU:
+    case VIR_DOMAIN_DEVICE_NONE:
+    case VIR_DOMAIN_DEVICE_LAST:
+        return 0;
+    }
 
     switch (type) {
     case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
@@ -443,7 +524,6 @@ qemuBuildVirtioDevStr(virBufferPtr buf,
     return 0;
 }
 
-
 static int
 qemuBuildVirtioOptionsStr(virBufferPtr buf,
                           virDomainVirtioOptionsPtr virtio,
@@ -2050,8 +2130,10 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
         break;
 
     case VIR_DOMAIN_DISK_BUS_VIRTIO:
-        if (qemuBuildVirtioDevStr(&opt, "virtio-blk", disk->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&opt, "virtio-blk", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_DISK, disk) < 0) {
             goto error;
+        }
 
         if (disk->iothread)
             virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread);
@@ -2639,8 +2721,10 @@ qemuBuildFSDevStr(const virDomainDef *def,
         goto error;
     }
 
-    if (qemuBuildVirtioDevStr(&opt, "virtio-9p", fs->info.type) < 0)
+    if (qemuBuildVirtioDevStr(&opt, "virtio-9p", qemuCaps,
+                              VIR_DOMAIN_DEVICE_FS, fs) < 0) {
         goto error;
+    }
 
     virBufferAsprintf(&opt, ",id=%s", fs->info.alias);
     virBufferAsprintf(&opt, ",fsdev=%s%s",
@@ -2845,8 +2929,10 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
     case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
         switch ((virDomainControllerModelSCSI) def->model) {
         case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
-            if (qemuBuildVirtioDevStr(&buf, "virtio-scsi", def->info.type) < 0)
+            if (qemuBuildVirtioDevStr(&buf, "virtio-scsi", qemuCaps,
+                                      VIR_DOMAIN_DEVICE_CONTROLLER, def) < 0) {
                 goto error;
+            }
 
             if (def->iothread) {
                 virBufferAsprintf(&buf, ",iothread=iothread%u",
@@ -2886,8 +2972,10 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
         break;
 
     case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
-        if (qemuBuildVirtioDevStr(&buf, "virtio-serial", def->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-serial", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_CONTROLLER, def) < 0) {
             goto error;
+        }
 
         virBufferAsprintf(&buf, ",id=%s", def->info.alias);
         if (def->opts.vioserial.ports != -1) {
@@ -3655,8 +3743,10 @@ qemuBuildNicDevStr(virDomainDefPtr def,
     char macaddr[VIR_MAC_STRING_BUFLEN];
 
     if (virDomainNetIsVirtioModel(net)) {
-        if (qemuBuildVirtioDevStr(&buf, "virtio-net", net->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-net", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_NET, net) < 0) {
             goto error;
+        }
 
         usingVirtio = true;
     } else {
@@ -4049,8 +4139,9 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd,
     if (!virDomainDefHasMemballoon(def))
         return 0;
 
-    if (qemuBuildVirtioDevStr(&buf, "virtio-balloon",
-                              def->memballoon->info.type) < 0) {
+    if (qemuBuildVirtioDevStr(&buf, "virtio-balloon", qemuCaps,
+                              VIR_DOMAIN_DEVICE_MEMBALLOON,
+                              def->memballoon) < 0) {
         goto error;
     }
 
@@ -4148,20 +4239,28 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def,
 
     switch ((virDomainInputType)dev->type) {
     case VIR_DOMAIN_INPUT_TYPE_MOUSE:
-        if (qemuBuildVirtioDevStr(&buf, "virtio-mouse", dev->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-mouse", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_INPUT, dev) < 0) {
             goto error;
+        }
         break;
     case VIR_DOMAIN_INPUT_TYPE_TABLET:
-        if (qemuBuildVirtioDevStr(&buf, "virtio-tablet", dev->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-tablet", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_INPUT, dev) < 0) {
             goto error;
+        }
         break;
     case VIR_DOMAIN_INPUT_TYPE_KBD:
-        if (qemuBuildVirtioDevStr(&buf, "virtio-keyboard", dev->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-keyboard", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_INPUT, dev) < 0) {
             goto error;
+        }
         break;
     case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
-        if (qemuBuildVirtioDevStr(&buf, "virtio-input-host", dev->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-input-host", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_INPUT, dev) < 0) {
             goto error;
+        }
         break;
     case VIR_DOMAIN_INPUT_TYPE_LAST:
     default:
@@ -4479,8 +4578,10 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
     }
 
     if (STREQ(model, "virtio-gpu")) {
-        if (qemuBuildVirtioDevStr(&buf, "virtio-gpu", video->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-gpu", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_VIDEO, video) < 0) {
             goto error;
+        }
     } else {
         virBufferAsprintf(&buf, "%s", model);
     }
@@ -4925,8 +5026,10 @@ qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def,
         goto cleanup;
     }
 
-    if (qemuBuildVirtioDevStr(&buf, "vhost-scsi", dev->info->type) < 0)
+    if (qemuBuildVirtioDevStr(&buf, "vhost-scsi", qemuCaps,
+                              VIR_DOMAIN_DEVICE_HOSTDEV, dev) < 0) {
         goto cleanup;
+    }
 
     virBufferAsprintf(&buf, ",wwpn=%s,vhostfd=%s,id=%s",
                       hostsrc->wwpn,
@@ -5873,8 +5976,10 @@ qemuBuildRNGDevStr(const virDomainDef *def,
                                               dev->source.file))
         goto error;
 
-    if (qemuBuildVirtioDevStr(&buf, "virtio-rng", dev->info.type) < 0)
+    if (qemuBuildVirtioDevStr(&buf, "virtio-rng", qemuCaps,
+                              VIR_DOMAIN_DEVICE_RNG, dev) < 0) {
         goto error;
+    }
 
     virBufferAsprintf(&buf, ",rng=obj%s,id=%s",
                       dev->info.alias, dev->info.alias);
@@ -10337,8 +10442,10 @@ qemuBuildVsockDevStr(virDomainDefPtr def,
     char *ret = NULL;
 
 
-    if (qemuBuildVirtioDevStr(&buf, "vhost-vsock", vsock->info.type) < 0)
+    if (qemuBuildVirtioDevStr(&buf, "vhost-vsock", qemuCaps,
+                              VIR_DOMAIN_DEVICE_VSOCK, vsock) < 0) {
         goto cleanup;
+    }
 
     virBufferAsprintf(&buf, ",id=%s", vsock->info.alias);
     virBufferAsprintf(&buf, ",guest-cid=%u", vsock->guest_cid);
-- 
2.20.1
>From b477d397deac304be61d60b4fc4592b4faa957ab Mon Sep 17 00:00:00 2001

Message-Id: <b477d397deac304be61d60b4fc4592b4faa957ab.1548192116.git.crobinso@redhat.com>
From: Cole Robinson <crobinso@redhat.com>

Date: Tue, 22 Jan 2019 15:19:29 -0500
Subject: [PATCH 1/2] conf: Add virDomainDeviceSetData

This is essentially a wrapper for easily setting the variable
name in virDomainDeviceDef that matches its associated
VIR_DOMAIN_DEVICE_TYPE.

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

---
 src/conf/domain_conf.c   | 93 ++++++++++++++++++++++++++++++++++++++++
 src/conf/domain_conf.h   |  3 ++
 src/libvirt_private.syms |  1 +
 3 files changed, 97 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index dc17a2fd59..313e1c1748 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3701,6 +3701,99 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
 }
 
 
+/**
+ * virDomainDeviceSetData
+ * @device: virDomainDeviceDefPtr with ->type filled in
+ * @data: *DefPtr data for a device. Ex: virDomainDiskDefPtr
+ *
+ * Set the data.X variable for the device->type value. Basically
+ * a mapping of virDomainDeviceType to the associated name in
+ * the virDomainDeviceDef union
+ */
+void
+virDomainDeviceSetData(virDomainDeviceDefPtr device,
+                       void *devicedata)
+{
+    switch ((virDomainDeviceType) device->type) {
+    case VIR_DOMAIN_DEVICE_DISK:
+        device->data.disk = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_NET:
+        device->data.net = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_SOUND:
+        device->data.sound = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_HOSTDEV:
+        device->data.hostdev = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_VIDEO:
+        device->data.video = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_CONTROLLER:
+        device->data.controller = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_GRAPHICS:
+        device->data.graphics = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_SMARTCARD:
+        device->data.smartcard = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_CHR:
+        device->data.chr = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_INPUT:
+        device->data.input = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_FS:
+        device->data.fs = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_WATCHDOG:
+        device->data.watchdog = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_MEMBALLOON:
+        device->data.memballoon = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_RNG:
+        device->data.rng = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_NVRAM:
+        device->data.nvram = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_HUB:
+        device->data.hub = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_SHMEM:
+        device->data.shmem = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_TPM:
+        device->data.tpm = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_PANIC:
+        device->data.panic = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_MEMORY:
+        device->data.memory = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_REDIRDEV:
+        device->data.redirdev = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_VSOCK:
+        device->data.vsock = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_IOMMU:
+        device->data.iommu = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_LEASE:
+        device->data.lease = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_NONE:
+    case VIR_DOMAIN_DEVICE_LAST:
+        break;
+    }
+}
+
+
 enum {
     DOMAIN_DEVICE_ITERATE_ALL_CONSOLES = 1 << 0,
     DOMAIN_DEVICE_ITERATE_GRAPHICS = 1 << 1
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index dcd84e2d94..b5894b96b6 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2934,6 +2934,9 @@ virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
 virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device);
 void virDomainTPMDefFree(virDomainTPMDefPtr def);
 
+void virDomainDeviceSetData(virDomainDeviceDefPtr device,
+                            void *devicedata);
+
 typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def,
                                            virDomainDeviceDefPtr dev,
                                            virDomainDeviceInfoPtr info,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6f4809a68a..89b8ca3b4f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -307,6 +307,7 @@ virDomainDeviceDefParse;
 virDomainDeviceFindSCSIController;
 virDomainDeviceGetInfo;
 virDomainDeviceInfoIterate;
+virDomainDeviceSetData;
 virDomainDeviceTypeToString;
 virDomainDeviceValidateAliasForHotplug;
 virDomainDiskBusTypeToString;
-- 
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani Jan. 23, 2019, 12:08 p.m. UTC | #4
On Tue, 2019-01-22 at 16:27 -0500, Cole Robinson wrote:
> On 01/22/2019 12:39 PM, Cole Robinson wrote:

> > On 01/21/2019 11:49 AM, Andrea Bolognani wrote:

> > > Completely agree with the rationale here; however, in order to

> > > really match the way other devices are handled, I think we should

> > > make it so you can use

> > > 

> > >   <controller type='scsi' model='virtio'/>

> > > 

> > > as well, which of course would behave the same as the currently

> > > available version. What do you think?

> > 

> > Yes I agree it would be nice to add that option. I suggest we make it a

> > separate issue from this series though incase it's a contentious idea,

> > v2 is already shaping up to be ~30 patches...


I don't think that's going to be particularly controversial, and we
should really make sure we get all the user-facing bits in at the
same time IMHO, so I'd say go for it... If you're really unsure
about it you can add that model in a separate patch right after this
one, that way if someone happens not to like that we can drop it and
otherwise we can squash them together before pushing.

> > > Using strstr() here is kinda crude, especially since the caller has

> > > enough information to pass the appropriate virDomainControllerType

> > > value, both in this case and later on for serial controllers.

> > > 

> > > I'd say just add yet another argument to the function. Yeah, it

> > > starts to get quite unsightly, but I'd really rather not resort to

> > > string comparison when a nice, type safe enum will do.

> > 

> > Yeah it's hacky. Adding another arg here is going to add extra pain if

> > when this is merged into qemuBuildVirtioStr, most callers are going have

> > NULL arguments, and an increased chance of new future callers passing in

> > incorrect values and accidentally triggering a wrong code path. I feel

> > like this is another argument for the separated BuildTransitional or

> > whatever, but I'm not sold either way so I'll stick with your suggestion

> 

> What do you think of this approach? See the attached two patches. It

> adds a domain_conf.c function virDomainDeviceSetData which makes it

> easier to create a virDomainDeviceDef. qemuBuildVirtioDevStr callers now

> pass in a virDomainDeviceType and the specific DefPtr for their device

> (virDomainDiskDefPtr, virDomainNetDefPtr etc). qemuBuildVirtioDevStr

> then turns that into a virDomainDeviceDef and acts on that locally.

> 

> Saves having to extend the argument list several times (like this

> example above, and the net->model string example). Seperately I think

> virDomainDeviceSetData can be used to clean up some device interactions

> elsewhere too


I like it! I actually wanted to suggest something like that earlier,
but for some reason I thought it would be more complicated than it
turned out to be...

Better yet, you don't even need to add that switch statement to
qemuBuildVirtioDevStr(): you can just use virDomainDeviceGetInfo()
instead, thus making the code shorter and nicer :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 23, 2019, 12:52 p.m. UTC | #5
On 01/23/2019 07:08 AM, Andrea Bolognani wrote:
> On Tue, 2019-01-22 at 16:27 -0500, Cole Robinson wrote:

>> On 01/22/2019 12:39 PM, Cole Robinson wrote:

>>> On 01/21/2019 11:49 AM, Andrea Bolognani wrote:

>>>> Completely agree with the rationale here; however, in order to

>>>> really match the way other devices are handled, I think we should

>>>> make it so you can use

>>>>

>>>>   <controller type='scsi' model='virtio'/>

>>>>

>>>> as well, which of course would behave the same as the currently

>>>> available version. What do you think?

>>>

>>> Yes I agree it would be nice to add that option. I suggest we make it a

>>> separate issue from this series though incase it's a contentious idea,

>>> v2 is already shaping up to be ~30 patches...

> 

> I don't think that's going to be particularly controversial, and we

> should really make sure we get all the user-facing bits in at the

> same time IMHO, so I'd say go for it... If you're really unsure

> about it you can add that model in a separate patch right after this

> one, that way if someone happens not to like that we can drop it and

> otherwise we can squash them together before pushing.

> 


Alright will do

>>>> Using strstr() here is kinda crude, especially since the caller has

>>>> enough information to pass the appropriate virDomainControllerType

>>>> value, both in this case and later on for serial controllers.

>>>>

>>>> I'd say just add yet another argument to the function. Yeah, it

>>>> starts to get quite unsightly, but I'd really rather not resort to

>>>> string comparison when a nice, type safe enum will do.

>>>

>>> Yeah it's hacky. Adding another arg here is going to add extra pain if

>>> when this is merged into qemuBuildVirtioStr, most callers are going have

>>> NULL arguments, and an increased chance of new future callers passing in

>>> incorrect values and accidentally triggering a wrong code path. I feel

>>> like this is another argument for the separated BuildTransitional or

>>> whatever, but I'm not sold either way so I'll stick with your suggestion

>>

>> What do you think of this approach? See the attached two patches. It

>> adds a domain_conf.c function virDomainDeviceSetData which makes it

>> easier to create a virDomainDeviceDef. qemuBuildVirtioDevStr callers now

>> pass in a virDomainDeviceType and the specific DefPtr for their device

>> (virDomainDiskDefPtr, virDomainNetDefPtr etc). qemuBuildVirtioDevStr

>> then turns that into a virDomainDeviceDef and acts on that locally.

>>

>> Saves having to extend the argument list several times (like this

>> example above, and the net->model string example). Seperately I think

>> virDomainDeviceSetData can be used to clean up some device interactions

>> elsewhere too

> 

> I like it! I actually wanted to suggest something like that earlier,

> but for some reason I thought it would be more complicated than it

> turned out to be...

> 

> Better yet, you don't even need to add that switch statement to

> qemuBuildVirtioDevStr(): you can just use virDomainDeviceGetInfo()

> instead, thus making the code shorter and nicer :)

> 


Ah good catch, though the switch statement will end up in the final
result anyways with all the transitional additions

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani Jan. 23, 2019, 1:03 p.m. UTC | #6
On Wed, 2019-01-23 at 07:52 -0500, Cole Robinson wrote:
> On 01/23/2019 07:08 AM, Andrea Bolognani wrote:

> > Better yet, you don't even need to add that switch statement to

> > qemuBuildVirtioDevStr(): you can just use virDomainDeviceGetInfo()

> > instead, thus making the code shorter and nicer :)

> 

> Ah good catch, though the switch statement will end up in the final

> result anyways with all the transitional additions


That's a very solid point :) Sorry for the noise.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 23, 2019, 11:58 p.m. UTC | #7
On 01/23/2019 07:52 AM, Cole Robinson wrote:
> On 01/23/2019 07:08 AM, Andrea Bolognani wrote:

>> On Tue, 2019-01-22 at 16:27 -0500, Cole Robinson wrote:

>>> On 01/22/2019 12:39 PM, Cole Robinson wrote:

>>>> On 01/21/2019 11:49 AM, Andrea Bolognani wrote:

>>>>> Completely agree with the rationale here; however, in order to

>>>>> really match the way other devices are handled, I think we should

>>>>> make it so you can use

>>>>>

>>>>>   <controller type='scsi' model='virtio'/>

>>>>>

>>>>> as well, which of course would behave the same as the currently

>>>>> available version. What do you think?

>>>>

>>>> Yes I agree it would be nice to add that option. I suggest we make it a

>>>> separate issue from this series though incase it's a contentious idea,

>>>> v2 is already shaping up to be ~30 patches...

>>

>> I don't think that's going to be particularly controversial, and we

>> should really make sure we get all the user-facing bits in at the

>> same time IMHO, so I'd say go for it... If you're really unsure

>> about it you can add that model in a separate patch right after this

>> one, that way if someone happens not to like that we can drop it and

>> otherwise we can squash them together before pushing.

>>

> 

> Alright will do

> 


I'm realizing now I forgot this, sorry. I'll add it to my todo list

Thanks,
Cole

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

Patch

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 63bdf2c86b..bdf533ba3e 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4139,8 +4139,8 @@ 
         <dt><code>scsi</code></dt>
         <dd>A <code>scsi</code> controller has an optional attribute
         <code>model</code>, which is one of 'auto', 'buslogic', 'ibmvscsi',
-        'lsilogic', 'lsisas1068', 'lsisas1078', 'virtio-scsi' or
-        'vmpvscsi'.</dd>
+        'lsilogic', 'lsisas1068', 'lsisas1078', 'virtio-scsi',
+        'vmpvscsi', 'virtio-transitional', 'virtio-non-transitional'</dd>
         <dt><code>usb</code></dt>
         <dd>A <code>usb</code> controller has an optional attribute
         <code>model</code>, which is one of "piix3-uhci", "piix4-uhci",
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 58c449ca80..242f237360 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2152,6 +2152,8 @@ 
                   <value>ibmvscsi</value>
                   <value>virtio-scsi</value>
                   <value>lsisas1078</value>
+                  <value>virtio-transitional</value>
+                  <value>virtio-non-transitional</value>
                 </choice>
               </attribute>
             </optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index cbba02929c..76955b6d78 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -359,7 +359,9 @@  VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAS
               "vmpvscsi",
               "ibmvscsi",
               "virtio-scsi",
-              "lsisas1078");
+              "lsisas1078",
+              "virtio-transitional",
+              "virtio-non-transitional");
 
 VIR_ENUM_IMPL(virDomainControllerModelUSB, VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST,
               "piix3-uhci",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ded84f52d5..9771b0b3d7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -756,6 +756,8 @@  typedef enum {
     VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI,
     VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI,
     VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078,
+    VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL,
+    VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL,
 
     VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST
 } virDomainControllerModelSCSI;
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index f5b9bc6937..0838f2a406 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -542,6 +542,8 @@  VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
               "vhost-vsock-pci-non-transitional",
               "virtio-input-host-pci-transitional",
               "virtio-input-host-pci-non-transitional",
+              "virtio-scsi-pci-transitional",
+              "virtio-scsi-pci-non-transitional",
     );
 
 
@@ -1146,6 +1148,8 @@  struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
     {"vhost-vsock-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL},
     {"virtio-input-host-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL},
     {"virtio-input-host-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL},
+    {"virtio-scsi-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL},
+    {"virtio-scsi-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL},
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 91aae8df5b..d23bd251f2 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -526,6 +526,8 @@  typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
     QEMU_CAPS_DEVICE_VHOST_VSOCK_NON_TRANSITIONAL, /* -device vhost-vsock-pci-non-transitional */
     QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL, /* -device virtio-input-host-pci-transitional */
     QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL, /* -device virtio-input-host-pci-non-transitional */
+    QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL, /* -device virtio-scsi-pci-transitional */
+    QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL, /* -device virtio-scsi-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 4d14ac9e5d..50042b2919 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -507,12 +507,20 @@  qemuBuildVirtioTransitional(virBufferPtr buf,
             tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_TRANSITIONAL;
             ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_INPUT_HOST_NON_TRANSITIONAL;
             break;
+        case VIR_DOMAIN_DEVICE_CONTROLLER:
+            if (strstr(baseName, "scsi")) {
+                has_tmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL;
+                has_ntmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL;
+                tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL;
+                ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL;
+                break;
+            }
+            return 0;
 
         case VIR_DOMAIN_DEVICE_LEASE:
         case VIR_DOMAIN_DEVICE_SOUND:
         case VIR_DOMAIN_DEVICE_VIDEO:
         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:
@@ -2971,8 +2979,14 @@  qemuBuildControllerDevStr(const virDomainDef *domainDef,
     case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
         switch ((virDomainControllerModelSCSI) def->model) {
         case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
-            if (qemuBuildVirtioDevStr(&buf, "virtio-scsi", def->info.type) < 0)
+        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
+        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
+            if (qemuBuildVirtioTransitional(&buf, "virtio-scsi", qemuCaps,
+                                            def->info.type,
+                                            def->model, NULL,
+                                            VIR_DOMAIN_DEVICE_CONTROLLER) < 0) {
                 goto error;
+            }
 
             if (def->iothread) {
                 virBufferAsprintf(&buf, ",iothread=iothread%u",
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6d54727a31..85f7b6a4c8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4972,7 +4972,9 @@  static int
 qemuDomainDeviceDefValidateControllerAttributes(const virDomainControllerDef *controller)
 {
     if (!(controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
-          controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
+          (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI ||
+           controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL ||
+           controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL))) {
         if (controller->queues) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("'queues' is only supported by virtio-scsi controller"));
@@ -5026,6 +5028,8 @@  qemuDomainCheckSCSIControllerModel(virQEMUCapsPtr qemuCaps,
         }
         break;
     case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+    case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
+    case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
         if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("This QEMU doesn't support "
@@ -5141,6 +5145,8 @@  qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll
 {
     switch ((virDomainControllerModelSCSI) controller->model) {
         case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
+        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
             if (!qemuDomainCheckSCSIControllerIOThreads(controller, def))
                 return -1;
             break;
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 70591dce25..9d07fa133e 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -651,8 +651,10 @@  qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
                 return 0;
 
             case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+            case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
                 return virtioFlags;
 
+            case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
             case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
             case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
             case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 664650f217..00d43d9a83 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -406,6 +406,8 @@  vboxSetStorageController(virDomainControllerDefPtr controller,
         case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
         case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
         case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
+        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
+        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
         case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            _("The vbox driver does not support %s SCSI "
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 92601291fd..a853c05e86 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -517,7 +517,9 @@  VIR_ENUM_IMPL(virVMXControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST,
               "pvscsi",
               "UNUSED ibmvscsi",
               "UNUSED virtio-scsi",
-              "UNUSED lsisas1078");
+              "UNUSED lsisas1078",
+              "UNUSED virtio-transitional",
+              "UNUSED virtio-non-transitional");
 
 
 /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
index d5bbbc40f7..aa9a0d2d3b 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
@@ -228,6 +228,8 @@ 
   <flag name='vhost-vsock-pci-non-transitional'/>
   <flag name='virtio-input-host-pci-transitional'/>
   <flag name='virtio-input-host-pci-non-transitional'/>
+  <flag name='virtio-scsi-pci-transitional'/>
+  <flag name='virtio-scsi-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 fc183757b0..6fea53fa6e 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
@@ -32,9 +32,12 @@  addr=0x1 \
 -device pcie-root-port,port=0xd,chassis=6,id=pci.6,bus=pcie.0,addr=0x1.0x5 \
 -device pcie-root-port,port=0xe,chassis=7,id=pci.7,bus=pcie.0,addr=0x1.0x6 \
 -device pcie-root-port,port=0xf,chassis=8,id=pci.8,bus=pcie.0,addr=0x1.0x7 \
--device pcie-root-port,port=0x10,chassis=9,id=pci.9,bus=pcie.0,addr=0x2 \
+-device pcie-root-port,port=0x10,chassis=9,id=pci.9,bus=pcie.0,\
+multifunction=on,addr=0x2 \
+-device pcie-root-port,port=0x11,chassis=10,id=pci.10,bus=pcie.0,addr=0x2.0x1 \
+-device virtio-scsi-pci,disable-legacy=on,id=scsi0,bus=pci.3,addr=0x0 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
--device virtio-blk-pci,disable-legacy=on,scsi=off,bus=pci.3,addr=0x0,\
+-device virtio-blk-pci,disable-legacy=on,scsi=off,bus=pci.4,addr=0x0,\
 drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
 -fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/fs1 \
 -device virtio-9p-pci,disable-legacy=on,id=fs0,fsdev=fsdev-fs0,mount_tag=fs1,\
@@ -43,15 +46,15 @@  bus=pci.1,addr=0x0 \
 -device virtio-net-pci,disable-legacy=on,netdev=hostnet0,id=net0,\
 mac=00:11:22:33:44:55,bus=pci.2,addr=0x0 \
 -device virtio-input-host-pci,disable-legacy=on,id=input0,\
-evdev=/dev/input/event1234,bus=pci.7,addr=0x0 \
+evdev=/dev/input/event1234,bus=pci.8,addr=0x0 \
 -device vhost-scsi-pci,disable-legacy=on,wwpn=naa.5123456789abcde0,vhostfd=3,\
-id=hostdev0,bus=pci.4,addr=0x0 \
--device virtio-balloon-pci,disable-legacy=on,id=balloon0,bus=pci.5,addr=0x0 \
+id=hostdev0,bus=pci.5,addr=0x0 \
+-device virtio-balloon-pci,disable-legacy=on,id=balloon0,bus=pci.6,addr=0x0 \
 -object rng-random,id=objrng0,filename=/dev/urandom \
--device virtio-rng-pci,disable-legacy=on,rng=objrng0,id=rng0,bus=pci.6,\
+-device virtio-rng-pci,disable-legacy=on,rng=objrng0,id=rng0,bus=pci.7,\
 addr=0x0 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
 -device vhost-vsock-pci,disable-legacy=on,id=vsock0,guest-cid=4,vhostfd=6789,\
-bus=pci.8,addr=0x0 \
+bus=pci.9,addr=0x0 \
 -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 383b29f629..ad644de35d 100644
--- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args
@@ -32,9 +32,12 @@  addr=0x1 \
 -device pcie-root-port,port=0xd,chassis=6,id=pci.6,bus=pcie.0,addr=0x1.0x5 \
 -device pcie-root-port,port=0xe,chassis=7,id=pci.7,bus=pcie.0,addr=0x1.0x6 \
 -device pcie-root-port,port=0xf,chassis=8,id=pci.8,bus=pcie.0,addr=0x1.0x7 \
--device pcie-root-port,port=0x10,chassis=9,id=pci.9,bus=pcie.0,addr=0x2 \
+-device pcie-root-port,port=0x10,chassis=9,id=pci.9,bus=pcie.0,\
+multifunction=on,addr=0x2 \
+-device pcie-root-port,port=0x11,chassis=10,id=pci.10,bus=pcie.0,addr=0x2.0x1 \
+-device virtio-scsi-pci-non-transitional,id=scsi0,bus=pci.3,addr=0x0 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
--device virtio-blk-pci-non-transitional,scsi=off,bus=pci.3,addr=0x0,\
+-device virtio-blk-pci-non-transitional,scsi=off,bus=pci.4,addr=0x0,\
 drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
 -fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/fs1 \
 -device virtio-9p-pci-non-transitional,id=fs0,fsdev=fsdev-fs0,mount_tag=fs1,\
@@ -43,14 +46,14 @@  bus=pci.1,addr=0x0 \
 -device virtio-net-pci-non-transitional,netdev=hostnet0,id=net0,\
 mac=00:11:22:33:44:55,bus=pci.2,addr=0x0 \
 -device virtio-input-host-pci-non-transitional,id=input0,\
-evdev=/dev/input/event1234,bus=pci.7,addr=0x0 \
+evdev=/dev/input/event1234,bus=pci.8,addr=0x0 \
 -device vhost-scsi-pci-non-transitional,wwpn=naa.5123456789abcde0,vhostfd=3,\
-id=hostdev0,bus=pci.4,addr=0x0 \
--device virtio-balloon-pci-non-transitional,id=balloon0,bus=pci.5,addr=0x0 \
+id=hostdev0,bus=pci.5,addr=0x0 \
+-device virtio-balloon-pci-non-transitional,id=balloon0,bus=pci.6,addr=0x0 \
 -object rng-random,id=objrng0,filename=/dev/urandom \
--device virtio-rng-pci-non-transitional,rng=objrng0,id=rng0,bus=pci.6,addr=0x0 \
+-device virtio-rng-pci-non-transitional,rng=objrng0,id=rng0,bus=pci.7,addr=0x0 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
 -device vhost-vsock-pci-non-transitional,id=vsock0,guest-cid=4,vhostfd=6789,\
-bus=pci.8,addr=0x0 \
+bus=pci.9,addr=0x0 \
 -msg timestamp=on
diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.xml b/tests/qemuxml2argvdata/virtio-non-transitional.xml
index 596aa1015b..03273dc2d9 100644
--- a/tests/qemuxml2argvdata/virtio-non-transitional.xml
+++ b/tests/qemuxml2argvdata/virtio-non-transitional.xml
@@ -29,6 +29,7 @@ 
     <input type='passthrough' bus='virtio' model='virtio-non-transitional'>
       <source evdev='/dev/input/event1234'/>
     </input>
+    <controller type='scsi' model='virtio-non-transitional'/>
     <controller type='usb' index='0' model='none'/>
     <memballoon model='virtio-non-transitional'/>
     <vsock model='virtio-non-transitional'>
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 8046e5c102..e8b3712acb 100644
--- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args
+++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args
@@ -27,8 +27,9 @@  file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 addr=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 \
+-device virtio-scsi-pci,id=scsi0,bus=pci.2,addr=0x3 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
--device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x3,drive=drive-virtio-disk0,\
+-device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x4,drive=drive-virtio-disk0,\
 id=virtio-disk0,bootindex=1 \
 -fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/fs1 \
 -device virtio-9p-pci,id=fs0,fsdev=fsdev-fs0,mount_tag=fs1,bus=pci.2,addr=0x1 \
@@ -36,13 +37,13 @@  id=virtio-disk0,bootindex=1 \
 -device virtio-net-pci,netdev=hostnet0,id=net0,mac=00:11:22:33:44:55,bus=pci.2,\
 addr=0x2 \
 -device virtio-input-host-pci,id=input0,evdev=/dev/input/event1234,bus=pci.2,\
-addr=0x7 \
+addr=0x8 \
 -device vhost-scsi-pci,wwpn=naa.5123456789abcde0,vhostfd=3,id=hostdev0,\
-bus=pci.2,addr=0x4 \
--device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x5 \
+bus=pci.2,addr=0x5 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x6 \
 -object rng-random,id=objrng0,filename=/dev/urandom \
--device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.2,addr=0x6 \
+-device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.2,addr=0x7 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
--device vhost-vsock-pci,id=vsock0,guest-cid=4,vhostfd=6789,bus=pci.2,addr=0x8 \
+-device vhost-vsock-pci,id=vsock0,guest-cid=4,vhostfd=6789,bus=pci.2,addr=0x9 \
 -msg timestamp=on
diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
index 410eb28f0a..95d20f8579 100644
--- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args
@@ -27,8 +27,9 @@  file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 addr=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 \
+-device virtio-scsi-pci-transitional,id=scsi0,bus=pci.2,addr=0x3 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \
--device virtio-blk-pci-transitional,scsi=off,bus=pci.2,addr=0x3,\
+-device virtio-blk-pci-transitional,scsi=off,bus=pci.2,addr=0x4,\
 drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
 -fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/fs1 \
 -device virtio-9p-pci-transitional,id=fs0,fsdev=fsdev-fs0,mount_tag=fs1,\
@@ -37,14 +38,14 @@  bus=pci.2,addr=0x1 \
 -device virtio-net-pci-transitional,netdev=hostnet0,id=net0,\
 mac=00:11:22:33:44:55,bus=pci.2,addr=0x2 \
 -device virtio-input-host-pci-transitional,id=input0,\
-evdev=/dev/input/event1234,bus=pci.2,addr=0x7 \
+evdev=/dev/input/event1234,bus=pci.2,addr=0x8 \
 -device vhost-scsi-pci-transitional,wwpn=naa.5123456789abcde0,vhostfd=3,\
-id=hostdev0,bus=pci.2,addr=0x4 \
--device virtio-balloon-pci-transitional,id=balloon0,bus=pci.2,addr=0x5 \
+id=hostdev0,bus=pci.2,addr=0x5 \
+-device virtio-balloon-pci-transitional,id=balloon0,bus=pci.2,addr=0x6 \
 -object rng-random,id=objrng0,filename=/dev/urandom \
--device virtio-rng-pci-transitional,rng=objrng0,id=rng0,bus=pci.2,addr=0x6 \
+-device virtio-rng-pci-transitional,rng=objrng0,id=rng0,bus=pci.2,addr=0x7 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
 -device vhost-vsock-pci-transitional,id=vsock0,guest-cid=4,vhostfd=6789,\
-bus=pci.2,addr=0x8 \
+bus=pci.2,addr=0x9 \
 -msg timestamp=on
diff --git a/tests/qemuxml2argvdata/virtio-transitional.xml b/tests/qemuxml2argvdata/virtio-transitional.xml
index 90fba68d9f..1616cb137b 100644
--- a/tests/qemuxml2argvdata/virtio-transitional.xml
+++ b/tests/qemuxml2argvdata/virtio-transitional.xml
@@ -29,6 +29,7 @@ 
     <input type='passthrough' bus='virtio' model='virtio-transitional'>
       <source evdev='/dev/input/event1234'/>
     </input>
+    <controller type='scsi' model='virtio-transitional'/>
     <controller type='usb' index='0' model='none'/>
     <memballoon model='virtio-transitional'/>
     <vsock model='virtio-transitional'>
diff --git a/tests/qemuxml2xmloutdata/virtio-non-transitional.xml b/tests/qemuxml2xmloutdata/virtio-non-transitional.xml
index b6e762c0a7..56b673d7fa 100644
--- a/tests/qemuxml2xmloutdata/virtio-non-transitional.xml
+++ b/tests/qemuxml2xmloutdata/virtio-non-transitional.xml
@@ -18,8 +18,11 @@ 
       <driver name='qemu' type='raw'/>
       <source dev='/dev/HostVG/QEMUGuest1'/>
       <target dev='vda' bus='virtio'/>
-      <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/>
     </disk>
+    <controller type='scsi' index='0' model='virtio-non-transitional'>
+      <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/>
+    </controller>
     <controller type='usb' index='0' model='none'/>
     <controller type='sata' index='0'>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
@@ -68,7 +71,12 @@ 
     <controller type='pci' index='9' model='pcie-root-port'>
       <model name='pcie-root-port'/>
       <target chassis='9' port='0x10'/>
-      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' multifunction='on'/>
+    </controller>
+    <controller type='pci' index='10' model='pcie-root-port'>
+      <model name='pcie-root-port'/>
+      <target chassis='10' port='0x11'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x1'/>
     </controller>
     <filesystem type='mount' accessmode='passthrough' model='virtio-non-transitional'>
       <source dir='/export/fs1'/>
@@ -82,24 +90,24 @@ 
     </interface>
     <input type='passthrough' bus='virtio' model='virtio-non-transitional'>
       <source evdev='/dev/input/event1234'/>
-      <address type='pci' domain='0x0000' bus='0x07' slot='0x00' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x08' slot='0x00' function='0x0'/>
     </input>
     <input type='mouse' bus='ps2'/>
     <input type='keyboard' bus='ps2'/>
     <hostdev mode='subsystem' type='scsi_host' managed='no' model='virtio-non-transitional'>
       <source protocol='vhost' wwpn='naa.5123456789abcde0'/>
-      <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x05' slot='0x00' function='0x0'/>
     </hostdev>
     <memballoon model='virtio-non-transitional'>
-      <address type='pci' domain='0x0000' bus='0x05' slot='0x00' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x06' slot='0x00' function='0x0'/>
     </memballoon>
     <rng model='virtio-non-transitional'>
       <backend model='random'>/dev/urandom</backend>
-      <address type='pci' domain='0x0000' bus='0x06' slot='0x00' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x07' slot='0x00' function='0x0'/>
     </rng>
     <vsock model='virtio-non-transitional'>
       <cid auto='no' address='4'/>
-      <address type='pci' domain='0x0000' bus='0x08' slot='0x00' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x09' slot='0x00' function='0x0'/>
     </vsock>
   </devices>
 </domain>
diff --git a/tests/qemuxml2xmloutdata/virtio-transitional.xml b/tests/qemuxml2xmloutdata/virtio-transitional.xml
index 9fa9732c2d..cf6c43bcb0 100644
--- a/tests/qemuxml2xmloutdata/virtio-transitional.xml
+++ b/tests/qemuxml2xmloutdata/virtio-transitional.xml
@@ -18,8 +18,11 @@ 
       <driver name='qemu' type='raw'/>
       <source dev='/dev/HostVG/QEMUGuest1'/>
       <target dev='vda' bus='virtio'/>
-      <address type='pci' domain='0x0000' bus='0x02' slot='0x03' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x04' function='0x0'/>
     </disk>
+    <controller type='scsi' index='0' model='virtio-transitional'>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x03' function='0x0'/>
+    </controller>
     <controller type='usb' index='0' model='none'/>
     <controller type='sata' index='0'>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
@@ -51,24 +54,24 @@ 
     </interface>
     <input type='passthrough' bus='virtio' model='virtio-transitional'>
       <source evdev='/dev/input/event1234'/>
-      <address type='pci' domain='0x0000' bus='0x02' slot='0x07' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x08' function='0x0'/>
     </input>
     <input type='mouse' bus='ps2'/>
     <input type='keyboard' bus='ps2'/>
     <hostdev mode='subsystem' type='scsi_host' managed='no' model='virtio-transitional'>
       <source protocol='vhost' wwpn='naa.5123456789abcde0'/>
-      <address type='pci' domain='0x0000' bus='0x02' slot='0x04' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x05' function='0x0'/>
     </hostdev>
     <memballoon model='virtio-transitional'>
-      <address type='pci' domain='0x0000' bus='0x02' slot='0x05' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x06' function='0x0'/>
     </memballoon>
     <rng model='virtio-transitional'>
       <backend model='random'>/dev/urandom</backend>
-      <address type='pci' domain='0x0000' bus='0x02' slot='0x06' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x07' function='0x0'/>
     </rng>
     <vsock model='virtio-transitional'>
       <cid auto='no' address='4'/>
-      <address type='pci' domain='0x0000' bus='0x02' slot='0x08' function='0x0'/>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x09' function='0x0'/>
     </vsock>
   </devices>
 </domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 9fe2b8e8a2..114ceca16c 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1271,14 +1271,16 @@  mymain(void)
             QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,
             QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,
             QEMU_CAPS_DEVICE_VHOST_VSOCK,
-            QEMU_CAPS_VIRTIO_INPUT_HOST);
+            QEMU_CAPS_VIRTIO_INPUT_HOST,
+            QEMU_CAPS_VIRTIO_SCSI);
     DO_TEST("virtio-non-transitional",
             QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
             QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE,
             QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,
             QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,
             QEMU_CAPS_DEVICE_VHOST_VSOCK,
-            QEMU_CAPS_VIRTIO_INPUT_HOST);
+            QEMU_CAPS_VIRTIO_INPUT_HOST,
+            QEMU_CAPS_VIRTIO_SCSI);
 
     if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
         virFileDeleteTree(fakerootdir);