On 11/19/2015 01:24 PM, Laine Stump wrote:

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 will make more sense once we hit patch 2. Just below here where multi = 0, can we take the opportunity to either update it now (I'm fine with just changing in this patch as long as it's attributed in the commit message). Or perhaps we add a patch before this one that just changes to use the "right" TRISTATE value. ACK what's here (since it seems reasonable to me). I can trust you know how to add the appropriate TRISTATE patch too ;-) John

/* 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)

On 11/19/2015 01:24 PM, Laine Stump wrote:

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)

Should param5 be 'false' since we're running from qemu_command and not fromConfig ?

+ goto cleanup;

So is this 'cleanup' case a condition where perhaps the bus was full? IOW: Do we want to fail or just let code that would handle the case where assign = false would then (I assume) later on assign an address on some available slot? So the result would be if (virDomainPCIAddressReserveAddr(...) == 0) { def->controllers ... } ?

+ 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;

Not for 'this' patch per se, but there's 3 other places in qemu_command.c that fill in multi with 0 or 1 that probably should use the TRISTATE value... One I noted from patch 1, but how about a patch 1.5 or 0.5 that changes all the existing multi settings to use the appropriate TRISTATE value. I'm assuming setting to 0 or 1 is correct where they are, but since you understand the topology better I figured we could use the opportunity to make sure that assumption is true!

+ } + } + 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; }

See and all these use the correct value (which is fine by me). Conditional ACK based on usage of virDomainPCIAddressReserveAddr param5 and what I believe should be only checking the success scenario and allowing whatever processing would happen to fill in the address in the event of failure... John

@@ -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");

On 12/09/2015 09:01 AM, John Ferlan wrote:

On 11/19/2015 01:25 PM, Laine Stump wrote:

...

We need a virDomainDefAddController() that doesn't check for an existing controller at the same index (since USB2 controllers must be added in sets of 4 that are all at the same index), so rather than duplicating the code in virDomainDefMaybeAddController(), split it into two functions, in the process eliminating existing duplicated code that loops through the controller list by calling virDomainControllerFind(), which does the same thing). As I found while working on a different issue - the "MaybeAdd" and "Add" functions were h

You left a thought half-fin :-)

...

--- src/conf/domain_conf.c | 56 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 16 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0ac7dbf..ab05e7f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13456,6 +13456,18 @@ virDomainControllerFind(virDomainDefPtr def, }

+static int +virDomainControllerUnusedIndex(virDomainDefPtr def, int type) How about "FindUnusedIndex" or "GetUnusedIndex" ?

Okay. Done.

...

+{ + int idx = 0; + + while (virDomainControllerFind(def, type, idx) >= 0) + idx++; + 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...

The difference is that for the network device we were doing an entire netlink message/response cycle for each index that we tried, which was *very* slow. So we made a data structure in memory that we could more easily scan. In this case we are already working with data in memory that we can easily scan, so I think there would be very little to gain. (note that in the case of tap devices, the kernel will already automatically create a unique name for us, so we don't need to try and guess as we do for macvtap devices).

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?

No, I think the people who added macvtap were just trying to make something that worked and figured they'd come back and optimize it later, but then forgot the 2nd half.

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.

Since everything is just looking through the entries of an array in memory comparing integers, I don't see the problem with leaving it like this even for a few hundred controllers, and I think we will have fallen to the machine overlords before we have to deal with a single virtual machine with more controllers than that.

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++; }

You're assuming that the items in the list are in order of index. Although there is a function that does that when inserting new elements (virDomainControllerInsertPreAlloced()) I don't have complete confidence that it is always used to add new controllers. And besides I think the counts are low enough that a simple function will do fine (and be easier to understand - it took me a few minutes to parse yours and understand its assumptions). (oh, as a matter of fact, virDomainControllerInsertPreAlloced() *isn't* always called - virDomainDefMaybeAddController() didn't use it, and after this patch virDomainControllerDefAddController() doesn't (because it just used the existing code in the other function).

} return idx;

...

+ return idx; +} + + const char * virDomainControllerAliasFind(virDomainDefPtr def, int type, int idx) @@ -14375,33 +14387,45 @@ virDomainEmulatorPinDefParseXML(xmlNodePtr node) }

-int -virDomainDefMaybeAddController(virDomainDefPtr def, - int type, - int idx, - int model) +static virDomainControllerDefPtr +virDomainDefAddController(virDomainDefPtr def, int type, int idx, int model) { - size_t i; virDomainControllerDefPtr cont;

- for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type == type && - def->controllers[i]->idx == idx) - return 0; - } - if (!(cont = virDomainControllerDefNew(type))) - return -1; + return NULL; + + if (idx < 0) + idx = virDomainControllerUnusedIndex(def, type);

cont->idx = idx; cont->model = model;

- if (VIR_APPEND_ELEMENT(def->controllers, def->ncontrollers, cont) < 0) { + if (VIR_APPEND_ELEMENT_COPY(def->controllers, def->ncontrollers, cont) < 0) { VIR_FREE(cont); - return -1; + return NULL; }

- return 1; + return cont; +} + + +int +virDomainDefMaybeAddController(virDomainDefPtr def, + int type, + int idx, + int model) +{ + /* skip if a specific index was given and it is already + * in use for that type of controller + */ + if (idx >= 0 && virDomainControllerFind(def, type, idx) >= 0) + return 0; + + if (virDomainDefAddController(def, type, idx, model)) + return 1; + else + return -1; The "else" isn't necessarily necessary ;-)

Yeah. I'll fix that.

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...

Okay. I changed the name and eliminated the extra superfluous else.

On 11/19/2015 01:25 PM, Laine Stump wrote:

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(+)

Wasn't able to "git am -3" this patch - so I assume you have some merge conflicts coming... Safe to assume from your side that I wasn't able to compile this - so I'll further assume this and the next patch will have gone through check/check-syntax processing

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". + */

Nice! comments, but the format I've seen lately has been: /* functionName * @arg1: description * @argn: description * * Function description * * Returns description */

+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" */ +

And the operating assumption being that none of the following exist? IOW: Would it be possible for someone to add these, but not the above? Not that anyone (qa) would do that...

+ 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,

Should this perhaps have been merged into patch 2? Just seems out of place here. ACK - seems reasonable to me. John

QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); DO_TEST("q35-usb2",