[PATCH v3 0/4] Acceptance tests: boot Linux with KVM test

[PATCH v3 0/4] Acceptance tests: boot Linux with KVM test

[Wainer dos Santos Moschetta]

This adds boot Linux tests for x86_64, aarch64, ppc64, and s390x
targets which, unlike others, enable the KVM acceleration. Likewise
it was added test cases for tcg.

It is introduced an infraestructure on avocado_qemu framework
so that:
a) simply tagging the test with `accel:kvm` (or `accel:tcg`) will
automatically set the corresponding '-accel' on the launched
QEMU;
b) test is canceled if the accelerator is not enabled on the QEMU
binary or not available in the host.

The acceptance builder on Travis was changed too in order to make
the test run.

Changes v2 -> v3:
- Uses '-accel kvm' rather than '-enable-kvm' when automatically setting
  the accelerator [thuth]
- Added patch 04 which enable the KVM acceptance tests to run on
  Travis [thuth]
v2: [PATCH v2 0/3] Acceptance tests: boot Linux with KVM test
- https://www.mail-archive.com/qemu-devel@nongnu.org/msg666238.html
v1: [PATCH 0/3] Acceptance tests: boot Linux with KVM test
- https://www.mail-archive.com/qemu-devel@nongnu.org/msg627498.html

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

CI:
- Travis (PASS): https://travis-ci.org/wainersm/qemu/builds/640172969

Wainer dos Santos Moschetta (4):
  tests/acceptance: avocado_qemu: Introduce the 'accel' test parameter
  tests/acceptance: boot_linux_console: Add boot Linux with kvm tests
  tests/acceptance: avocado_qemu: Refactor the handler of 'machine'
    parameter
  travis.yml: Enable acceptance KVM tests

 .travis.yml                               |  7 +-
 docs/devel/testing.rst                    | 16 +++++
 tests/acceptance/avocado_qemu/__init__.py | 27 ++++++-
 tests/acceptance/boot_linux_console.py    | 88 +++++++++++++++++------
 4 files changed, 115 insertions(+), 23 deletions(-)

-- 
2.23.0

[PATCH v3 1/4] tests/acceptance: avocado_qemu: Introduce the 'accel' test parameter

[Wainer dos Santos Moschetta]

The test case may need to boot the VM with an accelerator that
isn't actually enabled on the QEMU binary and/or present in the host. In
this case the test behavior is undefined, and the best course of
action is to skip its execution.

This change introduced the 'accel' parameter (and the handler of
tag with same name) used to indicate the test case requires a
given accelerator available. It was implemented a mechanism to
skip the test case if the accelerator is not available. Moreover,
 the QEMU -accel argument is set automatically to any VM
launched if the parameter is present.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 docs/devel/testing.rst                    | 16 ++++++++++++++++
 tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index ab5be0c729..d17d0e90aa 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -759,6 +759,17 @@ name.  If one is not given explicitly, it will either be set to
 ``None``, or, if the test is tagged with one (and only one)
 ``:avocado: tags=machine:VALUE`` tag, it will be set to ``VALUE``.
 
+accel
+~~~~~
+The accelerator that will be set to all QEMUMachine instances created
+by the test.
+
+The ``accel`` attribute will be set to the test parameter of the same
+name.  If one is not given explicitly, it will either be set to
+``None``, or, if the test is tagged with one (and only one)
+``:avocado: tags=accel:VALUE`` tag, it will be set to ``VALUE``. Currently
+``VALUE`` should be either ``kvm`` or ``tcg``.
+
 qemu_bin
 ~~~~~~~~
 
@@ -800,6 +811,11 @@ machine
 The machine type that will be set to all QEMUMachine instances created
 by the test.
 
+accel
+~~~~~
+The accelerator that will be set to all QEMUMachine instances created
+by the test. In case the accelerator is not available (both QEMU
+binary and the host system are checked) then the test is canceled.
 
 qemu_bin
 ~~~~~~~~
diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 6618ea67c1..c83a75ccbc 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -20,6 +20,7 @@ SRC_ROOT_DIR = os.path.join(os.path.dirname(__file__), '..', '..', '..')
 sys.path.append(os.path.join(SRC_ROOT_DIR, 'python'))
 
 from qemu.machine import QEMUMachine
+from qemu.accel import kvm_available, tcg_available
 
 def is_readable_executable_file(path):
     return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
@@ -111,6 +112,8 @@ class Test(avocado.Test):
 
     def setUp(self):
         self._vms = {}
+        # VM argumments that are mapped from parameters
+        self._param_to_vm_args = []
 
         self.arch = self.params.get('arch',
                                     default=self._get_unique_tag_val('arch'))
@@ -124,10 +127,30 @@ class Test(avocado.Test):
         if self.qemu_bin is None:
             self.cancel("No QEMU binary defined or found in the source tree")
 
+        self.accel = self.params.get('accel',
+                                     default=self._get_unique_tag_val('accel'))
+        if self.accel:
+            avail = False
+            if self.accel == 'kvm':
+                if kvm_available(self.arch, self.qemu_bin):
+                    avail = True
+            elif self.accel == 'tcg':
+                if tcg_available(self.qemu_bin):
+                    avail = True
+            else:
+                self.cancel("Unknown accelerator: %s" % self.accel)
+
+            if avail:
+                self._param_to_vm_args.extend(['-accel', self.accel])
+            else:
+                self.cancel("%s is not available" % self.accel)
+
     def _new_vm(self, *args):
         vm = QEMUMachine(self.qemu_bin, sock_dir=tempfile.mkdtemp())
         if args:
             vm.add_args(*args)
+        if self._param_to_vm_args:
+            vm.add_args(*self._param_to_vm_args)
         return vm
 
     @property
-- 
2.23.0

[PATCH v3 2/4] tests/acceptance: boot_linux_console: Add boot Linux with kvm tests

[Wainer dos Santos Moschetta]

Added boot Linux test cases that launch QEMU with kvm
enabled. Likewise it was added tests for tcg.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 tests/acceptance/boot_linux_console.py | 88 ++++++++++++++++++++------
 1 file changed, 68 insertions(+), 20 deletions(-)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index e40b84651b..a36eae630c 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -51,11 +51,7 @@ class BootLinuxConsole(Test):
         os.chdir(cwd)
         return self.workdir + path
 
-    def test_x86_64_pc(self):
-        """
-        :avocado: tags=arch:x86_64
-        :avocado: tags=machine:pc
-        """
+    def do_test_x86_64_pc(self):
         kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
                       '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
                       '/vmlinuz')
@@ -70,6 +66,22 @@ class BootLinuxConsole(Test):
         console_pattern = 'Kernel command line: %s' % kernel_command_line
         self.wait_for_console_pattern(console_pattern)
 
+    def test_x86_64_pc_kvm(self):
+        """
+        :avocado: tags=arch:x86_64
+        :avocado: tags=machine:pc
+        :avocado: tags=accel:kvm
+        """
+        self.do_test_x86_64_pc()
+
+    def test_x86_64_pc_tcg(self):
+        """
+        :avocado: tags=arch:x86_64
+        :avocado: tags=machine:pc
+        :avocado: tags=accel:tcg
+        """
+        self.do_test_x86_64_pc()
+
     def test_mips_malta(self):
         """
         :avocado: tags=arch:mips
@@ -258,11 +270,7 @@ class BootLinuxConsole(Test):
         kernel_hash = '18d1c68f2e23429e266ca39ba5349ccd0aeb7180'
         self.do_test_mips_malta32el_nanomips(kernel_url, kernel_hash)
 
-    def test_aarch64_virt(self):
-        """
-        :avocado: tags=arch:aarch64
-        :avocado: tags=machine:virt
-        """
+    def do_test_aarch64_virt(self):
         kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
                       '/linux/releases/29/Everything/aarch64/os/images/pxeboot'
                       '/vmlinuz')
@@ -279,6 +287,22 @@ class BootLinuxConsole(Test):
         console_pattern = 'Kernel command line: %s' % kernel_command_line
         self.wait_for_console_pattern(console_pattern)
 
+    def test_aarch64_virt_kvm(self):
+        """
+        :avocado: tags=arch:aarch64
+        :avocado: tags=machine:virt
+        :avocado: tags=accel:kvm
+        """
+        self.do_test_aarch64_virt()
+
+    def test_aarch64_virt_tcg(self):
+        """
+        :avocado: tags=arch:aarch64
+        :avocado: tags=machine:virt
+        :avocado: tags=accel:tcg
+        """
+        self.do_test_aarch64_virt()
+
     def test_arm_virt(self):
         """
         :avocado: tags=arch:arm
@@ -485,11 +509,7 @@ class BootLinuxConsole(Test):
         exec_command_and_wait_for_pattern(self, 'reboot',
                                                 'reboot: Restarting system')
 
-    def test_s390x_s390_ccw_virtio(self):
-        """
-        :avocado: tags=arch:s390x
-        :avocado: tags=machine:s390-ccw-virtio
-        """
+    def do_test_s390x_s390_ccw_virtio(self):
         kernel_url = ('https://archives.fedoraproject.org/pub/archive'
                       '/fedora-secondary/releases/29/Everything/s390x/os/images'
                       '/kernel.img')
