[libvirt] [RFC] qemu: Use virtio-pci by default for mach-virt guests
Laine Stump
laine at laine.org
Sun Oct 23 03:07:39 UTC 2016
(When I first saw your mail I didn't realize it was a patch, because it
didn't have "PATCH" in the subject)
On 10/22/2016 05:50 PM, Martin Kletzander wrote:
> On Fri, Oct 21, 2016 at 07:24:28PM +0200, 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 kinda 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.
>> ---
>> Sending this as an RFC for the time being because it clearly
>> needs some more polish, but I wanted to get the idea out
>> there sooner rather than later.
>>
>
> Makes sense for the non-user of this (or rather not-yet-user maybe). So
> I mention only few details inline.
I like that this makes pci truly the default in a simple manner, but
still allows switching back to mmio if necessary. On the other hand, it
puts the potential "switch" to decide whether or not to use mmio for all
devices down into the config of a single device, which is a bit weird to
explain. (On the other hand, how often will mmio be used in the future?
Maybe it doesn't matter if it's weird to explain...)
>
>> It needs to be applied on top of Laine's PCI series[1].
>>
>>
>> [1]
>> https://www.redhat.com/archives/libvir-list/2016-October/msg00699.html
>>
>> src/qemu/qemu_domain_address.c | 128
>> ++++++++++++++++++++-
>> ...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, 162 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain_address.c
>> b/src/qemu/qemu_domain_address.c
>> index f27d1e3..7f07764 100644
>> --- a/src/qemu/qemu_domain_address.c
>> +++ b/src/qemu/qemu_domain_address.c
>> @@ -265,6 +265,118 @@
>> qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
>> }
>>
>>
>> +static bool
>> +qemuDomainAnyDeviceHasAddressOfType(virDomainDefPtr def,
>> + virDomainDeviceAddressType type)
>> +{
>> + size_t i;
>> +
>
> It's super-easy to miss something here, moreover it's easy to forget
> adding stuff here in the future. You should either use
> virDomainDeviceInfoIterate() or at least (not a fan of that) copy the
> check from virDomainDeviceInfoIterateInternal() here, so that people are
> forced to add new device types here.
I agree with this (and I wish that the address assignment used
virDomainDeviceInfoIterate() when assigning addresses for the same
reasons (for brevity and to be sure new device types aren't forgotten);
the problem is that the order of devices during address assignment is
different, which would result in different PCI addresses for the same
input XML if we were to changeit, so we're stuck with that particular
extra manual enumeration of all the devices. But definitely let's not
make another.)
>
>
> [...]
>
>> static void
>> qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
>> virDomainDeviceAddressType type)
>> @@ -390,6 +502,8 @@ static void
>> qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
>> virQEMUCapsPtr qemuCaps)
>> {
>> + bool usesMMIO;
>> +
>> if (def->os.arch != VIR_ARCH_ARMV7L &&
>> def->os.arch != VIR_ARCH_AARCH64)
>> return;
>> @@ -398,9 +512,17 @@
>> 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 */
>> + usesMMIO = qemuDomainAnyDeviceHasAddressOfType(def,
>> + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO);
Any reason you created the temporary variable just to use it once? Also,
by calling this unconditionally in advance you eliminate the possibility
of avoiding that big long enumeration of all the devices in the case
that qemuDomainMachineHasPCIeRoot() returns false.
>> + if (qemuDomainMachineHasPCIeRoot(def) && !usesMMIO) {
>
> I'm guessing you are checking for the pcie-root so that we're not adding
> it for users that don't want any. That way for us to actually use
> virtio-pci by default you need to add that as well, which you haven't
> mentioned in the commit message. Not sure which one of those things was
> intentional.
Actually that's not true. By the time this function is called (during
device address assignment), default devices have already been added to
the domain, which will include pci-root or pcie-root as appropriate.
>
>> + 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 \
dmi-to-pci-bridge and pci-bridge should be unnecessary. See below..
>> +-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>
?? It shouldn't have needed to add these...
>> + <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'/>
Aha! You need to add the .....DISABLE_LEGACY capability to the test
cases so that libvirt will recognize that it can put virtio devices on a
PCIE slot.
>> </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'/>
This is also caused by lack of DISABLE_LEGACY
>> </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'/>
As is this. Once you add that cap to the test cases (for both xml2xml
and xml2argv) the test cases should come out right.
>> </rng>
>> </devices>
>> </domain>
>> --
>> 2.7.4
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list