12:24 p.m.
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
4:50 p.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:07 p.m.
(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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
1:43 a.m.
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
11:04 a.m.
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...
10:17 a.m.
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
10:47 a.m.
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
11:24 a.m.
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
7:10 a.m.
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
7:22 a.m.
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
2985
days inactive
2989
days old
9 comments
4 participants
participants (4)
-
Andrea Bolognani -
Laine Stump -
Martin Kletzander -
Pavel Hrdina