This makes the range and static host array management in virNetworkDHCPDefParseXML() more similar to what is done in virNetworkDefUpdateIPDHCPRange() and virNetworkDefUpdateIPDHCPHost() - they use VIR_APPEND_ELEMENT rather than a combination of VIR_REALLOC_N() and separate incrementing of the array size. The one functional change here is that a memory leak of the contents of the last (unsuccessful) virNetworkDHCPHostDef was previously leaked in certain failure conditions, but it is now properly cleaned up. --- new in V2 src/conf/network_conf.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 31d4463..25b5b81 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -997,33 +997,32 @@ virNetworkDHCPDefParseXML(const char *networkName, xmlNodePtr node, virNetworkIpDefPtr def) { - + int ret = -1; xmlNodePtr cur; + virSocketAddrRange range; + virNetworkDHCPHostDef host; + + memset(&range, 0, sizeof(range)); + memset(&host, 0, sizeof(host)); cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "range")) { - if (VIR_REALLOC_N(def->ranges, def->nranges + 1) < 0) - return -1; - if (virSocketAddrRangeParseXML(networkName, def, cur, - &def->ranges[def->nranges]) < 0) { - return -1; - } - def->nranges++; + if (virSocketAddrRangeParseXML(networkName, def, cur, &range) < 0) + goto cleanup; + if (VIR_APPEND_ELEMENT(def->ranges, def->nranges, range) < 0) + goto cleanup; } else if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "host")) { - if (VIR_REALLOC_N(def->hosts, def->nhosts + 1) < 0) - return -1; if (virNetworkDHCPHostDefParseXML(networkName, def, cur, - &def->hosts[def->nhosts], - false) < 0) { - return -1; - } - def->nhosts++; + &host, false) < 0) + goto cleanup; + if (VIR_APPEND_ELEMENT(def->hosts, def->nhosts, host) < 0) + goto cleanup; } else if (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) && cur->type == XML_ELEMENT_NODE && @@ -1043,7 +1042,7 @@ virNetworkDHCPDefParseXML(const char *networkName, virSocketAddrParse(&inaddr, server, AF_UNSPEC) < 0) { VIR_FREE(file); VIR_FREE(server); - return -1; + goto cleanup; } def->bootfile = file; @@ -1054,7 +1053,10 @@ virNetworkDHCPDefParseXML(const char *networkName, cur = cur->next; } - return 0; + ret = 0; + cleanup: + virNetworkDHCPHostDefClear(&host); + return ret; } static int -- 2.1.0

BTW, in case anyone is hesitant to ACK this patch due to concern of regressions - aside from my working through it by hand, there are multiple tests (including for example "pci-bridge-many-disks") which will fail if the exact same bus/slot is not assigned for many devices, so I am certain that the behavior wrt pci-root and pci-bridge is exactly the same as before the patch. (likewise, subsequent patches that introduce new controller types have tests that rely on the auto-assignment working properly for multiple controllers that have no explicit PCI address). On 07/17/2015 02:43 PM, Laine Stump wrote:

On 07/22/2015 02:50 PM, John Ferlan wrote: Correct. Thanks! Yes! Another good catch! I'm surprised that this didn't lead to any valgrind or coverity reports before now - that loop has been missing a VIR_FREE(addrStr) for a very long time.

This loop occurs just after we've assured that all devices that require a PCI device have been assigned and all necessary PCI controllers have been added. It is the perfect place to add other potentially auto-generated PCI controller attributes that are dependent on the controller's PCI address (upcoming patch). There is a convenient loop through all controllers at the end of the function, but the patch to add new functionality will be cleaner if we first rearrange that loop a bit. Note that the loop originally was accessing info.addr.pci.bus prior to determining that the pci part of the object was valid. This isn't dangerous in any way, but seemed a bit ugly, so I fixed it. --- new in V2 src/qemu/qemu_command.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b7b85ab..74f02f5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2240,20 +2240,24 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, goto cleanup; for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr cont = def->controllers[i]; + int idx = cont->idx; + virDevicePCIAddressPtr addr; + + if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) + continue; + + addr = &cont->info.addr.pci; /* check if every PCI bridge controller's ID is greater than * the bus it is placed onto */ - virDomainControllerDefPtr cont = def->controllers[i]; - int idx = cont->idx; - int bus = cont->info.addr.pci.bus; - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && - cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE && - idx <= bus) { + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE && + idx <= addr->bus) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("failed to create PCI bridge " "on bus %d: too many devices with fixed " "addresses"), - bus); + addr->bus); goto cleanup; } } -- 2.1.0

