* [bug report] drm/xe: Reset VMA attributes to default in SVM garbage collector
@ 2025-08-28 10:32 Dan Carpenter
2025-08-28 10:48 ` Ghimiray, Himal Prasad
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-08-28 10:32 UTC (permalink / raw)
To: Himal Prasad Ghimiray; +Cc: intel-xe
Hello Himal Prasad Ghimiray,
Commit a2eb8aec3ebe ("drm/xe: Reset VMA attributes to default in SVM
garbage collector") from Aug 21, 2025 (linux-next), leads to the
following Smatch static checker warning:
drivers/gpu/drm/xe/xe_vm.c:4442 xe_vm_alloc_vma()
error: uninitialized symbol 'default_pat'.
drivers/gpu/drm/xe/xe_vm.c
4396 static int xe_vm_alloc_vma(struct xe_vm *vm,
4397 struct drm_gpuvm_map_req *map_req,
4398 bool is_madvise)
4399 {
4400 struct xe_vma_ops vops;
4401 struct drm_gpuva_ops *ops = NULL;
4402 struct drm_gpuva_op *__op;
4403 bool is_cpu_addr_mirror = false;
4404 bool remap_op = false;
4405 struct xe_vma_mem_attr tmp_attr;
4406 u16 default_pat;
4407 int err;
4408
4409 lockdep_assert_held_write(&vm->lock);
4410
4411 if (is_madvise)
4412 ops = drm_gpuvm_madvise_ops_create(&vm->gpuvm, map_req);
4413 else
4414 ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, map_req);
4415
4416 if (IS_ERR(ops))
4417 return PTR_ERR(ops);
4418
4419 if (list_empty(&ops->list)) {
4420 err = 0;
4421 goto free_ops;
4422 }
4423
4424 drm_gpuva_for_each_op(__op, ops) {
4425 struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
4426 struct xe_vma *vma = NULL;
4427
4428 if (!is_madvise) {
4429 if (__op->op == DRM_GPUVA_OP_UNMAP) {
4430 vma = gpuva_to_vma(op->base.unmap.va);
4431 XE_WARN_ON(!xe_vma_has_default_mem_attrs(vma));
4432 default_pat = vma->attr.default_pat_index;
4433 }
4434
4435 if (__op->op == DRM_GPUVA_OP_REMAP) {
4436 vma = gpuva_to_vma(op->base.remap.unmap->va);
4437 default_pat = vma->attr.default_pat_index;
4438 }
4439
4440 if (__op->op == DRM_GPUVA_OP_MAP) {
4441 op->map.is_cpu_addr_mirror = true;
--> 4442 op->map.pat_index = default_pat;
Maybe we *know* the __ops are always in a specifc order so
REMAP and UNMAP come before MAP??
4443 }
4444 } else {
4445 if (__op->op == DRM_GPUVA_OP_REMAP) {
4446 vma = gpuva_to_vma(op->base.remap.unmap->va);
4447 xe_assert(vm->xe, !remap_op);
4448 xe_assert(vm->xe, xe_vma_has_no_bo(vma));
4449 remap_op = true;
4450
4451 if (xe_vma_is_cpu_addr_mirror(vma))
4452 is_cpu_addr_mirror = true;
4453 else
4454 is_cpu_addr_mirror = false;
4455 }
4456
4457 if (__op->op == DRM_GPUVA_OP_MAP) {
4458 xe_assert(vm->xe, remap_op);
4459 remap_op = false;
4460 /*
4461 * In case of madvise ops DRM_GPUVA_OP_MAP is
4462 * always after DRM_GPUVA_OP_REMAP, so ensure
4463 * we assign op->map.is_cpu_addr_mirror true
4464 * if REMAP is for xe_vma_is_cpu_addr_mirror vma
4465 */
4466 op->map.is_cpu_addr_mirror = is_cpu_addr_mirror;
4467 }
4468 }
4469 print_op(vm->xe, __op);
4470 }
4471
4472 xe_vma_ops_init(&vops, vm, NULL, NULL, 0);
4473
4474 if (is_madvise)
4475 vops.flags |= XE_VMA_OPS_FLAG_MADVISE;
4476
4477 err = vm_bind_ioctl_ops_parse(vm, ops, &vops);
4478 if (err)
4479 goto unwind_ops;
4480
4481 xe_vm_lock(vm, false);
4482
4483 drm_gpuva_for_each_op(__op, ops) {
4484 struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
4485 struct xe_vma *vma;
4486
4487 if (__op->op == DRM_GPUVA_OP_UNMAP) {
4488 vma = gpuva_to_vma(op->base.unmap.va);
4489 /* There should be no unmap for madvise */
4490 if (is_madvise)
4491 XE_WARN_ON("UNEXPECTED UNMAP");
4492
4493 xe_vma_destroy(vma, NULL);
4494 } else if (__op->op == DRM_GPUVA_OP_REMAP) {
4495 vma = gpuva_to_vma(op->base.remap.unmap->va);
4496 /* In case of madvise ops Store attributes for REMAP UNMAPPED
4497 * VMA, so they can be assigned to newly MAP created vma.
4498 */
4499 if (is_madvise)
4500 tmp_attr = vma->attr;
4501
4502 xe_vma_destroy(gpuva_to_vma(op->base.remap.unmap->va), NULL);
4503 } else if (__op->op == DRM_GPUVA_OP_MAP) {
4504 vma = op->map.vma;
4505 /* In case of madvise call, MAP will always be follwed by REMAP.
4506 * Therefore temp_attr will always have sane values, making it safe to
4507 * copy them to new vma.
4508 */
4509 if (is_madvise)
4510 vma->attr = tmp_attr;
4511 }
4512 }
4513
4514 xe_vm_unlock(vm);
4515 drm_gpuva_ops_free(&vm->gpuvm, ops);
4516 return 0;
4517
4518 unwind_ops:
4519 vm_bind_ioctl_ops_unwind(vm, &ops, 1);
4520 free_ops:
4521 drm_gpuva_ops_free(&vm->gpuvm, ops);
4522 return err;
4523 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] drm/xe: Reset VMA attributes to default in SVM garbage collector
2025-08-28 10:32 [bug report] drm/xe: Reset VMA attributes to default in SVM garbage collector Dan Carpenter
@ 2025-08-28 10:48 ` Ghimiray, Himal Prasad
2025-08-28 14:28 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Ghimiray, Himal Prasad @ 2025-08-28 10:48 UTC (permalink / raw)
To: Dan Carpenter; +Cc: intel-xe
On 28-08-2025 16:02, Dan Carpenter wrote:
> Hello Himal Prasad Ghimiray,
>
> Commit a2eb8aec3ebe ("drm/xe: Reset VMA attributes to default in SVM
> garbage collector") from Aug 21, 2025 (linux-next), leads to the
> following Smatch static checker warning:
>
> drivers/gpu/drm/xe/xe_vm.c:4442 xe_vm_alloc_vma()
> error: uninitialized symbol 'default_pat'.
>
> drivers/gpu/drm/xe/xe_vm.c
> 4396 static int xe_vm_alloc_vma(struct xe_vm *vm,
> 4397 struct drm_gpuvm_map_req *map_req,
> 4398 bool is_madvise)
> 4399 {
> 4400 struct xe_vma_ops vops;
> 4401 struct drm_gpuva_ops *ops = NULL;
> 4402 struct drm_gpuva_op *__op;
> 4403 bool is_cpu_addr_mirror = false;
> 4404 bool remap_op = false;
> 4405 struct xe_vma_mem_attr tmp_attr;
> 4406 u16 default_pat;
> 4407 int err;
> 4408
> 4409 lockdep_assert_held_write(&vm->lock);
> 4410
> 4411 if (is_madvise)
> 4412 ops = drm_gpuvm_madvise_ops_create(&vm->gpuvm, map_req);
> 4413 else
> 4414 ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, map_req);
> 4415
> 4416 if (IS_ERR(ops))
> 4417 return PTR_ERR(ops);
> 4418
> 4419 if (list_empty(&ops->list)) {
> 4420 err = 0;
> 4421 goto free_ops;
> 4422 }
> 4423
> 4424 drm_gpuva_for_each_op(__op, ops) {
> 4425 struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
> 4426 struct xe_vma *vma = NULL;
> 4427
> 4428 if (!is_madvise) {
> 4429 if (__op->op == DRM_GPUVA_OP_UNMAP) {
> 4430 vma = gpuva_to_vma(op->base.unmap.va);
> 4431 XE_WARN_ON(!xe_vma_has_default_mem_attrs(vma));
> 4432 default_pat = vma->attr.default_pat_index;
> 4433 }
> 4434
> 4435 if (__op->op == DRM_GPUVA_OP_REMAP) {
> 4436 vma = gpuva_to_vma(op->base.remap.unmap->va);
> 4437 default_pat = vma->attr.default_pat_index;
> 4438 }
> 4439
> 4440 if (__op->op == DRM_GPUVA_OP_MAP) {
> 4441 op->map.is_cpu_addr_mirror = true;
> --> 4442 op->map.pat_index = default_pat;
>
> Maybe we *know* the __ops are always in a specifc order so
> REMAP and UNMAP come before MAP??
True MAP is always follwed by REMAP.
>
> 4443 }
> 4444 } else {
> 4445 if (__op->op == DRM_GPUVA_OP_REMAP) {
> 4446 vma = gpuva_to_vma(op->base.remap.unmap->va);
> 4447 xe_assert(vm->xe, !remap_op);
> 4448 xe_assert(vm->xe, xe_vma_has_no_bo(vma));
> 4449 remap_op = true;
> 4450
> 4451 if (xe_vma_is_cpu_addr_mirror(vma))
> 4452 is_cpu_addr_mirror = true;
> 4453 else
> 4454 is_cpu_addr_mirror = false;
> 4455 }
> 4456
> 4457 if (__op->op == DRM_GPUVA_OP_MAP) {
> 4458 xe_assert(vm->xe, remap_op);
> 4459 remap_op = false;
> 4460 /*
> 4461 * In case of madvise ops DRM_GPUVA_OP_MAP is
> 4462 * always after DRM_GPUVA_OP_REMAP, so ensure
> 4463 * we assign op->map.is_cpu_addr_mirror true
> 4464 * if REMAP is for xe_vma_is_cpu_addr_mirror vma
> 4465 */
> 4466 op->map.is_cpu_addr_mirror = is_cpu_addr_mirror;
> 4467 }
> 4468 }
> 4469 print_op(vm->xe, __op);
> 4470 }
> 4471
> 4472 xe_vma_ops_init(&vops, vm, NULL, NULL, 0);
> 4473
> 4474 if (is_madvise)
> 4475 vops.flags |= XE_VMA_OPS_FLAG_MADVISE;
> 4476
> 4477 err = vm_bind_ioctl_ops_parse(vm, ops, &vops);
> 4478 if (err)
> 4479 goto unwind_ops;
> 4480
> 4481 xe_vm_lock(vm, false);
> 4482
> 4483 drm_gpuva_for_each_op(__op, ops) {
> 4484 struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
> 4485 struct xe_vma *vma;
> 4486
> 4487 if (__op->op == DRM_GPUVA_OP_UNMAP) {
> 4488 vma = gpuva_to_vma(op->base.unmap.va);
> 4489 /* There should be no unmap for madvise */
> 4490 if (is_madvise)
> 4491 XE_WARN_ON("UNEXPECTED UNMAP");
> 4492
> 4493 xe_vma_destroy(vma, NULL);
> 4494 } else if (__op->op == DRM_GPUVA_OP_REMAP) {
> 4495 vma = gpuva_to_vma(op->base.remap.unmap->va);
> 4496 /* In case of madvise ops Store attributes for REMAP UNMAPPED
> 4497 * VMA, so they can be assigned to newly MAP created vma.
> 4498 */
> 4499 if (is_madvise)
> 4500 tmp_attr = vma->attr;
> 4501
> 4502 xe_vma_destroy(gpuva_to_vma(op->base.remap.unmap->va), NULL);
> 4503 } else if (__op->op == DRM_GPUVA_OP_MAP) {
> 4504 vma = op->map.vma;
> 4505 /* In case of madvise call, MAP will always be follwed by REMAP.
> 4506 * Therefore temp_attr will always have sane values, making it safe to
> 4507 * copy them to new vma.
> 4508 */
> 4509 if (is_madvise)
> 4510 vma->attr = tmp_attr;
> 4511 }
> 4512 }
> 4513
> 4514 xe_vm_unlock(vm);
> 4515 drm_gpuva_ops_free(&vm->gpuvm, ops);
> 4516 return 0;
> 4517
> 4518 unwind_ops:
> 4519 vm_bind_ioctl_ops_unwind(vm, &ops, 1);
> 4520 free_ops:
> 4521 drm_gpuva_ops_free(&vm->gpuvm, ops);
> 4522 return err;
> 4523 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] drm/xe: Reset VMA attributes to default in SVM garbage collector
2025-08-28 10:48 ` Ghimiray, Himal Prasad
@ 2025-08-28 14:28 ` Dan Carpenter
2025-09-01 5:41 ` Ghimiray, Himal Prasad
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-08-28 14:28 UTC (permalink / raw)
To: Ghimiray, Himal Prasad; +Cc: intel-xe
On Thu, Aug 28, 2025 at 04:18:11PM +0530, Ghimiray, Himal Prasad wrote:
>
>
> On 28-08-2025 16:02, Dan Carpenter wrote:
> > Hello Himal Prasad Ghimiray,
> >
> > Commit a2eb8aec3ebe ("drm/xe: Reset VMA attributes to default in SVM
> > garbage collector") from Aug 21, 2025 (linux-next), leads to the
> > following Smatch static checker warning:
> >
> > drivers/gpu/drm/xe/xe_vm.c:4442 xe_vm_alloc_vma()
> > error: uninitialized symbol 'default_pat'.
> >
> > drivers/gpu/drm/xe/xe_vm.c
> > 4396 static int xe_vm_alloc_vma(struct xe_vm *vm,
> > 4397 struct drm_gpuvm_map_req *map_req,
> > 4398 bool is_madvise)
> > 4399 {
> > 4400 struct xe_vma_ops vops;
> > 4401 struct drm_gpuva_ops *ops = NULL;
> > 4402 struct drm_gpuva_op *__op;
> > 4403 bool is_cpu_addr_mirror = false;
> > 4404 bool remap_op = false;
> > 4405 struct xe_vma_mem_attr tmp_attr;
> > 4406 u16 default_pat;
> > 4407 int err;
> > 4408
> > 4409 lockdep_assert_held_write(&vm->lock);
> > 4410
> > 4411 if (is_madvise)
> > 4412 ops = drm_gpuvm_madvise_ops_create(&vm->gpuvm, map_req);
> > 4413 else
> > 4414 ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, map_req);
> > 4415
> > 4416 if (IS_ERR(ops))
> > 4417 return PTR_ERR(ops);
> > 4418
> > 4419 if (list_empty(&ops->list)) {
> > 4420 err = 0;
> > 4421 goto free_ops;
> > 4422 }
> > 4423
> > 4424 drm_gpuva_for_each_op(__op, ops) {
> > 4425 struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
> > 4426 struct xe_vma *vma = NULL;
> > 4427
> > 4428 if (!is_madvise) {
> > 4429 if (__op->op == DRM_GPUVA_OP_UNMAP) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 4430 vma = gpuva_to_vma(op->base.unmap.va);
> > 4431 XE_WARN_ON(!xe_vma_has_default_mem_attrs(vma));
> > 4432 default_pat = vma->attr.default_pat_index;
> > 4433 }
> > 4434
> > 4435 if (__op->op == DRM_GPUVA_OP_REMAP) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 4436 vma = gpuva_to_vma(op->base.remap.unmap->va);
> > 4437 default_pat = vma->attr.default_pat_index;
> > 4438 }
> > 4439
> > 4440 if (__op->op == DRM_GPUVA_OP_MAP) {
> > 4441 op->map.is_cpu_addr_mirror = true;
> > --> 4442 op->map.pat_index = default_pat;
> >
> > Maybe we *know* the __ops are always in a specifc order so
> > REMAP and UNMAP come before MAP??
>
> True MAP is always follwed by REMAP.
s/followed by/preceeded by/... For this to avoid an uninitialized
variable bug we first have to process the UNMAP or REMAP before we
process the MAP. The more I look at this the more if feels like a
bug, but okay...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] drm/xe: Reset VMA attributes to default in SVM garbage collector
2025-08-28 14:28 ` Dan Carpenter
@ 2025-09-01 5:41 ` Ghimiray, Himal Prasad
0 siblings, 0 replies; 4+ messages in thread
From: Ghimiray, Himal Prasad @ 2025-09-01 5:41 UTC (permalink / raw)
To: Dan Carpenter; +Cc: intel-xe
On 28-08-2025 19:58, Dan Carpenter wrote:
> On Thu, Aug 28, 2025 at 04:18:11PM +0530, Ghimiray, Himal Prasad wrote:
>>
>>
>> On 28-08-2025 16:02, Dan Carpenter wrote:
>>> Hello Himal Prasad Ghimiray,
>>>
>>> Commit a2eb8aec3ebe ("drm/xe: Reset VMA attributes to default in SVM
>>> garbage collector") from Aug 21, 2025 (linux-next), leads to the
>>> following Smatch static checker warning:
>>>
>>> drivers/gpu/drm/xe/xe_vm.c:4442 xe_vm_alloc_vma()
>>> error: uninitialized symbol 'default_pat'.
>>>
>>> drivers/gpu/drm/xe/xe_vm.c
>>> 4396 static int xe_vm_alloc_vma(struct xe_vm *vm,
>>> 4397 struct drm_gpuvm_map_req *map_req,
>>> 4398 bool is_madvise)
>>> 4399 {
>>> 4400 struct xe_vma_ops vops;
>>> 4401 struct drm_gpuva_ops *ops = NULL;
>>> 4402 struct drm_gpuva_op *__op;
>>> 4403 bool is_cpu_addr_mirror = false;
>>> 4404 bool remap_op = false;
>>> 4405 struct xe_vma_mem_attr tmp_attr;
>>> 4406 u16 default_pat;
>>> 4407 int err;
>>> 4408
>>> 4409 lockdep_assert_held_write(&vm->lock);
>>> 4410
>>> 4411 if (is_madvise)
>>> 4412 ops = drm_gpuvm_madvise_ops_create(&vm->gpuvm, map_req);
>>> 4413 else
>>> 4414 ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, map_req);
>>> 4415
>>> 4416 if (IS_ERR(ops))
>>> 4417 return PTR_ERR(ops);
>>> 4418
>>> 4419 if (list_empty(&ops->list)) {
>>> 4420 err = 0;
>>> 4421 goto free_ops;
>>> 4422 }
>>> 4423
>>> 4424 drm_gpuva_for_each_op(__op, ops) {
>>> 4425 struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
>>> 4426 struct xe_vma *vma = NULL;
>>> 4427
>>> 4428 if (!is_madvise) {
>>> 4429 if (__op->op == DRM_GPUVA_OP_UNMAP) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>>> 4430 vma = gpuva_to_vma(op->base.unmap.va);
>>> 4431 XE_WARN_ON(!xe_vma_has_default_mem_attrs(vma));
>>> 4432 default_pat = vma->attr.default_pat_index;
>>> 4433 }
>>> 4434
>>> 4435 if (__op->op == DRM_GPUVA_OP_REMAP) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>>> 4436 vma = gpuva_to_vma(op->base.remap.unmap->va);
>>> 4437 default_pat = vma->attr.default_pat_index;
>>> 4438 }
>>> 4439
>>> 4440 if (__op->op == DRM_GPUVA_OP_MAP) {
>>> 4441 op->map.is_cpu_addr_mirror = true;
>>> --> 4442 op->map.pat_index = default_pat;
>>>
>>> Maybe we *know* the __ops are always in a specifc order so
>>> REMAP and UNMAP come before MAP??
>>
>> True MAP is always follwed by REMAP.
>
> s/followed by/preceeded by/...
yes it should be preceeded by.
For this to avoid an uninitialized
> variable bug we first have to process the UNMAP or REMAP before we
> process the MAP. The more I look at this the more if feels like a
> bug, but okay...
Your analysis is right. REMAP or UNMAP will always happen first, which
will set up the default_pat to be used by MAP. So the default_pat will
always have sane value for MAP.
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-09-01 5:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-28 10:32 [bug report] drm/xe: Reset VMA attributes to default in SVM garbage collector Dan Carpenter
2025-08-28 10:48 ` Ghimiray, Himal Prasad
2025-08-28 14:28 ` Dan Carpenter
2025-09-01 5:41 ` Ghimiray, Himal Prasad
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.