[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 2/7] qemu: add pcie-root controller



On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine laine org> wrote:
> This controller is implicit on q35 machinetypes. It provides 31 PCIe
> (*not* PCI) slots as controller 0.
>
> Currently there are no devices that can connect to pcie-root, and no
> implicit pci controller on a q35 machine, so q35 is still
> unusable. For a usable q35 system, we need to add a
> "dmi-to-pci-bridge" pci controller, which can connect to pcie-root,
> and provides standard pci slots that can be used to connect other
> devices.
> ---
>  docs/formatdomain.html.in                          | 27 ++++++++++++++++------
>  docs/schemas/domaincommon.rng                      |  1 +
>  src/conf/domain_conf.c                             |  8 ++++---
>  src/conf/domain_conf.h                             |  1 +
>  src/qemu/qemu_command.c                            | 23 ++++++++++++++----
>  src/qemu/qemu_command.h                            |  4 +++-
>  src/qemu/qemu_domain.c                             | 19 +++++++++++----
>  tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args |  4 ++++
>  tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml  | 21 +++++++++++++++++
>  tests/qemuxml2argvtest.c                           |  2 ++
>  tests/qemuxml2xmltest.c                            |  1 +
>  11 files changed, 92 insertions(+), 19 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 49c7c8d..330dca2 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2338,13 +2338,11 @@
>
>      <p>
>        PCI controllers have an optional <code>model</code> attribute with
> -      possible values <code>pci-root</code> or <code>pci-bridge</code>.
> -      For machine types which provide an implicit pci bus, the pci-root
> +      possible values <code>pci-root</code>, <code>pcie-root</code>
> +      or <code>pci-bridge</code>.
> +      For machine types which provide an implicit PCI bus, the pci-root
>        controller with index=0 is auto-added and required to use PCI devices.
> -      PCI root has no address.
> -      PCI bridges are auto-added if there are too many devices to fit on
> -      the one bus provided by pci-root, or a PCI bus number greater than zero
> -      was specified.
> +      pci-root has no address.
>        PCI bridges can also be specified manually, but their addresses should
>        only refer to PCI buses provided by already specified PCI controllers.
>        Leaving gaps in the PCI controller indexes might lead to an invalid
> @@ -2356,11 +2354,26 @@
>    &lt;devices&gt;
>      &lt;controller type='pci' index='0' model='pci-root'/&gt;
>      &lt;controller type='pci' index='1' model='pci-bridge'&gt;
> -      &lt;address type='pci' domain='0' bus='0' slot='5' function='0' multifunction=off'/&gt;
> +      &lt;address type='pci' domain='0' bus='0' slot='5' function='0' multifunction='off'/&gt;
>      &lt;/controller&gt;
>    &lt;/devices&gt;
>    ...</pre>

Add the missing update to the since section from patch 3/7.

