Re: [PATCH 1/2] qemu: aarch64: Don't add PCIe controller by default

[Laine Stump <laine@xxxxxxxxx>]

On 01/28/2016 04:14 PM, Cole Robinson wrote:
> This was discussed here:
>
> https://www.redhat.com/archives/libvir-list/2015-December/msg00217.html
>
> The summary is that apps/users are in a tough spot WRT aarch64 -M virt
> and PCIe support: qemu has all the plumbing, but most distros don't
> support it yet. Presently libvirt adds a PCIe controller to the XML for
> new enough qemu, but this patch drops that behavior.

I read back through all of these messages and am more thoroughly 
undecided than ever about the proper thing to do.

I guess the end of that thread was that the best thing to do was (as 
you're doing in this patch) to temporarily (?) not add a pcie-root 
controller to a domain's XML for aarch64 -M virt, and if there is no PCI 
controller, then use virtio-mmio for the address of all devices; but if 
a pcie-root is manually added, to instead use pci addresses.

My main issue with that is that the XML is then not reflecting the 
hardware exactly in cases where the binary is creating a pcie-root 
automatically (but we're not using it). The alternative would be to 
force specifying the address type of each device though, and since that 
is impractical for management apps to deal with, I guess keying off the 
presence/absence of a manually-created pcie-root is the less evil of the 
options :-P.

Assuming that we're all in agreement with this, ACK to this patch, but 
only in combination with whatever followup is necessary to support doing 
the auto-addressing based on presence of pcie-root. (I'm actually 
wondering if, once we have that 2nd part figured out, it may be 
necessary to combine the two patches into one in order to not break any 
future bisecting to try and find bugs related to aarch64 that is using 
devices with PCI addresses, but we'll get to that later...)

> Upcoming patches will instead require users to manually specify a
> pcie controller in the XML as a way of telling libvirt 'the OS supports
> PCI', at which point libvirt can use it as a target for virtio-pci.
> ---
>   src/qemu/qemu_domain.c                                         |  4 ----
>   .../qemuxml2argv-aarch64-virtio-pci-default.args               |  2 --
>   tests/qemuxml2argvtest.c                                       |  6 ++----
>   .../qemuxml2xmlout-aarch64-virtio-pci-default.xml              | 10 ----------
>   tests/qemuxml2xmltest.c                                        |  2 ++
>   5 files changed, 4 insertions(+), 20 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 1df1b74..aea1ea5 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1109,10 +1109,6 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
>       case VIR_ARCH_AARCH64:
>           addDefaultUSB = false;
>           addDefaultMemballoon = false;
> -        if (STREQ(def->os.machine, "virt") ||
> -            STRPREFIX(def->os.machine, "virt-")) {
> -            addPCIeRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX);
> -        }
>           break;
>   
>       case VIR_ARCH_PPC64:
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-default.args b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-default.args
> index a49bc82..f879560 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-default.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-default.args
> @@ -21,8 +21,6 @@ QEMU_AUDIO_DRV=none \
>   -initrd /aarch64.initrd \
>   -append 'earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait' \
>   -dtb /aarch64.dtb \
> --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1 \
> --device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \
>   -device virtio-serial-device,id=virtio-serial0 \
>   -usb \
>   -drive file=/aarch64.raw,format=raw,if=none,id=drive-virtio-disk0 \
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index cfb46ef..b2f636e 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1642,9 +1642,7 @@ mymain(void)
>               QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM);
>   
>       /* Demonstrates the virtio-pci default... namely that there isn't any!
> -       q35 style PCI controllers will be added if the binary supports it,
> -       but virtio-mmio is always used unless PCI addresses are manually
> -       specified. */
> +       However this might change in the future... */
>       DO_TEST("aarch64-virtio-pci-default",
>               QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DTB,
>               QEMU_CAPS_DEVICE_VIRTIO_MMIO,
> @@ -1653,7 +1651,7 @@ mymain(void)
>               QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE);
>       /* Example of using virtio-pci with no explicit PCI controller
>          but with manual PCI addresses */
> -    DO_TEST("aarch64-virtio-pci-manual-addresses",
> +    DO_TEST_PARSE_ERROR("aarch64-virtio-pci-manual-addresses",
>               QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DTB,
>               QEMU_CAPS_DEVICE_VIRTIO_MMIO,
>               QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM,
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-default.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-default.xml
> index 6f1c53b..db2119c 100644
> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-default.xml
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-default.xml
> @@ -31,16 +31,6 @@
>         <target dev='vda' bus='virtio'/>
>         <address type='virtio-mmio'/>
>       </disk>
> -    <controller type='pci' index='0' model='pcie-root'/>
> -    <controller type='pci' index='1' model='dmi-to-pci-bridge'>
> -      <model name='i82801b11-bridge'/>
> -      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
> -    </controller>
> -    <controller type='pci' index='2' model='pci-bridge'>
> -      <model name='pci-bridge'/>
> -      <target chassisNr='2'/>
> -      <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0'/>
> -    </controller>
>       <controller type='virtio-serial' index='0'>
>         <address type='virtio-mmio'/>
>       </controller>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 581129c..cac401c 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -731,12 +731,14 @@ mymain(void)
>               QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM,
>               QEMU_CAPS_OBJECT_GPEX, QEMU_CAPS_DEVICE_PCI_BRIDGE,
>               QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_VIRTIO_SCSI);
> +    /*
>       DO_TEST_FULL("aarch64-virtio-pci-manual-addresses", WHEN_ACTIVE,
>               QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DTB,
>               QEMU_CAPS_DEVICE_VIRTIO_MMIO,
>               QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM,
>               QEMU_CAPS_OBJECT_GPEX, QEMU_CAPS_DEVICE_PCI_BRIDGE,
>               QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_VIRTIO_SCSI);
> +    */
>       DO_TEST("aarch64-gic");
>       DO_TEST("aarch64-gicv3");
>   

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list