[PATCH i-g-t 1/2] tests/intel/xe_vm: Add bind-array-flag-invalid subtest

[Dandamudi, Priyanka]

> -----Original Message-----
> From: Manszewski, Christoph <christoph.manszewski at intel.com>
> Sent: 13 August 2025 02:08 PM
> To: Dandamudi, Priyanka <priyanka.dandamudi at intel.com>; igt-
> dev at lists.freedesktop.org
> Cc: Dugast, Francois <francois.dugast at intel.com>; Brost, Matthew
> <matthew.brost at intel.com>
> Subject: Re: [PATCH i-g-t 1/2] tests/intel/xe_vm: Add bind-array-flag-invalid
> subtest
> 
> Hi Priyanka,
> 
> On 13.08.2025 05:57, Dandamudi, Priyanka wrote:
> >
> >
> >> -----Original Message-----
> >> From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of
> >> Christoph Manszewski
> >> Sent: 12 August 2025 06:32 PM
> >> To: igt-dev at lists.freedesktop.org
> >> Cc: Dugast, Francois <francois.dugast at intel.com>; Brost, Matthew
> >> <matthew.brost at intel.com>; Manszewski, Christoph
> >> <christoph.manszewski at intel.com>
> >> Subject: [PATCH i-g-t 1/2] tests/intel/xe_vm: Add
> >> bind-array-flag-invalid subtest
> >>
> >> Add a complementary subtest to 'bind-flag-invalid' which checks
> >> vm_bind ioctl behavior when provided a invalid flag value when
> >> submitting an array of bind operations.
> >>
> >> Cc: Matthew Brost <matthew.brost at intel.com>
> >> Cc: Francois Dugast <francois.dugast at intel.com>
> >> Signed-off-by: Christoph Manszewski <christoph.manszewski at intel.com>
> >> ---
> >>   tests/intel/xe_vm.c | 90
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 90 insertions(+)
> >>
> >> diff --git a/tests/intel/xe_vm.c b/tests/intel/xe_vm.c index
> >> 3ce54053c..236494407 100644
> >> --- a/tests/intel/xe_vm.c
> >> +++ b/tests/intel/xe_vm.c
> >> @@ -2271,6 +2271,93 @@ static void bind_flag_invalid(int fd)
> >>   	xe_vm_destroy(fd, vm);
> >>   }
> >>
> >> +static void __bind_flag_valid(int fd, uint32_t bo, struct
> >> +drm_xe_vm_bind
> >> bind,
> >> +			      struct drm_xe_vm_bind_op *bind_ops, int
> >> num_binds) {
> >> +	struct drm_xe_sync *sync = from_user_pointer(bind.syncs);
> >> +	unsigned int valid_flags[] = {
> >> +		0,
> >> +		DRM_XE_VM_BIND_FLAG_READONLY,
> >> +		DRM_XE_VM_BIND_FLAG_IMMEDIATE,
> >> +		DRM_XE_VM_BIND_FLAG_NULL,
> >> +		DRM_XE_VM_BIND_FLAG_DUMPABLE,
> >> +		DRM_XE_VM_BIND_FLAG_CHECK_PXP,
> >> +	};
> >> +
> >> +	for (int i = 0; i < ARRAY_SIZE(valid_flags); i++) {
> >> +		if (!pxp_interface_supported(fd) && valid_flags[i] ==
> >> DRM_XE_VM_BIND_FLAG_CHECK_PXP)
> >> +			continue;
> >> +
> >> +		for (int j = 0; j < num_binds; j++) {
> >> +			bind_ops[j].flags = valid_flags[i];
> >> +			bind_ops[j].obj = valid_flags[i] ==
> >> DRM_XE_VM_BIND_FLAG_NULL ? 0 : bo;
> >> +		}
> >> +
> >> +		igt_ioctl(fd, DRM_IOCTL_XE_VM_BIND, &bind);
> >> +		igt_assert(syncobj_wait(fd, &sync[0].handle, 1, INT64_MAX, 0,
> >> NULL));
> >> +		syncobj_reset(fd, &sync[0].handle, 1);
> >> +	}
> >> +}
> >> +
> >> +/**
> >> + * SUBTEST: bind-array-flag-invalid
> >> + * Functionality: bind
> >> + * Description: Ensure invalid bind flags are rejected when
> >> +submitting an
> >> array of binds.
> >> + * Test category: negative test
> >> + */
> >> +static void test_bind_flag_invalid(int fd, int num_binds) {
> >> +	struct drm_xe_vm_bind_op *bind_ops;
> >> +	struct drm_xe_vm_bind bind;
> >> +	uint32_t vm;
> >> +
> >> +	uint32_t bo, bo_size = xe_get_default_alignment(fd);
> >> +	uint64_t addr = 0x1a0000;
> >> +	struct drm_xe_sync sync[1] = {
> >> +		{ .type = DRM_XE_SYNC_TYPE_SYNCOBJ, .flags =
> >> DRM_XE_SYNC_FLAG_SIGNAL, },
> >> +	};
> >> +
> >> +	igt_assert(num_binds > 1);
> >> +
> >> +	vm = xe_vm_create(fd, 0, 0);
> >> +	bo = xe_bo_create(fd, vm, bo_size, vram_if_possible(fd, 0), 0);
> >> +	sync[0].handle = syncobj_create(fd, 0);
> >> +
> >> +	bind_ops = calloc(num_binds, sizeof(*bind_ops));
> >> +	for (int i = 0; i < num_binds; i++) {
> >> +		bind_ops[i].addr = addr + i * bo_size;
> >> +		bind_ops[i].range = bo_size;
> >> +		bind_ops[i].obj = bo;
> >> +		bind_ops[i].op = DRM_XE_VM_BIND_OP_MAP;
> >> +		bind_ops[i].pat_index = intel_get_pat_idx_wb(fd);
> >> +	}
> >> +
> >> +	memset(&bind, 0, sizeof(bind));
> >> +	bind.vector_of_binds = to_user_pointer(bind_ops);
> >> +	bind.num_binds = num_binds;
> >> +	bind.syncs = to_user_pointer(sync);
> >> +	bind.num_syncs = 1;
> >> +	bind.vm_id = vm;
> >> +
> >> +	/* Using valid flags should work */
> >> +	__bind_flag_valid(fd, bo, bind, bind_ops, num_binds);
> >> +
> >> +	/* Using invalid flags should not work */
> >> +	for (int i = 0; i < num_binds; i++) {
> >> +		bind_ops[i].flags = BIT(30);
> >> +		bind_ops[i].obj = bo;
> >> +	}
> >> +
> >> +	do_ioctl_err(fd, DRM_IOCTL_XE_VM_BIND, &bind, EINVAL);
> >> +
> >> +	/* Using valid flags should still work */
> >> +	__bind_flag_valid(fd, bo, bind, bind_ops, num_binds);
> >> +
> >> +	syncobj_destroy(fd, sync[0].handle);
> >> +	gem_close(fd, bo);
> >> +	xe_vm_destroy(fd, vm);
> >> +}
> >> +
> >>   /**
> >>    * SUBTEST: invalid-flag-%s
> >>    * Functionality: ioctl_input_validation @@ -2658,6 +2745,9 @@ igt_main
> >>   		xe_for_each_engine(fd, hwe)
> >>   			test_bind_array_conflict(fd, hwe, false, true);
> >>
> >> +	igt_subtest("bind-array-flag-invalid")
> >> +		test_bind_flag_invalid(fd, 16);
> > Remaining code looks fine for me, and one more you mentioned that you
> noticed a double free when the vm bind ioctl failed, do you mean its for
> negative flag ?
> 
> The double free path executes when the argument check for an array bind
> fails, so yes, an invalid flag triggers it. You can checkout the fix for more
> details: https://patchwork.freedesktop.org/series/152835/
> 
> > What is the reason behind fixing the number of binds to 16. Could you
> explain?
> 
> No good reason, I just saw some other sub test that passes 16 for the number
> of bind ops. It can be anything greater or equal 2.
> 
> Thanks,
> Christoph
> 
> 
> > Thanks,
> > Priyanka
> >> +
> >>   	for (bind_size = 0x1ull << 21; bind_size <= 0x1ull << 31;
> >>   	     bind_size = bind_size << 1) {
> >>   		igt_subtest_f("large-binds-%lld",
> >> --
> >> 2.47.1
> >
LGTM,
Reviewed-by: Priyanka Dandamudi <priyanka.dandamudi at intel.com>