[PATCH 0/3] tests/acceptance: Handle tests with "cpu" tag

[PATCH 0/3] tests/acceptance: Handle tests with "cpu" tag

[Wainer dos Santos Moschetta]

Currently the acceptance tests tagged with "machine" have the "-M TYPE"
automatically added to the list of arguments of the QEMUMachine object.
In other words, that option is passed to the launched QEMU. On this
series it is implemented the same feature but instead for tests marked
with "cpu".

There is a caveat, however, in case the test needs additional arguments to
the CPU type they cannot be passed via tag, because the tags parser split
values by comma. For example, in tests/acceptance/x86_cpu_model_versions.py,
there are cases where:

  * -cpu is set to "Cascadelake-Server,x-force-features=on,check=off,enforce=off"
  * if it was tagged like "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off"
    then the parser would break it into 4 tags ("cpu:Cascadelake-Server",
    "x-force-features=on", "check=off", "enforce=off")
  * resulting on "-cpu Cascadelake-Server" and the remaining arguments are ignored.

For the example above, one should tag it (or not at all) as "cpu:Cascadelake-Server"
AND self.vm.add_args('-cpu', "Cascadelake-Server,x-force-features=on,check=off,enforce=off"),
and that results on something like:

  "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu Cascadelake-Server,x-force-features=on,check=off,enforce=off".

QEMU is going to ignore the first -cpu argument. See the patch 0003 for a reference.

This series was tested on CI (https://gitlab.com/wainersm/qemu/-/pipelines/261254251)
and with the following code:

from avocado_qemu import Test

class CPUTest(Test):
    def test_cpu(self):
        """
        :avocado: tags=cpu:host
        """
        # The cpu property is set to the tag value, or None on its absence
        self.assertEqual(self.cpu, "host")
        # The created VM has the '-cpu host' option
        self.assertIn("-cpu host", " ".join(self.vm._args))
        self.vm.launch()

    def test_cpu_none(self):
        self.assertEqual(self.cpu, None)
        self.assertNotIn('-cpu', self.vm._args)

Wainer dos Santos Moschetta (3):
  tests/acceptance: Automatic set -cpu to the test vm
  tests/acceptance: Let the framework handle "cpu:VALUE" tagged tests
  tests/acceptance: Tagging tests with "cpu:VALUE"

 docs/devel/testing.rst                     |  8 ++++++++
 tests/acceptance/avocado_qemu/__init__.py  |  4 ++++
 tests/acceptance/boot_linux.py             |  3 ---
 tests/acceptance/boot_linux_console.py     | 16 +++++++++-------
 tests/acceptance/machine_mips_malta.py     |  7 +++----
 tests/acceptance/pc_cpu_hotplug_props.py   |  2 +-
 tests/acceptance/replay_kernel.py          | 17 +++++++++--------
 tests/acceptance/reverse_debugging.py      |  2 +-
 tests/acceptance/tcg_plugins.py            | 15 +++++++--------
 tests/acceptance/virtio-gpu.py             |  4 ++--
 tests/acceptance/x86_cpu_model_versions.py |  8 ++++++++
 11 files changed, 52 insertions(+), 34 deletions(-)

-- 
2.29.2

[PATCH 1/3] tests/acceptance: Automatic set -cpu to the test vm

[Wainer dos Santos Moschetta]

This introduces a new feature to the functional tests: automatic setting of
the '-cpu VALUE' option to the created vm if the test is tagged with
'cpu:VALUE'. The 'cpu' property is made available to the test object as well.

For example, for a simple test as:

    def test(self):
        """
        :avocado: tags=cpu:host
        """
        self.assertEqual(self.cpu, "host")
        self.vm.launch()

The resulted QEMU evocation will be like:

    qemu-system-x86_64 -display none -vga none -chardev socket,id=mon,path=/var/tmp/avo_qemu_sock_pdgzbgd_/qemu-1135557-monitor.sock -mon chardev=mon,mode=control -cpu host

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

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 00ce16de48..40478672c0 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -844,6 +844,14 @@ 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=arch:VALUE`` tag, it will be set to ``VALUE``.
 
+cpu
+~~~
+
+If the test is tagged with one (and only one) ``:avocado: tags=cpu:VALUE`` tag
+then the ``cpu`` attribute will be set to ``VALUE``, and the ``-cpu`` argument
+will be set to all QEMUMachine instances created by the test. Otherwise the
+attribute will be set to ``None``.
+
 machine
 ~~~~~~~
 
diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index df167b142c..0f4649b173 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -193,6 +193,8 @@ def setUp(self):
         self.arch = self.params.get('arch',
                                     default=self._get_unique_tag_val('arch'))
 
+        self.cpu = self._get_unique_tag_val('cpu')
+
         self.machine = self.params.get('machine',
                                        default=self._get_unique_tag_val('machine'))
 
@@ -218,6 +220,8 @@ def get_vm(self, *args, name=None):
             name = str(uuid.uuid4())
         if self._vms.get(name) is None:
             self._vms[name] = self._new_vm(*args)
+            if self.cpu is not None:
+                self._vms[name].add_args('-cpu', self.cpu)
             if self.machine is not None:
                 self._vms[name].set_machine(self.machine)
         return self._vms[name]
-- 
2.29.2

Re: [PATCH 1/3] tests/acceptance: Automatic set -cpu to the test vm

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 3057 bytes --]

On Wed, Feb 24, 2021 at 06:26:52PM -0300, Wainer dos Santos Moschetta wrote:
> This introduces a new feature to the functional tests: automatic setting of
> the '-cpu VALUE' option to the created vm if the test is tagged with
> 'cpu:VALUE'. The 'cpu' property is made available to the test object as well.
> 
> For example, for a simple test as:
> 
>     def test(self):
>         """
>         :avocado: tags=cpu:host
>         """
>         self.assertEqual(self.cpu, "host")
>         self.vm.launch()
> 
> The resulted QEMU evocation will be like:
>

Maybe you meant "resulting" ?

>     qemu-system-x86_64 -display none -vga none -chardev socket,id=mon,path=/var/tmp/avo_qemu_sock_pdgzbgd_/qemu-1135557-monitor.sock -mon chardev=mon,mode=control -cpu host
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  docs/devel/testing.rst                    | 8 ++++++++
>  tests/acceptance/avocado_qemu/__init__.py | 4 ++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 00ce16de48..40478672c0 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -844,6 +844,14 @@ 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=arch:VALUE`` tag, it will be set to ``VALUE``.
>  
> +cpu
> +~~~
> +
> +If the test is tagged with one (and only one) ``:avocado: tags=cpu:VALUE`` tag
> +then the ``cpu`` attribute will be set to ``VALUE``, and the ``-cpu`` argument
> +will be set to all QEMUMachine instances created by the test. Otherwise the
> +attribute will be set to ``None``.
> +
>  machine
>  ~~~~~~~
>  
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index df167b142c..0f4649b173 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -193,6 +193,8 @@ def setUp(self):
>          self.arch = self.params.get('arch',
>                                      default=self._get_unique_tag_val('arch'))
>  
> +        self.cpu = self._get_unique_tag_val('cpu')
> +
>          self.machine = self.params.get('machine',
>                                         default=self._get_unique_tag_val('machine'))
>  
> @@ -218,6 +220,8 @@ def get_vm(self, *args, name=None):
>              name = str(uuid.uuid4())
>          if self._vms.get(name) is None:
>              self._vms[name] = self._new_vm(*args)
> +            if self.cpu is not None:
> +                self._vms[name].add_args('-cpu', self.cpu)
>              if self.machine is not None:
>                  self._vms[name].set_machine(self.machine)
>          return self._vms[name]
> -- 
> 2.29.2
> 

Did you omit the support for the "cpu" parameter on purpose?  Unless
there's a good argument against it, I'd say we should try to keep
those tags/parameters/attributes consistently available.

Cheers,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/3] tests/acceptance: Let the framework handle "cpu:VALUE" tagged tests

[Wainer dos Santos Moschetta]

The tests that are already tagged with "cpu:VALUE" don't need to add
"-cpu VALUE" to the list of arguments of the vm object because the avocado_qemu
framework is able to handle it automatically. So this adjust those tests and
ensure their cpu's VALUE are recognized by QEMU.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 tests/acceptance/boot_linux.py         |  3 ---
 tests/acceptance/machine_mips_malta.py |  7 +++----
 tests/acceptance/replay_kernel.py      |  8 +++-----
 tests/acceptance/reverse_debugging.py  |  2 +-
 tests/acceptance/tcg_plugins.py        | 15 +++++++--------
 5 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index 0d178038a0..55637d126e 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -82,7 +82,6 @@ def test_virt_tcg(self):
         """
         self.require_accelerator("tcg")
         self.vm.add_args("-accel", "tcg")
-        self.vm.add_args("-cpu", "max")
         self.vm.add_args("-machine", "virt,gic-version=2")
         self.add_common_args()
         self.launch_and_wait()
@@ -95,7 +94,6 @@ def test_virt_kvm_gicv2(self):
         """
         self.require_accelerator("kvm")
         self.vm.add_args("-accel", "kvm")
-        self.vm.add_args("-cpu", "host")
         self.vm.add_args("-machine", "virt,gic-version=2")
         self.add_common_args()
         self.launch_and_wait()