There are some non-0 default values in virDomainControllerDef (and will soon be more) that are easier to not forget if the remembering is done by a single initializer function (rather than inline code after allocating the obejct with generic VIR_ALLOC(). --- new in V2 src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5a9a88d..8dd4bf0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1527,6 +1527,37 @@ virDomainDiskSetFormat(virDomainDiskDefPtr def, int format) } +static virDomainControllerDefPtr +virDomainControllerDefNew(virDomainControllerType type) +{ + virDomainControllerDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + def->type = type; + + /* initialize anything that has a non-0 default */ + switch ((virDomainControllerType) def->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: + def->opts.vioserial.ports = -1; + def->opts.vioserial.vectors = -1; + break; + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + case VIR_DOMAIN_CONTROLLER_TYPE_IDE: + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + case VIR_DOMAIN_CONTROLLER_TYPE_CCID: + case VIR_DOMAIN_CONTROLLER_TYPE_USB: + case VIR_DOMAIN_CONTROLLER_TYPE_LAST: + break; + } + + return def; +} + + void virDomainControllerDefFree(virDomainControllerDefPtr def) { if (!def) @@ -7597,9 +7628,10 @@ virDomainControllerDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, unsigned int flags) { - virDomainControllerDefPtr def; + virDomainControllerDefPtr def = NULL; + int type = 0; xmlNodePtr cur = NULL; - char *type = NULL; + char *typeStr = NULL; char *idx = NULL; char *model = NULL; char *queues = NULL; @@ -7610,18 +7642,18 @@ virDomainControllerDefParseXML(xmlNodePtr node, ctxt->node = node; - if (VIR_ALLOC(def) < 0) - return NULL; - - type = virXMLPropString(node, "type"); - if (type) { - if ((def->type = virDomainControllerTypeFromString(type)) < 0) { + typeStr = virXMLPropString(node, "type"); + if (typeStr) { + if ((type = virDomainControllerTypeFromString(typeStr)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown controller type '%s'"), type); + _("Unknown controller type '%s'"), typeStr); goto error; } } + if (!(def = virDomainControllerDefNew(type))) + return NULL; + idx = virXMLPropString(node, "index"); if (idx) { if (virStrToLong_ui(idx, NULL, 10, &def->idx) < 0 || @@ -7692,8 +7724,6 @@ virDomainControllerDefParseXML(xmlNodePtr node, VIR_FREE(ports); goto error; } - } else { - def->opts.vioserial.ports = -1; } VIR_FREE(ports); @@ -7707,8 +7737,6 @@ virDomainControllerDefParseXML(xmlNodePtr node, VIR_FREE(vectors); goto error; } - } else { - def->opts.vioserial.vectors = -1; } VIR_FREE(vectors); break; @@ -7780,7 +7808,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, cleanup: ctxt->node = saved; - VIR_FREE(type); + VIR_FREE(typeStr); VIR_FREE(idx); VIR_FREE(model); VIR_FREE(queues); @@ -13960,18 +13988,12 @@ virDomainDefMaybeAddController(virDomainDefPtr def, return 0; } - if (VIR_ALLOC(cont) < 0) + if (!(cont = virDomainControllerDefNew(type))) return -1; - cont->type = type; cont->idx = idx; cont->model = model; - if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) { - cont->opts.vioserial.ports = -1; - cont->opts.vioserial.vectors = -1; - } - if (VIR_APPEND_ELEMENT(def->controllers, def->ncontrollers, cont) < 0) { VIR_FREE(cont); return -1; -- 2.1.0

On 07/22/2015 03:11 PM, John Ferlan wrote: I think it makes more sense to have a separate patch that logs an error if no type is given - the RNG requires it.

On 07/23/2015 08:35 AM, Ján Tomko wrote: Thanks! I've made both of those changes.

On 07/17/2015 02:43 PM, Laine Stump wrote: The <code>type</code> attribute... 1.2.18 (at least for now) NB: As I read the code, only the *first* <model type='%s'> listed will be used, as virDomainControllerDefParseXML not parse a second entry nor does a second entry cause an error So subsequent "<model type='%s'>" are gleefully ignored? Should there be an error? IDC either way, as long as it's described/noted because you know there's someone from QA looking to add two <model...> entries and expecting the second one to be used or an error to be generated. Of course scrolling back to the RNG - syntactically there can only be one it seems. s/0/NULL/ Since examples still exist that do not have the <model type='%s'> I suppose it's OK to hijack an existing test, but having a "new" test probably would have been better ACK - with the adjustments - whether you update/add a new test is your call. John

On Fri, Jul 17, 2015 at 02:43:32PM -0400, Laine Stump wrote: I'd say 'type' would be more fitting for the generic class of the device (dmi-to-pci-bridge), not the exact device model (i82801b11-bridge) so <model name='i82801b11-bridge'/> looks nicer to me, but I'm not going to suggest it again. This would be nicer as an enum - we don't allow arbitrary strings there anyway. Jan

On 07/24/2015 07:28 AM, Martin Kletzander wrote: Using "model type" is based on the existing "model type" for <interface> and <video>. I also think "model name" is more fitting, but it breaks with precedent. Should we knowingly be inconsistent in order to make the new one better? I'm undecided. Again, I used a string because that's the way it is in existing code (interface), but will change it to an enum for efficiency's sake. (I'm curious what problem you're refering to though - is it the fact that qemuBuildNicDevStr() doesn't qualify the string before using it in the commandline? That is taken care of in the case of this code.)

On Fri, Jul 24, 2015 at 08:01:05AM -0400, Laine Stump wrote: I remember replying to this. Is this just another mail or did I just discovered the rest of the email just now? I guess Friday is to blame. Anyway ... <interface> is the only one that does this. And due to back-compat we can't go back (until I finish one RFC, yay). The problem is that you can do e.g. <model type='AC97'/> for an interface. It's not a big deal, it's just annoying because QEMU will use a weird error message. Two more tiny "problems" arise hand in hand with this. One is that we cannot check whether this or that model has some capabilities. Well, we could, but it's way easier when the names are mapped and it's a known list. Also, if you need to compare these throughout the code (which you do in at least one later patch), you need to compare strings instead of values. Martin

On 07/17/2015 02:43 PM, Laine Stump wrote: Without trying to follow all the discussion from v1 of the series, I am assuming this methodology has been "agreed" upon... Probably should have noted that in the previous patch ;-) As has been told to me before - virStrdup/VIR_STRDUP is "tolerant" of deviceName == NULL returning 0 if it's NULL, so just: if (VIR_STRDUP(options->type, deviceName) < 0) goto cleanup; is necessary unknown pci-bridge option model '%s' ? Alternatively if (STREQ_NULLABLE() { } else { error message using NULLSTR(def->opts.pciopts.type) I don't care either way - it's obvious unknown dmi-to-pci-bridge option model '%s' ?? Same here - again - doesn't really matter ACK with the VIR_STRDUP() cleanup - whether you adjust the error message or the if/else conditions is your call. John

On 07/23/2015 09:41 AM, Ján Tomko wrote: I agree, and originally tried to set them in the PostParse callbacks, but then figured out that 1) no new devices (including new controllers) have a PCI address yet at that time and 2) any extra auto-added pci-bridge controllers haven't been added yet either, so putting any sort of logic to set these values (which are completely dependent on the PCI address, which hasn't yet been assigned) in the PostParse is pointless. You can see the order of this in, e.g., qemuDomainDefineXMLFlags() - it calls virDomainDefParseString() (which will end up calling the PostParse), then several other things, and then finally gets around to calling qemuDomainAssignAddresses(). Short of completely reorganizing when or how PCI addresses are assigned (which I think is way beyond the scope of this patch series, and has the potential to upset quite a lot in between PostParse and qemuDomainAssignAddresses()), this is the most reasonable place I could find. Even that is enough to cause problems. The real killer is that even controllers that were added explicitly in the XML (but without PCI addresses) would need something done here. So essentially "all controllers".

There are some configuration options to some types of pci controllers that are currently automatically derived from other parts of the controller's configuration. For example, a pci-bridge controller has an option that is called "chassis_nr" in qemu; libvirt always sets chassis_nr to the index of the pci-bridge. So this: <controller type='pci' model='pci-bridge' index='2'/> will always result in: -device pci-bridge,chassis_nr=2,... on the qemu commandline. In the future we may decide there is a better way to derive that option, but even in that case we will need for existing domains to retain the same chassis_nr they were using in the past - that is something that is visible to the guest so it is part of the guest ABI and changing it would lead to problems for migrating guests (or just guests with very picky OSes). The <target> subelement has been added as a place to put the new "chassisNr" attribute that will be filled in by libvirt when it auto-generates the chassisNr; it will be saved in the config, then reused any time the domain is started: <controller type='pci' model='pci-bridge' index='2'> <model type='pci-bridge'/> <target chassisNr='2'/> </controller> The one oddity of all this is that if the controller configuration is changed (for example to change the index or the pci address where the controller is plugged in), the items in <target> will *not* be re-generated, which might lead to conflict. I can't really see any way around this, but fortunately if there is a material conflict qemu will let us know and we will pass that on to the user. --- new in V2 (previously was a part of the patch to add pcie-root-port, but adding the attributes under <model> instead of <target>) docs/formatdomain.html.in | 23 ++++++++++++++++++++ docs/schemas/domaincommon.rng | 10 +++++++++ src/conf/domain_conf.c | 28 +++++++++++++++++++++++-- src/conf/domain_conf.h | 10 ++++++++- tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 1 + tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 1 + 6 files changed, 70 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fa46276..ae0d66a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3049,6 +3049,29 @@ libvirt. <span class="since">Since 1.3.0 (QEMU only).</span> </p> <p> + PCI controllers also have an optional + subelement <code>&lt;target&gt;</code> with the attributes + listed below. These are configurable items that 1) are visible + to the guest OS so must be preserved for guest ABI + compatibility, and 2) are usually left to default values or + derived automatically by libvirt. In almost all cases, you + should not manually add a <code>&lt;target&gt;</code> subelement + to a controller, nor should you modify the values in the those + that are automatically generated by + libvirt. <span class="since">Since 1.3.0 (QEMU only).</span> + </p> + <dl> + <dt><code>chassisNr</code></dt> + <dd> + PCI controllers that have attribute model="pci-bridge", can + also have a <code>chassisNr</code> attribute in + the <code>&lt;target&gt;</code> subelement, which is used to + control QEMU's "chassis_nr" option for the pci-bridge device + (normally libvirt automatically sets this to the same value as + the index attribute of the pci controller). + </dd> + </dl> + <p> 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. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 66518f9..1b1f592 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1744,6 +1744,16 @@ <empty/> </element> </optional> + <optional> + <element name="target"> + <optional> + <attribute name='chassisNr'> + <ref name='uint8range'/> + </attribute> + </optional> + <empty/> + </element> + </optional> <!-- *-root controllers have an optional element "pcihole64"--> <choice> <group> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 380b758..17526d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1544,6 +1544,8 @@ virDomainControllerDefNew(virDomainControllerType type) def->opts.vioserial.vectors = -1; break; case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + def->opts.pciopts.chassisNr = -1; + break; case VIR_DOMAIN_CONTROLLER_TYPE_IDE: case VIR_DOMAIN_CONTROLLER_TYPE_FDC: case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: @@ -7638,6 +7640,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, char *cmd_per_lun = NULL; char *max_sectors = NULL; char *guestModel = NULL; + char *chassisNr = NULL; xmlNodePtr saved = ctxt->node; int rc; @@ -7686,6 +7689,9 @@ virDomainControllerDefParseXML(xmlNodePtr node, } else if (xmlStrEqual(cur->name, BAD_CAST "model")) { if (!guestModel) guestModel = virXMLPropString(cur, "type"); + } else if (xmlStrEqual(cur->name, BAD_CAST "target")) { + if (!chassisNr) + chassisNr = virXMLPropString(cur, "chassisNr"); } } cur = cur->next; @@ -7798,6 +7804,13 @@ virDomainControllerDefParseXML(xmlNodePtr node, def->opts.pciopts.type = guestModel; guestModel = 0; } + if (chassisNr && virStrToLong_i(chassisNr, NULL, 0, + &def->opts.pciopts.chassisNr) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassisNr '%s' in PCI controller"), + chassisNr); + goto error; + } break; default: @@ -7824,6 +7837,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, VIR_FREE(cmd_per_lun); VIR_FREE(max_sectors); VIR_FREE(guestModel); + VIR_FREE(chassisNr); return def; @@ -18833,7 +18847,7 @@ virDomainControllerDefFormat(virBufferPtr buf, { const char *type = virDomainControllerTypeToString(def->type); const char *model = NULL; - bool pcihole64 = false, pciModel = false; + bool pcihole64 = false, pciModel = false, pciTarget = false; if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -18875,13 +18889,15 @@ virDomainControllerDefFormat(virBufferPtr buf, pcihole64 = true; if (def->opts.pciopts.type) pciModel = true; + if (def->opts.pciopts.chassisNr != -1) + pciTarget = true; break; default: break; } - if (pciModel || + if (pciModel || pciTarget || def->queues || def->cmd_per_lun || def->max_sectors || virDomainDeviceInfoNeedsFormat(&def->info, flags) || pcihole64) { virBufferAddLit(buf, ">\n"); @@ -18893,6 +18909,14 @@ virDomainControllerDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } + if (pciTarget) { + virBufferAddLit(buf, "<target"); + if (def->opts.pciopts.chassisNr != -1) + virBufferAsprintf(buf, " chassisNr='%d'", + def->opts.pciopts.chassisNr); + virBufferAddLit(buf, "/>\n"); + } + if (def->queues || def->cmd_per_lun || def->max_sectors) { virBufferAddLit(buf, "<driver"); if (def->queues) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 09fe3c0..32d1073 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -805,7 +805,15 @@ struct _virDomainPCIControllerOpts { * similar to the model of <interface> devices. */ char *type; /* the exact name of the device in hypervisor */ -}; + + /* the following items are attributes of the "target" subelement + * of controller type='pci'. They are bits of configuration that + * are specified on the qemu commandline and are visible to the + * guest OS, so they must be preserved to ensure ABI + * compatibility. + */ + int chassisNr; /* used by pci-bridge, -1 == unspecified */ + }; /* Stores the virtual disk controller configuration */ struct _virDomainControllerDef { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml index 4623a5c..815eb08 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml @@ -25,6 +25,7 @@ </controller> <controller type='pci' index='2' model='pci-bridge'> <model type='pci-bridge'/> + <target chassisNr='56'/> </controller> <video> <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml index 760830a..13e4734 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -25,6 +25,7 @@ </controller> <controller type='pci' index='2' model='pci-bridge'> <model type='pci-bridge'/> + <target chassisNr='56'/> </controller> <controller type='sata' index='0'/> <video> -- 2.1.0

