Re: [PATCH i-g-t 2/2] tests/xe_eudebug.c: Add subtests for eudebug/SR-IOV exlusion

["Manszewski, Christoph" <christoph.manszewski@intel.com>]

Hi Kamil,

On 17.02.2025 15:36, Kamil Konieczny wrote:
> Hi Christoph,
> On 2025-02-14 at 13:46:43 +0100, Christoph Manszewski wrote:
> 
> please remove file suffix from subject, also fix typo:
> 
> tests/xe_eudebug: Add subtests for eudebug/SR-IOV exclusion

Sure, will do.

> 
> More nits below.
> 
>> Eudebug is not supported in PF mode with VFs enabled and in VF mode.
>> Provide subtests to ensure that:
>>    1. enabling eudebug is not permitted in PF mode with VFs enabled
>>    2. eudebug sysfs toggle is not available in VF mode
>>    3. enabling VFs is not permitted when eudebug is enabled
>>
>> Signed-off-by: Christoph Manszewski <christoph.manszewski@intel.com>
>> ---
>>   tests/intel/xe_eudebug.c | 123 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 123 insertions(+)
>>
>> diff --git a/tests/intel/xe_eudebug.c b/tests/intel/xe_eudebug.c
>> index 76870805c..c2f32c03b 100644
>> --- a/tests/intel/xe_eudebug.c
>> +++ b/tests/intel/xe_eudebug.c
>> @@ -20,7 +20,9 @@
>>   #include <sys/prctl.h>
>>   
>>   #include "igt.h"
>> +#include "igt_sysfs.h"
>>   #include "intel_pat.h"
>> +#include "lib/igt_sriov_device.h"
>>   #include "lib/igt_syncobj.h"
>>   #include "xe/xe_eudebug.h"
>>   #include "xe/xe_ioctl.h"
>> @@ -2710,12 +2712,133 @@ static void test_basic_exec_queues_enable(int fd)
>>   	xe_vm_destroy(fd, vm_non_lr);
>>   }
>>   
>> +static bool has_enable_eudebug_attr(int pf_fd, unsigned int vf_num)
>> +{
>> +	char path[PATH_MAX];
>> +	int sysfs;
>> +	bool ret;
>> +
>> +	igt_assert(vf_num > 0);
>> +
>> +	sysfs = igt_sysfs_open(pf_fd);
>> +	igt_assert_fd(sysfs);
>> +	/* vf_num is 1-based, but virtfn is 0-based */
>> +	snprintf(path, sizeof(path), "device/virtfn%u/enable_eudebug", vf_num - 1);
>> +	ret = igt_sysfs_has_attr(sysfs, path);
>> +	close(sysfs);
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * SUBTEST: deny-eudebug
>> + * Description:
>> + *	Check that eudebug toggle is not available for VFs, and that enabling
>> + *	eudebug with VFs enabled is not permitted.
>> + */
>> +static void test_deny_eudebug(int pf_fd)
>> +{
>> +	unsigned int num_vfs = igt_sriov_get_total_vfs(pf_fd);
>> +	bool eudebug_enable = true;
>> +	bool err = false;
>> +	int ret = 0;
>> +
>> +	igt_debug("Testing %u VFs\n", num_vfs);
>> +
>> +	igt_sriov_enable_driver_autoprobe(pf_fd);
>> +	igt_sriov_enable_vfs(pf_fd, num_vfs);
>> +	igt_assert_eq(num_vfs, igt_sriov_get_enabled_vfs(pf_fd));
>> +
>> +	for (int vf_num = 1; vf_num <= num_vfs; ++vf_num) {
>> +		if (!igt_sriov_is_vf_drm_driver_probed(pf_fd, vf_num)) {
>> +			igt_debug("VF%u probe failed\n", vf_num);
>> +			err = true;
>> +		} else if (has_enable_eudebug_attr(pf_fd, vf_num)) {
>> +			igt_debug("VF%u has enable_eudebug attribute\n", vf_num);
>> +			err = true;
>> +		}
>> +	}
>> +
>> +	if (err) {
>> +		igt_sriov_disable_vfs(pf_fd);
>> +		igt_abort_on_f(igt_sriov_get_enabled_vfs(pf_fd) > 0, "Failed to disable VF(s)");
>> +		igt_fail_on(err);
>> +	}
>> +
>> +	ret = __xe_eudebug_enable_getset(pf_fd, NULL, &eudebug_enable);
>> +	igt_assert_eq(ret, -1);
>> +	igt_assert_eq(errno, EPERM);
>> +
>> +	igt_sriov_disable_vfs(pf_fd);
>> +	igt_abort_on_f(igt_sriov_get_enabled_vfs(pf_fd) > 0, "Failed to disable VF(s)");
>> +}
>> +
>> +/**
>> + * SUBTEST: deny-sriov
>> + * Description:
>> + *	Check that VFs cannot be enabled when eudebug is enabled.
>> + */
>> +static void test_deny_sriov(int pf_fd)
>> +{
>> +	unsigned int num_vfs = igt_sriov_get_total_vfs(pf_fd);
>> +	bool ret = false;
>> +	int sysfs = 0;
>> +
>> +	igt_debug("Testing %u VFs\n", num_vfs);
>> +
>> +	sysfs = igt_sysfs_open(pf_fd);
>> +	igt_assert_fd(sysfs);
>> +
>> +	ret = __igt_sysfs_set_u32(sysfs, "device/sriov_numvfs", num_vfs);
>> +	close(sysfs);
>> +
>> +	if (!ret) {
>> +		igt_sriov_disable_vfs(pf_fd);
>> +		igt_abort_on_f(igt_sriov_get_enabled_vfs(pf_fd) > 0, "Failed to disable VF(s)");
>> +	}
>> +	igt_assert_eq(ret, false);
>> +	igt_assert_eq(errno, EPERM);
>> +}
>> +
>>   igt_main
>>   {
>>   	bool was_enabled;
>>   	bool *multigpu_was_enabled;
>>   	int fd, gpu_count;
>>   
>> +	/* sriov exclusivity */
>> +	igt_subtest_group {
>> +		igt_fixture {
>> +			fd = drm_open_driver(DRIVER_XE);
>> +			igt_require(igt_sriov_is_pf(fd));
>> +			igt_require(igt_sriov_vfs_supported(fd));
>> +			igt_require(igt_sriov_get_enabled_vfs(fd) == 0);
> 
> Move all these igt_require into each subtest which needs them.

But why? This subtest group is for sriov related tests.

> 
>> +		}
>> +
>> +		igt_subtest("deny-eudebug") {
>> +			bool vf_autoprobe = igt_sriov_is_driver_autoprobe_enabled(fd);
>> +
>> +			was_enabled = xe_eudebug_enable(fd, false);
>> +			test_deny_eudebug(fd);
>> +			xe_eudebug_enable(fd, was_enabled);
> 
> This will not run if test_deny_eudebug() will trigger igt_assert.
> 
>> +
>> +			vf_autoprobe ? igt_sriov_enable_driver_autoprobe(fd) :
>> +				       igt_sriov_disable_driver_autoprobe(fd);
>> +			igt_abort_on_f(vf_autoprobe != igt_sriov_is_driver_autoprobe_enabled(fd),
>> +				       "Failed to restore sriov_drivers_autoprobe value\n");
>> +		}
>> +
>> +		igt_subtest("deny-sriov") {
>> +			was_enabled = xe_eudebug_enable(fd, true);
>> +			test_deny_sriov(fd);
>> +			xe_eudebug_enable(fd, was_enabled);
> 
> This will not run if test_deny_sriov() will trigger igt_assert.

Sure, will move it into the fixture, along with the aborts for disabling 
vfs.

> 
>> +		}
>> +
>> +		igt_fixture {
>> +			close(fd);
> 
> Why closing when just below you will open it?

Well I introduced it as a separate subtest group, I wanted to keep the 
fixtures isolated. How do you propose to handle it? I could create an 
additional top level fixture at the begginging for opening.

Thanks,
Christoph

> 
> Regards,
> Kamil
> 
>> +		}
>> +	}
>> +
>>   	igt_fixture {
>>   		fd = drm_open_driver(DRIVER_XE);
>>   		was_enabled = xe_eudebug_enable(fd, true);
>> -- 
>> 2.34.1
>>