[libvirt] [PATCHv2 1/7] qemu: enable auto-allocate of all PCI addresses

[Laine Stump]

On 08/04/2013 05:01 PM, Doug Goldstein wrote:
> On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine at laine.org> wrote:
>> +        if ((cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 &&
>> +             addr->function == 1) ||
>> +            (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->idx == 0 &&
>> +             (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI ||
>> +              cont->model == -1) && addr->function == 2)) {
>> +            /* Note the check for nbuses > 0 - if there are no PCI
>> +             * buses, we skip this check. This is a quirk required for
>> +             * some machinetypes such as s390, which pretend to have a
>> +             * PCI bus for long enough to generate the "-usb" on the
>> +             * commandline, but that don't really care if a PCI bus
>> +             * actually exists. */
>> +            if (addrs->nbuses > 0 &&
>> +                !(addrs->buses[0].flags & QEMU_PCI_CONNECT_TYPE_PCI)) {
>> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                               _("Bus 0 must be PCI for integrated PIIX3 "
>> +                                 "USB or IDE controllers"));
>> +                return -1;
>> +            } else {
>> +                return 0;
>> +            }
>> +        }
> Still very hacky but at least you improved it by providing a code
> comment as to why and a sensible error message if the user gets in
> this path. I'd still love to see us improve the situation so we didn't
> have to play games like this to get -usb to be emitted for different
> machine types.


Yes. Totally agree.



>>      }
>>
>>      entireSlot = (addr->function == 0 &&
>> @@ -1695,8 +1728,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>>          int nbuses = 0;
>>          size_t i;
>>          int rv;
>> -        qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE |
>> -                                           QEMU_PCI_CONNECT_TYPE_PCI);
>> +        qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPE_PCI;
> So just for my edification, we previously were saying that pci-bridge
> devices were hotpluggable when in fact they are not? You could
> probably add a code comment above the default flags to that effect,
> but not strictly necessary.


You can hot-plug a device into a pci-bridge, but a pci-bridge itself
cannot be hot-plugged into the system. (Maybe a better way to describe
it is that pci-bridge slots can accept hot-plugs, but the pci-bridge
device cannot be hot-plugged. No, that's not any better. Let's see...)


>
>>          for (i = 0; i < def->ncontrollers; i++) {
>>              if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
>> @@ -1941,7 +1973,11 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
>>                                     virDomainDeviceInfoPtr dev)
>>  {
>>      int ret = 0;
>> -    /* FIXME: flags should be set according to the particular device */
>> +    /* Flags should be set according to the particular device,
>> +     * but only the caller knows the type of device. Currently this
>> +     * function is only used for hot-plug, though, and hot-plug is
>> +     * only supported for standard PCI devices, so we can safely use
>> +     * the setting below */
>>      qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE |
>>                                         QEMU_PCI_CONNECT_TYPE_PCI);
>>
>> @@ -2005,7 +2041,13 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
>>                                  virDevicePCIAddressPtr next_addr,
>>                                  qemuDomainPCIConnectFlags flags)
>>  {
>> -    virDevicePCIAddress a = addrs->lastaddr;
>> +    virDevicePCIAddress a = { 0, 0, 0, 0, false };
> Again, me being nitpicky but maybe a comment that says start the
> search from the top of the PCI addresses by settings this to all 0s
> and then below the comment says that this is a fast out when the flags
> match.

Is this better?


    /* default to starting the search for a free slot from
     * 0000:00:00.0
     */
    virDevicePCIAddress a = { 0, 0, 0, 0, false };

    /* except if this search is for the exact same type of device as
     * last time, continue the search from the previous match
     */
    if (flags == addrs->lastFlags)
        a = addrs->lastaddr;

I squashed that in.