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

Laine Stump laine at laine.org
Mon Jan 11 18:29:12 UTC 2016


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.




More information about the libvir-list mailing list