[PATCH 0/4] python/qemu: New accel module and improvements

[PATCH 0/4] python/qemu: New accel module and improvements

[Wainer dos Santos Moschetta]

On commit abf0bf998dcb John Snow moved some code out of __init__.py
to machine.py. kvm_available() remained in though. So on patch 01
I continue his work by creating a home for that method (the new
'accel' module). Honestly I was unsure about whether move the code
to any existing module or make a new, but since I am adding more
methods related with accelerators then I thought they would 
deserve a module.

The patches 02-04 introduce new helpers and make improvements. Later
I intend to use those methods on the acceptance tests such as
to automatically set the accelerator in QEMUMachine VM via Avocado
tags, and skip the test if the accelerator is not available.

Git:
 - Tree: https://github.com/wainersm/qemu
 - Branch: python_accel

CI:
 - Travis (FAIL): https://travis-ci.org/wainersm/qemu/builds/612382935
   Non-related fail due build timeout

Wainer dos Santos Moschetta (4):
  python/qemu: Move kvm_available() to its own module
  python/qemu: accel: Add list_accel() method
  python/qemu: accel: Strengthen kvm_available() checks
  python/qemu: accel: Add tcg_available() method

 python/qemu/__init__.py | 20 +----------
 python/qemu/accel.py    | 79 +++++++++++++++++++++++++++++++++++++++++
 tests/vm/basevm.py      |  2 +-
 3 files changed, 81 insertions(+), 20 deletions(-)
 create mode 100644 python/qemu/accel.py

-- 
2.18.1

[PATCH 1/4] python/qemu: Move kvm_available() to its own module

[Wainer dos Santos Moschetta]

This creates the 'accel' Python module to be the home for
utilities that deal with accelerators. Also moved kvm_available()
from __init__.py to this new module.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 python/qemu/__init__.py | 20 +-------------------
 python/qemu/accel.py    | 36 ++++++++++++++++++++++++++++++++++++
 tests/vm/basevm.py      |  2 +-
 3 files changed, 38 insertions(+), 20 deletions(-)
 create mode 100644 python/qemu/accel.py

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
index 6c919a3d56..eff17a306e 100644
--- a/python/qemu/__init__.py
+++ b/python/qemu/__init__.py
@@ -12,24 +12,6 @@
 # Based on qmp.py.
 #
 
-import logging
-import os
-
 from . import qmp
 from . import machine
-
-LOG = logging.getLogger(__name__)
-
-# Mapping host architecture to any additional architectures it can
-# support which often includes its 32 bit cousin.
-ADDITIONAL_ARCHES = {
-    "x86_64" : "i386",
-    "aarch64" : "armhf"
-}
-
-def kvm_available(target_arch=None):
-    host_arch = os.uname()[4]
-    if target_arch and target_arch != host_arch:
-        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
-            return False
-    return os.access("/dev/kvm", os.R_OK | os.W_OK)
+from . import accel
diff --git a/python/qemu/accel.py b/python/qemu/accel.py
new file mode 100644
index 0000000000..1b825126c5
--- /dev/null
+++ b/python/qemu/accel.py
@@ -0,0 +1,36 @@
+"""
+QEMU accel module:
+
+This module provides utilities for discover and check the availability of
+accelerators.
+"""
+# Copyright (C) 2015-2016 Red Hat Inc.
+# Copyright (C) 2012 IBM Corp.
+#
+# Authors:
+#  Fam Zheng <famz@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.  See
+# the COPYING file in the top-level directory.
+#
+# Based on qmp.py.
+#
+
+import logging
+import os
+
+LOG = logging.getLogger(__name__)
+
+# Mapping host architecture to any additional architectures it can
+# support which often includes its 32 bit cousin.
+ADDITIONAL_ARCHES = {
+    "x86_64" : "i386",
+    "aarch64" : "armhf"
+}
+
+def kvm_available(target_arch=None):
+    host_arch = os.uname()[4]
+    if target_arch and target_arch != host_arch:
+        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
+            return False
+    return os.access("/dev/kvm", os.R_OK | os.W_OK)
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 91a9226026..3e2b69c96c 100755
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -21,7 +21,7 @@ import logging
 import time
 import datetime
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu import kvm_available
+from qemu.accel import kvm_available
 from qemu.machine import QEMUMachine
 import subprocess
 import hashlib
-- 
2.18.1

[PATCH 2/4] python/qemu: accel: Add list_accel() method

[Wainer dos Santos Moschetta]

