diff mbox

[7/8] qemu: Wire up address type=pci auto_allocate

Message ID cfca80734f76937f67730948fc051e742a452b4e.1457454945.git.crobinso@redhat.com
State Superseded
Headers show

Commit Message

Cole Robinson March 8, 2016, 4:36 p.m. UTC 
We do this in 2 passes: before PCI addresses are about to be collected,
we convert type=pci auto_allocate=true to type=none auto_allocate=true,
since the existing code is already expecting type=none here.

After all PCI allocation should be complete, we do another pass of the
device addresses converting type=pci auto_allocate=true to
auto_allocate=false, so we don't trigger the unallocated address
validation check in generic domain code.
---
 src/qemu/qemu_domain_address.c                     | 47 ++++++++++++++++++++++
 .../qemuxml2argv-pci-autofill-addr.args            | 24 +++++++++++
 .../qemuxml2argv-pci-autofill-addr.xml             | 44 ++++++++++++++++++++
 tests/qemuxml2argvtest.c                           |  1 +
 .../qemuxml2xmlout-pci-autofill-addr.xml           | 46 +++++++++++++++++++++
 tests/qemuxml2xmltest.c                            |  1 +
 6 files changed, 163 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml

-- 
2.5.0

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

Comments

Cole Robinson May 15, 2016, 7:11 p.m. UTC | #1 
On 03/23/2016 02:20 PM, Laine Stump wrote:
> On 03/08/2016 11:36 AM, Cole Robinson wrote:

>> We do this in 2 passes: before PCI addresses are about to be collected,

>> we convert type=pci auto_allocate=true to type=none auto_allocate=true,

>> since the existing code is already expecting type=none here.

>>

>> After all PCI allocation should be complete, we do another pass of the

>> device addresses converting type=pci auto_allocate=true to

>> auto_allocate=false, so we don't trigger the unallocated address

>> validation check in generic domain code.

> 

> This sounds confusing. What about instead changing the existing code so that

> it checks for a valid PCI address instead of checking for type=none? A simple

> check for this is if domain == bus == slot == 0 (bus 0 is *always* either a

> pcie-root or a pci-root, and for both of those slot 0 is reserved, so once an

> address has been assigned, it will never be 0000:00:00.x .)

> 

> So rather than doing this dance with type and auto_allocate, you can just

> modify the auto allocation to say:

> 

