Re: [PATCH i-g-t v3 6/7] tests/intel/xe_pxp: Termination tests

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 2/14/2025 12:07 PM, Kamil Konieczny wrote:
> Hi Daniele,
> On 2025-02-12 at 16:39:00 -0800, Daniele Ceraolo Spurio wrote:
>> There are several events that can cause the PXP key to be invalidated
>> and trigger a PXP termination (suspend, PXP termination irq). After a
>> termination, we expect the key to be different and the raw encrypted
>> data to change for the same source data.
>> Additionally, all PXP objects are invalidated during a termination and
>> can no longer be used in submission or kept mapped to VMs; we therefore
>> need to test both the execution and bind ioctls to make sure they work
>> as expected after a termination.
>>
>> v2: move igt_require calls inside the subtest
>> v3: block rpm for tests trying a different type of termination, rework
>> invalid bind test to try a new vm as well (Alan)
> I have few nits, one is from our CI fail:
>
> ../tests/intel/xe_pxp.c:1065:53: error: ‘O_RDONLY’ undeclared (first use in this function); did you mean ‘STA_RONLY’?
> fw_handle = igt_debugfs_open(fd, "forcewake_all", O_RDONLY);
>                                                    ^~~~~~~~
> 						 STA_RONLY
> ../tests/intel/xe_pxp.c:1065:53: note: each undeclared identifier is reported only once for each function it appears in
>
> Please include a proper lib header in a first patch which uses O_RDONLY.

This builds fine for me locally, I guess I'm getting O_RDONLY from some 
system include. I'll fix it.

>
> Rest nits below.
>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
>> ---
>>   lib/intel_batchbuffer.c |   8 +-
>>   lib/intel_batchbuffer.h |   1 +
>>   lib/xe/xe_ioctl.c       |  12 +-
>>   lib/xe/xe_ioctl.h       |   2 +
>>   tests/intel/xe_pxp.c    | 455 +++++++++++++++++++++++++++++++++++++++-
>>   5 files changed, 465 insertions(+), 13 deletions(-)
>>
> [...]
>>   
>> +static void termination_tests(int fd, bool pxp_supported, uint32_t devid,
>> +			      enum termination_type type, const char *tag)
>> +{
>> +	int fw_handle;
>> +
>> +	if (type != PXP_TERMINATION_RPM) {
>> +		/* avoid rpm entry for non-rpm tests */
>> +		fw_handle = igt_debugfs_open(fd, "forcewake_all", O_RDONLY);
> File or dir opens should happen in igt_fixture
>
>> +		igt_require(fw_handle >= 0);
> This should be in each subtest which needs this.
>
>> +	} else {
>> +		igt_setup_runtime_pm(fd);
> This should also be in fixture, consider creating xe_pxp_pm test.
>
>> +	}
>> +
>> +	igt_subtest_f("pxp-termination-key-update-post-%s", tag) {
>> +		require_pxp_render(pxp_supported, devid);
>> +		test_pxp_teardown_keychange(fd, type);
>> +	}
>> +	igt_subtest_f("pxp-stale-bo-bind-post-%s", tag) {
>> +		require_pxp(pxp_supported);
>> +		test_pxp_stale_bo_bind(fd, type);
>> +	}
>> +	igt_subtest_f("pxp-stale-bo-exec-post-%s", tag) {
>> +		require_pxp(pxp_supported);
>> +		test_pxp_stale_bo_exec(fd, type);
>> +	}
>> +
>> +	/* An active PXP queue holds an RPM ref, so we can't test RPM with it */
>> +	if (type != PXP_TERMINATION_RPM) {
>> +		igt_subtest_f("pxp-stale-queue-post-%s", tag) {
>> +			require_pxp(pxp_supported);
>> +			test_pxp_stale_queue_execution(fd, type);
>> +		}
>> +
>> +		close(fw_handle);
>> +	} else {
>> +		igt_restore_runtime_pm();
>> +	}
> Same here, both close() and ...restore...() should be done in
> igt_fixture { ... }
>
> To test if all is ok call your test with --show-testlist
> without sudo:
>
> ./xe_pxp --show-testlist

is it ok if I move everything inside igt_subtest, without using 
igt_fixture? it makes things much simpler in code and I'm getting no 
errors from --show-testlist when doing so.

Thanks,
Daniele

>
> Regards,
> Kamil
>
>> +}
>> +
>>   igt_main
>>   {
>>   	int xe_fd = -1;
>> @@ -470,6 +891,28 @@ igt_main
>>   		}
>>   	}
>>   
>> +	igt_subtest_group {
>> +		const struct mode {
>> +			enum termination_type type;
>> +			const char *tag;
>> +		} modes[] = {
>> +			{ PXP_TERMINATION_IRQ, "termination-irq" },
>> +			{ PXP_TERMINATION_SUSPEND, "suspend" },
>> +			{ PXP_TERMINATION_RPM, "rpm" },
>> +			{ 0, NULL }
>> +		};
>> +
>> +		igt_describe("Verify teardown management");
>> +
>> +		for (const struct mode *m = modes; m->tag; m++)
>> +			termination_tests(xe_fd, pxp_supported, devid, m->type, m->tag);
>> +
>> +		igt_subtest("pxp-optout") {
>> +			require_pxp(pxp_supported);
>> +			test_pxp_optout(xe_fd);
>> +		}
>> +	}
>> +
>>   	igt_fixture {
>>   		close(xe_fd);
>>   	}
>> -- 
>> 2.43.0
>>