Re: [PATCH 1/2] tests/acceptance: Add PVH boot test

[Cleber Rosa <crosa@redhat.com>]

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

On Fri, Dec 06, 2019 at 09:00:11AM -0500, Wainer dos Santos Moschetta wrote:
> QEMU 4.0 onward is able to boot an uncompressed kernel
> image by using the x86/HVM direct boot ABI. It needs
> Linux >= 4.21 built with CONFIG_PVH=y.
> 
> This introduces an acceptance test which checks an
> uncompressed Linux kernel image boots properly.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  tests/acceptance/pvh.py | 48 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 tests/acceptance/pvh.py
> 
> diff --git a/tests/acceptance/pvh.py b/tests/acceptance/pvh.py
> new file mode 100644
> index 0000000000..c68489c273
> --- /dev/null
> +++ b/tests/acceptance/pvh.py
> @@ -0,0 +1,48 @@
> +# Copyright (c) 2019 Red Hat, Inc.
> +#
> +# Author:
> +#  Wainer dos Santos Moschetta <wainersm@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +"""
> +x86/HVM direct boot acceptance tests.
> +"""
> +
> +from avocado.utils.kernel import KernelBuild
> +from avocado_qemu import Test
> +from avocado_qemu import wait_for_console_pattern
> +
> +
> +class Pvh(Test):
> +    """
> +    Test suite for x86/HVM direct boot feature.
> +
> +    :avocado: tags=slow,arch=x86_64,machine=q35
> +    """
> +    def test_boot_vmlinux(self):
> +        """
> +        Boot uncompressed kernel image.
> +        """
> +        # QEMU can boot a vmlinux image for kernel >= 4.21 built
> +        # with CONFIG_PVH=y
> +        kernel_version = '5.4.1'
> +        kbuild = KernelBuild(kernel_version, work_dir=self.workdir)
> +        try:
> +            kbuild.download()
> +            kbuild.uncompress()
> +            kbuild.configure(targets=['defconfig', 'kvmconfig'],
> +                             extra_configs=['CONFIG_PVH=y'])

I'm aware of the reason why this uses APIs not fulfilled by
tests/requirements.txt, but, for the general public reviewing/testing
code with extra requirements, it's a good idea to bump the
requirements to a version that fulfills the requirement, and comment
out clearly on the temporary nature of the change (marking the patch).

For instance, for this requirement, we could have:

diff --git a/tests/requirements.txt b/tests/requirements.txt
index a2a587223a..5498d67bc1 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -1,4 +1,5 @@
 # Add Python module requirements, one per line, to be installed
 # in the tests/venv Python virtual environment. For more info,
 # refer to: https://pip.pypa.io/en/stable/user_guide/#id1
-avocado-framework==72.0
+# [REMOVE ME] use post 73.0 Avocado containing the new kernel build APIs
+-e git+https://github.com/avocado-framework/avocado@d6fb24edcf847f09c312b55df3c674c64c79793e#egg=avocado_framework

This will not only help people to test it, but should also make
it work transparently on CI.

> +            kbuild.build()

As stated in my response to the cover letter, I think we need to move
this elsewhere.  The *very* minimum is to have this in a setUp()
method, but we should strongly consider other solutions.

> +        except:
> +            self.cancel("Unable to build vanilla kernel %s" % kernel_version)
> +
> +        self.vm.set_machine('q35')
> +        self.vm.set_console()
> +        kernel_command_line = 'printk.time=0 console=ttyS0'
> +        self.vm.add_args('-kernel', kbuild.vmlinux,
> +                         '-append', kernel_command_line)

And just for being thorough (and purist? idealistic? Utopian? :), if
we stop and think about it, the following two lines are really what
this test is all about.  Everything else should be the test's setup.

I'm not arguing in favor of being radical and reject anything that is
not perfect, but just reminding ourselves (myself very much included)
of this general direction.

Cheers,
- Cleber.

> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Kernel command line: %s' %
> +                                 kernel_command_line)
> -- 
> 2.21.0
> 

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