On 12/09/2015 08:16 AM, John Ferlan wrote:
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 ?
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.
+ goto cleanup;
So is this 'cleanup' case a condition where perhaps the bus was full?
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.
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!
I made a patch to fix all other settings of multi to use the enum
instead of 0/1, which I'm posting separately.
+ }
+ }
+ 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...
I'm pushing the patch as-is (well, rebased), for the reasons stated above.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list