Like it usually happens, I fixed one thing and broke another:
in 803966c76 address allocation was fixed for SATA disks, but
broke that for virtio disks, because it dropped disk address
assignment completely. It's not needed for SATA disks anymore,
but still needed for the virtio ones.

Bring that back and add a couple of tests to make sure it won't
happen again.
---
 src/bhyve/bhyve_device.c                           | 16 +++++++++
 .../bhyvexml2argv-addr-multiple-virtio-disks.args  | 11 ++++++
 ...bhyvexml2argv-addr-multiple-virtio-disks.ldargs |  3 ++
 .../bhyvexml2argv-addr-multiple-virtio-disks.xml   | 32 +++++++++++++++++
 .../bhyvexml2argv-addr-single-virtio-disk.args     |  9 +++++
 .../bhyvexml2argv-addr-single-virtio-disk.ldargs   |  3 ++
 .../bhyvexml2argv-addr-single-virtio-disk.xml      | 22 ++++++++++++
 tests/bhyvexml2argvtest.c                          |  2 ++
 .../bhyvexml2xmlout-addr-multiple-virtio-disks.xml | 42 ++++++++++++++++++++++
 .../bhyvexml2xmlout-addr-single-virtio-disk.xml    | 30 ++++++++++++++++
 tests/bhyvexml2xmltest.c                           |  2 ++
 11 files changed, 172 insertions(+)
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.args
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.ldargs
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.xml
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.args
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.ldargs
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.xml
 create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-multiple-virtio-disks.xml
 create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-single-virtio-disk.xml

diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c
index 29528230f..55ce631ec 100644
--- a/src/bhyve/bhyve_device.c
+++ b/src/bhyve/bhyve_device.c
@@ -129,6 +129,22 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def,
             goto error;
     }
 
