Details
- Reviewers
markj
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Then the following is needed for drm-kmod:
diff --git a/drivers/gpu/drm/i915/i915_mm.c b/drivers/gpu/drm/i915/i915_mm.c index d1867ddd73..20406fba3e 100644 --- a/drivers/gpu/drm/i915/i915_mm.c +++ b/drivers/gpu/drm/i915/i915_mm.c @@ -126,7 +126,7 @@ static int remap_pfn(pte_t *pte, unsigned long addr, void *data) set_pte_at(r->mm, addr, pte, pte_mkspecial(pfn_pte(r->pfn, r->prot))); #elif defined(__FreeBSD__) vm_fault_t ret; - ret = lkpi_vmf_insert_pfn_prot_locked(r->vma, addr, r->pfn, r->prot); + ret = lkpi_vmf_insert_special_pte_locked(r->vma, addr, r->pfn, r->prot); if ((ret & VM_FAULT_OOM) != 0) return -ENOMEM; if ((ret & VM_FAULT_ERROR) != 0)
I will look at this soon, I just need to resurrect a laptop so that I can easily test i915 changes without taking down my main workstation.
So let me ask: is this related to the "fictitious pages" mentioned in D58320? If so, how is the LinuxKPI part to be written/work in the future world order with a real 'struct page'?
In some sense, yes, it is related. So far I do not know how that proposal about 'real struct page' would work at all. But I do not think that this change (in whatever form after debugging and reviewing it) would have any additional effect on the 'real struct page'.
(Still working on testing this. If I test drm-6.12 on my laptop, I get a corrupted display with or without this patch. After retesting drm-6.6 it seems there is some recent regression in main, as I now see similar symptoms there too, so I am bisecting that instead, hopefully will figure it out soon.)
Free allocated fake pages.
Add some comments explaining the intent.
Avoid going through the vm_page_array[] at all, the page physical address is all that is needed for a fake page construction.
In its current form (the linuxkpi/vm/drm-kmod changes), https://reviews.freebsd.org/D59883 can't resolve the ownership/reclamation bug, but it's possible you already know it.
The LLM told me, "I found a concrete omission in the current published combination: it leaves the affected CPU-mapping path using the old, ownership-changing helper. I checked the live review’s latest revision, diff 190340, including the accompanying drm-kmod change. That driver change switches remap_pfn() to lkpi_vmf_insert_special_pte_locked(). D59883 (https://reviews.freebsd.org/D59883?id=190340). However, the graphics-buffer mapping path involved in our ownership problem is:
vm_fault_cpu()
→ remap_io_sg()
→ remap_sg()
→ lkpi_vmf_insert_pfn_prot_locked()remap_sg() is a separate function, and the posted patch leaves it unchanged. I verified this by applying the accompanying patch to a scratch copy of upstream drm-kmod. This path never passes through the modified remap_pfn(). CPU fault handler (https://github.com/freebsd/drm-kmod/blob/c16de0f9fa84f70b9551f69fde837a7f3a73c0e9/drivers/gpu/drm/i915/gem/i915_gem_mman.c#L329), mapping callbacks (https://github.com/freebsd/drm-kmod/blob/c16de0f9fa84f70b9551f69fde837a7f3a73c0e9/drivers/gpu/drm/i915/i915_mm.c#L89). Consequently, the old helper can still discard a page’s swap backing, remove it from its original shmem object, and insert it into the mapping’s object. Later reclamation can therefore still lose contents that the graphics buffer needs to retain. D59883 adds the new helper without removing that behavior from the old one. Existing LinuxKPI implementation (https://github.com/freebsd/freebsd-src/blob/97d0c19e6af78dc7613563805653d13f03861737/sys/compat/linuxkpi/common/src/linux_page.c#L539). This is a definite reason the published combination leaves the ownership bug reachable."
Yes. Replace lkpi_vmf_insert_pfn_prot_locked() with lkpi_vmf_insert_special_pte_locked() in remap_io_sg() too. It appeared with GTT MMAP protocol v4 and was a main reason of backing ownership changing. It is in drivers/gpu/drm/i915/i915_mm.c line 102.
Well why did not you patched the other place too, then?
I think the following would do it, not even compiled.
diff --git a/drivers/gpu/drm/i915/i915_mm.c b/drivers/gpu/drm/i915/i915_mm.c index 711779c868..78fc3597d6 100644 --- a/drivers/gpu/drm/i915/i915_mm.c +++ b/drivers/gpu/drm/i915/i915_mm.c @@ -98,8 +98,8 @@ static int remap_sg(pte_t *pte, unsigned long addr, void *data) set_pte_at(r->mm, addr, pte, pte_mkspecial(pfn_pte(sgt_pfn(r), r->prot))); #elif defined(__FreeBSD__) - vm_fault_t ret = - lkpi_vmf_insert_pfn_prot_locked(r->vma, addr, sgt_pfn(r), r->prot); + vm_fault_t ret = lkpi_vmf_insert_special_pte_locked(r->vma, addr, + sgt_pfn(r), r->prot); if ((ret & VM_FAULT_OOM) != 0) return -ENOMEM; if ((ret & VM_FAULT_ERROR) != 0) @@ -126,7 +126,7 @@ static int remap_pfn(pte_t *pte, unsigned long addr, void *data) set_pte_at(r->mm, addr, pte, pte_mkspecial(pfn_pte(r->pfn, r->prot))); #elif defined(__FreeBSD__) vm_fault_t ret; - ret = lkpi_vmf_insert_pfn_prot_locked(r->vma, addr, r->pfn, r->prot); + ret = lkpi_vmf_insert_special_pte_locked(r->vma, addr, r->pfn, r->prot); if ((ret & VM_FAULT_OOM) != 0) return -ENOMEM; if ((ret & VM_FAULT_ERROR) != 0)