[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine laine org> wrote:
> Previous refactoring of the guest PCI address reservation/allocation
> code allowed for slot types other than basic PCI (e.g. PCI express,
> non-hotpluggable slots, etc) but would not auto-allocate a slot for a
> device that required any type other than a basic hot-pluggable
> PCI slot.
>
> This patch refactors the code to be aware of different slot types
> during auto-allocation of addresses as well - as long as there is an
> empty slot of the required type, it will be found and used.
>
> The piece that *wasn't* added is that we don't auto-create a new PCI
> bus when needed for anything except basic PCI devices. This is because
> there are multiple different types of controllers that can provide,
> for example, a PCI express slot (in addition to the pcie-root
> controller, these can also be found on a "root-port" or on a
> "downstream-switch-port"). Since we currently don't support any PCIe
> devices (except pending support for dmi-to-pci-bridge), we can defer
> any decision on what to do about this.
> ---
>  src/qemu/qemu_command.c | 112 ++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 90 insertions(+), 22 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index abc973a..cafc4bf 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1429,6 +1429,7 @@ struct _qemuDomainPCIAddressSet {
>      qemuDomainPCIAddressBus *buses;
>      size_t nbuses;
>      virDevicePCIAddress lastaddr;
> +    qemuDomainPCIConnectFlags lastFlags;
>      bool dryRun;          /* on a dry run, new buses are auto-added
>                               and addresses aren't saved in device infos */
>  };
> @@ -1630,7 +1631,7 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
>      int ret = -1;
>      virDevicePCIAddressPtr addr = &info->addr.pci;
>      bool entireSlot;
> -    /* FIXME: flags should be set according to the requirements of @device */
> +    /* flags may be changed from default below */
>      qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE |
>                                         QEMU_PCI_CONNECT_TYPE_PCI);
>
> @@ -1644,28 +1645,60 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
>          return 0;
>      }
>
> +    /* Change flags according to differing requirements of different
> +     * devices.
> +     */
> +    if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER &&
> +        device->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
> +        switch (device->data.controller->model) {
> +        case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +            /* pci-bridge needs a PCI slot, but it isn't
> +             * hot-pluggable, so it doesn't need a hot-pluggable slot.
> +             */
> +            flags = QEMU_PCI_CONNECT_TYPE_PCI;
> +            break;
> +        default:
> +            break;
> +        }
> +    }
> +
>      /* Ignore implicit controllers on slot 0:0:1.0:
>       * implicit IDE controller on 0:0:1.1 (no qemu command line)
>       * implicit USB controller on 0:0:1.2 (-usb)
>       *
>       * If the machine does have a PCI bus, they will get reserved
>       * in qemuAssignDevicePCISlots().
> -     *
> -     * FIXME: When we have support for a pcie-root controller at bus
> -     * 0, we will no longer be able to skip checking of these devices,
> -     * as they are PCI, and thus can't be connected to bus 0 if it is
> -     * PCIe rather than PCI.
> +     */
> +
> +    /* These are the IDE and USB controllers in the PIIX3, hardcoded
> +     * to bus 0 slot 1.  They cannot be attached to a PCIe slot, only
> +     * PCI.
>       */
>      if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER && addr->domain == 0 &&
>          addr->bus == 0 && addr->slot == 1) {
>          virDomainControllerDefPtr cont = device->data.controller;
> -        if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0 &&
> -            addr->function == 1)
> -            return 0;
> -        if (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)
> -            return 0;
> +
> +        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.

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

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

> +
> +    /* Avoid re-scanning from start if we're searching for exactly the
> +     * same type of slot as last time.
> +     */
> +    if (flags == addrs->lastFlags)
> +        a = addrs->lastaddr;
>
>      if (addrs->nbuses == 0) {
>          virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available"));
> @@ -2014,6 +2056,12 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
>
>      /* Start the search at the last used bus and slot */
>      for (a.slot++; a.bus < addrs->nbuses; a.bus++) {
> +        if (!qemuDomainPCIAddressFlagsCompatible(&a, addrs->buses[a.bus].flags,
> +                                                 flags, false)) {
> +            VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device",
> +                      a.domain, a.bus);
> +            continue;
> +        }
>          for (; a.slot <= QEMU_PCI_ADDRESS_SLOT_LAST; a.slot++) {
>              if (!qemuDomainPCIAddressSlotInUse(addrs, &a))
>                  goto success;
> @@ -2030,9 +2078,15 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
>          if (qemuDomainPCIAddressSetGrow(addrs, &a, flags) < 0)
>              return -1;
>          goto success;
> -    } else {
> +    } else if (flags == addrs->lastFlags) {
>          /* Check the buses from 0 up to the last used one */
>          for (a.bus = 0; a.bus <= addrs->lastaddr.bus; a.bus++) {
> +            if (!qemuDomainPCIAddressFlagsCompatible(&a, addrs->buses[a.bus].flags,
> +                                                     flags, false)) {
> +                VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device",
> +                          a.domain, a.bus);
> +                continue;
> +            }
>              for (a.slot = 1; a.slot <= QEMU_PCI_ADDRESS_SLOT_LAST; a.slot++) {
>                  if (!qemuDomainPCIAddressSlotInUse(addrs, &a))
>                      goto success;
> @@ -2072,6 +2126,7 @@ qemuDomainPCIAddressReserveNextSlot(qemuDomainPCIAddressSetPtr addrs,
>      }
>
>      addrs->lastaddr = addr;
> +    addrs->lastFlags = flags;
>      return 0;
>  }
>
> @@ -2285,15 +2340,26 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
>          goto error;
>      }
>
> -    flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI;
> -
>      /* PCI controllers */
>      for (i = 0; i < def->ncontrollers; i++) {
>          if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
> -            if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
> -                continue;
>              if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>                  continue;
> +            switch (def->controllers[i]->model) {
> +            case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +                /* pci-root is implicit in the machine,
> +                 * and needs no address */
> +                continue;
> +            case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +                /* pci-bridge doesn't require hot-plug
> +                 * (although it does provide hot-plug in its slots)
> +                 */
> +                flags = QEMU_PCI_CONNECT_TYPE_PCI;
> +                break;
> +            default:
> +                flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI;
> +                break;
> +            }
>              if (qemuDomainPCIAddressReserveNextSlot(addrs,
>                                                      &def->controllers[i]->info,
>                                                      flags) < 0)
> @@ -2301,6 +2367,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
>          }
>      }
>
> +    flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI;
> +
>      for (i = 0; i < def->nfss; i++) {
>          if (def->fss[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>              continue;
> --
> 1.7.11.7
>
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Overall looks good and I learned a bit about parts of libvirt I didn't
know much about. I had a few comment nitpicks but otherwise ACK from
me here.

-- 
Doug Goldstein


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]