[libvirt] [PATCHv2 0/7] Q35 support part 2 - RESEND with a few small changes
This obsoletes patches 3/7-10/7 of the previous posting of Q35 part 2. The first 3 patches had been ACKed so I pushed those, so patch 1/7 of this series was 3/7 of the v1. Most of the changes to patches 1-4 are small nits that I neglected to keep track of (e.g. fixing make check when run as each patch is applied, and changing the commandline args slightly as I figured out issues once I could actually start up and run a domain. The issue with pci.0 vs. pcie.0 has been fixed in this new series. As indicated in my latest response to the previous series, with the 3 new additional patches everything seems to work. A default USB controller is still not added for Q35 machines domains. domains that are created now will continue to be compatible once we decide what type of default USB controller to add though (if we do decide to do that). At this point I am declaring the Q35 support in libvirt "usable". Until some tweaks are made to virt-manager, it won't be possible to create a new q35 domain with virt-manager though. Laine Stump (7): qemu: enable auto-allocate of all PCI addresses qemu: add pcie-root controller qemu: add dmi-to-pci-bridge controller qemu: fix handling of default/implicit devices for q35 qemu: properly set/use device alias for pci controllers qemu: use standard pci-bridge in place of i82801b11-bridge qemu: enable using implicit sata controller in q35 machines docs/formatdomain.html.in | 43 +- docs/schemas/domaincommon.rng | 2 + src/conf/domain_conf.c | 11 +- src/conf/domain_conf.h | 2 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 517 +++++++++++++++++---- src/qemu/qemu_command.h | 32 +- src/qemu/qemu_domain.c | 36 +- src/qemu/qemu_hotplug.c | 6 +- tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 5 + tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml | 20 + tests/qemuxml2argvdata/qemuxml2argv-q35.args | 8 + tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 30 ++ tests/qemuxml2argvtest.c | 10 + .../qemuxml2xmlout-pcie-root.xml | 23 + tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 31 ++ tests/qemuxml2xmltest.c | 2 + 18 files changed, 675 insertions(+), 106 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml -- 1.7.11.7
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; + } + } } 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; 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 }; + + /* 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
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
On 08/04/2013 05:01 PM, Doug Goldstein 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
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote: 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.
On Mon, Aug 5, 2013 at 1:33 AM, Laine Stump <laine@laine.org> wrote:
On 08/04/2013 05:01 PM, Doug Goldstein 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
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote: 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
This controller is implicit on q35 machinetypes. It provides 31 PCIe (*not* PCI) slots as controller 0. Currently there are no devices that can connect to pcie-root, and no implicit pci controller on a q35 machine, so q35 is still unusable. For a usable q35 system, we need to add a "dmi-to-pci-bridge" pci controller, which can connect to pcie-root, and provides standard pci slots that can be used to connect other devices. --- docs/formatdomain.html.in | 27 ++++++++++++++++------ docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 8 ++++--- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 23 ++++++++++++++---- src/qemu/qemu_command.h | 4 +++- src/qemu/qemu_domain.c | 19 +++++++++++---- tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 4 ++++ tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml | 21 +++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ tests/qemuxml2xmltest.c | 1 + 11 files changed, 92 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 49c7c8d..330dca2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2338,13 +2338,11 @@ <p> PCI controllers have an optional <code>model</code> attribute with - possible values <code>pci-root</code> or <code>pci-bridge</code>. - For machine types which provide an implicit pci bus, the pci-root + possible values <code>pci-root</code>, <code>pcie-root</code> + or <code>pci-bridge</code>. + For machine types which provide an implicit PCI bus, the pci-root controller with index=0 is auto-added and required to use PCI devices. - PCI root has no address. - PCI bridges are auto-added if there are too many devices to fit on - the one bus provided by pci-root, or a PCI bus number greater than zero - was specified. + pci-root has no address. PCI bridges can also be specified manually, but their addresses should only refer to PCI buses provided by already specified PCI controllers. Leaving gaps in the PCI controller indexes might lead to an invalid @@ -2356,11 +2354,26 @@ <devices> <controller type='pci' index='0' model='pci-root'/> <controller type='pci' index='1' model='pci-bridge'> - <address type='pci' domain='0' bus='0' slot='5' function='0' multifunction=off'/> + <address type='pci' domain='0' bus='0' slot='5' function='0' multifunction='off'/> </controller> </devices> ...</pre> + <p> + For machine types which provide an implicit PCI Express (PCIe) + bus (for example, the machine types based on the Q35 chipset), + the pcie-root controller with index=0 is auto-added to the + domain's configuration. pcie-root has also no address, provides + 31 slots (numbered 1-31) and can only be used to attach PCIe + devices. (<span class="since">since 1.1.2</span>). + </p> +<pre> + ... + <devices> + <controller type='pci' index='0' model='pcie-root'/> + </devices> + ...</pre> + <h4><a name="elementsLease">Device leases</a></h4> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 745b959..e04be12 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1538,6 +1538,7 @@ <attribute name="model"> <choice> <value>pci-root</value> + <value>pcie-root</value> <value>pci-bridge</value> </choice> </attribute> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 63350b6..59a96f2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -310,6 +310,7 @@ VIR_ENUM_IMPL(virDomainController, VIR_DOMAIN_CONTROLLER_TYPE_LAST, VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pci-root", + "pcie-root", "pci-bridge") VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, @@ -5714,16 +5715,17 @@ virDomainControllerDefParseXML(xmlNodePtr node, case VIR_DOMAIN_CONTROLLER_TYPE_PCI: switch (def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("pci-root controller should not " + _("pci-root and pcie-root controllers should not " "have an address")); goto error; } if (def->idx != 0) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("pci-root controller should have " - "index 0")); + _("pci-root and pcie-root controllers " + "should have index 0")); goto error; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index de3b59c..63a1444 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -768,6 +768,7 @@ enum virDomainControllerType { typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cafc4bf..b6912ce 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1453,6 +1453,12 @@ qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr, "device. Device requires a standard PCI slot, " "which is not provided by this bus"), addr->domain, addr->bus); + } else if (devFlags & QEMU_PCI_CONNECT_TYPE_PCIE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("PCI bus %.4x:%.2x is not compatible with the " + "device. Device requires a PCI Express slot, " + "which is not provided by this bus"), + addr->domain, addr->bus); } else { /* this should never happen. If it does, there is a * bug in the code that sets the flag bits for devices. @@ -1549,6 +1555,12 @@ qemuDomainPCIAddressBusSetModel(qemuDomainPCIAddressBusPtr bus, bus->minSlot = 1; bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* slots 1 - 31, PCIe devices only, no hotplug */ + bus->flags = QEMU_PCI_CONNECT_TYPE_PCIE; + bus->minSlot = 1; + bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PCI controller model %d"), model); @@ -2347,7 +2359,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, continue; switch (def->controllers[i]->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - /* pci-root is implicit in the machine, + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* pci-root and pcie-root are implicit in the machine, * and needs no address */ continue; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: @@ -4336,8 +4349,9 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, def->idx, def->idx); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("wrong function called for pci-root")); + _("wrong function called for pci-root/pcie-root")); return NULL; } break; @@ -7615,9 +7629,10 @@ qemuBuildCommandLine(virConnectPtr conn, continue; } - /* Skip pci-root */ + /* Skip pci-root/pcie-root */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) { + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)) { continue; } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index bf4953a..e5111d2 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -233,13 +233,15 @@ typedef enum { QEMU_PCI_CONNECT_TYPE_PCI = 1 << 2, /* PCI devices can connect to this bus */ + QEMU_PCI_CONNECT_TYPE_PCIE = 1 << 3, + /* PCI Express devices can connect to this bus */ } qemuDomainPCIConnectFlags; /* a combination of all bit that describe the type of connections * allowed, e.g. PCI, PCIe, switch */ # define QEMU_PCI_CONNECT_TYPES_MASK \ - QEMU_PCI_CONNECT_TYPE_PCI + (QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE) int qemuDomainAssignPCIAddresses(virDomainDefPtr def, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d3da666..4d04609 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -701,6 +701,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, { bool addDefaultUSB = true; bool addPCIRoot = false; + bool addPCIeRoot = false; /* check for emulator and create a default one if needed */ if (!def->emulator && @@ -713,12 +714,16 @@ qemuDomainDefPostParse(virDomainDefPtr def, case VIR_ARCH_X86_64: if (!def->os.machine) break; - if (STRPREFIX(def->os.machine, "pc-q35") || - STREQ(def->os.machine, "q35") || - STREQ(def->os.machine, "isapc")) { + if (STREQ(def->os.machine, "isapc")) { addDefaultUSB = false; break; } + if (STRPREFIX(def->os.machine, "pc-q35") || + STREQ(def->os.machine, "q35")) { + addPCIeRoot = true; + addDefaultUSB = false; + break; + } if (!STRPREFIX(def->os.machine, "pc-0.") && !STRPREFIX(def->os.machine, "pc-1.") && !STRPREFIX(def->os.machine, "pc-i440") && @@ -755,6 +760,12 @@ qemuDomainDefPostParse(virDomainDefPtr def, VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) return -1; + if (addPCIeRoot && + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0) + return -1; + return 0; } @@ -1421,7 +1432,7 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver, if (pci && pci->idx == 0 && pci->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) { - VIR_DEBUG("Removing default 'pci-root' from domain '%s'" + VIR_DEBUG("Removing default pci-root from domain '%s'" " for migration compatibility", def->name); toremove++; } else { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args new file mode 100644 index 0000000..e937189 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ +-S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.1,addr=0x1 -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml new file mode 100644 index 0000000..1aa5455 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='usb' index='0'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b7485fc..57c6989 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -994,6 +994,8 @@ mymain(void) DO_TEST("pci-autoadd-idx", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pci-bridge-many-disks", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE_PCI_BRIDGE); + DO_TEST("pcie-root", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 66be40e..ea511b8 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -294,6 +294,7 @@ mymain(void) DO_TEST_DIFFERENT("pci-bridge-many-disks"); DO_TEST_DIFFERENT("pci-autoadd-addr"); DO_TEST_DIFFERENT("pci-autoadd-idx"); + DO_TEST("pcie-root"); DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 1.7.11.7
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote:
This controller is implicit on q35 machinetypes. It provides 31 PCIe (*not* PCI) slots as controller 0.
Currently there are no devices that can connect to pcie-root, and no implicit pci controller on a q35 machine, so q35 is still unusable. For a usable q35 system, we need to add a "dmi-to-pci-bridge" pci controller, which can connect to pcie-root, and provides standard pci slots that can be used to connect other devices. --- docs/formatdomain.html.in | 27 ++++++++++++++++------ docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 8 ++++--- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 23 ++++++++++++++---- src/qemu/qemu_command.h | 4 +++- src/qemu/qemu_domain.c | 19 +++++++++++---- tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 4 ++++ tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml | 21 +++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ tests/qemuxml2xmltest.c | 1 + 11 files changed, 92 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 49c7c8d..330dca2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2338,13 +2338,11 @@
<p> PCI controllers have an optional <code>model</code> attribute with - possible values <code>pci-root</code> or <code>pci-bridge</code>. - For machine types which provide an implicit pci bus, the pci-root + possible values <code>pci-root</code>, <code>pcie-root</code> + or <code>pci-bridge</code>. + For machine types which provide an implicit PCI bus, the pci-root controller with index=0 is auto-added and required to use PCI devices. - PCI root has no address. - PCI bridges are auto-added if there are too many devices to fit on - the one bus provided by pci-root, or a PCI bus number greater than zero - was specified. + pci-root has no address. PCI bridges can also be specified manually, but their addresses should only refer to PCI buses provided by already specified PCI controllers. Leaving gaps in the PCI controller indexes might lead to an invalid @@ -2356,11 +2354,26 @@ <devices> <controller type='pci' index='0' model='pci-root'/> <controller type='pci' index='1' model='pci-bridge'> - <address type='pci' domain='0' bus='0' slot='5' function='0' multifunction=off'/> + <address type='pci' domain='0' bus='0' slot='5' function='0' multifunction='off'/> </controller> </devices> ...</pre>
Add the missing update to the since section from patch 3/7.
+ <p> + For machine types which provide an implicit PCI Express (PCIe) + bus (for example, the machine types based on the Q35 chipset), + the pcie-root controller with index=0 is auto-added to the + domain's configuration. pcie-root has also no address, provides + 31 slots (numbered 1-31) and can only be used to attach PCIe + devices. (<span class="since">since 1.1.2</span>). + </p> +<pre> + ... + <devices> + <controller type='pci' index='0' model='pcie-root'/> + </devices> + ...</pre> + <h4><a name="elementsLease">Device leases</a></h4>
<p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 745b959..e04be12 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1538,6 +1538,7 @@ <attribute name="model"> <choice> <value>pci-root</value> + <value>pcie-root</value> <value>pci-bridge</value> </choice> </attribute> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 63350b6..59a96f2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -310,6 +310,7 @@ VIR_ENUM_IMPL(virDomainController, VIR_DOMAIN_CONTROLLER_TYPE_LAST,
VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pci-root", + "pcie-root", "pci-bridge")
VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, @@ -5714,16 +5715,17 @@ virDomainControllerDefParseXML(xmlNodePtr node, case VIR_DOMAIN_CONTROLLER_TYPE_PCI: switch (def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("pci-root controller should not " + _("pci-root and pcie-root controllers should not " "have an address")); goto error; } if (def->idx != 0) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("pci-root controller should have " - "index 0")); + _("pci-root and pcie-root controllers " + "should have index 0")); goto error; }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index de3b59c..63a1444 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -768,6 +768,7 @@ enum virDomainControllerType {
typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE,
VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cafc4bf..b6912ce 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1453,6 +1453,12 @@ qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr, "device. Device requires a standard PCI slot, " "which is not provided by this bus"), addr->domain, addr->bus); + } else if (devFlags & QEMU_PCI_CONNECT_TYPE_PCIE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("PCI bus %.4x:%.2x is not compatible with the " + "device. Device requires a PCI Express slot, " + "which is not provided by this bus"), + addr->domain, addr->bus); } else { /* this should never happen. If it does, there is a * bug in the code that sets the flag bits for devices. @@ -1549,6 +1555,12 @@ qemuDomainPCIAddressBusSetModel(qemuDomainPCIAddressBusPtr bus, bus->minSlot = 1; bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* slots 1 - 31, PCIe devices only, no hotplug */ + bus->flags = QEMU_PCI_CONNECT_TYPE_PCIE; + bus->minSlot = 1; + bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PCI controller model %d"), model); @@ -2347,7 +2359,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, continue; switch (def->controllers[i]->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: - /* pci-root is implicit in the machine, + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + /* pci-root and pcie-root are implicit in the machine, * and needs no address */ continue; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: @@ -4336,8 +4349,9 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, def->idx, def->idx); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("wrong function called for pci-root")); + _("wrong function called for pci-root/pcie-root")); return NULL; } break; @@ -7615,9 +7629,10 @@ qemuBuildCommandLine(virConnectPtr conn, continue; }
- /* Skip pci-root */ + /* Skip pci-root/pcie-root */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) { + (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)) { continue; }
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index bf4953a..e5111d2 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -233,13 +233,15 @@ typedef enum {
QEMU_PCI_CONNECT_TYPE_PCI = 1 << 2, /* PCI devices can connect to this bus */ + QEMU_PCI_CONNECT_TYPE_PCIE = 1 << 3, + /* PCI Express devices can connect to this bus */ } qemuDomainPCIConnectFlags;
/* a combination of all bit that describe the type of connections * allowed, e.g. PCI, PCIe, switch */ # define QEMU_PCI_CONNECT_TYPES_MASK \ - QEMU_PCI_CONNECT_TYPE_PCI + (QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE)
int qemuDomainAssignPCIAddresses(virDomainDefPtr def, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d3da666..4d04609 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -701,6 +701,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, { bool addDefaultUSB = true; bool addPCIRoot = false; + bool addPCIeRoot = false;
/* check for emulator and create a default one if needed */ if (!def->emulator && @@ -713,12 +714,16 @@ qemuDomainDefPostParse(virDomainDefPtr def, case VIR_ARCH_X86_64: if (!def->os.machine) break; - if (STRPREFIX(def->os.machine, "pc-q35") || - STREQ(def->os.machine, "q35") || - STREQ(def->os.machine, "isapc")) { + if (STREQ(def->os.machine, "isapc")) { addDefaultUSB = false; break; } + if (STRPREFIX(def->os.machine, "pc-q35") || + STREQ(def->os.machine, "q35")) { + addPCIeRoot = true; + addDefaultUSB = false; + break; + } if (!STRPREFIX(def->os.machine, "pc-0.") && !STRPREFIX(def->os.machine, "pc-1.") && !STRPREFIX(def->os.machine, "pc-i440") && @@ -755,6 +760,12 @@ qemuDomainDefPostParse(virDomainDefPtr def, VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) return -1;
+ if (addPCIeRoot && + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0) + return -1; + return 0; }
@@ -1421,7 +1432,7 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
if (pci && pci->idx == 0 && pci->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) { - VIR_DEBUG("Removing default 'pci-root' from domain '%s'" + VIR_DEBUG("Removing default pci-root from domain '%s'" " for migration compatibility", def->name); toremove++; } else { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args new file mode 100644 index 0000000..e937189 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ +-S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.1,addr=0x1 -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml new file mode 100644 index 0000000..1aa5455 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='usb' index='0'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b7485fc..57c6989 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -994,6 +994,8 @@ mymain(void) DO_TEST("pci-autoadd-idx", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pci-bridge-many-disks", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE_PCI_BRIDGE); + DO_TEST("pcie-root", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE);
DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 66be40e..ea511b8 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -294,6 +294,7 @@ mymain(void) DO_TEST_DIFFERENT("pci-bridge-many-disks"); DO_TEST_DIFFERENT("pci-autoadd-addr"); DO_TEST_DIFFERENT("pci-autoadd-idx"); + DO_TEST("pcie-root");
DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 1.7.11.7
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
ACK. Seems to just straight forward add pcie-root along side pci-root. I'd just move the fix to the since section to this patch from 3/7 that I noted above. -- Doug Goldstein
This PCI controller, named "dmi-to-pci-bridge" in the libvirt config, and implemented with qemu's "i82801b11-bridge" device, connects to a PCI Express slot (e.g. one of the slots provided by the pcie-root controller, aka "pcie.0" on the qemu commandline), and provides 31 *non-hot-pluggable* PCI (*not* PCIe) slots, numbered 1-31. Any time a machine is defined which has a pcie-root controller (i.e. any q35-based machinetype), libvirt will automatically add a dmi-to-pci-bridge controller if one doesn't exist, and also add a pci-bridge controller. The reasoning here is that any useful domain will have either an immediate (startup time) or eventual (subsequent hot-plug) need for a standard PCI slot; since the pcie-root controller only provides PCIe slots, we need to connect a dmi-to-pci-bridge controller to it in order to get a non-hot-plug PCI slot that we can then use to connect a pci-bridge - the slots provided by the pci-bridge will be both standard PCI and hot-pluggable. Since pci-bridge devices themselves are not hot-pluggable, any new pci-bridge controller that is added can (and will) be plugged into the dmi-to-pci-bridge as long as it has empty slots available. --- docs/formatdomain.html.in | 28 +++++++++++++++--- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 3 +- src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 33 ++++++++++++++++++++++ src/qemu/qemu_domain.c | 14 +++++++-- tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 3 +- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 7 +++++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 25 ++++++++++++++++ tests/qemuxml2argvtest.c | 8 +++++- .../qemuxml2xmlout-pcie-root.xml | 23 +++++++++++++++ tests/qemuxml2xmltest.c | 3 +- 14 files changed, 142 insertions(+), 10 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 330dca2..8fa4c0e 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2338,16 +2338,19 @@ <p> PCI controllers have an optional <code>model</code> attribute with - possible values <code>pci-root</code>, <code>pcie-root</code> - or <code>pci-bridge</code>. + possible values <code>pci-root</code>, <code>pcie-root</code>, + <code>pci-bridge</code>, or <code>dmi-to-pci-bridge</code>. For machine types which provide an implicit PCI bus, the pci-root controller with index=0 is auto-added and required to use PCI devices. pci-root has no address. + PCI bridges are auto-added if there are too many devices to fit on + the one bus provided by pci-root, or a PCI bus number greater than zero + was specified. PCI bridges can also be specified manually, but their addresses should only refer to PCI buses provided by already specified PCI controllers. Leaving gaps in the PCI controller indexes might lead to an invalid configuration. - (<span class="since">since 1.0.5</span>) + (pci-root and pci-bridge <span class="since">since 1.0.5</span>). </p> <pre> ... @@ -2365,12 +2368,29 @@ the pcie-root controller with index=0 is auto-added to the domain's configuration. pcie-root has also no address, provides 31 slots (numbered 1-31) and can only be used to attach PCIe - devices. (<span class="since">since 1.1.2</span>). + devices. In order to connect standard PCI devices on a system + which has a pcie-root controller, a pci controller + with <code>model='dmi-to-pci-bridge'</code> is automatically + added. A dmi-to-pci-bridge controller plugs into a PCIe slot (as + provided by pcie-root), and itself provides 31 standard PCI + slots (which are not hot-pluggable). In order to have + hot-pluggable PCI slots in the guest system, a pci-bridge + controller will also be automatically created and connected to + one of the slots of the auto-created dmi-to-pci-bridge + controller; all guest devices with PCI addresses that are + auto-determined by libvirt will be placed on this pci-bridge + device. (<span class="since">since 1.1.2</span>). </p> <pre> ... <devices> <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <address type='pci' domain='0' bus='0' slot='0xe' function='0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <address type='pci' domain='0' bus='1' slot='1' function='0'/> + </controller> </devices> ...</pre> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e04be12..173359c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1540,6 +1540,7 @@ <value>pci-root</value> <value>pcie-root</value> <value>pci-bridge</value> + <value>dmi-to-pci-bridge</value> </choice> </attribute> </group> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 59a96f2..d17008f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -311,7 +311,8 @@ VIR_ENUM_IMPL(virDomainController, VIR_DOMAIN_CONTROLLER_TYPE_LAST, VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pci-root", "pcie-root", - "pci-bridge") + "pci-bridge", + "dmi-to-pci-bridge") VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, "auto", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 63a1444..3e118d6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -770,6 +770,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE, + VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST } virDomainControllerModelPCI; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 08406b8..47cc07a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -234,6 +234,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "vnc-share-policy", /* 150 */ "device-del-event", + "dmi-to-pci-bridge", ); struct _virQEMUCaps { @@ -1381,6 +1382,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "pci-bridge", QEMU_CAPS_DEVICE_PCI_BRIDGE }, { "vfio-pci", QEMU_CAPS_DEVICE_VFIO_PCI }, { "scsi-generic", QEMU_CAPS_DEVICE_SCSI_GENERIC }, + { "i82801b11-bridge", QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f5f685d..074e55d 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -190,6 +190,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_MLOCK = 149, /* -realtime mlock=on|off */ QEMU_CAPS_VNC_SHARE_POLICY = 150, /* set display sharing policy */ QEMU_CAPS_DEVICE_DEL_EVENT = 151, /* DEVICE_DELETED event */ + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE = 152, /* -device i82801b11-bridge */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b6912ce..13a68a5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1561,6 +1561,13 @@ qemuDomainPCIAddressBusSetModel(qemuDomainPCIAddressBusPtr bus, bus->minSlot = 1; bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + /* slots 1 - 31, standard PCI slots, + * but *not* hot-pluggable */ + bus->flags = QEMU_PCI_CONNECT_TYPE_PCI; + bus->minSlot = 1; + bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PCI controller model %d"), model); @@ -1669,6 +1676,12 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, */ flags = QEMU_PCI_CONNECT_TYPE_PCI; break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + /* pci-bridge needs a PCIe slot, but it isn't + * hot-pluggable, so it doesn't need a hot-pluggable slot. + */ + flags = QEMU_PCI_CONNECT_TYPE_PCIE; + break; default: break; } @@ -2369,6 +2382,12 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, */ flags = QEMU_PCI_CONNECT_TYPE_PCI; break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + /* dmi-to-pci-bridge requires a non-hotplug PCIe + * slot + */ + flags = QEMU_PCI_CONNECT_TYPE_PCIE; + break; default: flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI; break; @@ -4348,6 +4367,20 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=pci.%d", def->idx, def->idx); break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("The dmi-to-pci-bridge (i82801b11-bridge) " + "controller is not supported in this QEMU binary")); + goto error; + } + if (def->idx == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("dmi-to-pci-bridge index should be > 0")); + goto error; + } + virBufferAsprintf(&buf, "i82801b11-bridge,id=pci.%d", def->idx); + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4d04609..f5030cd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -760,10 +760,20 @@ qemuDomainDefPostParse(virDomainDefPtr def, VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) return -1; + /* When a machine has a pcie-root, make sure that there is always + * a dmi-to-pci-bridge controller added as bus 1, and a pci-bridge + * as bus 2, so that standard PCI devices can be connected + */ if (addPCIeRoot && - virDomainDefMaybeAddController( + (virDomainDefMaybeAddController( def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, - VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0) + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0 || + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 1, + VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE) < 0 || + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 2, + VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE) < 0)) return -1; return 0; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index e937189..23db85c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,4 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.1,addr=0x1 -usb +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args new file mode 100644 index 0000000..ddff6f0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ +/usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-usb \ +-vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml new file mode 100644 index 0000000..3541b14 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml @@ -0,0 +1,25 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <video> + <model type='qxl' ram='65536' vram='18432' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 57c6989..aba0f88 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -995,7 +995,13 @@ mymain(void) DO_TEST("pci-bridge-many-disks", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pcie-root", - QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE); + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE); + DO_TEST("q35", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml new file mode 100644 index 0000000..25c77f1 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml @@ -0,0 +1,23 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='usb' index='0'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index ea511b8..8b4590a 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -294,7 +294,8 @@ mymain(void) DO_TEST_DIFFERENT("pci-bridge-many-disks"); DO_TEST_DIFFERENT("pci-autoadd-addr"); DO_TEST_DIFFERENT("pci-autoadd-idx"); - DO_TEST("pcie-root"); + DO_TEST_DIFFERENT("pcie-root"); + DO_TEST("q35"); DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 1.7.11.7
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote:
This PCI controller, named "dmi-to-pci-bridge" in the libvirt config, and implemented with qemu's "i82801b11-bridge" device, connects to a PCI Express slot (e.g. one of the slots provided by the pcie-root controller, aka "pcie.0" on the qemu commandline), and provides 31 *non-hot-pluggable* PCI (*not* PCIe) slots, numbered 1-31.
Any time a machine is defined which has a pcie-root controller (i.e. any q35-based machinetype), libvirt will automatically add a dmi-to-pci-bridge controller if one doesn't exist, and also add a pci-bridge controller. The reasoning here is that any useful domain will have either an immediate (startup time) or eventual (subsequent hot-plug) need for a standard PCI slot; since the pcie-root controller only provides PCIe slots, we need to connect a dmi-to-pci-bridge controller to it in order to get a non-hot-plug PCI slot that we can then use to connect a pci-bridge - the slots provided by the pci-bridge will be both standard PCI and hot-pluggable.
Since pci-bridge devices themselves are not hot-pluggable, any new pci-bridge controller that is added can (and will) be plugged into the dmi-to-pci-bridge as long as it has empty slots available.
Worth noting DO_TEST_DIFFERENT to pcie-root change here since you mention changes like that in later commits.
--- docs/formatdomain.html.in | 28 +++++++++++++++--- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 3 +- src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 33 ++++++++++++++++++++++ src/qemu/qemu_domain.c | 14 +++++++-- tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 3 +- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 7 +++++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 25 ++++++++++++++++ tests/qemuxml2argvtest.c | 8 +++++- .../qemuxml2xmlout-pcie-root.xml | 23 +++++++++++++++ tests/qemuxml2xmltest.c | 3 +- 14 files changed, 142 insertions(+), 10 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 330dca2..8fa4c0e 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2338,16 +2338,19 @@
<p> PCI controllers have an optional <code>model</code> attribute with - possible values <code>pci-root</code>, <code>pcie-root</code> - or <code>pci-bridge</code>. + possible values <code>pci-root</code>, <code>pcie-root</code>, + <code>pci-bridge</code>, or <code>dmi-to-pci-bridge</code>. For machine types which provide an implicit PCI bus, the pci-root controller with index=0 is auto-added and required to use PCI devices. pci-root has no address. + PCI bridges are auto-added if there are too many devices to fit on + the one bus provided by pci-root, or a PCI bus number greater than zero + was specified. PCI bridges can also be specified manually, but their addresses should only refer to PCI buses provided by already specified PCI controllers. Leaving gaps in the PCI controller indexes might lead to an invalid configuration. - (<span class="since">since 1.0.5</span>) + (pci-root and pci-bridge <span class="since">since 1.0.5</span>).
Probably belonged as part of the last patch technically, but they'll be part of the series so it should be ok.
</p> <pre> ... @@ -2365,12 +2368,29 @@ the pcie-root controller with index=0 is auto-added to the domain's configuration. pcie-root has also no address, provides 31 slots (numbered 1-31) and can only be used to attach PCIe - devices. (<span class="since">since 1.1.2</span>). + devices. In order to connect standard PCI devices on a system + which has a pcie-root controller, a pci controller + with <code>model='dmi-to-pci-bridge'</code> is automatically + added. A dmi-to-pci-bridge controller plugs into a PCIe slot (as + provided by pcie-root), and itself provides 31 standard PCI + slots (which are not hot-pluggable). In order to have + hot-pluggable PCI slots in the guest system, a pci-bridge + controller will also be automatically created and connected to + one of the slots of the auto-created dmi-to-pci-bridge + controller; all guest devices with PCI addresses that are + auto-determined by libvirt will be placed on this pci-bridge + device. (<span class="since">since 1.1.2</span>). </p> <pre> ... <devices> <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <address type='pci' domain='0' bus='0' slot='0xe' function='0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <address type='pci' domain='0' bus='1' slot='1' function='0'/> + </controller> </devices> ...</pre>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e04be12..173359c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1540,6 +1540,7 @@ <value>pci-root</value> <value>pcie-root</value> <value>pci-bridge</value> + <value>dmi-to-pci-bridge</value> </choice> </attribute> </group> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 59a96f2..d17008f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -311,7 +311,8 @@ VIR_ENUM_IMPL(virDomainController, VIR_DOMAIN_CONTROLLER_TYPE_LAST, VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pci-root", "pcie-root", - "pci-bridge") + "pci-bridge", + "dmi-to-pci-bridge")
VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, "auto", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 63a1444..3e118d6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -770,6 +770,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE, + VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE,
VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST } virDomainControllerModelPCI; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 08406b8..47cc07a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -234,6 +234,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"vnc-share-policy", /* 150 */ "device-del-event", + "dmi-to-pci-bridge", );
struct _virQEMUCaps { @@ -1381,6 +1382,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "pci-bridge", QEMU_CAPS_DEVICE_PCI_BRIDGE }, { "vfio-pci", QEMU_CAPS_DEVICE_VFIO_PCI }, { "scsi-generic", QEMU_CAPS_DEVICE_SCSI_GENERIC }, + { "i82801b11-bridge", QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE }, };
static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f5f685d..074e55d 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -190,6 +190,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_MLOCK = 149, /* -realtime mlock=on|off */ QEMU_CAPS_VNC_SHARE_POLICY = 150, /* set display sharing policy */ QEMU_CAPS_DEVICE_DEL_EVENT = 151, /* DEVICE_DELETED event */ + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE = 152, /* -device i82801b11-bridge */
QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b6912ce..13a68a5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1561,6 +1561,13 @@ qemuDomainPCIAddressBusSetModel(qemuDomainPCIAddressBusPtr bus, bus->minSlot = 1; bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + /* slots 1 - 31, standard PCI slots, + * but *not* hot-pluggable */ + bus->flags = QEMU_PCI_CONNECT_TYPE_PCI; + bus->minSlot = 1; + bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PCI controller model %d"), model); @@ -1669,6 +1676,12 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, */ flags = QEMU_PCI_CONNECT_TYPE_PCI; break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + /* pci-bridge needs a PCIe slot, but it isn't + * hot-pluggable, so it doesn't need a hot-pluggable slot. + */ + flags = QEMU_PCI_CONNECT_TYPE_PCIE; + break; default: break; } @@ -2369,6 +2382,12 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, */ flags = QEMU_PCI_CONNECT_TYPE_PCI; break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + /* dmi-to-pci-bridge requires a non-hotplug PCIe + * slot + */ + flags = QEMU_PCI_CONNECT_TYPE_PCIE; + break; default: flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI; break; @@ -4348,6 +4367,20 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=pci.%d", def->idx, def->idx); break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("The dmi-to-pci-bridge (i82801b11-bridge) " + "controller is not supported in this QEMU binary"));
Do we ever print out the path to the QEMU binary used in these kinds of errors? Might be nice.
+ goto error; + } + if (def->idx == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("dmi-to-pci-bridge index should be > 0")); + goto error; + } + virBufferAsprintf(&buf, "i82801b11-bridge,id=pci.%d", def->idx); + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4d04609..f5030cd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -760,10 +760,20 @@ qemuDomainDefPostParse(virDomainDefPtr def, VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) return -1;
+ /* When a machine has a pcie-root, make sure that there is always + * a dmi-to-pci-bridge controller added as bus 1, and a pci-bridge + * as bus 2, so that standard PCI devices can be connected + */ if (addPCIeRoot && - virDomainDefMaybeAddController( + (virDomainDefMaybeAddController( def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, - VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0) + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0 || + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 1, + VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE) < 0 || + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 2, + VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE) < 0)) return -1;
Not going to lie, that's an if check of hell. Had to really stare for a second to make sure everything was lined up correctly.
return 0; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index e937189..23db85c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,4 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.1,addr=0x1 -usb +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args new file mode 100644 index 0000000..ddff6f0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ +/usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-usb \ +-vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml new file mode 100644 index 0000000..3541b14 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml @@ -0,0 +1,25 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <video> + <model type='qxl' ram='65536' vram='18432' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 57c6989..aba0f88 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -995,7 +995,13 @@ mymain(void) DO_TEST("pci-bridge-many-disks", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pcie-root", - QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE); + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE); + DO_TEST("q35", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml new file mode 100644 index 0000000..25c77f1 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml @@ -0,0 +1,23 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='usb' index='0'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index ea511b8..8b4590a 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -294,7 +294,8 @@ mymain(void) DO_TEST_DIFFERENT("pci-bridge-many-disks"); DO_TEST_DIFFERENT("pci-autoadd-addr"); DO_TEST_DIFFERENT("pci-autoadd-idx"); - DO_TEST("pcie-root"); + DO_TEST_DIFFERENT("pcie-root"); + DO_TEST("q35");
DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 1.7.11.7
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Overall an ACK, just fix up the commit message and move the since documentation fixup to 2/7, which I think you can do without reposting. -- Doug Goldstein
On 08/04/2013 07:52 PM, Doug Goldstein wrote:
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote:
This PCI controller, named "dmi-to-pci-bridge" in the libvirt config, and implemented with qemu's "i82801b11-bridge" device, connects to a PCI Express slot (e.g. one of the slots provided by the pcie-root controller, aka "pcie.0" on the qemu commandline), and provides 31 *non-hot-pluggable* PCI (*not* PCIe) slots, numbered 1-31.
Any time a machine is defined which has a pcie-root controller (i.e. any q35-based machinetype), libvirt will automatically add a dmi-to-pci-bridge controller if one doesn't exist, and also add a pci-bridge controller. The reasoning here is that any useful domain will have either an immediate (startup time) or eventual (subsequent hot-plug) need for a standard PCI slot; since the pcie-root controller only provides PCIe slots, we need to connect a dmi-to-pci-bridge controller to it in order to get a non-hot-plug PCI slot that we can then use to connect a pci-bridge - the slots provided by the pci-bridge will be both standard PCI and hot-pluggable.
Since pci-bridge devices themselves are not hot-pluggable, any new pci-bridge controller that is added can (and will) be plugged into the dmi-to-pci-bridge as long as it has empty slots available. Worth noting DO_TEST_DIFFERENT to pcie-root change here since you mention changes like that in later commits.
Okay. I did that here so that the pcie-root test could also become a test to assure that the implicit dmi-to-pci-bridge and pci-bridge devices were added. I'll add that comment.
<p> PCI controllers have an optional <code>model</code> attribute with - possible values <code>pci-root</code>, <code>pcie-root</code> - or <code>pci-bridge</code>. + possible values <code>pci-root</code>, <code>pcie-root</code>, + <code>pci-bridge</code>, or <code>dmi-to-pci-bridge</code>. For machine types which provide an implicit PCI bus, the pci-root controller with index=0 is auto-added and required to use PCI devices. pci-root has no address. + PCI bridges are auto-added if there are too many devices to fit on + the one bus provided by pci-root, or a PCI bus number greater than zero + was specified. PCI bridges can also be specified manually, but their addresses should only refer to PCI buses provided by already specified PCI controllers. Leaving gaps in the PCI controller indexes might lead to an invalid configuration. - (<span class="since">since 1.0.5</span>) + (pci-root and pci-bridge <span class="since">since 1.0.5</span>).
Probably belonged as part of the last patch technically, but they'll be part of the series so it should be ok.
Okay, I'll move it.
+ case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("The dmi-to-pci-bridge (i82801b11-bridge) " + "controller is not supported in this QEMU binary")); Do we ever print out the path to the QEMU binary used in these kinds of errors? Might be nice.
The path to the qemu binary is part of the xml, so it's easily available (to us, and to the user). None of the "in this QEMU binary" messages print out the path, but I suppose it's worth discussing.
+ + /* When a machine has a pcie-root, make sure that there is always + * a dmi-to-pci-bridge controller added as bus 1, and a pci-bridge + * as bus 2, so that standard PCI devices can be connected + */ if (addPCIeRoot && - virDomainDefMaybeAddController( + (virDomainDefMaybeAddController( def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, - VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0) + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0 || + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 1, + VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE) < 0 || + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 2, + VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE) < 0)) return -1; Not going to lie, that's an if check of hell. Had to really stare for a second to make sure everything was lined up correctly.
I actually got it wrong when I first wrote it (but found the problem during testing). I'll break it up into two nested ifs, just for sake of understanding.
Overall an ACK, just fix up the commit message and move the since documentation fixup to 2/7, which I think you can do without reposting.
Okay, I've made those changes plus breaking up the "if from hell". Thanks for reviewing and testing!
This patch adds in special handling for a few devices that need to be treated differently for q35 domains: usb - there is no implicit/default usb controller for the q35 machinetype. This is done because normally the default usb controller is added to a domain by just adding "-usb" to the qemu commandline, and it's assumed that this will add a single piix3 usb1 controller at slot 1 function 2. That's not what happens when the machinetype is q35, though. Instead, adding -usb to the commandline adds 3 usb (version 2) controllers to the domain at slot 0x1D.{1,2,7}. Rather than having <controller type='usb' index='0'/> translate into 3 separate devices on the PCI bus, it's cleaner to not automatically add a default usb device; one can always be added explicitly if desired. Or we may decide that on q35 machines, 3 usb controllers will be automatically added when none is given. But for this initial commit, at least we aren't locking ourselves into something we later won't want. video - for pc machine types, the primary video device is always in slot 2, and that slot is reserved even when no video device has been specified. On q35, when you specify "-vga qxl" on the qemu commandline, the vga device is put in slot 1, not 2. Assuming that this was done for a reason, this patch always puts the primary video for q35 machines in slot 1, and reserves slot 1 even if there is no video. sata - a q35 machine always has a sata controller implicitly added at slot 0x1F, function 2. There is no way to avoid this controller, so we always add it. Note that the xml2xml tests for the pcie-root and q35 cases were changed to use DO_TEST_DIFFERENT() so that we can check for the sata controller being automatically added. This is especially important because we can't check for it in the xml2argv output (it has no effect on that output since it's an implicit device). ide - q35 has no ide controllers. isa and smbus controllers - these two are always present in a q35 (at slot 0x1F functions 0 and 3) but we have no way of modelling them in our config. We do need to reserve those functions so that the user doesn't attempt to put anything else there though. --- src/qemu/qemu_command.c | 150 ++++++++++++++++++++- src/qemu/qemu_domain.c | 7 + tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 4 +- tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml | 1 - tests/qemuxml2argvdata/qemuxml2argv-q35.args | 3 +- tests/qemuxml2argvtest.c | 2 + .../qemuxml2xmlout-pcie-root.xml | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 26 ++++ tests/qemuxml2xmltest.c | 2 +- 9 files changed, 189 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 13a68a5..a6d6819 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1686,6 +1686,18 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, break; } } + /* SATA controllers aren't hot-plugged, and can be put in either a + * PCI or PCIe slot + */ + if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER && + device->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) + flags = QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE; + + /* video cards aren't hot-plugged, and can be put in either a PCI + * or PCIe slot + */ + if (device->type == VIR_DOMAIN_DEVICE_VIDEO) + flags = QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE; /* Ignore implicit controllers on slot 0:0:1.0: * implicit IDE controller on 0:0:1.1 (no qemu command line) @@ -2315,6 +2327,133 @@ error: } +static bool +qemuDomainMachineIsQ35(virDomainDefPtr def) +{ + return (STRPREFIX(def->os.machine, "pc-q35") || + STREQ(def->os.machine, "q35")); +} + + +static int +qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + qemuDomainPCIAddressSetPtr addrs) +{ + size_t i; + virDevicePCIAddress tmp_addr; + bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + virDevicePCIAddressPtr addrptr; + qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPE_PCIE; + + /* Verify that the first SATA controller is at 00:1F.2 */ + /* the q35 machine type *always* has a SATA controller at this address */ + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && + def->controllers[i]->idx == 0) { + if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (def->controllers[i]->info.addr.pci.domain != 0 || + def->controllers[i]->info.addr.pci.bus != 0 || + def->controllers[i]->info.addr.pci.slot != 0x1F || + def->controllers[i]->info.addr.pci.function != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Primary SATA controller must have PCI address 0:0:1f.2")); + goto error; + } + } else { + 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 = 0x1F; + def->controllers[i]->info.addr.pci.function = 2; + } + } + } + + /* Reserve slot 0x1F function 0 (ISA bridge, not in config model) + * and function 3 (SMBus, also not (yet) in config model). As with + * the SATA controller, these devices are always present in a q35 + * machine; there is no way to not have them. + */ + if (addrs->nbuses) { + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 0x1F; + tmp_addr.function = 0; + tmp_addr.multi = 1; + if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, + false, false) < 0) + goto error; + tmp_addr.function = 3; + tmp_addr.multi = 0; + if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, + false, false) < 0) + goto error; + } + + if (def->nvideos > 0) { + /* NB: unlike the pc machinetypes, q35 machinetypes put the primary VGA + * at slot 1 for some reason. + */ + virDomainVideoDefPtr primaryVideo = def->videos[0]; + if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + primaryVideo->info.addr.pci.domain = 0; + primaryVideo->info.addr.pci.bus = 0; + primaryVideo->info.addr.pci.slot = 1; + primaryVideo->info.addr.pci.function = 0; + addrptr = &primaryVideo->info.addr.pci; + + if (!qemuDomainPCIAddressValidate(addrs, addrptr, flags)) + goto error; + + if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { + if (qemuDeviceVideoUsable) { + virResetLastError(); + if (qemuDomainPCIAddressReserveNextSlot(addrs, + &primaryVideo->info, + flags) < 0) + goto error; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("PCI address 0:0:1.0 is in use, " + "QEMU needs it for primary video")); + goto error; + } + } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) { + goto error; + } + } else if (!qemuDeviceVideoUsable) { + if (primaryVideo->info.addr.pci.domain != 0 || + primaryVideo->info.addr.pci.bus != 0 || + primaryVideo->info.addr.pci.slot != 1 || + primaryVideo->info.addr.pci.function != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Primary video card must have PCI address 0:0:1.0")); + goto error; + } + /* If TYPE==PCI, then qemuCollectPCIAddress() function + * has already reserved the address, so we must skip */ + } + } else if (addrs->nbuses && !qemuDeviceVideoUsable) { + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 1; + + if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { + VIR_DEBUG("PCI address 0:0:1.0 in use, future addition of a video" + " device will not be possible without manual" + " intervention"); + virResetLastError(); + } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) { + goto error; + } + } + return 0; + +error: + return -1; +} + + /* * This assigns static PCI slots to all configured devices. * The ordering here is chosen to match the ordering used @@ -2365,6 +2504,11 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, goto error; } + if (qemuDomainMachineIsQ35(def) && + qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { + goto error; + } + /* PCI controllers */ for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { @@ -7676,6 +7820,9 @@ qemuBuildCommandLine(virConnectPtr conn, _("SATA is not supported with this " "QEMU binary")); goto error; + } else if (cont->idx == 0 && qemuDomainMachineIsQ35(def)) { + /* first SATA controller on Q35 machines is implicit */ + continue; } else { char *devstr; @@ -7689,6 +7836,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == -1 && + !qemuDomainMachineIsQ35(def) && (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || def->os.arch == VIR_ARCH_PPC64)) { if (usblegacy) { @@ -7713,7 +7861,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } - if (usbcontroller == 0) + if (usbcontroller == 0 && !qemuDomainMachineIsQ35(def)) virCommandAddArg(cmd, "-usb"); for (i = 0; i < def->nhubs; i++) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f5030cd..75be591 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -700,6 +700,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, void *opaque ATTRIBUTE_UNUSED) { bool addDefaultUSB = true; + bool addImplicitSATA = false; bool addPCIRoot = false; bool addPCIeRoot = false; @@ -722,6 +723,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, STREQ(def->os.machine, "q35")) { addPCIeRoot = true; addDefaultUSB = false; + addImplicitSATA = true; break; } if (!STRPREFIX(def->os.machine, "pc-0.") && @@ -754,6 +756,11 @@ qemuDomainDefPostParse(virDomainDefPtr def, def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0, -1) < 0) return -1; + if (addImplicitSATA && + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, 0, -1) < 0) + return -1; + if (addPCIRoot && virDomainDefMaybeAddController( def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index 23db85c..cecef7b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ --device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 -usb +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml index 1aa5455..d7fb90c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml @@ -15,7 +15,6 @@ <devices> <emulator>/usr/libexec/qemu-kvm</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='usb' index='0'/> <memballoon model='none'/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index ddff6f0..6c24407 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,7 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ --usb \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index aba0f88..0068d27 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -995,11 +995,13 @@ mymain(void) DO_TEST("pci-bridge-many-disks", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pcie-root", + QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE); DO_TEST("q35", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml index 25c77f1..f10e85b 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/libexec/qemu-kvm</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='usb' index='0'/> + <controller type='sata' index='0'/> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> <controller type='pci' index='2' model='pci-bridge'/> <memballoon model='none'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml new file mode 100644 index 0000000..2a86e61 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='18432' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 8b4590a..5c6730d 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -295,7 +295,7 @@ mymain(void) DO_TEST_DIFFERENT("pci-autoadd-addr"); DO_TEST_DIFFERENT("pci-autoadd-idx"); DO_TEST_DIFFERENT("pcie-root"); - DO_TEST("q35"); + DO_TEST_DIFFERENT("q35"); DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 1.7.11.7
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote:
This patch adds in special handling for a few devices that need to be treated differently for q35 domains:
usb - there is no implicit/default usb controller for the q35 machinetype. This is done because normally the default usb controller is added to a domain by just adding "-usb" to the qemu commandline, and it's assumed that this will add a single piix3 usb1 controller at slot 1 function 2. That's not what happens when the machinetype is q35, though. Instead, adding -usb to the commandline adds 3 usb (version 2) controllers to the domain at slot 0x1D.{1,2,7}. Rather than having
<controller type='usb' index='0'/>
translate into 3 separate devices on the PCI bus, it's cleaner to not automatically add a default usb device; one can always be added explicitly if desired. Or we may decide that on q35 machines, 3 usb controllers will be automatically added when none is given. But for this initial commit, at least we aren't locking ourselves into something we later won't want.
video - for pc machine types, the primary video device is always in slot 2, and that slot is reserved even when no video device has been specified. On q35, when you specify "-vga qxl" on the qemu commandline, the vga device is put in slot 1, not 2. Assuming that this was done for a reason, this patch always puts the primary video for q35 machines in slot 1, and reserves slot 1 even if there is no video.
Might be worth updating the comments here to say that QEMU will assign it in the first available slot, which with q35 is slot 1 and i440fx (pc) is slot 2 (due to the always present IDE controller).
sata - a q35 machine always has a sata controller implicitly added at slot 0x1F, function 2. There is no way to avoid this controller, so we always add it. Note that the xml2xml tests for the pcie-root and q35 cases were changed to use DO_TEST_DIFFERENT() so that we can check for the sata controller being automatically added. This is especially important because we can't check for it in the xml2argv output (it has no effect on that output since it's an implicit device).
The note about pcie-root switching to DO_TEST_DIFFERENT should go into the earlier patch.
ide - q35 has no ide controllers.
isa and smbus controllers - these two are always present in a q35 (at slot 0x1F functions 0 and 3) but we have no way of modelling them in our config. We do need to reserve those functions so that the user doesn't attempt to put anything else there though. --- src/qemu/qemu_command.c | 150 ++++++++++++++++++++- src/qemu/qemu_domain.c | 7 + tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 4 +- tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml | 1 - tests/qemuxml2argvdata/qemuxml2argv-q35.args | 3 +- tests/qemuxml2argvtest.c | 2 + .../qemuxml2xmlout-pcie-root.xml | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 26 ++++ tests/qemuxml2xmltest.c | 2 +- 9 files changed, 189 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 13a68a5..a6d6819 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1686,6 +1686,18 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, break; } } + /* SATA controllers aren't hot-plugged, and can be put in either a + * PCI or PCIe slot + */ + if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER && + device->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) + flags = QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE; + + /* video cards aren't hot-plugged, and can be put in either a PCI + * or PCIe slot + */ + if (device->type == VIR_DOMAIN_DEVICE_VIDEO) + flags = QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE;
/* Ignore implicit controllers on slot 0:0:1.0: * implicit IDE controller on 0:0:1.1 (no qemu command line) @@ -2315,6 +2327,133 @@ error: }
+static bool +qemuDomainMachineIsQ35(virDomainDefPtr def) +{ + return (STRPREFIX(def->os.machine, "pc-q35") || + STREQ(def->os.machine, "q35")); +} + + +static int +qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + qemuDomainPCIAddressSetPtr addrs) +{ + size_t i; + virDevicePCIAddress tmp_addr; + bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + virDevicePCIAddressPtr addrptr; + qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPE_PCIE; + + /* Verify that the first SATA controller is at 00:1F.2 */ + /* the q35 machine type *always* has a SATA controller at this address */ + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && + def->controllers[i]->idx == 0) { + if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (def->controllers[i]->info.addr.pci.domain != 0 || + def->controllers[i]->info.addr.pci.bus != 0 || + def->controllers[i]->info.addr.pci.slot != 0x1F || + def->controllers[i]->info.addr.pci.function != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Primary SATA controller must have PCI address 0:0:1f.2")); + goto error; + } + } else { + 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 = 0x1F; + def->controllers[i]->info.addr.pci.function = 2; + } + } + } + + /* Reserve slot 0x1F function 0 (ISA bridge, not in config model) + * and function 3 (SMBus, also not (yet) in config model). As with + * the SATA controller, these devices are always present in a q35 + * machine; there is no way to not have them. + */ + if (addrs->nbuses) { + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 0x1F; + tmp_addr.function = 0; + tmp_addr.multi = 1; + if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, + false, false) < 0) + goto error; + tmp_addr.function = 3; + tmp_addr.multi = 0; + if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, + false, false) < 0) + goto error; + } + + if (def->nvideos > 0) { + /* NB: unlike the pc machinetypes, q35 machinetypes put the primary VGA + * at slot 1 for some reason. + */ + virDomainVideoDefPtr primaryVideo = def->videos[0]; + if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + primaryVideo->info.addr.pci.domain = 0; + primaryVideo->info.addr.pci.bus = 0; + primaryVideo->info.addr.pci.slot = 1; + primaryVideo->info.addr.pci.function = 0; + addrptr = &primaryVideo->info.addr.pci; + + if (!qemuDomainPCIAddressValidate(addrs, addrptr, flags)) + goto error; + + if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { + if (qemuDeviceVideoUsable) { + virResetLastError(); + if (qemuDomainPCIAddressReserveNextSlot(addrs, + &primaryVideo->info, + flags) < 0) + goto error; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("PCI address 0:0:1.0 is in use, " + "QEMU needs it for primary video")); + goto error; + } + } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) { + goto error; + } + } else if (!qemuDeviceVideoUsable) { + if (primaryVideo->info.addr.pci.domain != 0 || + primaryVideo->info.addr.pci.bus != 0 || + primaryVideo->info.addr.pci.slot != 1 || + primaryVideo->info.addr.pci.function != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Primary video card must have PCI address 0:0:1.0")); + goto error; + } + /* If TYPE==PCI, then qemuCollectPCIAddress() function + * has already reserved the address, so we must skip */ + } + } else if (addrs->nbuses && !qemuDeviceVideoUsable) { + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 1; + + if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { + VIR_DEBUG("PCI address 0:0:1.0 in use, future addition of a video" + " device will not be possible without manual" + " intervention"); + virResetLastError(); + } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) { + goto error; + } + }
Is this really necessary/correct? From the qemu-devel thread it really seems like the VGA controller will just take the first available slot but is it not possible for us to stick this somewhere other than slot 1? I know with current libvirt and qemu we limit ourselves to -vga qxl rather than -device qxl-vga due to a bug in QEMU but I think with QEMU 1.6, that limitation goes away so that might free us up to move it around.
+ return 0; + +error: + return -1; +} + + /* * This assigns static PCI slots to all configured devices. * The ordering here is chosen to match the ordering used @@ -2365,6 +2504,11 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, goto error; }
+ if (qemuDomainMachineIsQ35(def) && + qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { + goto error; + } + /* PCI controllers */ for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { @@ -7676,6 +7820,9 @@ qemuBuildCommandLine(virConnectPtr conn, _("SATA is not supported with this " "QEMU binary")); goto error; + } else if (cont->idx == 0 && qemuDomainMachineIsQ35(def)) { + /* first SATA controller on Q35 machines is implicit */ + continue; } else { char *devstr;
@@ -7689,6 +7836,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == -1 && + !qemuDomainMachineIsQ35(def) && (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || def->os.arch == VIR_ARCH_PPC64)) { if (usblegacy) { @@ -7713,7 +7861,7 @@ qemuBuildCommandLine(virConnectPtr conn, } }
- if (usbcontroller == 0) + if (usbcontroller == 0 && !qemuDomainMachineIsQ35(def)) virCommandAddArg(cmd, "-usb");
for (i = 0; i < def->nhubs; i++) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f5030cd..75be591 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -700,6 +700,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, void *opaque ATTRIBUTE_UNUSED) { bool addDefaultUSB = true; + bool addImplicitSATA = false; bool addPCIRoot = false; bool addPCIeRoot = false;
@@ -722,6 +723,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, STREQ(def->os.machine, "q35")) { addPCIeRoot = true; addDefaultUSB = false; + addImplicitSATA = true; break; } if (!STRPREFIX(def->os.machine, "pc-0.") && @@ -754,6 +756,11 @@ qemuDomainDefPostParse(virDomainDefPtr def, def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0, -1) < 0) return -1;
+ if (addImplicitSATA && + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, 0, -1) < 0) + return -1; + if (addPCIRoot && virDomainDefMaybeAddController( def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index 23db85c..cecef7b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ --device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 -usb +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml index 1aa5455..d7fb90c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml @@ -15,7 +15,6 @@ <devices> <emulator>/usr/libexec/qemu-kvm</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='usb' index='0'/> <memballoon model='none'/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index ddff6f0..6c24407 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,7 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ --usb \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index aba0f88..0068d27 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -995,11 +995,13 @@ mymain(void) DO_TEST("pci-bridge-many-disks", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pcie-root", + QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE); DO_TEST("q35", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml index 25c77f1..f10e85b 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/libexec/qemu-kvm</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='usb' index='0'/> + <controller type='sata' index='0'/> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> <controller type='pci' index='2' model='pci-bridge'/> <memballoon model='none'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml new file mode 100644 index 0000000..2a86e61 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='18432' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 8b4590a..5c6730d 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -295,7 +295,7 @@ mymain(void) DO_TEST_DIFFERENT("pci-autoadd-addr"); DO_TEST_DIFFERENT("pci-autoadd-idx"); DO_TEST_DIFFERENT("pcie-root"); - DO_TEST("q35"); + DO_TEST_DIFFERENT("q35");
DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 1.7.11.7
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
I'm going to hold off on saying ACK here due to my question above, but we might not even need a code change if my understanding is wrong. -- Doug Goldstein
On 08/04/2013 07:53 PM, Doug Goldstein wrote:
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote:
This patch adds in special handling for a few devices that need to be treated differently for q35 domains:
usb - there is no implicit/default usb controller for the q35 machinetype. This is done because normally the default usb controller is added to a domain by just adding "-usb" to the qemu commandline, and it's assumed that this will add a single piix3 usb1 controller at slot 1 function 2. That's not what happens when the machinetype is q35, though. Instead, adding -usb to the commandline adds 3 usb (version 2) controllers to the domain at slot 0x1D.{1,2,7}. Rather than having
<controller type='usb' index='0'/>
translate into 3 separate devices on the PCI bus, it's cleaner to not automatically add a default usb device; one can always be added explicitly if desired. Or we may decide that on q35 machines, 3 usb controllers will be automatically added when none is given. But for this initial commit, at least we aren't locking ourselves into something we later won't want.
video - for pc machine types, the primary video device is always in slot 2, and that slot is reserved even when no video device has been specified. On q35, when you specify "-vga qxl" on the qemu commandline, the vga device is put in slot 1, not 2. Assuming that this was done for a reason, this patch always puts the primary video for q35 machines in slot 1, and reserves slot 1 even if there is no video. Might be worth updating the comments here to say that QEMU will assign it in the first available slot, which with q35 is slot 1 and i440fx (pc) is slot 2 (due to the always present IDE controller).
I see you caught my question (and the later discussion) on qemu-devel :-) yeah, I think it would be reasonable to change this comment, now that I understand better what is happening.
sata - a q35 machine always has a sata controller implicitly added at slot 0x1F, function 2. There is no way to avoid this controller, so we always add it. Note that the xml2xml tests for the pcie-root and q35 cases were changed to use DO_TEST_DIFFERENT() so that we can check for the sata controller being automatically added. This is especially important because we can't check for it in the xml2argv output (it has no effect on that output since it's an implicit device). The note about pcie-root switching to DO_TEST_DIFFERENT should go into the earlier patch.
Done.
ide - q35 has no ide controllers.
isa and smbus controllers - these two are always present in a q35 (at slot 0x1F functions 0 and 3) but we have no way of modelling them in our config. We do need to reserve those functions so that the user doesn't attempt to put anything else there though. ---
+ if (def->nvideos > 0) { + /* NB: unlike the pc machinetypes, q35 machinetypes put the primary VGA + * at slot 1 for some reason. + */ + virDomainVideoDefPtr primaryVideo = def->videos[0]; + if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + primaryVideo->info.addr.pci.domain = 0; + primaryVideo->info.addr.pci.bus = 0; + primaryVideo->info.addr.pci.slot = 1; + primaryVideo->info.addr.pci.function = 0; + addrptr = &primaryVideo->info.addr.pci; + + if (!qemuDomainPCIAddressValidate(addrs, addrptr, flags)) + goto error; + + if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { + if (qemuDeviceVideoUsable) { + virResetLastError(); + if (qemuDomainPCIAddressReserveNextSlot(addrs, + &primaryVideo->info, + flags) < 0) + goto error; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("PCI address 0:0:1.0 is in use, " + "QEMU needs it for primary video")); + goto error; + } + } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) { + goto error; + } + } else if (!qemuDeviceVideoUsable) { + if (primaryVideo->info.addr.pci.domain != 0 || + primaryVideo->info.addr.pci.bus != 0 || + primaryVideo->info.addr.pci.slot != 1 || + primaryVideo->info.addr.pci.function != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Primary video card must have PCI address 0:0:1.0")); + goto error; + } + /* If TYPE==PCI, then qemuCollectPCIAddress() function + * has already reserved the address, so we must skip */ + } + } else if (addrs->nbuses && !qemuDeviceVideoUsable) { + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 1; + + if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { + VIR_DEBUG("PCI address 0:0:1.0 in use, future addition of a video" + " device will not be possible without manual" + " intervention"); + virResetLastError(); + } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) { + goto error; + } + } Is this really necessary/correct? From the qemu-devel thread it really seems like the VGA controller will just take the first available slot but is it not possible for us to stick this somewhere other than slot 1? I know with current libvirt and qemu we limit ourselves to -vga qxl rather than -device qxl-vga due to a bug in QEMU but I think with QEMU 1.6, that limitation goes away so that might free us up to move it around.
Unfortunately, I think that if we want to avoid surprises with the location of the video device then we have to do this. The problem is that not everybody has qemu 1.6+ (actually at this point *almost nobody* does). If we don't reserve "the first" slot (be it 1 or 2) for the video adapter when the domain is created, and continue to watch out for it as long as no video is added, another device would probably go in that place, and when the user finally did get around to adding a device, it would go in "some other" indeterminate place (well, we could figure it out, but it would be much more inconvenient that simply reserving slot 1 or 2 on every domain for video) I may be taking this too seriously though - does anyone else want to chime in one way or another?
+ return 0; + +error: + return -1; +} + + /* * This assigns static PCI slots to all configured devices. * The ordering here is chosen to match the ordering used @@ -2365,6 +2504,11 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, goto error; }
+ if (qemuDomainMachineIsQ35(def) && + qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { + goto error; + } + /* PCI controllers */ for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { @@ -7676,6 +7820,9 @@ qemuBuildCommandLine(virConnectPtr conn, _("SATA is not supported with this " "QEMU binary")); goto error; + } else if (cont->idx == 0 && qemuDomainMachineIsQ35(def)) { + /* first SATA controller on Q35 machines is implicit */ + continue; } else { char *devstr;
@@ -7689,6 +7836,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == -1 && + !qemuDomainMachineIsQ35(def) && (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || def->os.arch == VIR_ARCH_PPC64)) { if (usblegacy) { @@ -7713,7 +7861,7 @@ qemuBuildCommandLine(virConnectPtr conn, } }
- if (usbcontroller == 0) + if (usbcontroller == 0 && !qemuDomainMachineIsQ35(def)) virCommandAddArg(cmd, "-usb");
for (i = 0; i < def->nhubs; i++) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f5030cd..75be591 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -700,6 +700,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, void *opaque ATTRIBUTE_UNUSED) { bool addDefaultUSB = true; + bool addImplicitSATA = false; bool addPCIRoot = false; bool addPCIeRoot = false;
@@ -722,6 +723,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, STREQ(def->os.machine, "q35")) { addPCIeRoot = true; addDefaultUSB = false; + addImplicitSATA = true; break; } if (!STRPREFIX(def->os.machine, "pc-0.") && @@ -754,6 +756,11 @@ qemuDomainDefPostParse(virDomainDefPtr def, def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0, -1) < 0) return -1;
+ if (addImplicitSATA && + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, 0, -1) < 0) + return -1; + if (addPCIRoot && virDomainDefMaybeAddController( def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index 23db85c..cecef7b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ --device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 -usb +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml index 1aa5455..d7fb90c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml @@ -15,7 +15,6 @@ <devices> <emulator>/usr/libexec/qemu-kvm</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='usb' index='0'/> <memballoon model='none'/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index ddff6f0..6c24407 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,7 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ --usb \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index aba0f88..0068d27 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -995,11 +995,13 @@ mymain(void) DO_TEST("pci-bridge-many-disks", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pcie-root", + QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE); DO_TEST("q35", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml index 25c77f1..f10e85b 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/libexec/qemu-kvm</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='usb' index='0'/> + <controller type='sata' index='0'/> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> <controller type='pci' index='2' model='pci-bridge'/> <memballoon model='none'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml new file mode 100644 index 0000000..2a86e61 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='18432' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 8b4590a..5c6730d 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -295,7 +295,7 @@ mymain(void) DO_TEST_DIFFERENT("pci-autoadd-addr"); DO_TEST_DIFFERENT("pci-autoadd-idx"); DO_TEST_DIFFERENT("pcie-root"); - DO_TEST("q35"); + DO_TEST_DIFFERENT("q35");
DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 1.7.11.7
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list I'm going to hold off on saying ACK here due to my question above, but we might not even need a code change if my understanding is wrong.
On Mon, Aug 5, 2013 at 3:37 AM, Laine Stump <laine@laine.org> wrote:
On 08/04/2013 07:53 PM, Doug Goldstein wrote:
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote:
This patch adds in special handling for a few devices that need to be treated differently for q35 domains:
usb - there is no implicit/default usb controller for the q35 machinetype. This is done because normally the default usb controller is added to a domain by just adding "-usb" to the qemu commandline, and it's assumed that this will add a single piix3 usb1 controller at slot 1 function 2. That's not what happens when the machinetype is q35, though. Instead, adding -usb to the commandline adds 3 usb (version 2) controllers to the domain at slot 0x1D.{1,2,7}. Rather than having
<controller type='usb' index='0'/>
translate into 3 separate devices on the PCI bus, it's cleaner to not automatically add a default usb device; one can always be added explicitly if desired. Or we may decide that on q35 machines, 3 usb controllers will be automatically added when none is given. But for this initial commit, at least we aren't locking ourselves into something we later won't want.
video - for pc machine types, the primary video device is always in slot 2, and that slot is reserved even when no video device has been specified. On q35, when you specify "-vga qxl" on the qemu commandline, the vga device is put in slot 1, not 2. Assuming that this was done for a reason, this patch always puts the primary video for q35 machines in slot 1, and reserves slot 1 even if there is no video. Might be worth updating the comments here to say that QEMU will assign it in the first available slot, which with q35 is slot 1 and i440fx (pc) is slot 2 (due to the always present IDE controller).
I see you caught my question (and the later discussion) on qemu-devel :-) yeah, I think it would be reasonable to change this comment, now that I understand better what is happening.
sata - a q35 machine always has a sata controller implicitly added at slot 0x1F, function 2. There is no way to avoid this controller, so we always add it. Note that the xml2xml tests for the pcie-root and q35 cases were changed to use DO_TEST_DIFFERENT() so that we can check for the sata controller being automatically added. This is especially important because we can't check for it in the xml2argv output (it has no effect on that output since it's an implicit device). The note about pcie-root switching to DO_TEST_DIFFERENT should go into the earlier patch.
Done.
ide - q35 has no ide controllers.
isa and smbus controllers - these two are always present in a q35 (at slot 0x1F functions 0 and 3) but we have no way of modelling them in our config. We do need to reserve those functions so that the user doesn't attempt to put anything else there though. ---
+ if (def->nvideos > 0) { + /* NB: unlike the pc machinetypes, q35 machinetypes put the primary VGA + * at slot 1 for some reason. + */ + virDomainVideoDefPtr primaryVideo = def->videos[0]; + if (primaryVideo->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + primaryVideo->info.addr.pci.domain = 0; + primaryVideo->info.addr.pci.bus = 0; + primaryVideo->info.addr.pci.slot = 1; + primaryVideo->info.addr.pci.function = 0; + addrptr = &primaryVideo->info.addr.pci; + + if (!qemuDomainPCIAddressValidate(addrs, addrptr, flags)) + goto error; + + if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { + if (qemuDeviceVideoUsable) { + virResetLastError(); + if (qemuDomainPCIAddressReserveNextSlot(addrs, + &primaryVideo->info, + flags) < 0) + goto error; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("PCI address 0:0:1.0 is in use, " + "QEMU needs it for primary video")); + goto error; + } + } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) { + goto error; + } + } else if (!qemuDeviceVideoUsable) { + if (primaryVideo->info.addr.pci.domain != 0 || + primaryVideo->info.addr.pci.bus != 0 || + primaryVideo->info.addr.pci.slot != 1 || + primaryVideo->info.addr.pci.function != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Primary video card must have PCI address 0:0:1.0")); + goto error; + } + /* If TYPE==PCI, then qemuCollectPCIAddress() function + * has already reserved the address, so we must skip */ + } + } else if (addrs->nbuses && !qemuDeviceVideoUsable) { + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 1; + + if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { + VIR_DEBUG("PCI address 0:0:1.0 in use, future addition of a video" + " device will not be possible without manual" + " intervention"); + virResetLastError(); + } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) { + goto error; + } + } Is this really necessary/correct? From the qemu-devel thread it really seems like the VGA controller will just take the first available slot but is it not possible for us to stick this somewhere other than slot 1? I know with current libvirt and qemu we limit ourselves to -vga qxl rather than -device qxl-vga due to a bug in QEMU but I think with QEMU 1.6, that limitation goes away so that might free us up to move it around.
Unfortunately, I think that if we want to avoid surprises with the location of the video device then we have to do this. The problem is that not everybody has qemu 1.6+ (actually at this point *almost nobody* does). If we don't reserve "the first" slot (be it 1 or 2) for the video adapter when the domain is created, and continue to watch out for it as long as no video is added, another device would probably go in that place, and when the user finally did get around to adding a device, it would go in "some other" indeterminate place (well, we could figure it out, but it would be much more inconvenient that simply reserving slot 1 or 2 on every domain for video)
I may be taking this too seriously though - does anyone else want to chime in one way or another?
I think you're probably right and we want to keep this check in. Obviously we want libvirt to do the right thing for users and not give them a bunch of bits that they need to sort themselves so this check helps users along the right path and helps get a consistent environment for different QEMU binaries so keep it.
+ return 0; + +error: + return -1; +} + + /* * This assigns static PCI slots to all configured devices. * The ordering here is chosen to match the ordering used @@ -2365,6 +2504,11 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, goto error; }
+ if (qemuDomainMachineIsQ35(def) && + qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) { + goto error; + } + /* PCI controllers */ for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { @@ -7676,6 +7820,9 @@ qemuBuildCommandLine(virConnectPtr conn, _("SATA is not supported with this " "QEMU binary")); goto error; + } else if (cont->idx == 0 && qemuDomainMachineIsQ35(def)) { + /* first SATA controller on Q35 machines is implicit */ + continue; } else { char *devstr;
@@ -7689,6 +7836,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == -1 && + !qemuDomainMachineIsQ35(def) && (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || def->os.arch == VIR_ARCH_PPC64)) { if (usblegacy) { @@ -7713,7 +7861,7 @@ qemuBuildCommandLine(virConnectPtr conn, } }
- if (usbcontroller == 0) + if (usbcontroller == 0 && !qemuDomainMachineIsQ35(def)) virCommandAddArg(cmd, "-usb");
for (i = 0; i < def->nhubs; i++) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f5030cd..75be591 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -700,6 +700,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, void *opaque ATTRIBUTE_UNUSED) { bool addDefaultUSB = true; + bool addImplicitSATA = false; bool addPCIRoot = false; bool addPCIeRoot = false;
@@ -722,6 +723,7 @@ qemuDomainDefPostParse(virDomainDefPtr def, STREQ(def->os.machine, "q35")) { addPCIeRoot = true; addDefaultUSB = false; + addImplicitSATA = true; break; } if (!STRPREFIX(def->os.machine, "pc-0.") && @@ -754,6 +756,11 @@ qemuDomainDefPostParse(virDomainDefPtr def, def, VIR_DOMAIN_CONTROLLER_TYPE_USB, 0, -1) < 0) return -1;
+ if (addImplicitSATA && + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_SATA, 0, -1) < 0) + return -1; + if (addPCIRoot && virDomainDefMaybeAddController( def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index 23db85c..cecef7b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ --device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 -usb +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml index 1aa5455..d7fb90c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml @@ -15,7 +15,6 @@ <devices> <emulator>/usr/libexec/qemu-kvm</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='usb' index='0'/> <memballoon model='none'/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index ddff6f0..6c24407 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,7 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \ +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ --usb \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index aba0f88..0068d27 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -995,11 +995,13 @@ mymain(void) DO_TEST("pci-bridge-many-disks", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE_PCI_BRIDGE); DO_TEST("pcie-root", + QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE); DO_TEST("q35", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml index 25c77f1..f10e85b 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/libexec/qemu-kvm</emulator> <controller type='pci' index='0' model='pcie-root'/> - <controller type='usb' index='0'/> + <controller type='sata' index='0'/> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> <controller type='pci' index='2' model='pci-bridge'/> <memballoon model='none'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml new file mode 100644 index 0000000..2a86e61 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'/> + <controller type='pci' index='2' model='pci-bridge'/> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='18432' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 8b4590a..5c6730d 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -295,7 +295,7 @@ mymain(void) DO_TEST_DIFFERENT("pci-autoadd-addr"); DO_TEST_DIFFERENT("pci-autoadd-idx"); DO_TEST_DIFFERENT("pcie-root"); - DO_TEST("q35"); + DO_TEST_DIFFERENT("q35");
DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 1.7.11.7
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list I'm going to hold off on saying ACK here due to my question above, but we might not even need a code change if my understanding is wrong.
-- Doug Goldstein
We had been setting the device alias in the devinceinfo for pci controllers to "pci%u", but then hardcoding "pci.%u" when creating the device address for other devices using that pci bus. This all worked just fine until we encountered the built-in "pcie.0" bus (the PCIe root complex) in Q35 machines. In order to create the correct commandline for this one case, this patch: 1) sets the alias for PCI controllers correctly, to "pci.%u" (or "pcie.%u" for the pcie-root controller) 2) eliminates the hardcoded "pci.%u" for pci controllers when generatuing device address strings, and instead uses the controller's alias. 3) plumbs a pointer to the virDomainDef all the way down to qemuBuildDeviceAddressStr. This was necessary in order to make the aliase of the controller *used by a device* available (previously qemuBuildDeviceAddressStr only had the deviceinfo of the device itself, *not* of the controller it was connecting to). This made for a larger than desired diff, but at least in the future we won't have to do it again, since all the information we could possibly ever need for future enhancements is in the virDomainDef. (right?) This should be done for *all* controllers, but for now we just do it in the case of PCI controllers, to reduce the likelyhood of regression. --- src/qemu/qemu_command.c | 164 ++++++++++++++------- src/qemu/qemu_command.h | 28 ++-- src/qemu/qemu_hotplug.c | 6 +- tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- 5 files changed, 136 insertions(+), 66 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a6d6819..3353c61 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -860,10 +860,18 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) { const char *prefix = virDomainControllerTypeToString(controller->type); - if (virAsprintf(&controller->info.alias, "%s%d", prefix, - controller->idx) < 0) - return -1; - return 0; + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + /* only pcie-root uses a different naming convention + * ("pcie.0"), because it is hardcoded that way in qemu. All + * other buses use the consistent "pci.%u". + */ + if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return virAsprintf(&controller->info.alias, "pcie.%d", controller->idx); + else + return virAsprintf(&controller->info.alias, "pci.%d", controller->idx); + } + + return virAsprintf(&controller->info.alias, "%s%d", prefix, controller->idx); } static ssize_t @@ -2781,22 +2789,57 @@ qemuUsbId(virBufferPtr buf, int idx) static int qemuBuildDeviceAddressStr(virBufferPtr buf, + virDomainDefPtr domainDef, virDomainDeviceInfoPtr info, virQEMUCapsPtr qemuCaps) { + int ret = -1; + char *devStr = NULL; + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + const char *contAlias = NULL; + size_t i; + + if (!(devStr = qemuDomainPCIAddressAsString(&info->addr.pci))) + goto cleanup; + for (i = 0; i < domainDef->ncontrollers; i++) { + virDomainControllerDefPtr cont = domainDef->controllers[i]; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + cont->idx == info->addr.pci.bus) { + contAlias = cont->info.alias; + if (!contAlias) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Device alias was not set for PCI " + "controller with index %u required " + "for device at address %s"), + info->addr.pci.bus, devStr); + goto cleanup; + } + break; + } + } + if (!contAlias) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find PCI " + "controller with index %u required " + "for device at address %s"), + info->addr.pci.bus, devStr); + goto cleanup; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIFUNCTION)) { if (info->addr.pci.function != 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only PCI device addresses with function=0 " "are supported with this QEMU binary")); - return -1; + goto cleanup; } if (info->addr.pci.multi == VIR_DEVICE_ADDRESS_PCI_MULTI_ON) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("'multifunction=on' is not supported with " "this QEMU binary")); - return -1; + goto cleanup; } } @@ -2810,18 +2853,19 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, */ if (info->addr.pci.bus != 0) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { - virBufferAsprintf(buf, ",bus=pci.%u", info->addr.pci.bus); + virBufferAsprintf(buf, ",bus=%s", contAlias); } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Multiple PCI buses are not supported " "with this QEMU binary")); - return -1; + goto cleanup; } } else { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIBUS)) - virBufferAddLit(buf, ",bus=pci.0"); - else + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIBUS)) { + virBufferAsprintf(buf, ",bus=%s", contAlias); + } else { virBufferAddLit(buf, ",bus=pci"); + } } if (info->addr.pci.multi == VIR_DEVICE_ADDRESS_PCI_MULTI_ON) virBufferAddLit(buf, ",multifunction=on"); @@ -2845,7 +2889,10 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, info->addr.ccw.devno); } - return 0; + ret = 0; +cleanup: + VIR_FREE(devStr); + return ret; } static int @@ -4179,13 +4226,13 @@ qemuBuildDriveDevStr(virDomainDefPtr def, (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) ? "on" : "off"); } - if (qemuBuildDeviceAddressStr(&opt, &disk->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps) < 0) goto error; break; case VIR_DOMAIN_DISK_BUS_USB: virBufferAddLit(&opt, "usb-storage"); - if (qemuBuildDeviceAddressStr(&opt, &disk->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps) < 0) goto error; break; default: @@ -4309,7 +4356,8 @@ error: char * -qemuBuildFSDevStr(virDomainFSDefPtr fs, +qemuBuildFSDevStr(virDomainDefPtr def, + virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps) { virBuffer opt = VIR_BUFFER_INITIALIZER; @@ -4325,7 +4373,7 @@ qemuBuildFSDevStr(virDomainFSDefPtr fs, virBufferAsprintf(&opt, ",fsdev=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias); virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst); - if (qemuBuildDeviceAddressStr(&opt, &fs->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) goto error; if (virBufferError(&opt)) { @@ -4545,7 +4593,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, if (def->queues) virBufferAsprintf(&buf, ",num_queues=%u", def->queues); - if (qemuBuildDeviceAddressStr(&buf, &def->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, domainDef, &def->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -4583,7 +4631,8 @@ qemuBuildNicStr(virDomainNetDefPtr net, char * -qemuBuildNicDevStr(virDomainNetDefPtr net, +qemuBuildNicDevStr(virDomainDefPtr def, + virDomainNetDefPtr net, int vlan, int bootindex, virQEMUCapsPtr qemuCaps) @@ -4645,7 +4694,7 @@ qemuBuildNicDevStr(virDomainNetDefPtr net, virBufferAsprintf(&buf, ",id=%s", net->info.alias); virBufferAsprintf(&buf, ",mac=%s", virMacAddrFormat(&net->mac, macaddr)); - if (qemuBuildDeviceAddressStr(&buf, &net->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &net->info, qemuCaps) < 0) goto error; if (qemuBuildRomStr(&buf, &net->info, qemuCaps) < 0) goto error; @@ -4800,7 +4849,8 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, char * -qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, +qemuBuildWatchdogDevStr(virDomainDefPtr def, + virDomainWatchdogDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4813,7 +4863,7 @@ qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, } virBufferAsprintf(&buf, "%s,id=%s", model, dev->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -4830,7 +4880,8 @@ error: char * -qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev, +qemuBuildMemballoonDevStr(virDomainDefPtr def, + virDomainMemballoonDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4850,7 +4901,7 @@ qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev, } virBufferAsprintf(&buf, ",id=%s", dev->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -4893,7 +4944,8 @@ error: } char * -qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, +qemuBuildUSBInputDevStr(virDomainDefPtr def, + virDomainInputDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4902,7 +4954,7 @@ qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, dev->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ? "usb-mouse" : "usb-tablet", dev->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -4919,7 +4971,8 @@ error: char * -qemuBuildSoundDevStr(virDomainSoundDefPtr sound, +qemuBuildSoundDevStr(virDomainDefPtr def, + virDomainSoundDefPtr sound, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4940,7 +4993,7 @@ qemuBuildSoundDevStr(virDomainSoundDefPtr sound, model = "intel-hda"; virBufferAsprintf(&buf, "%s,id=%s", model, sound->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &sound->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &sound->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -5000,7 +5053,8 @@ error: } static char * -qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, +qemuBuildDeviceVideoStr(virDomainDefPtr def, + virDomainVideoDefPtr video, virQEMUCapsPtr qemuCaps, bool primary) { @@ -5054,7 +5108,7 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, virBufferAsprintf(&buf, ",vram_size=%u", video->vram * 1024); } - if (qemuBuildDeviceAddressStr(&buf, &video->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &video->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -5094,7 +5148,9 @@ qemuOpenPCIConfig(virDomainHostdevDefPtr dev) } char * -qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, const char *configfd, +qemuBuildPCIHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, + const char *configfd, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5114,7 +5170,7 @@ qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, const char *configfd, virBufferAsprintf(&buf, ",id=%s", dev->info->alias); if (dev->info->bootIndex) virBufferAsprintf(&buf, ",bootindex=%d", dev->info->bootIndex); - if (qemuBuildDeviceAddressStr(&buf, dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0) goto error; if (qemuBuildRomStr(&buf, dev->info, qemuCaps) < 0) goto error; @@ -5220,7 +5276,7 @@ qemuBuildRedirdevDevStr(virDomainDefPtr def, virBufferAsprintf(&buf, ",bootindex=%d", dev->info.bootIndex); } - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -5236,7 +5292,8 @@ error: } char * -qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, +qemuBuildUSBHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5259,7 +5316,7 @@ qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, if (dev->info->bootIndex) virBufferAsprintf(&buf, ",bootindex=%d", dev->info->bootIndex); - if (qemuBuildDeviceAddressStr(&buf, dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -5276,7 +5333,8 @@ error: char * -qemuBuildHubDevStr(virDomainHubDefPtr dev, +qemuBuildHubDevStr(virDomainDefPtr def, + virDomainHubDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5296,7 +5354,7 @@ qemuBuildHubDevStr(virDomainHubDefPtr dev, virBufferAddLit(&buf, "usb-hub"); virBufferAsprintf(&buf, ",id=%s", dev->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -5817,6 +5875,7 @@ cleanup: static int qemuBuildRNGDeviceArgs(virCommandPtr cmd, + virDomainDefPtr def, virDomainRNGDefPtr dev, virQEMUCapsPtr qemuCaps) { @@ -5846,7 +5905,7 @@ qemuBuildRNGDeviceArgs(virCommandPtr cmd, virBufferAddLit(&buf, ",period=1000"); } - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto cleanup; virCommandAddArg(cmd, "-device"); @@ -7121,7 +7180,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, virCommandAddArgList(cmd, "-netdev", host, NULL); } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (!(nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps))) + if (!(nic = qemuBuildNicDevStr(def, net, vlan, bootindex, qemuCaps))) goto cleanup; virCommandAddArgList(cmd, "-device", nic, NULL); } else { @@ -7869,7 +7928,7 @@ qemuBuildCommandLine(virConnectPtr conn, char *optstr; virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildHubDevStr(hub, qemuCaps))) + if (!(optstr = qemuBuildHubDevStr(def, hub, qemuCaps))) goto error; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); @@ -8090,7 +8149,7 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(optstr); virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildFSDevStr(fs, qemuCaps))) + if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps))) goto error; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); @@ -8448,7 +8507,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { char *optstr; virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildUSBInputDevStr(input, qemuCaps))) + if (!(optstr = qemuBuildUSBInputDevStr(def, input, qemuCaps))) goto error; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); @@ -8505,7 +8564,7 @@ qemuBuildCommandLine(virConnectPtr conn, for (i = 0; i < def->nvideos; i++) { char *str; virCommandAddArg(cmd, "-device"); - if (!(str = qemuBuildDeviceVideoStr(def->videos[i], qemuCaps, !i))) + if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i], qemuCaps, !i))) goto error; virCommandAddArg(cmd, str); @@ -8579,7 +8638,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-device"); - if (!(str = qemuBuildDeviceVideoStr(def->videos[i], qemuCaps, false))) + if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i], qemuCaps, false))) goto error; virCommandAddArg(cmd, str); @@ -8643,7 +8702,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL); } else { virCommandAddArg(cmd, "-device"); - if (!(str = qemuBuildSoundDevStr(sound, qemuCaps))) + if (!(str = qemuBuildSoundDevStr(def, sound, qemuCaps))) goto error; virCommandAddArg(cmd, str); @@ -8719,7 +8778,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { virCommandAddArg(cmd, "-device"); - optstr = qemuBuildWatchdogDevStr(watchdog, qemuCaps); + optstr = qemuBuildWatchdogDevStr(def, watchdog, qemuCaps); if (!optstr) goto error; } else { @@ -8832,7 +8891,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildUSBHostdevDevStr(hostdev, qemuCaps))) + if (!(devstr = qemuBuildUSBHostdevDevStr(def, hostdev, qemuCaps))) goto error; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); @@ -8880,7 +8939,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } virCommandAddArg(cmd, "-device"); - devstr = qemuBuildPCIHostdevDevStr(hostdev, configfd_name, qemuCaps); + devstr = qemuBuildPCIHostdevDevStr(def, hostdev, configfd_name, qemuCaps); VIR_FREE(configfd_name); if (!devstr) goto error; @@ -9009,7 +9068,7 @@ qemuBuildCommandLine(virConnectPtr conn, char *optstr; virCommandAddArg(cmd, "-device"); - optstr = qemuBuildMemballoonDevStr(def->memballoon, qemuCaps); + optstr = qemuBuildMemballoonDevStr(def, def->memballoon, qemuCaps); if (!optstr) goto error; virCommandAddArg(cmd, optstr); @@ -9025,7 +9084,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; /* add the device */ - if (qemuBuildRNGDeviceArgs(cmd, def->rng, qemuCaps) < 0) + if (qemuBuildRNGDeviceArgs(cmd, def, def->rng, qemuCaps) < 0) goto error; } @@ -9103,6 +9162,7 @@ error: */ static int qemuBuildSerialChrDeviceStr(char **deviceStr, + virDomainDefPtr def, virDomainChrDefPtr serial, virQEMUCapsPtr qemuCaps, virArch arch, @@ -9115,7 +9175,7 @@ qemuBuildSerialChrDeviceStr(char **deviceStr, serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { virBufferAsprintf(&cmd, "spapr-vty,chardev=char%s", serial->info.alias); - if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0) goto error; } } else { @@ -9137,7 +9197,7 @@ qemuBuildSerialChrDeviceStr(char **deviceStr, goto error; } - if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0) goto error; } } @@ -9252,7 +9312,7 @@ qemuBuildChrDeviceStr(char **deviceStr, switch ((enum virDomainChrDeviceType) chr->deviceType) { case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: - ret = qemuBuildSerialChrDeviceStr(deviceStr, chr, qemuCaps, + ret = qemuBuildSerialChrDeviceStr(deviceStr, vmdef, chr, qemuCaps, vmdef->os.arch, vmdef->os.machine); break; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index e5111d2..5c5c025 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -98,7 +98,8 @@ char * qemuBuildNicStr(virDomainNetDefPtr net, int vlan); /* Current, best practice */ -char * qemuBuildNicDevStr(virDomainNetDefPtr net, +char * qemuBuildNicDevStr(virDomainDefPtr def, + virDomainNetDefPtr net, int vlan, int bootindex, virQEMUCapsPtr qemuCaps); @@ -119,7 +120,8 @@ char * qemuBuildDriveDevStr(virDomainDefPtr def, virDomainDiskDefPtr disk, int bootindex, virQEMUCapsPtr qemuCaps); -char * qemuBuildFSDevStr(virDomainFSDefPtr fs, +char * qemuBuildFSDevStr(virDomainDefPtr domainDef, + virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps); /* Current, best practice */ char * qemuBuildControllerDevStr(virDomainDefPtr domainDef, @@ -127,22 +129,27 @@ char * qemuBuildControllerDevStr(virDomainDefPtr domainDef, virQEMUCapsPtr qemuCaps, int *nusbcontroller); -char * qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, +char * qemuBuildWatchdogDevStr(virDomainDefPtr domainDef, + virDomainWatchdogDefPtr dev, virQEMUCapsPtr qemuCaps); -char * qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev, +char * qemuBuildMemballoonDevStr(virDomainDefPtr domainDef, + virDomainMemballoonDefPtr dev, virQEMUCapsPtr qemuCaps); -char * qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, +char * qemuBuildUSBInputDevStr(virDomainDefPtr domainDef, + virDomainInputDefPtr dev, virQEMUCapsPtr qemuCaps); -char * qemuBuildSoundDevStr(virDomainSoundDefPtr sound, +char * qemuBuildSoundDevStr(virDomainDefPtr domainDef, + virDomainSoundDefPtr sound, virQEMUCapsPtr qemuCaps); /* Legacy, pre device support */ char * qemuBuildPCIHostdevPCIDevStr(virDomainHostdevDefPtr dev); /* Current, best practice */ -char * qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, +char * qemuBuildPCIHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, const char *configfd, virQEMUCapsPtr qemuCaps); @@ -151,7 +158,8 @@ int qemuOpenPCIConfig(virDomainHostdevDefPtr dev); /* Legacy, pre device support */ char * qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev); /* Current, best practice */ -char * qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, +char * qemuBuildUSBHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps); char * qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, @@ -162,7 +170,9 @@ char * qemuBuildSCSIHostdevDevStr(virDomainDefPtr def, virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps); -char * qemuBuildHubDevStr(virDomainHubDefPtr dev, virQEMUCapsPtr qemuCaps); +char * qemuBuildHubDevStr(virDomainDefPtr def, + virDomainHubDefPtr dev, + virQEMUCapsPtr qemuCaps); char * qemuBuildRedirdevDevStr(virDomainDefPtr def, virDomainRedirdevDefPtr dev, virQEMUCapsPtr qemuCaps); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 032de69..7a6946e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -859,7 +859,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, } if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - if (!(nicstr = qemuBuildNicDevStr(net, vlan, 0, priv->qemuCaps))) + if (!(nicstr = qemuBuildNicDevStr(vm->def, net, vlan, 0, priv->qemuCaps))) goto try_remove; } else { if (!(nicstr = qemuBuildNicStr(net, NULL, vlan))) @@ -1057,7 +1057,7 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, goto error; } - if (!(devstr = qemuBuildPCIHostdevDevStr(hostdev, configfd_name, + if (!(devstr = qemuBuildPCIHostdevDevStr(vm->def, hostdev, configfd_name, priv->qemuCaps))) goto error; @@ -1302,7 +1302,7 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) goto cleanup; - if (!(devstr = qemuBuildUSBHostdevDevStr(hostdev, priv->qemuCaps))) + if (!(devstr = qemuBuildUSBHostdevDevStr(vm->def, hostdev, priv->qemuCaps))) goto cleanup; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index cecef7b..84428f9 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index 6c24407..5ff4bc7 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,6 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 -- 1.7.11.7
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote:
We had been setting the device alias in the devinceinfo for pci controllers to "pci%u", but then hardcoding "pci.%u" when creating the device address for other devices using that pci bus. This all worked just fine until we encountered the built-in "pcie.0" bus (the PCIe root complex) in Q35 machines.
In order to create the correct commandline for this one case, this patch:
1) sets the alias for PCI controllers correctly, to "pci.%u" (or "pcie.%u" for the pcie-root controller)
2) eliminates the hardcoded "pci.%u" for pci controllers when generatuing device address strings, and instead uses the controller's alias.
3) plumbs a pointer to the virDomainDef all the way down to qemuBuildDeviceAddressStr. This was necessary in order to make the aliase of the controller *used by a device* available (previously qemuBuildDeviceAddressStr only had the deviceinfo of the device itself, *not* of the controller it was connecting to). This made for a larger than desired diff, but at least in the future we won't have to do it again, since all the information we could possibly ever need for future enhancements is in the virDomainDef. (right?)
This should be done for *all* controllers, but for now we just do it in the case of PCI controllers, to reduce the likelyhood of regression. --- src/qemu/qemu_command.c | 164 ++++++++++++++------- src/qemu/qemu_command.h | 28 ++-- src/qemu/qemu_hotplug.c | 6 +- tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- 5 files changed, 136 insertions(+), 66 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a6d6819..3353c61 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -860,10 +860,18 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) { const char *prefix = virDomainControllerTypeToString(controller->type);
- if (virAsprintf(&controller->info.alias, "%s%d", prefix, - controller->idx) < 0) - return -1; - return 0; + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + /* only pcie-root uses a different naming convention + * ("pcie.0"), because it is hardcoded that way in qemu. All + * other buses use the consistent "pci.%u". + */ + if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return virAsprintf(&controller->info.alias, "pcie.%d", controller->idx); + else + return virAsprintf(&controller->info.alias, "pci.%d", controller->idx); + } + + return virAsprintf(&controller->info.alias, "%s%d", prefix, controller->idx); }
static ssize_t @@ -2781,22 +2789,57 @@ qemuUsbId(virBufferPtr buf, int idx)
static int qemuBuildDeviceAddressStr(virBufferPtr buf, + virDomainDefPtr domainDef, virDomainDeviceInfoPtr info, virQEMUCapsPtr qemuCaps) { + int ret = -1; + char *devStr = NULL; + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + const char *contAlias = NULL; + size_t i; + + if (!(devStr = qemuDomainPCIAddressAsString(&info->addr.pci))) + goto cleanup; + for (i = 0; i < domainDef->ncontrollers; i++) { + virDomainControllerDefPtr cont = domainDef->controllers[i]; + + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + cont->idx == info->addr.pci.bus) { + contAlias = cont->info.alias; + if (!contAlias) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Device alias was not set for PCI " + "controller with index %u required " + "for device at address %s"), + info->addr.pci.bus, devStr); + goto cleanup; + } + break; + } + } + if (!contAlias) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find PCI " + "controller with index %u required " + "for device at address %s"), + info->addr.pci.bus, devStr); + goto cleanup; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIFUNCTION)) { if (info->addr.pci.function != 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only PCI device addresses with function=0 " "are supported with this QEMU binary")); - return -1; + goto cleanup; } if (info->addr.pci.multi == VIR_DEVICE_ADDRESS_PCI_MULTI_ON) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("'multifunction=on' is not supported with " "this QEMU binary")); - return -1; + goto cleanup; } }
@@ -2810,18 +2853,19 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, */ if (info->addr.pci.bus != 0) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { - virBufferAsprintf(buf, ",bus=pci.%u", info->addr.pci.bus); + virBufferAsprintf(buf, ",bus=%s", contAlias); } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Multiple PCI buses are not supported " "with this QEMU binary")); - return -1; + goto cleanup; } } else { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIBUS)) - virBufferAddLit(buf, ",bus=pci.0"); - else + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIBUS)) { + virBufferAsprintf(buf, ",bus=%s", contAlias); + } else { virBufferAddLit(buf, ",bus=pci"); + } } if (info->addr.pci.multi == VIR_DEVICE_ADDRESS_PCI_MULTI_ON) virBufferAddLit(buf, ",multifunction=on"); @@ -2845,7 +2889,10 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, info->addr.ccw.devno); }
- return 0; + ret = 0; +cleanup: + VIR_FREE(devStr); + return ret; }
static int @@ -4179,13 +4226,13 @@ qemuBuildDriveDevStr(virDomainDefPtr def, (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) ? "on" : "off"); } - if (qemuBuildDeviceAddressStr(&opt, &disk->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps) < 0) goto error; break; case VIR_DOMAIN_DISK_BUS_USB: virBufferAddLit(&opt, "usb-storage");
- if (qemuBuildDeviceAddressStr(&opt, &disk->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps) < 0) goto error; break; default: @@ -4309,7 +4356,8 @@ error:
char * -qemuBuildFSDevStr(virDomainFSDefPtr fs, +qemuBuildFSDevStr(virDomainDefPtr def, + virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps) { virBuffer opt = VIR_BUFFER_INITIALIZER; @@ -4325,7 +4373,7 @@ qemuBuildFSDevStr(virDomainFSDefPtr fs, virBufferAsprintf(&opt, ",fsdev=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias); virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst);
- if (qemuBuildDeviceAddressStr(&opt, &fs->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) goto error;
if (virBufferError(&opt)) { @@ -4545,7 +4593,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, if (def->queues) virBufferAsprintf(&buf, ",num_queues=%u", def->queues);
- if (qemuBuildDeviceAddressStr(&buf, &def->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, domainDef, &def->info, qemuCaps) < 0) goto error;
if (virBufferError(&buf)) { @@ -4583,7 +4631,8 @@ qemuBuildNicStr(virDomainNetDefPtr net,
char * -qemuBuildNicDevStr(virDomainNetDefPtr net, +qemuBuildNicDevStr(virDomainDefPtr def, + virDomainNetDefPtr net, int vlan, int bootindex, virQEMUCapsPtr qemuCaps) @@ -4645,7 +4694,7 @@ qemuBuildNicDevStr(virDomainNetDefPtr net, virBufferAsprintf(&buf, ",id=%s", net->info.alias); virBufferAsprintf(&buf, ",mac=%s", virMacAddrFormat(&net->mac, macaddr)); - if (qemuBuildDeviceAddressStr(&buf, &net->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &net->info, qemuCaps) < 0) goto error; if (qemuBuildRomStr(&buf, &net->info, qemuCaps) < 0) goto error; @@ -4800,7 +4849,8 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
char * -qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, +qemuBuildWatchdogDevStr(virDomainDefPtr def, + virDomainWatchdogDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4813,7 +4863,7 @@ qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, }
virBufferAsprintf(&buf, "%s,id=%s", model, dev->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error;
if (virBufferError(&buf)) { @@ -4830,7 +4880,8 @@ error:
char * -qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev, +qemuBuildMemballoonDevStr(virDomainDefPtr def, + virDomainMemballoonDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4850,7 +4901,7 @@ qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev, }
virBufferAsprintf(&buf, ",id=%s", dev->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error;
if (virBufferError(&buf)) { @@ -4893,7 +4944,8 @@ error: }
char * -qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, +qemuBuildUSBInputDevStr(virDomainDefPtr def, + virDomainInputDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4902,7 +4954,7 @@ qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, dev->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ? "usb-mouse" : "usb-tablet", dev->info.alias);
- if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error;
if (virBufferError(&buf)) { @@ -4919,7 +4971,8 @@ error:
char * -qemuBuildSoundDevStr(virDomainSoundDefPtr sound, +qemuBuildSoundDevStr(virDomainDefPtr def, + virDomainSoundDefPtr sound, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -4940,7 +4993,7 @@ qemuBuildSoundDevStr(virDomainSoundDefPtr sound, model = "intel-hda";
virBufferAsprintf(&buf, "%s,id=%s", model, sound->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &sound->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &sound->info, qemuCaps) < 0) goto error;
if (virBufferError(&buf)) { @@ -5000,7 +5053,8 @@ error: }
static char * -qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, +qemuBuildDeviceVideoStr(virDomainDefPtr def, + virDomainVideoDefPtr video, virQEMUCapsPtr qemuCaps, bool primary) { @@ -5054,7 +5108,7 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, virBufferAsprintf(&buf, ",vram_size=%u", video->vram * 1024); }
- if (qemuBuildDeviceAddressStr(&buf, &video->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &video->info, qemuCaps) < 0) goto error;
if (virBufferError(&buf)) { @@ -5094,7 +5148,9 @@ qemuOpenPCIConfig(virDomainHostdevDefPtr dev) }
char * -qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, const char *configfd, +qemuBuildPCIHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, + const char *configfd, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5114,7 +5170,7 @@ qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, const char *configfd, virBufferAsprintf(&buf, ",id=%s", dev->info->alias); if (dev->info->bootIndex) virBufferAsprintf(&buf, ",bootindex=%d", dev->info->bootIndex); - if (qemuBuildDeviceAddressStr(&buf, dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0) goto error; if (qemuBuildRomStr(&buf, dev->info, qemuCaps) < 0) goto error; @@ -5220,7 +5276,7 @@ qemuBuildRedirdevDevStr(virDomainDefPtr def, virBufferAsprintf(&buf, ",bootindex=%d", dev->info.bootIndex); }
- if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error;
if (virBufferError(&buf)) { @@ -5236,7 +5292,8 @@ error: }
char * -qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, +qemuBuildUSBHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5259,7 +5316,7 @@ qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, if (dev->info->bootIndex) virBufferAsprintf(&buf, ",bootindex=%d", dev->info->bootIndex);
- if (qemuBuildDeviceAddressStr(&buf, dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0) goto error;
if (virBufferError(&buf)) { @@ -5276,7 +5333,8 @@ error:
char * -qemuBuildHubDevStr(virDomainHubDefPtr dev, +qemuBuildHubDevStr(virDomainDefPtr def, + virDomainHubDefPtr dev, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5296,7 +5354,7 @@ qemuBuildHubDevStr(virDomainHubDefPtr dev,
virBufferAddLit(&buf, "usb-hub"); virBufferAsprintf(&buf, ",id=%s", dev->info.alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error;
if (virBufferError(&buf)) { @@ -5817,6 +5875,7 @@ cleanup:
static int qemuBuildRNGDeviceArgs(virCommandPtr cmd, + virDomainDefPtr def, virDomainRNGDefPtr dev, virQEMUCapsPtr qemuCaps) { @@ -5846,7 +5905,7 @@ qemuBuildRNGDeviceArgs(virCommandPtr cmd, virBufferAddLit(&buf, ",period=1000"); }
- if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto cleanup;
virCommandAddArg(cmd, "-device"); @@ -7121,7 +7180,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, virCommandAddArgList(cmd, "-netdev", host, NULL); } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - if (!(nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps))) + if (!(nic = qemuBuildNicDevStr(def, net, vlan, bootindex, qemuCaps))) goto cleanup; virCommandAddArgList(cmd, "-device", nic, NULL); } else { @@ -7869,7 +7928,7 @@ qemuBuildCommandLine(virConnectPtr conn, char *optstr;
virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildHubDevStr(hub, qemuCaps))) + if (!(optstr = qemuBuildHubDevStr(def, hub, qemuCaps))) goto error; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); @@ -8090,7 +8149,7 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(optstr);
virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildFSDevStr(fs, qemuCaps))) + if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps))) goto error; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); @@ -8448,7 +8507,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { char *optstr; virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildUSBInputDevStr(input, qemuCaps))) + if (!(optstr = qemuBuildUSBInputDevStr(def, input, qemuCaps))) goto error; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); @@ -8505,7 +8564,7 @@ qemuBuildCommandLine(virConnectPtr conn, for (i = 0; i < def->nvideos; i++) { char *str; virCommandAddArg(cmd, "-device"); - if (!(str = qemuBuildDeviceVideoStr(def->videos[i], qemuCaps, !i))) + if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i], qemuCaps, !i))) goto error;
virCommandAddArg(cmd, str); @@ -8579,7 +8638,7 @@ qemuBuildCommandLine(virConnectPtr conn,
virCommandAddArg(cmd, "-device");
- if (!(str = qemuBuildDeviceVideoStr(def->videos[i], qemuCaps, false))) + if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i], qemuCaps, false))) goto error;
virCommandAddArg(cmd, str); @@ -8643,7 +8702,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL); } else { virCommandAddArg(cmd, "-device"); - if (!(str = qemuBuildSoundDevStr(sound, qemuCaps))) + if (!(str = qemuBuildSoundDevStr(def, sound, qemuCaps))) goto error;
virCommandAddArg(cmd, str); @@ -8719,7 +8778,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { virCommandAddArg(cmd, "-device");
- optstr = qemuBuildWatchdogDevStr(watchdog, qemuCaps); + optstr = qemuBuildWatchdogDevStr(def, watchdog, qemuCaps); if (!optstr) goto error; } else { @@ -8832,7 +8891,7 @@ qemuBuildCommandLine(virConnectPtr conn,
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildUSBHostdevDevStr(hostdev, qemuCaps))) + if (!(devstr = qemuBuildUSBHostdevDevStr(def, hostdev, qemuCaps))) goto error; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); @@ -8880,7 +8939,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } virCommandAddArg(cmd, "-device"); - devstr = qemuBuildPCIHostdevDevStr(hostdev, configfd_name, qemuCaps); + devstr = qemuBuildPCIHostdevDevStr(def, hostdev, configfd_name, qemuCaps); VIR_FREE(configfd_name); if (!devstr) goto error; @@ -9009,7 +9068,7 @@ qemuBuildCommandLine(virConnectPtr conn, char *optstr; virCommandAddArg(cmd, "-device");
- optstr = qemuBuildMemballoonDevStr(def->memballoon, qemuCaps); + optstr = qemuBuildMemballoonDevStr(def, def->memballoon, qemuCaps); if (!optstr) goto error; virCommandAddArg(cmd, optstr); @@ -9025,7 +9084,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto error;
/* add the device */ - if (qemuBuildRNGDeviceArgs(cmd, def->rng, qemuCaps) < 0) + if (qemuBuildRNGDeviceArgs(cmd, def, def->rng, qemuCaps) < 0) goto error; }
@@ -9103,6 +9162,7 @@ error: */ static int qemuBuildSerialChrDeviceStr(char **deviceStr, + virDomainDefPtr def, virDomainChrDefPtr serial, virQEMUCapsPtr qemuCaps, virArch arch, @@ -9115,7 +9175,7 @@ qemuBuildSerialChrDeviceStr(char **deviceStr, serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { virBufferAsprintf(&cmd, "spapr-vty,chardev=char%s", serial->info.alias); - if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0) goto error; } } else { @@ -9137,7 +9197,7 @@ qemuBuildSerialChrDeviceStr(char **deviceStr, goto error; }
- if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0) goto error; } } @@ -9252,7 +9312,7 @@ qemuBuildChrDeviceStr(char **deviceStr,
switch ((enum virDomainChrDeviceType) chr->deviceType) { case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: - ret = qemuBuildSerialChrDeviceStr(deviceStr, chr, qemuCaps, + ret = qemuBuildSerialChrDeviceStr(deviceStr, vmdef, chr, qemuCaps, vmdef->os.arch, vmdef->os.machine); break; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index e5111d2..5c5c025 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -98,7 +98,8 @@ char * qemuBuildNicStr(virDomainNetDefPtr net, int vlan);
/* Current, best practice */ -char * qemuBuildNicDevStr(virDomainNetDefPtr net, +char * qemuBuildNicDevStr(virDomainDefPtr def, + virDomainNetDefPtr net, int vlan, int bootindex, virQEMUCapsPtr qemuCaps); @@ -119,7 +120,8 @@ char * qemuBuildDriveDevStr(virDomainDefPtr def, virDomainDiskDefPtr disk, int bootindex, virQEMUCapsPtr qemuCaps); -char * qemuBuildFSDevStr(virDomainFSDefPtr fs, +char * qemuBuildFSDevStr(virDomainDefPtr domainDef, + virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps); /* Current, best practice */ char * qemuBuildControllerDevStr(virDomainDefPtr domainDef, @@ -127,22 +129,27 @@ char * qemuBuildControllerDevStr(virDomainDefPtr domainDef, virQEMUCapsPtr qemuCaps, int *nusbcontroller);
-char * qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, +char * qemuBuildWatchdogDevStr(virDomainDefPtr domainDef, + virDomainWatchdogDefPtr dev, virQEMUCapsPtr qemuCaps);
-char * qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev, +char * qemuBuildMemballoonDevStr(virDomainDefPtr domainDef, + virDomainMemballoonDefPtr dev, virQEMUCapsPtr qemuCaps);
-char * qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, +char * qemuBuildUSBInputDevStr(virDomainDefPtr domainDef, + virDomainInputDefPtr dev, virQEMUCapsPtr qemuCaps);
-char * qemuBuildSoundDevStr(virDomainSoundDefPtr sound, +char * qemuBuildSoundDevStr(virDomainDefPtr domainDef, + virDomainSoundDefPtr sound, virQEMUCapsPtr qemuCaps);
/* Legacy, pre device support */ char * qemuBuildPCIHostdevPCIDevStr(virDomainHostdevDefPtr dev); /* Current, best practice */ -char * qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, +char * qemuBuildPCIHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, const char *configfd, virQEMUCapsPtr qemuCaps);
@@ -151,7 +158,8 @@ int qemuOpenPCIConfig(virDomainHostdevDefPtr dev); /* Legacy, pre device support */ char * qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev); /* Current, best practice */ -char * qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, +char * qemuBuildUSBHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps);
char * qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, @@ -162,7 +170,9 @@ char * qemuBuildSCSIHostdevDevStr(virDomainDefPtr def, virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps);
-char * qemuBuildHubDevStr(virDomainHubDefPtr dev, virQEMUCapsPtr qemuCaps); +char * qemuBuildHubDevStr(virDomainDefPtr def, + virDomainHubDefPtr dev, + virQEMUCapsPtr qemuCaps); char * qemuBuildRedirdevDevStr(virDomainDefPtr def, virDomainRedirdevDefPtr dev, virQEMUCapsPtr qemuCaps); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 032de69..7a6946e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -859,7 +859,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, }
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - if (!(nicstr = qemuBuildNicDevStr(net, vlan, 0, priv->qemuCaps))) + if (!(nicstr = qemuBuildNicDevStr(vm->def, net, vlan, 0, priv->qemuCaps))) goto try_remove; } else { if (!(nicstr = qemuBuildNicStr(net, NULL, vlan))) @@ -1057,7 +1057,7 @@ int qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, goto error; }
- if (!(devstr = qemuBuildPCIHostdevDevStr(hostdev, configfd_name, + if (!(devstr = qemuBuildPCIHostdevDevStr(vm->def, hostdev, configfd_name, priv->qemuCaps))) goto error;
@@ -1302,7 +1302,7 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) goto cleanup; - if (!(devstr = qemuBuildUSBHostdevDevStr(hostdev, priv->qemuCaps))) + if (!(devstr = qemuBuildUSBHostdevDevStr(vm->def, hostdev, priv->qemuCaps))) goto cleanup; }
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index cecef7b..84428f9 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index 6c24407..5ff4bc7 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,6 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x2 \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 -- 1.7.11.7
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Straight forward so ACK. This could probably be re-ordered before 4/7 if you want to merge in the ACK'd patches. -- Doug Goldstein
Current seabios doesn't recognize any disk device that is attached anywhere behind an i82801b11-bridge, so it can't boot from them. (It's possible/probable that seabios simply doesn't recognize that bridge at all, so *no* devices behind it are available to the bios). In order to allow booting from any disk no matter where it is attached, this patch flouts the hardware specs (which say that only PCIe devices can be connected directly to a PCIe bus such as the q35 machine's "pcie.0"), taking advantage of the fact that qemu also ignores this rule, and installs a stnadard pci-bridge device anywhere the config requests a "dmi-to-pci-bridge (which had been setup to use i82801b11-bridge). Because this isn't really the way things should be, the original code is left in place, and the new code is put inside an #ifdef indicating why it is there. --- src/qemu/qemu_command.c | 16 ++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3353c61..460144f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4571,7 +4571,23 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, _("dmi-to-pci-bridge index should be > 0")); goto error; } +#define SEABIOS_NO_I82801B11_SUPPORT +#if defined SEABIOS_NO_I82801B11_SUPPORT + /* since seabios currently doesn't recognize boot devices + * that are anywhere behind an i82801b11-bridge, we take + * advantage of the fact that qemu erroneously + * (generously) allows us to plug a pci-bridge into a PCIe + * slot, and create a pci-bridge to take the place of the + * i82801b11-bridge. When seabios is fixed, we will figure + * out the best way to determine whether or not the fix is + * present, and switch to the more proper device when + * appropriate. + */ + virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=pci.%d", + def->idx, def->idx); +#else virBufferAsprintf(&buf, "i82801b11-bridge,id=pci.%d", def->idx); +#endif break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index 84428f9..da970db 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index 5ff4bc7..a0ec66e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,6 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 -- 1.7.11.7
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote:
Current seabios doesn't recognize any disk device that is attached anywhere behind an i82801b11-bridge, so it can't boot from them. (It's possible/probable that seabios simply doesn't recognize that bridge at all, so *no* devices behind it are available to the bios). In order to allow booting from any disk no matter where it is attached, this patch flouts the hardware specs (which say that only PCIe devices can be connected directly to a PCIe bus such as the q35 machine's "pcie.0"), taking advantage of the fact that qemu also ignores this rule, and installs a stnadard pci-bridge device anywhere the config requests a "dmi-to-pci-bridge (which had been setup to use i82801b11-bridge).
Because this isn't really the way things should be, the original code is left in place, and the new code is put inside an #ifdef indicating why it is there. --- src/qemu/qemu_command.c | 16 ++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- 3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3353c61..460144f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4571,7 +4571,23 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, _("dmi-to-pci-bridge index should be > 0")); goto error; } +#define SEABIOS_NO_I82801B11_SUPPORT +#if defined SEABIOS_NO_I82801B11_SUPPORT + /* since seabios currently doesn't recognize boot devices + * that are anywhere behind an i82801b11-bridge, we take + * advantage of the fact that qemu erroneously + * (generously) allows us to plug a pci-bridge into a PCIe + * slot, and create a pci-bridge to take the place of the + * i82801b11-bridge. When seabios is fixed, we will figure + * out the best way to determine whether or not the fix is + * present, and switch to the more proper device when + * appropriate. + */ + virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=pci.%d", + def->idx, def->idx); +#else virBufferAsprintf(&buf, "i82801b11-bridge,id=pci.%d", def->idx); +#endif break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index 84428f9..da970db 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -1,5 +1,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index 5ff4bc7..a0ec66e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,6 +1,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 -- 1.7.11.7
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
While this certainly works, I'll wait until Eric or Dan weigh in on migration concerns with this approach as well as correctness (QEMU allowing you to attach PCI devices to PCIe buses and vis versa). If this approach is the one that goes forward, then consider this an ACK of this patch. -- Doug Goldstein
q35 machines have an implicit ahci (sata) controller at 00:1F.2 which has no "id" associated with it. For this reason, we can't refer to it as "ahci0". Instead, we don't give an id on the commandline, which qemu interprets as "use the first ahci controller". We then need to specify the unit with "unit=%d" rather than adding it onto the bus arg. --- src/qemu/qemu_command.c | 23 ++++++++++++++++++++--- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 ++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 5 +++++ tests/qemuxml2argvtest.c | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 5 +++++ 5 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 460144f..cb25659 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4199,9 +4199,26 @@ qemuBuildDriveDevStr(virDomainDefPtr def, virBufferAddLit(&opt, "ide-drive"); } - virBufferAsprintf(&opt, ",bus=ahci%d.%d", - disk->info.addr.drive.controller, - disk->info.addr.drive.unit); + if (qemuDomainMachineIsQ35(def) && + disk->info.addr.drive.controller == 0) { + /* Q35 machines have an implicit ahci (sata) controller at + * 00:1F.2 which has no "id" associated with it. For this + * reason, we can't refer to it as "ahci0". Instead, we + * don't give an id, which qemu interprets as "use the + * first ahci controller". We then need to specify the + * unit with "unit=%d" rather than adding it onto the bus + * arg. + */ + virBufferAsprintf(&opt, ",unit=%d", disk->info.addr.drive.unit); + } else { + /* All other ahci controllers have been created by + * libvirt, so they *do* have an id, and we can identify + * them that way. + */ + virBufferAsprintf(&opt, ",bus=ahci%d.%d", + disk->info.addr.drive.controller, + disk->info.addr.drive.unit); + } break; case VIR_DOMAIN_DISK_BUS_VIRTIO: if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index a0ec66e..6b30b4d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -3,4 +3,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ -device pci-bridge,chassis_nr=1,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ +-device ide-drive,unit=0,drive=drive-sata0-0-0,id=sata0-0-0 \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml index 3541b14..edaf6cb 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml @@ -14,6 +14,11 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> <controller type='pci' index='0' model='pcie-root'/> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> <controller type='pci' index='2' model='pci-bridge'/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0068d27..679124e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1001,7 +1001,7 @@ mymain(void) DO_TEST("q35", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_DRIVE, QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml index 2a86e61..96f8eaf 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -14,6 +14,11 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> <controller type='pci' index='0' model='pcie-root'/> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> <controller type='pci' index='2' model='pci-bridge'/> -- 1.7.11.7
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote:
q35 machines have an implicit ahci (sata) controller at 00:1F.2 which has no "id" associated with it. For this reason, we can't refer to it as "ahci0". Instead, we don't give an id on the commandline, which qemu interprets as "use the first ahci controller". We then need to specify the unit with "unit=%d" rather than adding it onto the bus arg.
Maybe its called ich9-ahci0 instead of ahci0? Or ide0 or ide1? The reason I ask is this code in QEMU. /* ahci and SATA device, for q35 1 ahci controller is built-in */ ahci = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_SATA1_DEV, ICH9_SATA1_FUNC), true, "ich9-ahci"); idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0"); idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1"); Which is from the q35 bring up code.
--- src/qemu/qemu_command.c | 23 ++++++++++++++++++++--- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 ++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 5 +++++ tests/qemuxml2argvtest.c | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 5 +++++ 5 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 460144f..cb25659 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4199,9 +4199,26 @@ qemuBuildDriveDevStr(virDomainDefPtr def, virBufferAddLit(&opt, "ide-drive"); }
- virBufferAsprintf(&opt, ",bus=ahci%d.%d", - disk->info.addr.drive.controller, - disk->info.addr.drive.unit); + if (qemuDomainMachineIsQ35(def) && + disk->info.addr.drive.controller == 0) { + /* Q35 machines have an implicit ahci (sata) controller at + * 00:1F.2 which has no "id" associated with it. For this + * reason, we can't refer to it as "ahci0". Instead, we + * don't give an id, which qemu interprets as "use the + * first ahci controller". We then need to specify the + * unit with "unit=%d" rather than adding it onto the bus + * arg. + */ + virBufferAsprintf(&opt, ",unit=%d", disk->info.addr.drive.unit); + } else { + /* All other ahci controllers have been created by + * libvirt, so they *do* have an id, and we can identify + * them that way. + */ + virBufferAsprintf(&opt, ",bus=ahci%d.%d", + disk->info.addr.drive.controller, + disk->info.addr.drive.unit); + } break; case VIR_DOMAIN_DISK_BUS_VIRTIO: if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index a0ec66e..6b30b4d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -3,4 +3,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ -device pci-bridge,chassis_nr=1,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ +-device ide-drive,unit=0,drive=drive-sata0-0-0,id=sata0-0-0 \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml index 3541b14..edaf6cb 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml @@ -14,6 +14,11 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> <controller type='pci' index='0' model='pcie-root'/> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> <controller type='pci' index='2' model='pci-bridge'/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0068d27..679124e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1001,7 +1001,7 @@ mymain(void) DO_TEST("q35", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_DRIVE, QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml index 2a86e61..96f8eaf 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -14,6 +14,11 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> <controller type='pci' index='0' model='pcie-root'/> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> <controller type='pci' index='2' model='pci-bridge'/> -- 1.7.11.7
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Had some questions above so holding off on ACK. -- Doug Goldstein
On 08/04/2013 07:55 PM, Doug Goldstein wrote:
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote:
q35 machines have an implicit ahci (sata) controller at 00:1F.2 which has no "id" associated with it. For this reason, we can't refer to it as "ahci0". Instead, we don't give an id on the commandline, which qemu interprets as "use the first ahci controller". We then need to specify the unit with "unit=%d" rather than adding it onto the bus arg. Maybe its called ich9-ahci0 instead of ahci0? Or ide0 or ide1? The reason I ask is this code in QEMU.
/* ahci and SATA device, for q35 1 ahci controller is built-in */ ahci = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_SATA1_DEV, ICH9_SATA1_FUNC), true, "ich9-ahci"); idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0"); idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
Which is from the q35 bring up code.
I'm not familiar enough the the qemu code to know where the "ich9-ahci" string is going, but my information source was running "virsh qemu-monitor-command --pretty $domain '{"execute":"query-pci"}'" on a running domain, then looking at the "qdev_id" attribute of the device. That attribute is '' on the sata controller, but (for example) "pci.2" on the pci-bridge, and "ahci1" on a 2nd sata controller that is added by libvirt. Andreas - does this implicit sata device really have no "id"? If not, then your suggestion of omiting the id for the first controller (I'm remembering correctly that it was you, right?) seems to work just fine (although I agree with Doug that it makes the code feel a bit "dirty").
--- src/qemu/qemu_command.c | 23 ++++++++++++++++++++--- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 ++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 5 +++++ tests/qemuxml2argvtest.c | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 5 +++++ 5 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 460144f..cb25659 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4199,9 +4199,26 @@ qemuBuildDriveDevStr(virDomainDefPtr def, virBufferAddLit(&opt, "ide-drive"); }
- virBufferAsprintf(&opt, ",bus=ahci%d.%d", - disk->info.addr.drive.controller, - disk->info.addr.drive.unit); + if (qemuDomainMachineIsQ35(def) && + disk->info.addr.drive.controller == 0) { + /* Q35 machines have an implicit ahci (sata) controller at + * 00:1F.2 which has no "id" associated with it. For this + * reason, we can't refer to it as "ahci0". Instead, we + * don't give an id, which qemu interprets as "use the + * first ahci controller". We then need to specify the + * unit with "unit=%d" rather than adding it onto the bus + * arg. + */ + virBufferAsprintf(&opt, ",unit=%d", disk->info.addr.drive.unit); + } else { + /* All other ahci controllers have been created by + * libvirt, so they *do* have an id, and we can identify + * them that way. + */ + virBufferAsprintf(&opt, ",bus=ahci%d.%d", + disk->info.addr.drive.controller, + disk->info.addr.drive.unit); + } break; case VIR_DOMAIN_DISK_BUS_VIRTIO: if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index a0ec66e..6b30b4d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -3,4 +3,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ -device pci-bridge,chassis_nr=1,id=pci.1,bus=pcie.0,addr=0x2 \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ +-device ide-drive,unit=0,drive=drive-sata0-0-0,id=sata0-0-0 \ -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml index 3541b14..edaf6cb 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml @@ -14,6 +14,11 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> <controller type='pci' index='0' model='pcie-root'/> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> <controller type='pci' index='2' model='pci-bridge'/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0068d27..679124e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1001,7 +1001,7 @@ mymain(void) DO_TEST("q35", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_DRIVE, QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml index 2a86e61..96f8eaf 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -14,6 +14,11 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/libexec/qemu-kvm</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> <controller type='pci' index='0' model='pcie-root'/> <controller type='pci' index='1' model='dmi-to-pci-bridge'/> <controller type='pci' index='2' model='pci-bridge'/> -- 1.7.11.7
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list Had some questions above so holding off on ACK.
Am 05.08.2013 09:40, schrieb Laine Stump:
On 08/04/2013 07:55 PM, Doug Goldstein wrote:
On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine@laine.org> wrote:
q35 machines have an implicit ahci (sata) controller at 00:1F.2 which has no "id" associated with it. For this reason, we can't refer to it as "ahci0". Instead, we don't give an id on the commandline, which qemu interprets as "use the first ahci controller". We then need to specify the unit with "unit=%d" rather than adding it onto the bus arg. Maybe its called ich9-ahci0 instead of ahci0? Or ide0 or ide1? The reason I ask is this code in QEMU.
/* ahci and SATA device, for q35 1 ahci controller is built-in */ ahci = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_SATA1_DEV, ICH9_SATA1_FUNC), true, "ich9-ahci"); idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0"); idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
Which is from the q35 bring up code.
I'm not familiar enough the the qemu code to know where the "ich9-ahci" string is going, but my information source was running "virsh qemu-monitor-command --pretty $domain '{"execute":"query-pci"}'" on a running domain, then looking at the "qdev_id" attribute of the device. That attribute is '' on the sata controller, but (for example) "pci.2" on the pci-bridge, and "ahci1" on a 2nd sata controller that is added by libvirt.
Andreas - does this implicit sata device really have no "id"? If not, then your suggestion of omiting the id for the first controller (I'm remembering correctly that it was you, right?) seems to work just fine (although I agree with Doug that it makes the code feel a bit "dirty").
The code snippet above (which I have not further verified) indicates two busses, named "ide.0" and "ide.1" respectively. So bus=ide.0 or bus=ide.1 should work, just like pcie.0 works for the built-in PCIe host bridge. "ich9-ahci" is the type name of the device, which you can use with -device and device-add or see in qom-list-types. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
This patch addresses two concerns with the error reporting when an incompatible PCI address is specified for a device: 1) It wasn't always apparent which device had the problem. With this patch applied, any error about an incompatible address will always contain the full address as given in the config, so it will be easier to determine which device's config aused the problem. 2) In some cases when the problem came from bad config, the error message was erroneously classified as VIR_ERR_INTERNAL_ERROR. With this patch applied, the same error message will be changed to indicate either "internal" or "xml" error depending on whether the address came from the config, or was automatically generated by libvirt. Note that in the case of "internal" (due to bad auto-generation) errors, the PCI address won't be of much use in finding the location in config to change (because it was automatically generated). Of course that makes perfect sense, but still the address could provide a clue about a bug in libvirt attempting to use a type of pci bus that doesn't have its flags set correctly (or something similar). In other words, it's not perfect, but it is definitely better. --- src/qemu/qemu_command.c | 224 +++++++++++++++++++++++++++++------------------- 1 file changed, 138 insertions(+), 86 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index af0dd27..75a3388 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1445,10 +1445,15 @@ struct _qemuDomainPCIAddressSet { static bool qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr, + char *addrStr, qemuDomainPCIConnectFlags busFlags, qemuDomainPCIConnectFlags devFlags, - bool reportError) + bool reportError, + bool fromConfig) { + virErrorNumber errType = (fromConfig + ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR); + /* If this bus doesn't allow the type of connection (PCI * vs. PCIe) required by the device, or if the device requires * hot-plug and this bus doesn't have it, return false. @@ -1456,24 +1461,24 @@ qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr, if (!(devFlags & busFlags & QEMU_PCI_CONNECT_TYPES_MASK)) { if (reportError) { if (devFlags & QEMU_PCI_CONNECT_TYPE_PCI) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("PCI bus %.4x:%.2x is not compatible with the " - "device. Device requires a standard PCI slot, " - "which is not provided by this bus"), - addr->domain, addr->bus); + virReportError(errType, + _("PCI bus is not compatible with the device " + "at %s. Device requires a standard PCI slot, " + "which is not provided by bus %.4x:%.2x"), + addrStr, addr->domain, addr->bus); } else if (devFlags & QEMU_PCI_CONNECT_TYPE_PCIE) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("PCI bus %.4x:%.2x is not compatible with the " - "device. Device requires a PCI Express slot, " - "which is not provided by this bus"), - addr->domain, addr->bus); + virReportError(errType, + _("PCI bus is not compatible with the device " + "at %s. Device requires a PCI Express slot, " + "which is not provided by bus %.4x:%.2x"), + addrStr, addr->domain, addr->bus); } else { /* this should never happen. If it does, there is a * bug in the code that sets the flag bits for devices. */ - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("The device information has no PCI " - "connection types listed")); + virReportError(errType, + _("The device information for %s has no PCI " + "connection types listed"), addrStr); } } return false; @@ -1481,11 +1486,11 @@ qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr, if ((devFlags & QEMU_PCI_CONNECT_HOTPLUGGABLE) && !(busFlags & QEMU_PCI_CONNECT_HOTPLUGGABLE)) { if (reportError) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("PCI bus %.4x:%.2x is not compatible with the " - "device. Device requires hot-plug capability, " - "which is not provided by the bus"), - addr->domain, addr->bus); + virReportError(errType, + _("PCI bus is not compatible with the device " + "at %s. Device requires hot-plug capability, " + "which is not provided by bus %.4x:%.2x"), + addrStr, addr->domain, addr->bus); } return false; } @@ -1500,23 +1505,30 @@ qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr, static bool qemuDomainPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs, virDevicePCIAddressPtr addr, - qemuDomainPCIConnectFlags flags) + char *addrStr, + qemuDomainPCIConnectFlags flags, + bool fromConfig) { qemuDomainPCIAddressBusPtr bus; + virErrorNumber errType = (fromConfig + ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR); if (addrs->nbuses == 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available")); + virReportError(errType, "%s", _("No PCI buses available")); return false; } if (addr->domain != 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Only PCI domain 0 is available")); + virReportError(errType, + _("Invalid PCI address %s. " + "Only PCI domain 0 is available"), + addrStr); return false; } if (addr->bus >= addrs->nbuses) { - virReportError(VIR_ERR_XML_ERROR, - _("Only PCI buses up to %zu are available"), - addrs->nbuses - 1); + virReportError(errType, + _("Invalid PCI address %s. " + "Only PCI buses up to %zu are available"), + addrStr, addrs->nbuses - 1); return false; } @@ -1525,26 +1537,27 @@ qemuDomainPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs, /* assure that at least one of the requested connection types is * provided by this bus */ - if (!qemuDomainPCIAddressFlagsCompatible(addr, bus->flags, flags, true)) + if (!qemuDomainPCIAddressFlagsCompatible(addr, addrStr, bus->flags, + flags, true, fromConfig)) return false; /* some "buses" are really just a single port */ if (bus->minSlot && addr->slot < bus->minSlot) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid PCI address: slot must be >= %zu"), - bus->minSlot); + virReportError(errType, + _("Invalid PCI address %s. slot must be >= %zu"), + addrStr, bus->minSlot); return false; } if (addr->slot > bus->maxSlot) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid PCI address: slot must be <= %zu"), - bus->maxSlot); + virReportError(errType, + _("Invalid PCI address %s. slot must be <= %zu"), + addrStr, bus->maxSlot); return false; } if (addr->function > QEMU_PCI_ADDRESS_FUNCTION_LAST) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid PCI address: function must be <= %u"), - QEMU_PCI_ADDRESS_FUNCTION_LAST); + virReportError(errType, + _("Invalid PCI address %s. function must be <= %u"), + addrStr, QEMU_PCI_ADDRESS_FUNCTION_LAST); return false; } return true; @@ -1953,12 +1966,12 @@ qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, bool fromConfig) { int ret = -1; - char *str = NULL; + char *addrStr = NULL; qemuDomainPCIAddressBusPtr bus; virErrorNumber errType = (fromConfig ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR); - if (!(str = qemuDomainPCIAddressAsString(addr))) + if (!(addrStr = qemuDomainPCIAddressAsString(addr))) goto cleanup; /* Add an extra bus if necessary */ @@ -1967,7 +1980,7 @@ qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, /* Check that the requested bus exists, is the correct type, and we * are asking for a valid slot */ - if (!qemuDomainPCIAddressValidate(addrs, addr, flags)) + if (!qemuDomainPCIAddressValidate(addrs, addr, addrStr, flags, fromConfig)) goto cleanup; bus = &addrs->buses[addr->bus]; @@ -1977,31 +1990,32 @@ qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, virReportError(errType, _("Attempted double use of PCI slot %s " "(may need \"multifunction='on'\" for " - "device on function 0)"), str); + "device on function 0)"), addrStr); goto cleanup; } bus->slots[addr->slot] = 0xFF; /* reserve all functions of slot */ - VIR_DEBUG("Reserving PCI slot %s (multifunction='off')", str); + VIR_DEBUG("Reserving PCI slot %s (multifunction='off')", addrStr); } else { if (bus->slots[addr->slot] & (1 << addr->function)) { if (addr->function == 0) { virReportError(errType, - _("Attempted double use of PCI Address %s"), str); + _("Attempted double use of PCI Address %s"), + addrStr); } else { virReportError(errType, _("Attempted double use of PCI Address %s " "(may need \"multifunction='on'\" " - "for device on function 0)"), str); + "for device on function 0)"), addrStr); } goto cleanup; } bus->slots[addr->slot] |= (1 << addr->function); - VIR_DEBUG("Reserving PCI address %s", str); + VIR_DEBUG("Reserving PCI address %s", addrStr); } ret = 0; cleanup: - VIR_FREE(str); + VIR_FREE(addrStr); return ret; } @@ -2017,7 +2031,8 @@ qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs, int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, virDomainDeviceInfoPtr dev) { - int ret = 0; + int ret = -1; + char *addrStr = NULL; /* 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 @@ -2026,6 +2041,9 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI); + if (!(addrStr = qemuDomainPCIAddressAsString(&dev->addr.pci))) + goto cleanup; + if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { /* We do not support hotplug multi-function PCI device now, so we should * reserve the whole slot. The function of the PCI device must be 0. @@ -2034,16 +2052,20 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Only PCI device addresses with function=0" " are supported")); - return -1; + goto cleanup; } - if (!qemuDomainPCIAddressValidate(addrs, &dev->addr.pci, flags)) - return -1; + if (!qemuDomainPCIAddressValidate(addrs, &dev->addr.pci, + addrStr, flags, true)) + goto cleanup; ret = qemuDomainPCIAddressReserveSlot(addrs, &dev->addr.pci, flags); } else { ret = qemuDomainPCIAddressReserveNextSlot(addrs, dev, flags); } + +cleanup: + VIR_FREE(addrStr); return ret; } @@ -2063,12 +2085,20 @@ qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, * already had it, and are giving it back. */ qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPES_MASK; + int ret = -1; + char *addrStr = NULL; - if (!qemuDomainPCIAddressValidate(addrs, addr, flags)) - return -1; + if (!(addrStr = qemuDomainPCIAddressAsString(addr))) + goto cleanup; + + if (!qemuDomainPCIAddressValidate(addrs, addr, addrStr, flags, false)) + goto cleanup; addrs->buses[addr->bus].slots[addr->slot] = 0; - return 0; + ret = 0; +cleanup: + VIR_FREE(addrStr); + return ret; } void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs) @@ -2090,6 +2120,7 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, * 0000:00:00.0 */ virDevicePCIAddress a = { 0, 0, 0, 0, false }; + char *addrStr = NULL; /* except if this search is for the exact same type of device as * last time, continue the search from the previous match @@ -2099,13 +2130,17 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, if (addrs->nbuses == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available")); - return -1; + goto error; } /* 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)) { + addrStr = NULL; + if (!(addrStr = qemuDomainPCIAddressAsString(&a))) + goto error; + if (!qemuDomainPCIAddressFlagsCompatible(&a, addrStr, + addrs->buses[a.bus].flags, + flags, false, false)) { VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device", a.domain, a.bus); continue; @@ -2124,13 +2159,17 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, if (addrs->dryRun) { /* a is already set to the first new bus and slot 1 */ if (qemuDomainPCIAddressSetGrow(addrs, &a, flags) < 0) - return -1; + goto error; goto success; } 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)) { + addrStr = NULL; + if (!(addrStr = qemuDomainPCIAddressAsString(&a))) + goto error; + if (!qemuDomainPCIAddressFlagsCompatible(&a, addrStr, + addrs->buses[a.bus].flags, + flags, false, false)) { VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device", a.domain, a.bus); continue; @@ -2147,12 +2186,15 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No more available PCI slots")); +error: + VIR_FREE(addrStr); return -1; success: VIR_DEBUG("Found free PCI slot %.4x:%.2x:%.2x", a.domain, a.bus, a.slot); *next_addr = a; + VIR_FREE(addrStr); return 0; } @@ -2217,10 +2259,12 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, qemuDomainPCIAddressSetPtr addrs) { + int ret = -1; size_t i; virDevicePCIAddress tmp_addr; bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); virDevicePCIAddressPtr addrptr; + char *addrStr = NULL; qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI; /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */ @@ -2235,7 +2279,7 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def, def->controllers[i]->info.addr.pci.function != 1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Primary IDE controller must have PCI address 0:0:1.1")); - goto error; + goto cleanup; } } else { def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; @@ -2255,7 +2299,7 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def, def->controllers[i]->info.addr.pci.function != 2) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PIIX3 USB controller must have PCI address 0:0:1.2")); - goto error; + goto cleanup; } } else { def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; @@ -2274,7 +2318,7 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def, memset(&tmp_addr, 0, sizeof(tmp_addr)); tmp_addr.slot = 1; if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) - goto error; + goto cleanup; } if (def->nvideos > 0) { @@ -2287,8 +2331,11 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def, primaryVideo->info.addr.pci.function = 0; addrptr = &primaryVideo->info.addr.pci; - if (!qemuDomainPCIAddressValidate(addrs, addrptr, flags)) - goto error; + if (!(addrStr = qemuDomainPCIAddressAsString(addrptr))) + goto cleanup; + if (!qemuDomainPCIAddressValidate(addrs, addrptr, + addrStr, flags, false)) + goto cleanup; if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { if (qemuDeviceVideoUsable) { @@ -2296,15 +2343,15 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def, if (qemuDomainPCIAddressReserveNextSlot(addrs, &primaryVideo->info, flags) < 0) - goto error; + goto cleanup; } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PCI address 0:0:2.0 is in use, " "QEMU needs it for primary video")); - goto error; + goto cleanup; } } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) { - goto error; + goto cleanup; } } else if (!qemuDeviceVideoUsable) { if (primaryVideo->info.addr.pci.domain != 0 || @@ -2313,7 +2360,7 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def, primaryVideo->info.addr.pci.function != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Primary video card must have PCI address 0:0:2.0")); - goto error; + goto cleanup; } /* If TYPE==PCI, then qemuCollectPCIAddress() function * has already reserved the address, so we must skip */ @@ -2328,13 +2375,13 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def, " intervention"); virResetLastError(); } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) { - goto error; + goto cleanup; } } - return 0; - -error: - return -1; + ret = 0; +cleanup: + VIR_FREE(addrStr); + return ret; } @@ -2351,10 +2398,12 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, qemuDomainPCIAddressSetPtr addrs) { + int ret = -1; size_t i; virDevicePCIAddress tmp_addr; bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); virDevicePCIAddressPtr addrptr; + char *addrStr = NULL; qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPE_PCIE; /* Verify that the first SATA controller is at 00:1F.2 */ @@ -2369,7 +2418,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, def->controllers[i]->info.addr.pci.function != 2) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Primary SATA controller must have PCI address 0:0:1f.2")); - goto error; + goto cleanup; } } else { def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; @@ -2393,12 +2442,12 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, tmp_addr.multi = 1; if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, false, false) < 0) - goto error; + goto cleanup; tmp_addr.function = 3; tmp_addr.multi = 0; if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, false, false) < 0) - goto error; + goto cleanup; } if (def->nvideos > 0) { @@ -2414,8 +2463,11 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, primaryVideo->info.addr.pci.function = 0; addrptr = &primaryVideo->info.addr.pci; - if (!qemuDomainPCIAddressValidate(addrs, addrptr, flags)) - goto error; + if (!(addrStr = qemuDomainPCIAddressAsString(addrptr))) + goto cleanup; + if (!qemuDomainPCIAddressValidate(addrs, addrptr, + addrStr, flags, false)) + goto cleanup; if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) { if (qemuDeviceVideoUsable) { @@ -2423,15 +2475,15 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, if (qemuDomainPCIAddressReserveNextSlot(addrs, &primaryVideo->info, flags) < 0) - goto error; + goto cleanup; } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PCI address 0:0:1.0 is in use, " "QEMU needs it for primary video")); - goto error; + goto cleanup; } } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) { - goto error; + goto cleanup; } } else if (!qemuDeviceVideoUsable) { if (primaryVideo->info.addr.pci.domain != 0 || @@ -2440,7 +2492,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, primaryVideo->info.addr.pci.function != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Primary video card must have PCI address 0:0:1.0")); - goto error; + goto cleanup; } /* If TYPE==PCI, then qemuCollectPCIAddress() function * has already reserved the address, so we must skip */ @@ -2455,13 +2507,13 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, " intervention"); virResetLastError(); } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) { - goto error; + goto cleanup; } } - return 0; - -error: - return -1; + ret = 0; +cleanup: + VIR_FREE(addrStr); + return ret; } -- 1.7.11.7
participants (3)
-
Andreas Färber -
Doug Goldstein -
Laine Stump