[PATCH 0/5] machine.py fix for ppc64 tests + avocado changes

[PATCH 0/5] machine.py fix for ppc64 tests + avocado changes

[Daniel Henrique Barboza]

Hello,

This series fixes a problem I'm having when running avocado tests in an
IBM Power9 ppc64 host, without firmware modifications that breaks the
default machine options of a pseries guest running KVM, and with
--disable-tcg. The problem is described in detail in patch 02.

The proposed fix consists of checking whether we're running a pseries
guest in the launch() method of  machine.py. A simple fix, if we could
rely on the QEMUSystemTest self.machine attribute to be set all the time
a machine type is being set.

This is not the case for some tests, e.g. the empty_cpu_model.py test
that, instead of using 'tags=machine:none' to set the 'none' machine
type, it hardcodes '-machine none' via self.vm.add_args(). This doesn't
set self.machine in the QEMUSystemTest class, and then we're left
wondered whether the machine is being run with the binary defaults (i.e.
no machine were set) or if the machine were set manually and we need to
verify every -machine option to check it up. Which is not trivial by any
means: multiple machine options can be passed via avocado and  you'll
have to parse a string using all known machine types to see if it was
set or not. 

To fix the issue I needed to make the assumption that the machine type
will be set in a way that self.machine is also set (i.e. using the
avocado tag). Making this assumption for empty_cpu_model.py alone (patch
01) is enough for me to fix the issue I'm experiencing in patch 02.

I decided to make this assumption across the board in all tests that
were setting the machine type by hand instead of using an avocado tag.
So in the end, after this series, all avocado tests that sets a machine
type are now setting via "avocado: tags=machine:<type>" annotation. 


Daniel Henrique Barboza (5):
  avocado/empty_cpu_model.py: use machine:none tag
  machine.py: add default pseries params in machine.py
  avocado/multiprocess.py: use tags=machine:pc|virt
  avocado/boot_linux.py: avocado tag fixes in BootLinuxAarch64
  avocado/virtio-gpu.py: use tags=machine:pc

 python/qemu/machine/machine.py   | 13 +++++++++++++
 tests/avocado/boot_linux.py      |  7 +++----
 tests/avocado/empty_cpu_model.py |  5 ++++-
 tests/avocado/multiprocess.py    | 14 ++++++++------
 tests/avocado/virtio-gpu.py      |  6 ++++--
 5 files changed, 32 insertions(+), 13 deletions(-)

-- 
2.32.0

[PATCH 1/5] avocado/empty_cpu_model.py: use machine:none tag

[Daniel Henrique Barboza]

Using tags=machine:none will do two things: it will avoid the need to
passing '-machine none' via self.vm.add_args() and it will set the
self.machine attribute of the parent QEMUSystemTest class (via its
setUp() method).

We'll be relying on self.machine being set apropriately for an upcoming
fix.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 tests/avocado/empty_cpu_model.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/avocado/empty_cpu_model.py b/tests/avocado/empty_cpu_model.py
index 22f504418d..723ecc73af 100644
--- a/tests/avocado/empty_cpu_model.py
+++ b/tests/avocado/empty_cpu_model.py
@@ -10,8 +10,11 @@
 from avocado_qemu import QemuSystemTest
 
 class EmptyCPUModel(QemuSystemTest):
+    """
+    :avocado: tags=machine:none
+    """
     def test(self):
-        self.vm.add_args('-S', '-display', 'none', '-machine', 'none', '-cpu', '')
+        self.vm.add_args('-S', '-display', 'none', '-cpu', '')
         self.vm.set_qmp_monitor(enabled=False)
         self.vm.launch()
         self.vm.wait()
-- 
2.32.0

[PATCH 2/5] machine.py: add default pseries params in machine.py

[Daniel Henrique Barboza]

pSeries guests set a handful of machine capabilities on by default, all
of them related to security mitigations, that aren't always available in
the host.

This means that, as is today, running avocado in a Power9 server without
the proper firmware support, and with --disable-tcg, this error will
occur:

 (1/1) tests/avocado/info_usernet.py:InfoUsernet.test_hostfwd: ERROR: ConnectError:
Failed to establish session: EOFError\n  Exit code: 1\n  (...)
(...)
        Command: ./qemu-system-ppc64 -display none -vga none (...)
        Output: qemu-system-ppc64: warning: netdev vnet has no peer
qemu-system-ppc64: Requested safe cache capability level not supported by KVM
Try appending -machine cap-cfpc=broken

