[libvirt] [PATCH 00/11] qemu: Improve / cleanup QEMU binary handling
This is the output of 'virsh capabilities' on my laptop: <guest> <os_type>hvm</os_type> <arch name='x86_64'> <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <machine maxCpus='255'>pc-i440fx-3.0</machine> <machine canonical='pc-i440fx-3.0' maxCpus='255'>pc</machine> <machine maxCpus='288'>pc-q35-3.0</machine> <machine canonical='pc-q35-3.0' maxCpus='288'>q35</machine> <!-- Actually way more machine types listed here --> <domain type='qemu'/> <domain type='kvm'> <emulator>/usr/bin/qemu-kvm</emulator> <machine maxCpus='255'>pc-i440fx-3.0</machine> <machine canonical='pc-i440fx-3.0' maxCpus='255'>pc</machine> <machine maxCpus='288'>pc-q35-3.0</machine> <machine canonical='pc-q35-3.0' maxCpus='288'>q35</machine> <!-- Actually way more machine types listed here --> </domain> </arch> <!-- Other stuff we don't care about --> </guest> Notice how all machine types are listed twice, and how we report that qemu-system-x86_64 for TCG guests qemu-kvm must be used for KVM guests - which is inaccurate, since the former can run KVM guests just fine. After this series, the output is much more reasonable: <guest> <os_type>hvm</os_type> <arch name='x86_64'> <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <machine maxCpus='255'>pc-i440fx-3.0</machine> <machine canonical='pc-i440fx-3.0' maxCpus='255'>pc</machine> <machine maxCpus='288'>pc-q35-3.0</machine> <machine canonical='pc-q35-3.0' maxCpus='288'>q35</machine> <!-- Actually way more machine types listed here --> <domain type='qemu'/> <domain type='kvm'/> </arch> <!-- Other stuff we don't care about --> </guest> As a bonus the code gets *simpler* in the process instead of more complicated, and we even get to shave off ~100 lines! Yay! Andrea Bolognani (11): qemu: Move comments to virQEMUCapsGuestIsNative() qemu: Don't duplicate binary name in capabilities qemu: Move armv7l-on-aarch64 special case qemu: Stop looking after finding the first binary qemu: Expect a single binary in virQEMUCapsInitGuest() qemu: Remove unnecessary variables qemu: Don't look for "qemu-kvm" and "kvm" binaries qemu: Simplify QEMU binary search qemu: Rename qemubinCaps => qemuCaps qemu: Refactor virQEMUCapsCacheLookupByArch() qemu: Prefer qemu-system-* binaries src/qemu/qemu_capabilities.c | 170 +++++++----------- src/qemu/qemu_capabilities.h | 4 +- .../qemucaps2xmloutdata/caps_1.5.3.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_1.6.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_1.7.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.1.1.x86_64.xml | 4 +- .../caps_2.10.0.aarch64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.10.0.ppc64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.10.0.s390x.xml | 4 +- .../caps_2.10.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.11.0.s390x.xml | 4 +- .../caps_2.11.0.x86_64.xml | 4 +- .../caps_2.12.0.aarch64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.12.0.ppc64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.12.0.s390x.xml | 4 +- .../caps_2.12.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.4.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.5.0.x86_64.xml | 4 +- .../caps_2.6.0.aarch64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.6.0.ppc64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.6.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.7.0.s390x.xml | 4 +- .../qemucaps2xmloutdata/caps_2.7.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.8.0.s390x.xml | 4 +- .../qemucaps2xmloutdata/caps_2.8.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.9.0.ppc64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.9.0.s390x.xml | 4 +- .../qemucaps2xmloutdata/caps_2.9.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_3.0.0.ppc64.xml | 4 +- .../qemucaps2xmloutdata/caps_3.0.0.x86_64.xml | 4 +- tests/qemucaps2xmltest.c | 2 - 31 files changed, 97 insertions(+), 191 deletions(-) -- 2.17.1
The function performing the checks, rather than its callers, should contain comments explaining the rationale behind said checks. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 04c2adcfb5..e14da5acab 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -631,15 +631,19 @@ bool virQEMUCapsGuestIsNative(virArch host, virArch guest) { + /* host & guest arches match */ if (host == guest) return true; + /* hostarch is x86_64 and guest arch is i686 (needs -cpu qemu32) */ if (host == VIR_ARCH_X86_64 && guest == VIR_ARCH_I686) return true; + /* hostarch is aarch64 and guest arch is armv7l (needs -cpu aarch64=off) */ if (host == VIR_ARCH_AARCH64 && guest == VIR_ARCH_ARMV7L) return true; + /* hostarch and guestarch are both ppc64 */ if (ARCH_IS_PPC64(host) && ARCH_IS_PPC64(guest)) return true; @@ -753,12 +757,6 @@ virQEMUCapsInitGuest(virCapsPtr caps, } } - /* qemu-kvm/kvm binaries can only be used if - * - host & guest arches match - * - hostarch is x86_64 and guest arch is i686 (needs -cpu qemu32) - * - hostarch is aarch64 and guest arch is armv7l (needs -cpu aarch64=off) - * - hostarch and guestarch are both ppc64* - */ if (virQEMUCapsGuestIsNative(hostarch, guestarch)) { const char *kvmbins[] = { "/usr/libexec/qemu-kvm", /* RHEL */ -- 2.17.1
virCapabilitiesAddGuestDomain() takes an optional binary name: this is intended for cases where a certain domain type can't use the default one registered for the guest architecture, but has to use a special binary instead. The current code, however, will pass 'binary' again when 'kvmbin' is not defined, which is unnecessary as 'binary' has been registered as default earlier, and will result in capabilities output such as <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> <domain type='kvm'> <emulator>/usr/bin/qemu-system-x86_64</emulator> </domain> with the second <emulator> element providing no additional information. Change it so that, when 'kvmbin' is not defined, NULL is passed and so the default emulator will be used instead. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 2 +- tests/qemucaps2xmloutdata/caps_1.5.3.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_1.6.0.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_1.7.0.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.1.1.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.10.0.aarch64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.10.0.ppc64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.10.0.s390x.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.10.0.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.11.0.s390x.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.11.0.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.12.0.aarch64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.12.0.ppc64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.12.0.s390x.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.12.0.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.4.0.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.5.0.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.6.0.aarch64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.6.0.ppc64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.6.0.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.7.0.s390x.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.7.0.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.8.0.s390x.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.8.0.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.9.0.ppc64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.9.0.s390x.xml | 4 +--- tests/qemucaps2xmloutdata/caps_2.9.0.x86_64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_3.0.0.ppc64.xml | 4 +--- tests/qemucaps2xmloutdata/caps_3.0.0.x86_64.xml | 4 +--- 29 files changed, 29 insertions(+), 85 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e14da5acab..88f5b1f34e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -885,7 +885,7 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, if ((dom = virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, - kvmbin ? kvmbin : binary, + kvmbin ? kvmbin : NULL, NULL, nmachines, machines)) == NULL) { diff --git a/tests/qemucaps2xmloutdata/caps_1.5.3.x86_64.xml b/tests/qemucaps2xmloutdata/caps_1.5.3.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_1.5.3.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_1.5.3.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_1.6.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_1.6.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_1.6.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_1.6.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_1.7.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_1.7.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_1.7.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_1.7.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.1.1.x86_64.xml b/tests/qemucaps2xmloutdata/caps_2.1.1.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_2.1.1.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.1.1.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.10.0.aarch64.xml b/tests/qemucaps2xmloutdata/caps_2.10.0.aarch64.xml index a879d67df3..f6572c8ecd 100644 --- a/tests/qemucaps2xmloutdata/caps_2.10.0.aarch64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.10.0.aarch64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-aarch64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-aarch64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.10.0.ppc64.xml b/tests/qemucaps2xmloutdata/caps_2.10.0.ppc64.xml index 74eaf3ba0e..85623f3980 100644 --- a/tests/qemucaps2xmloutdata/caps_2.10.0.ppc64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.10.0.ppc64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-ppc64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-ppc64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.10.0.s390x.xml b/tests/qemucaps2xmloutdata/caps_2.10.0.s390x.xml index 20ef995d62..bb82a15040 100644 --- a/tests/qemucaps2xmloutdata/caps_2.10.0.s390x.xml +++ b/tests/qemucaps2xmloutdata/caps_2.10.0.s390x.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-s390x</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-s390x</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.10.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_2.10.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_2.10.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.10.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.11.0.s390x.xml b/tests/qemucaps2xmloutdata/caps_2.11.0.s390x.xml index 20ef995d62..bb82a15040 100644 --- a/tests/qemucaps2xmloutdata/caps_2.11.0.s390x.xml +++ b/tests/qemucaps2xmloutdata/caps_2.11.0.s390x.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-s390x</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-s390x</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.11.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_2.11.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_2.11.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.11.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.12.0.aarch64.xml b/tests/qemucaps2xmloutdata/caps_2.12.0.aarch64.xml index a879d67df3..f6572c8ecd 100644 --- a/tests/qemucaps2xmloutdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.12.0.aarch64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-aarch64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-aarch64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.12.0.ppc64.xml b/tests/qemucaps2xmloutdata/caps_2.12.0.ppc64.xml index 74eaf3ba0e..85623f3980 100644 --- a/tests/qemucaps2xmloutdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.12.0.ppc64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-ppc64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-ppc64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.12.0.s390x.xml b/tests/qemucaps2xmloutdata/caps_2.12.0.s390x.xml index 20ef995d62..bb82a15040 100644 --- a/tests/qemucaps2xmloutdata/caps_2.12.0.s390x.xml +++ b/tests/qemucaps2xmloutdata/caps_2.12.0.s390x.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-s390x</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-s390x</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.12.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_2.12.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.12.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.4.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_2.4.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_2.4.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.4.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.5.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_2.5.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_2.5.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.5.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.6.0.aarch64.xml b/tests/qemucaps2xmloutdata/caps_2.6.0.aarch64.xml index a879d67df3..f6572c8ecd 100644 --- a/tests/qemucaps2xmloutdata/caps_2.6.0.aarch64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.6.0.aarch64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-aarch64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-aarch64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.6.0.ppc64.xml b/tests/qemucaps2xmloutdata/caps_2.6.0.ppc64.xml index 74eaf3ba0e..85623f3980 100644 --- a/tests/qemucaps2xmloutdata/caps_2.6.0.ppc64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.6.0.ppc64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-ppc64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-ppc64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.6.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_2.6.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_2.6.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.6.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.7.0.s390x.xml b/tests/qemucaps2xmloutdata/caps_2.7.0.s390x.xml index 20ef995d62..bb82a15040 100644 --- a/tests/qemucaps2xmloutdata/caps_2.7.0.s390x.xml +++ b/tests/qemucaps2xmloutdata/caps_2.7.0.s390x.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-s390x</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-s390x</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.7.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_2.7.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_2.7.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.7.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.8.0.s390x.xml b/tests/qemucaps2xmloutdata/caps_2.8.0.s390x.xml index 20ef995d62..bb82a15040 100644 --- a/tests/qemucaps2xmloutdata/caps_2.8.0.s390x.xml +++ b/tests/qemucaps2xmloutdata/caps_2.8.0.s390x.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-s390x</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-s390x</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.8.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_2.8.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_2.8.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.8.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.9.0.ppc64.xml b/tests/qemucaps2xmloutdata/caps_2.9.0.ppc64.xml index 74eaf3ba0e..85623f3980 100644 --- a/tests/qemucaps2xmloutdata/caps_2.9.0.ppc64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.9.0.ppc64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-ppc64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-ppc64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.9.0.s390x.xml b/tests/qemucaps2xmloutdata/caps_2.9.0.s390x.xml index 20ef995d62..bb82a15040 100644 --- a/tests/qemucaps2xmloutdata/caps_2.9.0.s390x.xml +++ b/tests/qemucaps2xmloutdata/caps_2.9.0.s390x.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-s390x</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-s390x</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_2.9.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_2.9.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_2.9.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_2.9.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_3.0.0.ppc64.xml b/tests/qemucaps2xmloutdata/caps_3.0.0.ppc64.xml index 74eaf3ba0e..85623f3980 100644 --- a/tests/qemucaps2xmloutdata/caps_3.0.0.ppc64.xml +++ b/tests/qemucaps2xmloutdata/caps_3.0.0.ppc64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-ppc64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-ppc64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> diff --git a/tests/qemucaps2xmloutdata/caps_3.0.0.x86_64.xml b/tests/qemucaps2xmloutdata/caps_3.0.0.x86_64.xml index b58f54fefd..d41693a001 100644 --- a/tests/qemucaps2xmloutdata/caps_3.0.0.x86_64.xml +++ b/tests/qemucaps2xmloutdata/caps_3.0.0.x86_64.xml @@ -14,9 +14,7 @@ <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <domain type='qemu'/> - <domain type='kvm'> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - </domain> + <domain type='kvm'/> </arch> <features> <cpuselection/> -- 2.17.1
When running an armv7l guest on an aarch64 hosts, the qemu-system-aarch64 binary should be our first choice instead of qemu-system-arm since the former can take advantage of KVM acceleration. Move the special case to virQEMUCapsFindBinaryForArch() so that it's handled along with all other cases rather than on its own later on. Doing so will also make further refactoring easier. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 88f5b1f34e..09d4af70ec 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -708,6 +708,15 @@ virQEMUCapsFindBinaryForArch(virArch hostarch, const char *archstr; virArch target; + /* armv7l guests can only take advantage of KVM on aarch64 hosts by + * using the qemu-system-aarch64 binary, so look for that one first + * to avoid using qemu-system-arm (and thus TCG) instead */ + if (hostarch == VIR_ARCH_AARCH64 && guestarch == VIR_ARCH_ARMV7L) { + archstr = virQEMUCapsArchToString(hostarch); + if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL) + goto out; + } + /* First attempt: try the guest architecture as it is */ archstr = virQEMUCapsArchToString(guestarch); if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL) @@ -762,24 +771,9 @@ virQEMUCapsInitGuest(virCapsPtr caps, "/usr/libexec/qemu-kvm", /* RHEL */ "qemu-kvm", /* Fedora */ "kvm", /* Debian/Ubuntu */ - NULL, }; - /* x86 32-on-64 can be used with qemu-system-i386 and - * qemu-system-x86_64, so if we don't find a specific kvm binary, - * we can just fall back to the host arch native binary and - * everything works fine. - * - * arm is different in that 32-on-64 _only_ works with - * qemu-system-aarch64. So we have to add it to the kvmbins list - */ - if (hostarch == VIR_ARCH_AARCH64 && guestarch == VIR_ARCH_ARMV7L) - kvmbins[3] = "qemu-system-aarch64"; - for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) { - if (!kvmbins[i]) - continue; - kvmbin = virFindFileInPath(kvmbins[i]); if (!kvmbin) -- 2.17.1
When the guest is native, we are currently looking at potential KVM binaries regardless of whether or not we have already located a QEMU binary suitable to run the guest. This made sense back when KVM support was not part of QEMU proper, but these days the KVM binaries are in most cases just trivial wrapper scripts around the native QEMU binary so it doesn't make sense to poke at them unless they're the only binaries on the system, such as when running on RHEL. This will allow us to simplify both virQEMUCapsInitGuest() and virQEMUCapsInitGuestFromBinary(). Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 09d4af70ec..647fde4b6b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -747,10 +747,8 @@ virQEMUCapsInitGuest(virCapsPtr caps, virArch guestarch) { size_t i; - char *kvmbin = NULL; char *binary = NULL; virQEMUCapsPtr qemubinCaps = NULL; - virQEMUCapsPtr kvmbinCaps = NULL; int ret = -1; /* Check for existence of base emulator, or alternate base @@ -766,7 +764,7 @@ virQEMUCapsInitGuest(virCapsPtr caps, } } - if (virQEMUCapsGuestIsNative(hostarch, guestarch)) { + if (virQEMUCapsGuestIsNative(hostarch, guestarch) && !binary) { const char *kvmbins[] = { "/usr/libexec/qemu-kvm", /* RHEL */ "qemu-kvm", /* Fedora */ @@ -774,36 +772,28 @@ virQEMUCapsInitGuest(virCapsPtr caps, }; for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) { - kvmbin = virFindFileInPath(kvmbins[i]); + binary = virFindFileInPath(kvmbins[i]); - if (!kvmbin) + if (!binary) continue; - if (!(kvmbinCaps = virQEMUCapsCacheLookup(cache, kvmbin))) { + if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary))) { virResetLastError(); - VIR_FREE(kvmbin); + VIR_FREE(binary); continue; } - if (!binary) { - binary = kvmbin; - qemubinCaps = kvmbinCaps; - kvmbin = NULL; - kvmbinCaps = NULL; - } break; } } ret = virQEMUCapsInitGuestFromBinary(caps, binary, qemubinCaps, - kvmbin, kvmbinCaps, + NULL, NULL, guestarch); VIR_FREE(binary); - VIR_FREE(kvmbin); virObjectUnref(qemubinCaps); - virObjectUnref(kvmbinCaps); return ret; } -- 2.17.1
We're only ever passing a single binary when calling this function, so we can remove all code dealing with the possibility of a second binary being specified. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 19 ++++--------------- src/qemu/qemu_capabilities.h | 2 -- tests/qemucaps2xmltest.c | 2 -- 3 files changed, 4 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 647fde4b6b..aa2de9018c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -789,7 +789,6 @@ virQEMUCapsInitGuest(virCapsPtr caps, ret = virQEMUCapsInitGuestFromBinary(caps, binary, qemubinCaps, - NULL, NULL, guestarch); VIR_FREE(binary); @@ -802,8 +801,6 @@ int virQEMUCapsInitGuestFromBinary(virCapsPtr caps, const char *binary, virQEMUCapsPtr qemubinCaps, - const char *kvmbin, - virQEMUCapsPtr kvmbinCaps, virArch guestarch) { virCapsGuestPtr guest; @@ -816,8 +813,7 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, if (!binary) return 0; - if (virQEMUCapsGet(qemubinCaps, QEMU_CAPS_KVM) || - kvmbin) + if (virQEMUCapsGet(qemubinCaps, QEMU_CAPS_KVM)) haskvm = true; if (virQEMUCapsGetMachineTypesCaps(qemubinCaps, &nmachines, &machines) < 0) @@ -863,21 +859,14 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, if (haskvm) { virCapsGuestDomainPtr dom; - if (kvmbin && - virQEMUCapsGetMachineTypesCaps(kvmbinCaps, &nmachines, &machines) < 0) - goto cleanup; - if ((dom = virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, - kvmbin ? kvmbin : NULL, NULL, - nmachines, - machines)) == NULL) { + NULL, + 0, + NULL)) == NULL) { goto cleanup; } - - machines = NULL; - nmachines = 0; } if ((ARCH_IS_X86(guestarch) || guestarch == VIR_ARCH_AARCH64) && diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index e671f74ebb..94b3bbb2f6 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -612,8 +612,6 @@ const char *virQEMUCapsGetPreferredMachine(virQEMUCapsPtr qemuCaps); int virQEMUCapsInitGuestFromBinary(virCapsPtr caps, const char *binary, virQEMUCapsPtr qemubinCaps, - const char *kvmbin, - virQEMUCapsPtr kvmbinCaps, virArch guestarch); int virQEMUCapsFillDomainCaps(virCapsPtr caps, diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c index e765a03b73..883909a973 100644 --- a/tests/qemucaps2xmltest.c +++ b/tests/qemucaps2xmltest.c @@ -106,8 +106,6 @@ testGetCaps(char *capsData, const testQemuData *data) if (virQEMUCapsInitGuestFromBinary(caps, binary, qemuCaps, - NULL, - NULL, arch) < 0) { fprintf(stderr, "failed to create the capabilities from qemu"); goto error; -- 2.17.1
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index aa2de9018c..01be627af0 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -804,7 +804,6 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, virArch guestarch) { virCapsGuestPtr guest; - bool haskvm = false; virCapsGuestMachinePtr *machines = NULL; size_t nmachines = 0; int ret = -1; @@ -813,9 +812,6 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, if (!binary) return 0; - if (virQEMUCapsGet(qemubinCaps, QEMU_CAPS_KVM)) - haskvm = true; - if (virQEMUCapsGetMachineTypesCaps(qemubinCaps, &nmachines, &machines) < 0) goto cleanup; @@ -856,15 +852,13 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, NULL) == NULL) goto cleanup; - if (haskvm) { - virCapsGuestDomainPtr dom; - - if ((dom = virCapabilitiesAddGuestDomain(guest, - VIR_DOMAIN_VIRT_KVM, - NULL, - NULL, - 0, - NULL)) == NULL) { + if (virQEMUCapsGet(qemubinCaps, QEMU_CAPS_KVM)) { + if (virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_KVM, + NULL, + NULL, + 0, + NULL) == NULL) { goto cleanup; } } -- 2.17.1
Both Fedora's qemu-kvm and Debian's/Ubuntu's kvm are nothing more than paper-thin wrappers around the native QEMU binary, so we gain nothing by looking for them. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 01be627af0..fd8badc60b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -767,8 +767,6 @@ virQEMUCapsInitGuest(virCapsPtr caps, if (virQEMUCapsGuestIsNative(hostarch, guestarch) && !binary) { const char *kvmbins[] = { "/usr/libexec/qemu-kvm", /* RHEL */ - "qemu-kvm", /* Fedora */ - "kvm", /* Debian/Ubuntu */ }; for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) { -- 2.17.1
Now that we have reduced the number of sensible options down to either the native QEMU binary or RHEL's qemu-kvm, we can make virQEMUCapsInitGuest() a bit simpler. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fd8badc60b..72fa19a2b7 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -746,7 +746,6 @@ virQEMUCapsInitGuest(virCapsPtr caps, virArch hostarch, virArch guestarch) { - size_t i; char *binary = NULL; virQEMUCapsPtr qemubinCaps = NULL; int ret = -1; @@ -756,6 +755,13 @@ virQEMUCapsInitGuest(virCapsPtr caps, */ binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch); + /* RHEL doesn't follow the usual naming for QEMU binaries and ships + * a single binary named qemu-kvm outside of $PATH instead */ + if (virQEMUCapsGuestIsNative(hostarch, guestarch) && !binary) { + if (VIR_STRDUP(binary, "/usr/libexec/qemu-kvm") < 0) + return -1; + } + /* Ignore binary if extracting version info fails */ if (binary) { if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary))) { @@ -764,27 +770,6 @@ virQEMUCapsInitGuest(virCapsPtr caps, } } - if (virQEMUCapsGuestIsNative(hostarch, guestarch) && !binary) { - const char *kvmbins[] = { - "/usr/libexec/qemu-kvm", /* RHEL */ - }; - - for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) { - binary = virFindFileInPath(kvmbins[i]); - - if (!binary) - continue; - - if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary))) { - virResetLastError(); - VIR_FREE(binary); - continue; - } - - break; - } - } - ret = virQEMUCapsInitGuestFromBinary(caps, binary, qemubinCaps, guestarch); -- 2.17.1
On Thu, Sep 20, 2018 at 05:25:26PM +0200, Andrea Bolognani wrote:
Now that we have reduced the number of sensible options down to either the native QEMU binary or RHEL's qemu-kvm, we can make virQEMUCapsInitGuest() a bit simpler.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fd8badc60b..72fa19a2b7 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -746,7 +746,6 @@ virQEMUCapsInitGuest(virCapsPtr caps, virArch hostarch, virArch guestarch) { - size_t i; char *binary = NULL; virQEMUCapsPtr qemubinCaps = NULL; int ret = -1; @@ -756,6 +755,13 @@ virQEMUCapsInitGuest(virCapsPtr caps, */ binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch);
+ /* RHEL doesn't follow the usual naming for QEMU binaries and ships + * a single binary named qemu-kvm outside of $PATH instead */
This does not look like something upstream libvirt should worry about. Jano
+ if (virQEMUCapsGuestIsNative(hostarch, guestarch) && !binary) { + if (VIR_STRDUP(binary, "/usr/libexec/qemu-kvm") < 0) + return -1; + } + /* Ignore binary if extracting version info fails */ if (binary) { if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary))) {
On Fri, 2018-10-05 at 16:43 +0200, Ján Tomko wrote:
On Thu, Sep 20, 2018 at 05:25:26PM +0200, Andrea Bolognani wrote:
@@ -756,6 +755,13 @@ virQEMUCapsInitGuest(virCapsPtr caps, */ binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch);
+ /* RHEL doesn't follow the usual naming for QEMU binaries and ships + * a single binary named qemu-kvm outside of $PATH instead */
This does not look like something upstream libvirt should worry about.
I would be simple enough to turn this into a downstream-only patch; however, by doing so we would lose the ability to run unchanged upstream libvirt on top of RHEL, so I think it's preferable to leave it alone. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Oct 05, 2018 at 17:18:44 +0200, Andrea Bolognani wrote:
On Fri, 2018-10-05 at 16:43 +0200, Ján Tomko wrote:
On Thu, Sep 20, 2018 at 05:25:26PM +0200, Andrea Bolognani wrote:
@@ -756,6 +755,13 @@ virQEMUCapsInitGuest(virCapsPtr caps, */ binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch);
+ /* RHEL doesn't follow the usual naming for QEMU binaries and ships + * a single binary named qemu-kvm outside of $PATH instead */
This does not look like something upstream libvirt should worry about.
I would be simple enough to turn this into a downstream-only patch; however, by doing so we would lose the ability to run unchanged upstream libvirt on top of RHEL, so I think it's preferable to leave it alone.
Well, if we are going to bend the rules and make a hack for the mess some downstream distro made, then we should keep the original code which allows to add mess for other distros as well.
On Mon, 2018-10-08 at 12:59 +0200, Peter Krempa wrote:
On Fri, Oct 05, 2018 at 17:18:44 +0200, Andrea Bolognani wrote:
On Fri, 2018-10-05 at 16:43 +0200, Ján Tomko wrote:
On Thu, Sep 20, 2018 at 05:25:26PM +0200, Andrea Bolognani wrote:
@@ -756,6 +755,13 @@ virQEMUCapsInitGuest(virCapsPtr caps, */ binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch);
+ /* RHEL doesn't follow the usual naming for QEMU binaries and ships + * a single binary named qemu-kvm outside of $PATH instead */
This does not look like something upstream libvirt should worry about.
I would be simple enough to turn this into a downstream-only patch; however, by doing so we would lose the ability to run unchanged upstream libvirt on top of RHEL, so I think it's preferable to leave it alone.
Well, if we are going to bend the rules and make a hack for the mess some downstream distro made, then we should keep the original code which allows to add mess for other distros as well.
The downstream path names I dropped are no longer necessary because Debian and friends are using the standard binary names in all releases we support; the same is not true of RHEL, which is why that one path has not been removed along with the rest. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Oct 08, 2018 at 13:04:17 +0200, Andrea Bolognani wrote:
On Mon, 2018-10-08 at 12:59 +0200, Peter Krempa wrote:
On Fri, Oct 05, 2018 at 17:18:44 +0200, Andrea Bolognani wrote:
On Fri, 2018-10-05 at 16:43 +0200, Ján Tomko wrote:
On Thu, Sep 20, 2018 at 05:25:26PM +0200, Andrea Bolognani wrote:
@@ -756,6 +755,13 @@ virQEMUCapsInitGuest(virCapsPtr caps, */ binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch);
+ /* RHEL doesn't follow the usual naming for QEMU binaries and ships + * a single binary named qemu-kvm outside of $PATH instead */
This does not look like something upstream libvirt should worry about.
I would be simple enough to turn this into a downstream-only patch; however, by doing so we would lose the ability to run unchanged upstream libvirt on top of RHEL, so I think it's preferable to leave it alone.
Well, if we are going to bend the rules and make a hack for the mess some downstream distro made, then we should keep the original code which allows to add mess for other distros as well.
The downstream path names I dropped are no longer necessary because Debian and friends are using the standard binary names in all releases we support; the same is not true of RHEL, which is why that one path has not been removed along with the rest.
That is perfectly fine with me. I'm arguing that we should keep the whole function that iterates the array of override paths even if it contains the exception only for RHEL. Doing a tailored special case for RHEL seems like we are favoriting it somehow which we should not. So either the loop is deleted without replacement and RHEL maintainers fix their mess or we keep it as-is so that other distros can add their mess as well.
On Mon, 2018-10-08 at 13:10 +0200, Peter Krempa wrote:
On Mon, Oct 08, 2018 at 13:04:17 +0200, Andrea Bolognani wrote:
On Mon, 2018-10-08 at 12:59 +0200, Peter Krempa wrote:
Well, if we are going to bend the rules and make a hack for the mess some downstream distro made, then we should keep the original code which allows to add mess for other distros as well.
The downstream path names I dropped are no longer necessary because Debian and friends are using the standard binary names in all releases we support; the same is not true of RHEL, which is why that one path has not been removed along with the rest.
That is perfectly fine with me. I'm arguing that we should keep the whole function that iterates the array of override paths even if it contains the exception only for RHEL.
Doing a tailored special case for RHEL seems like we are favoriting it somehow which we should not. So either the loop is deleted without replacement and RHEL maintainers fix their mess or we keep it as-is so that other distros can add their mess as well.
I don't expect distros that have gotten rid of their own incompatible binary names (eg. all except RHEL) will reintroduce them, so the loop would be needlessly verbose dead code right now and most likely going forward. If anyone will seriously accuse us of "favoring RHEL" because of how that piece of code is written, we can just point at the git history and show them that it was simply the most concise way to achieve its goal given the known constraints. Of course if at any point down the line I'm proven wrong about the assumption described above and we end up needing to special-case another binary name, then reintroducing the loop will be the correct solution. tl;dr I stand behind my changes and won't support reverting them unless a purely technical need motivates doing so. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Sep 20, 2018 at 05:25:26PM +0200, Andrea Bolognani wrote:
Now that we have reduced the number of sensible options down to either the native QEMU binary or RHEL's qemu-kvm, we can make virQEMUCapsInitGuest() a bit simpler.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fd8badc60b..72fa19a2b7 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -746,7 +746,6 @@ virQEMUCapsInitGuest(virCapsPtr caps, virArch hostarch, virArch guestarch) { - size_t i; char *binary = NULL; virQEMUCapsPtr qemubinCaps = NULL; int ret = -1; @@ -756,6 +755,13 @@ virQEMUCapsInitGuest(virCapsPtr caps, */ binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch);
+ /* RHEL doesn't follow the usual naming for QEMU binaries and ships + * a single binary named qemu-kvm outside of $PATH instead */ + if (virQEMUCapsGuestIsNative(hostarch, guestarch) && !binary) { + if (VIR_STRDUP(binary, "/usr/libexec/qemu-kvm") < 0) + return -1; + } + /* Ignore binary if extracting version info fails */ if (binary) { if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary))) { @@ -764,27 +770,6 @@ virQEMUCapsInitGuest(virCapsPtr caps, } }
- if (virQEMUCapsGuestIsNative(hostarch, guestarch) && !binary) { - const char *kvmbins[] = { - "/usr/libexec/qemu-kvm", /* RHEL */ - }; - - for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) { - binary = virFindFileInPath(kvmbins[i]); - - if (!binary) - continue; - - if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary))) { - virResetLastError(); - VIR_FREE(binary); - continue; - } - - break; - } - }
This patch is doing two things. It is moving the code block earlier, to let you drop the duplicated virQEMUCapsCacheLookup(). Second it is removing the array iteration & just checking one single path instead. I'd suggest we keep the array iteration, and just move the code. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, 2018-10-08 at 13:09 +0100, Daniel P. Berrangé wrote:
This patch is doing two things. It is moving the code block earlier, to let you drop the duplicated virQEMUCapsCacheLookup(). Second it is removing the array iteration & just checking one single path instead.
I'd suggest we keep the array iteration, and just move the code.
The patch has already been merged, so you'd have to partially revert it to achieve what you suggest. As explained elsewhere in the thread, the probability we would ever need more than one entry in the array is basically zero, so why have it? If it ever comes the time when we actually need a second entry, then sure, but now? Just in case? That's pretty much a textbook example of over-engineering IMHO. Anyway, I feel like I've spent way too much time arguing over what is ultimately a very, very minor detail already, and at the end of the day I just don't care enough to spend more energy on it. If either you or Peter want to reintroduce the array, then by all means go ahead. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Oct 08, 2018 at 02:35:30PM +0200, Andrea Bolognani wrote:
On Mon, 2018-10-08 at 13:09 +0100, Daniel P. Berrangé wrote:
This patch is doing two things. It is moving the code block earlier, to let you drop the duplicated virQEMUCapsCacheLookup(). Second it is removing the array iteration & just checking one single path instead.
I'd suggest we keep the array iteration, and just move the code.
The patch has already been merged, so you'd have to partially revert it to achieve what you suggest.
As explained elsewhere in the thread, the probability we would ever need more than one entry in the array is basically zero, so why have it? If it ever comes the time when we actually need a second entry, then sure, but now? Just in case? That's pretty much a textbook example of over-engineering IMHO.
Anyway, I feel like I've spent way too much time arguing over what is ultimately a very, very minor detail already, and at the end of the day I just don't care enough to spend more energy on it. If either you or Peter want to reintroduce the array, then by all means go ahead.
Oh, I missed that it was already merged. I don't care enough to change it post-merge. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The latter is used throughout libvirt, so use it here as well for consistency. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 16 ++++++++-------- src/qemu/qemu_capabilities.h | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 72fa19a2b7..b78b935404 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -747,7 +747,7 @@ virQEMUCapsInitGuest(virCapsPtr caps, virArch guestarch) { char *binary = NULL; - virQEMUCapsPtr qemubinCaps = NULL; + virQEMUCapsPtr qemuCaps = NULL; int ret = -1; /* Check for existence of base emulator, or alternate base @@ -764,18 +764,18 @@ virQEMUCapsInitGuest(virCapsPtr caps, /* Ignore binary if extracting version info fails */ if (binary) { - if (!(qemubinCaps = virQEMUCapsCacheLookup(cache, binary))) { + if (!(qemuCaps = virQEMUCapsCacheLookup(cache, binary))) { virResetLastError(); VIR_FREE(binary); } } ret = virQEMUCapsInitGuestFromBinary(caps, - binary, qemubinCaps, + binary, qemuCaps, guestarch); VIR_FREE(binary); - virObjectUnref(qemubinCaps); + virObjectUnref(qemuCaps); return ret; } @@ -783,7 +783,7 @@ virQEMUCapsInitGuest(virCapsPtr caps, int virQEMUCapsInitGuestFromBinary(virCapsPtr caps, const char *binary, - virQEMUCapsPtr qemubinCaps, + virQEMUCapsPtr qemuCaps, virArch guestarch) { virCapsGuestPtr guest; @@ -795,7 +795,7 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, if (!binary) return 0; - if (virQEMUCapsGetMachineTypesCaps(qemubinCaps, &nmachines, &machines) < 0) + if (virQEMUCapsGetMachineTypesCaps(qemuCaps, &nmachines, &machines) < 0) goto cleanup; /* We register kvm as the base emulator too, since we can @@ -820,7 +820,7 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, if (!virCapabilitiesAddGuestFeature(guest, "deviceboot", true, false)) goto cleanup; - if (virQEMUCapsGet(qemubinCaps, QEMU_CAPS_DISK_SNAPSHOT)) + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DISK_SNAPSHOT)) hasdisksnapshot = true; if (!virCapabilitiesAddGuestFeature(guest, "disksnapshot", hasdisksnapshot, @@ -835,7 +835,7 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, NULL) == NULL) goto cleanup; - if (virQEMUCapsGet(qemubinCaps, QEMU_CAPS_KVM)) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { if (virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM, NULL, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 94b3bbb2f6..253f19c2db 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -611,7 +611,7 @@ const char *virQEMUCapsGetPreferredMachine(virQEMUCapsPtr qemuCaps); int virQEMUCapsInitGuestFromBinary(virCapsPtr caps, const char *binary, - virQEMUCapsPtr qemubinCaps, + virQEMUCapsPtr qemuCaps, virArch guestarch); int virQEMUCapsFillDomainCaps(virCapsPtr caps, -- 2.17.1
The new implementation contains less duplicated code and is easier to extend. This commit is best viewed with 'git show -w'. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b78b935404..b621f39a82 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4752,26 +4752,27 @@ virQEMUCapsCacheLookupByArch(virFileCachePtr cache, virArch arch) { virQEMUCapsPtr ret = NULL; - virArch target; - struct virQEMUCapsSearchData data = { .arch = arch }; - - ret = virFileCacheLookupByFunc(cache, virQEMUCapsCompareArch, &data); - if (!ret) { - /* If the first attempt at finding capabilities has failed, try - * again using the QEMU target as lookup key instead */ - target = virQEMUCapsFindTarget(virArchFromHost(), data.arch); - if (target != data.arch) { - data.arch = target; - ret = virFileCacheLookupByFunc(cache, virQEMUCapsCompareArch, &data); - } - } + virArch archs[] = { + arch, + virQEMUCapsFindTarget(virArchFromHost(), arch), + }; + size_t j; - if (!ret) { - virReportError(VIR_ERR_INVALID_ARG, - _("unable to find any emulator to serve '%s' " - "architecture"), virArchToString(arch)); + for (j = 0; j < ARRAY_CARDINALITY(archs); j++) { + struct virQEMUCapsSearchData data = { + .arch = archs[j], + }; + + ret = virFileCacheLookupByFunc(cache, virQEMUCapsCompareArch, &data); + if (ret) + goto done; } + virReportError(VIR_ERR_INVALID_ARG, + _("unable to find any emulator to serve '%s' " + "architecture"), virArchToString(arch)); + + done: VIR_DEBUG("Returning caps %p for arch %s", ret, virArchToString(arch)); return ret; -- 2.17.1
We already prefer them in capabilities, and domcapabilities should be consistent with that. This commit is best viewed with 'git show -w'. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b621f39a82..77013a818e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -583,6 +583,7 @@ struct _virQEMUCaps { struct virQEMUCapsSearchData { virArch arch; + const char *binaryFilter; }; @@ -4743,7 +4744,15 @@ virQEMUCapsCompareArch(const void *payload, struct virQEMUCapsSearchData *data = (struct virQEMUCapsSearchData *)opaque; const virQEMUCaps *qemuCaps = payload; - return qemuCaps->arch == data->arch; + if (qemuCaps->arch != data->arch) + return false; + + if (data->binaryFilter && + !strstr(qemuCaps->binary, data->binaryFilter)) { + return false; + } + + return true; } @@ -4752,20 +4761,28 @@ virQEMUCapsCacheLookupByArch(virFileCachePtr cache, virArch arch) { virQEMUCapsPtr ret = NULL; + const char *binaryFilters[] = { + "qemu-system-", + NULL, + }; virArch archs[] = { arch, virQEMUCapsFindTarget(virArchFromHost(), arch), }; + size_t i; size_t j; - for (j = 0; j < ARRAY_CARDINALITY(archs); j++) { - struct virQEMUCapsSearchData data = { - .arch = archs[j], - }; + for (i = 0; i < ARRAY_CARDINALITY(binaryFilters); i++) { + for (j = 0; j < ARRAY_CARDINALITY(archs); j++) { + struct virQEMUCapsSearchData data = { + .arch = archs[j], + .binaryFilter = binaryFilters[i], + }; - ret = virFileCacheLookupByFunc(cache, virQEMUCapsCompareArch, &data); - if (ret) - goto done; + ret = virFileCacheLookupByFunc(cache, virQEMUCapsCompareArch, &data); + if (ret) + goto done; + } } virReportError(VIR_ERR_INVALID_ARG, -- 2.17.1
On 09/20/2018 05:25 PM, Andrea Bolognani wrote:
This is the output of 'virsh capabilities' on my laptop:
<guest> <os_type>hvm</os_type> <arch name='x86_64'> <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <machine maxCpus='255'>pc-i440fx-3.0</machine> <machine canonical='pc-i440fx-3.0' maxCpus='255'>pc</machine> <machine maxCpus='288'>pc-q35-3.0</machine> <machine canonical='pc-q35-3.0' maxCpus='288'>q35</machine> <!-- Actually way more machine types listed here --> <domain type='qemu'/> <domain type='kvm'> <emulator>/usr/bin/qemu-kvm</emulator> <machine maxCpus='255'>pc-i440fx-3.0</machine> <machine canonical='pc-i440fx-3.0' maxCpus='255'>pc</machine> <machine maxCpus='288'>pc-q35-3.0</machine> <machine canonical='pc-q35-3.0' maxCpus='288'>q35</machine> <!-- Actually way more machine types listed here --> </domain> </arch> <!-- Other stuff we don't care about --> </guest>
Notice how all machine types are listed twice, and how we report that qemu-system-x86_64 for TCG guests qemu-kvm must be used for KVM guests - which is inaccurate, since the former can run KVM guests just fine.
After this series, the output is much more reasonable:
<guest> <os_type>hvm</os_type> <arch name='x86_64'> <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <machine maxCpus='255'>pc-i440fx-3.0</machine> <machine canonical='pc-i440fx-3.0' maxCpus='255'>pc</machine> <machine maxCpus='288'>pc-q35-3.0</machine> <machine canonical='pc-q35-3.0' maxCpus='288'>q35</machine> <!-- Actually way more machine types listed here --> <domain type='qemu'/> <domain type='kvm'/> </arch> <!-- Other stuff we don't care about --> </guest>
As a bonus the code gets *simpler* in the process instead of more complicated, and we even get to shave off ~100 lines! Yay!
Andrea Bolognani (11): qemu: Move comments to virQEMUCapsGuestIsNative() qemu: Don't duplicate binary name in capabilities qemu: Move armv7l-on-aarch64 special case qemu: Stop looking after finding the first binary qemu: Expect a single binary in virQEMUCapsInitGuest() qemu: Remove unnecessary variables qemu: Don't look for "qemu-kvm" and "kvm" binaries qemu: Simplify QEMU binary search qemu: Rename qemubinCaps => qemuCaps qemu: Refactor virQEMUCapsCacheLookupByArch() qemu: Prefer qemu-system-* binaries
src/qemu/qemu_capabilities.c | 170 +++++++----------- src/qemu/qemu_capabilities.h | 4 +- .../qemucaps2xmloutdata/caps_1.5.3.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_1.6.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_1.7.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.1.1.x86_64.xml | 4 +- .../caps_2.10.0.aarch64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.10.0.ppc64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.10.0.s390x.xml | 4 +- .../caps_2.10.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.11.0.s390x.xml | 4 +- .../caps_2.11.0.x86_64.xml | 4 +- .../caps_2.12.0.aarch64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.12.0.ppc64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.12.0.s390x.xml | 4 +- .../caps_2.12.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.4.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.5.0.x86_64.xml | 4 +- .../caps_2.6.0.aarch64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.6.0.ppc64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.6.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.7.0.s390x.xml | 4 +- .../qemucaps2xmloutdata/caps_2.7.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.8.0.s390x.xml | 4 +- .../qemucaps2xmloutdata/caps_2.8.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.9.0.ppc64.xml | 4 +- .../qemucaps2xmloutdata/caps_2.9.0.s390x.xml | 4 +- .../qemucaps2xmloutdata/caps_2.9.0.x86_64.xml | 4 +- .../qemucaps2xmloutdata/caps_3.0.0.ppc64.xml | 4 +- .../qemucaps2xmloutdata/caps_3.0.0.x86_64.xml | 4 +- tests/qemucaps2xmltest.c | 2 - 31 files changed, 97 insertions(+), 191 deletions(-)
ACK Michal
participants (5)
-
Andrea Bolognani -
Daniel P. Berrangé -
Ján Tomko -
Michal Privoznik -
Peter Krempa