+    for (i = 0; i < def->ndisks; i++) {
+        /* We only handle virtio disk addresses as SATA disks are
+         * attached to a controller and don't have their own PCI
+         * addresses */
+        if (def->disks[i]->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
+            continue;
+
+        if (def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+            !virPCIDeviceAddressIsEmpty(&def->disks[i]->info.addr.pci))
+            continue;
+        if (virDomainPCIAddressReserveNextAddr(addrs, &def->disks[i]->info,
+                                               VIR_PCI_CONNECT_TYPE_PCI_DEVICE,
+                                               -1) < 0)
+            goto error;
+    }
+
     return 0;
 
  error:
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.args b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.args
new file mode 100644
index 000000000..8cc166894
--- /dev/null
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.args
@@ -0,0 +1,11 @@
+/usr/sbin/bhyve \
+-c 1 \
+-m 214 \
+-u \
+-H \
+-P \
+-s 0:0,hostbridge \
+-s 3:0,virtio-net,faketapdev,mac=52:54:00:bc:85:fe \
+-s 2:0,virtio-blk,/tmp/freebsd.img \
+-s 4:0,virtio-blk,/tmp/test.img \
+-s 5:0,virtio-blk,/tmp/test2.img bhyve
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.ldargs
new file mode 100644
index 000000000..32538b558
--- /dev/null
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.ldargs
@@ -0,0 +1,3 @@
+/usr/sbin/bhyveload \
+-m 214 \
+-d /tmp/freebsd.img bhyve
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.xml b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.xml
new file mode 100644
index 000000000..9bcd0a629
--- /dev/null
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-multiple-virtio-disks.xml
@@ -0,0 +1,32 @@
+<domain type='bhyve'>
+  <name>bhyve</name>
+  <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid>
+  <memory>219136</memory>
+  <vcpu>1</vcpu>
+  <os>
+    <type>hvm</type>
+  </os>
+  <devices>
+    <disk type='file' device='disk'>
+      <driver name='file' type='raw'/>
+      <source file='/tmp/freebsd.img'/>
+      <target dev='vda' bus='virtio'/>
+    </disk>
+    <disk type='file' device='disk'>
+      <driver name='file' type='raw'/>
+      <source file='/tmp/test.img'/>
+      <target dev='vdb' bus='virtio'/>
+    </disk>
+    <disk type='file' device='disk'>
+      <driver name='file' type='raw'/>
+      <source file='/tmp/test2.img'/>
+      <target dev='vdc' bus='virtio'/>
+    </disk>
+    <interface type='bridge'>
+      <mac address='52:54:00:bc:85:fe'/>
+      <model type='virtio'/>
+      <source bridge="virbr0"/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+    </interface>
+  </devices>
+</domain>
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.args b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.args
new file mode 100644
index 000000000..4dcc40404
--- /dev/null
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.args
@@ -0,0 +1,9 @@
+/usr/sbin/bhyve \
+-c 1 \
+-m 214 \
+-u \
+-H \
+-P \
+-s 0:0,hostbridge \
+-s 3:0,virtio-net,faketapdev,mac=52:54:00:bc:85:fe \
+-s 2:0,virtio-blk,/tmp/freebsd.img bhyve
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.ldargs
new file mode 100644
index 000000000..32538b558
--- /dev/null
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.ldargs
@@ -0,0 +1,3 @@
+/usr/sbin/bhyveload \
+-m 214 \
+-d /tmp/freebsd.img bhyve
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.xml b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.xml
new file mode 100644
index 000000000..6be9ae134
--- /dev/null
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-addr-single-virtio-disk.xml
@@ -0,0 +1,22 @@
+<domain type='bhyve'>
+  <name>bhyve</name>
+  <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid>
+  <memory>219136</memory>
+  <vcpu>1</vcpu>
+  <os>
+    <type>hvm</type>
+  </os>
+  <devices>
+    <disk type='file'>
+      <driver name='file' type='raw'/>
+      <source file='/tmp/freebsd.img'/>
+      <target dev='vda' bus='virtio'/>
+    </disk>
+    <interface type='bridge'>
+      <mac address='52:54:00:bc:85:fe'/>
+      <model type='virtio'/>
+      <source bridge="virbr0"/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+    </interface>
+  </devices>
+</domain>
diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c
index e80705780..c36b55a0a 100644
--- a/tests/bhyvexml2argvtest.c
+++ b/tests/bhyvexml2argvtest.c
@@ -190,6 +190,8 @@ mymain(void)
     DO_TEST("addr-single-sata-disk");
     DO_TEST("addr-multiple-sata-disks");
     DO_TEST("addr-more-than-32-sata-disks");
+    DO_TEST("addr-single-virtio-disk");
+    DO_TEST("addr-multiple-virtio-disks");
 
     /* The same without 32 devs per controller support */
     driver.bhyvecaps ^= BHYVE_CAP_AHCI32SLOT;
diff --git a/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-multiple-virtio-disks.xml b/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-multiple-virtio-disks.xml
new file mode 100644
index 000000000..542bff121
--- /dev/null
+++ b/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-multiple-virtio-disks.xml
@@ -0,0 +1,42 @@
+<domain type='bhyve'>
+  <name>bhyve</name>
+  <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid>
+  <memory unit='KiB'>219136</memory>
+  <currentMemory unit='KiB'>219136</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <disk type='file' device='disk'>
+      <driver name='file' type='raw'/>
+      <source file='/tmp/freebsd.img'/>
+      <target dev='vda' bus='virtio'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+    </disk>
+    <disk type='file' device='disk'>
+      <driver name='file' type='raw'/>
+      <source file='/tmp/test.img'/>
+      <target dev='vdb' bus='virtio'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+    </disk>
+    <disk type='file' device='disk'>
+      <driver name='file' type='raw'/>
+      <source file='/tmp/test2.img'/>
+      <target dev='vdc' bus='virtio'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>
+    </disk>
+    <controller type='pci' index='0' model='pci-root'/>
+    <interface type='bridge'>
+      <mac address='52:54:00:bc:85:fe'/>
+      <source bridge='virbr0'/>
+      <model type='virtio'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+    </interface>
+  </devices>
+</domain>
diff --git a/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-single-virtio-disk.xml b/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-single-virtio-disk.xml
new file mode 100644
index 000000000..d7abb5abc
--- /dev/null
+++ b/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-single-virtio-disk.xml
@@ -0,0 +1,30 @@
+<domain type='bhyve'>
+  <name>bhyve</name>
+  <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid>
+  <memory unit='KiB'>219136</memory>
+  <currentMemory unit='KiB'>219136</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <disk type='file' device='disk'>
+      <driver name='file' type='raw'/>
+      <source file='/tmp/freebsd.img'/>
+      <target dev='vda' bus='virtio'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+    </disk>
+    <controller type='pci' index='0' model='pci-root'/>
+    <interface type='bridge'>
+      <mac address='52:54:00:bc:85:fe'/>
+      <source bridge='virbr0'/>
+      <model type='virtio'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+    </interface>
+  </devices>
+</domain>
diff --git a/tests/bhyvexml2xmltest.c b/tests/bhyvexml2xmltest.c
index 004afda14..ba9af2996 100644
--- a/tests/bhyvexml2xmltest.c
+++ b/tests/bhyvexml2xmltest.c
@@ -109,6 +109,8 @@ mymain(void)
     DO_TEST_DIFFERENT("addr-single-sata-disk");
     DO_TEST_DIFFERENT("addr-multiple-sata-disks");
     DO_TEST_DIFFERENT("addr-more-than-32-sata-disks");
+    DO_TEST_DIFFERENT("addr-single-virtio-disk");
+    DO_TEST_DIFFERENT("addr-multiple-virtio-disks");
 
     /* The same without 32 devs per controller support */
     driver.bhyvecaps ^= BHYVE_CAP_AHCI32SLOT;
-- 
2.11.0

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

On Wed, 2017-02-01 at 20:25 +0400, Roman Bogorodskiy wrote:
> Like it usually happens, I fixed one thing and broke another:
> in 803966c76 address allocation was fixed for SATA disks, but
> broke that for virtio disks, because it dropped disk address
> assignment completely. It's not needed for SATA disks anymore,
> but still needed for the virtio ones.
> 
> Bring that back and add a couple of tests to make sure it won't
> happen again.

I didn't actually test this[1], but both the code and the
tests look reasonable enough, plus 'make check' and 'make
syntax-check' are all green[2] on FreeBSD, so:

ACK


[1] Can you run bhyve guests inside a FreeBSD KVM guest?
[2] Well, mostly. We really should fix qemuxml2argvtest once
    and for all, and it looks like we're sooo close now!
-- 
Andrea Bolognani / Red Hat / Virtualization

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

  Andrea Bolognani wrote:

> On Wed, 2017-02-01 at 20:25 +0400, Roman Bogorodskiy wrote:
> > Like it usually happens, I fixed one thing and broke another:
> > in 803966c76 address allocation was fixed for SATA disks, but
> > broke that for virtio disks, because it dropped disk address
> > assignment completely. It's not needed for SATA disks anymore,
> > but still needed for the virtio ones.
> > 
> > Bring that back and add a couple of tests to make sure it won't
> > happen again.
> 
> I didn't actually test this[1], but both the code and the
> tests look reasonable enough, plus 'make check' and 'make
> syntax-check' are all green[2] on FreeBSD, so:
> 
> ACK

Pushed, thanks!

> [1] Can you run bhyve guests inside a FreeBSD KVM guest?

I guess that generally you cannot because it requires hardware
virtualization support (i.e. POPCNT CPU feature and maybe something
else). Probably it's possible with KVM's nested virtualization, I've
been wanting to check that for a while but somehow always forget about
that when I have time.

> [2] Well, mostly. We really should fix qemuxml2argvtest once
>     and for all, and it looks like we're sooo close now!

True.

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