[libvirt] [PATCH v2 0/2] Allow PCI virtio on ARM "virt" machine
Virt machine in qemu since v2.3.0 has PCI generic host controller, and can use PCI devices. This provides performance improvement as well as vhost-net with irqfd support for virtio-net. However libvirt still insists on virtio devices attached to virt machine to have MMIO bindings. This patch allows to use both. If the user doesn't specify <address type='virtio-mmio'>, PCI will be used by default. Changes since v1: - Added capability based on qemu version number - Recognize also "virt-" prefix Pavel Fedin (2): Introduce QEMU_CAPS_ARM_VIRT_PCI Allow PCI virtio on ARM "virt" machine src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 15 ++++++++++++--- 3 files changed, 18 insertions(+), 3 deletions(-) -- 1.9.5.msysgit.0
This capability specifies that "virt" machine on ARM has PCI controller. Enabled when version is at least 2.3.0. Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 6 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index ca7a7c2..2eccc97 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -285,6 +285,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "dea-key-wrap", "pci-serial", "aarch64-off", + "arm-virt-pci", ); @@ -1330,6 +1331,10 @@ virQEMUCapsComputeCmdFlags(const char *help, virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_SHARE_POLICY); } + if (version >= 2003000) { + virQEMUCapsSet(qemuCaps, QEMU_CAPS_ARM_VIRT_PCI); + } + return 0; } diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b5a7770..3c1a8b9 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -229,6 +229,7 @@ typedef enum { QEMU_CAPS_DEA_KEY_WRAP = 187, /* -machine dea_key_wrap */ QEMU_CAPS_DEVICE_PCI_SERIAL = 188, /* -device pci-serial */ QEMU_CAPS_CPU_AARCH64_OFF = 189, /* -cpu ...,aarch64=off */ + QEMU_CAPS_ARM_VIRT_PCI = 190, /* ARM 'virt' machine has PCI bus */ QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; -- 1.9.5.msysgit.0
On 06/11/2015 02:40 AM, Pavel Fedin wrote:
This capability specifies that "virt" machine on ARM has PCI controller. Enabled when version is at least 2.3.0.
Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 6 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index ca7a7c2..2eccc97 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -285,6 +285,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "dea-key-wrap", "pci-serial", "aarch64-off", + "arm-virt-pci", );
@@ -1330,6 +1331,10 @@ virQEMUCapsComputeCmdFlags(const char *help, virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_SHARE_POLICY); }
+ if (version >= 2003000) { + virQEMUCapsSet(qemuCaps, QEMU_CAPS_ARM_VIRT_PCI); + } + return 0; }
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b5a7770..3c1a8b9 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -229,6 +229,7 @@ typedef enum { QEMU_CAPS_DEA_KEY_WRAP = 187, /* -machine dea_key_wrap */ QEMU_CAPS_DEVICE_PCI_SERIAL = 188, /* -device pci-serial */ QEMU_CAPS_CPU_AARCH64_OFF = 189, /* -cpu ...,aarch64=off */ + QEMU_CAPS_ARM_VIRT_PCI = 190, /* ARM 'virt' machine has PCI bus */
QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags;
ACK and pushed, tweaked to avoid the conflict with .git (since additions to qemu_capabilities are always conflicting, it's better to get this in early) - Cole
On Sun, Jun 21, 2015 at 16:10:47 -0400, Cole Robinson wrote:
On 06/11/2015 02:40 AM, Pavel Fedin wrote:
This capability specifies that "virt" machine on ARM has PCI controller. Enabled when version is at least 2.3.0.
Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 6 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index ca7a7c2..2eccc97 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -285,6 +285,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "dea-key-wrap", "pci-serial", "aarch64-off", + "arm-virt-pci", );
@@ -1330,6 +1331,10 @@ virQEMUCapsComputeCmdFlags(const char *help, virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_SHARE_POLICY); }
+ if (version >= 2003000) { + virQEMUCapsSet(qemuCaps, QEMU_CAPS_ARM_VIRT_PCI); + } + return 0; }
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b5a7770..3c1a8b9 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -229,6 +229,7 @@ typedef enum { QEMU_CAPS_DEA_KEY_WRAP = 187, /* -machine dea_key_wrap */ QEMU_CAPS_DEVICE_PCI_SERIAL = 188, /* -device pci-serial */ QEMU_CAPS_CPU_AARCH64_OFF = 189, /* -cpu ...,aarch64=off */ + QEMU_CAPS_ARM_VIRT_PCI = 190, /* ARM 'virt' machine has PCI bus */
QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags;
ACK and pushed, tweaked to avoid the conflict with .git (since additions to qemu_capabilities are always conflicting, it's better to get this in early)
It breaks syntax-check since the 'if' has a single line body with braces around it. Also pushing a capabiltity without the code that will actually use it is not exactly a good idea since it might never be used if the next patch gets abandoned and since they are considered public we might be stuck with it forever. Peter
On 06/22/2015 04:21 AM, Peter Krempa wrote:
On Sun, Jun 21, 2015 at 16:10:47 -0400, Cole Robinson wrote:
On 06/11/2015 02:40 AM, Pavel Fedin wrote:
This capability specifies that "virt" machine on ARM has PCI controller. Enabled when version is at least 2.3.0.
Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- src/qemu/qemu_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 6 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index ca7a7c2..2eccc97 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -285,6 +285,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "dea-key-wrap", "pci-serial", "aarch64-off", + "arm-virt-pci", );
@@ -1330,6 +1331,10 @@ virQEMUCapsComputeCmdFlags(const char *help, virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_SHARE_POLICY); }
+ if (version >= 2003000) { + virQEMUCapsSet(qemuCaps, QEMU_CAPS_ARM_VIRT_PCI); + } + return 0; }
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b5a7770..3c1a8b9 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -229,6 +229,7 @@ typedef enum { QEMU_CAPS_DEA_KEY_WRAP = 187, /* -machine dea_key_wrap */ QEMU_CAPS_DEVICE_PCI_SERIAL = 188, /* -device pci-serial */ QEMU_CAPS_CPU_AARCH64_OFF = 189, /* -cpu ...,aarch64=off */ + QEMU_CAPS_ARM_VIRT_PCI = 190, /* ARM 'virt' machine has PCI bus */
QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags;
ACK and pushed, tweaked to avoid the conflict with .git (since additions to qemu_capabilities are always conflicting, it's better to get this in early)
It breaks syntax-check since the 'if' has a single line body with braces around it.
Also pushing a capabiltity without the code that will actually use it is not exactly a good idea since it might never be used if the next patch gets abandoned and since they are considered public we might be stuck with it forever.
hmm sorry, I suck with syntax-check... but WRT to this specific capability, I don't know how we won't end up using it, so in this case I think it's pretty safe. But I won't push a check again without it's accompanying usage. - Cole
Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- src/qemu/qemu_command.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0a6d92f..2acdc6a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -457,7 +457,7 @@ qemuDomainSupportsNicdev(virDomainDefPtr def, /* non-virtio ARM nics require legacy -net nic */ if (((def->os.arch == VIR_ARCH_ARMV7L) || (def->os.arch == VIR_ARCH_AARCH64)) && - net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) + strcmp(net->model, "virtio")) return false; return true; @@ -1375,8 +1375,9 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def, if (((def->os.arch == VIR_ARCH_ARMV7L) || (def->os.arch == VIR_ARCH_AARCH64)) && (STRPREFIX(def->os.machine, "vexpress-") || - STREQ(def->os.machine, "virt") || - STRPREFIX(def->os.machine, "virt-")) && + (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ARM_VIRT_PCI) && + (STREQ(def->os.machine, "virt") || + STRPREFIX(def->os.machine, "virt-")))) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) { qemuDomainPrimeVirtioDeviceAddresses( def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO); @@ -2498,6 +2499,14 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) continue; + /* ARM virt machine can also have virtio-mmio devices */ + if (((def->os.arch == VIR_ARCH_ARMV7L) || + (def->os.arch == VIR_ARCH_AARCH64)) && + (STREQ(def->os.machine, "virt") || + STRPREFIX(def->os.machine, "virt-")) && + def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) + continue; + if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("virtio disk cannot have an address of type '%s'"), -- 1.9.5.msysgit.0
On 06/11/2015 02:40 AM, Pavel Fedin wrote:
Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- src/qemu/qemu_command.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0a6d92f..2acdc6a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -457,7 +457,7 @@ qemuDomainSupportsNicdev(virDomainDefPtr def, /* non-virtio ARM nics require legacy -net nic */ if (((def->os.arch == VIR_ARCH_ARMV7L) || (def->os.arch == VIR_ARCH_AARCH64)) && - net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) + strcmp(net->model, "virtio")) return false;
return true; @@ -1375,8 +1375,9 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def, if (((def->os.arch == VIR_ARCH_ARMV7L) || (def->os.arch == VIR_ARCH_AARCH64)) && (STRPREFIX(def->os.machine, "vexpress-") || - STREQ(def->os.machine, "virt") || - STRPREFIX(def->os.machine, "virt-")) && + (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ARM_VIRT_PCI) && + (STREQ(def->os.machine, "virt") || + STRPREFIX(def->os.machine, "virt-")))) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) { qemuDomainPrimeVirtioDeviceAddresses( def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO); @@ -2498,6 +2499,14 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) continue;
+ /* ARM virt machine can also have virtio-mmio devices */ + if (((def->os.arch == VIR_ARCH_ARMV7L) || + (def->os.arch == VIR_ARCH_AARCH64)) && + (STREQ(def->os.machine, "virt") || + STRPREFIX(def->os.machine, "virt-")) && + def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) + continue; + if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("virtio disk cannot have an address of type '%s'"),
We can't apply this without unit test additions. We will at least need a test for qemu 2.3+ defaulting to PCI, and a test to ensure that manually specified virtio-mmio continues to do the correct thing. I'll take a stab at it this week - Cole
Hello!
We can't apply this without unit test additions. We will at least need a test for qemu 2.3+ defaulting to PCI, and a test to ensure that manually specified virtio-mmio continues to do the correct thing. I'll take a stab at it this week
Thank you very much for your assistance. To tell the truth i'm not familiar with libvirt's unit-test system. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
Hello! PING!
We can't apply this without unit test additions. We will at least need a test for qemu 2.3+ defaulting to PCI, and a test to ensure that manually specified virtio-mmio continues to do the correct thing. I'll take a stab at it this week
How are things going? Something like 2 weeks have passed. I could write these tests if somebody explains me how our unit-testing works and where to put them. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On 06/29/2015 02:52 AM, Pavel Fedin wrote:
Hello! PING!
We can't apply this without unit test additions. We will at least need a test for qemu 2.3+ defaulting to PCI, and a test to ensure that manually specified virtio-mmio continues to do the correct thing. I'll take a stab at it this week
How are things going? Something like 2 weeks have passed.
Sorry, I've been slacking. Unfortunately thinking about this some more, the current patch approach might not be acceptable as is, since current distros as a guest don't support -M virt with PCI... for example Fedora 21 or 22. But they _do_ work with virtio-mmio. So I don't know if we can or should change the default outright to virtio-pci when qemu supports it. Maybe when we get versioned -M virt types we can key off a more modern type. Dunno, I have to play with it all first to get a better idea. That said, patches that just allow using pci instead of mmio if the user explicitly requests it should be fine for now before we consider if/how to change the default from mmio to pci.
I could write these tests if somebody explains me how our unit-testing works and where to put them.
Read HACKING bits about 'make check'. The main tests you'll be extending here are tests/qemuxml2argvtest.c with data in tests/qemuxml2argvdata/. Check the existing aarch64 and arm ones to get an idea, or look at some of my previous commits regarding arm/aarch64 for examples - Cole
Hello!
Unfortunately thinking about this some more, the current patch approach might not be acceptable as is, since current distros as a guest don't support -M virt with PCI... for example Fedora 21 or 22.
How can this be? If qemu version is new enough, virt machine should have a PCI controller. You do not have to supply any extra options, it's just there. It's in mainstream. Or do your distros artificially raise version number? Or are you talking about distros themselves? But... Again, how? Do you have "Generic PCI controller" and virtio-PCI drivers disabled in .config of your kernel? Why? Shouldn't this be fixed then? virtio-PCI has much better performance than virtio-mmio, because you can use vhost-net with irqfds on it. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On 06/30/2015 02:47 AM, Pavel Fedin wrote:
Hello!
Unfortunately thinking about this some more, the current patch approach might not be acceptable as is, since current distros as a guest don't support -M virt with PCI... for example Fedora 21 or 22.
How can this be? If qemu version is new enough, virt machine should have a PCI controller. You do not have to supply any extra options, it's just there. It's in mainstream. Or do your distros artificially raise version number? Or are you talking about distros themselves? But... Again, how? Do you have "Generic PCI controller" and virtio-PCI drivers disabled in .config of your kernel? Why? Shouldn't this be fixed then? virtio-PCI has much better performance than virtio-mmio, because you can use vhost-net with irqfds on it.
I'm talking about the distro kernel as a guest. My understanding is that Fedora 21/22 AArch64 does _not_ work with virtio-pci, but it does work with virtio-mmio. But I've yet to confirm yet... - Cole
On Mi, 2015-07-01 at 19:32 -0400, Cole Robinson wrote:
On 06/30/2015 02:47 AM, Pavel Fedin wrote:
Hello!
Unfortunately thinking about this some more, the current patch approach might not be acceptable as is, since current distros as a guest don't support -M virt with PCI... for example Fedora 21 or 22.
How can this be? If qemu version is new enough, virt machine should have a PCI controller. You do not have to supply any extra options, it's just there. It's in mainstream. Or do your distros artificially raise version number? Or are you talking about distros themselves? But... Again, how? Do you have "Generic PCI controller" and virtio-PCI drivers disabled in .config of your kernel? Why? Shouldn't this be fixed then? virtio-PCI has much better performance than virtio-mmio, because you can use vhost-net with irqfds on it.
I'm talking about the distro kernel as a guest. My understanding is that Fedora 21/22 AArch64 does _not_ work with virtio-pci, but it does work with virtio-mmio. But I've yet to confirm yet...
arm64 has no generic pci host support yet (as of upstream kernel 4.1). arm has it for a while (and at least f22 works just fine with it). cheers, Gerd
Hello!
arm64 has no generic pci host support yet (as of upstream kernel 4.1).
There are patches: http://comments.gmane.org/gmane.linux.kernel.pci/41240. They did not pass review, and so far no better version has been published. However, i can confirm that they work, and i successfully implemented MSI on top of that (a small patch is needed). I believe distro maintainers could pick them up. PCI support gives great performance advantage, you can use vhost-net with irqfds. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
Hello!
I'm talking about the distro kernel as a guest. My understanding is that Fedora 21/22 AArch64 does _not_ work with virtio-pci, but it does work with virtio-mmio. But I've yet to confirm yet...
arm64 has no generic pci host support yet (as of upstream kernel 4.1). arm has it for a while (and at least f22 works just fine with it).
I am working on tests, and i have discovered some issues with my current implementation. Whether i concentrate my efforts on fixing them or just redo everything from scratch, depends on what you say no. So what is the final verdict? Should we: a) Change the default to PCI, for better performance; or: b) Leave the default as it is, but make explicit <address type='pci'> working? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
Hi,
I am working on tests, and i have discovered some issues with my current implementation. Whether i concentrate my efforts on fixing them or just redo everything from scratch, depends on what you say no. So what is the final verdict? Should we: a) Change the default to PCI, for better performance; or: b) Leave the default as it is, but make explicit <address type='pci'> working?
Clearly (b). For the default it is important that it is backward compatible (to older libvirt versions) and works with as many guests as possible. You might want to have a look at libosinfo which records (among other information) which guest can handle which device type, exactly to allow this kind of optimization. cheers, Gerd
Hi!
Clearly (b). For the default it is important that it is backward compatible (to older libvirt versions) and works with as many guests as possible.
One very last counter-argument... Actually, even if we change our default, we are still backwards-compatible, because older libvirt automatically adds <address type='virtio-mmio'/> to the XML. So, all XMLs, created before the upgrade, will have it, so everything will keep working. The only thing affected by the change is domains created after the upgrade. You'll need to specify <address type='virtio-mmio'/> explicitly in order to work with older OSes. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
participants (4)
-
Cole Robinson -
Gerd Hoffmann -
Pavel Fedin -
Peter Krempa