Since commit cbe6d6365a48 the command `qemu -accel help` returns
the list of accelerators enabled in the QEMU binary. This adds
the list_accel() method which return that same list.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 python/qemu/accel.py | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/python/qemu/accel.py b/python/qemu/accel.py
index 1b825126c5..a63ff980a9 100644
--- a/python/qemu/accel.py
+++ b/python/qemu/accel.py
@@ -18,6 +18,7 @@ accelerators.
 
 import logging
 import os
+import subprocess
 
 LOG = logging.getLogger(__name__)
 
@@ -28,6 +29,25 @@ ADDITIONAL_ARCHES = {
     "aarch64" : "armhf"
 }
 
+def list_accel(qemu_bin):
+    """
+    List accelerators enabled in the QEMU binary.
+
+    @param qemu_bin (str): path to the QEMU binary.
+    @raise Exception: if failed to run `qemu -accel help`
+    @return a list of accelerator names.
+    """
+    if not qemu_bin:
+        return []
+    try:
+        out = subprocess.check_output("%s -accel help" % qemu_bin, shell=True)
+    except:
+        LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin)
+        raise
+    lines = out.decode().splitlines()
+    # Skip the first line which is the header.
+    return [l.strip() for l in lines[1:] if l]
+
 def kvm_available(target_arch=None):
     host_arch = os.uname()[4]
     if target_arch and target_arch != host_arch:
-- 
2.18.1

[PATCH 3/4] python/qemu: accel: Strengthen kvm_available() checks

[Wainer dos Santos Moschetta]

Currently kvm_available() checks for the presence of kvm module
and, if target and host arches don't mismatch. This patch adds
an 3rd checking: if QEMU binary was compiled with kvm
support.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 python/qemu/accel.py | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/python/qemu/accel.py b/python/qemu/accel.py
index a63ff980a9..1978fbee4e 100644
--- a/python/qemu/accel.py
+++ b/python/qemu/accel.py
@@ -48,9 +48,24 @@ def list_accel(qemu_bin):
     # Skip the first line which is the header.
     return [l.strip() for l in lines[1:] if l]
 
-def kvm_available(target_arch=None):
-    host_arch = os.uname()[4]
-    if target_arch and target_arch != host_arch:
-        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
-            return False
-    return os.access("/dev/kvm", os.R_OK | os.W_OK)
+def kvm_available(target_arch=None, qemu_bin=None):
+    """
+    Check if KVM is available using the following heuristic:
+      - Kernel module is present in the host;
+      - Target and host arches don't mismatch;
+      - KVM is enabled in the QEMU binary.
+
+    @param target_arch (str): target architecture
+    @param qemu_bin (str): path to the QEMU binary
+    @return True if kvm is available, otherwise False.
+    """
+    if not os.access("/dev/kvm", os.R_OK | os.W_OK):
+        return False
+    if target_arch:
+        host_arch = os.uname()[4]
+        if target_arch != host_arch:
+            if target_arch != ADDITIONAL_ARCHES.get(host_arch):
+                return False
+    if qemu_bin and "kvm" not in list_accel(qemu_bin):
+        return False
+    return True
-- 
2.18.1

[PATCH 4/4] python/qemu: accel: Add tcg_available() method

[Wainer dos Santos Moschetta]

This adds a method to check if the tcg accelerator is enabled
in the QEMU binary.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 python/qemu/accel.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/python/qemu/accel.py b/python/qemu/accel.py
index 1978fbee4e..513904d46d 100644
--- a/python/qemu/accel.py
+++ b/python/qemu/accel.py
@@ -69,3 +69,11 @@ def kvm_available(target_arch=None, qemu_bin=None):
     if qemu_bin and "kvm" not in list_accel(qemu_bin):
         return False
     return True
+
+def tcg_available(qemu_bin):
+    """
+    Check if TCG is available.
+
+    @param qemu_bin (str): path to the QEMU binary
+    """
+    return 'tcg' in list_accel(qemu_bin)
-- 
2.18.1

Re: [PATCH 1/4] python/qemu: Move kvm_available() to its own module

[Alex Bennée]

Wainer dos Santos Moschetta <wainersm@redhat.com> writes:

