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

Re: [libvirt] [PATCHv2 3/7] qemu: add dmi-to-pci-bridge controller



On Sat, Aug 3, 2013 at 9:01 PM, Laine Stump <laine laine org> wrote:
> This PCI controller, named "dmi-to-pci-bridge" in the libvirt config,
> and implemented with qemu's "i82801b11-bridge" device, connects to a
> PCI Express slot (e.g. one of the slots provided by the pcie-root
> controller, aka "pcie.0" on the qemu commandline), and provides 31
> *non-hot-pluggable* PCI (*not* PCIe) slots, numbered 1-31.
>
> Any time a machine is defined which has a pcie-root controller
> (i.e. any q35-based machinetype), libvirt will automatically add a
> dmi-to-pci-bridge controller if one doesn't exist, and also add a
> pci-bridge controller. The reasoning here is that any useful domain
> will have either an immediate (startup time) or eventual (subsequent
> hot-plug) need for a standard PCI slot; since the pcie-root controller
> only provides PCIe slots, we need to connect a dmi-to-pci-bridge
> controller to it in order to get a non-hot-plug PCI slot that we can
> then use to connect a pci-bridge - the slots provided by the
> pci-bridge will be both standard PCI and hot-pluggable.
>
> Since pci-bridge devices themselves are not hot-pluggable, any new
> pci-bridge controller that is added can (and will) be plugged into the
> dmi-to-pci-bridge as long as it has empty slots available.

Worth noting DO_TEST_DIFFERENT to pcie-root change here since you
mention changes like that in later commits.

> ---
>  docs/formatdomain.html.in                          | 28 +++++++++++++++---
>  docs/schemas/domaincommon.rng                      |  1 +
>  src/conf/domain_conf.c                             |  3 +-
>  src/conf/domain_conf.h                             |  1 +
>  src/qemu/qemu_capabilities.c                       |  2 ++
>  src/qemu/qemu_capabilities.h                       |  1 +
>  src/qemu/qemu_command.c                            | 33 ++++++++++++++++++++++
>  src/qemu/qemu_domain.c                             | 14 +++++++--
>  tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args |  3 +-
>  tests/qemuxml2argvdata/qemuxml2argv-q35.args       |  7 +++++
>  tests/qemuxml2argvdata/qemuxml2argv-q35.xml        | 25 ++++++++++++++++
>  tests/qemuxml2argvtest.c                           |  8 +++++-
>  .../qemuxml2xmlout-pcie-root.xml                   | 23 +++++++++++++++
>  tests/qemuxml2xmltest.c                            |  3 +-
>  14 files changed, 142 insertions(+), 10 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 330dca2..8fa4c0e 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2338,16 +2338,19 @@
>
>      <p>
>        PCI controllers have an optional <code>model</code> attribute with
> -      possible values <code>pci-root</code>, <code>pcie-root</code>
> -      or <code>pci-bridge</code>.
> +      possible values <code>pci-root</code>, <code>pcie-root</code>,
> +      <code>pci-bridge</code>, or <code>dmi-to-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 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
>        configuration.
> -      (<span class="since">since 1.0.5</span>)
> +      (pci-root and pci-bridge <span class="since">since 1.0.5</span>).

Probably belonged as part of the last patch technically, but they'll
be part of the series so it should be ok.

