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

[Cole Robinson]

On 06/08/2015 04:46 AM, Daniel P. Berrange wrote:
> 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 :-)
> 

Good point, I squashed in this and pushed:

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 50361fd..ca7a7c2 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -769,6 +769,14 @@ virQEMUCapsInitGuest(virCapsPtr caps,
             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 (arm_32on64_kvm)
             kvmbins[3] = "qemu-system-aarch64";

Thanks,
Cole