> This creates the 'accel' Python module to be the home for
> utilities that deal with accelerators. Also moved kvm_available()
> from __init__.py to this new module.
>
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  python/qemu/__init__.py | 20 +-------------------
>  python/qemu/accel.py    | 36 ++++++++++++++++++++++++++++++++++++
>  tests/vm/basevm.py      |  2 +-
>  3 files changed, 38 insertions(+), 20 deletions(-)
>  create mode 100644 python/qemu/accel.py
>
> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> index 6c919a3d56..eff17a306e 100644
> --- a/python/qemu/__init__.py
> +++ b/python/qemu/__init__.py
> @@ -12,24 +12,6 @@
>  # Based on qmp.py.
>  #
>  
> -import logging
> -import os
> -
>  from . import qmp
>  from . import machine
> -
> -LOG = logging.getLogger(__name__)
> -
> -# Mapping host architecture to any additional architectures it can
> -# support which often includes its 32 bit cousin.
> -ADDITIONAL_ARCHES = {
> -    "x86_64" : "i386",
> -    "aarch64" : "armhf"
> -}
> -
> -def kvm_available(target_arch=None):
> -    host_arch = os.uname()[4]
> -    if target_arch and target_arch != host_arch:
> -        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
> -            return False
> -    return os.access("/dev/kvm", os.R_OK | os.W_OK)
> +from . import accel
> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> new file mode 100644
> index 0000000000..1b825126c5
> --- /dev/null
> +++ b/python/qemu/accel.py
> @@ -0,0 +1,36 @@
> +"""
> +QEMU accel module:
> +
> +This module provides utilities for discover and check the availability of
> +accelerators.
> +"""
> +# Copyright (C) 2015-2016 Red Hat Inc.
> +# Copyright (C) 2012 IBM Corp.
> +#
> +# Authors:
> +#  Fam Zheng <famz@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.  See
> +# the COPYING file in the top-level directory.
> +#
> +# Based on qmp.py.

Don't think you need this - it's very much not based on qmp.py (also
quite small).

> +#
> +
> +import logging
> +import os
> +
> +LOG = logging.getLogger(__name__)

I don't think we use logging at all so you can drop the import and LOG
lines.

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> +
> +# Mapping host architecture to any additional architectures it can
> +# support which often includes its 32 bit cousin.
> +ADDITIONAL_ARCHES = {
> +    "x86_64" : "i386",
> +    "aarch64" : "armhf"
> +}
> +
> +def kvm_available(target_arch=None):
> +    host_arch = os.uname()[4]
> +    if target_arch and target_arch != host_arch:
> +        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
> +            return False
> +    return os.access("/dev/kvm", os.R_OK | os.W_OK)
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index 91a9226026..3e2b69c96c 100755
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -21,7 +21,7 @@ import logging
>  import time
>  import datetime
>  sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> -from qemu import kvm_available
> +from qemu.accel import kvm_available
>  from qemu.machine import QEMUMachine
>  import subprocess
>  import hashlib


-- 
Alex Bennée

Re: [PATCH 2/4] python/qemu: accel: Add list_accel() method

[Alex Bennée]

Wainer dos Santos Moschetta <wainersm@redhat.com> writes:

> Since commit cbe6d6365a48 the command `qemu -accel help` returns
> the list of accelerators enabled in the QEMU binary. This adds
> the list_accel() method which return that same list.
>
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  python/qemu/accel.py | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> index 1b825126c5..a63ff980a9 100644
> --- a/python/qemu/accel.py
> +++ b/python/qemu/accel.py
> @@ -18,6 +18,7 @@ accelerators.
>  
>  import logging
>  import os
> +import subprocess
>  
>  LOG = logging.getLogger(__name__)
>  
> @@ -28,6 +29,25 @@ ADDITIONAL_ARCHES = {
>      "aarch64" : "armhf"
>  }
>  
> +def list_accel(qemu_bin):
> +    """
> +    List accelerators enabled in the QEMU binary.
> +
> +    @param qemu_bin (str): path to the QEMU binary.
> +    @raise Exception: if failed to run `qemu -accel help`
> +    @return a list of accelerator names.
> +    """
> +    if not qemu_bin:
> +        return []
> +    try:
> +        out = subprocess.check_output("%s -accel help" % qemu_bin, shell=True)
> +    except:
> +        LOG.debug("Failed to get the list of accelerators in %s" %
> qemu_bin)

Ahh here it is. So I guess either mention the fact we will be using it
in the previous commit or bring it in here when we do use it.

I see we use LOG.debug for a bunch of failure reasons but surely
LOG.error is the more correct level for something that failed? I guess
it doesn't matter as we don't mess with the levels.