@@ -505,6 +525,22 @@ class BootLinuxConsole(Test):
         console_pattern = 'Kernel command line: %s' % kernel_command_line
         self.wait_for_console_pattern(console_pattern)
 
+    def test_s390x_s390_ccw_virtio_kvm(self):
+        """
+        :avocado: tags=arch:s390x
+        :avocado: tags=machine:s390-ccw-virtio
+        :avocado: tags=accel:kvm
+        """
+        self.do_test_s390x_s390_ccw_virtio()
+
+    def test_s390x_s390_ccw_virtio_tcg(self):
+        """
+        :avocado: tags=arch:s390x
+        :avocado: tags=machine:s390-ccw-virtio
+        :avocado: tags=accel:tcg
+        """
+        self.do_test_s390x_s390_ccw_virtio()
+
     def test_alpha_clipper(self):
         """
         :avocado: tags=arch:alpha
@@ -526,11 +562,7 @@ class BootLinuxConsole(Test):
         console_pattern = 'Kernel command line: %s' % kernel_command_line
         self.wait_for_console_pattern(console_pattern)
 
-    def test_ppc64_pseries(self):
-        """
-        :avocado: tags=arch:ppc64
-        :avocado: tags=machine:pseries
-        """
+    def do_test_ppc64_pseries(self):
         kernel_url = ('https://archives.fedoraproject.org/pub/archive'
                       '/fedora-secondary/releases/29/Everything/ppc64le/os'
                       '/ppc/ppc64/vmlinuz')
@@ -545,6 +577,22 @@ class BootLinuxConsole(Test):
         console_pattern = 'Kernel command line: %s' % kernel_command_line
         self.wait_for_console_pattern(console_pattern)
 
+    def test_ppc64le_pseries_kvm(self):
+        """
+        :avocado: tags=arch:ppc64
+        :avocado: tags=machine:pseries
+        :avocado: tags=accel:kvm
+        """
+        self.do_test_ppc64_pseries()
+
+    def test_ppc64le_pseries_tcg(self):
+        """
+        :avocado: tags=arch:ppc64
+        :avocado: tags=machine:pseries
+        :avocado: tags=accel:tcg
+        """
+        self.do_test_ppc64_pseries()
+
     def test_m68k_q800(self):
         """
         :avocado: tags=arch:m68k
-- 
2.23.0

[PATCH v3 3/4] tests/acceptance: avocado_qemu: Refactor the handler of 'machine' parameter

[Wainer dos Santos Moschetta]

The Test._param_to_vm_args variable contain VM arguments that should be added
at launch which were originated from test parameters. Use this variable
to set -M from 'machine' parameter as well.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index c83a75ccbc..443ac02aff 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -120,6 +120,8 @@ class Test(avocado.Test):
 
         self.machine = self.params.get('machine',
                                        default=self._get_unique_tag_val('machine'))
+        if self.machine:
+            self._param_to_vm_args.extend(['-M', self.machine])
 
         default_qemu_bin = pick_default_qemu_bin(arch=self.arch)
         self.qemu_bin = self.params.get('qemu_bin',
@@ -162,8 +164,6 @@ class Test(avocado.Test):
             name = str(uuid.uuid4())
         if self._vms.get(name) is None:
             self._vms[name] = self._new_vm(*args)
-            if self.machine is not None:
-                self._vms[name].set_machine(self.machine)
         return self._vms[name]
 
     def tearDown(self):
-- 
2.23.0

[PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Wainer dos Santos Moschetta]

Some acceptance tests require KVM or they are skipped. Travis
enables nested virtualization by default with Ubuntu
18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
changed the acceptance builder to run in a Bionic VM. Also
it was needed to ensure the current user has rw permission
to /dev/kvm.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 .travis.yml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 6c1038a0f1..c3edd0a907 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -2,6 +2,7 @@
 # Additional builds with specific requirements for a full VM need to
 # be added as additional matrix: entries later on
 dist: xenial
+sudo: true
 language: c
 compiler:
   - gcc
@@ -83,6 +84,9 @@ git:
 
 before_script:
   - if command -v ccache ; then ccache --zero-stats ; fi
+  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
+        sudo chmod o+rw /dev/kvm ;
+    fi
   - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
   - ${SRC_DIR}/configure ${BASE_CONFIG} ${CONFIG} || { cat config.log && exit 1; }
 script:
@@ -272,12 +276,13 @@ matrix:
         - TEST_CMD="make check-acceptance"
       after_script:
         - python3 -c 'import json; r = json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) for t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
+      dist: bionic
       addons:
         apt:
           packages:
             - python3-pil
             - python3-pip
-            - python3.5-venv
+            - python3.6-venv
             - tesseract-ocr
             - tesseract-ocr-eng
 
-- 
2.23.0

Re: [PATCH v3 2/4] tests/acceptance: boot_linux_console: Add boot Linux with kvm tests

[Andrew Jones]

On Tue, Jan 21, 2020 at 10:27:51PM -0300, Wainer dos Santos Moschetta wrote:
> +    def test_aarch64_virt_kvm(self):
> +        """
> +        :avocado: tags=arch:aarch64
> +        :avocado: tags=machine:virt
> +        :avocado: tags=accel:kvm
> +        """
> +        self.do_test_aarch64_virt()
> +
> +    def test_aarch64_virt_tcg(self):
> +        """
> +        :avocado: tags=arch:aarch64
> +        :avocado: tags=machine:virt
> +        :avocado: tags=accel:tcg
> +        """
> +        self.do_test_aarch64_virt()
> +

Does do_test_aarch64_virt() add more machine parameters? Also, which cpu
type does it choose? The reason I ask is because aarch64 virt will fail to
run with KVM unless the appropriate gic version is specified (the
gic-version machine parameter). Also the cpu type must be 'host' or 'max'.
'max' is the better choice as it also works for tcg. gic-version also
takes 'max' allowing it to auto-select the appropriate version. So if it's
not already there somewhere, then please ensure aarch64 has these
additional parameters:

 machine:gic-version=max
 cpu:max

Thanks,
drew

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Thomas Huth]

On 22/01/2020 02.27, Wainer dos Santos Moschetta wrote:
> Some acceptance tests require KVM or they are skipped. Travis
> enables nested virtualization by default with Ubuntu
> 18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
> changed the acceptance builder to run in a Bionic VM. Also
> it was needed to ensure the current user has rw permission
> to /dev/kvm.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  .travis.yml | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 6c1038a0f1..c3edd0a907 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -2,6 +2,7 @@
>  # Additional builds with specific requirements for a full VM need to
>  # be added as additional matrix: entries later on
>  dist: xenial
> +sudo: true
>  language: c
>  compiler:
>    - gcc
> @@ -83,6 +84,9 @@ git:
>  
>  before_script:
>    - if command -v ccache ; then ccache --zero-stats ; fi
> +  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
> +        sudo chmod o+rw /dev/kvm ;
> +    fi
>    - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
>    - ${SRC_DIR}/configure ${BASE_CONFIG} ${CONFIG} || { cat config.log && exit 1; }
>  script:
> @@ -272,12 +276,13 @@ matrix:
>          - TEST_CMD="make check-acceptance"
>        after_script:
>          - python3 -c 'import json; r = json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) for t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
> +      dist: bionic
>        addons:
>          apt:
>            packages:
>              - python3-pil
>              - python3-pip
> -            - python3.5-venv
> +            - python3.6-venv
>              - tesseract-ocr
>              - tesseract-ocr-eng

I'm surprised that the chmod is sufficient enough here (since I was
having trouble with that in the kvm-unit-tests), but it seems to work,
indeed:

 https://travis-ci.com/huth/qemu/jobs/278226646#L3762

So:
Tested-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Thomas Huth]

On 22/01/2020 02.27, Wainer dos Santos Moschetta wrote:
> Some acceptance tests require KVM or they are skipped. Travis
> enables nested virtualization by default with Ubuntu
> 18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
> changed the acceptance builder to run in a Bionic VM. Also
> it was needed to ensure the current user has rw permission
> to /dev/kvm.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  .travis.yml | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 6c1038a0f1..c3edd0a907 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -2,6 +2,7 @@
>  # Additional builds with specific requirements for a full VM need to
>  # be added as additional matrix: entries later on
>  dist: xenial
> +sudo: true
>  language: c
>  compiler:
>    - gcc
> @@ -83,6 +84,9 @@ git:
>  
>  before_script:
>    - if command -v ccache ; then ccache --zero-stats ; fi
> +  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then

By the way, in case you respin, could you please use singel "[" instead
of "[[" ... since that's what we use in almost all other shell scripts, too.

 Thomas

Re: [PATCH v3 2/4] tests/acceptance: boot_linux_console: Add boot Linux with kvm tests

[Wainer dos Santos Moschetta]

