Re: [Qemu-devel] [PATCH 2/4] BootLinuxConsoleTest: Test nanoMIPS kernels on the I7200 CPU

[Cleber Rosa]

On Tue, May 21, 2019 at 01:19:08AM +0200, Philippe Mathieu-Daudé wrote:
> Similar to the x86_64/pc test, it boots a Linux kernel on a Malta
> machine and verify the serial is working.
> 
> Use the documentation added in commit f7d257cb4a17 to test
> nanoMIPS kernels and the I7200 CPU.
> 
> This test can be run using:
> 
>   $ avocado --show=console run -t arch:mipsel 
> tests/acceptance/boot_linux_console.py
>   console: [    0.000000] Linux version 4.15.18-00432-gb2eb9a8b 
> (address@hidden) (gcc version 6.3.0 (Codescape GNU Tools 2018.04-02 for 
> nanoMIPS Linux)) #1 SMP Wed Jun 27 11:10:08 PDT 2018
>   console: [    0.000000] GCRs appear to have been moved (expected them at 
> 0x1fbf8000)!
>   console: [    0.000000] GCRs appear to have been moved (expected them at 
> 0x1fbf8000)!
>   console: [    0.000000] CPU0 revision is: 00010000 (MIPS GENERIC QEMU)
>   console: [    0.000000] MIPS: machine is mti,malta
>   console: [    0.000000] Determined physical RAM map:
>   console: [    0.000000]  memory: 08000000 @ 00000000 (usable)
>   console: [    0.000000] earlycon: ns16550a0 at I/O port 0x3f8 (options 
> '38400n8')
>   console: [    0.000000] bootconsole [ns16550a0] enabled
>   console: [    0.000000] User-defined physical RAM map:
>   console: [    0.000000]  memory: 10000000 @ 00000000 (usable)
>   console: [    0.000000] Initrd not found or empty - disabling initrd
>   console: [    0.000000] MIPS CPS SMP unable to proceed without a CM
>   console: [    0.000000] Primary instruction cache 32kB, VIPT, 4-way, 
> linesize 32 bytes.
>   console: [    0.000000] Primary data cache 32kB, 4-way, VIPT, cache 
> aliases, linesize 32 bytes
>   console: [    0.000000] This processor doesn't support highmem. -262144k 
> highmem ignored
>   console: [    0.000000] Zone ranges:
>   console: [    0.000000]   Normal   [mem 
> 0x0000000000000000-0x000000000fffffff]
>   console: [    0.000000]   HighMem  empty
>   console: [    0.000000] Movable zone start for each node
>   console: [    0.000000] Early memory node ranges
>   console: [    0.000000]   node   0: [mem 
> 0x0000000000000000-0x000000000fffffff]
>   console: [    0.000000] Initmem setup node 0 [mem 
> 0x0000000000000000-0x000000000fffffff]
>   console: [    0.000000] random: get_random_bytes called from 
> start_kernel+0x60/0x2f0 with crng_init=0
>   console: [    0.000000] percpu: Embedded 16 pages/cpu @(ptrval) s36620 
> r8192 d20724 u65536
>   console: [    0.000000] Built 1 zonelists, mobility grouping on.  Total 
> pages: 64960
>   console: [    0.000000] Kernel command line: printk.time=0 mem=256m@@0x0 
> console=ttyS0 earlycon
> 
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  tests/acceptance/boot_linux_console.py | 58 ++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/tests/acceptance/boot_linux_console.py 
> b/tests/acceptance/boot_linux_console.py
> index 6aa084e049..1c330871c0 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -10,6 +10,8 @@
>  
>  import os
>  import logging
> +import lzma
> +import shutil
>  
>  from avocado_qemu import Test
>  from avocado.utils import process
> @@ -136,6 +138,62 @@ class BootLinuxConsole(Test):
>          console_pattern = 'Kernel command line: %s' % kernel_command_line
>          self.wait_for_console_pattern(console_pattern)
>  
> +    def do_test_mips_malta32el_nanomips(self, kernel_url, kernel_hash):

Nitpick: It's clear that having helper methods that tests build on is
a common pattern (even when we improve the parameter situation, so
that individual tests can be fed their own set of parameters).  What
bothers me a bit is the "_test_" component in their names, because it
makes it a bit harder on my eyes to distinguish between tests and
other methods.

Given that we are just starting to build our conventions, so I guess
I'd make sense to at try follow some prior art.  Looking at
virtio_version.py, I see that Eduardo used "check_" as a prefix for
methods of similar purpose.  Let me know if you think this seems worth
doing.

> +        kernel_path_xz = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> +        kernel_path = self.workdir + "kernel"
> +        with lzma.open(kernel_path_xz, 'rb') as f_in:
> +            with open(kernel_path, 'wb') as f_out:
> +                shutil.copyfileobj(f_in, f_out)
> +

avocado.utils.archive *almost* suits this lzma case (but not quite), so
ack on this.

> +        self.vm.set_machine('malta')
> +        self.vm.set_console()
> +        kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE
> +                               + 'mem=256m@@0x0 '
> +                               + 'console=ttyS0')

Another nitpick: these last two lines can be a single one (and avoid
one extra string concatenation).

> +        self.vm.add_args('-no-reboot',
> +                         '-cpu', 'I7200',
> +                         '-kernel', kernel_path,
> +                         '-append', kernel_command_line)
> +        self.vm.launch()
> +        console_pattern = 'Kernel command line: %s' % kernel_command_line
> +        self.wait_for_console_pattern(console_pattern)
> +
> +    def test_mips_malta32el_nanomips_4k(self):
> +        """
> +        :avocado: tags=arch:mipsel
> +        :avocado: tags=machine:malta
> +        :avocado: tags=endian:little
> +        """
> +        kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
> +                      'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
> +                      'generic_nano32r6el_page4k.xz')
> +        kernel_hash = '477456aafd2a0f1ddc9482727f20fe9575565dd6'
> +        self.do_test_mips_malta32el_nanomips(kernel_url, kernel_hash)
> +
> +    def test_mips_malta32el_nanomips_16k_up(self):
> +        """
> +        :avocado: tags=arch:mipsel
> +        :avocado: tags=machine:malta
> +        :avocado: tags=endian:little
> +        """
> +        kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
> +                      'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
> +                      'generic_nano32r6el_page16k_up.xz')
> +        kernel_hash = 'e882868f944c71c816e832e2303b7874d044a7bc'
> +        self.do_test_mips_malta32el_nanomips(kernel_url, kernel_hash)
> +
> +    def test_mips_malta32el_nanomips_64k_dbg(self):
> +        """
> +        :avocado: tags=arch:mipsel
> +        :avocado: tags=machine:malta
> +        :avocado: tags=endian:little
> +        """
> +        kernel_url = ('https://mipsdistros.mips.com/LinuxDistro/nanomips/'
> +                      'kernels/v4.15.18-432-gb2eb9a8b07a1-20180627102142/'
> +                      'generic_nano32r6el_page64k_dbg.xz')
> +        kernel_hash = '18d1c68f2e23429e266ca39ba5349ccd0aeb7180'
> +        self.do_test_mips_malta32el_nanomips(kernel_url, kernel_hash)
> +
>      def test_aarch64_virt(self):
>          """
>          :avocado: tags=arch:aarch64
> -- 
> 2.19.1
> 

I'd suggest you to add the mipsel target to the configure list on
.travis.yaml, ir accordance with the previous patches that added tests
for new arches.

Other than that, it looks good to me.

Tested-by: Cleber Rosa <address@hidden>