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:

The function that auto-assigns PCI addresses was written with the hardcoded assumptions that any PCI bus would have slots available starting at 1 and ending at 31. This isn't true for many types of controller (some have a single slot/port at 0, some have slots/ports from 0 to 31). This patch updates that function to remove the hardcoded assumptions. It will properly find/assign addresses for devices that can only connect to pcie-(root|downstream)-port (which have minSlot/maxSlot of 0/0) or a pcie-switch-upstream-port (0/31).

It still will not auto-create a new bus of the proper kind for these connections when one doesn't exist, that task is for another day. --- new in V2

src/conf/domain_addr.c | 65 +++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 2be98c5..bc09279 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -471,24 +471,30 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs, virDomainPCIConnectFlags flags) { /* default to starting the search for a free slot from - * 0000:00:00.0 + * the first slot of domain 0 bus 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 - */ - if (flags == addrs->lastFlags) - a = addrs->lastaddr; - if (addrs->nbuses == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available")); goto error; }

- /* Start the search at the last used bus and slot */ - for (a.slot++; a.bus < addrs->nbuses; a.bus++) { + /* ...unless this search is for the exact same type of device as + * last time, then continue the search from the next slot after + * the previous match. + */ + if (flags == addrs->lastFlags) { + a = addrs->lastaddr; + if (++a.slot > addrs->buses[a.bus].maxSlot && + ++a.bus < addrs->nbuses) + a.slot = addrs->buses[a.bus].minSlot; + } else { + a.slot = addrs->buses[0].minSlot; + } + + while (a.bus < addrs->nbuses) { if (!(addrStr = virDomainPCIAddressAsString(&a))) goto error; if (!virDomainPCIAddressFlagsCompatible(&a, addrStr, @@ -497,29 +503,33 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs, VIR_FREE(addrStr); VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device", a.domain, a.bus); - continue; - } - for (; a.slot <= VIR_PCI_ADDRESS_SLOT_LAST; a.slot++) { - if (!virDomainPCIAddressSlotInUse(addrs, &a)) - goto success; + } else { + while (a.slot <= addrs->buses[a.bus].maxSlot) { + if (!virDomainPCIAddressSlotInUse(addrs, &a)) + goto success;

- VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", - a.domain, a.bus, a.slot); + VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", + a.domain, a.bus, a.slot); + a.slot++; + } } - a.slot = 1; + if (++a.bus < addrs->nbuses) + a.slot = addrs->buses[a.bus].minSlot; VIR_FREE(addrStr); }

/* There were no free slots after the last used one */ if (addrs->dryRun) { - /* a is already set to the first new bus and slot 1 */ + /* a is already set to the first new bus */ if (virDomainPCIAddressSetGrow(addrs, &a, flags) < 0) goto error; + /* this device will use the first slot of the new bus */ + a.slot = addrs->buses[a.bus].minSlot; 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++) { - addrStr = NULL; + a.slot = addrs->buses[a.bus].minSlot; if (!(addrStr = virDomainPCIAddressAsString(&a))) goto error; if (!virDomainPCIAddressFlagsCompatible(&a, addrStr, @@ -527,14 +537,15 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs, flags, false, false)) { VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device", a.domain, a.bus); - continue; - } - for (a.slot = 1; a.slot <= VIR_PCI_ADDRESS_SLOT_LAST; a.slot++) { - if (!virDomainPCIAddressSlotInUse(addrs, &a)) - goto success; - - VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", - a.domain, a.bus, a.slot); + } else { + while (a.slot <= addrs->buses[a.bus].maxSlot) { + if (!virDomainPCIAddressSlotInUse(addrs, &a)) + goto success; + + VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", + a.domain, a.bus, a.slot); + a.slot++; + } } } }

On 07/22/2015 02:50 PM, John Ferlan wrote:

On 07/17/2015 02:43 PM, Laine Stump wrote:

...

The function that auto-assigns PCI addresses was written with the hardcoded assumptions that any PCI bus would have slots available starting at 1 and ending at 31. This isn't true for many types of controller (some have a single slot/port at 0, some have slots/ports from 0 to 31). This patch updates that function to remove the hardcoded assumptions. It will properly find/assign addresses for devices that can only connect to pcie-(root|downstream)-port (which have minSlot/maxSlot of 0/0) or a pcie-switch-upstream-port (0/31).

It still will not auto-create a new bus of the proper kind for these connections when one doesn't exist, that task is for another day. --- new in V2

src/conf/domain_addr.c | 65 +++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 2be98c5..bc09279 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -471,24 +471,30 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs, virDomainPCIConnectFlags flags) { /* default to starting the search for a free slot from - * 0000:00:00.0 + * the first slot of domain 0 bus 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 - */ - if (flags == addrs->lastFlags) - a = addrs->lastaddr; - if (addrs->nbuses == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available")); goto error; }

- /* Start the search at the last used bus and slot */ - for (a.slot++; a.bus < addrs->nbuses; a.bus++) { + /* ...unless this search is for the exact same type of device as + * last time, then continue the search from the next slot after + * the previous match. next slot and possibly first slot of next bus

...

+ */ + if (flags == addrs->lastFlags) { + a = addrs->lastaddr; + if (++a.slot > addrs->buses[a.bus].maxSlot && + ++a.bus < addrs->nbuses) + a.slot = addrs->buses[a.bus].minSlot; + } else { + a.slot = addrs->buses[0].minSlot; + } + + while (a.bus < addrs->nbuses) { if (!(addrStr = virDomainPCIAddressAsString(&a))) goto error; if (!virDomainPCIAddressFlagsCompatible(&a, addrStr, @@ -497,29 +503,33 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs, VIR_FREE(addrStr); I think with the new logic to use "if / else" rather than "if ... continue;", this VIR_FREE is unnecessary since it's done at then end of the while loop

Correct. Thanks!

...

VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device", a.domain, a.bus); - continue; - } - for (; a.slot <= VIR_PCI_ADDRESS_SLOT_LAST; a.slot++) { - if (!virDomainPCIAddressSlotInUse(addrs, &a)) - goto success; + } else { + while (a.slot <= addrs->buses[a.bus].maxSlot) { + if (!virDomainPCIAddressSlotInUse(addrs, &a)) + goto success;

- VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", - a.domain, a.bus, a.slot); + VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", + a.domain, a.bus, a.slot); + a.slot++; + } } - a.slot = 1; + if (++a.bus < addrs->nbuses) + a.slot = addrs->buses[a.bus].minSlot; VIR_FREE(addrStr); }

/* There were no free slots after the last used one */

So essentially we're going to search everything before to see if there's any openings to use.

...

if (addrs->dryRun) { - /* a is already set to the first new bus and slot 1 */ + /* a is already set to the first new bus */ if (virDomainPCIAddressSetGrow(addrs, &a, flags) < 0) goto error; + /* this device will use the first slot of the new bus */ + a.slot = addrs->buses[a.bus].minSlot; 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++) { - addrStr = NULL; + a.slot = addrs->buses[a.bus].minSlot; if (!(addrStr = virDomainPCIAddressAsString(&a))) goto error; if (!virDomainPCIAddressFlagsCompatible(&a, addrStr, @@ -527,14 +537,15 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs, flags, false, false)) { VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device", a.domain, a.bus); - continue; - } - for (a.slot = 1; a.slot <= VIR_PCI_ADDRESS_SLOT_LAST; a.slot++) { - if (!virDomainPCIAddressSlotInUse(addrs, &a)) - goto success; - - VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", - a.domain, a.bus, a.slot); + } else { + while (a.slot <= addrs->buses[a.bus].maxSlot) { + if (!virDomainPCIAddressSlotInUse(addrs, &a)) + goto success; + + VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", + a.domain, a.bus, a.slot); + a.slot++; + } }

Perhaps preexisting, but one would think a VIR_FREE(addrStr) would be needed here just as it was in the first pass... [Coverity didn't find this either]

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.

ACK with the adjustment

John

...

} }

On 07/22/2015 03:11 PM, John Ferlan wrote:

On 07/17/2015 02:43 PM, Laine Stump wrote:

...

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; Should we make this?

type = VIR_DOMAIN_CONTROLLER_TYPE_IDE;

Not that it perhaps matters too much, but does perhaps point to where/why an IDE controller got created if "type='xxx'"

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:

[reducing the cc-list]

On Fri, Jul 17, 2015 at 02:43:31PM -0400, Laine Stump wrote:

...

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; + } + The model could be initialized to -1 here as well.

...

+ return def; +} + + void virDomainControllerDefFree(virDomainControllerDefPtr def) { if (!def) @@ -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; goto error; Otherwise typeStr gets leaked.

Thanks! I've made both of those changes.

On 07/17/2015 02:43 PM, Laine Stump wrote:

This new subelement is used in PCI controllers: the toplevel *attribute* "model" of a controller denotes what kind of PCI controller is being described, e.g. a "dmi-to-pci-bridge", "pci-bridge", or "pci-root". But in the future there will be different implementations of some of those types of PCI controllers, which behave similarly from libvirt's point of view (and so should have the same model), but use a different device in qemu (and present themselves as a different piece of hardware in the guest). In an ideal world we (i.e. "I") would have thought of that back when the pci controllers were added, and used some sort of type/class/model notation (where class was used in the way we are now using model, and model was used for the actual manufacturer's model number of a particular family of PCI controller), but that opportunity is long past, so as an alternative, this patch allows selecting a particular implementation of a pci controller with the "type" attribute of the <model> subelement, e.g.:

<controller type='pci' model='dmi-to-pci-bridge' index='1'> <model type='i82801b11-bridge'/> </controller>

In this case, "dmi-to-pci-bridge" is the kind of controller (one that has a single PCIe port upstream, and 32 standard PCI ports downstream, which are not hotpluggable), and the qemu device to be used to implement this kind of controller is named "i82801b11-bridge".

Implementing the above now will allow us in the future to add a new kind of dmi-to-pci-bridge that doesn't use qemu's i82801b11-bridge device, but instead uses something else (which doesn't yet exist, but qemu people have been discussing it), all without breaking existing configs.

(note that for the existing "pci-bridge" type of PCI controller, both the model attribute and <model> type are 'pci-bridge'. This is just a coincidence, since it turns out that in this case the device name in qemu really is a generic 'pci-bridge' rather than being the name of some real-world chip) --- new in V2 (previously was a part of the patch to add pcie-root-port)

docs/formatdomain.html.in | 12 ++++++++++++ docs/schemas/domaincommon.rng | 13 +++++++++++++ src/conf/domain_conf.c | 23 +++++++++++++++++++++-- src/conf/domain_conf.h | 8 ++++++++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 ++++++-- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 ++++++-- 6 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8cd8d09..fa46276 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3037,6 +3037,18 @@ (set to 0). <span class="since">Since 1.1.2 (QEMU only)</span> </p> <p> + PCI controllers also have an optional + subelement <code><model></code> with an attribute named + "type". The type attribute holds the name of the specific device

The <code>type</code> attribute...

+ that qemu is emulating (e.g. "i82801b11-bridge") rather than + simply the class of device ("dmi-to-pci-bridge", "pci-bridge"), + which is set in the controller element's model <b>attribute</b>. + In almost all cases, you should not manually add + a <code><model></code> subelement to a controller, nor + should you modify one that is automatically generated by + libvirt. <span class="since">Since 1.3.0 (QEMU only).</span>

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

+ </p> + <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 1120003..66518f9 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1731,6 +1731,19 @@ <attribute name="type"> <value>pci</value> </attribute> + <optional> + <element name="model"> + <attribute name="type"> + <choice> + <!-- implementations of 'pci-bridge' --> + <value>pci-bridge</value> + <!-- implementations of 'dmi-to-pci-bridge' --> + <value>i82801b11-bridge</value> + </choice> + </attribute> + <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 8dd4bf0..380b758 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7637,6 +7637,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, char *queues = NULL; char *cmd_per_lun = NULL; char *max_sectors = NULL; + char *guestModel = NULL; xmlNodePtr saved = ctxt->node; int rc;

@@ -7682,6 +7683,9 @@ virDomainControllerDefParseXML(xmlNodePtr node, queues = virXMLPropString(cur, "queues"); cmd_per_lun = virXMLPropString(cur, "cmd_per_lun"); max_sectors = virXMLPropString(cur, "max_sectors"); + } else if (xmlStrEqual(cur->name, BAD_CAST "model")) { + if (!guestModel) + guestModel = virXMLPropString(cur, "type");

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.

} } cur = cur->next; @@ -7790,6 +7794,11 @@ virDomainControllerDefParseXML(xmlNodePtr node, def->opts.pciopts.pcihole64size = VIR_DIV_UP(bytes, 1024); } } + if (guestModel) { + def->opts.pciopts.type = guestModel; + guestModel = 0;

s/0/NULL/

+ } + break;

default: break; @@ -7814,6 +7823,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, VIR_FREE(queues); VIR_FREE(cmd_per_lun); VIR_FREE(max_sectors); + VIR_FREE(guestModel);

return def;

@@ -18823,7 +18833,7 @@ virDomainControllerDefFormat(virBufferPtr buf, { const char *type = virDomainControllerTypeToString(def->type); const char *model = NULL; - bool pcihole64 = false; + bool pcihole64 = false, pciModel = false;

if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -18863,17 +18873,26 @@ virDomainControllerDefFormat(virBufferPtr buf, case VIR_DOMAIN_CONTROLLER_TYPE_PCI: if (def->opts.pciopts.pcihole64) pcihole64 = true; + if (def->opts.pciopts.type) + pciModel = true; break;

default: break; }

- if (def->queues || def->cmd_per_lun || def->max_sectors || + if (pciModel || + def->queues || def->cmd_per_lun || def->max_sectors || virDomainDeviceInfoNeedsFormat(&def->info, flags) || pcihole64) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2);

+ if (pciModel) { + virBufferAddLit(buf, "<model"); + virBufferEscapeString(buf, " type='%s'", def->opts.pciopts.type); + 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 50750c1..09fe3c0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -797,6 +797,14 @@ typedef virDomainPCIControllerOpts *virDomainPCIControllerOptsPtr; struct _virDomainPCIControllerOpts { bool pcihole64; unsigned long pcihole64size; + + /* the type is in the "model" subelement, e.g.: + * <controller type='pci' model='pcie-root-port'> + * <model type='ioh3420''/> + * ... + * similar to the model of <interface> devices. + */ + char *type; /* the exact name of the device in hypervisor */ };

/* Stores the virtual disk controller configuration */

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

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml index 05967a4..4623a5c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml @@ -20,8 +20,12 @@ <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='1' model='dmi-to-pci-bridge'> + <model type='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model type='pci-bridge'/> + </controller> <video> <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> </video> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml index 9dd4162..760830a 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml @@ -20,8 +20,12 @@ <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='1' model='dmi-to-pci-bridge'> + <model type='i82801b11-bridge'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model type='pci-bridge'/> + </controller> <controller type='sata' index='0'/> <video> <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/>

On Fri, Jul 17, 2015 at 02:43:32PM -0400, Laine Stump wrote:

This new subelement is used in PCI controllers: the toplevel *attribute* "model" of a controller denotes what kind of PCI controller is being described, e.g. a "dmi-to-pci-bridge", "pci-bridge", or "pci-root". But in the future there will be different implementations of some of those types of PCI controllers, which behave similarly from libvirt's point of view (and so should have the same model), but use a different device in qemu (and present themselves as a different piece of hardware in the guest). In an ideal world we (i.e. "I") would have thought of that back when the pci controllers were added, and used some sort of type/class/model notation (where class was used in the way we are now using model, and model was used for the actual manufacturer's model number of a particular family of PCI controller), but that opportunity is long past, so as an alternative, this patch allows selecting a particular implementation of a pci controller with the "type" attribute of the <model> subelement, e.g.:

<controller type='pci' model='dmi-to-pci-bridge' index='1'> <model type='i82801b11-bridge'/> </controller>

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.

In this case, "dmi-to-pci-bridge" is the kind of controller (one that has a single PCIe port upstream, and 32 standard PCI ports downstream, which are not hotpluggable), and the qemu device to be used to implement this kind of controller is named "i82801b11-bridge".

Implementing the above now will allow us in the future to add a new kind of dmi-to-pci-bridge that doesn't use qemu's i82801b11-bridge device, but instead uses something else (which doesn't yet exist, but qemu people have been discussing it), all without breaking existing configs.

(note that for the existing "pci-bridge" type of PCI controller, both the model attribute and <model> type are 'pci-bridge'. This is just a coincidence, since it turns out that in this case the device name in qemu really is a generic 'pci-bridge' rather than being the name of some real-world chip) --- new in V2 (previously was a part of the patch to add pcie-root-port)

docs/formatdomain.html.in | 12 ++++++++++++ docs/schemas/domaincommon.rng | 13 +++++++++++++ src/conf/domain_conf.c | 23 +++++++++++++++++++++-- src/conf/domain_conf.h | 8 ++++++++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 ++++++-- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 ++++++-- 6 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 50750c1..09fe3c0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -797,6 +797,14 @@ typedef virDomainPCIControllerOpts *virDomainPCIControllerOptsPtr; struct _virDomainPCIControllerOpts { bool pcihole64; unsigned long pcihole64size; + + /* the type is in the "model" subelement, e.g.: + * <controller type='pci' model='pcie-root-port'> + * <model type='ioh3420''/> + * ... + * similar to the model of <interface> devices. + */ + char *type; /* the exact name of the device in hypervisor */

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:

On Thu, Jul 23, 2015 at 03:18:04PM +0200, Ján Tomko wrote:

...

On Fri, Jul 17, 2015 at 02:43:32PM -0400, Laine Stump wrote:

...

This new subelement is used in PCI controllers: the toplevel *attribute* "model" of a controller denotes what kind of PCI controller is being described, e.g. a "dmi-to-pci-bridge", "pci-bridge", or "pci-root". But in the future there will be different implementations of some of those types of PCI controllers, which behave similarly from libvirt's point of view (and so should have the same model), but use a different device in qemu (and present themselves as a different piece of hardware in the guest). In an ideal world we (i.e. "I") would have thought of that back when the pci controllers were added, and used some sort of type/class/model notation (where class was used in the way we are now using model, and model was used for the actual manufacturer's model number of a particular family of PCI controller), but that opportunity is long past, so as an alternative, this patch allows selecting a particular implementation of a pci controller with the "type" attribute of the <model> subelement, e.g.:

<controller type='pci' model='dmi-to-pci-bridge' index='1'> <model type='i82801b11-bridge'/> </controller>

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.

Well, you just did. But I must say I like "model name" better too. When I pitched the "model type" I did that to show the creation of the new element, not the spelling itself.

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.

...

...

In this case, "dmi-to-pci-bridge" is the kind of controller (one that has a single PCIe port upstream, and 32 standard PCI ports downstream, which are not hotpluggable), and the qemu device to be used to implement this kind of controller is named "i82801b11-bridge".

Implementing the above now will allow us in the future to add a new kind of dmi-to-pci-bridge that doesn't use qemu's i82801b11-bridge device, but instead uses something else (which doesn't yet exist, but qemu people have been discussing it), all without breaking existing configs.

(note that for the existing "pci-bridge" type of PCI controller, both the model attribute and <model> type are 'pci-bridge'. This is just a coincidence, since it turns out that in this case the device name in qemu really is a generic 'pci-bridge' rather than being the name of some real-world chip) --- new in V2 (previously was a part of the patch to add pcie-root-port)

docs/formatdomain.html.in | 12 ++++++++++++ docs/schemas/domaincommon.rng | 13 +++++++++++++ src/conf/domain_conf.c | 23 +++++++++++++++++++++-- src/conf/domain_conf.h | 8 ++++++++ tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 8 ++++++-- tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 8 ++++++-- 6 files changed, 66 insertions(+), 6 deletions(-)

...

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 50750c1..09fe3c0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -797,6 +797,14 @@ typedef virDomainPCIControllerOpts *virDomainPCIControllerOptsPtr; struct _virDomainPCIControllerOpts { bool pcihole64; unsigned long pcihole64size; + + /* the type is in the "model" subelement, e.g.: + * <controller type='pci' model='pcie-root-port'> + * <model type='ioh3420''/> + * ... + * similar to the model of <interface> devices. + */ + char *type; /* the exact name of the device in hypervisor */

This would be nicer as an enum - we don't allow arbitrary strings there anyway.

The only place where we allow arbitrary strings is the interface model and due to that we're having some problems. Not allowing strings is good, storing the enum value is even better.

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:

On 07/24/2015 07:28 AM, Martin Kletzander wrote:

...

On Thu, Jul 23, 2015 at 03:18:04PM +0200, Ján Tomko wrote:

...

On Fri, Jul 17, 2015 at 02:43:32PM -0400, Laine Stump wrote:

...

This new subelement is used in PCI controllers: the toplevel *attribute* "model" of a controller denotes what kind of PCI controller is being described, e.g. a "dmi-to-pci-bridge", "pci-bridge", or "pci-root". But in the future there will be different implementations of some of those types of PCI controllers, which behave similarly from libvirt's point of view (and so should have the same model), but use a different device in qemu (and present themselves as a different piece of hardware in the guest). In an ideal world we (i.e. "I") would have thought of that back when the pci controllers were added, and used some sort of type/class/model notation (where class was used in the way we are now using model, and model was used for the actual manufacturer's model number of a particular family of PCI controller), but that opportunity is long past, so as an alternative, this patch allows selecting a particular implementation of a pci controller with the "type" attribute of the <model> subelement, e.g.:

<controller type='pci' model='dmi-to-pci-bridge' index='1'> <model type='i82801b11-bridge'/> </controller>

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.

Well, you just did. But I must say I like "model name" better too. When I pitched the "model type" I did that to show the creation of the new element, not the spelling itself.

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.

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

...

...

...

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 50750c1..09fe3c0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -797,6 +797,14 @@ typedef virDomainPCIControllerOpts *virDomainPCIControllerOptsPtr; struct _virDomainPCIControllerOpts { bool pcihole64; unsigned long pcihole64size; + + /* the type is in the "model" subelement, e.g.: + * <controller type='pci' model='pcie-root-port'> + * <model type='ioh3420''/> + * ... + * similar to the model of <interface> devices. + */ + char *type; /* the exact name of the device in hypervisor */

This would be nicer as an enum - we don't allow arbitrary strings there anyway.

The only place where we allow arbitrary strings is the interface model and due to that we're having some problems. Not allowing strings is good, storing the enum value is even better.

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

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

This patch provides qemu support for the contents of <model> in <controller> for the two existing PCI controller types that need it (i.e. the two controller types that are backed by a device that must be specified on the qemu commandline):

1) pci-bridge - sets <model> type attribute default as "pci-bridge"

2) dmi-to-pci-bridge - sets <model> type attribute default as "i82801b11-bridge".

These both match current hardcoded practice.

The defaults are set at the end of qemuDomainAssignPCIAddresses(), It can't be done earlier because some of the options that will be autogenerated need full PCI address info for the controller and because qemuDomainAssignPCIAddresses() might create extra controllers which would need default settings added. This is still prior to the XML being written to disk, though, so the autogenerated defaults are persistent.

qemu capabilities bits aren't checked until the commandline is actually created (so the domain can possibly be defined on a host that doesn't yet have support for the give n device, or a host different from the one where it will eventually be run). At that time we compare the type strings to known qemu device names and check for the capabilities bit for that device. --- new in V2 (previously was a part of the patch to add pcie-root-port)

src/qemu/qemu_command.c | 70 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 64 insertions(+), 6 deletions(-)

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 ;-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 74f02f5..8868e18 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2243,11 +2243,37 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, virDomainControllerDefPtr cont = def->controllers[i]; int idx = cont->idx; virDevicePCIAddressPtr addr; + virDomainPCIControllerOptsPtr options; + const char *deviceName = NULL;

if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) continue;

addr = &cont->info.addr.pci; + options = &cont->opts.pciopts; + + /* set defaults for any other auto-generated config + * options for this controller that haven't been + * specified in config. + */ + switch ((virDomainControllerModelPCI)cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + if (!options->type) + deviceName = "pci-bridge"; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + if (!options->type) + deviceName = "i82801b11-bridge"; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + break; + } + if (deviceName && + VIR_STRDUP(options->type, deviceName) < 0) + goto cleanup; +

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

/* check if every PCI bridge controller's ID is greater than * the bus it is placed onto */ @@ -4614,17 +4640,49 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, } switch (def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=%s", + if (!def->opts.pciopts.type) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pci-bridge options not set")); + goto error; + } + if (STREQ(def->opts.pciopts.type, "pci-bridge")) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pci-bridge controller " + "is not supported in this QEMU binary")); + goto error; + } + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown pci-bridge device '%s'"),

