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