Re: [libvirt] [PATCHv2 10/17] conf: new pci controller model "pcie-root-port"

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

> 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>&lt;model&gt;</code> subelement, which is used to > + control QEMU's "chassis" option for the device (normally > + libvirt automatically sets this to the same value as the index > + attribute of the pci controller). > + </dd> > + <dt><code>port</code></dt> > + <dd> > + pcie-root-port controllers can also have a <code>port</code> > + attribute in the <code>&lt;model&gt;</code> subelement, which > + is used to control QEMU's "port" option for the device > + (normally libvirt automatically sets this based on the PCI > + address where the root port is connected using the formula > + (slot &lt;&lt; 3) + function (although this could change in > + the future). > + </dd> I see from the code that this is printed as a hex number - something we should perhaps document here.

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

> + </p> > <pre> > ... > &lt;devices&gt; > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 1b1f592..0efa0f0 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -1739,6 +1739,8 @@ > <value>pci-bridge</value> > <!-- implementations of 'dmi-to-pci-bridge' --> > <value>i82801b11-bridge</value> > + <!-- implementations of 'pcie-root-port' --> > + <value>ioh3420</value> > </choice> > </attribute> > <empty/> > @@ -1751,7 +1753,17 @@ > <ref name='uint8range'/> > </attribute> > </optional> > - <empty/> > + <optional> > + <attribute name="chassis"> > + <ref name='uint8range'/> > + </attribute> > + </optional> > + <optional> > + <attribute name="port"> > + <ref name='uint8range'/> > + </attribute> > + </optional> > + <empty/> Similar to chassisNr - defined in domain_conf.h as 'int's, so limiting in RNG to 0 to 255 inclusive doesn't seem right.

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

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

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

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

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>

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