On 1/22/20 7:02 AM, Andrew Jones wrote:
> On Tue, Jan 21, 2020 at 10:27:51PM -0300, Wainer dos Santos Moschetta wrote:
>> +    def test_aarch64_virt_kvm(self):
>> +        """
>> +        :avocado: tags=arch:aarch64
>> +        :avocado: tags=machine:virt
>> +        :avocado: tags=accel:kvm
>> +        """
>> +        self.do_test_aarch64_virt()
>> +
>> +    def test_aarch64_virt_tcg(self):
>> +        """
>> +        :avocado: tags=arch:aarch64
>> +        :avocado: tags=machine:virt
>> +        :avocado: tags=accel:tcg
>> +        """
>> +        self.do_test_aarch64_virt()
>> +
> Does do_test_aarch64_virt() add more machine parameters? Also, which cpu
> type does it choose? The reason I ask is because aarch64 virt will fail to
> run with KVM unless the appropriate gic version is specified (the
> gic-version machine parameter). Also the cpu type must be 'host' or 'max'.
> 'max' is the better choice as it also works for tcg. gic-version also
> takes 'max' allowing it to auto-select the appropriate version. So if it's
> not already there somewhere, then please ensure aarch64 has these
> additional parameters:
>
>   machine:gic-version=max
>   cpu:max


The test was passing '-cpu cortex-a56', I replaced with '-cpu max'. 
Also, now, it passes the gic version as you pointed out. I will send 
those changes on a v4.

Other than that, I tried '-cpu max -machine virt' (without gic-version) 
and QEMU crashed:

[root@virtlab-arm03 build]# ./aarch64-softmmu/qemu-system-aarch64 -accel 
kvm -cpu max -machine virt -display none -vga none
qemu-system-aarch64: PMU: KVM_SET_DEVICE_ATTR: Invalid argument
qemu-system-aarch64: failed to set irq for PMU
Aborted (core dumped)

----

Should I expect it to crash or rather nicely fail?

Thanks !

- Wainer


>
> Thanks,
> drew

Re: [PATCH v3 2/4] tests/acceptance: boot_linux_console: Add boot Linux with kvm tests

[Andrew Jones]

On Thu, Jan 23, 2020 at 07:47:19PM -0200, Wainer dos Santos Moschetta wrote:
> 
> On 1/22/20 7:02 AM, Andrew Jones wrote:
> > On Tue, Jan 21, 2020 at 10:27:51PM -0300, Wainer dos Santos Moschetta wrote:
> > > +    def test_aarch64_virt_kvm(self):
> > > +        """
> > > +        :avocado: tags=arch:aarch64
> > > +        :avocado: tags=machine:virt
> > > +        :avocado: tags=accel:kvm
> > > +        """
> > > +        self.do_test_aarch64_virt()
> > > +
> > > +    def test_aarch64_virt_tcg(self):
> > > +        """
> > > +        :avocado: tags=arch:aarch64
> > > +        :avocado: tags=machine:virt
> > > +        :avocado: tags=accel:tcg
> > > +        """
> > > +        self.do_test_aarch64_virt()
> > > +
> > Does do_test_aarch64_virt() add more machine parameters? Also, which cpu
> > type does it choose? The reason I ask is because aarch64 virt will fail to
> > run with KVM unless the appropriate gic version is specified (the
> > gic-version machine parameter). Also the cpu type must be 'host' or 'max'.
> > 'max' is the better choice as it also works for tcg. gic-version also
> > takes 'max' allowing it to auto-select the appropriate version. So if it's
> > not already there somewhere, then please ensure aarch64 has these
> > additional parameters:
> > 
> >   machine:gic-version=max
> >   cpu:max
> 
> 
> The test was passing '-cpu cortex-a56', I replaced with '-cpu max'. Also,
> now, it passes the gic version as you pointed out. I will send those changes
> on a v4.
> 
> Other than that, I tried '-cpu max -machine virt' (without gic-version) and
> QEMU crashed:
> 
> [root@virtlab-arm03 build]# ./aarch64-softmmu/qemu-system-aarch64 -accel kvm
> -cpu max -machine virt -display none -vga none
> qemu-system-aarch64: PMU: KVM_SET_DEVICE_ATTR: Invalid argument
> qemu-system-aarch64: failed to set irq for PMU
> Aborted (core dumped)
> 
> ----
> 
> Should I expect it to crash or rather nicely fail?

crash, unfortunately. I recall we once had a plan to send patches to
fail nicer, and to output a hint on how to resolve the issue, but I
guess that never happened...

Thanks,
drew

Re: [PATCH v3 1/4] tests/acceptance: avocado_qemu: Introduce the 'accel' test parameter

[Philippe Mathieu-Daudé]

On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
> The test case may need to boot the VM with an accelerator that
> isn't actually enabled on the QEMU binary and/or present in the host. In
> this case the test behavior is undefined, and the best course of
> action is to skip its execution.
> 
> This change introduced the 'accel' parameter (and the handler of
> tag with same name) used to indicate the test case requires a
> given accelerator available. It was implemented a mechanism to
> skip the test case if the accelerator is not available. Moreover,
>   the QEMU -accel argument is set automatically to any VM
> launched if the parameter is present.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>   docs/devel/testing.rst                    | 16 ++++++++++++++++
>   tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
>   2 files changed, 39 insertions(+)
> 
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index ab5be0c729..d17d0e90aa 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -759,6 +759,17 @@ name.  If one is not given explicitly, it will either be set to
>   ``None``, or, if the test is tagged with one (and only one)
>   ``:avocado: tags=machine:VALUE`` tag, it will be set to ``VALUE``.
>   
> +accel
> +~~~~~
> +The accelerator that will be set to all QEMUMachine instances created
> +by the test.
> +
> +The ``accel`` attribute will be set to the test parameter of the same
> +name.  If one is not given explicitly, it will either be set to
> +``None``, or, if the test is tagged with one (and only one)
> +``:avocado: tags=accel:VALUE`` tag, it will be set to ``VALUE``. Currently
> +``VALUE`` should be either ``kvm`` or ``tcg``.
> +
>   qemu_bin
>   ~~~~~~~~
>   
> @@ -800,6 +811,11 @@ machine
>   The machine type that will be set to all QEMUMachine instances created
>   by the test.
>   
> +accel
> +~~~~~
> +The accelerator that will be set to all QEMUMachine instances created
> +by the test. In case the accelerator is not available (both QEMU
> +binary and the host system are checked) then the test is canceled.
>   
>   qemu_bin
>   ~~~~~~~~
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 6618ea67c1..c83a75ccbc 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -20,6 +20,7 @@ SRC_ROOT_DIR = os.path.join(os.path.dirname(__file__), '..', '..', '..')
>   sys.path.append(os.path.join(SRC_ROOT_DIR, 'python'))
>   
>   from qemu.machine import QEMUMachine
> +from qemu.accel import kvm_available, tcg_available
>   
>   def is_readable_executable_file(path):
>       return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
> @@ -111,6 +112,8 @@ class Test(avocado.Test):
>   
>       def setUp(self):
>           self._vms = {}
> +        # VM argumments that are mapped from parameters
> +        self._param_to_vm_args = []
>   
>           self.arch = self.params.get('arch',
>                                       default=self._get_unique_tag_val('arch'))
> @@ -124,10 +127,30 @@ class Test(avocado.Test):
>           if self.qemu_bin is None:
>               self.cancel("No QEMU binary defined or found in the source tree")
>   
> +        self.accel = self.params.get('accel',
> +                                     default=self._get_unique_tag_val('accel'))
> +        if self.accel:
> +            avail = False
> +            if self.accel == 'kvm':
> +                if kvm_available(self.arch, self.qemu_bin):
> +                    avail = True
> +            elif self.accel == 'tcg':
> +                if tcg_available(self.qemu_bin):
> +                    avail = True
> +            else:
> +                self.cancel("Unknown accelerator: %s" % self.accel)
> +
> +            if avail:
> +                self._param_to_vm_args.extend(['-accel', self.accel])
> +            else:
> +                self.cancel("%s is not available" % self.accel)

Why refuse to test the other accelerators?

Isn't it better to QMP-ask QEMU which accelerator it supports, and SKIP 
if it isn't available?

> +
>       def _new_vm(self, *args):
>           vm = QEMUMachine(self.qemu_bin, sock_dir=tempfile.mkdtemp())
>           if args:
>               vm.add_args(*args)
> +        if self._param_to_vm_args:
> +            vm.add_args(*self._param_to_vm_args)
>           return vm
>   
>       @property
> 

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Philippe Mathieu-Daudé]

On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
> Some acceptance tests require KVM or they are skipped. Travis
> enables nested virtualization by default with Ubuntu
> 18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
> changed the acceptance builder to run in a Bionic VM. Also
> it was needed to ensure the current user has rw permission
> to /dev/kvm.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>   .travis.yml | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 6c1038a0f1..c3edd0a907 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -2,6 +2,7 @@
>   # Additional builds with specific requirements for a full VM need to
>   # be added as additional matrix: entries later on
>   dist: xenial
> +sudo: true
>   language: c
>   compiler:
>     - gcc
> @@ -83,6 +84,9 @@ git:
>   
>   before_script:
>     - if command -v ccache ; then ccache --zero-stats ; fi
> +  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
> +        sudo chmod o+rw /dev/kvm ;
> +    fi
>     - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
>     - ${SRC_DIR}/configure ${BASE_CONFIG} ${CONFIG} || { cat config.log && exit 1; }
>   script:
> @@ -272,12 +276,13 @@ matrix:
>           - TEST_CMD="make check-acceptance"
>         after_script:
>           - python3 -c 'import json; r = json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) for t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
> +      dist: bionic
>         addons:
>           apt:
>             packages:
>               - python3-pil
>               - python3-pip
> -            - python3.5-venv
> +            - python3.6-venv

