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

2015-06-08 Thread 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



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-06-08 Thread Daniel P. Berrange 
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 :-)

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-06-06 Thread Cole Robinson 
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.

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2015-05-28 Thread Daniel P. Berrange 
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.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list