info_usernet.py happens to trigger this error first, but all tests would
fail in this configuration because the host does not support the default
'cap-cfpc' capability.

A similar situation was already fixed a couple of years ago by Greg Kurz
(commit 63d57c8f91d0) but it was focused on TCG warnings for these same
capabilities and running C qtests. This commit ended up preventing the
problem we're facing with avocado when running qtests with KVM support.

This patch does a similar approach by amending machine.py to disable
these security capabilities in case we're running a pseries guest. The
change is made in the _launch() callback to be sure that we're already
commited into launching the guest. It's also worth noticing that we're
relying on self._machine being set accordingly (i.e. via tag:machine),
which is currently the case for all ppc64 related avocado tests.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 python/qemu/machine/machine.py | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 07ac5a710b..12e5e37bff 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -51,6 +51,11 @@
 
 
 LOG = logging.getLogger(__name__)
+PSERIES_DEFAULT_CAPABILITIES = ("cap-cfpc=broken,"
+                                "cap-sbbc=broken,"
+                                "cap-ibs=broken,"
+                                "cap-ccf-assist=off,"
+                                "cap-fwnmi=off")
 
 
 class QEMUMachineError(Exception):
@@ -447,6 +452,14 @@ def _launch(self) -> None:
         """
         Launch the VM and establish a QMP connection
         """
+
+        # pseries needs extra machine options to disable Spectre/Meltdown
+        # KVM related capabilities that might not be available in the
+        # host.
+        if "qemu-system-ppc64" in self._binary:
+            if self._machine is None or "pseries" in self._machine:
+                self._args.extend(['-machine', PSERIES_DEFAULT_CAPABILITIES])
+
         self._pre_launch()
         LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
 
-- 
2.32.0

[PATCH 3/5] avocado/multiprocess.py: use tags=machine:pc|virt

[Daniel Henrique Barboza]

Assigning the machine type via the avocado tag will set self.machine
from QEMUSystemTest and avoid the need to set the machine type by using
self.vm.add_args().

do_test() was changed to receive a 'machine_opts' that will allow the
aarch64 test to pass the additional '-machine gic-version=3' parameter
it requires.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 tests/avocado/multiprocess.py | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/avocado/multiprocess.py b/tests/avocado/multiprocess.py
index 80a3b8f442..9a7cc6becb 100644
--- a/tests/avocado/multiprocess.py
+++ b/tests/avocado/multiprocess.py
@@ -19,7 +19,7 @@ class Multiprocess(QemuSystemTest):
     KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
 
     def do_test(self, kernel_url, initrd_url, kernel_command_line,
-                machine_type):
+                machine_opts=None):
         """Main test method"""
         self.require_accelerator('kvm')
 
@@ -43,7 +43,8 @@ def do_test(self, kernel_url, initrd_url, kernel_command_line,
 
         # Create proxy process
         self.vm.set_console()
-        self.vm.add_args('-machine', machine_type)
+        if machine_opts:
+            self.vm.add_args('-machine', machine_opts)
         self.vm.add_args('-accel', 'kvm')
         self.vm.add_args('-cpu', 'host')
         self.vm.add_args('-object',
@@ -67,6 +68,7 @@ def do_test(self, kernel_url, initrd_url, kernel_command_line,
     def test_multiprocess_x86_64(self):
         """
         :avocado: tags=arch:x86_64
+        :avocado: tags=machine:pc
         """
         kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
                       '/linux/releases/31/Everything/x86_64/os/images'
@@ -76,12 +78,12 @@ def test_multiprocess_x86_64(self):
                       '/pxeboot/initrd.img')
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
                                'console=ttyS0 rdinit=/bin/bash')
-        machine_type = 'pc'
-        self.do_test(kernel_url, initrd_url, kernel_command_line, machine_type)
+        self.do_test(kernel_url, initrd_url, kernel_command_line)
 
     def test_multiprocess_aarch64(self):
         """
         :avocado: tags=arch:aarch64
+        :avocado: tags=machine:virt
         """
         kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
                       '/linux/releases/31/Everything/aarch64/os/images'
@@ -91,5 +93,5 @@ def test_multiprocess_aarch64(self):
                       '/pxeboot/initrd.img')
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
                                'rdinit=/bin/bash console=ttyAMA0')
-        machine_type = 'virt,gic-version=3'
-        self.do_test(kernel_url, initrd_url, kernel_command_line, machine_type)
+        machine_opts = 'gic-version=3'
+        self.do_test(kernel_url, initrd_url, kernel_command_line, machine_opts)
-- 
2.32.0