This line doesn't seem related to the patch.

>               - tesseract-ocr
>               - tesseract-ocr-eng
>   
> 

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Thomas Huth]

On 24/01/2020 10.38, Philippe Mathieu-Daudé wrote:
> On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
>> Some acceptance tests require KVM or they are skipped. Travis
>> enables nested virtualization by default with Ubuntu
>> 18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
>> changed the acceptance builder to run in a Bionic VM. Also
>> it was needed to ensure the current user has rw permission
>> to /dev/kvm.
>>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> ---
>>   .travis.yml | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/.travis.yml b/.travis.yml
>> index 6c1038a0f1..c3edd0a907 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -2,6 +2,7 @@
>>   # Additional builds with specific requirements for a full VM need to
>>   # be added as additional matrix: entries later on
>>   dist: xenial
>> +sudo: true
>>   language: c
>>   compiler:
>>     - gcc
>> @@ -83,6 +84,9 @@ git:
>>     before_script:
>>     - if command -v ccache ; then ccache --zero-stats ; fi
>> +  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
>> +        sudo chmod o+rw /dev/kvm ;
>> +    fi
>>     - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
>>     - ${SRC_DIR}/configure ${BASE_CONFIG} ${CONFIG} || { cat
>> config.log && exit 1; }
>>   script:
>> @@ -272,12 +276,13 @@ matrix:
>>           - TEST_CMD="make check-acceptance"
>>         after_script:
>>           - python3 -c 'import json; r =
>> json.load(open("tests/results/latest/results.json"));
>> [print(t["logfile"]) for t in r["tests"] if t["status"] not in
>> ("PASS", "SKIP")]' | xargs cat
>> +      dist: bionic
>>         addons:
>>           apt:
>>             packages:
>>               - python3-pil
>>               - python3-pip
>> -            - python3.5-venv
>> +            - python3.6-venv
> 
> This line doesn't seem related to the patch.

"dist:" has been switched from xenial to bionic, so I think it is
required to update to python3.6 here, too?

 Thomas

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Philippe Mathieu-Daudé]

On 1/24/20 10:44 AM, Thomas Huth wrote:
> On 24/01/2020 10.38, Philippe Mathieu-Daudé wrote:
>> On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
>>> Some acceptance tests require KVM or they are skipped. Travis
>>> enables nested virtualization by default with Ubuntu
>>> 18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
>>> changed the acceptance builder to run in a Bionic VM. Also
>>> it was needed to ensure the current user has rw permission
>>> to /dev/kvm.
>>>
>>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>> ---
>>>    .travis.yml | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/.travis.yml b/.travis.yml
>>> index 6c1038a0f1..c3edd0a907 100644
>>> --- a/.travis.yml
>>> +++ b/.travis.yml
>>> @@ -2,6 +2,7 @@
>>>    # Additional builds with specific requirements for a full VM need to
>>>    # be added as additional matrix: entries later on
>>>    dist: xenial
>>> +sudo: true
>>>    language: c
>>>    compiler:
>>>      - gcc
>>> @@ -83,6 +84,9 @@ git:
>>>      before_script:
>>>      - if command -v ccache ; then ccache --zero-stats ; fi
>>> +  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
>>> +        sudo chmod o+rw /dev/kvm ;
>>> +    fi
>>>      - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
>>>      - ${SRC_DIR}/configure ${BASE_CONFIG} ${CONFIG} || { cat
>>> config.log && exit 1; }
>>>    script:
>>> @@ -272,12 +276,13 @@ matrix:
>>>            - TEST_CMD="make check-acceptance"
>>>          after_script:
>>>            - python3 -c 'import json; r =
>>> json.load(open("tests/results/latest/results.json"));
>>> [print(t["logfile"]) for t in r["tests"] if t["status"] not in
>>> ("PASS", "SKIP")]' | xargs cat
>>> +      dist: bionic
>>>          addons:
>>>            apt:
>>>              packages:
>>>                - python3-pil
>>>                - python3-pip
>>> -            - python3.5-venv
>>> +            - python3.6-venv
>>
>> This line doesn't seem related to the patch.
> 
> "dist:" has been switched from xenial to bionic, so I think it is
> required to update to python3.6 here, too?

OK, I got confused because line 4 is still "dist: xenial".

Wainer can you add a comment about this in the commit description?

I'm still not convinced we should enable "sudo: true" on all our jobs.

Re: [PATCH v3 2/4] tests/acceptance: boot_linux_console: Add boot Linux with kvm tests

[Philippe Mathieu-Daudé]

On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
> Added boot Linux test cases that launch QEMU with kvm
> enabled. Likewise it was added tests for tcg.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>   tests/acceptance/boot_linux_console.py | 88 ++++++++++++++++++++------
>   1 file changed, 68 insertions(+), 20 deletions(-)
> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index e40b84651b..a36eae630c 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -51,11 +51,7 @@ class BootLinuxConsole(Test):
>           os.chdir(cwd)
>           return self.workdir + path
>   
> -    def test_x86_64_pc(self):
> -        """
> -        :avocado: tags=arch:x86_64
> -        :avocado: tags=machine:pc
> -        """
> +    def do_test_x86_64_pc(self):
>           kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
>                         '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
>                         '/vmlinuz')
> @@ -70,6 +66,22 @@ class BootLinuxConsole(Test):
>           console_pattern = 'Kernel command line: %s' % kernel_command_line
>           self.wait_for_console_pattern(console_pattern)
>   
> +    def test_x86_64_pc_kvm(self):
> +        """
> +        :avocado: tags=arch:x86_64
> +        :avocado: tags=machine:pc
> +        :avocado: tags=accel:kvm
> +        """
> +        self.do_test_x86_64_pc()
> +
> +    def test_x86_64_pc_tcg(self):
> +        """
> +        :avocado: tags=arch:x86_64
> +        :avocado: tags=machine:pc
> +        :avocado: tags=accel:tcg
> +        """
> +        self.do_test_x86_64_pc()
[...]
So you want to test a binary linked with multiple accelerators.

Isn't it possible to have something clever/simpler?

    def test_x86_64_pc(self):
        """
        :avocado: tags=arch:x86_64
        :avocado: tags=machine:pc
        :avocado: tags=accel:kvm
        :avocado: tags=accel:tcg
        """
        self.do_test_x86_64_pc()

And use a mux config?

Because else we are duplicating a lot of code, and there are various 
accelerators available:

$ git grep ACCEL_CLASS_NAME
accel/qtest.c:41:#define TYPE_QTEST_ACCEL ACCEL_CLASS_NAME("qtest")
accel/tcg/tcg-all.c:46:#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
hw/xen/xen-common.c:200:#define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
include/sysemu/hvf.h:100:#define TYPE_HVF_ACCEL ACCEL_CLASS_NAME("hvf")
include/sysemu/kvm_int.h:36:#define TYPE_KVM_ACCEL ACCEL_CLASS_NAME("kvm")
target/i386/hax-all.c:1127:    .name = ACCEL_CLASS_NAME("hax"),
target/i386/whpx-all.c:1533:    .name = ACCEL_CLASS_NAME("whpx"),

And also pending:
target/i386/nvmm-all.c:   .name = ACCEL_CLASS_NAME("nvmm"),
https://www.mail-archive.com/qemu-devel@nongnu.org/msg668697.html

Re: [PATCH v3 2/4] tests/acceptance: boot_linux_console: Add boot Linux with kvm tests

[Thomas Huth]

On 24/01/2020 16.45, Philippe Mathieu-Daudé wrote:
> On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
>> Added boot Linux test cases that launch QEMU with kvm
>> enabled. Likewise it was added tests for tcg.
>>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> ---
>>   tests/acceptance/boot_linux_console.py | 88 ++++++++++++++++++++------
>>   1 file changed, 68 insertions(+), 20 deletions(-)
>>
>> diff --git a/tests/acceptance/boot_linux_console.py
>> b/tests/acceptance/boot_linux_console.py
>> index e40b84651b..a36eae630c 100644
>> --- a/tests/acceptance/boot_linux_console.py
>> +++ b/tests/acceptance/boot_linux_console.py
>> @@ -51,11 +51,7 @@ class BootLinuxConsole(Test):
>>           os.chdir(cwd)
>>           return self.workdir + path
>>   -    def test_x86_64_pc(self):
>> -        """
>> -        :avocado: tags=arch:x86_64
>> -        :avocado: tags=machine:pc
>> -        """
>> +    def do_test_x86_64_pc(self):
>>           kernel_url =
>> ('https://archives.fedoraproject.org/pub/archive/fedora'
>>                        
>> '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
>>                         '/vmlinuz')
>> @@ -70,6 +66,22 @@ class BootLinuxConsole(Test):
>>           console_pattern = 'Kernel command line: %s' %
>> kernel_command_line
>>           self.wait_for_console_pattern(console_pattern)
>>   +    def test_x86_64_pc_kvm(self):
>> +        """
>> +        :avocado: tags=arch:x86_64
>> +        :avocado: tags=machine:pc
>> +        :avocado: tags=accel:kvm
>> +        """
>> +        self.do_test_x86_64_pc()
>> +
>> +    def test_x86_64_pc_tcg(self):
>> +        """
>> +        :avocado: tags=arch:x86_64
>> +        :avocado: tags=machine:pc
>> +        :avocado: tags=accel:tcg
>> +        """
>> +        self.do_test_x86_64_pc()
> [...]
> So you want to test a binary linked with multiple accelerators.
> 
> Isn't it possible to have something clever/simpler?
> 
>    def test_x86_64_pc(self):
>        """
>        :avocado: tags=arch:x86_64
>        :avocado: tags=machine:pc
>        :avocado: tags=accel:kvm
>        :avocado: tags=accel:tcg
>        """
>        self.do_test_x86_64_pc()
> 
> And use a mux config?
> 
> Because else we are duplicating a lot of code, and there are various
> accelerators available:
> 
> $ git grep ACCEL_CLASS_NAME
> accel/qtest.c:41:#define TYPE_QTEST_ACCEL ACCEL_CLASS_NAME("qtest")
> accel/tcg/tcg-all.c:46:#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
> hw/xen/xen-common.c:200:#define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> include/sysemu/hvf.h:100:#define TYPE_HVF_ACCEL ACCEL_CLASS_NAME("hvf")
> include/sysemu/kvm_int.h:36:#define TYPE_KVM_ACCEL ACCEL_CLASS_NAME("kvm")
> target/i386/hax-all.c:1127:    .name = ACCEL_CLASS_NAME("hax"),
> target/i386/whpx-all.c:1533:    .name = ACCEL_CLASS_NAME("whpx"),
> 
> And also pending:
> target/i386/nvmm-all.c:   .name = ACCEL_CLASS_NAME("nvmm"),
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg668697.html

