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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list