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

[Doug Goldstein]

On Mon, Aug 5, 2013 at 1:33 AM, Laine Stump <laine at laine.org> wrote:
> 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...)

No that makes sense.

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

ACK this patch. Thanks for the updates.

-- 
Doug Goldstein