Re: [PATCH for-10.0] tests/functional: Convert the intel_iommu avocado test

[Eric Auger]

On 12/9/24 10:25, Thomas Huth wrote:
> On 09/12/2024 10.09, Daniel P. Berrangé wrote:
>> On Mon, Dec 09, 2024 at 09:26:21AM +0100, Thomas Huth wrote:
>>> On 09/12/2024 09.12, Eric Auger wrote:
>>>> Hi Thomas,
>>>>
>>>> On 12/6/24 19:17, Thomas Huth wrote:
>>>>> Convert the intel_iommu test to the new functional framework.
>>>>> This test needs some changes since we neither support the old
>>>>> 'LinuxTest'
>>>>> class in the functional framework yet, nor a way to use SSH for
>>>>> running
>>>>> commands in the guest. So we now directly download a Fedora kernel
>>>>> and
>>>>> initrd and set up the serial console for executing the commands
>>>>> and for
>>>>> looking for the results.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>    MAINTAINERS                                   |   1 +
>>>>>    tests/functional/meson.build                  |   1 +
>>>>>    .../test_intel_iommu.py}                      | 119
>>>>> ++++++++----------
>>>>>    3 files changed, 51 insertions(+), 70 deletions(-)
>>>>>    rename tests/{avocado/intel_iommu.py =>
>>>>> functional/test_intel_iommu.py} (41%)
>>>>>    mode change 100644 => 100755
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index a62659b330..2ca452dbf9 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -3679,6 +3679,7 @@ S: Supported
>>>>>    F: hw/i386/intel_iommu.c
>>>>>    F: hw/i386/intel_iommu_internal.h
>>>>>    F: include/hw/i386/intel_iommu.h
>>>>> +F: tests/functional/test_intel_iommu.py
>>>>>    AMD-Vi Emulation
>>>>>    S: Orphan
>>>>> diff --git a/tests/functional/meson.build
>>>>> b/tests/functional/meson.build
>>>>> index 30c3eda7e4..dfc95fd904 100644
>>>>> --- a/tests/functional/meson.build
>>>>> +++ b/tests/functional/meson.build
>>>>> @@ -238,6 +238,7 @@ tests_x86_64_system_quick = [
>>>>>    tests_x86_64_system_thorough = [
>>>>>      'acpi_bits',
>>>>> +  'intel_iommu',
>>>>>      'x86_64_tuxrun',
>>>>>      'linux_initrd',
>>>>>      'multiprocess',
>>>>> diff --git a/tests/avocado/intel_iommu.py
>>>>> b/tests/functional/test_intel_iommu.py
>>>>> old mode 100644
>>>>> new mode 100755
>>>>> similarity index 41%
>>>>> rename from tests/avocado/intel_iommu.py
>>>>> rename to tests/functional/test_intel_iommu.py
>>>>> index 992583fa7d..6e47b1e9de
>>>>> --- a/tests/avocado/intel_iommu.py
>>>>> +++ b/tests/functional/test_intel_iommu.py
>>>>> @@ -1,3 +1,5 @@
>>>>> +#!/usr/bin/env python3
>>>>> +#
>>>>>    # INTEL_IOMMU Functional tests
>>>>>    #
>>>>>    # Copyright (c) 2021 Red Hat, Inc.
>>>>> @@ -7,116 +9,93 @@
>>>>>    #
>>>>>    # This work is licensed under the terms of the GNU GPL, version
>>>>> 2 or
>>>>>    # later.  See the COPYING file in the top-level directory.
>>>>> -import os
>>>>> -from avocado import skipUnless
>>>>> -from avocado_qemu.linuxtest import LinuxTest
>>>>> +from qemu_test import LinuxKernelTest, Asset,
>>>>> exec_command_and_wait_for_pattern
>>>>> +
>>>>> +class IntelIOMMU(LinuxKernelTest):
>>>>> -@skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'), 'Test is unstable
>>>>> on GitLab')
>>>>> -class IntelIOMMU(LinuxTest):
>>>>> -    """
>>>>> -    :avocado: tags=arch:x86_64
>>>>> -    :avocado: tags=distro:fedora
>>>>> -    :avocado: tags=distro_version:31
>>>>> -    :avocado: tags=machine:q35
>>>>> -    :avocado: tags=accel:kvm
>>>>> -    :avocado: tags=intel_iommu
>>>>> -    :avocado: tags=flaky
>>>>> -    """
>>>>> +    ASSET_KERNEL = Asset(
>>>>> +       
>>>>> ('https://archives.fedoraproject.org/pub/archive/fedora/linux/'
>>>>> +         'releases/39/Server/x86_64/os/images/pxeboot/vmlinuz'),
>>>>> +       
>>>>> '5f2ef0de47f8d79d5ee9bf8b0ee6d5ba4d987c2f9a16b8b511a7c69e53931fe3')
>>>>> +
>>>>> +    ASSET_INITRD = Asset(
>>>>> +       
>>>>> ('https://archives.fedoraproject.org/pub/archive/fedora/linux/'
>>>>> +         'releases/39/Server/x86_64/os/images/pxeboot/initrd.img'),
>>>>> +       
>>>>> '5bc29e2d872ceeb39a9698d42da3fb0afd7583dc7180de05a6b78bcc726674bb')
>>>>>        IOMMU_ADDON =
>>>>> ',iommu_platform=on,disable-modern=off,disable-legacy=on'
>>>>> +    default_kernel_params = 'console=ttyS0 rd.rescue quiet '
>>>>>        kernel_path = None
>>>>>        initrd_path = None
>>>>>        kernel_params = None
>>>>> -    def set_up_boot(self):
>>>>> -        path = self.download_boot()
>>>>> -        self.vm.add_args('-device', 'virtio-blk-pci,bus=pcie.0,' +
>>>>> -                         'drive=drv0,id=virtio-disk0,bootindex=1,'
>>>>> -                         'werror=stop,rerror=stop' +
>>>>> self.IOMMU_ADDON)
>>>>> -        self.vm.add_args('-device', 'virtio-gpu-pci' +
>>>>> self.IOMMU_ADDON)
>>>>> -        self.vm.add_args('-drive',
>>>>> -                        
>>>>> 'file=%s,if=none,cache=writethrough,id=drv0' % path)
>>>>> -
>>>>> -    def setUp(self):
>>>>> -        super(IntelIOMMU, self).setUp(None, 'virtio-net-pci' +
>>>>> self.IOMMU_ADDON)
>>>>> -
>>>>>        def add_common_args(self):
>>>>>            self.vm.add_args('-device', 'virtio-rng-pci,rng=rng0')
>>>>>            self.vm.add_args('-object',
>>>>>                            
>>>>> 'rng-random,id=rng0,filename=/dev/urandom')
>>>>> +        self.vm.add_args('-device', 'virtio-net-pci' +
>>>>> self.IOMMU_ADDON)
>>>>> +        self.vm.add_args('-device', 'virtio-gpu-pci' +
>>>>> self.IOMMU_ADDON)
>>>>> +        self.vm.add_args("-m", "1G")
>>>>> -    def common_vm_setup(self, custom_kernel=None):
>>>>> +    def common_vm_setup(self):
>>>>> +        self.set_machine('q35')
>>>>>            self.require_accelerator("kvm")
>>>>>            self.add_common_args()
>>>>>            self.vm.add_args("-accel", "kvm")
>>>>> -        if custom_kernel is None:
>>>>> -            return
>>>>> -
>>>>> -        kernel_url = self.distro.pxeboot_url + 'vmlinuz'
>>>>> -        kernel_hash = '5b6f6876e1b5bda314f93893271da0d5777b1f3c'
>>>>> -        initrd_url = self.distro.pxeboot_url + 'initrd.img'
>>>>> -        initrd_hash = 'dd0340a1b39bd28f88532babd4581c67649ec5b1'
>>>>> -        self.kernel_path = self.fetch_asset(kernel_url,
>>>>> asset_hash=kernel_hash)
>>>>> -        self.initrd_path = self.fetch_asset(initrd_url,
>>>>> asset_hash=initrd_hash)
>>>>> +        self.kernel_path = self.ASSET_KERNEL.fetch()
>>>>> +        self.initrd_path = self.ASSET_INITRD.fetch()
>>>>> +        self.kernel_params = self.default_kernel_params
>>>>>        def run_and_check(self):
>>>>>            if self.kernel_path:
>>>>>                self.vm.add_args('-kernel', self.kernel_path,
>>>>>                                 '-append', self.kernel_params,
>>>>>                                 '-initrd', self.initrd_path)
>>>>> -        self.launch_and_wait()
>>>>> -        self.ssh_command('cat /proc/cmdline')
>>>>> -        self.ssh_command('dmesg | grep -e DMAR -e IOMMU')
>>>>> -        self.ssh_command('find /sys/kernel/iommu_groups/ -type l')
>>>>> -        self.ssh_command('dnf -y install numactl-devel')
>>>> I understand you cannot use ssh yet but the bulk of the test was the
>>>> execution of the dnf install meaning we lose the main substance of it
>>>> through the conversion.
>>>
>>> Ah, I see, so this was exercising the virtio-net device with the
>>> IOMMU ...
>>> and I already wondered why there was this "dnf install" at the end
>>> without
>>> doing anything with  the numactl-devel package ... (a comment would
>>> have
>>> been helpful here)
>>
>> FYI, I find 'dnf instal' to be a *highly* undesirable thing todo in
>> our test functional. Its performance is highly non-deterministic
>> depending on what mirror you happen to get sent to, such that it could
>> easily push us over the timeouts. It is also susceptible to periodic
>> broken mirrors, and instability around time of Fefdora EOL. I can't
>> remember if it was this test case, or a different one, but I've seen
>> problems before in avocado with 'dnf install'.
>>
>> If we want to test working networking, then can we arrange for something
>> more simple & targetted to run, with better worst case performance.
>
> That might also be the reason why the avocado test was marked with
> "QEMU_TEST_FLAKY_TESTS" ... I think we might want to replace the "dnf
> install" with some local network file transfer between the guest and
> the host instead. I'll have a look at it when I find some spare time
> for v2 of my patch.

I think the actual reason of QEMU_TEST_FLAKY_TESTS presence is the test
was too long to execute. But indeed if we can get rid of the dnf install
and exercise the network a little bit, that's the best solution.

Thanks

Eric
>
>  Thomas
>