Re: [libvirt] [PATCH 2/5] qemu: prefer 00:1D.x and 00:1A.x for USB2 controllers on Q35

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