unknown pci-bridge option model '%s' ?

+ def->opts.pciopts.type); + goto error; + }

Alternatively if (STREQ_NULLABLE() { } else { error message using NULLSTR(def->opts.pciopts.type) I don't care either way - it's obvious

+ virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s", + def->opts.pciopts.type, def->idx, def->info.alias); 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")); + if (!def->opts.pciopts.type) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated dmi-to-pci-bridge options not set")); + goto error; + } + if (STREQ(def->opts.pciopts.type, "i82801b11-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; + } + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown dmi-to-pci-bridge device '%s'"),

unknown dmi-to-pci-bridge option model '%s' ??

+ def->opts.pciopts.type); goto error; }

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

- virBufferAsprintf(&buf, "i82801b11-bridge,id=%s", def->info.alias); + virBufferAsprintf(&buf, "%s,id=%s", + def->opts.pciopts.type, def->info.alias); break; } break;

On 07/23/2015 09:41 AM, Ján Tomko wrote:

[Reducing the cc-list for this one too]

On Fri, Jul 17, 2015 at 02:43:33PM -0400, Laine Stump wrote:

...

This patch provides qemu support for the contents of <model> in <controller> for the two existing PCI controller types that need it (i.e. the two controller types that are backed by a device that must be specified on the qemu commandline):

