When qemuAssignDevicePCISlots() is looking for companion controllers for a USB controller that has no PCI address specified, it initializes a virDevicePCIAddress to 0000:00:00.0, fills it in with the companion's address if one is found, then checks whether or not there was a find based on slot == 0. On a system with a single PCI bus, that is a valid way to check, because slot 0 is reserved, but on most other PCI buses, slot 0 is not reserved, and is open for use by any device. This patch adds a separate bool that is set when a companion is found rather than relying on the faulty information provided with "slot == 0". --- src/qemu/qemu_command.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8fdf90c..3c867de 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2519,11 +2519,14 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, /* USB2 needs special handling to put all companions in the same slot */ if (IS_USB2_CONTROLLER(def->controllers[i])) { virDevicePCIAddress addr = { 0, 0, 0, 0, false }; + bool foundAddr = false; + memset(&tmp_addr, 0, sizeof(tmp_addr)); for (j = 0; j < i; j++) { if (IS_USB2_CONTROLLER(def->controllers[j]) && def->controllers[j]->idx == def->controllers[i]->idx) { addr = def->controllers[j]->info.addr.pci; + foundAddr = true; break; } } @@ -2544,7 +2547,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, break; } - if (addr.slot == 0) { + if (!foundAddr) { /* This is the first part of the controller, so need * to find a free slot & then reserve a function */ if (virDomainPCIAddressGetNextSlot(addrs, &tmp_addr, flags) < 0) -- 2.4.3

