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

John Ferlan jferlan at redhat.com
Wed Jul 22 21:38:08 UTC 2015



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

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

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

>              virBufferAddLit(buf, "/>\n");
>          }
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 32d1073..a4df2a6 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -752,6 +752,7 @@ typedef enum {
>      VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT,
>      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_PCI_LAST
>  } virDomainControllerModelPCI;
> @@ -813,7 +814,11 @@ struct _virDomainPCIControllerOpts {
>       * compatibility.
>       */
>      int chassisNr; /* used by pci-bridge, -1 == unspecified */
> - };
> +    /* chassis & port used by
> +     * pcie-root-port/pcie-switch-downstream-port, -1 = unspecified */
> +    int chassis;
> +    int port;
> +};
>  
>  /* Stores the virtual disk controller configuration */
>  struct _virDomainControllerDef {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 9f15eb6..53f5317 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2267,6 +2267,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>                      if (!options->type)
>                          deviceName = "i82801b11-bridge";
>                      break;
> +                case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_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-root-port.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml
> new file mode 100644
> index 0000000..38418bb
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.xml
> @@ -0,0 +1,36 @@
> +<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'>
> +      <model type='ioh3420'/>
> +      <target chassis='40' port='0x1a'/>
> +    </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 7a41a18..6b48bf4 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -567,6 +567,7 @@ mymain(void)
>      DO_TEST_DIFFERENT("pci-autoadd-idx");
>      DO_TEST_DIFFERENT("pcie-root");
>      DO_TEST_DIFFERENT("q35");
> +    DO_TEST("pcie-root-port");
>  
>      DO_TEST("hostdev-scsi-lsi");
>      DO_TEST("hostdev-scsi-virtio-scsi");
> 




More information about the libvir-list mailing list