[PATCH 4/5] avocado/boot_linux.py: avocado tag fixes in BootLinuxAarch64

[Daniel Henrique Barboza]

BootLinuxAarch64 is already setting machine:virt in the avocado tags,
meaning that we don't need to add '-machine virt' via add_args().

It's also adding an extra '-machine gic-version=2' parameter via an
avocado tag, which is not ideal because:

- it prevents self.machine from QEMUSystemTest to be set since there are
multiple 'machine' avocado tests being set;

- the tests are using different 'gic-version' setting, meaning that
we're still needing to add '-machine gic-version=N' via add_args()
regardless.

Removing the 'machine=gic-version=2' tag allows us to set 'self.machine'
without adding extra work.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 tests/avocado/boot_linux.py | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tests/avocado/boot_linux.py b/tests/avocado/boot_linux.py
index ee584d2fdf..1fbedbab5c 100644
--- a/tests/avocado/boot_linux.py
+++ b/tests/avocado/boot_linux.py
@@ -61,7 +61,6 @@ class BootLinuxAarch64(LinuxTest):
     """
     :avocado: tags=arch:aarch64
     :avocado: tags=machine:virt
-    :avocado: tags=machine:gic-version=2
     """
 
     def add_common_args(self):
@@ -80,7 +79,7 @@ def test_virt_tcg_gicv2(self):
         self.require_accelerator("tcg")
         self.vm.add_args("-accel", "tcg")
         self.vm.add_args("-cpu", "max,lpa2=off")
-        self.vm.add_args("-machine", "virt,gic-version=2")
+        self.vm.add_args("-machine", "gic-version=2")
         self.add_common_args()
         self.launch_and_wait(set_up_ssh_connection=False)
 
@@ -93,7 +92,7 @@ def test_virt_tcg_gicv3(self):
         self.require_accelerator("tcg")
         self.vm.add_args("-accel", "tcg")
         self.vm.add_args("-cpu", "max,lpa2=off")
-        self.vm.add_args("-machine", "virt,gic-version=3")
+        self.vm.add_args("-machine", "gic-version=3")
         self.add_common_args()
         self.launch_and_wait(set_up_ssh_connection=False)
 
@@ -104,7 +103,7 @@ def test_virt_kvm(self):
         """
         self.require_accelerator("kvm")
         self.vm.add_args("-accel", "kvm")
-        self.vm.add_args("-machine", "virt,gic-version=host")
+        self.vm.add_args("-machine", "gic-version=host")
         self.add_common_args()
         self.launch_and_wait(set_up_ssh_connection=False)
 
-- 
2.32.0

[PATCH 5/5] avocado/virtio-gpu.py: use tags=machine:pc

[Daniel Henrique Barboza]

Using tags=machine:pc will do two things: it will avoid the need to
passing '-machine pc' via self.vm.add_args() and it will set the
self.machine attribute of the parent QEMUSystemTest class (via its
setUp() method).

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 tests/avocado/virtio-gpu.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/avocado/virtio-gpu.py b/tests/avocado/virtio-gpu.py
index 2a249a3a2c..a88f43d312 100644
--- a/tests/avocado/virtio-gpu.py
+++ b/tests/avocado/virtio-gpu.py
@@ -59,6 +59,7 @@ def wait_for_console_pattern(self, success_message, vm=None):
     def test_virtio_vga_virgl(self):
         """
         :avocado: tags=device:virtio-vga-gl
+        :avocado: tags=machine:pc
         """
         # FIXME: should check presence of virtio, virgl etc
         self.require_accelerator('kvm')
@@ -68,7 +69,7 @@ def test_virtio_vga_virgl(self):
 
         self.vm.set_console()
         self.vm.add_args("-m", "2G")
-        self.vm.add_args("-machine", "pc,accel=kvm")
+        self.vm.add_args("-machine", "accel=kvm")
         self.vm.add_args("-device", "virtio-vga-gl")
         self.vm.add_args("-display", "egl-headless")
         self.vm.add_args(
@@ -94,6 +95,7 @@ def test_virtio_vga_virgl(self):
     def test_vhost_user_vga_virgl(self):
         """
         :avocado: tags=device:vhost-user-vga
