Re: [PATCH i-g-t 2/4] tests/intel/xe_sriov_vfio: Add dynamic bind-unbind-vfs subtest

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

On 3/24/2026 3:14 PM, Laguna, Lukasz wrote:
>
> On 3/18/2026 17:14, Marcin Bernatowicz wrote:
>> For each supported numvfs-N configuration, enable N VFs and
>> bind/unbind each VF to xe-vfio-pci through driver_override.
>>
>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>> Cc: Adam Miszczak <adam.miszczak@linux.intel.com>
>> Cc: Jakub Kolakowski <jakub1.kolakowski@intel.com>
>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> Cc: Lukasz Laguna <lukasz.laguna@intel.com>
>> ---
>>   tests/intel/xe_sriov_vfio.c | 169 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 169 insertions(+)
>>
>> diff --git a/tests/intel/xe_sriov_vfio.c b/tests/intel/xe_sriov_vfio.c
>> index 2fe0137e1..a2b2959e2 100644
>> --- a/tests/intel/xe_sriov_vfio.c
>> +++ b/tests/intel/xe_sriov_vfio.c
>> @@ -5,9 +5,13 @@
>>     #include <ctype.h>
>>   #include <dirent.h>
>> +#include <fcntl.h>
>>   +#include "drmtest.h"
>>   #include "igt_core.h"
>>   #include "igt_kmod.h"
>> +#include "igt_pci.h"
>> +#include "igt_sriov_device.h"
>>     /**
>>    * TEST: xe_sriov_vfio
>> @@ -22,13 +26,23 @@
>>    *
>>    * SUBTEST: unload-xe-vfio-pci
>>    * Description: Attempt to unload xe_vfio_pci module (skips if busy).
>> + *
>> + * SUBTEST: bind-unbind-vfs
>> + * Description: Enable VFs and bind/unbind each one to xe-vfio-pci 
>> via driver_override.
>>    */
>>     IGT_TEST_DESCRIPTION("Xe SR-IOV VFIO tests (xe-vfio-pci)");
>>   +#define DRIVER_OVERRIDE_TIMEOUT_MS 200
>> +
>>   static const char *XE_VFIO_PCI_MOD = "xe_vfio_pci";
>>   static const char *XE_VFIO_PCI_DRV = "xe-vfio-pci";
>>   +static int pf_fd = -1;
>> +static bool autoprobe;
>> +static bool xe_vfio_loaded_initially;
>> +static bool restore_xe_vfio_on_exit;
>> +
>>   static bool pci_driver_has_any_bound_devices(const char *driver)
>>   {
>>       char path[PATH_MAX];
>> @@ -54,8 +68,143 @@ static bool 
>> pci_driver_has_any_bound_devices(const char *driver)
>>       return false;
>>   }
>>   +static void assert_bound_driver_is(const char *pci_slot, const 
>> char *expected)
>
> I think it would be better to have two helpers: assert_driver_bound() 
> and assert_driver_unbound()
>
>> +{
>> +    char bound[64];
>> +    int ret;
>> +
>> +    ret = igt_pci_get_bound_driver_name(pci_slot, bound, 
>> sizeof(bound));
>> +    igt_assert_f(ret >= 0, "Failed to read bound driver for %s 
>> (%d)\n", pci_slot, ret);
>> +
>> +    if (!expected) {
>> +        igt_assert_f(ret == 0, "Expected %s to be unbound, but is 
>> bound to %s\n",
>> +                 pci_slot, bound);
>> +        return;
>> +    }
>> +
>> +    igt_assert_f(ret > 0, "Expected %s to be bound to %s, but it is 
>> unbound\n",
>> +             pci_slot, expected);
>> +    igt_assert_f(!strcmp(bound, expected), "Expected %s to be bound 
>> to %s, got %s\n",
>> +             pci_slot, expected, bound);
>> +}
>> +
>> +static char *vf_pci_slot_alloc(unsigned int vf_id)
>> +{
>> +    char *slot = igt_sriov_get_vf_pci_slot_alloc(pf_fd, vf_id);
>> +
>> +    igt_assert_f(slot, "Failed to get VF%u PCI slot\n", vf_id);
>> +    return slot;
>> +}
>> +
>> +static void vf_bind_override(unsigned int vf_id, const char *driver)
>> +{
>> +    char *slot = vf_pci_slot_alloc(vf_id);
>> +    int ret;
>> +
>> +    ret = igt_pci_bind_driver_override(slot, driver, 
>> DRIVER_OVERRIDE_TIMEOUT_MS);
>> +    igt_assert_f(ret == 0, "bind %s (VF%u) to %s failed (%d)\n", 
>> slot, vf_id, driver, ret);
>> +    assert_bound_driver_is(slot, driver);
>> +
>> +    free(slot);
>> +}
>> +
>> +static void vf_unbind_override(unsigned int vf_id)
>> +{
>> +    char *slot = vf_pci_slot_alloc(vf_id);
>> +    int ret;
>> +
>> +    ret = igt_pci_unbind_driver_override(slot, 
>> DRIVER_OVERRIDE_TIMEOUT_MS);
>> +    igt_assert_f(ret == 0, "unbind %s (VF%u) failed (%d)\n", slot, 
>> vf_id, ret);
>> +    assert_bound_driver_is(slot, NULL);
>> +
>> +    free(slot);
>> +}
>> +
>> +static void bind_unbind_vfs(unsigned int num_vfs)
>> +{
>> +    igt_sriov_disable_driver_autoprobe(pf_fd);
>> +    igt_sriov_enable_vfs(pf_fd, num_vfs);
>> +
>> +    igt_require_f(!igt_pci_system_reinit(), "Failed to refresh PCI 
>> state\n");
>> +
>> +    igt_sriov_enable_driver_autoprobe(pf_fd);
>> +
>> +    for (unsigned int vf_id = 1; vf_id <= num_vfs; vf_id++) {
>> +        vf_unbind_override(vf_id);
>
> Why unbind here?
Good catch, not needed when VFs created with autoprobe disabled
>
>> +        vf_bind_override(vf_id, XE_VFIO_PCI_DRV);
>> +        vf_unbind_override(vf_id);
>> +    }
>> +
>> +    igt_sriov_disable_vfs(pf_fd);
>> +}
>> +
>> +static void open_pf(void)
>> +{
>> +    int fd;
>> +
>> +    if (pf_fd >= 0)
>> +        return;
>> +
>> +    fd = drm_open_driver(DRIVER_XE);
>> +    igt_assert_fd(fd);
>> +
>> +    if (!igt_sriov_is_pf(fd)) {
>> +        drm_close_driver(fd);
>> +        igt_skip("Xe device is not an SR-IOV PF\n");
>> +    }
>> +
>> +    if (igt_sriov_get_enabled_vfs(fd) != 0) {
>> +        drm_close_driver(fd);
>> +        igt_skip("VFs must be disabled before running this test\n");
>> +    }
>> +
>> +    autoprobe = igt_sriov_is_driver_autoprobe_enabled(fd);
>> +    pf_fd = fd;
>> +}
>> +
>> +static void cleanup_pf(void)
>> +{
>> +    if (pf_fd < 0)
>> +        return;
>> +
>> +    igt_sriov_disable_vfs(pf_fd);
>> +    igt_abort_on_f(igt_sriov_get_enabled_vfs(pf_fd) > 0, "Failed to 
>> disable VF(s)\n");
>> +    autoprobe ? igt_sriov_enable_driver_autoprobe(pf_fd) :
>> +           igt_sriov_disable_driver_autoprobe(pf_fd);
>> +    igt_abort_on_f(autoprobe != 
>> igt_sriov_is_driver_autoprobe_enabled(pf_fd),
>> +               "Failed to restore sriov_drivers_autoprobe value\n");
>> +    drm_close_driver(pf_fd);
>> +    pf_fd = -1;
>> +}
>> +
>> +static void restore_xe_vfio_module(void)
>> +{
>> +    if (!restore_xe_vfio_on_exit)
>
> If you go with one subtest (load-unload) approach, then I think we 
> should always leave the environment in the original state, so this 
> variable would not be needed. 
>> +        return;
>> +
>> +    if (xe_vfio_loaded_initially) {
>> +        igt_abort_on_f(igt_kmod_load(XE_VFIO_PCI_MOD, NULL),
>> +                   "Failed to restore %s load state\n",
>> +                   XE_VFIO_PCI_MOD);
>> +        igt_abort_on_f(!igt_kmod_is_loaded(XE_VFIO_PCI_MOD),
>> +                   "%s should be loaded after cleanup\n",
>> +                   XE_VFIO_PCI_MOD);
>> +    } else if (igt_kmod_is_loaded(XE_VFIO_PCI_MOD)) {
>> +        igt_abort_on_f(igt_kmod_unload(XE_VFIO_PCI_MOD),
>> +                   "Failed to unload %s\n", XE_VFIO_PCI_MOD);
>
> I think it should be consistent - "Failed to restore %s load state".
>
>> + igt_abort_on_f(igt_kmod_is_loaded(XE_VFIO_PCI_MOD),
>> +                   "%s still loaded\n", XE_VFIO_PCI_MOD);
>
> If it's an abort I would say something like "Failed to do sth" or 
> "Can't do sth"
>
> Maybe use err variable, and at the end use one igt_abort_on_f(err, 
> "Failed to restore %s module load state") ?
>
yes, thanks
>> +    }
>> +
>> +    restore_xe_vfio_on_exit = false;
>> +}
>> +
>>   int igt_main()
>>   {
>> +    igt_fixture() {
>> +        xe_vfio_loaded_initially = igt_kmod_is_loaded(XE_VFIO_PCI_MOD);
>> +    }
>> +
>>       igt_describe("Attempt to load xe_vfio_pci module.");
>>       igt_subtest("load-xe-vfio-pci") {
>>           int ret;
>> @@ -85,4 +234,24 @@ int igt_main()
>>           igt_assert_f(!igt_kmod_is_loaded(XE_VFIO_PCI_MOD), "%s 
>> still loaded\n",
>>                    XE_VFIO_PCI_MOD);
>>       }
>> +
>> +    igt_describe("Enable VFs and bind/unbind each one to xe-vfio-pci 
>> via driver_override.");
>> +    igt_subtest_with_dynamic("bind-unbind-vfs") {
>> +        igt_skip_on_f(igt_kmod_load(XE_VFIO_PCI_MOD, NULL),
>> +                  "Failed to load %s\n", XE_VFIO_PCI_MOD);
>> +        restore_xe_vfio_on_exit = true;
>> +
>> +        open_pf();
>> +
>> +        for_each_sriov_num_vfs(pf_fd, num_vfs) {
>> +            igt_dynamic_f("numvfs-%u", num_vfs) {
>> +                bind_unbind_vfs(num_vfs);
>> +            }
>> +        }
>> +    }
>> +
>> +    igt_fixture() {
>> +        cleanup_pf();
>> +        restore_xe_vfio_module();
>> +    }
>>   }