> +        raise
> +    lines = out.decode().splitlines()
> +    # Skip the first line which is the header.
> +    return [l.strip() for l in lines[1:] if l]
> +
>  def kvm_available(target_arch=None):
>      host_arch = os.uname()[4]
>      if target_arch and target_arch != host_arch:

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


-- 
Alex Bennée

Re: [PATCH 3/4] python/qemu: accel: Strengthen kvm_available() checks

[Alex Bennée]

Wainer dos Santos Moschetta <wainersm@redhat.com> writes:

> Currently kvm_available() checks for the presence of kvm module
> and, if target and host arches don't mismatch. This patch adds
> an 3rd checking: if QEMU binary was compiled with kvm
> support.
>
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  python/qemu/accel.py | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> index a63ff980a9..1978fbee4e 100644
> --- a/python/qemu/accel.py
> +++ b/python/qemu/accel.py
> @@ -48,9 +48,24 @@ def list_accel(qemu_bin):
>      # Skip the first line which is the header.
>      return [l.strip() for l in lines[1:] if l]
>  
> -def kvm_available(target_arch=None):
> -    host_arch = os.uname()[4]
> -    if target_arch and target_arch != host_arch:
> -        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
> -            return False
> -    return os.access("/dev/kvm", os.R_OK | os.W_OK)
> +def kvm_available(target_arch=None, qemu_bin=None):
> +    """
> +    Check if KVM is available using the following heuristic:
> +      - Kernel module is present in the host;
> +      - Target and host arches don't mismatch;
> +      - KVM is enabled in the QEMU binary.
> +
> +    @param target_arch (str): target architecture
> +    @param qemu_bin (str): path to the QEMU binary
> +    @return True if kvm is available, otherwise False.
> +    """
> +    if not os.access("/dev/kvm", os.R_OK | os.W_OK):
> +        return False
> +    if target_arch:
> +        host_arch = os.uname()[4]
> +        if target_arch != host_arch:
> +            if target_arch != ADDITIONAL_ARCHES.get(host_arch):
> +                return False
> +    if qemu_bin and "kvm" not in list_accel(qemu_bin):
> +        return False
> +    return True


-- 
Alex Bennée

Re: [PATCH 4/4] python/qemu: accel: Add tcg_available() method

[Alex Bennée]

Wainer dos Santos Moschetta <wainersm@redhat.com> writes:

> This adds a method to check if the tcg accelerator is enabled
> in the QEMU binary.
>
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

So is this series going to be combined with another avocado series?

> ---
>  python/qemu/accel.py | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> index 1978fbee4e..513904d46d 100644
> --- a/python/qemu/accel.py
> +++ b/python/qemu/accel.py
> @@ -69,3 +69,11 @@ def kvm_available(target_arch=None, qemu_bin=None):
>      if qemu_bin and "kvm" not in list_accel(qemu_bin):
>          return False
>      return True
> +
> +def tcg_available(qemu_bin):
> +    """
> +    Check if TCG is available.
> +
> +    @param qemu_bin (str): path to the QEMU binary
> +    """
> +    return 'tcg' in list_accel(qemu_bin)


-- 
Alex Bennée

Re: [PATCH 0/4] python/qemu: New accel module and improvements

[Philippe Mathieu-Daudé]

On 11/15/19 7:08 PM, Wainer dos Santos Moschetta wrote:
> On commit abf0bf998dcb John Snow moved some code out of __init__.py
> to machine.py. kvm_available() remained in though. So on patch 01
> I continue his work by creating a home for that method (the new
> 'accel' module). Honestly I was unsure about whether move the code
> to any existing module or make a new, but since I am adding more
> methods related with accelerators then I thought they would
> deserve a module.
> 
> The patches 02-04 introduce new helpers and make improvements. Later
> I intend to use those methods on the acceptance tests such as
> to automatically set the accelerator in QEMUMachine VM via Avocado
> tags, and skip the test if the accelerator is not available.
> 
> Git:
>   - Tree: https://github.com/wainersm/qemu
>   - Branch: python_accel
> 
> CI:
>   - Travis (FAIL): https://travis-ci.org/wainersm/qemu/builds/612382935
>     Non-related fail due build timeout
> 
> Wainer dos Santos Moschetta (4):
>    python/qemu: Move kvm_available() to its own module
>    python/qemu: accel: Add list_accel() method
>    python/qemu: accel: Strengthen kvm_available() checks
>    python/qemu: accel: Add tcg_available() method

No more comment that what Alex said, I'd move the logging import in 
patch #2 where you use LOG.
With Alex comment fixed:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>