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

Laine Stump laine at laine.org
Fri Jul 24 22:04:21 UTC 2015


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
>




More information about the libvir-list mailing list