From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: mm/hmm: do the plumbing for HMM to participate in migration
Date: Tue, 26 May 2026 07:29:20 +1000
Message-ID: <review-patch3-20260525084524.139868-4-mpenttil@redhat.com>
In-Reply-To: <20260525084524.139868-4-mpenttil@redhat.com>
References: <20260525084524.139868-1-mpenttil@redhat.com> <20260525084524.139868-4-mpenttil@redhat.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the largest structural change =E2=80=94 it reworks `hmm_vma_walk` t=
o carry migration state, adds pmd/pte lock tracking, and introduces `hmm_vm=
a_capture_migrate_range()` for capturing the VMA and MMU notifier range dur=
ing the walk.

**Issue 1 =E2=80=94 Potential duplicate definitions when CONFIG_DEVICE_MIGR=
ATION is set:**

Patch adds a fallback `enum migrate_vma_info` and `hmm_select_migrate()` in=
side `#ifdef CONFIG_MIGRATION`:
```c
+enum migrate_vma_info {
+	MIGRATE_VMA_SELECT_NONE =3D 0,
+	MIGRATE_VMA_SELECT_COMPOUND =3D MIGRATE_VMA_SELECT_NONE,
+};
+
+static inline enum migrate_vma_info hmm_select_migrate(struct hmm_range *r=
ange)
+{
+	return MIGRATE_VMA_SELECT_NONE;
+}
+
 #endif /* CONFIG_MIGRATION */
```

And then in the `#ifdef CONFIG_DEVICE_MIGRATION` block (which implies `CONF=
IG_MIGRATION`):
```c
-enum migrate_vma_direction {
+enum migrate_vma_info {
 	MIGRATE_VMA_SELECT_SYSTEM =3D 1 << 0,
 ...
+static inline enum migrate_vma_info hmm_select_migrate(struct hmm_range *r=
ange)
+{
+	return 0;
+}
```

When `CONFIG_DEVICE_MIGRATION=3Dy` (which requires `CONFIG_MIGRATION=3Dy`),=
 both blocks are compiled, giving duplicate definitions of the enum and the=
 inline function. This should be a compile error. The fallback definition n=
eeds a `#ifndef CONFIG_DEVICE_MIGRATION` guard. I may be misreading the bas=
e tree structure (the patches target drm-tip, not mainline), but if both bl=
ocks are unconditionally compiled, this won't build with `CONFIG_DEVICE_MIG=
RATION=3Dy`.

**Issue 2 =E2=80=94 `hmm_vma_walk` struct growth:**
```c
 struct hmm_vma_walk {
-	struct hmm_range	*range;
-	unsigned long		last;
+	struct mmu_notifier_range	mmu_range;
+	struct vm_area_struct		*vma;
+	struct hmm_range		*range;
+	unsigned long			start;
+	unsigned long			end;
+	unsigned long			last;
+	bool				ptelocked;
+	bool				pmdlocked;
+	spinlock_t			*ptl;
 };
```
This struct is stack-allocated in `hmm_range_fault()`. The `mmu_notifier_ra=
nge` alone is a significant addition. The overhead is always paid even when=
 not migrating. This is fine functionally but worth noting for non-migratio=
n callers.

**Issue 3 =E2=80=94 Lock state sharing via single `ptl` field:**

The `ptl` pointer is used for both PMD and PTE locks, which works because t=
hey're never held simultaneously. But it makes the code harder to reason ab=
out =E2=80=94 a future change that accidentally holds both would corrupt st=
ate silently. The `HMM_ASSERT_*` macros help but only catch issues at runti=
me.

**Issue 4 =E2=80=94 `hmm_vma_capture_migrate_range` MMU notifier range only=
 covers the first callback:**
```c
+	if (!hmm_vma_walk->mmu_range.owner) {
+		mmu_notifier_range_init_owner(&hmm_vma_walk->mmu_range, MMU_NOTIFY_MIGRA=
TE, 0,
+					      walk->vma->vm_mm, start, end,
+					      range->dev_private_owner);
+		mmu_notifier_invalidate_range_start(&hmm_vma_walk->mmu_range);
+	}
```
The MMU notifier range is initialized once with the first `start`/`end` fro=
m `hmm_vma_walk_test`. Subsequent calls update `hmm_vma_walk->end` but NOT =
the notifier range. Since the series enforces single-VMA via the `-ERANGE` =
check, this should be fine for the per-VMA test callback, but the `start`/`=
end` from `hmm_vma_walk_test` may be the per-VMA range which could differ f=
rom individual PMD-level sub-ranges.

**Issue 5 =E2=80=94 `else` after `}` without braces (kernel style):**
Multiple instances of:
```c
+		} else
+			pte_unmap(ptep);
```
Kernel coding style requires braces on the `else` if the `if` block has bra=
ces.

**Issue 6 =E2=80=94 C++ style comments for stubs:**
```c
+	// TODO: implement migration entry insertion
+	return 0;
```
These are removed in patch 4, so it's fine for bisect, but `/* */` is prefe=
rred in kernel code (though `//` is increasingly accepted).

---
Generated by Claude Code Patch Reviewer