@@ -108,7 +106,6 @@ def test_virt_kvm_gicv3(self):
         """
         self.require_accelerator("kvm")
         self.vm.add_args("-accel", "kvm")
-        self.vm.add_args("-cpu", "host")
         self.vm.add_args("-machine", "virt,gic-version=3")
         self.add_common_args()
         self.launch_and_wait()
diff --git a/tests/acceptance/machine_mips_malta.py b/tests/acceptance/machine_mips_malta.py
index 7c9a4ee4d2..b67d8cb141 100644
--- a/tests/acceptance/machine_mips_malta.py
+++ b/tests/acceptance/machine_mips_malta.py
@@ -62,7 +62,6 @@ def do_test_i6400_framebuffer_logo(self, cpu_cores_count):
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
                                'clocksource=GIC console=tty0 console=ttyS0')
         self.vm.add_args('-kernel', kernel_path,
-                         '-cpu', 'I6400',
                          '-smp', '%u' % cpu_cores_count,
                          '-vga', 'std',
                          '-append', kernel_command_line)
@@ -96,7 +95,7 @@ def test_mips_malta_i6400_framebuffer_logo_1core(self):
         """
         :avocado: tags=arch:mips64el
         :avocado: tags=machine:malta
-        :avocado: tags=cpu:i6400
+        :avocado: tags=cpu:I6400
         """
         self.do_test_i6400_framebuffer_logo(1)
 
@@ -105,7 +104,7 @@ def test_mips_malta_i6400_framebuffer_logo_7cores(self):
         """
         :avocado: tags=arch:mips64el
         :avocado: tags=machine:malta
-        :avocado: tags=cpu:i6400
+        :avocado: tags=cpu:I6400
         :avocado: tags=mips:smp
         """
         self.do_test_i6400_framebuffer_logo(7)
@@ -115,7 +114,7 @@ def test_mips_malta_i6400_framebuffer_logo_8cores(self):
         """
         :avocado: tags=arch:mips64el
         :avocado: tags=machine:malta
-        :avocado: tags=cpu:i6400
+        :avocado: tags=cpu:I6400
         :avocado: tags=mips:smp
         """
         self.do_test_i6400_framebuffer_logo(8)
diff --git a/tests/acceptance/replay_kernel.py b/tests/acceptance/replay_kernel.py
index c1cb862468..6ae18485be 100644
--- a/tests/acceptance/replay_kernel.py
+++ b/tests/acceptance/replay_kernel.py
@@ -156,8 +156,7 @@ def test_aarch64_virt(self):
                                'console=ttyAMA0')
         console_pattern = 'VFS: Cannot open root device'
 
-        self.run_rr(kernel_path, kernel_command_line, console_pattern,
-                    args=('-cpu', 'cortex-a53'))
+        self.run_rr(kernel_path, kernel_command_line, console_pattern)
 
     def test_arm_virt(self):
         """
@@ -303,7 +302,7 @@ def test_ppc64_e500(self):
         tar_url = ('https://www.qemu-advent-calendar.org'
                    '/2018/download/day19.tar.xz')
         file_path = self.fetch_asset(tar_url, asset_hash=tar_hash)
-        self.do_test_advcal_2018(file_path, 'uImage', ('-cpu', 'e5500'))
+        self.do_test_advcal_2018(file_path, 'uImage')
 
     def test_ppc_g3beige(self):
         """
@@ -350,8 +349,7 @@ def test_xtensa_lx60(self):
         tar_url = ('https://www.qemu-advent-calendar.org'
                    '/2018/download/day02.tar.xz')
         file_path = self.fetch_asset(tar_url, asset_hash=tar_hash)
-        self.do_test_advcal_2018(file_path, 'santas-sleigh-ride.elf',
-                                 args=('-cpu', 'dc233c'))
+        self.do_test_advcal_2018(file_path, 'santas-sleigh-ride.elf')
 
 @skipUnless(os.getenv('AVOCADO_TIMEOUT_EXPECTED'), 'Test might timeout')
 class ReplayKernelSlow(ReplayKernelBase):
diff --git a/tests/acceptance/reverse_debugging.py b/tests/acceptance/reverse_debugging.py
index be01aca217..d2921e70c3 100644
--- a/tests/acceptance/reverse_debugging.py
+++ b/tests/acceptance/reverse_debugging.py
@@ -207,4 +207,4 @@ def test_aarch64_virt(self):
         kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
 
         self.reverse_debugging(
-            args=('-kernel', kernel_path, '-cpu', 'cortex-a53'))
+            args=('-kernel', kernel_path))
diff --git a/tests/acceptance/tcg_plugins.py b/tests/acceptance/tcg_plugins.py
index c21bf9e52a..9ca1515c3b 100644
--- a/tests/acceptance/tcg_plugins.py
+++ b/tests/acceptance/tcg_plugins.py
@@ -25,7 +25,7 @@ class PluginKernelBase(LinuxKernelTest):
     KERNEL_COMMON_COMMAND_LINE = 'printk.time=1 panic=-1 '
 
     def run_vm(self, kernel_path, kernel_command_line,
-               plugin, plugin_log, console_pattern, args):
+               plugin, plugin_log, console_pattern, args=None):
 
         vm = self.get_vm()
         vm.set_console()
@@ -68,7 +68,7 @@ def test_aarch64_virt_insn(self):
         :avocado: tags=accel:tcg
         :avocado: tags=arch:aarch64
         :avocado: tags=machine:virt
-        :avocado: tags=cpu:cortex-a57
+        :avocado: tags=cpu:cortex-a53
         """
         kernel_path = self._grab_aarch64_kernel()
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
@@ -80,8 +80,7 @@ def test_aarch64_virt_insn(self):
 
         self.run_vm(kernel_path, kernel_command_line,
                     "tests/plugin/libinsn.so", plugin_log.name,
-                    console_pattern,
-                    args=('-cpu', 'cortex-a53'))
+                    console_pattern)
 
         with plugin_log as lf, \
              mmap.mmap(lf.fileno(), 0, access=mmap.ACCESS_READ) as s:
@@ -95,7 +94,7 @@ def test_aarch64_virt_insn_icount(self):
         :avocado: tags=accel:tcg
         :avocado: tags=arch:aarch64
         :avocado: tags=machine:virt
-        :avocado: tags=cpu:cortex-a57
+        :avocado: tags=cpu:cortex-a53
         """
         kernel_path = self._grab_aarch64_kernel()
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
@@ -108,7 +107,7 @@ def test_aarch64_virt_insn_icount(self):
         self.run_vm(kernel_path, kernel_command_line,
                     "tests/plugin/libinsn.so", plugin_log.name,
                     console_pattern,
-                    args=('-cpu', 'cortex-a53', '-icount', 'shift=1'))
+                    args=('-icount', 'shift=1'))
 
         with plugin_log as lf, \
              mmap.mmap(lf.fileno(), 0, access=mmap.ACCESS_READ) as s:
@@ -121,7 +120,7 @@ def test_aarch64_virt_mem_icount(self):
         :avocado: tags=accel:tcg
         :avocado: tags=arch:aarch64
         :avocado: tags=machine:virt
-        :avocado: tags=cpu:cortex-a57
+        :avocado: tags=cpu:cortex-a53
         """
         kernel_path = self._grab_aarch64_kernel()
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
@@ -134,7 +133,7 @@ def test_aarch64_virt_mem_icount(self):
         self.run_vm(kernel_path, kernel_command_line,
                     "tests/plugin/libmem.so,arg=both", plugin_log.name,
                     console_pattern,
-                    args=('-cpu', 'cortex-a53', '-icount', 'shift=1'))
+                    args=('-icount', 'shift=1'))
 
         with plugin_log as lf, \
              mmap.mmap(lf.fileno(), 0, access=mmap.ACCESS_READ) as s:
-- 
2.29.2

Re: [PATCH 2/3] tests/acceptance: Let the framework handle "cpu:VALUE" tagged tests

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 9511 bytes --]

On Wed, Feb 24, 2021 at 06:26:53PM -0300, Wainer dos Santos Moschetta wrote:
> The tests that are already tagged with "cpu:VALUE" don't need to add
> "-cpu VALUE" to the list of arguments of the vm object because the avocado_qemu
> framework is able to handle it automatically. So this adjust those tests and
> ensure their cpu's VALUE are recognized by QEMU.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  tests/acceptance/boot_linux.py         |  3 ---
>  tests/acceptance/machine_mips_malta.py |  7 +++----
>  tests/acceptance/replay_kernel.py      |  8 +++-----
>  tests/acceptance/reverse_debugging.py  |  2 +-
>  tests/acceptance/tcg_plugins.py        | 15 +++++++--------
>  5 files changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
> index 0d178038a0..55637d126e 100644
> --- a/tests/acceptance/boot_linux.py
> +++ b/tests/acceptance/boot_linux.py
> @@ -82,7 +82,6 @@ def test_virt_tcg(self):
>          """
>          self.require_accelerator("tcg")
>          self.vm.add_args("-accel", "tcg")
> -        self.vm.add_args("-cpu", "max")
>          self.vm.add_args("-machine", "virt,gic-version=2")
>          self.add_common_args()
>          self.launch_and_wait()
> @@ -95,7 +94,6 @@ def test_virt_kvm_gicv2(self):
>          """
>          self.require_accelerator("kvm")
>          self.vm.add_args("-accel", "kvm")
> -        self.vm.add_args("-cpu", "host")
>          self.vm.add_args("-machine", "virt,gic-version=2")
>          self.add_common_args()
>          self.launch_and_wait()
> @@ -108,7 +106,6 @@ def test_virt_kvm_gicv3(self):
>          """
>          self.require_accelerator("kvm")
>          self.vm.add_args("-accel", "kvm")
> -        self.vm.add_args("-cpu", "host")
>          self.vm.add_args("-machine", "virt,gic-version=3")
>          self.add_common_args()
>          self.launch_and_wait()
> diff --git a/tests/acceptance/machine_mips_malta.py b/tests/acceptance/machine_mips_malta.py
> index 7c9a4ee4d2..b67d8cb141 100644
> --- a/tests/acceptance/machine_mips_malta.py
> +++ b/tests/acceptance/machine_mips_malta.py
> @@ -62,7 +62,6 @@ def do_test_i6400_framebuffer_logo(self, cpu_cores_count):
>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
>                                 'clocksource=GIC console=tty0 console=ttyS0')
>          self.vm.add_args('-kernel', kernel_path,
> -                         '-cpu', 'I6400',
>                           '-smp', '%u' % cpu_cores_count,
>                           '-vga', 'std',
>                           '-append', kernel_command_line)
> @@ -96,7 +95,7 @@ def test_mips_malta_i6400_framebuffer_logo_1core(self):
>          """
>          :avocado: tags=arch:mips64el
>          :avocado: tags=machine:malta
> -        :avocado: tags=cpu:i6400
> +        :avocado: tags=cpu:I6400

