[libvirt] [RFC PATCH v2] qemu: Use virtio-pci by default for mach-virt guests
Laine Stump
laine at laine.org
Tue Oct 25 02:07:48 UTC 2016
On 10/24/2016 11:28 AM, Andrea Bolognani wrote:
> virtio-pci is the way forward for aarch64 guests: it's faster
> and less alien to people coming from other architectures.
> Now that guest support is finally getting there, we'd like to
> start using it by default instead of virtio-mmio.
>
> Users and applications can already opt-in by explicitly using
>
> <address type='pci'/>
>
> inside the relevant elements, but that's kind of cumbersome and
> requires all users and management applications to adapt, which
> we'd really like to avoid.
>
> What we can do instead is use virtio-mmio only if the guest
> already has at least one virtio-mmio device, and use virtio-pci
> in all other situations.
>
> That means existing virtio-mmio guests will keep using the old
> addressing scheme, and new guests will automatically be created
> using virtio-pci instead. Users can still override the default
> in either direction.
> ---
> RFC: still needs polish (documentation, test cases) before it
> can be considered for merging, plus it builds on top of changes
> that haven't made it into master yet.
>
> Changes from v1:
>
> * use virDomainDeviceInfoIterate(), as suggested by Martin
> and Laine, which results in cleaner and more robust code
>
> src/qemu/qemu_domain_address.c | 39 ++++++++++++++++++++--
> ...l2argv-aarch64-virt-2.6-virtio-pci-default.args | 14 +++++---
> .../qemuxml2argv-aarch64-virtio-pci-default.args | 14 +++++---
> .../qemuxml2xmlout-aarch64-virtio-pci-default.xml | 24 ++++++++++---
> 4 files changed, 73 insertions(+), 18 deletions(-)
>
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index f27d1e3..30b1ddf 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -386,6 +386,32 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
> }
>
>
> +static int
> +qemuDomainCountVirtioMMIODevicesCallback(virDomainDefPtr def ATTRIBUTE_UNUSED,
> + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
> + virDomainDeviceInfoPtr info,
> + void *opaque)
> +{
> + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO)
> + (*((size_t *) opaque))++;
(For some reason I had believed that info could be NULL, and had written
one of my own callback functions accordingly, but I doublechecked the
function that calls this callback and verified that info is always valid.)
A small optimization (that you may not want to make, since you're only
doing this once) would be to return -1 when type == MMIO - this causes
the iterate to end early. The function would then be less generally
useful, though (and you would probably want to rename it accordingly,
since it would only ever return 0 or 1).
Another optimization would be to force it to return count == 0 as soon
as it encountered any pci address (since the two are mutually exclusive
- unless we hav allowed manually mixing them in the past, or maybe it
has happened "accidentally" due to libvirt auto-adding a
dmi-to-pci-bridge in some older version of libvirt.). If you made such
an optimization, you would again probably want the function named
differently though - maybe if could be called
qemuDomainDetermineDefaultAddressType(), and would return a
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_*
All that is purely speculative though - what you have here works just
fine and takes an insignificant time to execute relative to everything
else that has to happen :-)
> + return 0;
> +}
> +
> +
> +static size_t
> +qemuDomainCountVirtioMMIODevices(virDomainDefPtr def)
> +{
> + size_t count = 0;
> +
> + virDomainDeviceInfoIterate(def,
> + qemuDomainCountVirtioMMIODevicesCallback,
> + &count);
> +
> + return count;
> +}
> +
> +
> static void
> qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
> virQEMUCapsPtr qemuCaps)
> @@ -398,9 +424,16 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
> qemuDomainMachineIsVirt(def)))
> return;
>
> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) {
> - qemuDomainPrimeVirtioDeviceAddresses(
> - def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO);
> + /* We use virtio-mmio by default on mach-virt guests only if they already
> + * have at least one virtio-mmio device: in all other cases, we prefer
> + * virtio-pci */
> + if (qemuDomainMachineHasPCIeRoot(def) &&
> + qemuDomainCountVirtioMMIODevices(def) == 0) {
> + qemuDomainPrimeVirtioDeviceAddresses(def,
> + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
> + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) {
> + qemuDomainPrimeVirtioDeviceAddresses(def,
> + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO);
> }
> }
>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virt-2.6-virtio-pci-default.args b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virt-2.6-virtio-pci-default.args
> index 75db1a4..df03c6e 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virt-2.6-virtio-pci-default.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virt-2.6-virtio-pci-default.args
> @@ -21,14 +21,18 @@ QEMU_AUDIO_DRV=none \
> -initrd /aarch64.initrd \
> -append 'earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait' \
> -dtb /aarch64.dtb \
> --device virtio-serial-device,id=virtio-serial0 \
> +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1 \
> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
> +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
> +-device virtio-serial-pci,id=virtio-serial0,bus=pci.2,addr=0x2 \
> -drive file=/aarch64.raw,format=raw,if=none,id=drive-virtio-disk0 \
> --device virtio-blk-device,drive=drive-virtio-disk0,id=virtio-disk0 \
> --device virtio-net-device,vlan=0,id=net0,mac=52:54:00:09:a4:37 \
> +-device virtio-blk-pci,bus=pci.2,addr=0x3,drive=drive-virtio-disk0,\
> +id=virtio-disk0 \
> +-device virtio-net-pci,vlan=0,id=net0,mac=52:54:00:09:a4:37,bus=pci.2,addr=0x1 \
> -net user,vlan=0,name=hostnet0 \
> -serial pty \
> -chardev pty,id=charconsole1 \
> -device virtconsole,chardev=charconsole1,id=console1 \
> --device virtio-balloon-device,id=balloon0 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x4 \
> -object rng-random,id=objrng0,filename=/dev/random \
> --device virtio-rng-device,rng=objrng0,id=rng0
> +-device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.2,addr=0x5
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-default.args b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-default.args
> index b5b010c..f205d9a 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-default.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virtio-pci-default.args
> @@ -21,14 +21,18 @@ QEMU_AUDIO_DRV=none \
> -initrd /aarch64.initrd \
> -append 'earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait' \
> -dtb /aarch64.dtb \
> --device virtio-serial-device,id=virtio-serial0 \
> +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1 \
> +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
> +-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
> +-device virtio-serial-pci,id=virtio-serial0,bus=pci.2,addr=0x2 \
> -drive file=/aarch64.raw,format=raw,if=none,id=drive-virtio-disk0 \
> --device virtio-blk-device,drive=drive-virtio-disk0,id=virtio-disk0 \
> --device virtio-net-device,vlan=0,id=net0,mac=52:54:00:09:a4:37 \
> +-device virtio-blk-pci,bus=pci.2,addr=0x3,drive=drive-virtio-disk0,\
> +id=virtio-disk0 \
> +-device virtio-net-pci,vlan=0,id=net0,mac=52:54:00:09:a4:37,bus=pci.2,addr=0x1 \
> -net user,vlan=0,name=hostnet0 \
> -serial pty \
> -chardev pty,id=charconsole1 \
> -device virtconsole,chardev=charconsole1,id=console1 \
> --device virtio-balloon-device,id=balloon0 \
> +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x4 \
> -object rng-random,id=objrng0,filename=/dev/random \
> --device virtio-rng-device,rng=objrng0,id=rng0
> +-device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.2,addr=0x5
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-default.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-default.xml
> index 7c3fc19..90659a1 100644
> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-default.xml
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-virtio-pci-default.xml
> @@ -30,16 +30,30 @@
> <disk type='file' device='disk'>
> <source file='/aarch64.raw'/>
> <target dev='vda' bus='virtio'/>
> - <address type='virtio-mmio'/>
> + <address type='pci' domain='0x0000' bus='0x02' slot='0x03' function='0x0'/>
> </disk>
> <controller type='pci' index='0' model='pcie-root'/>
> <controller type='virtio-serial' index='0'>
> - <address type='virtio-mmio'/>
> + <address type='pci' domain='0x0000' bus='0x02' slot='0x02' function='0x0'/>
> + </controller>
> + <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='0x00' function='0x0'/>
> + </controller>
> + <controller type='pci' index='3' model='pcie-root-port'>
> + <model name='ioh3420'/>
> + <target chassis='3' port='0x10'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
> </controller>
> <interface type='user'>
> <mac address='52:54:00:09:a4:37'/>
> <model type='virtio'/>
> - <address type='virtio-mmio'/>
> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
> </interface>
> <serial type='pty'>
> <target port='0'/>
> @@ -51,11 +65,11 @@
> <target type='virtio' port='1'/>
> </console>
> <memballoon model='virtio'>
> - <address type='virtio-mmio'/>
> + <address type='pci' domain='0x0000' bus='0x02' slot='0x04' function='0x0'/>
> </memballoon>
> <rng model='virtio'>
> <backend model='random'>/dev/random</backend>
> - <address type='virtio-mmio'/>
> + <address type='pci' domain='0x0000' bus='0x02' slot='0x05' function='0x0'/>
> </rng>
> </devices>
> </domain>
I guess I can agree with keeping this test case as is. It would be nice
to have another test that used the identical input file (via symlink)
but had the extra flags defined (but it's not needed in this patch). Too
bad I didn't think of that as a test case for the patches that changed
the behavior for virtio devices (and also that I don't feel like adding
it now :-P)
More information about the libvir-list
mailing list