The real Q35 machine puts the first USB controller set (EHCI+(UHCIx4)) on bus 0 slot 0x1D, and the 2nd USB controller set on bus 0 slot 0x1A, so let's attempt to make the virtual machine match that for controllers with auto-assigned addresses when possible. Three test cases were added to assure that the proper addresses are assigned - one with a single set of unaddressed USB controllers, one with 3 (to grab both preferred slots plus one more), and one with the order of the controller definitions reordered, to assure that the auto-assignment isn't mixed up by order. --- src/qemu/qemu_command.c | 52 ++++++++++++++++- .../qemuxml2argv-q35-usb2-multi.args | 40 +++++++++++++ .../qemuxml2argv-q35-usb2-multi.xml | 47 +++++++++++++++ .../qemuxml2argv-q35-usb2-reorder.args | 40 +++++++++++++ .../qemuxml2argv-q35-usb2-reorder.xml | 47 +++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.args | 30 ++++++++++ tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.xml | 39 +++++++++++++ tests/qemuxml2argvtest.c | 21 +++++++ .../qemuxml2xmlout-q35-usb2-multi.xml | 66 ++++++++++++++++++++++ .../qemuxml2xmlout-q35-usb2-reorder.xml | 66 ++++++++++++++++++++++ .../qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2.xml | 46 +++++++++++++++ tests/qemuxml2xmltest.c | 3 + 12 files changed, 494 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-multi.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-multi.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-reorder.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-reorder.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2-multi.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2-reorder.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3c867de..06e3073 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2049,6 +2049,47 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, } break; + case VIR_DOMAIN_CONTROLLER_TYPE_USB: + if ((def->controllers[i]->model + == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1) && + (def->controllers[i]->info.type + == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)) { + /* Try to assign the first found USB2 controller to + * 00:1D.0 and 2nd to 00:1A.0 (because that is their + * standard location on real Q35 hardware) unless they + * are already taken, but don't insist on it. + * + * (NB: all other controllers at the same index will + * get assigned to the same slot as the UHCI1 when + * addresses are later assigned to all devices.) + */ + bool assign = false; + + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 0x1D; + if (!virDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { + assign = true; + } else { + tmp_addr.slot = 0x1A; + if (!virDomainPCIAddressSlotInUse(addrs, &tmp_addr)) + assign = true; + } + if (assign) { + if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, + flags, false, true) < 0) + goto cleanup; + def->controllers[i]->info.type + = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + def->controllers[i]->info.addr.pci.domain = 0; + def->controllers[i]->info.addr.pci.bus = 0; + def->controllers[i]->info.addr.pci.slot = tmp_addr.slot; + def->controllers[i]->info.addr.pci.function = 0; + def->controllers[i]->info.addr.pci.multi + = VIR_TRISTATE_SWITCH_ON; + } + } + break; + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE && def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { @@ -2522,9 +2563,11 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, bool foundAddr = false; memset(&tmp_addr, 0, sizeof(tmp_addr)); - for (j = 0; j < i; j++) { + for (j = 0; j < def->ncontrollers; j++) { if (IS_USB2_CONTROLLER(def->controllers[j]) && - def->controllers[j]->idx == def->controllers[i]->idx) { + def->controllers[j]->idx == def->controllers[i]->idx && + def->controllers[j]->info.type + == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { addr = def->controllers[j]->info.addr.pci; foundAddr = true; break; @@ -2534,6 +2577,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, switch (def->controllers[i]->model) { case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1: addr.function = 7; + addr.multi = VIR_TRISTATE_SWITCH_ABSENT; break; case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1: addr.function = 0; @@ -2541,9 +2585,11 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, break; case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2: addr.function = 1; + addr.multi = VIR_TRISTATE_SWITCH_ABSENT; break; case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3: addr.function = 2; + addr.multi = VIR_TRISTATE_SWITCH_ABSENT; break; } @@ -2562,7 +2608,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, } /* Finally we can reserve the slot+function */ if (virDomainPCIAddressReserveAddr(addrs, &addr, flags, - false, false) < 0) + false, foundAddr) < 0) goto error; def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-multi.args b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-multi.args new file mode 100644 index 0000000..0d6ddd8 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-multi.args @@ -0,0 +1,40 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-kvm \ +-name q35-test \ +-S \ +-M q35 \ +-m 2048 \ +-smp 2 \ +-uuid 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi \ +-boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=56,id=pci.2,bus=pci.1,addr=0x1 \ +-device ich9-usb-ehci1,id=usb,bus=pcie.0,addr=0x1d.0x7 \ +-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pcie.0,multifunction=on,\ +addr=0x1d \ +-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pcie.0,addr=0x1d.0x1 \ +-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pcie.0,addr=0x1d.0x2 \ +-device ich9-usb-ehci1,id=usb1,bus=pcie.0,addr=0x1a.0x7 \ +-device ich9-usb-uhci1,masterbus=usb1.0,firstport=0,bus=pcie.0,multifunction=on,\ +addr=0x1a \ +-device ich9-usb-uhci2,masterbus=usb1.0,firstport=2,bus=pcie.0,addr=0x1a.0x1 \ +-device ich9-usb-uhci3,masterbus=usb1.0,firstport=4,bus=pcie.0,addr=0x1a.0x2 \ +-device ich9-usb-ehci1,id=usb2,bus=pci.2,addr=0x1.0x7 \ +-device ich9-usb-uhci1,masterbus=usb2.0,firstport=0,bus=pci.2,multifunction=on,\ +addr=0x1 \ +-device ich9-usb-uhci2,masterbus=usb2.0,firstport=2,bus=pci.2,addr=0x1.0x1 \ +-device ich9-usb-uhci3,masterbus=usb2.0,firstport=4,bus=pci.2,addr=0x1.0x2 \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0,format=raw \ +-device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ +-vga qxl \ +-global qxl-vga.ram_size=67108864 \ +-global qxl-vga.vram_size=33554432 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-multi.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-multi.xml new file mode 100644 index 0000000..3adb81f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-multi.xml @@ -0,0 +1,47 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='56'/> + </controller> + <controller type='usb' index='0' model='ich9-ehci1'/> + <controller type='usb' index='0' model='ich9-uhci1'/> + <controller type='usb' index='0' model='ich9-uhci2'/> + <controller type='usb' index='0' model='ich9-uhci3'/> + <controller type='usb' index='1' model='ich9-ehci1'/> + <controller type='usb' index='1' model='ich9-uhci1'/> + <controller type='usb' index='1' model='ich9-uhci2'/> + <controller type='usb' index='1' model='ich9-uhci3'/> + <controller type='usb' index='2' model='ich9-ehci1'/> + <controller type='usb' index='2' model='ich9-uhci1'/> + <controller type='usb' index='2' model='ich9-uhci2'/> + <controller type='usb' index='2' model='ich9-uhci3'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-reorder.args b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-reorder.args new file mode 100644 index 0000000..6ce05f4 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-reorder.args @@ -0,0 +1,40 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-kvm \ +-name q35-test \ +-S \ +-M q35 \ +-m 2048 \ +-smp 2 \ +-uuid 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi \ +-boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=56,id=pci.2,bus=pci.1,addr=0x1 \ +-device ich9-usb-ehci1,id=usb,bus=pcie.0,addr=0x1d.0x7 \ +-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pcie.0,multifunction=on,\ +addr=0x1d \ +-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pcie.0,addr=0x1d.0x1 \ +-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pcie.0,addr=0x1d.0x2 \ +-device ich9-usb-ehci1,id=usb1,bus=pcie.0,addr=0x1a.0x7 \ +-device ich9-usb-uhci1,masterbus=usb1.0,firstport=0,bus=pcie.0,multifunction=on,\ +addr=0x1a \ +-device ich9-usb-uhci3,masterbus=usb1.0,firstport=4,bus=pcie.0,addr=0x1a.0x2 \ +-device ich9-usb-uhci2,masterbus=usb1.0,firstport=2,bus=pcie.0,addr=0x1a.0x1 \ +-device ich9-usb-ehci1,id=usb2,bus=pci.2,addr=0x1.0x7 \ +-device ich9-usb-uhci3,masterbus=usb2.0,firstport=4,bus=pci.2,addr=0x1.0x2 \ +-device ich9-usb-uhci2,masterbus=usb2.0,firstport=2,bus=pci.2,addr=0x1.0x1 \ +-device ich9-usb-uhci1,masterbus=usb2.0,firstport=0,bus=pci.2,multifunction=on,\ +addr=0x1 \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0,format=raw \ +-device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ +-vga qxl \ +-global qxl-vga.ram_size=67108864 \ +-global qxl-vga.vram_size=33554432 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-reorder.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-reorder.xml new file mode 100644 index 0000000..f7efdc2 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2-reorder.xml @@ -0,0 +1,47 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='56'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'/> + <controller type='usb' index='0' model='ich9-uhci2'/> + <controller type='usb' index='0' model='ich9-uhci3'/> + <controller type='usb' index='1' model='ich9-uhci1'/> + <controller type='usb' index='2' model='ich9-uhci3'/> + <controller type='usb' index='0' model='ich9-ehci1'/> + <controller type='usb' index='2' model='ich9-ehci1'/> + <controller type='usb' index='1' model='ich9-uhci3'/> + <controller type='usb' index='1' model='ich9-uhci2'/> + <controller type='usb' index='1' model='ich9-ehci1'/> + <controller type='usb' index='2' model='ich9-uhci2'/> + <controller type='usb' index='2' model='ich9-uhci1'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.args b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.args new file mode 100644 index 0000000..705c076 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.args @@ -0,0 +1,30 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-kvm \ +-name q35-test \ +-S \ +-M q35 \ +-m 2048 \ +-smp 2 \ +-uuid 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-no-acpi \ +-boot c \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=56,id=pci.2,bus=pci.1,addr=0x1 \ +-device ich9-usb-ehci1,id=usb,bus=pcie.0,addr=0x1d.0x7 \ +-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pcie.0,multifunction=on,\ +addr=0x1d \ +-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pcie.0,addr=0x1d.0x1 \ +-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pcie.0,addr=0x1d.0x2 \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0,format=raw \ +-device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ +-vga qxl \ +-global qxl-vga.ram_size=67108864 \ +-global qxl-vga.vram_size=33554432 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.xml new file mode 100644 index 0000000..001aece --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-usb2.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='56'/> + </controller> + <controller type='usb' index='0' model='ich9-ehci1'/> + <controller type='usb' index='0' model='ich9-uhci1'/> + <controller type='usb' index='0' model='ich9-uhci2'/> + <controller type='usb' index='0' model='ich9-uhci3'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b245486..5fe52b0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1476,6 +1476,27 @@ mymain(void) QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + DO_TEST("q35-usb2", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + DO_TEST("q35-usb2-multi", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); + DO_TEST("q35-usb2-reorder", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); DO_TEST("pcie-root-port", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2-multi.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2-multi.xml new file mode 100644 index 0000000..5f62468 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2-multi.xml @@ -0,0 +1,66 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='56'/> + </controller> + <controller type='usb' index='0' model='ich9-ehci1'/> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + </controller> + <controller type='usb' index='1' model='ich9-ehci1'/> + <controller type='usb' index='1' model='ich9-uhci1'> + <master startport='0'/> + </controller> + <controller type='usb' index='1' model='ich9-uhci2'> + <master startport='2'/> + </controller> + <controller type='usb' index='1' model='ich9-uhci3'> + <master startport='4'/> + </controller> + <controller type='usb' index='2' model='ich9-ehci1'/> + <controller type='usb' index='2' model='ich9-uhci1'> + <master startport='0'/> + </controller> + <controller type='usb' index='2' model='ich9-uhci2'> + <master startport='2'/> + </controller> + <controller type='usb' index='2' model='ich9-uhci3'> + <master startport='4'/> + </controller> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2-reorder.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2-reorder.xml new file mode 100644 index 0000000..1791b7a --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2-reorder.xml @@ -0,0 +1,66 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='56'/> + </controller> + <controller type='usb' index='0' model='ich9-ehci1'/> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + </controller> + <controller type='usb' index='1' model='ich9-ehci1'/> + <controller type='usb' index='1' model='ich9-uhci1'> + <master startport='0'/> + </controller> + <controller type='usb' index='1' model='ich9-uhci3'> + <master startport='4'/> + </controller> + <controller type='usb' index='1' model='ich9-uhci2'> + <master startport='2'/> + </controller> + <controller type='usb' index='2' model='ich9-ehci1'/> + <controller type='usb' index='2' model='ich9-uhci3'> + <master startport='4'/> + </controller> + <controller type='usb' index='2' model='ich9-uhci2'> + <master startport='2'/> + </controller> + <controller type='usb' index='2' model='ich9-uhci1'> + <master startport='0'/> + </controller> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2.xml new file mode 100644 index 0000000..2dfd9d5 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-usb2.xml @@ -0,0 +1,46 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='56'/> + </controller> + <controller type='usb' index='0' model='ich9-ehci1'/> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + </controller> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index cbd4d0d..5608311 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -561,6 +561,9 @@ mymain(void) DO_TEST_DIFFERENT("pci-autoadd-idx"); DO_TEST_DIFFERENT("pcie-root"); DO_TEST_DIFFERENT("q35"); + DO_TEST_DIFFERENT("q35-usb2"); + DO_TEST_DIFFERENT("q35-usb2-multi"); + DO_TEST_DIFFERENT("q35-usb2-reorder"); DO_TEST("pcie-root-port"); DO_TEST("pcie-root-port-too-many"); DO_TEST("pcie-switch-upstream-port"); -- 2.4.3

On 12/09/2015 08:16 AM, John Ferlan wrote: No. What that parameter *really* means is "this address was explicitly requested", not "it was written by the user in the config". i.e. it wasn't just the first available address of the desired type. No. This cleanup case is a situation where our code is busted - we just checked to see if this particular address was free, so we'd better be able to reserve it. If we can't then something is seriously wrong and the build should never make it out into the wild. Covering up for a failure would not be desirable. I made a patch to fix all other settings of multi to use the enum instead of 0/1, which I'm posting separately. I'm pushing the patch as-is (well, rebased), for the reasons stated above.

On 11/19/2015 01:25 PM, Laine Stump wrote: As I found while working on a different issue - the "MaybeAdd" and "Add" functions were h How about "FindUnusedIndex" or "GetUnusedIndex" ? Something tells me virDomainControllerFind could one day be a bottleneck as each time through we start at 0 (ncontrollers) looking for matching 'type' && 'idx'... I'm thinking of the recent head banging over the network device (?tap*?) lookup algorithm... One would think we don't have that many different controllers to make a difference, but then again did we ever assume the same for the network algorithm? Whether creating a hash tree for each type of controller is worth the effort would seem to be outside the scope of this set of patches, but perhaps something that needs to be considered. That being said - since it seems we're trying to find an available index for a specific type of controller, perhaps it would be better to do something like: idx = 0; for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == type) { if (def->controllers[i]->idx != idx) return idx; idx++; } } return idx; The "else" isn't necessarily necessary ;-) ACK - I would like to see the function name change only to indicate that it's an action Find/Get. Whether you adjust the find algorithm depends on whether you'd like to revisit this code some day... John

This new function will add a single controller of the given model, except the case of ich9-usb-ehci1 (the master controller for a USB2 controller set) in which case a set of related controllers will be added (EHCI1, UHCI1, UHCI2, UHCI3). These controllers will not be given PCI addresses, but should be otherwise ready to use. "-1" is allowed for controller model, and means "default for this machinetype". This matches the existing practice in qemuDomainDefPostParse(), which always adds the default controller with model = -1, and relies on the commandline builder to set a model (that is wrong, but will be fixed later). --- src/conf/domain_conf.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + tests/qemuxml2argvtest.c | 1 + 4 files changed, 52 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ab05e7f..63888b1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14410,6 +14410,54 @@ virDomainDefAddController(virDomainDefPtr def, int type, int idx, int model) } +/* + * virDomainDefAddUSBController() - add a USB controller of the + * specified model. If model is ich9-usb-ehci, also add companion + * uhci1, uhci2, and uhci3 controllers at the same index. Note that a + * model of "-1" is allowed for backward compatibility, and means + * "default USB controller for this machinetype". + */ +int +virDomainDefAddUSBController(virDomainDefPtr def, int idx, int model) +{ + virDomainControllerDefPtr cont; /* this is a *copy* of the DefPtr */ + + cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, + idx, model); + if (!cont) + return -1; + + if (model != VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1) + return 0; + + /* When the initial controller is ich9-usb-ehci, also add the + * companion controllers + */ + + idx = cont->idx; /* in case original request was "-1" */ + + if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, + idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1))) + return -1; + cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB; + cont->info.master.usb.startport = 0; + + if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, + idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2))) + return -1; + cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB; + cont->info.master.usb.startport = 2; + + if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB, + idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3))) + return -1; + cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB; + cont->info.master.usb.startport = 4; + + return 0; +} + + int virDomainDefMaybeAddController(virDomainDefPtr def, int type, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8d43ee6..c34bfd0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3169,6 +3169,8 @@ int virDomainObjListConvert(virDomainObjListPtr domlist, bool skip_missing); int +virDomainDefAddUSBController(virDomainDefPtr def, int idx, int model); +int virDomainDefMaybeAddController(virDomainDefPtr def, int type, int idx, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7e60d87..b7008e0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -200,6 +200,7 @@ virDomainControllerTypeToString; virDomainCpuPlacementModeTypeFromString; virDomainCpuPlacementModeTypeToString; virDomainDefAddImplicitControllers; +virDomainDefAddUSBController; virDomainDefCheckABIStability; virDomainDefCheckDuplicateDiskInfo; virDomainDefCheckUnsupportedMemoryHotplug; diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 5fe52b0..25ffbea 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1474,6 +1474,7 @@ mymain(void) QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); DO_TEST("q35-usb2", -- 2.4.3

On 12/09/2015 09:36 AM, John Ferlan wrote: 'git rebase -i -x "make -j8 check && make -j8 syntax-check" master' is my friend :-) Actually, more like this: /** * functionName: * * @arg1: description * @argn: description * * Function description * * Returns description */ I changed it accordingly. This is only called if there are no other USB controllers in the config, so no that could never happen. No, it should be a part of patch 5. Must have gotten mixed up during a rebase. I moved it.

On 11/19/2015 01:25 PM, Laine Stump wrote: ?machinetype? Perhaps "obvious" that it only changes for one type of machine if one looks. ACK - don't care either way if comment is kept or dropped John