1) pci-bridge - sets <model> type attribute default as "pci-bridge"

2) dmi-to-pci-bridge - sets <model> type attribute default as "i82801b11-bridge".

These both match current hardcoded practice.

The defaults are set at the end of qemuDomainAssignPCIAddresses(), It can't be done earlier because some of the options that will be autogenerated need full PCI address info for the controller and because qemuDomainAssignPCIAddresses() might create extra controllers which would need default settings added. This is still prior to the XML being written to disk, though, so the autogenerated defaults are persistent.

qemu capabilities bits aren't checked until the commandline is actually created (so the domain can possibly be defined on a host that doesn't yet have support for the give n device, or a host different from the one where it will eventually be run). At that time we compare the type strings to known qemu device names and check for the capabilities bit for that device. --- new in V2 (previously was a part of the patch to add pcie-root-port)

src/qemu/qemu_command.c | 70 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 64 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 74f02f5..8868e18 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2243,11 +2243,37 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, virDomainControllerDefPtr cont = def->controllers[i]; int idx = cont->idx; virDevicePCIAddressPtr addr; + virDomainPCIControllerOptsPtr options; + const char *deviceName = NULL;

if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) continue;

addr = &cont->info.addr.pci; + options = &cont->opts.pciopts; + + /* set defaults for any other auto-generated config + * options for this controller that haven't been + * specified in config. + */ Setting the defaults here feels out of place.

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.