Is anybody running any of these in a CI environment?

 Thomas

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Wainer dos Santos Moschetta]

On 1/24/20 7:54 AM, Philippe Mathieu-Daudé wrote:
> On 1/24/20 10:44 AM, Thomas Huth wrote:
>> On 24/01/2020 10.38, Philippe Mathieu-Daudé wrote:
>>> On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
>>>> Some acceptance tests require KVM or they are skipped. Travis
>>>> enables nested virtualization by default with Ubuntu
>>>> 18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
>>>> changed the acceptance builder to run in a Bionic VM. Also
>>>> it was needed to ensure the current user has rw permission
>>>> to /dev/kvm.
>>>>
>>>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>>> ---
>>>>    .travis.yml | 7 ++++++-
>>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/.travis.yml b/.travis.yml
>>>> index 6c1038a0f1..c3edd0a907 100644
>>>> --- a/.travis.yml
>>>> +++ b/.travis.yml
>>>> @@ -2,6 +2,7 @@
>>>>    # Additional builds with specific requirements for a full VM 
>>>> need to
>>>>    # be added as additional matrix: entries later on
>>>>    dist: xenial
>>>> +sudo: true
>>>>    language: c
>>>>    compiler:
>>>>      - gcc
>>>> @@ -83,6 +84,9 @@ git:
>>>>      before_script:
>>>>      - if command -v ccache ; then ccache --zero-stats ; fi
>>>> +  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
>>>> +        sudo chmod o+rw /dev/kvm ;
>>>> +    fi


Philippe, anwsering here your question about 'sudo'.

The above statement runs on before_script for all the builders. As far 
as I know only on Bionic-based builders 'chmod' (that needs sudo) will 
be executed, so technically 'sudo' should  be enabled only on those 
builders. But I thought that would be error-prone not enable it globally 
since the code requiring it is globally declared too. All in all, I 
don't have a strong option for this.


>>>>
>>>>      - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
>>>>      - ${SRC_DIR}/configure ${BASE_CONFIG} ${CONFIG} || { cat
>>>> config.log && exit 1; }
>>>>    script:
>>>> @@ -272,12 +276,13 @@ matrix:
>>>>            - TEST_CMD="make check-acceptance"
>>>>          after_script:
>>>>            - python3 -c 'import json; r =
>>>> json.load(open("tests/results/latest/results.json"));
>>>> [print(t["logfile"]) for t in r["tests"] if t["status"] not in
>>>> ("PASS", "SKIP")]' | xargs cat
>>>> +      dist: bionic
>>>>          addons:
>>>>            apt:
>>>>              packages:
>>>>                - python3-pil
>>>>                - python3-pip
>>>> -            - python3.5-venv
>>>> +            - python3.6-venv
>>>
>>> This line doesn't seem related to the patch.
>>
>> "dist:" has been switched from xenial to bionic, so I think it is
>> required to update to python3.6 here, too?


Thomas is right, python3.5-venv isn't available on Ubuntu Bionic.


>>
>
> OK, I got confused because line 4 is still "dist: xenial".


I'm about to send a proposal to bump dist to bionic. There are some 
non-acceptance tests being skipped because of the lack of nested kvm on 
Travis's xenial VMs, so that would be beneficial to them as well.

Thomas mentioned in another email thread that there is a build problem 
with the libssh version of Bionic (I hope that can be worked out). Other 
than that, do you see any impediment to switch all builders completely?


>
>
> Wainer can you add a comment about this in the commit description?


Sure, actually I should have done it. Thanks for raising that point too.

- Wainer

>
>
> I'm still not convinced we should enable "sudo: true" on all our jobs.
>

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Wainer dos Santos Moschetta]

On 1/22/20 7:22 AM, Thomas Huth wrote:
> On 22/01/2020 02.27, Wainer dos Santos Moschetta wrote:
>> Some acceptance tests require KVM or they are skipped. Travis
>> enables nested virtualization by default with Ubuntu
>> 18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
>> changed the acceptance builder to run in a Bionic VM. Also
>> it was needed to ensure the current user has rw permission
>> to /dev/kvm.
>>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> ---
>>   .travis.yml | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/.travis.yml b/.travis.yml
>> index 6c1038a0f1..c3edd0a907 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -2,6 +2,7 @@
>>   # Additional builds with specific requirements for a full VM need to
>>   # be added as additional matrix: entries later on
>>   dist: xenial
>> +sudo: true
>>   language: c
>>   compiler:
>>     - gcc
>> @@ -83,6 +84,9 @@ git:
>>   
>>   before_script:
>>     - if command -v ccache ; then ccache --zero-stats ; fi
>> +  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
> By the way, in case you respin, could you please use singel "[" instead
> of "[[" ... since that's what we use in almost all other shell scripts, too.

Like this? ->

if [ -e /dev/kvm ] && [ ! -r /dev/kvm ] || [ ! -w /dev/kvm ]; then

Thanks,

Wainer

>
>   Thomas

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Thomas Huth]

On 24/01/2020 22.15, Wainer dos Santos Moschetta wrote:
> 
> On 1/22/20 7:22 AM, Thomas Huth wrote:
>> On 22/01/2020 02.27, Wainer dos Santos Moschetta wrote:
>>> Some acceptance tests require KVM or they are skipped. Travis
>>> enables nested virtualization by default with Ubuntu
>>> 18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
>>> changed the acceptance builder to run in a Bionic VM. Also
>>> it was needed to ensure the current user has rw permission
>>> to /dev/kvm.
>>>
>>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>> ---
>>>   .travis.yml | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/.travis.yml b/.travis.yml
>>> index 6c1038a0f1..c3edd0a907 100644
>>> --- a/.travis.yml
>>> +++ b/.travis.yml
>>> @@ -2,6 +2,7 @@
>>>   # Additional builds with specific requirements for a full VM need to
>>>   # be added as additional matrix: entries later on
>>>   dist: xenial
>>> +sudo: true
>>>   language: c
>>>   compiler:
>>>     - gcc
>>> @@ -83,6 +84,9 @@ git:
>>>     before_script:
>>>     - if command -v ccache ; then ccache --zero-stats ; fi
>>> +  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
>> By the way, in case you respin, could you please use singel "[" instead
>> of "[[" ... since that's what we use in almost all other shell
>> scripts, too.
> 
> Like this? ->
> 
> if [ -e /dev/kvm ] && [ ! -r /dev/kvm ] || [ ! -w /dev/kvm ]; then

If I get the man-page of bash right, && and || have equal precedence ...
so I'd maybe rather write it as:

 if [ -e /dev/kvm ]; then if [ ! -r /dev/kvm ] || [ ! -w /dev/kvm ]; ...

... ok, this is getting uglier ...maybe it's better to rather stick with
your original code...?

 Thomas

PS: You could also use -c instead -e in the first test.

Re: [PATCH v3 4/4] travis.yml: Enable acceptance KVM tests

[Philippe Mathieu-Daudé]

