Re: [PATCH 02/12] tests/avocado: retire the Aarch64 TCG tests from boot_linux.py

[Philippe Mathieu-Daudé]

On 15/2/23 20:25, Alex Bennée wrote:
> The two TCG tests for GICv2 and GICv3 are very heavy weight distros
> that take a long time to boot up, especially for an --enable-debug
> build. The total code coverage they give is:
>
>    Overall coverage rate:
>      lines......: 11.2% (59584 of 530123 lines)
>      functions..: 15.0% (7436 of 49443 functions)
>      branches...: 6.3% (19273 of 303933 branches)
>
> We already get pretty close to that with the machine_aarch64_virt
> tests which only does one full boot (~120s vs ~600s) of alpine. We
> expand the kernel+initrd boot (~8s) to test both GICs and also add an
> RNG device and a block device to generate a few IRQs and exercise the
> storage layer. With that we get to a coverage of:
>
>    Overall coverage rate:
>      lines......: 11.0% (58121 of 530123 lines)
>      functions..: 14.9% (7343 of 49443 functions)
>      branches...: 6.0% (18269 of 303933 branches)
>
> which I feel is close enough given the massive time saving. If we want
> to target any more sub-systems we can use lighter weight more directed
> tests.
>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Acked-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
>
> ---
> v2
>    - fix checkpatch warning
> ---
>   tests/avocado/boot_linux.py           | 48 ++++----------------
>   tests/avocado/machine_aarch64_virt.py | 64 ++++++++++++++++++++++++---
>   2 files changed, 66 insertions(+), 46 deletions(-)


> -    def test_aarch64_virt(self):
> +    def common_aarch64_virt(self, machine):
>           """
> -        :avocado: tags=arch:aarch64
> -        :avocado: tags=machine:virt
> -        :avocado: tags=accel:tcg
> -        :avocado: tags=cpu:max
> +        Common code to launch basic virt machine with kernel+initrd
> +        and a scratch disk.
>           """
> +        logger = logging.getLogger('aarch64_virt')
> +
>           kernel_url = ('https://fileserver.linaro.org/s/'
>                         'z6B2ARM7DQT3HWN/download')
> -
>           kernel_hash = 'ed11daab50c151dde0e1e9c9cb8b2d9bd3215347'
>           kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
>   
> @@ -83,13 +85,63 @@ def test_aarch64_virt(self):
>                                  'console=ttyAMA0')
>           self.require_accelerator("tcg")
>           self.vm.add_args('-cpu', 'max,pauth-impdef=on',
> +                         '-machine', machine,

I'm surprised you had to add that, this should be handled by
avocado_qemu::get_vm which calls QEMUMachine::set_machine:

    def get_vm(self, *args, name=None):
            if self.machine is not None:
                self._vms[name].set_machine(self.machine)

>                            '-accel', 'tcg',
>                            '-kernel', kernel_path,
>                            '-append', kernel_command_line)
> +
> +        # A RNG offers an easy way to generate a few IRQs
> +        self.vm.add_args('-device', 'virtio-rng-pci,rng=rng0')
> +        self.vm.add_args('-object',
> +                         'rng-random,id=rng0,filename=/dev/urandom')
> +
> +        # Also add a scratch block device
> +        logger.info('creating scratch qcow2 image')
> +        image_path = os.path.join(self.workdir, 'scratch.qcow2')
> +        qemu_img = os.path.join(BUILD_DIR, 'qemu-img')
> +        if not os.path.exists(qemu_img):
> +            qemu_img = find_command('qemu-img', False)
> +        if qemu_img is False:
> +            self.cancel('Could not find "qemu-img", which is required to '
> +                        'create the temporary qcow2 image')
> +        cmd = '%s create -f qcow2 %s 8M' % (qemu_img, image_path)
> +        process.run(cmd)
> +
> +        # Add the device
> +        self.vm.add_args('-blockdev',
> +                         f"driver=qcow2,file.driver=file,"
> +                         f"file.filename={image_path},node-name=scratch")
> +        self.vm.add_args('-device',
> +                         'virtio-blk-device,drive=scratch')
> +
>           self.vm.launch()
>           self.wait_for_console_pattern('Welcome to Buildroot')
>           time.sleep(0.1)
>           exec_command(self, 'root')
>           time.sleep(0.1)
> +        exec_command(self, 'dd if=/dev/hwrng of=/dev/vda bs=512 count=4')
> +        time.sleep(0.1)
> +        exec_command(self, 'md5sum /dev/vda')
> +        time.sleep(0.1)
> +        exec_command(self, 'cat /proc/interrupts')
> +        time.sleep(0.1)
>           exec_command(self, 'cat /proc/self/maps')
>           time.sleep(0.1)
> +
> +    def test_aarch64_virt_gicv3(self):
> +        """
> +        :avocado: tags=arch:aarch64
> +        :avocado: tags=machine:virt

(machine set here)

> +        :avocado: tags=accel:tcg
> +        :avocado: tags=cpu:max
> +        """
> +        self.common_aarch64_virt("virt,gic_version=3")

Ah, you duplicate as "-machine virt -machine virt,gic_version=3".

It would be clearer if you rename common_aarch64_virt() argument
as 'machine_options' and pass the option as:

         """
         :avocado: tags=machine:virt
         """
         self.common_aarch64_virt("gic_version=3")

to produce "-machine virt -machine gic_version=3".

Anyhow,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>