On 07/22/2015 04:20 PM, John Ferlan wrote: As far as I understand from Alex, it is 0-255 "-1" is used to indicate "not specified" (of course when it isn't specified, it is auto-filled later on). Yes. I'll put that in. Currently true. But as more things are added to <target> (in another couple patches) that will no longer be true, so I opted to put in from the beginning the code that would later be needed, rather than changing it later. Not after patch I see this code as just making an adjustment to the existing controllers, so it makes sense to me to adjust the test (while assuring there are other tests around to make sure that it continues to work with the new attributes not set).

This controller can be connected (at domain startup time only - not hotpluggable) only to a port on the pcie root complex ("pcie-root" in libvirt config), hence the new connect type VIR_PCI_CONNECT_TYPE_PCIE_ROOT. It provides a hotpluggable port that will accept any PCI or PCIe device. New attributes must be added to the controller <target> subelement for this - chassis and port are guest-visible option values that will be set by libvirt with values derived from the controller's index and pci address information. --- docs/formatdomain.html.in | 35 ++++++++++++++++++-- docs/schemas/domaincommon.rng | 15 ++++++++- src/conf/domain_addr.c | 10 ++++-- src/conf/domain_addr.h | 5 ++- src/conf/domain_conf.c | 37 ++++++++++++++++++++-- src/conf/domain_conf.h | 7 +++- src/qemu/qemu_command.c | 1 + .../qemuxml2argv-pcie-root-port.xml | 36 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 9 files changed, 138 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ae0d66a..dcbca75 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3024,10 +3024,11 @@ <p> PCI controllers have an optional <code>model</code> attribute with possible values <code>pci-root</code>, <code>pcie-root</code>, - <code>pci-bridge</code>, or <code>dmi-to-pci-bridge</code>. + <code>pcie-root-port</code>, <code>pci-bridge</code>, + or <code>dmi-to-pci-bridge</code>. (pci-root and pci-bridge <span class="since">since 1.0.5</span>, pcie-root and dmi-to-pci-bridge <span class="since">since - 1.1.2</span>) + 1.1.2</span>, pcie-root-port <span class="since">since 1.3.0</span>) The root controllers (<code>pci-root</code> and <code>pcie-root</code>) have an optional <code>pcihole64</code> element specifying how big (in kilobytes, or in the unit specified by <code>pcihole64</code>'s @@ -3070,6 +3071,25 @@ (normally libvirt automatically sets this to the same value as the index attribute of the pci controller). </dd> + <dt><code>chassis</code></dt> + <dd> + pcie-root-port controllers can also have + a <code>chassis</code> attribute in + the <code>&lt;model&gt;</code> subelement, which is used to + control QEMU's "chassis" option for the device (normally + libvirt automatically sets this to the same value as the index + attribute of the pci controller). + </dd> + <dt><code>port</code></dt> + <dd> + pcie-root-port controllers can also have a <code>port</code> + attribute in the <code>&lt;model&gt;</code> subelement, which + is used to control QEMU's "port" option for the device + (normally libvirt automatically sets this based on the PCI + address where the root port is connected using the formula + (slot &lt;&lt; 3) + function (although this could change in + the future). + </dd> </dl> <p> For machine types which provide an implicit PCI bus, the pci-root @@ -3116,6 +3136,17 @@ auto-determined by libvirt will be placed on this pci-bridge device. (<span class="since">since 1.1.2</span>). </p> + <p> + Domains with an implicit pcie-root can also add controllers + with <code>model='pcie-root-port'</code>. This is a simple type of + bridge device that can connect only to one of the 31 slots on + the pcie-root bus on its upstream side, and makes a single + (PCIe, hotpluggable) port (at slot='0') available on the + downstream side. This controller can be used to provide a single + slot to later hotplug a PCIe device (but is not itself + hotpluggable - it must be in the configuration when the domain + is started). (<span class="since">since 1.3.0</span>) + </p> <pre> ... &lt;devices&gt; diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 1b1f592..0efa0f0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1739,6 +1739,8 @@ <value>pci-bridge</value> <!-- implementations of 'dmi-to-pci-bridge' --> <value>i82801b11-bridge</value> + <!-- implementations of 'pcie-root-port' --> + <value>ioh3420</value> </choice> </attribute> <empty/> @@ -1751,7 +1753,17 @@ <ref name='uint8range'/> </attribute> </optional> - <empty/> + <optional> + <attribute name="chassis"> + <ref name='uint8range'/> + </attribute> + </optional> + <optional> + <attribute name="port"> + <ref name='uint8range'/> + </attribute> + </optional> + <empty/> </element> </optional> <!-- *-root controllers have an optional element "pcihole64"--> @@ -1774,6 +1786,7 @@ <choice> <value>pci-bridge</value> <value>dmi-to-pci-bridge</value> + <value>pcie-root-port</value> </choice> </attribute> </group> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index bc09279..fba0eff 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -183,9 +183,9 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: /* slots 1 - 31, no hotplug, PCIe only unless the address was * specified in user config *and* the particular device being - * attached also allows it + * attached also allows it. */ - bus->flags = VIR_PCI_CONNECT_TYPE_PCIE; + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE | VIR_PCI_CONNECT_TYPE_PCIE_ROOT; bus->minSlot = 1; bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; break; @@ -196,6 +196,12 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->minSlot = 1; bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + /* provides one slot which is pcie and hotpluggable */ + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE | VIR_PCI_CONNECT_HOTPLUGGABLE; + bus->minSlot = 0; + bus->maxSlot = 0; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PCI controller model %d"), model); diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index dcfd2e5..2a0ff96 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -39,6 +39,8 @@ typedef enum { /* PCI devices can connect to this bus */ VIR_PCI_CONNECT_TYPE_PCIE = 1 << 3, /* PCI Express devices can connect to this bus */ + VIR_PCI_CONNECT_TYPE_PCIE_ROOT = 1 << 4, + /* for devices that can only connect to pcie-root (i.e. root-port) */ } virDomainPCIConnectFlags; typedef struct { @@ -70,7 +72,8 @@ typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; * allowed, e.g. PCI, PCIe, switch */ # define VIR_PCI_CONNECT_TYPES_MASK \ - (VIR_PCI_CONNECT_TYPE_PCI | VIR_PCI_CONNECT_TYPE_PCIE) + (VIR_PCI_CONNECT_TYPE_PCI | VIR_PCI_CONNECT_TYPE_PCIE | \ + VIR_PCI_CONNECT_TYPE_PCIE_ROOT) /* combination of all bits that could be used to connect a normal * endpoint device (i.e. excluding the connection possible between an diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 17526d4..e02c861 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -324,7 +324,8 @@ VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pci-root", "pcie-root", "pci-bridge", - "dmi-to-pci-bridge") + "dmi-to-pci-bridge", + "pcie-root-port") VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, "auto", @@ -1545,6 +1546,8 @@ virDomainControllerDefNew(virDomainControllerType type) break; case VIR_DOMAIN_CONTROLLER_TYPE_PCI: def->opts.pciopts.chassisNr = -1; + def->opts.pciopts.chassis = -1; + def->opts.pciopts.port = -1; break; case VIR_DOMAIN_CONTROLLER_TYPE_IDE: case VIR_DOMAIN_CONTROLLER_TYPE_FDC: @@ -7641,6 +7644,8 @@ virDomainControllerDefParseXML(xmlNodePtr node, char *max_sectors = NULL; char *guestModel = NULL; char *chassisNr = NULL; + char *chassis = NULL; + char *port = NULL; xmlNodePtr saved = ctxt->node; int rc; @@ -7692,6 +7697,10 @@ virDomainControllerDefParseXML(xmlNodePtr node, } else if (xmlStrEqual(cur->name, BAD_CAST "target")) { if (!chassisNr) chassisNr = virXMLPropString(cur, "chassisNr"); + if (!chassis) + chassis = virXMLPropString(cur, "chassis"); + if (!port) + port = virXMLPropString(cur, "port"); } } cur = cur->next; @@ -7811,6 +7820,20 @@ virDomainControllerDefParseXML(xmlNodePtr node, chassisNr); goto error; } + if (chassis && virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid chassis '%s' in PCI controller"), + chassis); + goto error; + } + if (port && virStrToLong_i(port, NULL, 0, + &def->opts.pciopts.port) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid port '%s' in PCI controller"), + port); + goto error; + } break; default: @@ -7838,6 +7861,8 @@ virDomainControllerDefParseXML(xmlNodePtr node, VIR_FREE(max_sectors); VIR_FREE(guestModel); VIR_FREE(chassisNr); + VIR_FREE(chassis); + VIR_FREE(port); return def; @@ -18889,7 +18914,9 @@ virDomainControllerDefFormat(virBufferPtr buf, pcihole64 = true; if (def->opts.pciopts.type) pciModel = true; - if (def->opts.pciopts.chassisNr != -1) + if (def->opts.pciopts.chassisNr != -1 || + def->opts.pciopts.chassis != -1 || + def->opts.pciopts.port != -1) pciTarget = true; break; @@ -18914,6 +18941,12 @@ virDomainControllerDefFormat(virBufferPtr buf, if (def->opts.pciopts.chassisNr != -1) virBufferAsprintf(buf, " chassisNr='%d'", def->opts.pciopts.chassisNr); + if (def->opts.pciopts.chassis != -1) + virBufferAsprintf(buf, " chassis='%d'", + def->opts.pciopts.chassis); + if (def->opts.pciopts.port != -1) + virBufferAsprintf(buf, " port='0x%x'", + def->opts.pciopts.port); virBufferAddLit(buf, "/>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 32d1073..a4df2a6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -752,6 +752,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST } virDomainControllerModelPCI; @@ -813,7 +814,11 @@ struct _virDomainPCIControllerOpts { * compatibility. */ int chassisNr; /* used by pci-bridge, -1 == unspecified */ - }; + /* chassis & port used by + * pcie-root-port/pcie-switch-downstream-port, -1 = unspecified */ + int chassis; + int port; +}; /* Stores the virtual disk controller configuration */ struct _virDomainControllerDef { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9f15eb6..53f5317 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2267,6 +2267,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (!options->type) deviceName = "i82801b11-bridge"; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml new file mode 100644 index 0000000..38418bb --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml @@ -0,0 +1,36 @@ +<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> + <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'/> + <controller type='pci' index='3' model='pcie-root-port'/> + <controller type='pci' index='4' model='pcie-root-port'> + <model type='ioh3420'/> + <target chassis='40' port='0x1a'/> + </controller> + <controller type='sata' index='0'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7a41a18..6b48bf4 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -567,6 +567,7 @@ mymain(void) DO_TEST_DIFFERENT("pci-autoadd-idx"); DO_TEST_DIFFERENT("pcie-root"); DO_TEST_DIFFERENT("q35"); + DO_TEST("pcie-root-port"); DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); -- 2.1.0

On 07/22/2015 05:38 PM, John Ferlan wrote: Not really. You can't test chassis/port without having a controller type that they are used with. I always just figured "a number is a number, and it will be converted as appropriate". I see that bus/slot in PCI addresses are documented as "a hex number", but they have a different type in the RNG which forces input as a hex number (with leading 0x). Interestingly, the parser doesn't enforce that (I think the RNG should lighten up and the parser should stay the same). There will be an error because there won't be any bus to plug it into. If qemu came up with another machinetype that had a pcie-root and a device called ioh3420, we *would* automatically support it. Also, I'm not sure about the "qemu 1.2.2" statement. From my point of view "it's in the versions that it's in". I suppose I can add that though. But it *is* right. The registers in the emulated hardware that hold these values are 8 bits, but the struct in libvirt needs to have an int so that (internally only) it can hold -1 to indicate "unspecified". I've added a check on the parsed values to assure that these are all 0 - 255 only. The parser needs to check limits, and it now does. Not any more :-) Since the RNG isn't always referenced, we need to do *something*. I've actually changed it to have a "processedTarget" boolean that is set when <target> has been seen once. If it's seen again, an error will be logged. See above. Which comments are those? There was a comment in a previous patch about not needing the boolean "pciTarget" that wasn't valid, but your comments about the range were certainly valid.

On 07/17/2015 02:43 PM, Laine Stump wrote: missed on first pass This <empty/> seems to be off a space - should it line up under optional and not element? John

On 07/24/2015 12:07 PM, Martin Kletzander wrote: To do that would require a temporary variable (since chassis is an int so that it can hold the "-1" value meaning "unspecified". Since the limits are much lower than what can fit in an int, I'd rather continue to use *_i and check both ends of the range after conversion (which I'm adding in to all of them now) I forget the exact reason, but it had something to do with matching other existing things. I think in the case of port it is coming from slot and slot is always printed in hex, but chassis was coming from index, and index is always printed in decimal. Makes it easier to visually verify that they match, but no better reason than that :-) The amount of negative tests that could be added is endless. Some negative tests are impossible though - anything that is outside the schema in the RNG can never get past the domainschematest even if it's supposed to fail.

On 07/17/2015 02:43 PM, Laine Stump wrote: Similar to 06/17 unknown pcii-root-port option model '%s' ACK John

On 07/24/2015 12:14 PM, Martin Kletzander wrote: They can happen if somebody screws up the code. That's why they're logged as "INTERNAL_ERROR" - they should never happen, and are just there as a sanity check. Actually it ends up not being any shorter, since we have to check for internal inconsistencies. It's definitely faster though (for whatever that's worth).

On 07/17/2015 02:43 PM, Laine Stump wrote: 1.2.18 Similar to 10/17 - there's no 'extra' explanation... Perhaps something after the <p> that starts "Domains with an implicit pcie-root can also..." The new section should at least describe "how" one does the connections and of course because the pci-root-port can accept the pcie-switch-upstream-port either at boot or via hotplug. Someone "planning" to add the upstream-port device would need to have "enough" pci-root-port's available at boot time (the 1x1 relationship) Also while I'm thinking about it (and since it dawned on me only today)... Assuming the pcie-root-port connects to some pcie-root and only one pcie-root can exist, is or should there be a "expect failure" test for trying to add 33? I guess this started dawning on my while looking at the XML attached to this patch which as two pcie-root-port's and two pci-switch-upstream-port's. The wheels started turning wondering how the output XML of the input XML should appear with the addresses assigned on the controllers. When I peek to the next patch, it seems the two pci-root-port's connect to the pcie-root as expected and then the two pci-switch-upstream-port's have their 1x1 connection to the pci-root-port. It seems there needs to be a : tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root-port.xml Which probably should be added in patch10. Then this patch would follow with the output for qemuxml2argv-pcie-switch-upstream-port.xml Another thought - a test/different that provides two upstream-port's, but only one pcie-root-port in order to show the failure to find something to connect to for the second one. Making a simple adjustment to the test here to remove a pci-root-port, results in the expected error: "libvirt: Domain Config error : internal error: Cannot automatically add a new PCI bus for a device requiring a slot other than standard PCI." So "conceptually" an upstream can be hotplugged into a pci-root-port, but nothing can hotplug into it? Thus doesn't seem hotplug is very useful. Perhaps I'm missing something. I haven't read the last 3 patches for the downstream-port yet. It's not a "problem" per se, but whatever can and cannot be done needs to be documented as well as of course the seemingly "suggested" way of defining at boot time. I think with some additional tests and adjustment to documentation, this can be ACK'd John

On 07/17/2015 02:43 PM, Laine Stump wrote: similar to previous unknown pcie-switch-upstream-port option model '%s' Of course expanding upon Jan's comment in 05/17 these all end up being conversions from enum type to string which is something I had considered asking about earlier, but since I didn't have the complete picture in mind yet I wasn't sure whether it'd be worth it. I'm 40/50 either way, but I think with constant char strings you do avoid one extra alloc/strdup/free cycle. The default 'type' would then be _NONE and conversions only necessary if set. Rather than STREQ you get numerical comparison. As I type here I think I'm convincing myself of the value. John

On 07/17/2015 02:43 PM, Laine Stump wrote: Repetitive comments from prior reviews (9 & 12) Otherwise seems fine. John

On 07/17/2015 02:43 PM, Laine Stump wrote: 1.2.18 1.2.18 The upstream-port discussion from the previous patch will/should flow into this I assume. The whole hotplug conversation is really confusing - upstream/ downstream what can plug into what and when - my eyes are rolling around in circles. The upstream cannot be hotplugged into; however, the downstream has one port that can be hotplugged. The downstream can only connect into the upstream and the upstream can connect either into a downstream or a root-port, but itself isn't hotpluggable. So hot plugging one into a downstream still means one cannot hotplug anything into that upstream device. A downstream device cannot plug into a root-port. I can only hope I got that right ;-). I think I need one of the pile higher and deeper degrees! Based one what I see in the next patch, the index=9 upstream isn't used.. Perhaps would be nice to show it's use in some way. May also be useful to show a downstream plugged into a pcie-root-port and how that's used. Yes, I'm a visual learner ;-) I think having/using the xmlout would be beneficial too. John

This is backed by the qemu device xio3130-downstream. It can only be connected to a pcie-switch-upstream-port (x3130-upstream) on the upstream side. --- V2: * change test case to reflect additional pcie-root-port between pcie-root and pcie-switch-upstream-port and add a 2nd switch that is connected to a downstream port of the 1st switch. src/qemu/qemu_command.c | 42 ++++++++++++++++++++++ .../qemuxml2argv-pcie-switch-downstream-port.args | 18 ++++++++++ tests/qemuxml2argvtest.c | 9 +++++ 3 files changed, 69 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e3f0663..155a94a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1610,6 +1610,12 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, */ flags = VIR_PCI_CONNECT_TYPE_PCIE_PORT; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + /* pcie-switch-downstream-port can only connect to a + * pcie-switch-upstream-port, and can't be hot-plugged. + */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_SWITCH; + break; default: break; } @@ -2292,6 +2298,13 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, deviceName = "x3130-upstream"; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + if (!options->type) + deviceName = "xio3130-downstream"; + if (options->chassis == -1) + options->chassis = cont->idx; + if (options->port == -1) + options->port = addr->slot; + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: @@ -2416,6 +2429,10 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, */ flags = VIR_PCI_CONNECT_TYPE_PCIE_PORT; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + /* pcie-switch-port can only plug into pcie-switch */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_SWITCH; + break; default: flags = VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI; break; @@ -4766,6 +4783,31 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, virBufferAsprintf(&buf, "%s,id=%s", def->opts.pciopts.type, def->info.alias); break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + if (!def->opts.pciopts.type) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pcie-switch-downstream-port " + "options not set")); + goto error; + } + if (STREQ(def->opts.pciopts.type, "xio3130-downstream")) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("The pcie-switch-downstream-port " + "(xio3130-downstream) controller " + "is not supported in this QEMU binary")); + goto error; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pcie-switch-downstream-port device '%s'"), + def->opts.pciopts.type); + goto error; + } + virBufferAsprintf(&buf, "%s,port=0x%x,chassis=%d,id=%s", + def->opts.pciopts.type, def->opts.pciopts.port, + def->opts.pciopts.chassis, def->info.alias); + break; } break; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args new file mode 100644 index 0000000..fefefa2 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.args @@ -0,0 +1,18 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/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=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \ +-device x3130-upstream,id=pci.4,bus=pci.3,addr=0x0 \ +-device xio3130-downstream,port=0x0,chassis=5,id=pci.5,bus=pci.4,addr=0x0 \ +-device xio3130-downstream,port=0x1,chassis=6,id=pci.6,bus=pci.4,addr=0x1 \ +-device xio3130-downstream,port=0x2,chassis=7,id=pci.7,bus=pci.4,addr=0x2 \ +-device xio3130-downstream,port=0x27,chassis=30,id=pci.8,bus=pci.4,addr=0x3 \ +-device x3130-upstream,id=pci.9,bus=pci.5,addr=0x0 \ +-device xio3130-downstream,port=0x4,chassis=10,id=pci.10,bus=pci.4,addr=0x4 \ +-device xio3130-downstream,port=0x5,chassis=11,id=pci.11,bus=pci.4,addr=0x5 \ +-device xio3130-downstream,port=0x6,chassis=12,id=pci.12,bus=pci.4,addr=0x6 \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ +-device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ +-vga qxl -global qxl-vga.ram_size=67108864 -global qxl-vga.vram_size=33554432 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b42d483..287df6a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1512,6 +1512,15 @@ mymain(void) 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); + DO_TEST("pcie-switch-downstream-port", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_X3130_UPSTREAM, + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM, + 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); DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, -- 2.1.0