+        :avocado: tags=machine:pc
         """
         # FIXME: should check presence of vhost-user-gpu, virgl, memfd etc
         self.require_accelerator('kvm')
@@ -131,7 +133,7 @@ def test_vhost_user_vga_virgl(self):
         self.vm.set_console()
         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")
+        self.vm.add_args("-machine", "memory-backend=mem,accel=kvm")
         self.vm.add_args("-chardev", "socket,id=vug,fd=%d" % qemu_sock.fileno())
         self.vm.add_args("-device", "vhost-user-vga,chardev=vug")
         self.vm.add_args("-display", "egl-headless")
-- 
2.32.0

Re: [PATCH 2/5] machine.py: add default pseries params in machine.py

[John Snow]

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

On Mon, May 16, 2022, 12:53 PM Daniel Henrique Barboza <
danielhb413@gmail.com> wrote:

> pSeries guests set a handful of machine capabilities on by default, all
> of them related to security mitigations, that aren't always available in
> the host.
>
> This means that, as is today, running avocado in a Power9 server without
> the proper firmware support, and with --disable-tcg, this error will
> occur:
>
>  (1/1) tests/avocado/info_usernet.py:InfoUsernet.test_hostfwd: ERROR:
> ConnectError:
> Failed to establish session: EOFError\n  Exit code: 1\n  (...)
> (...)
>         Command: ./qemu-system-ppc64 -display none -vga none (...)
>         Output: qemu-system-ppc64: warning: netdev vnet has no peer
> qemu-system-ppc64: Requested safe cache capability level not supported by
> KVM
> Try appending -machine cap-cfpc=broken
>
> info_usernet.py happens to trigger this error first, but all tests would
> fail in this configuration because the host does not support the default
> 'cap-cfpc' capability.
>
> A similar situation was already fixed a couple of years ago by Greg Kurz
> (commit 63d57c8f91d0) but it was focused on TCG warnings for these same
> capabilities and running C qtests. This commit ended up preventing the
> problem we're facing with avocado when running qtests with KVM support.
>
> This patch does a similar approach by amending machine.py to disable
> these security capabilities in case we're running a pseries guest. The
> change is made in the _launch() callback to be sure that we're already
> commited into launching the guest. It's also worth noticing that we're
> relying on self._machine being set accordingly (i.e. via tag:machine),
> which is currently the case for all ppc64 related avocado tests.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  python/qemu/machine/machine.py | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/python/qemu/machine/machine.py
> b/python/qemu/machine/machine.py
> index 07ac5a710b..12e5e37bff 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -51,6 +51,11 @@
>
>
>  LOG = logging.getLogger(__name__)
> +PSERIES_DEFAULT_CAPABILITIES = ("cap-cfpc=broken,"
> +                                "cap-sbbc=broken,"
> +                                "cap-ibs=broken,"
> +                                "cap-ccf-assist=off,"
> +                                "cap-fwnmi=off")
>
>
>  class QEMUMachineError(Exception):
> @@ -447,6 +452,14 @@ def _launch(self) -> None:
>          """
>          Launch the VM and establish a QMP connection
>          """
> +
> +        # pseries needs extra machine options to disable Spectre/Meltdown
> +        # KVM related capabilities that might not be available in the
> +        # host.
> +        if "qemu-system-ppc64" in self._binary:
> +            if self._machine is None or "pseries" in self._machine:
> +                self._args.extend(['-machine',
> PSERIES_DEFAULT_CAPABILITIES])
> +
>          self._pre_launch()
>          LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
>
> --
> 2.32.0
>

Hm, okay.

I have plans to try and factor the machine appliance out and into an
upstream package in the near future, so I want to avoid more hardcoding of
defaults.

Does avocado have a subclass of QEMUMachine where it might be more
appropriate to stick this bandaid? Can we make one?

(I don't think iotests runs into this problem because we always use
machine:none there, I think. VM tests might have a similar problem though,
and then it'd be reasonable to want the bandaid here in machine.py ...
well, boo. okay.)

My verdict is that it's a bandaid, but I'll accept it if the avocado folks
agree to it and I'll sort it out later when I do my rewrite.

I don't think I have access to a power9 machine to test this with either,
so I might want a tested-by from someone who does.

--js

>

[-- Attachment #2: Type: text/html, Size: 5332 bytes --]

Re: [PATCH 2/5] machine.py: add default pseries params in machine.py

[Matheus K. Ferst]

On 19/05/2022 20:18, John Snow wrote:
> On Mon, May 16, 2022, 12:53 PM Daniel Henrique Barboza 
> <danielhb413@gmail.com <mailto:danielhb413@gmail.com>> wrote:
> 
>     pSeries guests set a handful of machine capabilities on by default, all
>     of them related to security mitigations, that aren't always available in
>     the host.
> 
>     This means that, as is today, running avocado in a Power9 server without
>     the proper firmware support, and with --disable-tcg, this error will
>     occur:
> 
>       (1/1) tests/avocado/info_usernet.py:InfoUsernet.test_hostfwd:
>     ERROR: ConnectError:
>     Failed to establish session: EOFError\n  Exit code: 1\n  (...)
>     (...)
>              Command: ./qemu-system-ppc64 -display none -vga none (...)
>              Output: qemu-system-ppc64: warning: netdev vnet has no peer
>     qemu-system-ppc64: Requested safe cache capability level not
>     supported by KVM
>     Try appending -machine cap-cfpc=broken
> 
>     info_usernet.py happens to trigger this error first, but all tests would
>     fail in this configuration because the host does not support the default
>     'cap-cfpc' capability.
> 
>     A similar situation was already fixed a couple of years ago by Greg Kurz
>     (commit 63d57c8f91d0) but it was focused on TCG warnings for these same
>     capabilities and running C qtests. This commit ended up preventing the
>     problem we're facing with avocado when running qtests with KVM support.
> 
>     This patch does a similar approach by amending machine.py to disable
>     these security capabilities in case we're running a pseries guest. The
>     change is made in the _launch() callback to be sure that we're already
>     commited into launching the guest. It's also worth noticing that we're
>     relying on self._machine being set accordingly (i.e. via tag:machine),
>     which is currently the case for all ppc64 related avocado tests.
> 
>     Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com
>     <mailto:danielhb413@gmail.com>>
>     ---
>       python/qemu/machine/machine.py | 13 +++++++++++++
>       1 file changed, 13 insertions(+)
> 
>     diff --git a/python/qemu/machine/machine.py
>     b/python/qemu/machine/machine.py
>     index 07ac5a710b..12e5e37bff 100644
>     --- a/python/qemu/machine/machine.py
>     +++ b/python/qemu/machine/machine.py
>     @@ -51,6 +51,11 @@
> 
> 
>       LOG = logging.getLogger(__name__)
>     +PSERIES_DEFAULT_CAPABILITIES = ("cap-cfpc=broken,"
>     +                                "cap-sbbc=broken,"
>     +                                "cap-ibs=broken,"
>     +                                "cap-ccf-assist=off,"
>     +                                "cap-fwnmi=off")
> 
> 
>       class QEMUMachineError(Exception):
>     @@ -447,6 +452,14 @@ def _launch(self) -> None:
>               """
>               Launch the VM and establish a QMP connection
>               """
>     +
>     +        # pseries needs extra machine options to disable
>     Spectre/Meltdown
>     +        # KVM related capabilities that might not be available in the
>     +        # host.
>     +        if "qemu-system-ppc64" in self._binary:
>     +            if self._machine is None or "pseries" in self._machine:
>     +                self._args.extend(['-machine',
>     PSERIES_DEFAULT_CAPABILITIES])
>     +
>               self._pre_launch()
>               LOG.debug('VM launch command: %r', '
>     '.join(self._qemu_full_args))
> 
>     -- 
>     2.32.0
> 
> 
> Hm, okay.
> 
> I have plans to try and factor the machine appliance out and into an 
> upstream package in the near future, so I want to avoid more hardcoding 
> of defaults.
> 
> Does avocado have a subclass of QEMUMachine where it might be more 
> appropriate to stick this bandaid? Can we make one?
> 
> (I don't think iotests runs into this problem because we always use 
> machine:none there, I think. VM tests might have a similar problem 
> though, and then it'd be reasonable to want the bandaid here in 
> machine.py ... well, boo. okay.)
> 
> My verdict is that it's a bandaid, but I'll accept it if the avocado 
> folks agree to it and I'll sort it out later when I do my rewrite.
> 
> I don't think I have access to a power9 machine to test this with 
> either, so I might want a tested-by from someone who does.
> 
> --js
> 

Unfortunately, none of our POWER9 machines had a firmware old enough to 
be affected by this issue. The closest I can test is a nested KVM-HV 
with L0 using cap-cfpc=broken, so the L1 receives the quoted message 
when running 'make check-avocado'.

With this setup I can confirm that the patch fixes this error, so
Tested-by: Matheus Ferst <matheus.ferst@eldorado.org.br>

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>