If we set them in the PostParse callbacks, they will also get formatted back (which should not matter when migrating to older libvirt, as it does not parse it anyway), which is what we do for lots of other default values.

That way only the controllers auto-added a few lines above this code would need their chassis filled.

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

Jan

On 07/22/2015 04:20 PM, John Ferlan wrote:

On 07/17/2015 02:43 PM, Laine Stump wrote:

...

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(-)

Again assuming this is the generally acceptable mechanism. It seems fine to me though

...

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 s/also// + subelement <code><target></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><target></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> 1.2.18

...

+ </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><target></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). Is there a valid range that needs to be documented? See comments for rng and parse.

As far as I understand from Alex, it is 0-255

...

+ </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'/> This is an int in domain_conf.h and is set to -1 in virDomainControllerDefNew.

So one or the other seems to need adjustment since by this rule we can only be between 0 and 255 inclusive.

"-1" is used to indicate "not specified" (of course when it isn't specified, it is auto-filled later on).

...

+ </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"); Similar to note about guestModel - multiple chassisNr aren't allowed by RNG rules...

...

} } 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) {

^ one extra space too many (just had a shot of coffee so I saw it!)

Also if the range is truly 0 to 255 inclusive, then don't we need to test for that here as well?

Yes. I'll put that in.

...

+ 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; [this]

...

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) {

This can only be true if chassisNr != -1

...

+ virBufferAddLit(buf, "<target"); + if (def->opts.pciopts.chassisNr != -1) [which means this if isn't necessary]

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.

...

+ virBufferAsprintf(buf, " chassisNr='%d'", + def->opts.pciopts.chassisNr); + virBufferAddLit(buf, "/>\n"); and of course means the whole thing can be formatted in one line

Not after patch

...

+ } + 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 { Similar to previous comment about "reusing" a test - IDC, but perhaps this should have been a "new" test which we're building upon rather than stealing an existing one.

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

On 07/22/2015 05:38 PM, John Ferlan wrote:

On 07/17/2015 02:43 PM, Laine Stump wrote:

...

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

Feels like two things happening in this patch - adding pcie-root and adding chassis/port attributes. Are they separable - if so this patch should be further split into two if only to aid bisection

Not really. You can't test chassis/port without having a controller type that they are used with.

...

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>) 1.2.18

...

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><model></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><model></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 << 3) + function (although this could change in + the future). + </dd>