This is actually a fix in itself, as the CPU model is indeed case
sensitive:

  $ ./qemu-system-mips64el -cpu i6400
  qemu-system-mips64el: unable to find CPU model 'i6400'

I'd put it in a separate patch.

>          """
>          self.do_test_i6400_framebuffer_logo(1)
>  
> @@ -105,7 +104,7 @@ def test_mips_malta_i6400_framebuffer_logo_7cores(self):
>          """
>          :avocado: tags=arch:mips64el
>          :avocado: tags=machine:malta
> -        :avocado: tags=cpu:i6400
> +        :avocado: tags=cpu:I6400
>          :avocado: tags=mips:smp
>          """
>          self.do_test_i6400_framebuffer_logo(7)
> @@ -115,7 +114,7 @@ def test_mips_malta_i6400_framebuffer_logo_8cores(self):
>          """
>          :avocado: tags=arch:mips64el
>          :avocado: tags=machine:malta
> -        :avocado: tags=cpu:i6400
> +        :avocado: tags=cpu:I6400
>          :avocado: tags=mips:smp
>          """
>          self.do_test_i6400_framebuffer_logo(8)
> diff --git a/tests/acceptance/replay_kernel.py b/tests/acceptance/replay_kernel.py
> index c1cb862468..6ae18485be 100644
> --- a/tests/acceptance/replay_kernel.py
> +++ b/tests/acceptance/replay_kernel.py
> @@ -156,8 +156,7 @@ def test_aarch64_virt(self):
>                                 'console=ttyAMA0')
>          console_pattern = 'VFS: Cannot open root device'
>  
> -        self.run_rr(kernel_path, kernel_command_line, console_pattern,
> -                    args=('-cpu', 'cortex-a53'))
> +        self.run_rr(kernel_path, kernel_command_line, console_pattern)
>  
>      def test_arm_virt(self):
>          """
> @@ -303,7 +302,7 @@ def test_ppc64_e500(self):
>          tar_url = ('https://www.qemu-advent-calendar.org'
>                     '/2018/download/day19.tar.xz')
>          file_path = self.fetch_asset(tar_url, asset_hash=tar_hash)
> -        self.do_test_advcal_2018(file_path, 'uImage', ('-cpu', 'e5500'))
> +        self.do_test_advcal_2018(file_path, 'uImage')
>  
>      def test_ppc_g3beige(self):
>          """
> @@ -350,8 +349,7 @@ def test_xtensa_lx60(self):
>          tar_url = ('https://www.qemu-advent-calendar.org'
>                     '/2018/download/day02.tar.xz')
>          file_path = self.fetch_asset(tar_url, asset_hash=tar_hash)
> -        self.do_test_advcal_2018(file_path, 'santas-sleigh-ride.elf',
> -                                 args=('-cpu', 'dc233c'))
> +        self.do_test_advcal_2018(file_path, 'santas-sleigh-ride.elf')
>  
>  @skipUnless(os.getenv('AVOCADO_TIMEOUT_EXPECTED'), 'Test might timeout')
>  class ReplayKernelSlow(ReplayKernelBase):
> diff --git a/tests/acceptance/reverse_debugging.py b/tests/acceptance/reverse_debugging.py
> index be01aca217..d2921e70c3 100644
> --- a/tests/acceptance/reverse_debugging.py
> +++ b/tests/acceptance/reverse_debugging.py
> @@ -207,4 +207,4 @@ def test_aarch64_virt(self):
>          kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
>  
>          self.reverse_debugging(
> -            args=('-kernel', kernel_path, '-cpu', 'cortex-a53'))
> +            args=('-kernel', kernel_path))
> diff --git a/tests/acceptance/tcg_plugins.py b/tests/acceptance/tcg_plugins.py
> index c21bf9e52a..9ca1515c3b 100644
> --- a/tests/acceptance/tcg_plugins.py
> +++ b/tests/acceptance/tcg_plugins.py
> @@ -25,7 +25,7 @@ class PluginKernelBase(LinuxKernelTest):
>      KERNEL_COMMON_COMMAND_LINE = 'printk.time=1 panic=-1 '
>  
>      def run_vm(self, kernel_path, kernel_command_line,
> -               plugin, plugin_log, console_pattern, args):
> +               plugin, plugin_log, console_pattern, args=None):
>  
>          vm = self.get_vm()
>          vm.set_console()
> @@ -68,7 +68,7 @@ def test_aarch64_virt_insn(self):
>          :avocado: tags=accel:tcg
>          :avocado: tags=arch:aarch64
>          :avocado: tags=machine:virt
> -        :avocado: tags=cpu:cortex-a57
> +        :avocado: tags=cpu:cortex-a53

Another good catch, another fix that deserve to be split out of this
patch IMO.

Also, I'd double check with Alex if it makes any difference picking
a53 or a57 for this test case.

>          """
>          kernel_path = self._grab_aarch64_kernel()
>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
> @@ -80,8 +80,7 @@ def test_aarch64_virt_insn(self):
>  
>          self.run_vm(kernel_path, kernel_command_line,
>                      "tests/plugin/libinsn.so", plugin_log.name,
> -                    console_pattern,
> -                    args=('-cpu', 'cortex-a53'))
> +                    console_pattern)
>  
>          with plugin_log as lf, \
>               mmap.mmap(lf.fileno(), 0, access=mmap.ACCESS_READ) as s:
> @@ -95,7 +94,7 @@ def test_aarch64_virt_insn_icount(self):
>          :avocado: tags=accel:tcg
>          :avocado: tags=arch:aarch64
>          :avocado: tags=machine:virt
> -        :avocado: tags=cpu:cortex-a57
> +        :avocado: tags=cpu:cortex-a53
>          """
>          kernel_path = self._grab_aarch64_kernel()
>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
> @@ -108,7 +107,7 @@ def test_aarch64_virt_insn_icount(self):
>          self.run_vm(kernel_path, kernel_command_line,
>                      "tests/plugin/libinsn.so", plugin_log.name,
>                      console_pattern,
> -                    args=('-cpu', 'cortex-a53', '-icount', 'shift=1'))
> +                    args=('-icount', 'shift=1'))
>  
>          with plugin_log as lf, \
>               mmap.mmap(lf.fileno(), 0, access=mmap.ACCESS_READ) as s:
> @@ -121,7 +120,7 @@ def test_aarch64_virt_mem_icount(self):
>          :avocado: tags=accel:tcg
>          :avocado: tags=arch:aarch64
>          :avocado: tags=machine:virt
> -        :avocado: tags=cpu:cortex-a57
> +        :avocado: tags=cpu:cortex-a53
>          """
>          kernel_path = self._grab_aarch64_kernel()
>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
> @@ -134,7 +133,7 @@ def test_aarch64_virt_mem_icount(self):
>          self.run_vm(kernel_path, kernel_command_line,
>                      "tests/plugin/libmem.so,arg=both", plugin_log.name,
>                      console_pattern,
> -                    args=('-cpu', 'cortex-a53', '-icount', 'shift=1'))
> +                    args=('-icount', 'shift=1'))
>  
>          with plugin_log as lf, \
>               mmap.mmap(lf.fileno(), 0, access=mmap.ACCESS_READ) as s:
> -- 
> 2.29.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] tests/acceptance: Tagging tests with "cpu:VALUE"

[Wainer dos Santos Moschetta]

The existing tests which are passing "-cpu VALUE" argument to the vm object
are now properly "cpu:VALUE" tagged, so letting the avocado_qemu framework to
handle that automatically.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 tests/acceptance/boot_linux_console.py     | 16 +++++++++-------
 tests/acceptance/pc_cpu_hotplug_props.py   |  2 +-
 tests/acceptance/replay_kernel.py          |  9 ++++++---
 tests/acceptance/virtio-gpu.py             |  4 ++--
 tests/acceptance/x86_cpu_model_versions.py |  8 ++++++++
 5 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index eb01286799..2447b370ff 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -238,6 +238,7 @@ def test_mips64el_malta_5KEc_cpio(self):
         :avocado: tags=arch:mips64el
         :avocado: tags=machine:malta
         :avocado: tags=endian:little
+        :avocado: tags=cpu:5KEc
         """
         kernel_url = ('https://github.com/philmd/qemu-testing-blob/'
                       'raw/9ad2df38/mips/malta/mips64el/'
@@ -257,8 +258,7 @@ def test_mips64el_malta_5KEc_cpio(self):
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE
                                + 'console=ttyS0 console=tty '
                                + 'rdinit=/sbin/init noreboot')
-        self.vm.add_args('-cpu', '5KEc',
-                         '-kernel', kernel_path,
+        self.vm.add_args('-kernel', kernel_path,
                          '-initrd', initrd_path,
                          '-append', kernel_command_line,
                          '-no-reboot')
@@ -286,7 +286,6 @@ def do_test_mips_malta32el_nanomips(self, kernel_url, kernel_hash):
                                + 'mem=256m@@0x0 '
                                + 'console=ttyS0')
         self.vm.add_args('-no-reboot',
-                         '-cpu', 'I7200',
                          '-kernel', kernel_path,
                          '-append', kernel_command_line)
         self.vm.launch()
@@ -298,6 +297,7 @@ def test_mips_malta32el_nanomips_4k(self):
         :avocado: tags=arch:mipsel
         :avocado: tags=machine:malta
         :avocado: tags=endian:little
+        :avocado: tags=cpu:I7200
         """
         kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
