Skip to contentSkip to conversation

drm/xe/userptr: properly setup pfn_flags_mask

PATCH Superseded Matthew Auld 3 participants 3 messages Feb 26, 2025 – Feb 27, 2025

Subsystems (3)
Stable branch DRM drivers Intel xe

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Matthew Auld

author
#0 lore raw

PATCH 1/1 drm/xe/userptr: properly setup pfn_flags_mask

Currently we just leave it uninitialised, which at first looks harmless, however we also don't zero out the pfn array, and with pfn_flags_mask the idea is to be able set individual flags for a given range of pfn or completely ignore them, outside of default_flags. So here we end up with pfn[i] & pfn_flags_mask, and if both are uninitialised we might get back an unexpected flags value, like asking for read only with default_flags, but getting back write on top, leading to potentially bogus behaviour.

To fix this ensure we zero the pfn_flags_mask, such that hmm only considers the default_flags and not also the initial pfn[i] value.

Fixes 81e058a3e7fd ("drm/xe: Introduce helper to populate userptr")
Signed-off-by Matthew Auld <[email protected]>
Cc Matthew Brost <[email protected]>
Cc Thomas Hellström <[email protected]>
Cc <[email protected]> # v6.10+
---
 drivers/gpu/drm/xe/xe_hmm.c | 1 +
 1 file changed, 1 insertion(+)
drivers/gpu/drm/xe/xe_hmm.c +1 -0
index 089834467880..8c3cd65fa4b3 100644
@@ -206,6 +206,7 @@ int xe_hmm_userptr_populate_range(struct xe_userptr_vma *uvma,
 		goto free_pfns;
 	}
 
+	hmm_range.pfn_flags_mask = 0;
 	hmm_range.default_flags = flags;
 	hmm_range.hmm_pfns = pfns;
 	hmm_range.notifier = &userptr->notifier;
Signature · 1 line
2.48.1
Reviewed-by Thomas Hellström on v1 1/1
#1 lore raw
Quoted 37 lines · Matthew Auld

Currently we just leave it uninitialised, which at first looks harmless,
however we also don't zero out the pfn array, and with pfn_flags_mask the idea is to be able set individual flags for a given range of pfn or
completely ignore them, outside of default_flags. So here we end up with
pfn[i] & pfn_flags_mask, and if both are uninitialised we might get back
an unexpected flags value, like asking for read only with default_flags,
but getting back write on top, leading to potentially bogus behaviour.

To fix this ensure we zero the pfn_flags_mask, such that hmm only considers the default_flags and not also the initial pfn[i] value.

Fixes 81e058a3e7fd ("drm/xe: Introduce helper to populate userptr")
Signed-off-by Matthew Auld <[email protected]>
Cc Matthew Brost <[email protected]>
Cc Thomas Hellström <[email protected]>
Cc <[email protected]> # v6.10+

---
 drivers/gpu/drm/xe/xe_hmm.c | 1 +

 1 file changed, 1 insertion(+)
drivers/gpu/drm/xe/xe_hmm.c +0 -0
index 089834467880..8c3cd65fa4b3 100644
@@ -206,6 +206,7 @@ int xe_hmm_userptr_populate_range(struct
 xe_userptr_vma *uvma,
  		goto free_pfns;
  	}

drivers/gpu/drm/xe/xe_hmm.c +1 -0
+	hmm_range.pfn_flags_mask = 0;
Quoted 3 lines

  hmm_range.default_flags = flags;
  hmm_range.hmm_pfns = pfns;
  hmm_range.notifier = &userptr->notifier;

Is there a chance we can modify the function to have an initializer for hmm_range

struct hmm_range hmm_range = {...

That'd also make adding fields to struct hmm_range less fragile.

Either way,

Reviewed-by Thomas Hellström <[email protected]>
Reviewed-by on v1 1/1
#2 lore raw
In reply to Matthew Auld
Quoted 36 lines

-----Original Message-----
From: Intel-xe <[email protected]> On Behalf Of Matthew Auld
Sent: Wednesday, February 26, 2025 10:47 PM
To: [email protected]
Cc: Brost, Matthew <[email protected]>; Hellstrom, Thomas <[email protected]>; [email protected]
Subject: [PATCH] drm/xe/userptr: properly setup pfn_flags_mask

Currently we just leave it uninitialised, which at first looks harmless, however we also don't zero out the pfn array, and with pfn_flags_mask the idea is to be able set individual flags for a given range of pfn or completely ignore them, outside of default_flags. So here we end up with pfn[i] & pfn_flags_mask, and if both are uninitialised we might get back an unexpected flags value, like asking for read only with default_flags, but getting back write on top, leading to potentially bogus behaviour.

To fix this ensure we zero the pfn_flags_mask, such that hmm only considers the default_flags and not also the initial pfn[i] value.

Fixes 81e058a3e7fd ("drm/xe: Introduce helper to populate userptr")
Signed-off-by Matthew Auld <[email protected]>
Cc Matthew Brost <[email protected]>
Cc Thomas Hellström <[email protected]>
Cc <[email protected]> # v6.10+
---
 drivers/gpu/drm/xe/xe_hmm.c | 1 +
 1 file changed, 1 insertion(+)
drivers/gpu/drm/xe/xe_hmm.c +0 -0
index 089834467880..8c3cd65fa4b3 100644

drivers/gpu/drm/xe/xe_hmm.c int xe_hmm_userptr_populate_range(struct · line 210

Quoted 5 lines
drivers/gpu/drm/xe/xe_hmm.c +1 -0
@@ -206,6 +206,7 @@ int xe_hmm_userptr_populate_range(struct
 xe_userptr_vma *uvma,
 		goto free_pfns;
 	}
 
+	hmm_range.pfn_flags_mask = 0;

Looks correct to me,

Reviewed-by Tejas Upadhyay <[email protected]>

Tejas

Quoted 5 lines
drivers/gpu/drm/xe/xe_hmm.c +0 -0
 	hmm_range.default_flags = flags;
 	hmm_range.hmm_pfns = pfns;

hmm_range.notifier = &userptr->notifier;
--
2.48.1