>
> +    <p>
> +      For machine types which provide an implicit PCI Express (PCIe)
> +      bus (for example, the machine types based on the Q35 chipset),
> +      the pcie-root controller with index=0 is auto-added to the
> +      domain's configuration. pcie-root has also no address, provides
> +      31 slots (numbered 1-31) and can only be used to attach PCIe
> +      devices.  (<span class="since">since 1.1.2</span>).
> +    </p>
> +<pre>
> +  ...
> +  &lt;devices&gt;
> +    &lt;controller type='pci' index='0' model='pcie-root'/&gt;
> +  &lt;/devices&gt;
> +  ...</pre>
> +
>      <h4><a name="elementsLease">Device leases</a></h4>
>
>      <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 745b959..e04be12 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1538,6 +1538,7 @@
>              <attribute name="model">
>                <choice>
>                  <value>pci-root</value>
> +                <value>pcie-root</value>
>                  <value>pci-bridge</value>
>                </choice>
>              </attribute>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 63350b6..59a96f2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -310,6 +310,7 @@ VIR_ENUM_IMPL(virDomainController, VIR_DOMAIN_CONTROLLER_TYPE_LAST,
>
>  VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST,
>                "pci-root",
> +              "pcie-root",
>                "pci-bridge")
>
>  VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST,
> @@ -5714,16 +5715,17 @@ virDomainControllerDefParseXML(xmlNodePtr node,
>      case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
>          switch (def->model) {
>          case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +        case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
>              if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
>                  virReportError(VIR_ERR_XML_ERROR, "%s",
> -                               _("pci-root controller should not "
> +                               _("pci-root and pcie-root controllers should not "
>                                   "have an address"));
>                  goto error;
>              }
>              if (def->idx != 0) {
>                  virReportError(VIR_ERR_XML_ERROR, "%s",
> -                               _("pci-root controller should have "
> -                                 "index 0"));
> +                               _("pci-root and pcie-root controllers "
> +                                 "should have index 0"));
>                  goto error;
>              }
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index de3b59c..63a1444 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -768,6 +768,7 @@ enum virDomainControllerType {
>
>  typedef enum {
>      VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT,
> +    VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT,
>      VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE,
>
>      VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index cafc4bf..b6912ce 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1453,6 +1453,12 @@ qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr,
>                                   "device. Device requires a standard PCI slot, "
>                                   "which is not provided by this bus"),
>                                 addr->domain, addr->bus);
> +            } else if (devFlags & QEMU_PCI_CONNECT_TYPE_PCIE) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("PCI bus %.4x:%.2x is not compatible with the "
> +                                 "device. Device requires a PCI Express slot, "
> +                                 "which is not provided by this bus"),
> +                               addr->domain, addr->bus);
>              } else {
>                  /* this should never happen. If it does, there is a
>                   * bug in the code that sets the flag bits for devices.
> @@ -1549,6 +1555,12 @@ qemuDomainPCIAddressBusSetModel(qemuDomainPCIAddressBusPtr bus,
>          bus->minSlot = 1;
>          bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST;
>          break;
> +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +        /* slots 1 - 31, PCIe devices only, no hotplug */
> +        bus->flags = QEMU_PCI_CONNECT_TYPE_PCIE;
> +        bus->minSlot = 1;
> +        bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST;
> +        break;
>      default:
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Invalid PCI controller model %d"), model);
> @@ -2347,7 +2359,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
>                  continue;
>              switch (def->controllers[i]->model) {
>              case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> -                /* pci-root is implicit in the machine,
> +            case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +                /* pci-root and pcie-root are implicit in the machine,
>                   * and needs no address */
>                  continue;
>              case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> @@ -4336,8 +4349,9 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
>                                def->idx, def->idx);
>              break;
>          case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +        case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("wrong function called for pci-root"));
> +                           _("wrong function called for pci-root/pcie-root"));
>              return NULL;
>          }
>          break;
> @@ -7615,9 +7629,10 @@ qemuBuildCommandLine(virConnectPtr conn,
>                      continue;
>                  }
>
> -                /* Skip pci-root */
> +                /* Skip pci-root/pcie-root */
>                  if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> -                    cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) {
> +                    (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
> +                     cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)) {
>                      continue;
>                  }
>
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index bf4953a..e5111d2 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -233,13 +233,15 @@ typedef enum {
>
>     QEMU_PCI_CONNECT_TYPE_PCI     = 1 << 2,
>     /* PCI devices can connect to this bus */
> +   QEMU_PCI_CONNECT_TYPE_PCIE    = 1 << 3,
> +   /* PCI Express devices can connect to this bus */
>  } qemuDomainPCIConnectFlags;
>
>  /* a combination of all bit that describe the type of connections
>   * allowed, e.g. PCI, PCIe, switch
>   */
>  # define QEMU_PCI_CONNECT_TYPES_MASK \
> -    QEMU_PCI_CONNECT_TYPE_PCI
> +   (QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE)
>
>
>  int qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index d3da666..4d04609 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -701,6 +701,7 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>  {
>      bool addDefaultUSB = true;
>      bool addPCIRoot = false;
> +    bool addPCIeRoot = false;
>
>      /* check for emulator and create a default one if needed */
>      if (!def->emulator &&
> @@ -713,12 +714,16 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>      case VIR_ARCH_X86_64:
>          if (!def->os.machine)
>              break;
> -        if (STRPREFIX(def->os.machine, "pc-q35") ||
> -            STREQ(def->os.machine, "q35") ||
> -            STREQ(def->os.machine, "isapc")) {
> +        if (STREQ(def->os.machine, "isapc")) {
>              addDefaultUSB = false;
>              break;
>          }
> +        if (STRPREFIX(def->os.machine, "pc-q35") ||
> +            STREQ(def->os.machine, "q35")) {
> +           addPCIeRoot = true;
> +           addDefaultUSB = false;
> +           break;
> +        }
>          if (!STRPREFIX(def->os.machine, "pc-0.") &&
>              !STRPREFIX(def->os.machine, "pc-1.") &&
>              !STRPREFIX(def->os.machine, "pc-i440") &&
> @@ -755,6 +760,12 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>              VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0)
>          return -1;
>
> +    if (addPCIeRoot &&
> +        virDomainDefMaybeAddController(
> +            def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0,
> +            VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0)
> +        return -1;
> +
>      return 0;
>  }
>
> @@ -1421,7 +1432,7 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
>
>          if (pci && pci->idx == 0 &&
>              pci->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) {
> -            VIR_DEBUG("Removing default 'pci-root' from domain '%s'"
> +            VIR_DEBUG("Removing default pci-root from domain '%s'"
>                        " for migration compatibility", def->name);
>              toremove++;
>          } else {
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args
> new file mode 100644
> index 0000000..e937189
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args
> @@ -0,0 +1,4 @@
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/libexec/qemu-kvm \
> +-S -M q35 -m 2048 -smp 2 -nographic -nodefaults \
> +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \
> +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.1,addr=0x1 -usb
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml
> new file mode 100644
> index 0000000..1aa5455
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.xml
> @@ -0,0 +1,21 @@
> +<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>
> +    <controller type='pci' index='0' model='pcie-root'/>
> +    <controller type='usb' index='0'/>
> +    <memballoon model='none'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index b7485fc..57c6989 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -994,6 +994,8 @@ mymain(void)
>      DO_TEST("pci-autoadd-idx", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE);
>      DO_TEST("pci-bridge-many-disks",
>              QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE_PCI_BRIDGE);
> +    DO_TEST("pcie-root",
> +            QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE);
>
>      DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_DRIVE,
>              QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE,
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 66be40e..ea511b8 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -294,6 +294,7 @@ mymain(void)
>      DO_TEST_DIFFERENT("pci-bridge-many-disks");
>      DO_TEST_DIFFERENT("pci-autoadd-addr");
>      DO_TEST_DIFFERENT("pci-autoadd-idx");
> +    DO_TEST("pcie-root");
>
>      DO_TEST("hostdev-scsi-lsi");
>      DO_TEST("hostdev-scsi-virtio-scsi");
> --
> 1.7.11.7
>
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

ACK. Seems to just straight forward add pcie-root along side pci-root.
I'd just move the fix to the since section to this patch from 3/7 that
I noted above.

-- 
Doug Goldstein


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]