@@ -310,6 +310,7 @@ def test_mips_malta32el_nanomips_16k_up(self):
         :avocado: tags=arch:mipsel
         :avocado: tags=machine:malta
         :avocado: tags=endian:little
+        :avocado: tags=cpu:I7200
         """
         kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
@@ -322,6 +323,7 @@ def test_mips_malta32el_nanomips_64k_dbg(self):
         :avocado: tags=arch:mipsel
         :avocado: tags=machine:malta
         :avocado: tags=endian:little
+        :avocado: tags=cpu:I7200
         """
         kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
@@ -333,6 +335,7 @@ def test_aarch64_virt(self):
         """
         :avocado: tags=arch:aarch64
         :avocado: tags=machine:virt
+        :avocado: tags=cpu:cortex-a53
         """
         kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
                       '/linux/releases/29/Everything/aarch64/os/images/pxeboot'
@@ -343,8 +346,7 @@ def test_aarch64_virt(self):
         self.vm.set_console()
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
                                'console=ttyAMA0')
-        self.vm.add_args('-cpu', 'cortex-a53',
-                         '-kernel', kernel_path,
+        self.vm.add_args('-kernel', kernel_path,
                          '-append', kernel_command_line)
         self.vm.launch()
         console_pattern = 'Kernel command line: %s' % kernel_command_line
@@ -1076,9 +1078,9 @@ def test_ppc64_e500(self):
         """
         :avocado: tags=arch:ppc64
         :avocado: tags=machine:ppce500
+        :avocado: tags=cpu:e5500
         """
         tar_hash = '6951d86d644b302898da2fd701739c9406527fe1'
-        self.vm.add_args('-cpu', 'e5500')
         self.do_test_advcal_2018('19', tar_hash, 'uImage')
 
     def test_ppc_g3beige(self):
@@ -1120,7 +1122,7 @@ def test_xtensa_lx60(self):
         """
         :avocado: tags=arch:xtensa
         :avocado: tags=machine:lx60
+        :avocado: tags=cpu:dc233c
         """
         tar_hash = '49e88d9933742f0164b60839886c9739cb7a0d34'
-        self.vm.add_args('-cpu', 'dc233c')
         self.do_test_advcal_2018('02', tar_hash, 'santas-sleigh-ride.elf')
diff --git a/tests/acceptance/pc_cpu_hotplug_props.py b/tests/acceptance/pc_cpu_hotplug_props.py
index e49bf33fc5..f8a39e6d0a 100644
--- a/tests/acceptance/pc_cpu_hotplug_props.py
+++ b/tests/acceptance/pc_cpu_hotplug_props.py
@@ -25,11 +25,11 @@
 class OmittedCPUProps(Test):
     """
     :avocado: tags=arch:x86_64
+    :avocado: tags=cpu:qemu64
     """
     def test_no_die_id(self):
         self.vm.add_args('-nodefaults', '-S')
         self.vm.add_args('-smp', '1,sockets=2,cores=2,threads=2,maxcpus=8')
-        self.vm.add_args('-cpu', 'qemu64')
         self.vm.add_args('-device', 'qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0')
         self.vm.launch()
         self.assertEquals(len(self.vm.command('query-cpus')), 2)
diff --git a/tests/acceptance/replay_kernel.py b/tests/acceptance/replay_kernel.py
index 6ae18485be..fefa6d8550 100644
--- a/tests/acceptance/replay_kernel.py
+++ b/tests/acceptance/replay_kernel.py
@@ -394,6 +394,7 @@ def test_mips64el_malta_5KEc_cpio(self):
         :avocado: tags=machine:malta
         :avocado: tags=endian:little
         :avocado: tags=slowness:high
+        :avocado: tags=cpu:5KEc
         """
         kernel_url = ('https://github.com/philmd/qemu-testing-blob/'
                       'raw/9ad2df38/mips/malta/mips64el/'
@@ -414,7 +415,7 @@ def test_mips64el_malta_5KEc_cpio(self):
                                'rdinit=/sbin/init noreboot')
         console_pattern = 'Boot successful.'
         self.run_rr(kernel_path, kernel_command_line, console_pattern, shift=5,
-                    args=('-initrd', initrd_path, '-cpu', '5KEc'))
+                    args=('-initrd', initrd_path))
 
     def do_test_mips_malta32el_nanomips(self, kernel_path_xz):
         kernel_path = self.workdir + "kernel"
@@ -426,14 +427,14 @@ def do_test_mips_malta32el_nanomips(self, kernel_path_xz):
                                'mem=256m@@0x0 '
                                'console=ttyS0')
         console_pattern = 'Kernel command line: %s' % kernel_command_line
-        self.run_rr(kernel_path, kernel_command_line, console_pattern, shift=5,
-                    args=('-cpu', 'I7200'))
+        self.run_rr(kernel_path, kernel_command_line, console_pattern, shift=5)
 
     def test_mips_malta32el_nanomips_4k(self):
         """
         :avocado: tags=arch:mipsel
         :avocado: tags=machine:malta
         :avocado: tags=endian:little
+        :avocado: tags=cpu:I7200
         """
         kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
@@ -447,6 +448,7 @@ def test_mips_malta32el_nanomips_16k_up(self):
         :avocado: tags=arch:mipsel
         :avocado: tags=machine:malta
         :avocado: tags=endian:little
+        :avocado: tags=cpu:I7200
         """
         kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
@@ -460,6 +462,7 @@ def test_mips_malta32el_nanomips_64k_dbg(self):
         :avocado: tags=arch:mipsel
         :avocado: tags=machine:malta
         :avocado: tags=endian:little
+        :avocado: tags=cpu:I7200
         """
         kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
                       'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py
index ab1a4c1a71..c650427379 100644
--- a/tests/acceptance/virtio-gpu.py
+++ b/tests/acceptance/virtio-gpu.py
@@ -60,6 +60,7 @@ def test_virtio_vga_virgl(self):
         """
         :avocado: tags=arch:x86_64
         :avocado: tags=device:virtio-vga
+        :avocado: tags=cpu:host
         """
         kernel_command_line = (
             self.KERNEL_COMMON_COMMAND_LINE + "console=ttyS0 rdinit=/bin/bash"
@@ -72,7 +73,6 @@ def test_virtio_vga_virgl(self):
         initrd_path = self.fetch_asset(self.INITRD_URL)
 
         self.vm.set_console()
-        self.vm.add_args("-cpu", "host")
         self.vm.add_args("-m", "2G")
         self.vm.add_args("-machine", "pc,accel=kvm")
         self.vm.add_args("-device", "virtio-vga,virgl=on")
@@ -96,6 +96,7 @@ def test_vhost_user_vga_virgl(self):
         """
         :avocado: tags=arch:x86_64
         :avocado: tags=device:vhost-user-vga
+        :avocado: tags=cpu:host
         """
         kernel_command_line = (
             self.KERNEL_COMMON_COMMAND_LINE + "console=ttyS0 rdinit=/bin/bash"
@@ -135,7 +136,6 @@ def test_vhost_user_vga_virgl(self):
         )
 
         self.vm.set_console()
-        self.vm.add_args("-cpu", "host")
         self.vm.add_args("-m", "2G")
         self.vm.add_args("-object", "memory-backend-memfd,id=mem,size=2G")
         self.vm.add_args("-machine", "pc,memory-backend=mem,accel=kvm")
diff --git a/tests/acceptance/x86_cpu_model_versions.py b/tests/acceptance/x86_cpu_model_versions.py
index 2b7461bb41..8a0a07ef71 100644
--- a/tests/acceptance/x86_cpu_model_versions.py
+++ b/tests/acceptance/x86_cpu_model_versions.py
@@ -252,6 +252,7 @@ def get_cpu_prop(self, prop):
     def test_4_1(self):
         """
         :avocado: tags=machine:pc-i440fx-4.1
+        :avocado: tags=cpu:Cascadelake-Server
         """
         # machine-type only:
         self.vm.add_args('-S')
@@ -263,6 +264,7 @@ def test_4_1(self):
     def test_4_0(self):
         """
         :avocado: tags=machine:pc-i440fx-4.0
+        :avocado: tags=cpu:Cascadelake-Server
         """
         self.vm.add_args('-S')
         self.vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off')
@@ -273,6 +275,7 @@ def test_4_0(self):
     def test_set_4_0(self):
         """
         :avocado: tags=machine:pc-i440fx-4.0
+        :avocado: tags=cpu:Cascadelake-Server
         """
         # command line must override machine-type if CPU model is not versioned:
         self.vm.add_args('-S')
@@ -284,6 +287,7 @@ def test_set_4_0(self):
     def test_unset_4_1(self):
         """
         :avocado: tags=machine:pc-i440fx-4.1
+        :avocado: tags=cpu:Cascadelake-Server
         """
         self.vm.add_args('-S')
         self.vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off,-arch-capabilities')
@@ -294,6 +298,7 @@ def test_unset_4_1(self):
     def test_v1_4_0(self):
         """
         :avocado: tags=machine:pc-i440fx-4.0
+        :avocado: tags=cpu:Cascadelake-Server
         """
         # versioned CPU model overrides machine-type:
         self.vm.add_args('-S')
@@ -305,6 +310,7 @@ def test_v1_4_0(self):
     def test_v2_4_0(self):
         """
         :avocado: tags=machine:pc-i440fx-4.0
+        :avocado: tags=cpu:Cascadelake-Server
         """
         self.vm.add_args('-S')
         self.vm.add_args('-cpu', 'Cascadelake-Server-v2,x-force-features=on,check=off,enforce=off')
@@ -315,6 +321,7 @@ def test_v2_4_0(self):
     def test_v1_set_4_0(self):
         """
         :avocado: tags=machine:pc-i440fx-4.0
+        :avocado: tags=cpu:Cascadelake-Server
         """
         # command line must override machine-type and versioned CPU model:
         self.vm.add_args('-S')
@@ -326,6 +333,7 @@ def test_v1_set_4_0(self):
     def test_v2_unset_4_1(self):
         """
         :avocado: tags=machine:pc-i440fx-4.1
+        :avocado: tags=cpu:Cascadelake-Server
         """
         self.vm.add_args('-S')
         self.vm.add_args('-cpu', 'Cascadelake-Server-v2,x-force-features=on,check=off,enforce=off,-arch-capabilities')
-- 
2.29.2

Re: [PATCH 3/3] tests/acceptance: Tagging tests with "cpu:VALUE"

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 14226 bytes --]

On Wed, Feb 24, 2021 at 06:26:54PM -0300, Wainer dos Santos Moschetta wrote:
> The existing tests which are passing "-cpu VALUE" argument to the vm object
> are now properly "cpu:VALUE" tagged, so letting the avocado_qemu framework to
> handle that automatically.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  tests/acceptance/boot_linux_console.py     | 16 +++++++++-------
>  tests/acceptance/pc_cpu_hotplug_props.py   |  2 +-
>  tests/acceptance/replay_kernel.py          |  9 ++++++---
>  tests/acceptance/virtio-gpu.py             |  4 ++--
>  tests/acceptance/x86_cpu_model_versions.py |  8 ++++++++
>  5 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index eb01286799..2447b370ff 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -238,6 +238,7 @@ def test_mips64el_malta_5KEc_cpio(self):
>          :avocado: tags=arch:mips64el
>          :avocado: tags=machine:malta
>          :avocado: tags=endian:little
> +        :avocado: tags=cpu:5KEc
>          """
>          kernel_url = ('https://github.com/philmd/qemu-testing-blob/'
>                        'raw/9ad2df38/mips/malta/mips64el/'
> @@ -257,8 +258,7 @@ def test_mips64el_malta_5KEc_cpio(self):
>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE
>                                 + 'console=ttyS0 console=tty '
>                                 + 'rdinit=/sbin/init noreboot')
> -        self.vm.add_args('-cpu', '5KEc',
> -                         '-kernel', kernel_path,
> +        self.vm.add_args('-kernel', kernel_path,
>                           '-initrd', initrd_path,
>                           '-append', kernel_command_line,
>                           '-no-reboot')
> @@ -286,7 +286,6 @@ def do_test_mips_malta32el_nanomips(self, kernel_url, kernel_hash):
>                                 + 'mem=256m@@0x0 '
>                                 + 'console=ttyS0')
>          self.vm.add_args('-no-reboot',
> -                         '-cpu', 'I7200',
>                           '-kernel', kernel_path,
>                           '-append', kernel_command_line)
>          self.vm.launch()
> @@ -298,6 +297,7 @@ def test_mips_malta32el_nanomips_4k(self):
>          :avocado: tags=arch:mipsel
>          :avocado: tags=machine:malta
>          :avocado: tags=endian:little
> +        :avocado: tags=cpu:I7200
>          """
>          kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
>                        'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
> @@ -310,6 +310,7 @@ def test_mips_malta32el_nanomips_16k_up(self):
>          :avocado: tags=arch:mipsel
>          :avocado: tags=machine:malta
>          :avocado: tags=endian:little
> +        :avocado: tags=cpu:I7200
>          """
>          kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
>                        'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
> @@ -322,6 +323,7 @@ def test_mips_malta32el_nanomips_64k_dbg(self):
>          :avocado: tags=arch:mipsel
>          :avocado: tags=machine:malta
>          :avocado: tags=endian:little
> +        :avocado: tags=cpu:I7200
>          """
>          kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
>                        'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
> @@ -333,6 +335,7 @@ def test_aarch64_virt(self):
>          """
>          :avocado: tags=arch:aarch64
>          :avocado: tags=machine:virt
> +        :avocado: tags=cpu:cortex-a53
>          """
>          kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
>                        '/linux/releases/29/Everything/aarch64/os/images/pxeboot'
> @@ -343,8 +346,7 @@ def test_aarch64_virt(self):
>          self.vm.set_console()
>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
>                                 'console=ttyAMA0')
> -        self.vm.add_args('-cpu', 'cortex-a53',
> -                         '-kernel', kernel_path,
> +        self.vm.add_args('-kernel', kernel_path,
>                           '-append', kernel_command_line)
>          self.vm.launch()
>          console_pattern = 'Kernel command line: %s' % kernel_command_line
> @@ -1076,9 +1078,9 @@ def test_ppc64_e500(self):
>          """
>          :avocado: tags=arch:ppc64
>          :avocado: tags=machine:ppce500
> +        :avocado: tags=cpu:e5500
>          """
>          tar_hash = '6951d86d644b302898da2fd701739c9406527fe1'
> -        self.vm.add_args('-cpu', 'e5500')
>          self.do_test_advcal_2018('19', tar_hash, 'uImage')
>  
>      def test_ppc_g3beige(self):
> @@ -1120,7 +1122,7 @@ def test_xtensa_lx60(self):
>          """
>          :avocado: tags=arch:xtensa
>          :avocado: tags=machine:lx60
> +        :avocado: tags=cpu:dc233c
>          """
>          tar_hash = '49e88d9933742f0164b60839886c9739cb7a0d34'
> -        self.vm.add_args('-cpu', 'dc233c')
>          self.do_test_advcal_2018('02', tar_hash, 'santas-sleigh-ride.elf')
> diff --git a/tests/acceptance/pc_cpu_hotplug_props.py b/tests/acceptance/pc_cpu_hotplug_props.py
> index e49bf33fc5..f8a39e6d0a 100644
> --- a/tests/acceptance/pc_cpu_hotplug_props.py
> +++ b/tests/acceptance/pc_cpu_hotplug_props.py
> @@ -25,11 +25,11 @@
>  class OmittedCPUProps(Test):
>      """
>      :avocado: tags=arch:x86_64
> +    :avocado: tags=cpu:qemu64
>      """
>      def test_no_die_id(self):
>          self.vm.add_args('-nodefaults', '-S')
>          self.vm.add_args('-smp', '1,sockets=2,cores=2,threads=2,maxcpus=8')
> -        self.vm.add_args('-cpu', 'qemu64')
>          self.vm.add_args('-device', 'qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0')
>          self.vm.launch()
>          self.assertEquals(len(self.vm.command('query-cpus')), 2)
> diff --git a/tests/acceptance/replay_kernel.py b/tests/acceptance/replay_kernel.py
> index 6ae18485be..fefa6d8550 100644
> --- a/tests/acceptance/replay_kernel.py
> +++ b/tests/acceptance/replay_kernel.py
> @@ -394,6 +394,7 @@ def test_mips64el_malta_5KEc_cpio(self):
>          :avocado: tags=machine:malta
>          :avocado: tags=endian:little
>          :avocado: tags=slowness:high
> +        :avocado: tags=cpu:5KEc
>          """
>          kernel_url = ('https://github.com/philmd/qemu-testing-blob/'
>                        'raw/9ad2df38/mips/malta/mips64el/'
> @@ -414,7 +415,7 @@ def test_mips64el_malta_5KEc_cpio(self):
>                                 'rdinit=/sbin/init noreboot')
>          console_pattern = 'Boot successful.'
>          self.run_rr(kernel_path, kernel_command_line, console_pattern, shift=5,
> -                    args=('-initrd', initrd_path, '-cpu', '5KEc'))
> +                    args=('-initrd', initrd_path))
>  
>      def do_test_mips_malta32el_nanomips(self, kernel_path_xz):
>          kernel_path = self.workdir + "kernel"
> @@ -426,14 +427,14 @@ def do_test_mips_malta32el_nanomips(self, kernel_path_xz):
>                                 'mem=256m@@0x0 '
>                                 'console=ttyS0')
>          console_pattern = 'Kernel command line: %s' % kernel_command_line
> -        self.run_rr(kernel_path, kernel_command_line, console_pattern, shift=5,
> -                    args=('-cpu', 'I7200'))
> +        self.run_rr(kernel_path, kernel_command_line, console_pattern, shift=5)
>  
>      def test_mips_malta32el_nanomips_4k(self):
>          """
>          :avocado: tags=arch:mipsel
>          :avocado: tags=machine:malta
>          :avocado: tags=endian:little
> +        :avocado: tags=cpu:I7200
>          """
>          kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
>                        'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
> @@ -447,6 +448,7 @@ def test_mips_malta32el_nanomips_16k_up(self):
>          :avocado: tags=arch:mipsel
>          :avocado: tags=machine:malta
>          :avocado: tags=endian:little
> +        :avocado: tags=cpu:I7200
>          """
>          kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
>                        'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
> @@ -460,6 +462,7 @@ def test_mips_malta32el_nanomips_64k_dbg(self):
>          :avocado: tags=arch:mipsel
>          :avocado: tags=machine:malta
>          :avocado: tags=endian:little
> +        :avocado: tags=cpu:I7200
>          """
>          kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
>                        'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
> diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py
> index ab1a4c1a71..c650427379 100644
> --- a/tests/acceptance/virtio-gpu.py
> +++ b/tests/acceptance/virtio-gpu.py
> @@ -60,6 +60,7 @@ def test_virtio_vga_virgl(self):
>          """
>          :avocado: tags=arch:x86_64
>          :avocado: tags=device:virtio-vga
> +        :avocado: tags=cpu:host
>          """
>          kernel_command_line = (
>              self.KERNEL_COMMON_COMMAND_LINE + "console=ttyS0 rdinit=/bin/bash"
> @@ -72,7 +73,6 @@ def test_virtio_vga_virgl(self):
>          initrd_path = self.fetch_asset(self.INITRD_URL)
>  
>          self.vm.set_console()
> -        self.vm.add_args("-cpu", "host")
>          self.vm.add_args("-m", "2G")
>          self.vm.add_args("-machine", "pc,accel=kvm")
>          self.vm.add_args("-device", "virtio-vga,virgl=on")
> @@ -96,6 +96,7 @@ def test_vhost_user_vga_virgl(self):
>          """
>          :avocado: tags=arch:x86_64
>          :avocado: tags=device:vhost-user-vga
> +        :avocado: tags=cpu:host
>          """
>          kernel_command_line = (
>              self.KERNEL_COMMON_COMMAND_LINE + "console=ttyS0 rdinit=/bin/bash"
> @@ -135,7 +136,6 @@ def test_vhost_user_vga_virgl(self):
>          )
>  
>          self.vm.set_console()
> -        self.vm.add_args("-cpu", "host")
>          self.vm.add_args("-m", "2G")
>          self.vm.add_args("-object", "memory-backend-memfd,id=mem,size=2G")
>          self.vm.add_args("-machine", "pc,memory-backend=mem,accel=kvm")
> diff --git a/tests/acceptance/x86_cpu_model_versions.py b/tests/acceptance/x86_cpu_model_versions.py
> index 2b7461bb41..8a0a07ef71 100644
> --- a/tests/acceptance/x86_cpu_model_versions.py
> +++ b/tests/acceptance/x86_cpu_model_versions.py
> @@ -252,6 +252,7 @@ def get_cpu_prop(self, prop):
>      def test_4_1(self):
>          """
>          :avocado: tags=machine:pc-i440fx-4.1
> +        :avocado: tags=cpu:Cascadelake-Server
>          """
>          # machine-type only:
>          self.vm.add_args('-S')
> @@ -263,6 +264,7 @@ def test_4_1(self):
>      def test_4_0(self):
>          """
>          :avocado: tags=machine:pc-i440fx-4.0
> +        :avocado: tags=cpu:Cascadelake-Server
>          """
>          self.vm.add_args('-S')
>          self.vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off')
> @@ -273,6 +275,7 @@ def test_4_0(self):
>      def test_set_4_0(self):
>          """
>          :avocado: tags=machine:pc-i440fx-4.0
> +        :avocado: tags=cpu:Cascadelake-Server
>          """
>          # command line must override machine-type if CPU model is not versioned:
>          self.vm.add_args('-S')
> @@ -284,6 +287,7 @@ def test_set_4_0(self):
>      def test_unset_4_1(self):
>          """
>          :avocado: tags=machine:pc-i440fx-4.1
> +        :avocado: tags=cpu:Cascadelake-Server
>          """
>          self.vm.add_args('-S')
>          self.vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off,-arch-capabilities')
> @@ -294,6 +298,7 @@ def test_unset_4_1(self):
>      def test_v1_4_0(self):
>          """
>          :avocado: tags=machine:pc-i440fx-4.0
> +        :avocado: tags=cpu:Cascadelake-Server
>          """
>          # versioned CPU model overrides machine-type:
>          self.vm.add_args('-S')
> @@ -305,6 +310,7 @@ def test_v1_4_0(self):
>      def test_v2_4_0(self):
>          """
>          :avocado: tags=machine:pc-i440fx-4.0
> +        :avocado: tags=cpu:Cascadelake-Server
>          """
>          self.vm.add_args('-S')
>          self.vm.add_args('-cpu', 'Cascadelake-Server-v2,x-force-features=on,check=off,enforce=off')
> @@ -315,6 +321,7 @@ def test_v2_4_0(self):
>      def test_v1_set_4_0(self):
>          """
>          :avocado: tags=machine:pc-i440fx-4.0
> +        :avocado: tags=cpu:Cascadelake-Server
>          """
>          # command line must override machine-type and versioned CPU model:
>          self.vm.add_args('-S')
> @@ -326,6 +333,7 @@ def test_v1_set_4_0(self):
>      def test_v2_unset_4_1(self):
>          """
>          :avocado: tags=machine:pc-i440fx-4.1
> +        :avocado: tags=cpu:Cascadelake-Server
>          """
>          self.vm.add_args('-S')
>          self.vm.add_args('-cpu', 'Cascadelake-Server-v2,x-force-features=on,check=off,enforce=off,-arch-capabilities')
> -- 
> 2.29.2
> 

So these do produce duplicate '-cpu' arguments indeed:

   VM launch command: './qemu-system-x86_64 -display none -vga none
       -chardev socket,id=mon,path=/var/tmp/avo_qemu_sock_syr8rstd/qemu-2162592-monitor.sock
       -mon chardev=mon,mode=control -machine pc-i440fx-4.1
       -cpu Cascadelake-Server -S
       -cpu Cascadelake-Server,x-force-features=on,check=off,enforce=off'

Like I said elsewhere, I think we should prevent this from happen (and
it looks like it wouldn't be very hard to do so).

Also, IIUC, you left this commit to *add* tags (and consequently
remove the manual setting of the '-cpu' args.  But, on the previous
patch, because of the fixes, you still do some of that, so it's not as
easy to spot the difference in intention between this patch and the
previous one.  IMO, it's another indication that you should split out
the tag fixes.

Regards,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] tests/acceptance: Handle tests with "cpu" tag

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 2409 bytes --]

On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos Moschetta wrote:
> Currently the acceptance tests tagged with "machine" have the "-M TYPE"
> automatically added to the list of arguments of the QEMUMachine object.
> In other words, that option is passed to the launched QEMU. On this
> series it is implemented the same feature but instead for tests marked
> with "cpu".
>

Good!

> There is a caveat, however, in case the test needs additional arguments to
> the CPU type they cannot be passed via tag, because the tags parser split
> values by comma. For example, in tests/acceptance/x86_cpu_model_versions.py,
> there are cases where:
> 
>   * -cpu is set to "Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>   * if it was tagged like "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>     then the parser would break it into 4 tags ("cpu:Cascadelake-Server",
>     "x-force-features=on", "check=off", "enforce=off")
>   * resulting on "-cpu Cascadelake-Server" and the remaining arguments are ignored.
> 
> For the example above, one should tag it (or not at all) as "cpu:Cascadelake-Server"
> AND self.vm.add_args('-cpu', "Cascadelake-Server,x-force-features=on,check=off,enforce=off"),
> and that results on something like:
> 
>   "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu Cascadelake-Server,x-force-features=on,check=off,enforce=off".
>

There are clearly two problems here:

1) the tag is meant to be succinct, so that it can be used by users
   selecting which tests to run.  At the same time, it's a waste
   to throw away the other information or keep it duplicate or
   incosistent.