>      </p>
>  <pre>
>    ...
> @@ -2365,12 +2368,29 @@
>        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>).
> +      devices. In order to connect standard PCI devices on a system
> +      which has a pcie-root controller, a pci controller
> +      with <code>model='dmi-to-pci-bridge'</code> is automatically
> +      added. A dmi-to-pci-bridge controller plugs into a PCIe slot (as
> +      provided by pcie-root), and itself provides 31 standard PCI
> +      slots (which are not hot-pluggable). In order to have
> +      hot-pluggable PCI slots in the guest system, a pci-bridge
> +      controller will also be automatically created and connected to
> +      one of the slots of the auto-created dmi-to-pci-bridge
> +      controller; all guest devices with PCI addresses that are
> +      auto-determined by libvirt will be placed on this pci-bridge
> +      device.  (<span class="since">since 1.1.2</span>).
>      </p>
>  <pre>
>    ...
>    &lt;devices&gt;
>      &lt;controller type='pci' index='0' model='pcie-root'/&gt;
> +    &lt;controller type='pci' index='1' model='dmi-to-pci-bridge'&gt;
> +      &lt;address type='pci' domain='0' bus='0' slot='0xe' function='0'/&gt;
> +    &lt;/controller&gt;
> +    &lt;controller type='pci' index='2' model='pci-bridge'&gt;
> +      &lt;address type='pci' domain='0' bus='1' slot='1' function='0'/&gt;
> +    &lt;/controller&gt;
>    &lt;/devices&gt;
>    ...</pre>
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index e04be12..173359c 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1540,6 +1540,7 @@
>                  <value>pci-root</value>
>                  <value>pcie-root</value>
>                  <value>pci-bridge</value>
> +                <value>dmi-to-pci-bridge</value>
>                </choice>
>              </attribute>
>            </group>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 59a96f2..d17008f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -311,7 +311,8 @@ 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")
> +              "pci-bridge",
> +              "dmi-to-pci-bridge")
>
>  VIR_ENUM_IMPL(virDomainControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST,
>                "auto",
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 63a1444..3e118d6 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -770,6 +770,7 @@ typedef enum {
>      VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT,
>      VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT,
>      VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE,
> +    VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE,
>
>      VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST
>  } virDomainControllerModelPCI;
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 08406b8..47cc07a 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -234,6 +234,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>
>                "vnc-share-policy", /* 150 */
>                "device-del-event",
> +              "dmi-to-pci-bridge",
>      );
>
>  struct _virQEMUCaps {
> @@ -1381,6 +1382,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
>      { "pci-bridge", QEMU_CAPS_DEVICE_PCI_BRIDGE },
>      { "vfio-pci", QEMU_CAPS_DEVICE_VFIO_PCI },
>      { "scsi-generic", QEMU_CAPS_DEVICE_SCSI_GENERIC },
> +    { "i82801b11-bridge", QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE },
>  };
>
>  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index f5f685d..074e55d 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -190,6 +190,7 @@ enum virQEMUCapsFlags {
>      QEMU_CAPS_MLOCK              = 149, /* -realtime mlock=on|off */
>      QEMU_CAPS_VNC_SHARE_POLICY   = 150, /* set display sharing policy */
>      QEMU_CAPS_DEVICE_DEL_EVENT   = 151, /* DEVICE_DELETED event */
> +    QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE  = 152, /* -device i82801b11-bridge */
>
>      QEMU_CAPS_LAST,                   /* this must always be the last item */
>  };
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b6912ce..13a68a5 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1561,6 +1561,13 @@ qemuDomainPCIAddressBusSetModel(qemuDomainPCIAddressBusPtr bus,
>          bus->minSlot = 1;
>          bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST;
>          break;
> +    case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +        /* slots 1 - 31, standard PCI slots,
> +         * but *not* hot-pluggable */
> +        bus->flags = QEMU_PCI_CONNECT_TYPE_PCI;
> +        bus->minSlot = 1;
> +        bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST;
> +        break;
>      default:
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Invalid PCI controller model %d"), model);
> @@ -1669,6 +1676,12 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
>               */
>              flags = QEMU_PCI_CONNECT_TYPE_PCI;
>              break;
> +        case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +            /* pci-bridge needs a PCIe slot, but it isn't
> +             * hot-pluggable, so it doesn't need a hot-pluggable slot.
> +             */
> +            flags = QEMU_PCI_CONNECT_TYPE_PCIE;
> +            break;
>          default:
>              break;
>          }
> @@ -2369,6 +2382,12 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,
>                   */
>                  flags = QEMU_PCI_CONNECT_TYPE_PCI;
>                  break;
> +            case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +                /* dmi-to-pci-bridge requires a non-hotplug PCIe
> +                 * slot
> +                 */
> +                flags = QEMU_PCI_CONNECT_TYPE_PCIE;
> +                break;
>              default:
>                  flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI;
>                  break;
> @@ -4348,6 +4367,20 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef,
>              virBufferAsprintf(&buf, "pci-bridge,chassis_nr=%d,id=pci.%d",
>                                def->idx, def->idx);
>              break;
> +        case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("The dmi-to-pci-bridge (i82801b11-bridge) "
> +                                 "controller is not supported in this QEMU binary"));

Do we ever print out the path to the QEMU binary used in these kinds
of errors? Might be nice.

> +                goto error;
> +            }
> +            if (def->idx == 0) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("dmi-to-pci-bridge index should be > 0"));
> +                goto error;
> +            }
> +            virBufferAsprintf(&buf, "i82801b11-bridge,id=pci.%d", def->idx);
> +            break;
>          case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
>          case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 4d04609..f5030cd 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -760,10 +760,20 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>              VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0)
>          return -1;
>
> +    /* When a machine has a pcie-root, make sure that there is always
> +     * a dmi-to-pci-bridge controller added as bus 1, and a pci-bridge
> +     * as bus 2, so that standard PCI devices can be connected
> +     */
>      if (addPCIeRoot &&
> -        virDomainDefMaybeAddController(
> +        (virDomainDefMaybeAddController(
>              def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0,
> -            VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0)
> +            VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) < 0 ||
> +         virDomainDefMaybeAddController(
> +             def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 1,
> +             VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE) < 0 ||
> +         virDomainDefMaybeAddController(
> +             def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 2,
> +             VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE) < 0))
>          return -1;

Not going to lie, that's an if check of hell. Had to really stare for
a second to make sure everything was lined up correctly.

>
>      return 0;
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args
> index e937189..23db85c 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args
> @@ -1,4 +1,5 @@
>  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
> +-device i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \
> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 -usb
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args
> new file mode 100644
> index 0000000..ddff6f0
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args
> @@ -0,0 +1,7 @@
> +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 i82801b11-bridge,id=pci.1,bus=pci.0,addr=0x1 \
> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \
> +-usb \
> +-vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml
> new file mode 100644
> index 0000000..3541b14
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml
> @@ -0,0 +1,25 @@
> +<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='pci' index='1' model='dmi-to-pci-bridge'/>
> +    <controller type='pci' index='2' model='pci-bridge'/>
> +    <video>
> +      <model type='qxl' ram='65536' vram='18432' heads='1'/>
> +    </video>
> +    <memballoon model='none'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 57c6989..aba0f88 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -995,7 +995,13 @@ mymain(void)
>      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);
> +            QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE,
> +            QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE);
> +    DO_TEST("q35",
> +            QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE,
> +            QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> +            QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> +            QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
>
>      DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_DRIVE,
>              QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE,
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml
> new file mode 100644
> index 0000000..25c77f1
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root.xml
> @@ -0,0 +1,23 @@
> +<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'/>
> +    <controller type='pci' index='1' model='dmi-to-pci-bridge'/>
> +    <controller type='pci' index='2' model='pci-bridge'/>
> +    <memballoon model='none'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index ea511b8..8b4590a 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -294,7 +294,8 @@ 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_DIFFERENT("pcie-root");
> +    DO_TEST("q35");
>
>      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

Overall an ACK, just fix up the commit message and move the since
documentation fixup to 2/7, which I think you can do without
reposting.

-- 
Doug Goldstein


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