I see from the code that this is printed as a hex number - something we should perhaps document here.

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

...

</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>)

1.2.18

Seems to me we should state in some way that it's only supported on q35 machine for qemu 1.2.2 and beyond (where 1.2.2 is the patch 8 comment)

If added to other machine types, I assume it's ignored...

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.

...

+ </p> <pre> ... <devices> 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/> Similar to chassisNr - defined in domain_conf.h as 'int's, so limiting in RNG to 0 to 255 inclusive doesn't seem right.

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.

Unless of course it is right in which case domain_conf needs changes...

The parser needs to check limits, and it now does.

FWIW: A change to the .xml in this patch and the .args file in the following patch to change the port to 0x11a "failed" schematest, but passed libvirt's other tests

Not any more :-)

...

</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");

Similar to earlier comments - RNG doesn't allow multiple entries, so is it necessary to check !chassis and !port? E.G. if they're already found, then it probably should be an error or at least documented only the first is consumed.

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.

Also if there is supposed to be a range, it needs to be checked here.

See above.

That is the RNG expects 0 to 255 inclusive, but that's not handled here.

...

} } 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);

OH... And this is what I get for not reading ahead... So now my previous comments are unfounded... <sigh>

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.

Also I see that ports is saved as a hex value - that's something we should document in formatdomain and I noted above.

ACK as I trust you can handle the nits and range checks that may be necessary.

John

On 07/17/2015 02:43 PM, Laine Stump wrote: missed on first pass

<devices> 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/>

This <empty/> seems to be off a space - should it line up under optional and not element? John

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

On 07/24/2015 12:07 PM, Martin Kletzander wrote:

On Fri, Jul 17, 2015 at 02:43:37PM -0400, Laine Stump wrote:

...

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 @@ -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><model></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).

Talking about QEMU-only options it feels like we're not abstracting anything although trying to be an abstraction layer. But I don't have any better explanation, so it's fine, I guess :)

Also I wouldn't mention how the number is calculated, just so you don't need to mention that it might change.

...

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 @@ -7811,6 +7820,20 @@ virDomainControllerDefParseXML(xmlNodePtr node, chassisNr); goto error; } + if (chassis && virStrToLong_i(chassis, NULL, 0, + &def->opts.pciopts.chassis) < 0) {

You probably want to use _ui variants not just here, but throughout the patches.

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)

...

@@ -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);

Just out of curiosity, why did you choose to format chassis as decimal and and port as hexadecimal?

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 :-)

Negative tests wouldn't hurt ;)

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:

This is backed by the qemu device ioh3420.

chassis and port from the <model> subelement are used to store/set the respective qemu device options for the ioh3420. Currently, chassis is set to be the index of the controller, and port is set to "(slot << 3) + function" (per suggestion from Alex Williamson). --- src/qemu/qemu_command.c | 40 ++++++++++++++++++++++ .../qemuxml2argv-pcie-root-port.args | 10 ++++++ tests/qemuxml2argvtest.c | 7 ++++ 3 files changed, 57 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 53f5317..1b86f1d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1598,6 +1598,12 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, */ flags = VIR_PCI_CONNECT_TYPE_PCIE; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + /* pcie-root-port can only connect to pcie-root, isn't + * hot-pluggable + */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_ROOT; + break; default: break; } @@ -2268,6 +2274,13 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, deviceName = "i82801b11-bridge"; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + if (!options->type) + deviceName = "ioh3420"; + if (options->chassis == -1) + options->chassis = cont->idx; + if (options->port == -1) + options->port = (addr->slot << 3) + addr->function; + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: @@ -2382,6 +2395,10 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, */ flags = VIR_PCI_CONNECT_TYPE_PCIE; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + /* pcie-root-port can only plug into pcie-root */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_ROOT; + break; default: flags = VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI; break; @@ -4687,6 +4704,29 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, virBufferAsprintf(&buf, "%s,id=%s", def->opts.pciopts.type, def->info.alias); break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + if (!def->opts.pciopts.type) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pcie-root-port options not set")); + goto error; + } + if (STREQ(def->opts.pciopts.type, "ioh3420")) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pcie-root-port (ioh3420) " + "controller is not supported in this QEMU binary")); + goto error; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pcie-root-port device '%s'"), + def->opts.pciopts.type);