2) QEMUMachine doesn't keep track of command line arguments
   (add_args() makes it pretty clear what's doing).  But, on this type
   of use case, a "set_args()" is desirable, in which case it would
   overwrite the existing arguments for a given command line option.

> QEMU is going to ignore the first -cpu argument. See the patch 0003 for a reference.
>

But this would still be creating a QEMU command line with multiple
'-cpu' arguments, right?  I understand this could be useful for
testing the behavior of the parameter parsing (if that's intended) but
it's bad practice to be generating incorrect command line in tests.

Maybe just by tackling issue #2 this could be avoided.

Cheers,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] tests/acceptance: Handle tests with "cpu" tag

[Wainer dos Santos Moschetta]

Added John and Eduardo,

On 3/9/21 3:52 PM, Cleber Rosa wrote:
> On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos Moschetta wrote:
>> Currently the acceptance tests tagged with "machine" have the "-M TYPE"
>> automatically added to the list of arguments of the QEMUMachine object.
>> In other words, that option is passed to the launched QEMU. On this
>> series it is implemented the same feature but instead for tests marked
>> with "cpu".
>>
> Good!
>
>> There is a caveat, however, in case the test needs additional arguments to
>> the CPU type they cannot be passed via tag, because the tags parser split
>> values by comma. For example, in tests/acceptance/x86_cpu_model_versions.py,
>> there are cases where:
>>
>>    * -cpu is set to "Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>>    * if it was tagged like "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>>      then the parser would break it into 4 tags ("cpu:Cascadelake-Server",
>>      "x-force-features=on", "check=off", "enforce=off")
>>    * resulting on "-cpu Cascadelake-Server" and the remaining arguments are ignored.
>>
>> For the example above, one should tag it (or not at all) as "cpu:Cascadelake-Server"
>> AND self.vm.add_args('-cpu', "Cascadelake-Server,x-force-features=on,check=off,enforce=off"),
>> and that results on something like:
>>
>>    "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu Cascadelake-Server,x-force-features=on,check=off,enforce=off".
>>
> There are clearly two problems here:
>
> 1) the tag is meant to be succinct, so that it can be used by users
>     selecting which tests to run.  At the same time, it's a waste
>     to throw away the other information or keep it duplicate or
>     incosistent.
>
> 2) QEMUMachine doesn't keep track of command line arguments
>     (add_args() makes it pretty clear what's doing).  But, on this type
>     of use case, a "set_args()" is desirable, in which case it would
>     overwrite the existing arguments for a given command line option.

I like the idea of a "set_args()" to QEMUMachine as you describe above 
but it needs further discussion because I can see at least one corner 
case; for example, one can set the machine type as either -machine or 
-M, then what key it should be searched-and-replaced (if any) on the 
list of args?

Unlike your suggestion, I thought on implement the method to deal with a 
single argument at time, as:

     def set_arg(self, arg: Union[str, list], value: str) -> None:
         """
         Set the value of an argument from the list of extra arguments to be
         given to the QEMU binary. If the argument does not exist then it is
         added to the list.

         If the ``arg`` parameter is a list then it will search and 
replace all
         occurencies (if any). Otherwise a new argument is added and it is
         used the first value of the ``arg`` list.
         """
         pass

Does it sound good to you?

Thanks!

Wainer

>> QEMU is going to ignore the first -cpu argument. See the patch 0003 for a reference.
>>
> But this would still be creating a QEMU command line with multiple
> '-cpu' arguments, right?  I understand this could be useful for
> testing the behavior of the parameter parsing (if that's intended) but
> it's bad practice to be generating incorrect command line in tests.
>
> Maybe just by tackling issue #2 this could be avoided.
>
> Cheers,
> - Cleber.

Re: [PATCH 0/3] tests/acceptance: Handle tests with "cpu" tag

[John Snow]

On 3/17/21 3:16 PM, Wainer dos Santos Moschetta wrote:
> Added John and Eduardo,
> 
> On 3/9/21 3:52 PM, Cleber Rosa wrote:
>> On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos Moschetta 
>> wrote:
>>> Currently the acceptance tests tagged with "machine" have the "-M TYPE"
>>> automatically added to the list of arguments of the QEMUMachine object.
>>> In other words, that option is passed to the launched QEMU. On this
>>> series it is implemented the same feature but instead for tests marked
>>> with "cpu".
>>>
>> Good!
>>
>>> There is a caveat, however, in case the test needs additional 
>>> arguments to
>>> the CPU type they cannot be passed via tag, because the tags parser 
>>> split
>>> values by comma. For example, in 
>>> tests/acceptance/x86_cpu_model_versions.py,
>>> there are cases where:
>>>
>>>    * -cpu is set to 
>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>>>    * if it was tagged like 
>>> "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>>>      then the parser would break it into 4 tags 
>>> ("cpu:Cascadelake-Server",
>>>      "x-force-features=on", "check=off", "enforce=off")
>>>    * resulting on "-cpu Cascadelake-Server" and the remaining 
>>> arguments are ignored.
>>>
>>> For the example above, one should tag it (or not at all) as 
>>> "cpu:Cascadelake-Server"
>>> AND self.vm.add_args('-cpu', 
>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off"),
>>> and that results on something like:
>>>
>>>    "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu 
>>> Cascadelake-Server,x-force-features=on,check=off,enforce=off".
>>>
>> There are clearly two problems here:
>>
>> 1) the tag is meant to be succinct, so that it can be used by users
>>     selecting which tests to run.  At the same time, it's a waste
>>     to throw away the other information or keep it duplicate or
>>     incosistent.
>>
>> 2) QEMUMachine doesn't keep track of command line arguments
>>     (add_args() makes it pretty clear what's doing).  But, on this type
>>     of use case, a "set_args()" is desirable, in which case it would
>>     overwrite the existing arguments for a given command line option.
> 
> I like the idea of a "set_args()" to QEMUMachine as you describe above 
> but it needs further discussion because I can see at least one corner 
> case; for example, one can set the machine type as either -machine or 
> -M, then what key it should be searched-and-replaced (if any) on the 
> list of args?
> 
> Unlike your suggestion, I thought on implement the method to deal with a 
> single argument at time, as:
> 
>      def set_arg(self, arg: Union[str, list], value: str) -> None:
>          """
>          Set the value of an argument from the list of extra arguments 
> to be
>          given to the QEMU binary. If the argument does not exist then 
> it is
>          added to the list.
> 
>          If the ``arg`` parameter is a list then it will search and 
> replace all
>          occurencies (if any). Otherwise a new argument is added and it is
>          used the first value of the ``arg`` list.
>          """
>          pass
> 
> Does it sound good to you?
> 
> Thanks!
> 
> Wainer
> 

A little hokey, but I suppose that's true of our CLI interface in general.

I'd prefer not get into the business of building a "config" inside the 
python module if we can help it right now, but if "setting" individual 
args is something you truly need to do, I won't stand in the way.

Do what's least-gross.

--js

>>> QEMU is going to ignore the first -cpu argument. See the patch 0003 
>>> for a reference.
>>>
>> But this would still be creating a QEMU command line with multiple
>> '-cpu' arguments, right?  I understand this could be useful for
>> testing the behavior of the parameter parsing (if that's intended) but
>> it's bad practice to be generating incorrect command line in tests.
>>
>> Maybe just by tackling issue #2 this could be avoided.
>>
>> Cheers,
>> - Cleber.

Re: [PATCH 0/3] tests/acceptance: Handle tests with "cpu" tag

[Eduardo Habkost]

On Tue, Mar 23, 2021 at 05:01:09PM -0400, John Snow wrote:
> On 3/17/21 3:16 PM, Wainer dos Santos Moschetta wrote:
> > Added John and Eduardo,
> > 
> > On 3/9/21 3:52 PM, Cleber Rosa wrote:
> > > On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos
> > > Moschetta wrote:
> > > > Currently the acceptance tests tagged with "machine" have the "-M TYPE"
> > > > automatically added to the list of arguments of the QEMUMachine object.
> > > > In other words, that option is passed to the launched QEMU. On this
> > > > series it is implemented the same feature but instead for tests marked
> > > > with "cpu".
> > > > 
> > > Good!
> > > 
> > > > There is a caveat, however, in case the test needs additional
> > > > arguments to
> > > > the CPU type they cannot be passed via tag, because the tags
> > > > parser split
> > > > values by comma. For example, in
> > > > tests/acceptance/x86_cpu_model_versions.py,
> > > > there are cases where:
> > > > 
> > > >    * -cpu is set to
> > > > "Cascadelake-Server,x-force-features=on,check=off,enforce=off"
> > > >    * if it was tagged like
> > > > "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off"
> > > >      then the parser would break it into 4 tags
> > > > ("cpu:Cascadelake-Server",
> > > >      "x-force-features=on", "check=off", "enforce=off")
> > > >    * resulting on "-cpu Cascadelake-Server" and the remaining
> > > > arguments are ignored.
> > > > 
> > > > For the example above, one should tag it (or not at all) as
> > > > "cpu:Cascadelake-Server"
> > > > AND self.vm.add_args('-cpu',
> > > > "Cascadelake-Server,x-force-features=on,check=off,enforce=off"),
> > > > and that results on something like:
> > > > 
> > > >    "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu
> > > > Cascadelake-Server,x-force-features=on,check=off,enforce=off".
> > > > 
> > > There are clearly two problems here:
> > > 
> > > 1) the tag is meant to be succinct, so that it can be used by users
> > >     selecting which tests to run.  At the same time, it's a waste
> > >     to throw away the other information or keep it duplicate or
> > >     incosistent.
> > > 
> > > 2) QEMUMachine doesn't keep track of command line arguments
> > >     (add_args() makes it pretty clear what's doing).  But, on this type
> > >     of use case, a "set_args()" is desirable, in which case it would
> > >     overwrite the existing arguments for a given command line option.
> > 
> > I like the idea of a "set_args()" to QEMUMachine as you describe above
> > but it needs further discussion because I can see at least one corner
> > case; for example, one can set the machine type as either -machine or
> > -M, then what key it should be searched-and-replaced (if any) on the
> > list of args?
> > 
> > Unlike your suggestion, I thought on implement the method to deal with a
> > single argument at time, as:
> > 
> >      def set_arg(self, arg: Union[str, list], value: str) -> None:
> >          """
> >          Set the value of an argument from the list of extra arguments
> > to be
> >          given to the QEMU binary. If the argument does not exist then
> > it is
> >          added to the list.
> > 
> >          If the ``arg`` parameter is a list then it will search and
> > replace all
> >          occurencies (if any). Otherwise a new argument is added and it is
> >          used the first value of the ``arg`` list.
> >          """
> >          pass
> > 
> > Does it sound good to you?
> > 
> > Thanks!
> > 
> > Wainer
> > 
> 
> A little hokey, but I suppose that's true of our CLI interface in general.
> 
> I'd prefer not get into the business of building a "config" inside the
> python module if we can help it right now, but if "setting" individual args
> is something you truly need to do, I won't stand in the way.
> 
> Do what's least-gross.

I don't have any specific suggestions on how the API should look
like, but I'm having trouble understanding the documentation
above.

I don't know what "it will search and replace all occurrences"
means.  Occurrences of what?

I don't understand what "it is used the first value of the `arg`
list" means, either.  I understand you are going to use the first
value of the list, but you don't say what you are going to do
with it.

-- 
Eduardo

Re: [PATCH 0/3] tests/acceptance: Handle tests with "cpu" tag

[Wainer dos Santos Moschetta]

Hi,

On 4/7/21 5:01 PM, Eduardo Habkost wrote:
> On Tue, Mar 23, 2021 at 05:01:09PM -0400, John Snow wrote:
>> On 3/17/21 3:16 PM, Wainer dos Santos Moschetta wrote:
>>> Added John and Eduardo,
>>>
>>> On 3/9/21 3:52 PM, Cleber Rosa wrote:
>>>> On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos
>>>> Moschetta wrote:
>>>>> Currently the acceptance tests tagged with "machine" have the "-M TYPE"
>>>>> automatically added to the list of arguments of the QEMUMachine object.
>>>>> In other words, that option is passed to the launched QEMU. On this
>>>>> series it is implemented the same feature but instead for tests marked
>>>>> with "cpu".
>>>>>
>>>> Good!
>>>>
>>>>> There is a caveat, however, in case the test needs additional
>>>>> arguments to
>>>>> the CPU type they cannot be passed via tag, because the tags
>>>>> parser split
>>>>> values by comma. For example, in
>>>>> tests/acceptance/x86_cpu_model_versions.py,
>>>>> there are cases where:
>>>>>
>>>>>     * -cpu is set to
>>>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>>>>>     * if it was tagged like
>>>>> "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>>>>>       then the parser would break it into 4 tags
>>>>> ("cpu:Cascadelake-Server",
>>>>>       "x-force-features=on", "check=off", "enforce=off")
>>>>>     * resulting on "-cpu Cascadelake-Server" and the remaining
>>>>> arguments are ignored.
>>>>>
>>>>> For the example above, one should tag it (or not at all) as
>>>>> "cpu:Cascadelake-Server"
>>>>> AND self.vm.add_args('-cpu',
>>>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off"),
>>>>> and that results on something like:
>>>>>
>>>>>     "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu
>>>>> Cascadelake-Server,x-force-features=on,check=off,enforce=off".
>>>>>
>>>> There are clearly two problems here:
>>>>
>>>> 1) the tag is meant to be succinct, so that it can be used by users
>>>>      selecting which tests to run.  At the same time, it's a waste
>>>>      to throw away the other information or keep it duplicate or
>>>>      incosistent.
>>>>
>>>> 2) QEMUMachine doesn't keep track of command line arguments
>>>>      (add_args() makes it pretty clear what's doing).  But, on this type
>>>>      of use case, a "set_args()" is desirable, in which case it would
>>>>      overwrite the existing arguments for a given command line option.
>>> I like the idea of a "set_args()" to QEMUMachine as you describe above
>>> but it needs further discussion because I can see at least one corner
>>> case; for example, one can set the machine type as either -machine or
>>> -M, then what key it should be searched-and-replaced (if any) on the
>>> list of args?
>>>
>>> Unlike your suggestion, I thought on implement the method to deal with a
>>> single argument at time, as:
>>>
>>>       def set_arg(self, arg: Union[str, list], value: str) -> None:
>>>           """
>>>           Set the value of an argument from the list of extra arguments
>>> to be
>>>           given to the QEMU binary. If the argument does not exist then
>>> it is
>>>           added to the list.
>>>
>>>           If the ``arg`` parameter is a list then it will search and
>>> replace all
>>>           occurencies (if any). Otherwise a new argument is added and it is
>>>           used the first value of the ``arg`` list.
>>>           """
>>>           pass
>>>
>>> Does it sound good to you?
>>>
>>> Thanks!
>>>
>>> Wainer
>>>
>> A little hokey, but I suppose that's true of our CLI interface in general.
>>
>> I'd prefer not get into the business of building a "config" inside the
>> python module if we can help it right now, but if "setting" individual args
>> is something you truly need to do, I won't stand in the way.
>>
>> Do what's least-gross.
> I don't have any specific suggestions on how the API should look
> like, but I'm having trouble understanding the documentation
> above.
>
> I don't know what "it will search and replace all occurrences"
> means.  Occurrences of what?
>
> I don't understand what "it is used the first value of the `arg`
> list" means, either.  I understand you are going to use the first
> value of the list, but you don't say what you are going to do
> with it.


The documentation was indeed confusing but, please, disregard it. Based 
on John's comments on this thread I decided to not introduce yet another 
specialized function to QEMUMachine class. Instead I added the "args" 
property so that users will have access to QEMUMachine._args to change 
it whatever they like. You will find that implemented on the v2 of this 
series:

'[PATCH v2 0/7] tests/acceptance: Handle tests with "cpu" tag'

Thanks!

- Wainer


>

Re: [PATCH 0/3] tests/acceptance: Handle tests with "cpu" tag

[John Snow]

On 4/9/21 10:53 AM, Wainer dos Santos Moschetta wrote:
> Hi,
> 
> On 4/7/21 5:01 PM, Eduardo Habkost wrote:
>> On Tue, Mar 23, 2021 at 05:01:09PM -0400, John Snow wrote:
>>> On 3/17/21 3:16 PM, Wainer dos Santos Moschetta wrote:
>>>> Added John and Eduardo,
>>>>
>>>> On 3/9/21 3:52 PM, Cleber Rosa wrote:
>>>>> On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos
>>>>> Moschetta wrote:
>>>>>> Currently the acceptance tests tagged with "machine" have the "-M 
>>>>>> TYPE"
>>>>>> automatically added to the list of arguments of the QEMUMachine 
>>>>>> object.
>>>>>> In other words, that option is passed to the launched QEMU. On this
>>>>>> series it is implemented the same feature but instead for tests 
>>>>>> marked
>>>>>> with "cpu".
>>>>>>
>>>>> Good!
>>>>>
>>>>>> There is a caveat, however, in case the test needs additional
>>>>>> arguments to
>>>>>> the CPU type they cannot be passed via tag, because the tags
>>>>>> parser split
>>>>>> values by comma. For example, in
>>>>>> tests/acceptance/x86_cpu_model_versions.py,
>>>>>> there are cases where:
>>>>>>
>>>>>>     * -cpu is set to
>>>>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>>>>>>     * if it was tagged like
>>>>>> "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>>>>>>       then the parser would break it into 4 tags
>>>>>> ("cpu:Cascadelake-Server",
>>>>>>       "x-force-features=on", "check=off", "enforce=off")
>>>>>>     * resulting on "-cpu Cascadelake-Server" and the remaining
>>>>>> arguments are ignored.
>>>>>>
>>>>>> For the example above, one should tag it (or not at all) as
>>>>>> "cpu:Cascadelake-Server"
>>>>>> AND self.vm.add_args('-cpu',
>>>>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off"),
>>>>>> and that results on something like:
>>>>>>
>>>>>>     "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu
>>>>>> Cascadelake-Server,x-force-features=on,check=off,enforce=off".
>>>>>>
>>>>> There are clearly two problems here:
>>>>>
>>>>> 1) the tag is meant to be succinct, so that it can be used by users
>>>>>      selecting which tests to run.  At the same time, it's a waste
>>>>>      to throw away the other information or keep it duplicate or
>>>>>      incosistent.
>>>>>
>>>>> 2) QEMUMachine doesn't keep track of command line arguments
>>>>>      (add_args() makes it pretty clear what's doing).  But, on this 
>>>>> type
>>>>>      of use case, a "set_args()" is desirable, in which case it would
>>>>>      overwrite the existing arguments for a given command line option.
>>>> I like the idea of a "set_args()" to QEMUMachine as you describe above
>>>> but it needs further discussion because I can see at least one corner
>>>> case; for example, one can set the machine type as either -machine or
>>>> -M, then what key it should be searched-and-replaced (if any) on the
>>>> list of args?
>>>>
>>>> Unlike your suggestion, I thought on implement the method to deal 
>>>> with a
>>>> single argument at time, as:
>>>>
>>>>       def set_arg(self, arg: Union[str, list], value: str) -> None:
>>>>           """
>>>>           Set the value of an argument from the list of extra arguments
>>>> to be
>>>>           given to the QEMU binary. If the argument does not exist then
>>>> it is
>>>>           added to the list.
>>>>
>>>>           If the ``arg`` parameter is a list then it will search and
>>>> replace all
>>>>           occurencies (if any). Otherwise a new argument is added 
>>>> and it is
>>>>           used the first value of the ``arg`` list.
>>>>           """
>>>>           pass
>>>>
>>>> Does it sound good to you?
>>>>
>>>> Thanks!
>>>>
>>>> Wainer
>>>>
>>> A little hokey, but I suppose that's true of our CLI interface in 
>>> general.
>>>
>>> I'd prefer not get into the business of building a "config" inside the
>>> python module if we can help it right now, but if "setting" 
>>> individual args
>>> is something you truly need to do, I won't stand in the way.
>>>
>>> Do what's least-gross.
>> I don't have any specific suggestions on how the API should look
>> like, but I'm having trouble understanding the documentation
>> above.
>>
>> I don't know what "it will search and replace all occurrences"
>> means.  Occurrences of what?
>>
>> I don't understand what "it is used the first value of the `arg`
>> list" means, either.  I understand you are going to use the first
>> value of the list, but you don't say what you are going to do
>> with it.
> 
> 
> The documentation was indeed confusing but, please, disregard it. Based 
> on John's comments on this thread I decided to not introduce yet another 
> specialized function to QEMUMachine class. Instead I added the "args" 
> property so that users will have access to QEMUMachine._args to change 
> it whatever they like. You will find that implemented on the v2 of this 
> series:
> 
> '[PATCH v2 0/7] tests/acceptance: Handle tests with "cpu" tag'
> 
> Thanks!
> 
> - Wainer
> 
> 
>>

It would truly be very cool if we had a QEMUMachineConfig class that we 
could build out properly.

In the hypothetical perfect future world where we have a json-based 
config file format that mapped perfectly to QMP commands, we could 
create a class that represents loading and representing such a format, 
and allow callers to change values at runtime, like:

config.machine = q35

but this treads so absurdly close to what libvirt already does that I am 
hesitant to work on it without addressing some of the core 
organizational problems with our CLI first.

A frequent source of anguish is how we treat multiple or repeating 
values on the CLI, which we do not treat consistently. Many options use 
their own parsers and consistent behavior at the API level here requires 
a lot of special-casing.

Trying to enumerate the cases like -m/-machine and other conflicting 
things like -drive/-blockdev and so on seems difficult to get right and 
manage correctly. Smarter move is not to try, I think.

For now, it's sadly likely best that the caller simply reaches in and 
fiddles with the args according to its whims like Wainer has suggested. 
Caller knows best.

If you are seeing lots of repeated boilerplate though, feel free to add 
helper functions outside of the class for various test-users to use. 
Document their behavior rigorously.

thanks!
--js