On 1/24/20 8:55 PM, Wainer dos Santos Moschetta wrote:
> On 1/24/20 7:54 AM, Philippe Mathieu-Daudé wrote:
>> On 1/24/20 10:44 AM, Thomas Huth wrote:
>>> On 24/01/2020 10.38, Philippe Mathieu-Daudé wrote:
>>>> On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
>>>>> Some acceptance tests require KVM or they are skipped. Travis
>>>>> enables nested virtualization by default with Ubuntu
>>>>> 18.04 (Bionic) on x86_64. So in order to run the kvm tests, this
>>>>> changed the acceptance builder to run in a Bionic VM. Also
>>>>> it was needed to ensure the current user has rw permission
>>>>> to /dev/kvm.
>>>>>
>>>>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>>>> ---
>>>>>    .travis.yml | 7 ++++++-
>>>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/.travis.yml b/.travis.yml
>>>>> index 6c1038a0f1..c3edd0a907 100644
>>>>> --- a/.travis.yml
>>>>> +++ b/.travis.yml
>>>>> @@ -2,6 +2,7 @@
>>>>>    # Additional builds with specific requirements for a full VM 
>>>>> need to
>>>>>    # be added as additional matrix: entries later on
>>>>>    dist: xenial
>>>>> +sudo: true
>>>>>    language: c
>>>>>    compiler:
>>>>>      - gcc
>>>>> @@ -83,6 +84,9 @@ git:
>>>>>      before_script:
>>>>>      - if command -v ccache ; then ccache --zero-stats ; fi
>>>>> +  - if [[ -e /dev/kvm ]] && ! [[ -r /dev/kvm && -w /dev/kvm ]]; then
>>>>> +        sudo chmod o+rw /dev/kvm ;
>>>>> +    fi
> 
> 
> Philippe, anwsering here your question about 'sudo'.
> 
> The above statement runs on before_script for all the builders. As far 
> as I know only on Bionic-based builders 'chmod' (that needs sudo) will 
> be executed, so technically 'sudo' should  be enabled only on those 
> builders. But I thought that would be error-prone not enable it globally 
> since the code requiring it is globally declared too. All in all, I 
> don't have a strong option for this.
> 
> 
>>>>>
>>>>>      - mkdir -p ${BUILD_DIR} && cd ${BUILD_DIR}
>>>>>      - ${SRC_DIR}/configure ${BASE_CONFIG} ${CONFIG} || { cat
>>>>> config.log && exit 1; }
>>>>>    script:
>>>>> @@ -272,12 +276,13 @@ matrix:
>>>>>            - TEST_CMD="make check-acceptance"
>>>>>          after_script:
>>>>>            - python3 -c 'import json; r =
>>>>> json.load(open("tests/results/latest/results.json"));
>>>>> [print(t["logfile"]) for t in r["tests"] if t["status"] not in
>>>>> ("PASS", "SKIP")]' | xargs cat
>>>>> +      dist: bionic
>>>>>          addons:
>>>>>            apt:
>>>>>              packages:
>>>>>                - python3-pil
>>>>>                - python3-pip
>>>>> -            - python3.5-venv
>>>>> +            - python3.6-venv
>>>>
>>>> This line doesn't seem related to the patch.
>>>
>>> "dist:" has been switched from xenial to bionic, so I think it is
>>> required to update to python3.6 here, too?
> 
> 
> Thomas is right, python3.5-venv isn't available on Ubuntu Bionic.
> 
> 
>>>
>>
>> OK, I got confused because line 4 is still "dist: xenial".
> 
> 
> I'm about to send a proposal to bump dist to bionic. There are some 
> non-acceptance tests being skipped because of the lack of nested kvm on 
> Travis's xenial VMs, so that would be beneficial to them as well.
> 
> Thomas mentioned in another email thread that there is a build problem 
> with the libssh version of Bionic (I hope that can be worked out). Other 
> than that, do you see any impediment to switch all builders completely?

No, this is a distribution bug, we can use "--disable-libssh" in the 
Bionic jobs.
>>
>>
>> Wainer can you add a comment about this in the commit description?
> 
> 
> Sure, actually I should have done it. Thanks for raising that point too.
> 
> - Wainer
> 
>>
>>
>> I'm still not convinced we should enable "sudo: true" on all our jobs.
>>
> 

Re: [PATCH v3 1/4] tests/acceptance: avocado_qemu: Introduce the 'accel' test parameter

[Wainer dos Santos Moschetta]