Similar to 06/17 unknown pcii-root-port option model '%s' ACK John

+ 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-root-port.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args new file mode 100644 index 0000000..5e4891c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args @@ -0,0 +1,10 @@ +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 ioh3420,port=0x1a,chassis=40,id=pci.4,bus=pcie.0,addr=0x3 \ +-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 f5cbe3a..0129717 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1496,6 +1496,13 @@ 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-root-port", + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + 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,

On 07/24/2015 12:14 PM, Martin Kletzander wrote:

On Fri, Jul 17, 2015 at 02:43:38PM -0400, Laine Stump wrote:

...

This is backed by the qemu device ioh3420.

chassis and port from the <model> subelement are used to store/set the respective qemu device options for the ioh3420. Currently, chassis is set to be the index of the controller, and port is set to "(slot << 3) + function" (per suggestion from Alex Williamson). --- src/qemu/qemu_command.c | 40 ++++++++++++++++++++++ .../qemuxml2argv-pcie-root-port.args | 10 ++++++ tests/qemuxml2argvtest.c | 7 ++++ 3 files changed, 57 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 53f5317..1b86f1d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4687,6 +4704,29 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, virBufferAsprintf(&buf, "%s,id=%s", def->opts.pciopts.type, def->info.alias); break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: + if (!def->opts.pciopts.type) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated pcie-root-port options not set")); + goto error; + }

I wonder how these errors can happen when the parsing itself will always set them up.

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.

...

+ if (STREQ(def->opts.pciopts.type, "ioh3420")) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pcie-root-port (ioh3420) " + "controller is not supported in this QEMU binary")); + goto error; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pcie-root-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;

This whole hunk would be way shorter with the type stored as an enum

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:

This controller can be connected only to a pcie-root-port or a pcie-switch-downstream-port (which will be added in a later patch), which is the reason for the new connect type VIR_PCI_CONNECT_TYPE_PCIE_PORT. A pcie-switch-upstream-port provides 32 ports (slot=0 to slot=31) on the downstream side, which can only have pci controllers of model "pcie-switch-downstream-port" plugged into them, which is the reason for the other new connect type VIR_PCI_CONNECT_TYPE_PCIE_SWITCH. ---

V2:

* Only allow connecting a pcie-upstream-port to a pcie-root-port or pcie-switch-downstream-port (V1 allowed connecting to any PCIe port, which doesn't work)

* change test case to reflect additional pcie-root-port between pcie-root and pcie-switch-upstream-port

docs/formatdomain.html.in | 5 +-- docs/schemas/domaincommon.rng | 3 ++ src/conf/domain_addr.c | 15 +++++++-- src/conf/domain_addr.h | 9 +++++- src/conf/domain_conf.c | 3 +- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 1 + .../qemuxml2argv-pcie-switch-upstream-port.xml | 37 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 9 files changed, 69 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index dcbca75..eb5b9b7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3025,10 +3025,11 @@ PCI controllers have an optional <code>model</code> attribute with possible values <code>pci-root</code>, <code>pcie-root</code>, <code>pcie-root-port</code>, <code>pci-bridge</code>, - or <code>dmi-to-pci-bridge</code>. + <code>dmi-to-pci-bridge</code>, or <code>pcie-switch-upstream-port</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>, pcie-root-port <span class="since">since 1.3.0</span>) + 1.1.2</span>, pcie-root-port and + pcie-switch-upstream-port <span class="since">since 1.3.0</span>)

1.2.18

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

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

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 0efa0f0..8b2f29c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1741,6 +1741,8 @@ <value>i82801b11-bridge</value> <!-- implementations of 'pcie-root-port' --> <value>ioh3420</value> + <!-- implementations of 'pcie-switch-upstream-port' --> + <value>x3130-upstream</value> </choice> </attribute> <empty/> @@ -1787,6 +1789,7 @@ <value>pci-bridge</value> <value>dmi-to-pci-bridge</value> <value>pcie-root-port</value> + <value>pcie-switch-upstream-port</value> </choice> </attribute> </group> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index fba0eff..e40a66c 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -197,11 +197,22 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, 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; + /* provides one slot which is pcie, can be used by devices + * that must connect to some type of "pcie-*-port", and + * is hotpluggable + */ + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE + | VIR_PCI_CONNECT_TYPE_PCIE_PORT + | VIR_PCI_CONNECT_HOTPLUGGABLE; bus->minSlot = 0; bus->maxSlot = 0; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + /* 31 slots, can only accept pcie-switch-port, no hotplug */ + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE_SWITCH; + bus->minSlot = 0; + bus->maxSlot = 31; + break;

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

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 2a0ff96..2220a79 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -41,6 +41,12 @@ typedef enum { /* 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) */ + VIR_PCI_CONNECT_TYPE_PCIE_PORT = 1 << 5, + /* devices that can only connect to a pcie-root-port + * or pcie-downstream-switch-port + */ + VIR_PCI_CONNECT_TYPE_PCIE_SWITCH = 1 << 6, + /* devices that can only connect to a pcie-switch */ } virDomainPCIConnectFlags;

