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

[John Snow <jsnow@redhat.com>]

[-- 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 --]