On 06/23/2017 11:03 AM, Andrea Bolognani wrote:
> PCI bus has to be numbered sequentially, and no index can be
> missing,
I thought we had figured out later that this was a
misunderstanding/misinterpretation/creation based on nothing, and that
it really didn't matter.
Keep in mind that the index of a PCI controller is used only for two
things: 1) a device that is to be connected to PCI controller of
index='x' should have "bus='x'" in its PCI address. 2) The value of
index is used when creating the alias ("id" in qemu terms) of the
controller, e.g. "pci.2". It is this string "pci.2" which is used on the
qemu commandline to connect the controller and the devices plugged into
that controller; as far as the guest firmware/OS are concerned, there
are just a bunch of PCI controllers, and they are given bus numbers
during probing according to the order they are encountered, *NOT*
according to the id/alias of the controller. The result is that it's
completely meaningless to create a bunch of empty PCI bridges/root-ports
just so that you'll have controllers named "pcie.0, pci.1, pci.2, pci.3"
instead of only "pcie.0, pci.3" (with a gap in the ordering).
> so libvirt will fill in the blanks automatically for
> the user.
Hmm. I guess the *one* place that it matters is in our PCI address
allocation functions, which assume that all the buses from 0 to the
final bus can accept *something*.
> Up until now, it has done so using either pci-bridge, for machine
> types based on legacy PCI, or pcie-root-port, for machine types
> based on PCI Express. Neither choice is good for pSeries guests,
> where PHBs (pci-root) should be used instead.
>
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> src/qemu/qemu_domain_address.c | 16 +++++++++++++---
> .../qemuxml2argv-pseries-many-buses-2.args | 2 +-
> tests/qemuxml2argvtest.c | 1 -
> .../qemuxml2xmlout-pseries-many-buses-2.xml | 7 +++----
> tests/qemuxml2xmltest.c | 1 -
> 5 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 960ea04..082cb72 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -1097,10 +1097,14 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
> * that don't yet have a corresponding controller in the domain
> * config.
> */
> - if (hasPCIeRoot)
> + if (qemuDomainIsPSeries(def)) {
> + /* pSeries guests should use PHBs (pci-root controllers) */
> + defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT;
> + } else if (hasPCIeRoot) {
> defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
> - else
> + } else {
> defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE;
> + }
>
> for (i = 1; i < addrs->nbuses; i++) {
>
> @@ -2182,7 +2186,13 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> dev.data.controller = def->controllers[contIndex];
> /* set connect flags so it will be properly addressed */
> qemuDomainFillDevicePCIConnectFlags(def, &dev, qemuCaps, driver);
> - if (qemuDomainPCIAddressReserveNextAddr(addrs,
> +
> + /* Reserve an address for the controller. pci-root and pcie-root
> + * controllers don't plug into any other PCI controller, hence
> + * they should skip this step */
> + if (bus->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
> + bus->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT &&
> + qemuDomainPCIAddressReserveNextAddr(addrs,
> &dev.data.controller->info) < 0) {
> goto cleanup;
> }
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.args
> index 1cb5831..13fed02 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-many-buses-2.args
> @@ -19,4 +19,4 @@ server,nowait \
> -mon chardev=charmonitor,id=monitor,mode=readline \
> -boot c \
> -device spapr-pci-host-bridge,index=1,id=pci.2 \
> --device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1
> +-device spapr-pci-host-bridge,index=2,id=pci.1
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 137f59a..f1720cb 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1730,7 +1730,6 @@ mymain(void)
> DO_TEST("pseries-many-buses-2",
> QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG,
> QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE,
> - QEMU_CAPS_DEVICE_PCI_BRIDGE,
> QEMU_CAPS_VIRTIO_SCSI);
> DO_TEST("pseries-hostdevs-1",
> QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG,
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-2.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-2.xml
> index 75dfabf..14f3e36 100644
> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-2.xml
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-many-buses-2.xml
> @@ -23,10 +23,9 @@
> <target index='1'/>
> </controller>
> <controller type='usb' index='0' model='none'/>
> - <controller type='pci' index='1' model='pci-bridge'>
> - <model name='pci-bridge'/>
> - <target chassisNr='1'/>
> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
> + <controller type='pci' index='1' model='pci-root'>
> + <model name='spapr-pci-host-bridge'/>
> + <target index='2'/>
Interesting - since we initially had one with controller index=2 that
was automatically given target index=1, now that this PHB controller is
being automatically added at index=1, it uses the next free target
index, which is '2'. I assume that works correctly?
Reviewed-by: Laine Stump <laine@xxxxxxxxx>
Although I think (in a separate patch) someone should eliminate all this
cruft about filling in empty places in the bus numbering with unused
controllers - it's pointless, and in the case of pci-bridge at least, I
think it creates controllers that are unusable (because they have no io
space allocated).
> </controller>
> <memballoon model='none'/>
> <panic model='pseries'/>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 312e2b1..378cd85 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -696,7 +696,6 @@ mymain(void)
> DO_TEST("pseries-many-buses-2",
> QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG,
> QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE,
> - QEMU_CAPS_DEVICE_PCI_BRIDGE,
> QEMU_CAPS_VIRTIO_SCSI);
> DO_TEST("pseries-hostdevs-1",
> QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