typedef struct { @@ -73,7 +79,8 @@ typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; */ # define VIR_PCI_CONNECT_TYPES_MASK \ (VIR_PCI_CONNECT_TYPE_PCI | VIR_PCI_CONNECT_TYPE_PCIE | \ - VIR_PCI_CONNECT_TYPE_PCIE_ROOT) + VIR_PCI_CONNECT_TYPE_PCIE_ROOT | VIR_PCI_CONNECT_TYPE_PCIE_PORT | \ + VIR_PCI_CONNECT_TYPE_PCIE_SWITCH)

/* 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 e02c861..4eeaa84 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -325,7 +325,8 @@ VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pcie-root", "pci-bridge", "dmi-to-pci-bridge", - "pcie-root-port") + "pcie-root-port", + "pcie-switch-upstream-port")

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 a4df2a6..b49c803 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -753,6 +753,7 @@ typedef enum { 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_PCIE_SWITCH_UPSTREAM_PORT,

VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST } virDomainControllerModelPCI; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1b86f1d..725360c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2281,6 +2281,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (options->port == -1) options->port = (addr->slot << 3) + addr->function; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_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-switch-upstream-port.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml new file mode 100644 index 0000000..13125a9 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml @@ -0,0 +1,37 @@ +<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'/> + <controller type='pci' index='5' model='pcie-switch-upstream-port'/> + <controller type='pci' index='6' model='pcie-switch-upstream-port'> + <model type='x3130-upstream'/> + </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 6b48bf4..80686ae 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -568,6 +568,7 @@ mymain(void) DO_TEST_DIFFERENT("pcie-root"); DO_TEST_DIFFERENT("q35"); DO_TEST("pcie-root-port"); + DO_TEST("pcie-switch-upstream-port");

DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi");

On 07/17/2015 02:43 PM, Laine Stump wrote:

this is backed by the qemu device x3130-upstream. It can only plug into a pcie-root-port or pcie-switch-downstream-port. ---

V2: change test case to reflect additional pcie-root-port between pcie-root and pcie-switch-upstream-port

src/qemu/qemu_command.c | 37 ++++++++++++++++++++++ .../qemuxml2argv-pcie-switch-upstream-port.args | 12 +++++++ tests/qemuxml2argvtest.c | 9 ++++++ 3 files changed, 58 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 725360c..30f54ce 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1604,6 +1604,12 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, */ flags = VIR_PCI_CONNECT_TYPE_PCIE_ROOT; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + /* pcie-switch can only connect to a true + * pcie bus, and can't be hot-plugged. + */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_PORT; + break; default: break; } @@ -2282,6 +2288,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, options->port = (addr->slot << 3) + addr->function; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + if (!options->type) + deviceName = "x3130-upstream"; + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: @@ -2400,6 +2409,12 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, /* pcie-root-port can only plug into pcie-root */ flags = VIR_PCI_CONNECT_TYPE_PCIE_ROOT; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + /* pcie-switch really does need a real PCIe + * port, but it doesn't need to be pcie-root + */ + flags = VIR_PCI_CONNECT_TYPE_PCIE_PORT; + break; default: flags = VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI; break; @@ -4728,6 +4743,28 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, def->opts.pciopts.type, def->opts.pciopts.port, def->opts.pciopts.chassis, def->info.alias); break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: + if (!def->opts.pciopts.type) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("autogenerated dmi-to-pci-bridge options not set")); + goto error; + } + if (STREQ(def->opts.pciopts.type, "x3130-upstream")) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the pcie-switch-upstream-port (x3130-upstream) " + "controller is not supported in this QEMU binary")); + goto error; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pcie-switch-upstream-port device '%s'"),

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

+ def->opts.pciopts.type); + goto error; + } + virBufferAsprintf(&buf, "%s,id=%s", + def->opts.pciopts.type, def->info.alias); + break; } break;

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args new file mode 100644 index 0000000..c08cd4a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args @@ -0,0 +1,12 @@ +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 ioh3420,port=0x18,chassis=4,id=pci.4,bus=pcie.0,addr=0x3 \ +-device x3130-upstream,id=pci.5,bus=pci.3,addr=0x0 \ +-device x3130-upstream,id=pci.6,bus=pci.4,addr=0x0 \ +-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 0129717..b42d483 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1504,6 +1504,15 @@ mymain(void) QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);

+ DO_TEST("pcie-switch-upstream-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_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, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI,

On 07/17/2015 02:43 PM, Laine Stump wrote:

The downstream ports of an x3130-upstream switch can each have one of these plugged into them (and that is the only place they can be connected). Each xio3130-downstream provides a single PCIe port that can have PCI or PCIe devices hotplugged into it. Apparently an entire set of x3130-upstream + several xio3130-downstreams can be hotplugged as a unit, but it's not clear to me yet how that would be done, since qemu only allows attaching a single device at a time.

This device will be used to implement the "pcie-switch-port" model of pci controller. --- unchanged from V1

src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_1.2.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.3.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.4.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.5.3-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.50-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemuhelptest.c | 6 ++++-- 10 files changed, 14 insertions(+), 2 deletions(-)

Repetitive comments from prior reviews (9 & 12) Otherwise seems fine. John

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 9c61387..361d738 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -290,6 +290,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "migration-event", "ioh3420", "x3130-upstream", + "xio3130-downstream", );

@@ -1572,6 +1573,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "pci-serial", QEMU_CAPS_DEVICE_PCI_SERIAL }, { "ioh3420", QEMU_CAPS_DEVICE_IOH3420 }, { "x3130-upstream", QEMU_CAPS_DEVICE_X3130_UPSTREAM }, + { "xio3130-downstream", QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM }, };

