[libvirt] [PATCHv2 13/17] conf: new pci controller model "pcie-switch-upstream-port"

John Ferlan jferlan at redhat.com
Thu Jul 23 13:22:21 UTC 2015



On 07/17/2015 02:43 PM, Laine Stump wrote:
> This controller can be connected only to a pcie-root-port or a
> pcie-switch-downstream-port (which will be added in a later patch),
> which is the reason for the new connect type
> VIR_PCI_CONNECT_TYPE_PCIE_PORT. A pcie-switch-upstream-port provides
> 32 ports (slot=0 to slot=31) on the downstream side, which can only
> have pci controllers of model "pcie-switch-downstream-port" plugged
> into them, which is the reason for the other new connect type
> VIR_PCI_CONNECT_TYPE_PCIE_SWITCH.
> ---
> 
> V2:
> 
> * Only allow connecting a pcie-upstream-port to a pcie-root-port or
>   pcie-switch-downstream-port (V1 allowed connecting to any PCIe port,
>   which doesn't work)
> 
> * change test case to reflect additional pcie-root-port between
>   pcie-root and pcie-switch-upstream-port
> 
> 
>  docs/formatdomain.html.in                          |  5 +--
>  docs/schemas/domaincommon.rng                      |  3 ++
>  src/conf/domain_addr.c                             | 15 +++++++--
>  src/conf/domain_addr.h                             |  9 +++++-
>  src/conf/domain_conf.c                             |  3 +-
>  src/conf/domain_conf.h                             |  1 +
>  src/qemu/qemu_command.c                            |  1 +
>  .../qemuxml2argv-pcie-switch-upstream-port.xml     | 37 ++++++++++++++++++++++
>  tests/qemuxml2xmltest.c                            |  1 +
>  9 files changed, 69 insertions(+), 6 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index dcbca75..eb5b9b7 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3025,10 +3025,11 @@
>        PCI controllers have an optional <code>model</code> attribute with
>        possible values <code>pci-root</code>, <code>pcie-root</code>,
>        <code>pcie-root-port</code>, <code>pci-bridge</code>,
> -      or <code>dmi-to-pci-bridge</code>.
> +      <code>dmi-to-pci-bridge</code>, or <code>pcie-switch-upstream-port</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>, pcie-root-port <span class="since">since 1.3.0</span>)
> +      1.1.2</span>, pcie-root-port and
> +      pcie-switch-upstream-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

Similar to 10/17 - there's no 'extra' explanation... Perhaps something
after the <p> that starts "Domains with an implicit pcie-root can
also..."  The new section should at least describe "how" one does the
connections and of course because the pci-root-port can accept the
pcie-switch-upstream-port either at boot or via hotplug.  Someone
"planning" to add the upstream-port device would need to have "enough"
pci-root-port's available at boot time (the 1x1 relationship)

Also while I'm thinking about it (and since it dawned on me only
today)... Assuming the pcie-root-port connects to some pcie-root and
only one pcie-root can exist, is or should there be a "expect failure"
test for trying to add 33?

I guess this started dawning on my while looking at the XML attached to
this patch which as two pcie-root-port's and two
pci-switch-upstream-port's. The wheels started turning wondering how the
output XML of the input XML should appear with the addresses assigned on
the controllers. When I peek to the next patch, it seems the two
pci-root-port's connect to the pcie-root as expected and then the two
pci-switch-upstream-port's have their 1x1 connection to the pci-root-port.

It seems there needs to be a :

tests/qemuxml2xmloutdata/qemuxml2xmlout-pcie-root-port.xml

Which probably should be added in patch10.

Then this patch would follow with the output for
qemuxml2argv-pcie-switch-upstream-port.xml


Another thought - a test/different that provides two upstream-port's,
but only one pcie-root-port in order to show the failure to find
something to connect to for the second one. Making a simple adjustment
to the test here to remove a pci-root-port, results in the expected error:

"libvirt: Domain Config error : internal error: Cannot automatically add
a new PCI bus for a device requiring a slot other than standard PCI."


> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 0efa0f0..8b2f29c 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1741,6 +1741,8 @@
>                      <value>i82801b11-bridge</value>
>                      <!-- implementations of 'pcie-root-port' -->
>                      <value>ioh3420</value>
> +                    <!-- implementations of 'pcie-switch-upstream-port' -->
> +                    <value>x3130-upstream</value>
>                    </choice>
>                  </attribute>
>                <empty/>
> @@ -1787,6 +1789,7 @@
>                      <value>pci-bridge</value>
>                      <value>dmi-to-pci-bridge</value>
>                      <value>pcie-root-port</value>
> +                    <value>pcie-switch-upstream-port</value>
>                    </choice>
>                  </attribute>
>                </group>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index fba0eff..e40a66c 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -197,11 +197,22 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus,
>          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;
> +        /* provides one slot which is pcie, can be used by devices
> +         * that must connect to some type of "pcie-*-port", and
> +         * is hotpluggable
> +         */
> +        bus->flags = VIR_PCI_CONNECT_TYPE_PCIE
> +           | VIR_PCI_CONNECT_TYPE_PCIE_PORT
> +           | VIR_PCI_CONNECT_HOTPLUGGABLE;
>          bus->minSlot = 0;
>          bus->maxSlot = 0;
>          break;
> +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> +        /* 31 slots, can only accept pcie-switch-port, no hotplug */
> +        bus->flags = VIR_PCI_CONNECT_TYPE_PCIE_SWITCH;
> +        bus->minSlot = 0;
> +        bus->maxSlot = 31;
> +        break;

So "conceptually" an upstream can be hotplugged into a pci-root-port,
but nothing can hotplug into it?  Thus doesn't seem hotplug is very
useful. Perhaps I'm missing something.  I haven't read the last 3
patches for the downstream-port yet.

It's not a "problem" per se, but whatever can and cannot be done needs
to be documented as well as of course the seemingly "suggested" way of
defining at boot time.

I think with some additional tests and adjustment to documentation, this
can be ACK'd

John
>      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 2a0ff96..2220a79 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -41,6 +41,12 @@ typedef enum {
>     /* 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) */
> +   VIR_PCI_CONNECT_TYPE_PCIE_PORT = 1 << 5,
> +   /* devices that can only connect to a pcie-root-port
> +    * or pcie-downstream-switch-port
> +    */
> +   VIR_PCI_CONNECT_TYPE_PCIE_SWITCH = 1 << 6,
> +   /* devices that can only connect to a pcie-switch */
>  } virDomainPCIConnectFlags;
>  
>  typedef struct {
> @@ -73,7 +79,8 @@ typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr;
>   */
>  # define VIR_PCI_CONNECT_TYPES_MASK \
>     (VIR_PCI_CONNECT_TYPE_PCI | VIR_PCI_CONNECT_TYPE_PCIE | \
> -    VIR_PCI_CONNECT_TYPE_PCIE_ROOT)
> +    VIR_PCI_CONNECT_TYPE_PCIE_ROOT | VIR_PCI_CONNECT_TYPE_PCIE_PORT | \
> +    VIR_PCI_CONNECT_TYPE_PCIE_SWITCH)
>  
>  /* 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 e02c861..4eeaa84 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -325,7 +325,8 @@ VIR_ENUM_IMPL(virDomainControllerModelPCI, VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST,
>                "pcie-root",
>                "pci-bridge",
>                "dmi-to-pci-bridge",
> -              "pcie-root-port")
> +              "pcie-root-port",
> +              "pcie-switch-upstream-port")
>  
>  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 a4df2a6..b49c803 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -753,6 +753,7 @@ typedef enum {
>      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_PCIE_SWITCH_UPSTREAM_PORT,
>  
>      VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST
>  } virDomainControllerModelPCI;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1b86f1d..725360c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2281,6 +2281,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>                      if (options->port == -1)
>                         options->port = (addr->slot << 3) + addr->function;
>                      break;
> +                case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_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-switch-upstream-port.xml b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml
> new file mode 100644
> index 0000000..13125a9
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.xml
> @@ -0,0 +1,37 @@
> +<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'/>
> +    <controller type='pci' index='5' model='pcie-switch-upstream-port'/>
> +    <controller type='pci' index='6' model='pcie-switch-upstream-port'>
> +      <model type='x3130-upstream'/>
> +    </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 6b48bf4..80686ae 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -568,6 +568,7 @@ mymain(void)
>      DO_TEST_DIFFERENT("pcie-root");
>      DO_TEST_DIFFERENT("q35");
>      DO_TEST("pcie-root-port");
> +    DO_TEST("pcie-switch-upstream-port");
>  
>      DO_TEST("hostdev-scsi-lsi");
>      DO_TEST("hostdev-scsi-virtio-scsi");
> 




More information about the libvir-list mailing list