[libvirt] [RFC] qemu: Use virtio-pci by default for mach-virt guests
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. 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; + + for (i = 0; i < def->ndisks; i++) { + if (def->disks[i]->info.type == type) + return true; + } + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nfss; i++) { + if (def->fss[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nnets; i++) { + if (def->nets[i]->info.type == type) + return true; + } + + for (i = 0; i < def->ninputs; i++) { + if (def->inputs[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nsounds; i++) { + if (def->sounds[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nvideos; i++) { + if (def->videos[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nhostdevs; i++) { + if (def->hostdevs[i]->info->type == type) + return true; + } + + for (i = 0; i < def->nredirdevs; i++) { + if (def->redirdevs[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nsmartcards; i++) { + if (def->smartcards[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nserials; i++) { + if (def->serials[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nparallels; i++) { + if (def->parallels[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nchannels; i++) { + if (def->channels[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nconsoles; i++) { + if (def->consoles[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nhubs; i++) { + if (def->hubs[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nrngs; i++) { + if (def->rngs[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nshmems; i++) { + if (def->shmems[i]->info.type == type) + return true; + } + + for (i = 0; i < def->nmems; i++) { + if (def->mems[i]->info.type == type) + return true; + } + + if (def->memballoon && def->memballoon->info.type == type) + return true; + + if (def->watchdog && def->watchdog->info.type == type) + return true; + + if (def->nvram && def->nvram->info.type == type) + return true; + + if (def->tpm && def->tpm->info.type == type) + return true; + + return false; +} + + 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); + if (qemuDomainMachineHasPCIeRoot(def) && !usesMMIO) { + 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> -- 2.7.4
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.
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. [...]
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); + 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.
+ 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> -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
(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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, Oct 22, 2016 at 11:07:39PM -0400, Laine Stump wrote:
(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.)
I'm not sure, but I would bet quite some sum of money thatwe don't have to guarantee how we allocate addresses for XMLs that don't have any address assigned. That's because it wouldn't make any sense to guarantee that and no piece of code should depend on that. Having said that, I should probably clean-up the patches for the address assignment and send them to the list as it looks like there will be no future work from the GSoC student we had for that. But I can change some of the stuff to use virDomainDeviceInfoIterate() to assign the addresses if possible and we'll see... But that's a different topic =) Have a nice day (night), Martin
On 10/24/2016 02:43 AM, Martin Kletzander wrote:
On Sat, Oct 22, 2016 at 11:07:39PM -0400, Laine Stump wrote:
(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.)
I'm not sure, but I would bet quite some sum of money thatwe don't have to guarantee how we allocate addresses for XMLs that don't have any address assigned. That's because it wouldn't make any sense to guarantee that and no piece of code should depend on that.
Yes and no. Certainly it would be bad to knowingly make code dependent on it, but back when explicit PCI address support was first added, it was implemented (I *think* - I wasn't around then, or at least wasn't paying attention to PCI addresses) such that the explicit addresses assigned by libvirt would exactly match the addresses that would have been assigned by qemu if we didn't give any address info on the commandline. This was done to make sure that guests be started on newer libvirt/qemu would experience no changes in guest ABI. In other words, the order of devices in address assignment was very important. At this point it's almost a certainty that all persistent guests that were originally defined on libvirt old enough to not have explicit PCI addresses have had their config modified and resaved at least once, so changing the order of devices in PCI address allocation *probably* wouldn't have an affect on them. However, there is still the case of transient guests, where libvirt is handed the "original" config each time the guest is started. If there is any management application naive enough to do that without "priming" the PCI addresses, then we need to preserve the order of assigning addresses. (Personally I'd be completely happy to start using virDomainDeviceInfoIterate() to assign addresses (Damn the torpedoes! Full steam ahead!!), and I've even made the function that calculates pciConnectFlags so that it will put a 0 in the flags for those devices that don't need a PCI address, which should make it quite easy to implement. I wouldn't want the finger pointed at me as the cause of some horrid regression in ovirt or whatever random management application though :-)
Having said that, I should probably clean-up the patches for the address assignment and send them to the list as it looks like there will be no future work from the GSoC student we had for that.
Anything PCI-related may run into lots of merge conflicts. Especially after my next series...
On Sat, 2016-10-22 at 23:07 -0400, Laine Stump wrote:
(When I first saw your mail I didn't realize it was a patch, because it didn't have "PATCH" in the subject)
Fair enough, will send the next iteration as [RFC PATCH v2] :)
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...)
We really want to push for virtio-pci going forward as it has a number of advantages, but there are still legacy guest OSs out there that don't support virtio-pci at all yet we still want to be able to run. virt-install is already capable of overriding the default address type on a per-device basis, eg. --network network=default,model=virtio,address.type=pci or --disk size=10,bus=virtio,address.type=virtio-mmio I think we should have some sort of global switch that sets address.type for all devices, so you don't have to repeat yourself. Or does something like that exist already? I'm also not sure whether virt-manager is using libosinfo, but ideally the default address type would be something that can be chosen automatically based on the guest OS. CC'ing Cole and Pavel so they can provide some insights. I feel like I've strayed quite a bit from the original question, so let me get back to it: going forward, the only situation where we'd find ourselves choosing virtio-mmio addressess instead of virtio-pci addresses is when dealing with legacy guests. We definitely don't want to mix virtio-pci and virtio-mmio in the same guest. So this approach, I think, makes perfect sense and "just works" in all reasonable situations. We should also document this and other stuff in the same general area somewhere. I'll try not to forget about it by the time I submit this for real, so I can include some sort of attempt at documenting along with it. [...]
+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.)
While I was writing this POC, I kept thinking "there has to be a better way"... Turns out I was right, and virDomainDeviceInfoIterate() is exactly what I was looking for. Thanks to both of you for pointing me in the right direction! The next version will use it :) [...]
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.
Fixed in the next version.
+ 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.
Except we will only add pcie-root to a virt guest if the QEMU binary has the QEMU_CAPS_OBJECT_GPEX capability: that's the reason why we can't just check to see if the machine type is virt. [...]
+ <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...
[...]
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.
[...]
This is also caused by lack of DISABLE_LEGACY
[...]
As is this. Once you add that cap to the test cases (for both xml2xml and xml2argv) the test cases should come out right.
Should have pointed that out, sorry. I was aware of that, but this commit is not the place to add capabilities to existing test cases: it should be done in a separate commit, either before or after this one. Or not done at all: I think it's good that our test cases are not too uniform in this case, because they cover more use cases and, in a way, reflect more realistically the kind of guest configuration we can expect to have found their way into the wild over time. So I'd vote for keeping it this way. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Oct 24, 2016 at 05:17:07PM +0200, Andrea Bolognani wrote:
On Sat, 2016-10-22 at 23:07 -0400, Laine Stump wrote:
(When I first saw your mail I didn't realize it was a patch, because it didn't have "PATCH" in the subject)
Fair enough, will send the next iteration as [RFC PATCH v2] :)
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...)
We really want to push for virtio-pci going forward as it has a number of advantages, but there are still legacy guest OSs out there that don't support virtio-pci at all yet we still want to be able to run.
Changing the default is usually a tricky part and may break some users. I'm not sure that this will save the need to adapt management applications and users. They will have to adapt in both cases to support legacy and new OSes based on libosinfo or another tool/database. If we make virtio-pci the default one, which I think is the way it should be used, they will have to make sure that with new libvirt for the same OS they will fallback to virtio-mmio. From what I can remember we've never done such change of default device model or default address and we always left it no management application or user to change the default to better model. In case of management applications it's not an issue because they will make sure that the best device models and device address are used. I'm not against changing the default to virtio-pci, but we may break things for some users and management tools like virt-manager unless they will adapt to this change.
virt-install is already capable of overriding the default address type on a per-device basis, eg.
--network network=default,model=virtio,address.type=pci
or
--disk size=10,bus=virtio,address.type=virtio-mmio
I think we should have some sort of global switch that sets address.type for all devices, so you don't have to repeat yourself. Or does something like that exist already?
I'm also not sure whether virt-manager is using libosinfo, but ideally the default address type would be something that can be chosen automatically based on the guest OS. CC'ing Cole and Pavel so they can provide some insights.
Virt-manager uses libosinfo and falls back to some sane hardcoded defaults. This should be definitely added to libosinfo. Pavel
On Mon, 2016-10-24 at 17:47 +0200, Pavel Hrdina wrote:
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...) We really want to push for virtio-pci going forward as it has a number of advantages, but there are still legacy guest OSs out there that don't support virtio-pci at all yet we still want to be able to run. Changing the default is usually a tricky part and may break some users. I'm not sure that this will save the need to adapt management applications and users. They will have to adapt in both cases to support legacy and new OSes based on libosinfo or another tool/database. If we make virtio-pci the default one, which I think is the way it should be used, they will have to make sure that with new libvirt for the same OS they will fallback to virtio-mmio. From what I can remember we've never done such change of default device model or default address and we always left it no management application or user to change the default to better model. In case of management applications it's not an issue because they will make sure that the best device models and device address are used. I'm not against changing the default to virtio-pci, but we may break things for some users and management tools like virt-manager unless they will adapt to this change.
You raise very good points, thanks for your input! :) AFAICT the only use case that we'd risk breaking is installing a legacy guest OS that doesn't support virtio-pci without requiring the user to explicitly ask for virtio-mmio addresses. Once libosinfo has learned about this, and virt-manager has been updated to query libosinfo and switch to virtio-mmio automatically if required, would you be okay with this change? I think for aarch64 we're still in a phase where we can afford to take some tradeoffs when it comes to compatibility, if they're properly motivated: in this specific case, seeing as basic stuff like device hotplug has simply never worked for virtio-mmio, I'm fairly confident nobody will want to stick with virtio-mmio for very long now that virtio-pci is finally viable. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Oct 24, 2016 at 06:24:07PM +0200, Andrea Bolognani wrote:
On Mon, 2016-10-24 at 17:47 +0200, Pavel Hrdina wrote:
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...) We really want to push for virtio-pci going forward as it has a number of advantages, but there are still legacy guest OSs out there that don't support virtio-pci at all yet we still want to be able to run. Changing the default is usually a tricky part and may break some users. I'm not sure that this will save the need to adapt management applications and users. They will have to adapt in both cases to support legacy and new OSes based on libosinfo or another tool/database. If we make virtio-pci the default one, which I think is the way it should be used, they will have to make sure that with new libvirt for the same OS they will fallback to virtio-mmio. From what I can remember we've never done such change of default device model or default address and we always left it no management application or user to change the default to better model. In case of management applications it's not an issue because they will make sure that the best device models and device address are used. I'm not against changing the default to virtio-pci, but we may break things for some users and management tools like virt-manager unless they will adapt to this change.
You raise very good points, thanks for your input! :)
AFAICT the only use case that we'd risk breaking is installing a legacy guest OS that doesn't support virtio-pci without requiring the user to explicitly ask for virtio-mmio addresses.
And that is only for new installs (just if someone misses that you said "installs").
Once libosinfo has learned about this, and virt-manager has been updated to query libosinfo and switch to virtio-mmio automatically if required, would you be okay with this change?
I think for aarch64 we're still in a phase where we can afford to take some tradeoffs when it comes to compatibility, if they're properly motivated: in this specific case, seeing as basic stuff like device hotplug has simply never worked for virtio-mmio, I'm fairly confident nobody will want to stick with virtio-mmio for very long now that virtio-pci is finally viable.
And I feel like we can change defaults like that, especially with new installs, that's why we save the generated info in the XMLs. If we were not able to change the defaults, we would not be able to add *anything* to the command line by default. And, yes, aarch64 is still in its diapers, so I, too, feel like we have even more leeway in such scenarios.
-- Andrea Bolognani / Red Hat / Virtualization
On Tue, Oct 25, 2016 at 02:10:37PM +0200, Martin Kletzander wrote:
On Mon, Oct 24, 2016 at 06:24:07PM +0200, Andrea Bolognani wrote:
On Mon, 2016-10-24 at 17:47 +0200, Pavel Hrdina wrote:
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...) We really want to push for virtio-pci going forward as it has a number of advantages, but there are still legacy guest OSs out there that don't support virtio-pci at all yet we still want to be able to run. Changing the default is usually a tricky part and may break some users. I'm not sure that this will save the need to adapt management applications and users. They will have to adapt in both cases to support legacy and new OSes based on libosinfo or another tool/database. If we make virtio-pci the default one, which I think is the way it should be used, they will have to make sure that with new libvirt for the same OS they will fallback to virtio-mmio. From what I can remember we've never done such change of default device model or default address and we always left it no management application or user to change the default to better model. In case of management applications it's not an issue because they will make sure that the best device models and device address are used. I'm not against changing the default to virtio-pci, but we may break things for some users and management tools like virt-manager unless they will adapt to this change.
You raise very good points, thanks for your input! :)
AFAICT the only use case that we'd risk breaking is installing a legacy guest OS that doesn't support virtio-pci without requiring the user to explicitly ask for virtio-mmio addresses.
And that is only for new installs (just if someone misses that you said "installs").
Yes it will break only new installs.
Once libosinfo has learned about this, and virt-manager has been updated to query libosinfo and switch to virtio-mmio automatically if required, would you be okay with this change?
I think for aarch64 we're still in a phase where we can afford to take some tradeoffs when it comes to compatibility, if they're properly motivated: in this specific case, seeing as basic stuff like device hotplug has simply never worked for virtio-mmio, I'm fairly confident nobody will want to stick with virtio-mmio for very long now that virtio-pci is finally viable.
And I feel like we can change defaults like that, especially with new installs, that's why we save the generated info in the XMLs. If we were not able to change the defaults, we would not be able to add *anything* to the command line by default. And, yes, aarch64 is still in its diapers, so I, too, feel like we have even more leeway in such scenarios.
I agree that we can change it. It was just to make a not that the management application will have to update itself in any case to adapt to new libvirt. Pavel
participants (4)
-
Andrea Bolognani -
Laine Stump -
Martin Kletzander -
Pavel Hrdina