On 1/24/20 7:36 AM, Philippe Mathieu-Daudé wrote:
> On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
>> The test case may need to boot the VM with an accelerator that
>> isn't actually enabled on the QEMU binary and/or present in the host. In
>> this case the test behavior is undefined, and the best course of
>> action is to skip its execution.
>>
>> This change introduced the 'accel' parameter (and the handler of
>> tag with same name) used to indicate the test case requires a
>> given accelerator available. It was implemented a mechanism to
>> skip the test case if the accelerator is not available. Moreover,
>>   the QEMU -accel argument is set automatically to any VM
>> launched if the parameter is present.
>>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> ---
>>   docs/devel/testing.rst                    | 16 ++++++++++++++++
>>   tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
>>   2 files changed, 39 insertions(+)
>>
>> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
>> index ab5be0c729..d17d0e90aa 100644
>> --- a/docs/devel/testing.rst
>> +++ b/docs/devel/testing.rst
>> @@ -759,6 +759,17 @@ name.  If one is not given explicitly, it will 
>> either be set to
>>   ``None``, or, if the test is tagged with one (and only one)
>>   ``:avocado: tags=machine:VALUE`` tag, it will be set to ``VALUE``.
>>   +accel
>> +~~~~~
>> +The accelerator that will be set to all QEMUMachine instances created
>> +by the test.
>> +
>> +The ``accel`` attribute will be set to the test parameter of the same
>> +name.  If one is not given explicitly, it will either be set to
>> +``None``, or, if the test is tagged with one (and only one)
>> +``:avocado: tags=accel:VALUE`` tag, it will be set to ``VALUE``. 
>> Currently
>> +``VALUE`` should be either ``kvm`` or ``tcg``.
>> +
>>   qemu_bin
>>   ~~~~~~~~
>>   @@ -800,6 +811,11 @@ machine
>>   The machine type that will be set to all QEMUMachine instances created
>>   by the test.
>>   +accel
>> +~~~~~
>> +The accelerator that will be set to all QEMUMachine instances created
>> +by the test. In case the accelerator is not available (both QEMU
>> +binary and the host system are checked) then the test is canceled.
>>     qemu_bin
>>   ~~~~~~~~
>> diff --git a/tests/acceptance/avocado_qemu/__init__.py 
>> b/tests/acceptance/avocado_qemu/__init__.py
>> index 6618ea67c1..c83a75ccbc 100644
>> --- a/tests/acceptance/avocado_qemu/__init__.py
>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>> @@ -20,6 +20,7 @@ SRC_ROOT_DIR = 
>> os.path.join(os.path.dirname(__file__), '..', '..', '..')
>>   sys.path.append(os.path.join(SRC_ROOT_DIR, 'python'))
>>     from qemu.machine import QEMUMachine
>> +from qemu.accel import kvm_available, tcg_available
>>     def is_readable_executable_file(path):
>>       return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
>> @@ -111,6 +112,8 @@ class Test(avocado.Test):
>>         def setUp(self):
>>           self._vms = {}
>> +        # VM argumments that are mapped from parameters
>> +        self._param_to_vm_args = []
>>             self.arch = self.params.get('arch',
>> default=self._get_unique_tag_val('arch'))
>> @@ -124,10 +127,30 @@ class Test(avocado.Test):
>>           if self.qemu_bin is None:
>>               self.cancel("No QEMU binary defined or found in the 
>> source tree")
>>   +        self.accel = self.params.get('accel',
>> + default=self._get_unique_tag_val('accel'))
>> +        if self.accel:
>> +            avail = False
>> +            if self.accel == 'kvm':
>> +                if kvm_available(self.arch, self.qemu_bin):
>> +                    avail = True
>> +            elif self.accel == 'tcg':
>> +                if tcg_available(self.qemu_bin):
>> +                    avail = True
>> +            else:
>> +                self.cancel("Unknown accelerator: %s" % self.accel)
>> +
>> +            if avail:
>> +                self._param_to_vm_args.extend(['-accel', self.accel])
>> +            else:
>> +                self.cancel("%s is not available" % self.accel)
>
> Why refuse to test the other accelerators?
>
> Isn't it better to QMP-ask QEMU which accelerator it supports, and 
> SKIP if it isn't available?


python/qemu/accel.py:list_accel(qemu_bin) can be used for that end. For 
example:

if self.accel not in list_accel(self.qemu_bin):

     self.cancel("%s is not available" % self.accel)

However checking the support only on QEMU's binary may be very weak 
(take KVM as an example). I implemented checkers for kvm and tcg on 
python/qemu/accel.py. Given that I have zero knowledge on the other 
accelerators,  I simply did not touch on them.

That said, IMHO it needs to implement checkers for those others 
accelerator before they get reliably handled by avocado_qemu 
automatically. And test writers can still run tests with those 
accelerators as long as 'accel' tag is not used.

What do you think Philippe?

Thanks, good point!

- Wainer


>
>
>> +
>>       def _new_vm(self, *args):
>>           vm = QEMUMachine(self.qemu_bin, sock_dir=tempfile.mkdtemp())
>>           if args:
>>               vm.add_args(*args)
>> +        if self._param_to_vm_args:
>> +            vm.add_args(*self._param_to_vm_args)
>>           return vm
>>         @property
>>
>

Re: [PATCH v3 1/4] tests/acceptance: avocado_qemu: Introduce the 'accel' test parameter

[Philippe Mathieu-Daudé]

On 1/27/20 8:28 PM, Wainer dos Santos Moschetta wrote:
> On 1/24/20 7:36 AM, Philippe Mathieu-Daudé wrote:
>> On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
>>> The test case may need to boot the VM with an accelerator that
>>> isn't actually enabled on the QEMU binary and/or present in the host. In
>>> this case the test behavior is undefined, and the best course of
>>> action is to skip its execution.
>>>
>>> This change introduced the 'accel' parameter (and the handler of
>>> tag with same name) used to indicate the test case requires a
>>> given accelerator available. It was implemented a mechanism to
>>> skip the test case if the accelerator is not available. Moreover,
>>>   the QEMU -accel argument is set automatically to any VM
>>> launched if the parameter is present.
>>>
>>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>> ---
>>>   docs/devel/testing.rst                    | 16 ++++++++++++++++
>>>   tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
>>>   2 files changed, 39 insertions(+)
>>>
>>> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
>>> index ab5be0c729..d17d0e90aa 100644
>>> --- a/docs/devel/testing.rst
>>> +++ b/docs/devel/testing.rst
>>> @@ -759,6 +759,17 @@ name.  If one is not given explicitly, it will 
>>> either be set to
>>>   ``None``, or, if the test is tagged with one (and only one)
>>>   ``:avocado: tags=machine:VALUE`` tag, it will be set to ``VALUE``.
>>>   +accel
>>> +~~~~~
>>> +The accelerator that will be set to all QEMUMachine instances created
>>> +by the test.
>>> +
>>> +The ``accel`` attribute will be set to the test parameter of the same
>>> +name.  If one is not given explicitly, it will either be set to
>>> +``None``, or, if the test is tagged with one (and only one)
>>> +``:avocado: tags=accel:VALUE`` tag, it will be set to ``VALUE``. 
>>> Currently
>>> +``VALUE`` should be either ``kvm`` or ``tcg``.
>>> +
>>>   qemu_bin
>>>   ~~~~~~~~
>>>   @@ -800,6 +811,11 @@ machine
>>>   The machine type that will be set to all QEMUMachine instances created
>>>   by the test.
>>>   +accel
>>> +~~~~~
>>> +The accelerator that will be set to all QEMUMachine instances created
>>> +by the test. In case the accelerator is not available (both QEMU
>>> +binary and the host system are checked) then the test is canceled.
>>>     qemu_bin
>>>   ~~~~~~~~
>>> diff --git a/tests/acceptance/avocado_qemu/__init__.py 
>>> b/tests/acceptance/avocado_qemu/__init__.py
>>> index 6618ea67c1..c83a75ccbc 100644
>>> --- a/tests/acceptance/avocado_qemu/__init__.py
>>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>>> @@ -20,6 +20,7 @@ SRC_ROOT_DIR = 
>>> os.path.join(os.path.dirname(__file__), '..', '..', '..')
>>>   sys.path.append(os.path.join(SRC_ROOT_DIR, 'python'))
>>>     from qemu.machine import QEMUMachine
>>> +from qemu.accel import kvm_available, tcg_available
>>>     def is_readable_executable_file(path):
>>>       return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
>>> @@ -111,6 +112,8 @@ class Test(avocado.Test):
>>>         def setUp(self):
>>>           self._vms = {}
>>> +        # VM argumments that are mapped from parameters
>>> +        self._param_to_vm_args = []
>>>             self.arch = self.params.get('arch',
>>> default=self._get_unique_tag_val('arch'))
>>> @@ -124,10 +127,30 @@ class Test(avocado.Test):
>>>           if self.qemu_bin is None:
>>>               self.cancel("No QEMU binary defined or found in the 
>>> source tree")
>>>   +        self.accel = self.params.get('accel',
>>> + default=self._get_unique_tag_val('accel'))
>>> +        if self.accel:
>>> +            avail = False
>>> +            if self.accel == 'kvm':
>>> +                if kvm_available(self.arch, self.qemu_bin):
>>> +                    avail = True
>>> +            elif self.accel == 'tcg':
>>> +                if tcg_available(self.qemu_bin):
>>> +                    avail = True
>>> +            else:
>>> +                self.cancel("Unknown accelerator: %s" % self.accel)
>>> +
>>> +            if avail:
>>> +                self._param_to_vm_args.extend(['-accel', self.accel])
>>> +            else:
>>> +                self.cancel("%s is not available" % self.accel)
>>
>> Why refuse to test the other accelerators?
>>
>> Isn't it better to QMP-ask QEMU which accelerator it supports, and 
>> SKIP if it isn't available?
> 
> 
> python/qemu/accel.py:list_accel(qemu_bin) can be used for that end. For 
> example:
> 
> if self.accel not in list_accel(self.qemu_bin):
> 
>      self.cancel("%s is not available" % self.accel)
> 
> However checking the support only on QEMU's binary may be very weak 
> (take KVM as an example). I implemented checkers for kvm and tcg on 
> python/qemu/accel.py. Given that I have zero knowledge on the other 
> accelerators,  I simply did not touch on them.
> 
> That said, IMHO it needs to implement checkers for those others 
> accelerator before they get reliably handled by avocado_qemu 
> automatically. And test writers can still run tests with those 
> accelerators as long as 'accel' tag is not used.
> 
> What do you think Philippe?

Can you have a look at the binary_get_accels() method which aims to be 
more generic?

https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg07312.html
"python/qemu: Add binutils::binary_get_accels()"

https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg07313.html
"python/qemu/accel: Use binutils::binary_get_accels()"

> 
> Thanks, good point!
> 
> - Wainer
> 
> 
>>
>>
>>> +
>>>       def _new_vm(self, *args):
>>>           vm = QEMUMachine(self.qemu_bin, sock_dir=tempfile.mkdtemp())
>>>           if args:
>>>               vm.add_args(*args)
>>> +        if self._param_to_vm_args:
>>> +            vm.add_args(*self._param_to_vm_args)
>>>           return vm
>>>         @property
>>>
>>
> 

Re: [PATCH v3 3/4] tests/acceptance: avocado_qemu: Refactor the handler of 'machine' parameter

[Philippe Mathieu-Daudé]

On Wed, Jan 22, 2020 at 2:28 AM Wainer dos Santos Moschetta
<wainersm@redhat.com> wrote:
>
> The Test._param_to_vm_args variable contain VM arguments that should be added
> at launch which were originated from test parameters. Use this variable
> to set -M from 'machine' parameter as well.
>
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  tests/acceptance/avocado_qemu/__init__.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index c83a75ccbc..443ac02aff 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -120,6 +120,8 @@ class Test(avocado.Test):
>
>          self.machine = self.params.get('machine',
>                                         default=self._get_unique_tag_val('machine'))
> +        if self.machine:
> +            self._param_to_vm_args.extend(['-M', self.machine])
>
>          default_qemu_bin = pick_default_qemu_bin(arch=self.arch)
>          self.qemu_bin = self.params.get('qemu_bin',
> @@ -162,8 +164,6 @@ class Test(avocado.Test):
>              name = str(uuid.uuid4())
>          if self._vms.get(name) is None:
>              self._vms[name] = self._new_vm(*args)
> -            if self.machine is not None:
> -                self._vms[name].set_machine(self.machine)
>          return self._vms[name]
>
>      def tearDown(self):
> --
> 2.23.0
>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v3 2/4] tests/acceptance: boot_linux_console: Add boot Linux with kvm tests

[Wainer dos Santos Moschetta]

On 1/24/20 1:45 PM, Philippe Mathieu-Daudé wrote:
> On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
>> Added boot Linux test cases that launch QEMU with kvm
>> enabled. Likewise it was added tests for tcg.
>>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> ---
>>   tests/acceptance/boot_linux_console.py | 88 ++++++++++++++++++++------
>>   1 file changed, 68 insertions(+), 20 deletions(-)
>>
>> diff --git a/tests/acceptance/boot_linux_console.py 
>> b/tests/acceptance/boot_linux_console.py
>> index e40b84651b..a36eae630c 100644
>> --- a/tests/acceptance/boot_linux_console.py
>> +++ b/tests/acceptance/boot_linux_console.py
>> @@ -51,11 +51,7 @@ class BootLinuxConsole(Test):
>>           os.chdir(cwd)
>>           return self.workdir + path
>>   -    def test_x86_64_pc(self):
>> -        """
>> -        :avocado: tags=arch:x86_64
>> -        :avocado: tags=machine:pc
>> -        """
>> +    def do_test_x86_64_pc(self):
>>           kernel_url = 
>> ('https://archives.fedoraproject.org/pub/archive/fedora'
>> '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
>>                         '/vmlinuz')
>> @@ -70,6 +66,22 @@ class BootLinuxConsole(Test):
>>           console_pattern = 'Kernel command line: %s' % 
>> kernel_command_line
>>           self.wait_for_console_pattern(console_pattern)
>>   +    def test_x86_64_pc_kvm(self):
>> +        """
>> +        :avocado: tags=arch:x86_64
>> +        :avocado: tags=machine:pc
>> +        :avocado: tags=accel:kvm
>> +        """
>> +        self.do_test_x86_64_pc()
>> +
>> +    def test_x86_64_pc_tcg(self):
>> +        """
>> +        :avocado: tags=arch:x86_64
>> +        :avocado: tags=machine:pc
>> +        :avocado: tags=accel:tcg
>> +        """
>> +        self.do_test_x86_64_pc()
> [...]
> So you want to test a binary linked with multiple accelerators.
>
> Isn't it possible to have something clever/simpler?


It is feasible using Avocado's CIT [1] or PICT [2] plug-ins. This work 
is suggested by Cleber in [3] and I've it on my radar for future work.

But, for the moment, I would like to have those simple boot with KVM 
tests merged so that they are executed in our Travis CI.

[1] 
https://avocado-framework.readthedocs.io/en/75.1/plugins/optional/varianter_cit.html

[2] 
https://avocado-framework.readthedocs.io/en/75.1/plugins/optional/varianter_pict.html

[3] 
https://trello.com/c/1wvzcxHY/105-create-cit-parameter-for-acceptance-tests

>
>
>    def test_x86_64_pc(self):
>        """
>        :avocado: tags=arch:x86_64
>        :avocado: tags=machine:pc
>        :avocado: tags=accel:kvm
>        :avocado: tags=accel:tcg
>        """
>        self.do_test_x86_64_pc()
>
> And use a mux config?
>
> Because else we are duplicating a lot of code, and there are various 
> accelerators available:
>
> $ git grep ACCEL_CLASS_NAME
> accel/qtest.c:41:#define TYPE_QTEST_ACCEL ACCEL_CLASS_NAME("qtest")
> accel/tcg/tcg-all.c:46:#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
> hw/xen/xen-common.c:200:#define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> include/sysemu/hvf.h:100:#define TYPE_HVF_ACCEL ACCEL_CLASS_NAME("hvf")
> include/sysemu/kvm_int.h:36:#define TYPE_KVM_ACCEL 
> ACCEL_CLASS_NAME("kvm")
> target/i386/hax-all.c:1127:    .name = ACCEL_CLASS_NAME("hax"),
> target/i386/whpx-all.c:1533:    .name = ACCEL_CLASS_NAME("whpx"),

Unfortunately I don't have enough time, resources and knowledge to work 
on accelerators other than kvm and tcg. I appreciate if others could 
contribute here, and I'm happy to review the patches.

Another point is how those tests could be ran on our CI.

Thanks for bringing those points!

- Wainer

>
>
> And also pending:
> target/i386/nvmm-all.c:   .name = ACCEL_CLASS_NAME("nvmm"),
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg668697.html
>

Re: [PATCH v3 1/4] tests/acceptance: avocado_qemu: Introduce the 'accel' test parameter

[Wainer dos Santos Moschetta]

On 1/30/20 8:51 PM, Philippe Mathieu-Daudé wrote:
> On 1/27/20 8:28 PM, Wainer dos Santos Moschetta wrote:
>> On 1/24/20 7:36 AM, Philippe Mathieu-Daudé wrote:
>>> On 1/22/20 2:27 AM, Wainer dos Santos Moschetta wrote:
>>>> The test case may need to boot the VM with an accelerator that
>>>> isn't actually enabled on the QEMU binary and/or present in the 
>>>> host. In
>>>> this case the test behavior is undefined, and the best course of
>>>> action is to skip its execution.
>>>>
>>>> This change introduced the 'accel' parameter (and the handler of
>>>> tag with same name) used to indicate the test case requires a
>>>> given accelerator available. It was implemented a mechanism to
>>>> skip the test case if the accelerator is not available. Moreover,
>>>>   the QEMU -accel argument is set automatically to any VM
>>>> launched if the parameter is present.
>>>>
>>>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>>> ---
>>>>   docs/devel/testing.rst                    | 16 ++++++++++++++++
>>>>   tests/acceptance/avocado_qemu/__init__.py | 23 
>>>> +++++++++++++++++++++++
>>>>   2 files changed, 39 insertions(+)
>>>>
>>>> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
>>>> index ab5be0c729..d17d0e90aa 100644
>>>> --- a/docs/devel/testing.rst
>>>> +++ b/docs/devel/testing.rst
>>>> @@ -759,6 +759,17 @@ name.  If one is not given explicitly, it will 
>>>> either be set to
>>>>   ``None``, or, if the test is tagged with one (and only one)
>>>>   ``:avocado: tags=machine:VALUE`` tag, it will be set to ``VALUE``.
>>>>   +accel
>>>> +~~~~~
>>>> +The accelerator that will be set to all QEMUMachine instances created
>>>> +by the test.
>>>> +
>>>> +The ``accel`` attribute will be set to the test parameter of the same
>>>> +name.  If one is not given explicitly, it will either be set to
>>>> +``None``, or, if the test is tagged with one (and only one)
>>>> +``:avocado: tags=accel:VALUE`` tag, it will be set to ``VALUE``. 
>>>> Currently
>>>> +``VALUE`` should be either ``kvm`` or ``tcg``.
>>>> +
>>>>   qemu_bin
>>>>   ~~~~~~~~
>>>>   @@ -800,6 +811,11 @@ machine
>>>>   The machine type that will be set to all QEMUMachine instances 
>>>> created
>>>>   by the test.
>>>>   +accel
>>>> +~~~~~
>>>> +The accelerator that will be set to all QEMUMachine instances created
>>>> +by the test. In case the accelerator is not available (both QEMU
>>>> +binary and the host system are checked) then the test is canceled.
>>>>     qemu_bin
>>>>   ~~~~~~~~
>>>> diff --git a/tests/acceptance/avocado_qemu/__init__.py 
>>>> b/tests/acceptance/avocado_qemu/__init__.py
>>>> index 6618ea67c1..c83a75ccbc 100644
>>>> --- a/tests/acceptance/avocado_qemu/__init__.py
>>>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>>>> @@ -20,6 +20,7 @@ SRC_ROOT_DIR = 
>>>> os.path.join(os.path.dirname(__file__), '..', '..', '..')
>>>>   sys.path.append(os.path.join(SRC_ROOT_DIR, 'python'))
>>>>     from qemu.machine import QEMUMachine
>>>> +from qemu.accel import kvm_available, tcg_available
>>>>     def is_readable_executable_file(path):
>>>>       return os.path.isfile(path) and os.access(path, os.R_OK | 
>>>> os.X_OK)
>>>> @@ -111,6 +112,8 @@ class Test(avocado.Test):
>>>>         def setUp(self):
>>>>           self._vms = {}
>>>> +        # VM argumments that are mapped from parameters
>>>> +        self._param_to_vm_args = []
>>>>             self.arch = self.params.get('arch',
>>>> default=self._get_unique_tag_val('arch'))
>>>> @@ -124,10 +127,30 @@ class Test(avocado.Test):
>>>>           if self.qemu_bin is None:
>>>>               self.cancel("No QEMU binary defined or found in the 
>>>> source tree")
>>>>   +        self.accel = self.params.get('accel',
>>>> + default=self._get_unique_tag_val('accel'))
>>>> +        if self.accel:
>>>> +            avail = False
>>>> +            if self.accel == 'kvm':
>>>> +                if kvm_available(self.arch, self.qemu_bin):
>>>> +                    avail = True
>>>> +            elif self.accel == 'tcg':
>>>> +                if tcg_available(self.qemu_bin):
>>>> +                    avail = True
>>>> +            else:
>>>> +                self.cancel("Unknown accelerator: %s" % self.accel)
>>>> +
>>>> +            if avail:
>>>> +                self._param_to_vm_args.extend(['-accel', self.accel])
>>>> +            else:
>>>> +                self.cancel("%s is not available" % self.accel)
>>>
>>> Why refuse to test the other accelerators?
>>>
>>> Isn't it better to QMP-ask QEMU which accelerator it supports, and 
>>> SKIP if it isn't available?
>>
>>
>> python/qemu/accel.py:list_accel(qemu_bin) can be used for that end. 
>> For example:
>>
>> if self.accel not in list_accel(self.qemu_bin):
>>
>>      self.cancel("%s is not available" % self.accel)
>>
>> However checking the support only on QEMU's binary may be very weak 
>> (take KVM as an example). I implemented checkers for kvm and tcg on 
>> python/qemu/accel.py. Given that I have zero knowledge on the other 
>> accelerators,  I simply did not touch on them.
>>
>> That said, IMHO it needs to implement checkers for those others 
>> accelerator before they get reliably handled by avocado_qemu 
>> automatically. And test writers can still run tests with those 
>> accelerators as long as 'accel' tag is not used.
>>
>> What do you think Philippe?
>
> Can you have a look at the binary_get_accels() method which aims to be 
> more generic?
>
> https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg07312.html
> "python/qemu: Add binutils::binary_get_accels()"
>
> https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg07313.html
> "python/qemu/accel: Use binutils::binary_get_accels()"


Sure, I will have a look at those.

But I prefer to stick with the implementation proposed on this patch. 
IMHO if avocado_qemu is going to set the accelerator from the tag, then 
it should/could go further with additional checks (e.g. if kvm is 
enabled at host, not just at QEMU binary).

- Wainer

>
>>
>> Thanks, good point!
>>
>> - Wainer
>>
>>
>>>
>>>
>>>> +
>>>>       def _new_vm(self, *args):
>>>>           vm = QEMUMachine(self.qemu_bin, sock_dir=tempfile.mkdtemp())
>>>>           if args:
>>>>               vm.add_args(*args)
>>>> +        if self._param_to_vm_args:
>>>> +            vm.add_args(*self._param_to_vm_args)
>>>>           return vm
>>>>         @property
>>>>
>>>
>>
>