On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@xxxxxxxxx> 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@xxxxxxxxxx
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list