[libvirt] [PATCH] Fix logic for assigning PCI addresses to USB2 companion controllers
Hans de Goede
hdegoede at redhat.com
Mon May 14 11:06:50 UTC 2012
Hi,
Thanks for working on this. I've little useful feedback atm
I'm afraid, but I do have a question based on reading the xml
for the tests, is it necessary to set the startports explicitly?
or do they get the correct default based on the model, ie uhci1
startport 0, uhci startport 2, etc?
Regards,
Hans
On 05/14/2012 12:19 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at redhat.com>
>
> Currently each USB2 companion controller gets put on a separate
> PCI slot. Not only is this wasteful of PCI slots, but it is not
> in compliance with the spec for USB2 controllers. The master
> echi1 and all companion controllers should be in the same slot,
> with echi1 in function 7, and uhci1-3 in functions 0-2 respectively.
>
> * src/qemu/qemu_command.c: Special case handling of USB2 controllers
> to apply correct pci slot assignment
> * tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-ehci-addr.args,
> tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-ehci-addr.xml: Expand
> test to cover automatic slot assignment
> ---
> src/qemu/qemu_command.c | 107 ++++++++++++++++----
> .../qemuxml2argv-usb-ich9-ehci-addr.args | 15 ++-
> .../qemuxml2argv-usb-ich9-ehci-addr.xml | 37 ++++++-
> 3 files changed, 138 insertions(+), 21 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 117542f..1ec5a92 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1168,8 +1168,7 @@ void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs)
> }
>
>
> -int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
> - virDomainDeviceInfoPtr dev)
> +static int qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs)
> {
> int i;
> int iteration;
> @@ -1196,20 +1195,10 @@ int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
> continue;
> }
>
> - VIR_DEBUG("Allocating PCI addr %s", addr);
> + VIR_DEBUG("Found free PCI addr %s", addr);
> VIR_FREE(addr);
>
> - if (qemuDomainPCIAddressReserveSlot(addrs, i)< 0)
> - return -1;
> -
> - dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> - dev->addr.pci = maybe.addr.pci;
> -
> - addrs->nextslot = i + 1;
> - if (QEMU_PCI_ADDRESS_LAST_SLOT< addrs->nextslot)
> - addrs->nextslot = 0;
> -
> - return 0;
> + return i;
> }
>
> qemuReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1217,6 +1206,38 @@ int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
> return -1;
> }
>
> +int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
> + virDomainDeviceInfoPtr dev)
> +{
> + int slot = qemuDomainPCIAddressGetNextSlot(addrs);
> +
> + if (slot< 0)
> + return -1;
> +
> + if (qemuDomainPCIAddressReserveSlot(addrs, slot)< 0)
> + return -1;
> +
> + dev->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> + dev->addr.pci.bus = 0;
> + dev->addr.pci.domain = 0;
> + dev->addr.pci.slot = slot;
> + dev->addr.pci.function = 0;
> +
> + addrs->nextslot = slot + 1;
> + if (QEMU_PCI_ADDRESS_LAST_SLOT< addrs->nextslot)
> + addrs->nextslot = 0;
> +
> + return 0;
> +}
> +
> +
> +#define IS_USB2_CONTROLLER(ctrl) \
> + (((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)&& \
> + ((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1 || \
> + (ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1 || \
> + (ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2 || \
> + (ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3))
> +
> /*
> * This assigns static PCI slots to all configured devices.
> * The ordering here is chosen to match the ordering used
> @@ -1252,7 +1273,7 @@ int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
> int
> qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs)
> {
> - int i;
> + size_t i, j;
> bool reservedIDE = false;
> bool reservedUSB = false;
> int function;
> @@ -1396,7 +1417,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs)
> goto error;
> }
>
> - /* Disk controllers (SCSI only for now) */
> + /* Device controllers (SCSI, USB, but not IDE, FDC or CCID) */
> for (i = 0; i< def->ncontrollers ; i++) {
> /* FDC lives behind the ISA bridge; CCID is a usb device */
> if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_FDC ||
> @@ -1413,8 +1434,58 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs)
> continue;
> if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> continue;
> - if (qemuDomainPCIAddressSetNextAddr(addrs,&def->controllers[i]->info)< 0)
> - goto error;
> +
> + /* USB2 needs special handling to put all companions in the same slot */
> + if (IS_USB2_CONTROLLER(def->controllers[i])) {
> + virDomainDevicePCIAddress addr = { 0, 0, 0, 0, false };
> + 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;
> + break;
> + }
> + }
> +
> + switch (def->controllers[i]->model) {
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
> + addr.function = 7;
> + break;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
> + addr.function = 0;
> + addr.multi = VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_ON;
> + break;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
> + addr.function = 1;
> + break;
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
> + addr.function = 2;
> + break;
> + }
> +
> + if (addr.slot == 0) {
> + /* This is the first part of the controller, so need
> + * to find a free slot& then reserve a function */
> + int slot = qemuDomainPCIAddressGetNextSlot(addrs);
> + if (slot< 0)
> + goto error;
> +
> + addr.slot = slot;
> + addrs->nextslot = addr.slot + 1;
> + if (QEMU_PCI_ADDRESS_LAST_SLOT< addrs->nextslot)
> + addrs->nextslot = 0;
> + }
> + /* Finally we can reserve the slot+function */
> + if (qemuDomainPCIAddressReserveFunction(addrs,
> + addr.slot,
> + addr.function)< 0)
> + goto error;
> +
> + def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> + def->controllers[i]->info.addr.pci = addr;
> + } else {
> + if (qemuDomainPCIAddressSetNextAddr(addrs,&def->controllers[i]->info)< 0)
> + goto error;
> + }
> }
>
> /* Disks (VirtIO only for now) */
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-ehci-addr.args b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-ehci-addr.args
> index babd4f8..cf070a1 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-ehci-addr.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-ehci-addr.args
> @@ -2,5 +2,16 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S \
> -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
> -chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
> -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c \
> --device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x4.0x7 \
> --device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
> +-device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x3.0x7 \
> +-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x3 \
> +-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x3.0x2 \
> +-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x3.0x1 \
> +-device ich9-usb-ehci1,id=usb1,bus=pci.0,addr=0x4.0x7 \
> +-device ich9-usb-uhci1,masterbus=usb1.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4 \
> +-device ich9-usb-uhci3,masterbus=usb1.0,firstport=4,bus=pci.0,addr=0x4.0x2 \
> +-device ich9-usb-uhci2,masterbus=usb1.0,firstport=2,bus=pci.0,addr=0x4.0x1 \
> +-device ich9-usb-ehci1,id=usb2,bus=pci.0,addr=0x5.0x7 \
> +-device ich9-usb-uhci1,masterbus=usb2.0,firstport=0,bus=pci.0,multifunction=on,addr=0x5 \
> +-device ich9-usb-uhci3,masterbus=usb2.0,firstport=4,bus=pci.0,addr=0x5.0x2 \
> +-device ich9-usb-uhci2,masterbus=usb2.0,firstport=2,bus=pci.0,addr=0x5.0x1 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-ehci-addr.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-ehci-addr.xml
> index 09633ae..8eff1d7 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-ehci-addr.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-ehci-addr.xml
> @@ -10,8 +10,43 @@
> </os>
> <devices>
> <emulator>/usr/bin/qemu</emulator>
> +<!-- Intentionally mixed up ordering to check we assign
> + addresses to the correct matching companions -->
> <controller type='usb' index='0' model='ich9-ehci1'>
> -<address type='pci' domain='0' bus='0' slot='4' function='7'/>
> +</controller>
> +<controller type='usb' index='2' model='ich9-ehci1'>
> +</controller>
> +<controller type='usb' index='1' model='ich9-ehci1'>
> +</controller>
> +
> +<controller type='usb' index='0' model='ich9-uhci1'>
> +<master startport='0'/>
> +</controller>
> +<controller type='usb' index='1' model='ich9-uhci1'>
> +<master startport='0'/>
> +</controller>
> +<controller type='usb' index='2' model='ich9-uhci1'>
> +<master startport='0'/>
> +</controller>
> +
> +<controller type='usb' index='0' model='ich9-uhci3'>
> +<master startport='4'/>
> +</controller>
> +<controller type='usb' index='1' model='ich9-uhci3'>
> +<master startport='4'/>
> +</controller>
> +<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='1' model='ich9-uhci2'>
> +<master startport='2'/>
> +</controller>
> +<controller type='usb' index='0' model='ich9-uhci2'>
> +<master startport='2'/>
> </controller>
> <memballoon model='virtio'/>
> </devices>
More information about the libvir-list
mailing list