static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index e677065..2429ff7 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -233,6 +233,7 @@ typedef enum { QEMU_CAPS_MIGRATION_EVENT = 191, /* MIGRATION event */ QEMU_CAPS_DEVICE_IOH3420 = 192, /* -device ioh3420 */ QEMU_CAPS_DEVICE_X3130_UPSTREAM = 193, /* -device x3130-upstream */ + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM = 194, /* -device xio3130-downstream */

QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps index 78d7b82..ba16635 100644 --- a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps @@ -122,4 +122,5 @@ <flag name='qxl-vga.vgamem_mb'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.3.1-1.caps b/tests/qemucapabilitiesdata/caps_1.3.1-1.caps index 7cec7f9..51cd6d9 100644 --- a/tests/qemucapabilitiesdata/caps_1.3.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.3.1-1.caps @@ -137,4 +137,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.4.2-1.caps b/tests/qemucapabilitiesdata/caps_1.4.2-1.caps index f5f0034..03d0a3e 100644 --- a/tests/qemucapabilitiesdata/caps_1.4.2-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.4.2-1.caps @@ -138,4 +138,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.5.3-1.caps b/tests/qemucapabilitiesdata/caps_1.5.3-1.caps index 9f0461a..e2f22e4 100644 --- a/tests/qemucapabilitiesdata/caps_1.5.3-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.5.3-1.caps @@ -147,4 +147,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.6.0-1.caps b/tests/qemucapabilitiesdata/caps_1.6.0-1.caps index 1b23b82..874a050 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.6.0-1.caps @@ -153,4 +153,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.6.50-1.caps b/tests/qemucapabilitiesdata/caps_1.6.50-1.caps index ff0427f..dd3bcda 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.50-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.6.50-1.caps @@ -153,4 +153,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps index 56b27e5..3ee2d6f 100644 --- a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps @@ -169,4 +169,5 @@ <flag name='pci-serial'/> <flag name='ioh3420'/> <flag name='x3130-upstream'/> + <flag name='xio3130-downstream'/> </qemuCaps> diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 62b9a0c..8f317d4 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -755,7 +755,8 @@ mymain(void) QEMU_CAPS_SPLASH_TIMEOUT, QEMU_CAPS_DEVICE_IVSHMEM, QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_DEVICE_X3130_UPSTREAM); + QEMU_CAPS_DEVICE_X3130_UPSTREAM, + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM); DO_TEST("qemu-1.1.0", 1001000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -857,7 +858,8 @@ mymain(void) QEMU_CAPS_SPLASH_TIMEOUT, QEMU_CAPS_DEVICE_IVSHMEM, QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_DEVICE_X3130_UPSTREAM); + QEMU_CAPS_DEVICE_X3130_UPSTREAM, + QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM); DO_TEST_FULL("qemu-1.2.0", 1002000, 0, 0, VIR_ERR_CONFIG_UNSUPPORTED, QEMU_CAPS_LAST); DO_TEST_FULL("qemu-kvm-1.2.0", 1002000, 1, 0, VIR_ERR_CONFIG_UNSUPPORTED,

On 07/17/2015 02:43 PM, Laine Stump wrote:

This controller can be connected only to a port on a pcie-switch-upstream-port. It provides a single hotpluggable port that will accept any PCI or PCIe device, as well as any device requiring a pcie-*-port (the only current example of such a device is the pcie-switch-upstream-port). --- 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.

docs/formatdomain.html.in | 51 ++++++++++++++-------- docs/schemas/domaincommon.rng | 3 ++ src/conf/domain_addr.c | 11 +++++ src/conf/domain_conf.c | 3 +- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 1 + .../qemuxml2argv-pcie-switch-downstream-port.xml | 44 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 8 files changed, 95 insertions(+), 20 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index eb5b9b7..c2023d0 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3025,11 +3025,12 @@ PCI controllers have an optional <code>model</code> attribute with possible values <code>pci-root</code>, <code>pcie-root</code>, <code>pcie-root-port</code>, <code>pci-bridge</code>, - <code>dmi-to-pci-bridge</code>, or <code>pcie-switch-upstream-port</code>. + <code>dmi-to-pci-bridge</code>, <code>pcie-switch-upstream-port</code>, or + <code>pcie-switch-downstream-port</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>, pcie-root-port and - pcie-switch-upstream-port <span class="since">since 1.3.0</span>) + 1.1.2</span>, pcie-root-port, pcie-switch-upstream-port, and + pcie-switch-downstream-port <span class="since">since 1.3.0</span>)

1.2.18

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 @@ -3074,8 +3075,8 @@ </dd> <dt><code>chassis</code></dt> <dd> - pcie-root-port controllers can also have - a <code>chassis</code> attribute in + pcie-root-port and pcie-switch-downstream-port controllers can + also have a <code>chassis</code> attribute in the <code><model></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 @@ -3083,13 +3084,13 @@ </dd> <dt><code>port</code></dt> <dd> - pcie-root-port controllers can also have a <code>port</code> - attribute in the <code><model></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 << 3) + function (although this could change in - the future). + pcie-root-port and pcie-switch-downstream-port controllers can + also have a <code>port</code> attribute in + the <code><model></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 << 3) + + function (although this could change in the future). </dd> </dl> <p> @@ -3139,14 +3140,26 @@ </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 + with <code>model='pcie-root-port'</code>, + <code>model='pcie-switch-upstream-port'</code>, + and <code>model='pcie-switch-downstream-port'</code>. pcie-root-port + 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>) + is started). pcie-switch-upstream-port is a more flexible (but + also more complex) device that can only plug into a + pcie-root-port or pcie-switch-downstream-port on the upstream + side, and provides 32 ports on the downstream side (slot='0' - + slot='31') that accept only pcie-switch-downstream-port devices; + each pcie-switch-downstream-port device can only plug into a + pcie-switch-upstream-port on its upstream side, and on its + downstream side provides a single hotpluggable pcie port that + can accept any standard pci or pcie device (or another + pcie-switch-upstream-port). (<span class="since">since + 1.3.0</span>)

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!

</p> <pre> ... diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8b2f29c..f465ec4 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1743,6 +1743,8 @@ <value>ioh3420</value> <!-- implementations of 'pcie-switch-upstream-port' --> <value>x3130-upstream</value> + <!-- implementations of 'pcie-switch-downstream-port' --> + <value>xio3130-downstream</value> </choice> </attribute> <empty/> @@ -1790,6 +1792,7 @@ <value>dmi-to-pci-bridge</value> <value>pcie-root-port</value> <value>pcie-switch-upstream-port</value> + <value>pcie-switch-downstream-port</value> </choice> </attribute> </group> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index e40a66c..40ee659 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -213,6 +213,17 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->minSlot = 0; bus->maxSlot = 31; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: + /* provides one slot which is pcie, can be used by devices + * that must connect to some type of "pcie-*-port", and + * is hotpluggable + */ + bus->flags = VIR_PCI_CONNECT_TYPE_PCIE + | VIR_PCI_CONNECT_TYPE_PCIE_PORT + | 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_conf.c b/src/conf/domain_conf.c index 4eeaa84..adf0af4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -326,7 +326,8 @@ VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST, "pci-bridge", "dmi-to-pci-bridge", "pcie-root-port", - "pcie-switch-upstream-port") + "pcie-switch-upstream-port", + "pcie-switch-downstream-port")

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 b49c803..94b71bc 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -754,6 +754,7 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE, VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT, VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT, + VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT,

VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST } virDomainControllerModelPCI; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 30f54ce..e3f0663 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2291,6 +2291,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (!options->type) deviceName = "x3130-upstream"; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:

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

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml new file mode 100644 index 0000000..ab32d44 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-downstream-port.xml @@ -0,0 +1,44 @@ +<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-switch-upstream-port'/> + <controller type='pci' index='5' model='pcie-switch-downstream-port'/> + <controller type='pci' index='6' model='pcie-switch-downstream-port'/> + <controller type='pci' index='7' model='pcie-switch-downstream-port'/> + <controller type='pci' index='8' model='pcie-switch-downstream-port'> + <model type='xio3130-downstream'/> + <target chassis='30' port='0x27'/> + </controller> + <controller type='pci' index='9' model='pcie-switch-upstream-port'/> + <controller type='pci' index='10' model='pcie-switch-downstream-port'/> + <controller type='pci' index='11' model='pcie-switch-downstream-port'/> + <controller type='pci' index='12' model='pcie-switch-downstream-port'/> + <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 80686ae..d5a565f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -569,6 +569,7 @@ mymain(void) DO_TEST_DIFFERENT("q35"); DO_TEST("pcie-root-port"); DO_TEST("pcie-switch-upstream-port"); + DO_TEST("pcie-switch-downstream-port");

DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi");