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

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

Thanks for the tweaks / refactors from last review - all looks good now:

Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>


On Wed, 2025-02-12 at 16:39 -0800, Ceraolo Spurio, Daniele 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)
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> 
alan:snip

> +static void pxp_vm_bind_sync(int fd, uint32_t vm, uint32_t bo, uint64_t addr,
> +                            uint64_t size, uint32_t op)
> +{
> +       struct drm_xe_sync sync = {
> +               .type = DRM_XE_SYNC_TYPE_SYNCOBJ,
> +               .flags = DRM_XE_SYNC_FLAG_SIGNAL,
> +               .handle = syncobj_create(fd, 0),
> +       };
> +
> +       __xe_vm_bind_assert(fd, vm, 0, bo, 0, addr, size, op,
> +                           DRM_XE_VM_BIND_FLAG_CHECK_PXP, &sync, 1, 0, 0);
> +
> +       igt_assert(syncobj_wait(fd, &sync.handle, 1, INT64_MAX, 0, NULL));
> +       syncobj_destroy(fd, sync.handle);
> +}
> +
> +/**
> + * SUBTEST: pxp-stale-bo-bind-post-termination-irq
> + * Description: verify that VM bind on a stale BO (due to a termination irq) is rejected.
> + */
> +
> +/**
> + * SUBTEST: pxp-stale-bo-bind-post-suspend
> + * Description: verify that VM bind on a stale BO (due to a suspend/resume cycle)
> + *              is rejected.
> + */
> +
> +/**
> + * SUBTEST: pxp-stale-bo-bind-post-rpm
> + * Description: verify that VM bind on a stale BO (due to a runtime suspend/resume
> + *              cycle) is rejected.
> + */
> +
> +static void __test_pxp_stale_bo_bind(int fd, enum termination_type type, bool pxp)
> +{
> +       uint32_t vm, q;
> +       uint32_t bo;
> +       uint32_t flags = pxp ? DRM_XE_VM_BIND_FLAG_CHECK_PXP : 0;
> +       int ret;
> +
> +       vm = xe_vm_create(fd, 0, 0);
> +       q = create_pxp_rcs_queue(fd, vm); /* start PXP session */
> +
> +       bo = pxp_bo_create(fd, 0, 4096, DRM_XE_PXP_TYPE_HWDRM);
> +
> +       /* map the BO to the VM to make sure it works */
> +       pxp_vm_bind_sync(fd, vm, bo, 0, 4096, DRM_XE_VM_BIND_OP_MAP);
> +
> +       xe_exec_queue_destroy(fd, q);
> +       trigger_termination(fd, type);
> +
> +       /* map of a stale PXP BO must fail if (and only if) the CHECK_PXP flag is set */
> +       ret = __xe_vm_bind(fd, vm, 0, bo, 0, 0, 4096, DRM_XE_VM_BIND_OP_MAP,
> +                          flags, NULL, 0, 0, DEFAULT_PAT_INDEX, 0);
> +       igt_assert_eq(ret, pxp ? -ENOEXEC : 0);
> +
> +       /* unmap must always work */
> +       pxp_vm_bind_sync(fd, vm, bo, 0, 0, DRM_XE_VM_BIND_OP_UNMAP_ALL);
> +
> +       xe_vm_destroy(fd, vm);
> 
alan: The rework in this function means we are more targetted on
addressing the different aspects of the expected UAPI behaviors.
Nice! - thanks.
> +
> +       /* mapping on a brand new vm should have the same behavior */
> +       vm = xe_vm_create(fd, 0, 0);
> +       ret = __xe_vm_bind(fd, vm, 0, bo, 0, 0, 4096, DRM_XE_VM_BIND_OP_MAP,
> +                          flags, NULL, 0, 0, DEFAULT_PAT_INDEX, 0);
> +       igt_assert_eq(ret, pxp ? -ENOEXEC : 0);
> +
> +       gem_close(fd, bo);
> +       xe_vm_destroy(fd, vm);
> +}
> +
> 
alan:snip
>  
> +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);
> +               igt_require(fw_handle >= 0);
> +       } else {
> +               igt_setup_runtime_pm(fd);
> +       }
alan: This is a better approach here - now we can guarantee control
over it being a runtime-rpm triggered teardown vs other trigger types.
Thanks for this change.
> +
> +       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();
> +       }
> +}
> +
alan:snip