Re: [libvirt] [PATCH 3/3] qemu: caps: Advertise arm 32-on-64 KVM option

On Sat, Jun 06, 2015 at 07:06:42PM -0400, Cole Robinson wrote: > On 05/28/2015 07:31 AM, Daniel P. Berrange wrote: >> On Thu, May 21, 2015 at 07:03:28PM -0400, Cole Robinson wrote: >>> We need to use qemu-system-aarch64 to run armv7l KVM VMs on an aarch64 >>> host. >>> --- >>> src/qemu/qemu_capabilities.c | 42 ++++++++++++++++++++++-------------------- >>> 1 file changed, 22 insertions(+), 20 deletions(-) >>> >>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c >>> index 1e7bddb..7181865 100644 >>> --- a/src/qemu/qemu_capabilities.c >>> +++ b/src/qemu/qemu_capabilities.c >>> @@ -723,19 +723,6 @@ virQEMUCapsFindBinaryForArch(virArch hostarch, >>> return ret; >>> } >>> >>> - >>> -static bool >>> -virQEMUCapsIsValidForKVM(virArch hostarch, >>> - virArch guestarch) >>> -{ >>> - if (hostarch == guestarch) >>> - return true; >>> - if (hostarch == VIR_ARCH_X86_64 && >>> - guestarch == VIR_ARCH_I686) >>> - return true; >>> - return false; >>> -} >>> - >>> static int >>> virQEMUCapsInitGuest(virCapsPtr caps, >>> virQEMUCapsCachePtr cache, >>> @@ -747,6 +734,7 @@ virQEMUCapsInitGuest(virCapsPtr caps, >>> char *binary = NULL; >>> virQEMUCapsPtr qemubinCaps = NULL; >>> virQEMUCapsPtr kvmbinCaps = NULL; >>> + bool native_kvm, x86_32on64_kvm, arm_32on64_kvm; >>> int ret = -1; >>> >>> /* Check for existence of base emulator, or alternate base >>> @@ -764,16 +752,30 @@ virQEMUCapsInitGuest(virCapsPtr caps, >>> >>> /* qemu-kvm/kvm binaries can only be used if >>> * - host & guest arches match >>> - * Or >>> - * - hostarch is x86_64 and guest arch is i686 >>> - * The latter simply needs "-cpu qemu32" >>> + * - hostarch is x86_64 and guest arch is i686 (needs -cpu qemu32) >>> + * - hostarch is aarch64 and guest arch is armv7l (needs -cpu aarch64=off) >>> */ >>> - if (virQEMUCapsIsValidForKVM(hostarch, guestarch)) { >>> - const char *const kvmbins[] = { "/usr/libexec/qemu-kvm", /* RHEL */ >>> - "qemu-kvm", /* Fedora */ >>> - "kvm" }; /* Upstream .spec */ >>> + native_kvm = (hostarch == guestarch); >>> + x86_32on64_kvm = (hostarch == VIR_ARCH_X86_64 && >>> + guestarch == VIR_ARCH_I686); >>> + arm_32on64_kvm = (hostarch == VIR_ARCH_AARCH64 && >>> + guestarch == VIR_ARCH_ARMV7L); >>> + >>> + if (native_kvm || x86_32on64_kvm || arm_32on64_kvm) { >>> + const char *kvmbins[] = { >>> + "/usr/libexec/qemu-kvm", /* RHEL */ >>> + "qemu-kvm", /* Fedora */ >>> + "kvm", /* Debian/Ubuntu */ >>> + NULL, >>> + }; >>> + >>> + if (arm_32on64_kvm) >>> + kvmbins[3] = "qemu-system-aarch64"; >> >> I'm unclear why you need to be adding this. We don't need it for >> the equivalent i686 with qemu-system-x86_64, as the earlier call >> to virQEMUCapsFindBinaryForArch() will already return the binary >> qemu-system-x86_64. IIUC, it should have returned the binary >> qemu-system-aarch64 too, so this just seems to duplicate the >> check for that binary. > > We need this in the case you are running on an aarch64 host and have both > qemu-system-arm and qemu-system-aarch64 installed. In this case, when you want > to use KVM for arm32, you _have_ to use qemu-system-aarch64, qemu-system-arm > does not work. What you suggest would mean that qemu-system-arm is grabbed > from the caps cache. > > x86 doesn't have this problem because qemu-system-i386, qemu-system-x86_64 and > by extension qemu-kvm can all be used to do 32-on-64 KVM. Ok, I think we need to have this explained in the code comments, because that is none obvious from reading the code & we don't want to be wondering why we did this when looking at the code again in a year :-)