>     if (info->type == ...NONE ||

>         (info->type == ...PCI && !virDevicePCIAddressIsValid(&info->addr.pci)) {

> 

>       // Bob Loblaw

> 

>     }

> 

> (virDevicePCIAddressIsValid() has some extra checks for values being too

> large, but does the essential check for domain/bus/slot == 0 as well)

> 


I'm refreshing these patches at the moment. I poked at this but it's a bit of
a pain... Multiple places in the address assignment code assume 'if info->type
== PCI, an address has been specified', like the code the scoops up all the
already specified PCI addresses to check for collisions. Every place that
makes that assumption now needs to be taught to check for auto_allocate and
PCIDeviceAddressIsValid which seems fragile, since other checks may slip in
that don't take those values into account.

I'm reposting the patches using the old technique, but I'm happy to change
things if we can come up with some more clear and sustainable alternative

Thanks,
Cole

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

Patch

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index eff33fc..74d13b6 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1399,6 +1399,45 @@  qemuDomainSupportsPCI(virDomainDefPtr def,
 
 
 static int
+qemuDomainPrepPCIAutoAllocate(virDomainDefPtr def ATTRIBUTE_UNUSED,
+                              virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
+                              virDomainDeviceInfoPtr info,
+                              void *opaque ATTRIBUTE_UNUSED)
+{
+    /* If PCI auto_allocate requested, set type to NONE since the rest
+       of the code expects it. */
+    if (info->auto_allocate &&
+        info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+        info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
+
+    return 0;
+}
+
+
+static int
+qemuDomainFinishPCIAutoAllocate(virDomainDefPtr def ATTRIBUTE_UNUSED,
+                                virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
+                                virDomainDeviceInfoPtr info,
+                                void *opaque ATTRIBUTE_UNUSED)
+{
+    /* A PCI device was allocated as requested, unset auto_allocate so
+       we don't trip the domain error about unallocated addresses */
+    if (info->auto_allocate &&
+        info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+        info->auto_allocate = false;
+
+    /* We wanted to allocate a PCI address but it was never filled in...
+       this is likely an XML error. Re-set type=PCI to give a correct
+       error from domain conf */
+    if (info->auto_allocate &&
+        info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
+        info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
+
+    return 0;
+}
+
+
+static int
 qemuDomainAssignPCIAddresses(virDomainDefPtr def,
                              virQEMUCapsPtr qemuCaps,
                              virDomainObjPtr obj)
@@ -1423,6 +1462,10 @@  qemuDomainAssignPCIAddresses(virDomainDefPtr def,
             }
         }
 
+        if (virDomainDeviceInfoIterate(def, qemuDomainPrepPCIAutoAllocate,
+                                       NULL) < 0)
+            goto cleanup;
+
         nbuses = max_idx + 1;
 
         if (nbuses > 0 &&
@@ -1553,6 +1596,10 @@  qemuDomainAssignPCIAddresses(virDomainDefPtr def,
                 }
             }
         }
+
+        if (virDomainDeviceInfoIterate(def, qemuDomainFinishPCIAutoAllocate,
+                                       NULL) < 0)
+            goto cleanup;
     }
 
     if (obj && obj->privateData) {
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args
new file mode 100644
index 0000000..2ab305e
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.args
@@ -0,0 +1,24 @@ 
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/libexec/qemu-kvm \
+-name fdr-br \
+-S \
+-M pc-1.2 \
+-m 2048 \
+-smp 2 \
+-uuid 3ec6cbe1-b5a2-4515-b800-31a61855df41 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-fdr-br/monitor.sock,server,nowait \
+-boot c \
+-usb \
+-drive file=/var/iso/f18kde.iso,format=raw,if=none,media=cdrom,\
+id=drive-virtio-disk0 \
+-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\
+id=virtio-disk0 \
+-vga cirrus \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml
new file mode 100644
index 0000000..8e7b6ab
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-autofill-addr.xml
@@ -0,0 +1,44 @@ 
+<domain type='qemu'>
+  <name>fdr-br</name>
+  <uuid>3ec6cbe1-b5a2-4515-b800-31a61855df41</uuid>
+  <memory unit='KiB'>2097152</memory>
+  <currentMemory unit='KiB'>2097152</currentMemory>
+  <vcpu placement='static' cpuset='0-1'>2</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc-1.2'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <pae/>
+  </features>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>restart</on_crash>
+  <devices>
+    <emulator>/usr/libexec/qemu-kvm</emulator>
+    <disk type='file' device='cdrom'>
+      <driver name='qemu' type='raw'/>
+      <source file='/var/iso/f18kde.iso'/>
+      <target dev='vda' bus='virtio'/>
+      <readonly/>
+      <address type='pci'/>
+    </disk>
+    <controller type='usb' index='0'>
+      <address type='pci'/>
+    </controller>
+    <controller type='ide' index='0'>
+      <address type='pci'/>
+    </controller>
+    <input type='mouse' bus='ps2'/>
+    <video>
+      <model type='cirrus' vram='16384' heads='1'/>
+      <address type='pci'/>
+    </video>
+    <memballoon model='virtio'>
+      <address type='pci'/>
+    </memballoon>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index aaec9de..79339c8 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1524,6 +1524,7 @@  mymain(void)
 
     DO_TEST("pci-autoadd-addr", QEMU_CAPS_DEVICE_PCI_BRIDGE);
     DO_TEST("pci-autoadd-idx", QEMU_CAPS_DEVICE_PCI_BRIDGE);
+    DO_TEST("pci-autofill-addr", NONE);
     DO_TEST("pci-many",
             QEMU_CAPS_DEVICE_PCI_BRIDGE);
     DO_TEST("pci-bridge-many-disks",
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml
new file mode 100644
index 0000000..0f32c23
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-autofill-addr.xml
@@ -0,0 +1,46 @@ 
+<domain type='qemu'>
+  <name>fdr-br</name>
+  <uuid>3ec6cbe1-b5a2-4515-b800-31a61855df41</uuid>
+  <memory unit='KiB'>2097152</memory>
+  <currentMemory unit='KiB'>2097152</currentMemory>
+  <vcpu placement='static' cpuset='0-1'>2</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc-1.2'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <pae/>
+  </features>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>restart</on_crash>
+  <devices>
+    <emulator>/usr/libexec/qemu-kvm</emulator>
+    <disk type='file' device='cdrom'>
+      <driver name='qemu' type='raw'/>
+      <source file='/var/iso/f18kde.iso'/>
+      <target dev='vda' bus='virtio'/>
+      <readonly/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+    </disk>
+    <controller type='usb' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+    </controller>
+    <controller type='ide' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'/>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <video>
+      <model type='cirrus' vram='16384' heads='1' primary='yes'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+    </video>
+    <memballoon model='virtio'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+    </memballoon>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index b3568df..d43ca13 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -580,6 +580,7 @@  mymain(void)
                  QEMU_CAPS_DEVICE_PCI_BRIDGE);
     DO_TEST_FULL("pci-autoadd-idx", WHEN_ACTIVE,
                  QEMU_CAPS_DEVICE_PCI_BRIDGE);
+    DO_TEST("pci-autofill-addr");
 
     DO_TEST_FULL("q35", WHEN_ACTIVE,
                  QEMU_CAPS_DEVICE_PCI_BRIDGE,