[libvirt] [PATCH v2 00/15] qemu: Validate PCI controller options
Changes from [v1]: * error out instead of silently accept invalid options; * shave quite a lot of yaks. [v1] https://www.redhat.com/archives/libvir-list/2018-February/msg00244.html Andrea Bolognani (15): tests: Add some tests for PCI controller options qemu: Move 'done' label in qemuBuildControllerDevStr() qemu: Move skip for implicit PHB of pSeries guests qemu: Restrict scope in qemuBuildControllerDevStr() qemu: Simplify modelName stringification qemu: Defer capability check to command line generation time qemu: Clear out qemuDomainDeviceDefValidateControllerPCI() qemu: Validate PCI controllers (index) qemu: Validate PCI controllers (modelName) qemu: Validate PCI controllers (targetIndex) qemu: Validate PCI controllers (pcihole64) qemu: Validate PCI controllers (busNr) qemu: Validate PCI controllers (numaNode) qemu: Validate PCI controllers (chassisNr) qemu: Validate PCI controllers (chassis and port) src/qemu/qemu_command.c | 80 +++- src/qemu/qemu_domain.c | 425 +++++++++++++-------- .../i440fx-controllers-pciopts.xml | 36 ++ tests/qemuxml2argvdata/pcie-expander-bus.xml | 3 - .../pseries-controllers-pciopts.xml | 35 ++ tests/qemuxml2argvdata/q35-controllers-pciopts.xml | 60 +++ tests/qemuxml2argvtest.c | 4 + tests/qemuxml2xmloutdata/pcie-expander-bus.xml | 4 +- 8 files changed, 475 insertions(+), 172 deletions(-) create mode 100644 tests/qemuxml2argvdata/i440fx-controllers-pciopts.xml create mode 100644 tests/qemuxml2argvdata/pseries-controllers-pciopts.xml create mode 100644 tests/qemuxml2argvdata/q35-controllers-pciopts.xml -- 2.14.3
The input configurations set all existing options for all PCI controllers, to see what ends up showing up in the output. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../i440fx-controllers-pciopts.args | 24 +++++++ .../i440fx-controllers-pciopts.xml | 36 ++++++++++ .../pseries-controllers-pciopts.args | 22 +++++++ .../pseries-controllers-pciopts.xml | 35 ++++++++++ .../qemuxml2argvdata/q35-controllers-pciopts.args | 28 ++++++++ tests/qemuxml2argvdata/q35-controllers-pciopts.xml | 60 +++++++++++++++++ tests/qemuxml2argvtest.c | 17 +++++ .../i440fx-controllers-pciopts.xml | 45 +++++++++++++ .../pseries-controllers-pciopts.xml | 43 ++++++++++++ .../qemuxml2xmloutdata/q35-controllers-pciopts.xml | 76 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 17 +++++ 11 files changed, 403 insertions(+) create mode 100644 tests/qemuxml2argvdata/i440fx-controllers-pciopts.args create mode 100644 tests/qemuxml2argvdata/i440fx-controllers-pciopts.xml create mode 100644 tests/qemuxml2argvdata/pseries-controllers-pciopts.args create mode 100644 tests/qemuxml2argvdata/pseries-controllers-pciopts.xml create mode 100644 tests/qemuxml2argvdata/q35-controllers-pciopts.args create mode 100644 tests/qemuxml2argvdata/q35-controllers-pciopts.xml create mode 100644 tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml create mode 100644 tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml create mode 100644 tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml diff --git a/tests/qemuxml2argvdata/i440fx-controllers-pciopts.args b/tests/qemuxml2argvdata/i440fx-controllers-pciopts.args new file mode 100644 index 000000000..d85fae5c9 --- /dev/null +++ b/tests/qemuxml2argvdata/i440fx-controllers-pciopts.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest \ +-S \ +-M pc \ +-m 1024 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-numa node,nodeid=0,cpus=0,mem=1024 \ +-uuid 496d7ea8-9739-544b-4ebd-ef08be936e8b \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-global i440FX-pcihost.pci-hole64-size=1024K \ +-device pci-bridge,chassis_nr=2,id=pci.1,bus=pci.0,addr=0x3 \ +-device pxb,bus_nr=3,id=pci.2,numa_node=0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/i440fx-controllers-pciopts.xml b/tests/qemuxml2argvdata/i440fx-controllers-pciopts.xml new file mode 100644 index 000000000..861a66589 --- /dev/null +++ b/tests/qemuxml2argvdata/i440fx-controllers-pciopts.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + </os> + <cpu> + <numa> + <cell id='0' cpus='0' memory='1048576' unit='KiB'/> + </numa> + </cpu> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pci-root'> + <!-- Only pcihole64 should be preserved --> + <target busNr='1' chassis='1' chassisNr='1' index='0' port='1'/> + <pcihole64 unit='KiB'>1024</pcihole64> + </controller> + <controller type='pci' index='1' model='pci-bridge'> + <!-- Only chassisNr should be preserved --> + <target busNr='2' chassis='2' chassisNr='2' index='2' port='2'> + <node>0</node> + </target> + </controller> + <controller type='pci' index='2' model='pci-expander-bus'> + <!-- Only busNr and numaNode should be preserved --> + <target busNr='3' chassis='3' chassisNr='3' index='3' port='3'> + <node>0</node> + </target> + </controller> + <controller type='usb' index='0' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/pseries-controllers-pciopts.args b/tests/qemuxml2argvdata/pseries-controllers-pciopts.args new file mode 100644 index 000000000..5f1edfc83 --- /dev/null +++ b/tests/qemuxml2argvdata/pseries-controllers-pciopts.args @@ -0,0 +1,22 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 \ +-name guest \ +-S \ +-M pseries \ +-m 1024 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-numa node,nodeid=0,cpus=0,mem=1024 \ +-uuid 496d7ea8-9739-544b-4ebd-ef08be936e8b \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-boot c \ +-device spapr-pci-host-bridge,index=1,id=pci.1,numa_node=0 \ +-device pci-bridge,chassis_nr=3,id=pci.2,bus=pci.0,addr=0x1 diff --git a/tests/qemuxml2argvdata/pseries-controllers-pciopts.xml b/tests/qemuxml2argvdata/pseries-controllers-pciopts.xml new file mode 100644 index 000000000..43353cba6 --- /dev/null +++ b/tests/qemuxml2argvdata/pseries-controllers-pciopts.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + </os> + <cpu> + <numa> + <cell id='0' cpus='0' memory='1048576' unit='KiB'/> + </numa> + </cpu> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='pci' index='0' model='pci-root'> + <!-- Only targetIndex should be preserved --> + <target busNr='1' chassis='1' chassisNr='1' index='0' port='1'/> + </controller> + <controller type='pci' index='1' model='pci-root'> + <!-- Only numaNode and targetIndex should be preserved --> + <target busNr='2' chassis='2' chassisNr='2' index='1' port='2'> + <node>0</node> + </target> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <!-- Only chassisNr should be preserved --> + <target busNr='3' chassis='3' chassisNr='3' index='3' port='3'> + <node>0</node> + </target> + </controller> + <controller type='usb' index='0' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/q35-controllers-pciopts.args b/tests/qemuxml2argvdata/q35-controllers-pciopts.args new file mode 100644 index 000000000..44259f502 --- /dev/null +++ b/tests/qemuxml2argvdata/q35-controllers-pciopts.args @@ -0,0 +1,28 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest \ +-S \ +-M q35 \ +-m 1024 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-numa node,nodeid=0,cpus=0,mem=1024 \ +-uuid 496d7ea8-9739-544b-4ebd-ef08be936e8b \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-global q35-pcihost.pci-hole64-size=1024K \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=3,id=pci.2,bus=pci.1,addr=0x0 \ +-device pxb-pcie,bus_nr=4,id=pci.3,numa_node=0,bus=pcie.0,addr=0x2 \ +-device pcie-root-port,port=0x5,chassis=5,id=pci.4,bus=pcie.0,addr=0x3 \ +-device x3130-upstream,id=pci.5,bus=pci.4,addr=0x0 \ +-device xio3130-downstream,port=0x7,chassis=7,id=pci.6,bus=pci.5,addr=0x0 diff --git a/tests/qemuxml2argvdata/q35-controllers-pciopts.xml b/tests/qemuxml2argvdata/q35-controllers-pciopts.xml new file mode 100644 index 000000000..40db83615 --- /dev/null +++ b/tests/qemuxml2argvdata/q35-controllers-pciopts.xml @@ -0,0 +1,60 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + </os> + <cpu> + <numa> + <cell id='0' cpus='0' memory='1048576' unit='KiB'/> + </numa> + </cpu> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pcie-root'> + <!-- Only pcihole64 should be preserved --> + <target busNr='1' chassis='1' chassisNr='1' index='0' port='1'/> + <pcihole64 unit='KiB'>1024</pcihole64> + </controller> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <!-- No option should be preserved --> + <target busNr='2' chassis='2' chassisNr='2' index='2' port='2'> + <node>0</node> + </target> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <!-- Only chassisNr should be preserved --> + <target busNr='3' chassis='3' chassisNr='3' index='3' port='3'> + <node>0</node> + </target> + </controller> + <controller type='pci' index='3' model='pcie-expander-bus'> + <!-- Only busNr and numaNode should be preserved --> + <target busNr='4' chassis='4' chassisNr='4' index='4' port='4'> + <node>0</node> + </target> + </controller> + <controller type='pci' index='4' model='pcie-root-port'> + <!-- Only chassis and port should be preserved --> + <target busNr='5' chassis='5' chassisNr='5' index='5' port='5'> + <node>0</node> + </target> + </controller> + <controller type='pci' index='5' model='pcie-switch-upstream-port'> + <!-- No option should be preserved --> + <target busNr='6' chassis='6' chassisNr='6' index='6' port='6'> + <node>0</node> + </target> + </controller> + <controller type='pci' index='6' model='pcie-switch-downstream-port'> + <!-- Only chassis and port should be preserved --> + <target busNr='7' chassis='7' chassisNr='7' index='7' port='7'> + <node>0</node> + </target> + </controller> + <controller type='usb' index='0' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ade18c055..4154663da 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2435,6 +2435,23 @@ mymain(void) QEMU_CAPS_DEVICE_IOH3420, QEMU_CAPS_DEVICE_PXB_PCIE); + DO_TEST("i440fx-controllers-pciopts", + QEMU_CAPS_I440FX_PCI_HOLE64_SIZE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PXB); + DO_TEST("q35-controllers-pciopts", + QEMU_CAPS_Q35_PCI_HOLE64_SIZE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PXB_PCIE, + QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, + QEMU_CAPS_DEVICE_X3130_UPSTREAM, + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM); + DO_TEST("pseries-controllers-pciopts", + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE, + QEMU_CAPS_DEVICE_PCI_BRIDGE); + DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_DEVICE_SCSI_GENERIC); diff --git a/tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml b/tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml new file mode 100644 index 000000000..d171d1370 --- /dev/null +++ b/tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml @@ -0,0 +1,45 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <numa> + <cell id='0' cpus='0' memory='1048576' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pci-root'> + <target chassisNr='1' chassis='1' port='0x1' busNr='1' index='0'/> + <pcihole64 unit='KiB'>1024</pcihole64> + </controller> + <controller type='pci' index='1' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2' chassis='2' port='0x2' busNr='2' index='2'> + <node>0</node> + </target> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-expander-bus'> + <model name='pxb'/> + <target chassisNr='3' chassis='3' port='0x3' busNr='3' index='3'> + <node>0</node> + </target> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <controller type='usb' index='0' model='none'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml b/tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml new file mode 100644 index 000000000..bbe360e25 --- /dev/null +++ b/tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml @@ -0,0 +1,43 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <numa> + <cell id='0' cpus='0' memory='1048576' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target chassisNr='1' chassis='1' port='0x1' busNr='1' index='0'/> + </controller> + <controller type='pci' index='1' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target chassisNr='2' chassis='2' port='0x2' busNr='2' index='1'> + <node>0</node> + </target> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='3' chassis='3' port='0x3' busNr='3' index='3'> + <node>0</node> + </target> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='usb' index='0' model='none'/> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml b/tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml new file mode 100644 index 000000000..5ef7aa564 --- /dev/null +++ b/tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml @@ -0,0 +1,76 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <numa> + <cell id='0' cpus='0' memory='1048576' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pcie-root'> + <target chassisNr='1' chassis='1' port='0x1' busNr='1' index='0'/> + <pcihole64 unit='KiB'>1024</pcihole64> + </controller> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <target chassisNr='2' chassis='2' port='0x2' busNr='2' index='2'> + <node>0</node> + </target> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='3' chassis='3' port='0x3' busNr='3' index='3'> + <node>0</node> + </target> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-expander-bus'> + <model name='pxb-pcie'/> + <target chassisNr='4' chassis='4' port='0x4' busNr='4' index='4'> + <node>0</node> + </target> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </controller> + <controller type='pci' index='4' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassisNr='5' chassis='5' port='0x5' busNr='5' index='5'> + <node>0</node> + </target> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='5' model='pcie-switch-upstream-port'> + <model name='x3130-upstream'/> + <target chassisNr='6' chassis='6' port='0x6' busNr='6' index='6'> + <node>0</node> + </target> + <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='6' model='pcie-switch-downstream-port'> + <model name='xio3130-downstream'/> + <target chassisNr='7' chassis='7' port='0x7' busNr='7' index='7'> + <node>0</node> + </target> + <address type='pci' domain='0x0000' bus='0x05' 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'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0eb9e6c77..7b8a16078 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1102,6 +1102,23 @@ mymain(void) QEMU_CAPS_DEVICE_IOH3420, QEMU_CAPS_HDA_DUPLEX); + DO_TEST("i440fx-controllers-pciopts", + QEMU_CAPS_I440FX_PCI_HOLE64_SIZE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PXB); + DO_TEST("q35-controllers-pciopts", + QEMU_CAPS_Q35_PCI_HOLE64_SIZE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PXB_PCIE, + QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, + QEMU_CAPS_DEVICE_X3130_UPSTREAM, + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM); + DO_TEST("pseries-controllers-pciopts", + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE, + QEMU_CAPS_DEVICE_PCI_BRIDGE); + DO_TEST("hostdev-scsi-vhost-scsi-ccw", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VHOST_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC, QEMU_CAPS_VIRTIO_CCW); -- 2.14.3
Even when we skip part of the processing, we still want error checking on the buffer. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 55e573b61..1ab5b0818 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2801,10 +2801,10 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, if (qemuBuildDeviceAddressStr(&buf, domainDef, &def->info, qemuCaps) < 0) goto error; + done: if (virBufferCheckError(&buf) < 0) goto error; - done: *devstr = virBufferContentAndReset(&buf); return 0; -- 2.14.3
On Fri, Feb 16, 2018 at 17:27:59 +0100, Andrea Bolognani wrote:
Even when we skip part of the processing, we still want error checking on the buffer.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
Performing the skip earlier will help us making the function nicer later on. We also make the condition for the skip a bit more precise, though that'a more for self-documenting purposes and doesn't change anything in practice. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1ab5b0818..5e4dfcf75 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2732,6 +2732,14 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + /* Skip the implicit PHB for pSeries guests */ + if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && + pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE && + pciopts->targetIndex == 0 && + qemuDomainIsPSeries(domainDef)) { + goto done; + } + switch ((virDomainControllerModelPCI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", @@ -2759,10 +2767,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->chassis, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - /* Skip the implicit one */ - if (pciopts->targetIndex == 0) - goto done; - virBufferAsprintf(&buf, "%s,index=%d,id=%s", modelName, pciopts->targetIndex, def->info.alias); -- 2.14.3
On Fri, Feb 16, 2018 at 17:28:00 +0100, Andrea Bolognani wrote:
Performing the skip earlier will help us making the function nicer later on. We also make the condition for the skip a bit more precise, though that'a more for self-documenting purposes and doesn't change anything in practice.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1ab5b0818..5e4dfcf75 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2732,6 +2732,14 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
+ /* Skip the implicit PHB for pSeries guests */ + if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && + pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE && + pciopts->targetIndex == 0 && + qemuDomainIsPSeries(domainDef)) {
I'm not sure about the last line. Shouldn't that alredy be validated? At least in the case when the PHB model should not be present on non-pseries qemus? ACK if you agree and drop the last line.
On Mon, 2018-02-19 at 15:58 +0100, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 17:28:00 +0100, Andrea Bolognani wrote:
Performing the skip earlier will help us making the function nicer later on. We also make the condition for the skip a bit more precise, though that'a more for self-documenting purposes and doesn't change anything in practice.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1ab5b0818..5e4dfcf75 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2732,6 +2732,14 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
+ /* Skip the implicit PHB for pSeries guests */ + if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && + pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE && + pciopts->targetIndex == 0 && + qemuDomainIsPSeries(domainDef)) {
I'm not sure about the last line. Shouldn't that alredy be validated? At least in the case when the PHB model should not be present on non-pseries qemus?
ACK if you agree and drop the last line.
Strictly speaking, both the check on modelName and that on the machine type are redundant, because we make sure targetIndex is only set for PHBs and thus only for pSeries guests. That said, checking again doesn't hurt and makes the reason for skipping more obvious. It might also help catch bugs introduced in the validate callback that would result in a controller that was supposed to be skipped showing up in the output for some test case. -- Andrea Bolognani / Red Hat / Virtualization
Some variables are only used for PCI controllers, and we're going to add more soon. We can declare them in the 'case' scope rather than in the function scope to make it a bit nicer. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5e4dfcf75..2291bf5da 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2625,8 +2625,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, { virBuffer buf = VIR_BUFFER_INITIALIZER; int address_type = def->info.type; - const virDomainPCIControllerOpts *pciopts; - const char *modelName = NULL; *devstr = NULL; @@ -2726,7 +2724,10 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; - case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: { + const virDomainPCIControllerOpts *pciopts; + const char *modelName = NULL; + pciopts = &def->opts.pciopts; if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT && def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) @@ -2781,6 +2782,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto error; } break; + } case VIR_DOMAIN_CONTROLLER_TYPE_IDE: case VIR_DOMAIN_CONTROLLER_TYPE_FDC: -- 2.14.3
On Fri, Feb 16, 2018 at 17:28:01 +0100, Andrea Bolognani wrote:
Some variables are only used for PCI controllers, and we're going to add more soon. We can declare them in the 'case' scope rather than in the function scope to make it a bit nicer.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5e4dfcf75..2291bf5da 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2625,8 +2625,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, { virBuffer buf = VIR_BUFFER_INITIALIZER; int address_type = def->info.type; - const virDomainPCIControllerOpts *pciopts; - const char *modelName = NULL;
*devstr = NULL;
Since the next patch changes both lines again I suggest you squash it into the next patch.
On Mon, 2018-02-19 at 16:00 +0100, Peter Krempa wrote:
@@ -2625,8 +2625,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, { virBuffer buf = VIR_BUFFER_INITIALIZER; int address_type = def->info.type; - const virDomainPCIControllerOpts *pciopts; - const char *modelName = NULL;
*devstr = NULL;
Since the next patch changes both lines again I suggest you squash it into the next patch.
They're semantically independent changes so I thought it would be better to perform them separately, but I can squash them together if that's your preference. -- Andrea Bolognani / Red Hat / Virtualization
There's no need to perform checks before conversion, we can just go try and check the results later on. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2291bf5da..a44a1b2d2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2725,13 +2725,8 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; case VIR_DOMAIN_CONTROLLER_TYPE_PCI: { - const virDomainPCIControllerOpts *pciopts; - const char *modelName = NULL; - - pciopts = &def->opts.pciopts; - if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT && - def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + const virDomainPCIControllerOpts *pciopts = &def->opts.pciopts; + const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); /* Skip the implicit PHB for pSeries guests */ if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && @@ -2741,6 +2736,13 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto done; } + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown virDomainControllerPCIModelName value: %d"), + pciopts->modelName); + return -1; + } + switch ((virDomainControllerModelPCI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", -- 2.14.3
On Fri, Feb 16, 2018 at 17:28:02 +0100, Andrea Bolognani wrote:
There's no need to perform checks before conversion, we can just go try and check the results later on.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2291bf5da..a44a1b2d2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2725,13 +2725,8 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break;
case VIR_DOMAIN_CONTROLLER_TYPE_PCI: { - const virDomainPCIControllerOpts *pciopts; - const char *modelName = NULL; - - pciopts = &def->opts.pciopts; - if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT && - def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + const virDomainPCIControllerOpts *pciopts = &def->opts.pciopts; + const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
This is not equivalent. virDomainControllerPCIModelName implementation actually defines a string for VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT so it will not return NULL and the commit message does not explain why/if it is actually okay.
/* Skip the implicit PHB for pSeries guests */ if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && @@ -2741,6 +2736,13 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, goto done; }
+ if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown virDomainControllerPCIModelName value: %d"),
I think this was under discussion somewhere in this series.
+ pciopts->modelName); + return -1; + } + switch ((virDomainControllerModelPCI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", -- 2.14.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2018-02-19 at 16:06 +0100, Peter Krempa wrote:
case VIR_DOMAIN_CONTROLLER_TYPE_PCI: { - const virDomainPCIControllerOpts *pciopts; - const char *modelName = NULL; - - pciopts = &def->opts.pciopts; - if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT && - def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + const virDomainPCIControllerOpts *pciopts = &def->opts.pciopts; + const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
This is not equivalent. virDomainControllerPCIModelName implementation actually defines a string for VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT so it will not return NULL and the commit message does not explain why/if it is actually okay.
The result doesn't change because the PCIE_ROOT case in the switch statement immediately following the conversion does nothing but error out, which makes whether or not modelName is NULL irrelevant. I can add the explanation above to the commit message if you want. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Feb 19, 2018 at 16:26:35 +0100, Andrea Bolognani wrote:
On Mon, 2018-02-19 at 16:06 +0100, Peter Krempa wrote:
case VIR_DOMAIN_CONTROLLER_TYPE_PCI: { - const virDomainPCIControllerOpts *pciopts; - const char *modelName = NULL; - - pciopts = &def->opts.pciopts; - if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT && - def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + const virDomainPCIControllerOpts *pciopts = &def->opts.pciopts; + const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
This is not equivalent. virDomainControllerPCIModelName implementation actually defines a string for VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT so it will not return NULL and the commit message does not explain why/if it is actually okay.
The result doesn't change because the PCIE_ROOT case in the switch statement immediately following the conversion does nothing but error out, which makes whether or not modelName is NULL irrelevant.
I mistakenly looked into the case for PCI_ROOT, so that mislead me. I think it's okay in this case even without a comment. Thanks for the explanation.
Validate time is a bit too early to check whether the required capabilities are available, since the QEMU binary might have been updated or replaced by the time we are asked to run the guest. Move capability checks (back) to qemuBuildControllerDevStr() and get rid of a lot of redundancy in the process. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 50 +++++++++++++++++++++++++++++++ src/qemu/qemu_domain.c | 80 ++----------------------------------------------- 2 files changed, 52 insertions(+), 78 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a44a1b2d2..a957132bd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2595,6 +2595,35 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, return 0; } +static int +virDomainControllerPCIModelNameToQEMUCaps(int modelName) +{ + switch ((virDomainControllerPCIModelName) modelName) { + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE: + return QEMU_CAPS_DEVICE_PCI_BRIDGE; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE: + return QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420: + return QEMU_CAPS_DEVICE_IOH3420; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM: + return QEMU_CAPS_DEVICE_X3130_UPSTREAM; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM: + return QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB: + return QEMU_CAPS_DEVICE_PXB; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE: + return QEMU_CAPS_DEVICE_PXB_PCIE; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT: + return QEMU_CAPS_DEVICE_PCIE_ROOT_PORT; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE: + return QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE; + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE: + case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST: + break; + } + return -1; +} + /** * qemuBuildControllerDevStr: @@ -2727,6 +2756,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, case VIR_DOMAIN_CONTROLLER_TYPE_PCI: { const virDomainPCIControllerOpts *pciopts = &def->opts.pciopts; const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + int cap = virDomainControllerPCIModelNameToQEMUCaps(pciopts->modelName); /* Skip the implicit PHB for pSeries guests */ if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && @@ -2742,6 +2772,18 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->modelName); return -1; } + if (cap < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown QEMU device for '%s' controller"), + modelName); + return -1; + } + if (!virQEMUCapsGet(qemuCaps, cap)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The '%s' device is not supported by this QEMU binary"), + modelName); + return -1; + } switch ((virDomainControllerModelPCI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: @@ -2770,6 +2812,14 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, pciopts->chassis, def->info.alias); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + if (pciopts->numaNode != -1 && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the spapr-pci-host-bridge controller doesn't " + "support numa_node in this QEMU binary")); + return -1; + } + virBufferAsprintf(&buf, "%s,index=%d,id=%s", modelName, pciopts->targetIndex, def->info.alias); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2bc0259ea..2fbae695a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4244,8 +4244,7 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll static int qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) + const virDomainDef *def) { virDomainControllerModelPCI model = controller->model; const virDomainPCIControllerOpts *pciopts; @@ -4322,13 +4321,6 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pci-bridge controller is not supported " - "in this QEMU binary")); - return -1; - } - break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: @@ -4346,13 +4338,6 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pxb controller is not supported in this " - "QEMU binary")); - return -1; - } - break; case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: @@ -4364,13 +4349,6 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the dmi-to-pci-bridge (i82801b11-bridge) " - "controller is not supported in this QEMU binary")); - return -1; - } - break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: @@ -4389,22 +4367,6 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } - if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pcie-root-port (ioh3420) controller " - "is not supported in this QEMU binary")); - return -1; - } - - if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCIE_ROOT_PORT)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pcie-root-port (pcie-root-port) controller " - "is not supported in this QEMU binary")); - return -1; - } - break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: @@ -4416,13 +4378,6 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pcie-switch-upstream-port (x3130-upstream) " - "controller is not supported in this QEMU binary")); - return -1; - } - break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: @@ -4441,14 +4396,6 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("The pcie-switch-downstream-port " - "(xio3130-downstream) controller is not " - "supported in this QEMU binary")); - return -1; - } - break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: @@ -4466,13 +4413,6 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the pxb-pcie controller is not supported " - "in this QEMU binary")); - return -1; - } - break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: @@ -4494,21 +4434,6 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle return -1; } - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the spapr-pci-host-bridge controller is not " - "supported in this QEMU binary")); - return -1; - } - - if (pciopts->numaNode != -1 && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("the spapr-pci-host-bridge controller doesn't " - "support numa_node in this QEMU binary")); - return -1; - } - break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: @@ -4566,8 +4491,7 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, break; case VIR_DOMAIN_CONTROLLER_TYPE_PCI: - ret = qemuDomainDeviceDefValidateControllerPCI(controller, def, - qemuCaps); + ret = qemuDomainDeviceDefValidateControllerPCI(controller, def); break; case VIR_DOMAIN_CONTROLLER_TYPE_SATA: -- 2.14.3
On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
Validate time is a bit too early to check whether the required capabilities are available, since the QEMU binary might have been updated or replaced by the time we are asked to run the guest.
So are you having problem with the fact that the definition will be rejected right away and not just when you try to start it? Validate is re-run when starting the VM so a downgrade is handled properly.
On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
Validate time is a bit too early to check whether the required capabilities are available, since the QEMU binary might have been updated or replaced by the time we are asked to run the guest.
So are you having problem with the fact that the definition will be rejected right away and not just when you try to start it?
Validate is re-run when starting the VM so a downgrade is handled properly.
Right, but isn't checking for QEMU capabilities at validate time unreasonably strict? A guest which uses eg. an invalid combination of machine type and architecture will never become valid at a later point, but a guest should not be considered invalid just because the QEMU binary you happened to have installed at the time you defined it lacked some features - the guest itself is perfectly valid, it just can't be run :) -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Feb 19, 2018 at 10:29:18 +0100, Andrea Bolognani wrote:
On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
Validate time is a bit too early to check whether the required capabilities are available, since the QEMU binary might have been updated or replaced by the time we are asked to run the guest.
So are you having problem with the fact that the definition will be rejected right away and not just when you try to start it?
Validate is re-run when starting the VM so a downgrade is handled properly.
Right, but isn't checking for QEMU capabilities at validate time unreasonably strict?
Well, the only difference is that you are unable to 'define' such XML. I don't think it's strict since you would not be able to run it anyways.
A guest which uses eg. an invalid combination of machine type and architecture will never become valid at a later point, but a guest should not be considered invalid just because the QEMU binary you happened to have installed at the time you defined it lacked some features - the guest itself is perfectly valid, it just can't be run :)
I think the term 'guest' here is misleading. The configuration is invalid and thus you can't really create a guest out of it. This is actually a great example, as you actually are not able to define a XML which would fail to be run anyways. The specific reason for the validation callback is to not make the definition vanish in such case. In general, I think that the command line generator is the wrong place for such checks since they are scattered around and you can't really figure out whether you can start such config until you've done a lot of preparation steps.
On 02/19/2018 04:29 AM, Andrea Bolognani wrote:
On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
Validate time is a bit too early to check whether the required capabilities are available, since the QEMU binary might have been updated or replaced by the time we are asked to run the guest.
So are you having problem with the fact that the definition will be rejected right away and not just when you try to start it?
Validate is re-run when starting the VM so a downgrade is handled properly.
Right, but isn't checking for QEMU capabilities at validate time unreasonably strict? A guest which uses eg. an invalid combination of machine type and architecture will never become valid at a later point, but a guest should not be considered invalid just because the QEMU binary you happened to have installed at the time you defined it lacked some features - the guest itself is perfectly valid, it just can't be run :)
IIRC - the decision processing I used for moving the capability checking into Validation for controller options (PCI, SCSI, USB, SATA) was because at least qemuDomainDefValidateMemoryHotplug already had failure paths when capabilities weren't set. Undoing PCI would perhaps mean quite a few more places that need adjustment to get that bald yak. As for the question of "unreasonably strict" at Validation time - isn't a purpose of Validation to ensure certain combinations that were defined would work together properly? It just so happens that using certain combinations of options with a specific version of a binary could be that type of check. Although I do see your point. Personally I'd rather get something "right" when defining it rather than find out later - it's one of those - well why didn't you tell me that before type gripes. John
On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote:
On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
Validate time is a bit too early to check whether the required capabilities are available, since the QEMU binary might have been updated or replaced by the time we are asked to run the guest.
So are you having problem with the fact that the definition will be rejected right away and not just when you try to start it?
Validate is re-run when starting the VM so a downgrade is handled properly.
Right, but isn't checking for QEMU capabilities at validate time unreasonably strict? A guest which uses eg. an invalid combination of machine type and architecture will never become valid at a later point, but a guest should not be considered invalid just because the QEMU binary you happened to have installed at the time you defined it lacked some features - the guest itself is perfectly valid, it just can't be run :)
Given that we increasingly fill in alot of information in the XML at define time, we already have a general expectation that the QEMU binary will be present at define time. I think this is not unreasonable - we suggest apps call virConnectGetCapabilities and/or virDomainGetCapabilities to understand what is installed/available before creating an XML document to define. Those APIs of course require binaries to be installed too. So I don't think we should really put effort into coping with defining XML for a time when the QEMU binaries aren't installed. Such a scenario is so unlikely to be hit that any code trying to cope with that is going to be largely untested and fragile, so it would be a disservice to pretend it'll be something worth relying on. 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 :|
On Mon, Feb 19, 2018 at 13:04:08 +0000, Daniel Berrange wrote:
On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote:
On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
Validate time is a bit too early to check whether the required capabilities are available, since the QEMU binary might have been updated or replaced by the time we are asked to run the guest.
So are you having problem with the fact that the definition will be rejected right away and not just when you try to start it?
Validate is re-run when starting the VM so a downgrade is handled properly.
Right, but isn't checking for QEMU capabilities at validate time unreasonably strict? A guest which uses eg. an invalid combination of machine type and architecture will never become valid at a later point, but a guest should not be considered invalid just because the QEMU binary you happened to have installed at the time you defined it lacked some features - the guest itself is perfectly valid, it just can't be run :)
Given that we increasingly fill in alot of information in the XML at define time, we already have a general expectation that the QEMU binary will be present at define time. I think this is not unreasonable - we suggest apps call virConnectGetCapabilities and/or virDomainGetCapabilities to understand what is installed/available before creating an XML document to define. Those APIs of course require binaries to be installed too. So I don't think we should really put effort into coping with defining XML for a time when the QEMU binaries aren't installed. Such a scenario is so unlikely to be hit that any code trying to cope with that is going to be largely untested and fragile, so it would be a disservice to pretend it'll be something worth relying on.
The only situation when we should not fail if QEMU is not installed and you restart libvirtd. Making defined domains disappear is bad. The good thing is that at that point all defaults should be already filled in so it should not matter much whether capabilities are present. Other than that, I agree. Checking stuff uprfront is usually better than get to the situation when it fails to start.
On Mon, Feb 19, 2018 at 02:13:14PM +0100, Peter Krempa wrote:
On Mon, Feb 19, 2018 at 13:04:08 +0000, Daniel Berrange wrote:
On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote:
On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
Validate time is a bit too early to check whether the required capabilities are available, since the QEMU binary might have been updated or replaced by the time we are asked to run the guest.
So are you having problem with the fact that the definition will be rejected right away and not just when you try to start it?
Validate is re-run when starting the VM so a downgrade is handled properly.
Right, but isn't checking for QEMU capabilities at validate time unreasonably strict? A guest which uses eg. an invalid combination of machine type and architecture will never become valid at a later point, but a guest should not be considered invalid just because the QEMU binary you happened to have installed at the time you defined it lacked some features - the guest itself is perfectly valid, it just can't be run :)
Given that we increasingly fill in alot of information in the XML at define time, we already have a general expectation that the QEMU binary will be present at define time. I think this is not unreasonable - we suggest apps call virConnectGetCapabilities and/or virDomainGetCapabilities to understand what is installed/available before creating an XML document to define. Those APIs of course require binaries to be installed too. So I don't think we should really put effort into coping with defining XML for a time when the QEMU binaries aren't installed. Such a scenario is so unlikely to be hit that any code trying to cope with that is going to be largely untested and fragile, so it would be a disservice to pretend it'll be something worth relying on.
The only situation when we should not fail if QEMU is not installed and you restart libvirtd. Making defined domains disappear is bad.
A long time back we did have a patch series somewhere that tried to keep domain's loaded, but mark them as "broken" if the QEMU we needed was missing. Can't remember why we never went further with it now... 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 :|
On Mon, Feb 19, 2018 at 13:21:35 +0000, Daniel Berrange wrote:
On Mon, Feb 19, 2018 at 02:13:14PM +0100, Peter Krempa wrote:
On Mon, Feb 19, 2018 at 13:04:08 +0000, Daniel Berrange wrote:
On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote:
On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote:
Validate time is a bit too early to check whether the required capabilities are available, since the QEMU binary might have been updated or replaced by the time we are asked to run the guest.
So are you having problem with the fact that the definition will be rejected right away and not just when you try to start it?
Validate is re-run when starting the VM so a downgrade is handled properly.
Right, but isn't checking for QEMU capabilities at validate time unreasonably strict? A guest which uses eg. an invalid combination of machine type and architecture will never become valid at a later point, but a guest should not be considered invalid just because the QEMU binary you happened to have installed at the time you defined it lacked some features - the guest itself is perfectly valid, it just can't be run :)
Given that we increasingly fill in alot of information in the XML at define time, we already have a general expectation that the QEMU binary will be present at define time. I think this is not unreasonable - we suggest apps call virConnectGetCapabilities and/or virDomainGetCapabilities to understand what is installed/available before creating an XML document to define. Those APIs of course require binaries to be installed too. So I don't think we should really put effort into coping with defining XML for a time when the QEMU binaries aren't installed. Such a scenario is so unlikely to be hit that any code trying to cope with that is going to be largely untested and fragile, so it would be a disservice to pretend it'll be something worth relying on.
The only situation when we should not fail if QEMU is not installed and you restart libvirtd. Making defined domains disappear is bad.
A long time back we did have a patch series somewhere that tried to keep domain's loaded, but mark them as "broken" if the QEMU we needed was missing. Can't remember why we never went further with it now...
Well in the meanwhile I've made qemuCaps in the PostParse callbacks optional and it's re-run in such case on startup of the VM. The validation callbacks are not run when parsing status/config XMLs so it should mostly work right now properly. (at least it fixed my case when my binaries failed to exec due to broken deps)
On Mon, 2018-02-19 at 14:30 +0100, Peter Krempa wrote:
On Mon, Feb 19, 2018 at 13:21:35 +0000, Daniel Berrange wrote:
On Mon, Feb 19, 2018 at 02:13:14PM +0100, Peter Krempa wrote:
The only situation when we should not fail if QEMU is not installed and you restart libvirtd. Making defined domains disappear is bad.
I think we can all agree on that :)
A long time back we did have a patch series somewhere that tried to keep domain's loaded, but mark them as "broken" if the QEMU we needed was missing. Can't remember why we never went further with it now...
Well in the meanwhile I've made qemuCaps in the PostParse callbacks optional and it's re-run in such case on startup of the VM. The validation callbacks are not run when parsing status/config XMLs so it should mostly work right now properly. (at least it fixed my case when my binaries failed to exec due to broken deps)
Okay, I'll leave capabilities checks in the validate callback. -- Andrea Bolognani / Red Hat / Virtualization
We will rewrite pretty much every single line of this function over the course of the next several commits, and starting from a clean slate rather than replacing it bit by bit makes the resulting diffs unmeasurably easier to read and understand, and you need fewer of them to boot. Trust me, I tried the other approach first :) Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 199 +------------------------------------------------ 1 file changed, 2 insertions(+), 197 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2fbae695a..9dc3d5597 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4243,204 +4243,9 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll static int -qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, - const virDomainDef *def) +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller ATTRIBUTE_UNUSED, + const virDomainDef *def ATTRIBUTE_UNUSED) { - virDomainControllerModelPCI model = controller->model; - const virDomainPCIControllerOpts *pciopts; - const char *modelName = NULL; - - /* skip pcie-root */ - if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) - return 0; - - /* Skip pci-root, except for pSeries guests (which actually - * support more than one PCI Host Bridge per guest) */ - if (!qemuDomainIsPSeries(def) && - controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) - return 0; - - /* First pass - just check the controller index for the model's - * that we care to check... */ - switch (model) { - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - if (controller->idx == 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("index for pci controllers of model '%s' must be > 0"), - virDomainControllerModelPCITypeToString(model)); - return -1; - } - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: - break; - } - - pciopts = &controller->opts.pciopts; - if (controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT && - controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) { - if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("autogenerated %s options not set"), - virDomainControllerModelPCITypeToString(controller->model)); - return -1; - } - - modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); - if (!modelName) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown %s modelName value %d"), - virDomainControllerModelPCITypeToString(controller->model), - pciopts->modelName); - return -1; - } - } - - /* Second pass - now the model specific checks */ - switch (model) { - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - if (pciopts->chassisNr == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-bridge options not set")); - return -1; - } - - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pci-bridge"), - modelName); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: - if (pciopts->busNr == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-expander-bus options not set")); - return -1; - } - - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pci-expander-bus"), - modelName); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a dmi-to-pci-bridge"), - modelName); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: - if (pciopts->chassis == -1 || pciopts->port == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-root-port options not set")); - return -1; - } - - if ((pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420) && - (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pcie-root-port"), - modelName); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pcie-switch-upstream-port"), - modelName); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: - if (pciopts->chassis == -1 || pciopts->port == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-switch-downstream-port " - "options not set")); - return -1; - } - - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pcie-switch-downstream-port"), - modelName); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: - if (pciopts->busNr == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pcie-expander-bus options not set")); - return -1; - } - - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pcie-expander-bus"), - modelName); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - if (pciopts->targetIndex == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("autogenerated pci-root options not set")); - return -1; - } - - /* Skip the implicit one */ - if (pciopts->targetIndex == 0) - return 0; - - if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("PCI controller model name '%s' is not valid " - "for a pci-root"), - modelName); - return -1; - } - - break; - - case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: - break; - } - return 0; } -- 2.14.3
On Fri, Feb 16, 2018 at 17:28:04 +0100, Andrea Bolognani wrote:
We will rewrite pretty much every single line of this function over the course of the next several commits, and starting from a clean slate rather than replacing it bit by bit makes the resulting diffs unmeasurably easier to read and understand, and you need fewer of them to boot. Trust me, I tried the other approach first :)
Will this remove any checks during the series? If yes, then you probably should at first rename this function and add a almost-empty wrapper then add new code to the wrapper and delete the renamed function at the end.
On Mon, 2018-02-19 at 07:28 +0100, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 17:28:04 +0100, Andrea Bolognani wrote:
We will rewrite pretty much every single line of this function over the course of the next several commits, and starting from a clean slate rather than replacing it bit by bit makes the resulting diffs unmeasurably easier to read and understand, and you need fewer of them to boot. Trust me, I tried the other approach first :)
Will this remove any checks during the series? If yes, then you probably should at first rename this function and add a almost-empty wrapper then add new code to the wrapper and delete the renamed function at the end.
No, if anything it *adds* a bunch of checks :) Renaming the function won't work because then the compiler will complain about it being unused. Unless you meant something like /* Delete once done */ ValidateControllerPCIOld() { /* Existing checks here */ } ValidateControllerPCI() { /* New checks here */ /* Delete once done */ ValidateControllerPCIOld(); } which could actually do the trick. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Feb 19, 2018 at 10:22:29 +0100, Andrea Bolognani wrote:
On Mon, 2018-02-19 at 07:28 +0100, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 17:28:04 +0100, Andrea Bolognani wrote:
We will rewrite pretty much every single line of this function over the course of the next several commits, and starting from a clean slate rather than replacing it bit by bit makes the resulting diffs unmeasurably easier to read and understand, and you need fewer of them to boot. Trust me, I tried the other approach first :)
Will this remove any checks during the series? If yes, then you probably should at first rename this function and add a almost-empty wrapper then add new code to the wrapper and delete the renamed function at the end.
No, if anything it *adds* a bunch of checks :)
Well, I meant that after applying this patch a bunch of checks will vanish until you add them in the next patches, which I don't think we should do.
Renaming the function won't work because then the compiler will complain about it being unused. Unless you meant something like
/* Delete once done */ ValidateControllerPCIOld() { /* Existing checks here */ }
ValidateControllerPCI() { /* New checks here */
/* Delete once done */ ValidateControllerPCIOld(); }
which could actually do the trick.
I meant this flow obviously, so that the checks are kept until fixed/reimplemented.
On Mon, 2018-02-19 at 10:40 +0100, Peter Krempa wrote:
Renaming the function won't work because then the compiler will complain about it being unused. Unless you meant something like
/* Delete once done */ ValidateControllerPCIOld() { /* Existing checks here */ }
ValidateControllerPCI() { /* New checks here */
/* Delete once done */ ValidateControllerPCIOld(); }
which could actually do the trick.
I meant this flow obviously, so that the checks are kept until fixed/reimplemented.
It was not obvious to me when reading the first message :) But I agree it's a better way to handle the situation, so I will do it this way in v3. -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 54 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9dc3d5597..e4e67c585 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4243,9 +4243,61 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll static int -qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller ATTRIBUTE_UNUSED, - const virDomainDef *def ATTRIBUTE_UNUSED) +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, + const virDomainDef *def) { + const virDomainPCIControllerOpts *pciopts = &controller->opts.pciopts; + const char *model = virDomainControllerModelPCITypeToString(controller->model); + const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + + if (!model) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown virDomainControllerModelPCI value: %d"), + controller->model); + return -1; + } + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown virDomainControllerPCIModelName value: %d"), + pciopts->modelName); + return -1; + } + + /* index */ + switch ((virDomainControllerModelPCI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (controller->idx == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Index for '%s' controller must be > 0"), + model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* pci-root controllers for pSeries guests can have any index, but + * all other pci-root and pcie-root controller must have index 0 */ + if (controller->idx != 0 && + !(qemuDomainIsPSeries(def) && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Index for '%s' controller must be 0"), + model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + return 0; } -- 2.14.3
On Fri, Feb 16, 2018 at 05:28:05PM +0100, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 54 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9dc3d5597..e4e67c585 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4243,9 +4243,61 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll
static int -qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller ATTRIBUTE_UNUSED, - const virDomainDef *def ATTRIBUTE_UNUSED) +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, + const virDomainDef *def) { + const virDomainPCIControllerOpts *pciopts = &controller->opts.pciopts; + const char *model = virDomainControllerModelPCITypeToString(controller->model); + const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + + if (!model) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown virDomainControllerModelPCI value: %d"),
Including type names in error messages is not too nice as a user facing error - better to use plain english "Unexpected PCI controller model")
+ controller->model); + return -1; + } + if (!modelName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown virDomainControllerPCIModelName value: %d"), + pciopts->modelName);
Likewise here.
+ return -1; + } + + /* index */ + switch ((virDomainControllerModelPCI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (controller->idx == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Index for '%s' controller must be > 0"), + model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* pci-root controllers for pSeries guests can have any index, but + * all other pci-root and pcie-root controller must have index 0 */ + if (controller->idx != 0 && + !(qemuDomainIsPSeries(def) && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Index for '%s' controller must be 0"), + model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
Any time there is a _LAST element we should have an error report too virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected controller model %d"), controller->model); return -1; It should also have an 'default:' next to the LAST. Both are things that should never occur unless we've screwed up code elsewherein libvirt, but we want to be robust about catching such screw ups. This is particularly important for controller models, as we have many different controller type specific enums all stored in the same struct field 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 :|
On Mon, 2018-02-19 at 11:52 +0000, Daniel P. Berrangé wrote:
+qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller, + const virDomainDef *def) { + const virDomainPCIControllerOpts *pciopts = &controller->opts.pciopts; + const char *model = virDomainControllerModelPCITypeToString(controller->model); + const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName); + + if (!model) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown virDomainControllerModelPCI value: %d"),
Including type names in error messages is not too nice as a user facing error - better to use plain english "Unexpected PCI controller model")
But these are marked as internal errors, eg. They Should Never Happen™, and if they ever do it's useful for developers to have more detailed information. Users can't really do anything but report the issue, after all.
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* pci-root controllers for pSeries guests can have any index, but + * all other pci-root and pcie-root controller must have index 0 */ + if (controller->idx != 0 && + !(qemuDomainIsPSeries(def) && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Index for '%s' controller must be 0"), + model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
Any time there is a _LAST element we should have an error report too
virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected controller model %d"), controller->model); return -1;
It should also have an 'default:' next to the LAST. Both are things that should never occur unless we've screwed up code elsewherein libvirt, but we want to be robust about catching such screw ups. This is particularly important for controller models, as we have many different controller type specific enums all stored in the same struct field
Okay, I'll address that, but the point above about what error message to show in such scenarios still applies IMHO. -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e4e67c585..2f76464df 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4298,6 +4298,116 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle break; } + /* modelName */ + switch ((virDomainControllerModelPCI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "modelName", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + switch ((virDomainControllerModelPCI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE && + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pci-root"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pci-bridge"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "dmi-to-pci-bridge"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420 && + pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-root-port"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-switch-upstream-port"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pci-switch-downstream-port"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pci-expander-bus"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-expander-bus"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Model name '%s' is not valid for '%s' controller"), + modelName, "pcie-root"); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + return 0; } -- 2.14.3
Some test cases that were specifically set up to exploit the lack of checks on PCI controller options start failing with these changes, so they are marked as such. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 43 ++++++++++++ .../i440fx-controllers-pciopts.args | 24 ------- .../pseries-controllers-pciopts.args | 22 ------- .../qemuxml2argvdata/q35-controllers-pciopts.args | 28 -------- tests/qemuxml2argvtest.c | 19 +----- .../i440fx-controllers-pciopts.xml | 45 ------------- .../pseries-controllers-pciopts.xml | 43 ------------ .../qemuxml2xmloutdata/q35-controllers-pciopts.xml | 76 ---------------------- tests/qemuxml2xmltest.c | 17 ----- 9 files changed, 46 insertions(+), 271 deletions(-) delete mode 100644 tests/qemuxml2argvdata/i440fx-controllers-pciopts.args delete mode 100644 tests/qemuxml2argvdata/pseries-controllers-pciopts.args delete mode 100644 tests/qemuxml2argvdata/q35-controllers-pciopts.args delete mode 100644 tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml delete mode 100644 tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml delete mode 100644 tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2f76464df..52c76b515 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4408,6 +4408,49 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle break; } + /* targetIndex */ + switch ((virDomainControllerModelPCI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + /* PHBs for pSeries guests are stored as pci-root controllers and + * must have been assigned a targetIndex */ + if (pciopts->targetIndex == -1 && + qemuDomainIsPSeries(def)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "targetIndex", model); + return -1; + } + /* targetIndex only applies to pSeries guests, so for any other + * guest type it being present is an error */ + if (pciopts->targetIndex != -1 && + !qemuDomainIsPSeries(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "targetIndex", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->targetIndex != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "targetIndex", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + return 0; } diff --git a/tests/qemuxml2argvdata/i440fx-controllers-pciopts.args b/tests/qemuxml2argvdata/i440fx-controllers-pciopts.args deleted file mode 100644 index d85fae5c9..000000000 --- a/tests/qemuxml2argvdata/i440fx-controllers-pciopts.args +++ /dev/null @@ -1,24 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/home/test \ -USER=test \ -LOGNAME=test \ -QEMU_AUDIO_DRV=none \ -/usr/bin/qemu-system-x86_64 \ --name guest \ --S \ --M pc \ --m 1024 \ --smp 1,sockets=1,cores=1,threads=1 \ --numa node,nodeid=0,cpus=0,mem=1024 \ --uuid 496d7ea8-9739-544b-4ebd-ef08be936e8b \ --nographic \ --nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ -server,nowait \ --mon chardev=charmonitor,id=monitor,mode=readline \ --no-acpi \ --boot c \ --global i440FX-pcihost.pci-hole64-size=1024K \ --device pci-bridge,chassis_nr=2,id=pci.1,bus=pci.0,addr=0x3 \ --device pxb,bus_nr=3,id=pci.2,numa_node=0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/pseries-controllers-pciopts.args b/tests/qemuxml2argvdata/pseries-controllers-pciopts.args deleted file mode 100644 index 5f1edfc83..000000000 --- a/tests/qemuxml2argvdata/pseries-controllers-pciopts.args +++ /dev/null @@ -1,22 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/home/test \ -USER=test \ -LOGNAME=test \ -QEMU_AUDIO_DRV=none \ -/usr/bin/qemu-system-ppc64 \ --name guest \ --S \ --M pseries \ --m 1024 \ --smp 1,sockets=1,cores=1,threads=1 \ --numa node,nodeid=0,cpus=0,mem=1024 \ --uuid 496d7ea8-9739-544b-4ebd-ef08be936e8b \ --nographic \ --nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ -server,nowait \ --mon chardev=charmonitor,id=monitor,mode=readline \ --boot c \ --device spapr-pci-host-bridge,index=1,id=pci.1,numa_node=0 \ --device pci-bridge,chassis_nr=3,id=pci.2,bus=pci.0,addr=0x1 diff --git a/tests/qemuxml2argvdata/q35-controllers-pciopts.args b/tests/qemuxml2argvdata/q35-controllers-pciopts.args deleted file mode 100644 index 44259f502..000000000 --- a/tests/qemuxml2argvdata/q35-controllers-pciopts.args +++ /dev/null @@ -1,28 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/home/test \ -USER=test \ -LOGNAME=test \ -QEMU_AUDIO_DRV=none \ -/usr/bin/qemu-system-x86_64 \ --name guest \ --S \ --M q35 \ --m 1024 \ --smp 1,sockets=1,cores=1,threads=1 \ --numa node,nodeid=0,cpus=0,mem=1024 \ --uuid 496d7ea8-9739-544b-4ebd-ef08be936e8b \ --nographic \ --nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ -server,nowait \ --mon chardev=charmonitor,id=monitor,mode=readline \ --no-acpi \ --boot c \ --global q35-pcihost.pci-hole64-size=1024K \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ --device pci-bridge,chassis_nr=3,id=pci.2,bus=pci.1,addr=0x0 \ --device pxb-pcie,bus_nr=4,id=pci.3,numa_node=0,bus=pcie.0,addr=0x2 \ --device pcie-root-port,port=0x5,chassis=5,id=pci.4,bus=pcie.0,addr=0x3 \ --device x3130-upstream,id=pci.5,bus=pci.4,addr=0x0 \ --device xio3130-downstream,port=0x7,chassis=7,id=pci.6,bus=pci.5,addr=0x0 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4154663da..525042e95 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2435,22 +2435,9 @@ mymain(void) QEMU_CAPS_DEVICE_IOH3420, QEMU_CAPS_DEVICE_PXB_PCIE); - DO_TEST("i440fx-controllers-pciopts", - QEMU_CAPS_I440FX_PCI_HOLE64_SIZE, - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_PXB); - DO_TEST("q35-controllers-pciopts", - QEMU_CAPS_Q35_PCI_HOLE64_SIZE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_PXB_PCIE, - QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, - QEMU_CAPS_DEVICE_X3130_UPSTREAM, - QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM); - DO_TEST("pseries-controllers-pciopts", - QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, - QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE, - QEMU_CAPS_DEVICE_PCI_BRIDGE); + DO_TEST_PARSE_ERROR("i440fx-controllers-pciopts", NONE); + DO_TEST_PARSE_ERROR("q35-controllers-pciopts", NONE); + DO_TEST_PARSE_ERROR("pseries-controllers-pciopts", NONE); DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI, diff --git a/tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml b/tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml deleted file mode 100644 index d171d1370..000000000 --- a/tests/qemuxml2xmloutdata/i440fx-controllers-pciopts.xml +++ /dev/null @@ -1,45 +0,0 @@ -<domain type='qemu'> - <name>guest</name> - <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> - <memory unit='KiB'>1048576</memory> - <currentMemory unit='KiB'>1048576</currentMemory> - <vcpu placement='static'>1</vcpu> - <os> - <type arch='x86_64' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <cpu> - <numa> - <cell id='0' cpus='0' memory='1048576' unit='KiB'/> - </numa> - </cpu> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - <controller type='pci' index='0' model='pci-root'> - <target chassisNr='1' chassis='1' port='0x1' busNr='1' index='0'/> - <pcihole64 unit='KiB'>1024</pcihole64> - </controller> - <controller type='pci' index='1' model='pci-bridge'> - <model name='pci-bridge'/> - <target chassisNr='2' chassis='2' port='0x2' busNr='2' index='2'> - <node>0</node> - </target> - <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> - </controller> - <controller type='pci' index='2' model='pci-expander-bus'> - <model name='pxb'/> - <target chassisNr='3' chassis='3' port='0x3' busNr='3' index='3'> - <node>0</node> - </target> - <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> - </controller> - <controller type='usb' index='0' model='none'/> - <input type='mouse' bus='ps2'/> - <input type='keyboard' bus='ps2'/> - <memballoon model='none'/> - </devices> -</domain> diff --git a/tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml b/tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml deleted file mode 100644 index bbe360e25..000000000 --- a/tests/qemuxml2xmloutdata/pseries-controllers-pciopts.xml +++ /dev/null @@ -1,43 +0,0 @@ -<domain type='qemu'> - <name>guest</name> - <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> - <memory unit='KiB'>1048576</memory> - <currentMemory unit='KiB'>1048576</currentMemory> - <vcpu placement='static'>1</vcpu> - <os> - <type arch='ppc64' machine='pseries'>hvm</type> - <boot dev='hd'/> - </os> - <cpu> - <numa> - <cell id='0' cpus='0' memory='1048576' unit='KiB'/> - </numa> - </cpu> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu-system-ppc64</emulator> - <controller type='pci' index='0' model='pci-root'> - <model name='spapr-pci-host-bridge'/> - <target chassisNr='1' chassis='1' port='0x1' busNr='1' index='0'/> - </controller> - <controller type='pci' index='1' model='pci-root'> - <model name='spapr-pci-host-bridge'/> - <target chassisNr='2' chassis='2' port='0x2' busNr='2' index='1'> - <node>0</node> - </target> - </controller> - <controller type='pci' index='2' model='pci-bridge'> - <model name='pci-bridge'/> - <target chassisNr='3' chassis='3' port='0x3' busNr='3' index='3'> - <node>0</node> - </target> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> - </controller> - <controller type='usb' index='0' model='none'/> - <memballoon model='none'/> - <panic model='pseries'/> - </devices> -</domain> diff --git a/tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml b/tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml deleted file mode 100644 index 5ef7aa564..000000000 --- a/tests/qemuxml2xmloutdata/q35-controllers-pciopts.xml +++ /dev/null @@ -1,76 +0,0 @@ -<domain type='qemu'> - <name>guest</name> - <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> - <memory unit='KiB'>1048576</memory> - <currentMemory unit='KiB'>1048576</currentMemory> - <vcpu placement='static'>1</vcpu> - <os> - <type arch='x86_64' machine='q35'>hvm</type> - <boot dev='hd'/> - </os> - <cpu> - <numa> - <cell id='0' cpus='0' memory='1048576' unit='KiB'/> - </numa> - </cpu> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - <controller type='pci' index='0' model='pcie-root'> - <target chassisNr='1' chassis='1' port='0x1' busNr='1' index='0'/> - <pcihole64 unit='KiB'>1024</pcihole64> - </controller> - <controller type='pci' index='1' model='dmi-to-pci-bridge'> - <model name='i82801b11-bridge'/> - <target chassisNr='2' chassis='2' port='0x2' busNr='2' index='2'> - <node>0</node> - </target> - <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> - </controller> - <controller type='pci' index='2' model='pci-bridge'> - <model name='pci-bridge'/> - <target chassisNr='3' chassis='3' port='0x3' busNr='3' index='3'> - <node>0</node> - </target> - <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> - </controller> - <controller type='pci' index='3' model='pcie-expander-bus'> - <model name='pxb-pcie'/> - <target chassisNr='4' chassis='4' port='0x4' busNr='4' index='4'> - <node>0</node> - </target> - <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> - </controller> - <controller type='pci' index='4' model='pcie-root-port'> - <model name='pcie-root-port'/> - <target chassisNr='5' chassis='5' port='0x5' busNr='5' index='5'> - <node>0</node> - </target> - <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> - </controller> - <controller type='pci' index='5' model='pcie-switch-upstream-port'> - <model name='x3130-upstream'/> - <target chassisNr='6' chassis='6' port='0x6' busNr='6' index='6'> - <node>0</node> - </target> - <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/> - </controller> - <controller type='pci' index='6' model='pcie-switch-downstream-port'> - <model name='xio3130-downstream'/> - <target chassisNr='7' chassis='7' port='0x7' busNr='7' index='7'> - <node>0</node> - </target> - <address type='pci' domain='0x0000' bus='0x05' 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'/> - </controller> - <input type='mouse' bus='ps2'/> - <input type='keyboard' bus='ps2'/> - <memballoon model='none'/> - </devices> -</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7b8a16078..0eb9e6c77 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1102,23 +1102,6 @@ mymain(void) QEMU_CAPS_DEVICE_IOH3420, QEMU_CAPS_HDA_DUPLEX); - DO_TEST("i440fx-controllers-pciopts", - QEMU_CAPS_I440FX_PCI_HOLE64_SIZE, - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_PXB); - DO_TEST("q35-controllers-pciopts", - QEMU_CAPS_Q35_PCI_HOLE64_SIZE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_PXB_PCIE, - QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, - QEMU_CAPS_DEVICE_X3130_UPSTREAM, - QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM); - DO_TEST("pseries-controllers-pciopts", - QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, - QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE, - QEMU_CAPS_DEVICE_PCI_BRIDGE); - DO_TEST("hostdev-scsi-vhost-scsi-ccw", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VHOST_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC, QEMU_CAPS_VIRTIO_CCW); -- 2.14.3
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 52c76b515..b4dd7a7e8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4451,6 +4451,43 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle break; } + /* pcihole64 */ + switch ((virDomainControllerModelPCI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* The pcihole64 option only applies to x86 guests */ + if ((pciopts->pcihole64 || + pciopts->pcihole64size != 0) && + !ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller " + "on '%s' architecture or '%s' machine type"), + "pcihole64", model, + virArchToString(def->os.arch), def->os.machine); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (pciopts->pcihole64 || + pciopts->pcihole64size != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "pcihole64", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + return 0; } -- 2.14.3
This change catches an invalid use of the option in our test suite. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 31 ++++++++++++++++++++++++++ tests/qemuxml2argvdata/pcie-expander-bus.xml | 2 +- tests/qemuxml2xmloutdata/pcie-expander-bus.xml | 2 +- 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b4dd7a7e8..81f347ce4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4488,6 +4488,37 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle break; } + /* busNr */ + switch ((virDomainControllerModelPCI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + if (pciopts->busNr == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "busNr", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->busNr != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "busNr", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + return 0; } diff --git a/tests/qemuxml2argvdata/pcie-expander-bus.xml b/tests/qemuxml2argvdata/pcie-expander-bus.xml index ac01c26cc..237430a1e 100644 --- a/tests/qemuxml2argvdata/pcie-expander-bus.xml +++ b/tests/qemuxml2argvdata/pcie-expander-bus.xml @@ -35,7 +35,7 @@ <address type='pci' bus='0x00' slot='4'/> </controller> <controller type='pci' index='2' model='pcie-root-port'> - <target busNr='220'> + <target> <node>1</node> </target> <address type='pci' bus='0x01'/> diff --git a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml index aaac423ca..d5e741b80 100644 --- a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml +++ b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml @@ -36,7 +36,7 @@ </controller> <controller type='pci' index='2' model='pcie-root-port'> <model name='ioh3420'/> - <target chassis='2' port='0x0' busNr='220'> + <target chassis='2' port='0x0'> <node>1</node> </target> <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> -- 2.14.3
This change catches an invalid use of the option in our test suite. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 39 ++++++++++++++++++++++++++ tests/qemuxml2argvdata/pcie-expander-bus.xml | 3 -- tests/qemuxml2xmloutdata/pcie-expander-bus.xml | 4 +-- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 81f347ce4..965a8384c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4519,6 +4519,45 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle break; } + /* numaNode */ + switch ((virDomainControllerModelPCI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + /* numaNode can be used for these controllers, but it's not set + * automatically so it can be missing */ + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + /* The pci-root controller only support numaNode for pSeries guests */ + if (pciopts->numaNode != -1 && + !qemuDomainIsPSeries(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller " + "on '%s' architecture or '%s' machine type"), + "numaNode", model, + virArchToString(def->os.arch), def->os.machine); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->numaNode != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "numaNode", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + return 0; } diff --git a/tests/qemuxml2argvdata/pcie-expander-bus.xml b/tests/qemuxml2argvdata/pcie-expander-bus.xml index 237430a1e..5c5d34d1e 100644 --- a/tests/qemuxml2argvdata/pcie-expander-bus.xml +++ b/tests/qemuxml2argvdata/pcie-expander-bus.xml @@ -35,9 +35,6 @@ <address type='pci' bus='0x00' slot='4'/> </controller> <controller type='pci' index='2' model='pcie-root-port'> - <target> - <node>1</node> - </target> <address type='pci' bus='0x01'/> </controller> <controller type='pci' index='3' model='pcie-switch-upstream-port'> diff --git a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml index d5e741b80..b6498fd2e 100644 --- a/tests/qemuxml2xmloutdata/pcie-expander-bus.xml +++ b/tests/qemuxml2xmloutdata/pcie-expander-bus.xml @@ -36,9 +36,7 @@ </controller> <controller type='pci' index='2' model='pcie-root-port'> <model name='ioh3420'/> - <target chassis='2' port='0x0'> - <node>1</node> - </target> + <target chassis='2' port='0x0'/> <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> </controller> <controller type='pci' index='3' model='pcie-switch-upstream-port'> -- 2.14.3
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 965a8384c..99573dee9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4558,6 +4558,37 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle break; } + /* chassisNr */ + switch ((virDomainControllerModelPCI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (pciopts->chassisNr == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "chassisNr", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->chassisNr != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "chassisNr", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + return 0; } -- 2.14.3
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 99573dee9..df725ed74 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4589,6 +4589,49 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle break; } + /* chassis and port */ + switch ((virDomainControllerModelPCI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + if (pciopts->chassis == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "chassis", model); + return -1; + } + if (pciopts->port == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Option '%s' not set for '%s' controller"), + "port", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + if (pciopts->chassis != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "chassis", model); + return -1; + } + if (pciopts->port != -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Option '%s' is not valid for '%s' controller"), + "port", model); + return -1; + } + break; + + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + return 0; } -- 2.14.3
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
John Ferlan -
Peter Krempa