[PATCH] drm/gem-shmem: Immediately record writable mmap; drop pfn_mkwrite

[PATCH] drm/gem-shmem: Immediately record writable mmap; drop pfn_mkwrite

[Thomas Zimmermann]

Using pfn_mkwrite breaks KVM with "error: kvm run failed Bad address".
Seen on a Mali-G610 GPU. Fix this by marking writable mmapped pages as
written when installing the mapping in the page table.

Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reported-by: Igor Torrente <igor.torrente@collabora.com>
Closes: https://lore.kernel.org/dri-devel/d89422f1-f00a-47b1-b68a-d949d6d6a974@collabora.com/raw
Tested-by: Igor Torrente <igor.torrente@collabora.com>
Fixes: 28e3918179aa ("drm/gem-shmem: Track folio accessed/dirty status in mmap")
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 48 ++++++++------------------
 1 file changed, 14 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 545933c7f712..7af31932af84 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -554,21 +554,6 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_dumb_create);
 
-static void drm_gem_shmem_record_mkwrite(struct vm_fault *vmf)
-{
-	struct vm_area_struct *vma = vmf->vma;
-	struct drm_gem_object *obj = vma->vm_private_data;
-	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
-	loff_t num_pages = obj->size >> PAGE_SHIFT;
-	pgoff_t page_offset = vmf->pgoff - vma->vm_pgoff; /* page offset within VMA */
-
-	if (drm_WARN_ON(obj->dev, !shmem->pages || page_offset >= num_pages))
-		return;
-
-	file_update_time(vma->vm_file);
-	folio_mark_dirty(page_folio(shmem->pages[page_offset]));
-}
-
 static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
 				 unsigned long pfn)
 {
@@ -581,23 +566,15 @@ static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
 
 		if (aligned &&
 		    folio_test_pmd_mappable(page_folio(pfn_to_page(pfn)))) {
-			vm_fault_t ret;
-
 			pfn &= PMD_MASK >> PAGE_SHIFT;
 
-			/* Unlike PTEs which are automatically upgraded to
+			/*
+			 * Unlike PTEs which are automatically upgraded to
 			 * writeable entries, the PMD upgrades go through
 			 * .huge_fault(). Make sure we pass the "write" info
 			 * along in that case.
-			 * This also means we have to record the write fault
-			 * here, instead of in .pfn_mkwrite().
 			 */
-			ret = vmf_insert_pfn_pmd(vmf, pfn,
-						 vmf->flags & FAULT_FLAG_WRITE);
-			if (ret == VM_FAULT_NOPAGE && (vmf->flags & FAULT_FLAG_WRITE))
-				drm_gem_shmem_record_mkwrite(vmf);
-
-			return ret;
+			return vmf_insert_pfn_pmd(vmf, pfn, false);
 		}
 #endif
 	}
@@ -635,8 +612,18 @@ static vm_fault_t drm_gem_shmem_any_fault(struct vm_fault *vmf, unsigned int ord
 	pfn = page_to_pfn(page);
 
 	ret = try_insert_pfn(vmf, order, pfn);
-	if (ret == VM_FAULT_NOPAGE)
+	if (ret == VM_FAULT_NOPAGE) {
 		folio_mark_accessed(folio);
+		/*
+		 * Immediately record write access to the buffer. The
+		 * natural place would be pfn_mkwrite at the time when
+		 * the access happens, but this breaks KVM.
+		 */
+		if (vma->vm_flags & VM_WRITE) {
+			file_update_time(vma->vm_file);
+			folio_mark_dirty(folio);
+		}
+	}
 
 out:
 	dma_resv_unlock(obj->resv);
@@ -683,12 +670,6 @@ static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
 	drm_gem_vm_close(vma);
 }
 
-static vm_fault_t drm_gem_shmem_pfn_mkwrite(struct vm_fault *vmf)
-{
-	drm_gem_shmem_record_mkwrite(vmf);
-	return 0;
-}
-
 const struct vm_operations_struct drm_gem_shmem_vm_ops = {
 	.fault = drm_gem_shmem_fault,
 #ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
@@ -696,7 +677,6 @@ const struct vm_operations_struct drm_gem_shmem_vm_ops = {
 #endif
 	.open = drm_gem_shmem_vm_open,
 	.close = drm_gem_shmem_vm_close,
-	.pfn_mkwrite = drm_gem_shmem_pfn_mkwrite,
 };
 EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
 
-- 
2.54.0

Re: [PATCH] drm/gem-shmem: Immediately record writable mmap; drop pfn_mkwrite

[Boris Brezillon]

On Thu, 28 May 2026 12:27:59 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Using pfn_mkwrite breaks KVM with "error: kvm run failed Bad address".
> Seen on a Mali-G610 GPU. Fix this by marking writable mmapped pages as
> written when installing the mapping in the page table.
> 
> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reported-by: Igor Torrente <igor.torrente@collabora.com>
> Closes: https://lore.kernel.org/dri-devel/d89422f1-f00a-47b1-b68a-d949d6d6a974@collabora.com/raw
> Tested-by: Igor Torrente <igor.torrente@collabora.com>
> Fixes: 28e3918179aa ("drm/gem-shmem: Track folio accessed/dirty status in mmap")

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Two nits below.

> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 48 ++++++++------------------
>  1 file changed, 14 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 545933c7f712..7af31932af84 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -554,21 +554,6 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_dumb_create);
>  
> -static void drm_gem_shmem_record_mkwrite(struct vm_fault *vmf)
> -{
> -	struct vm_area_struct *vma = vmf->vma;
> -	struct drm_gem_object *obj = vma->vm_private_data;
> -	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> -	loff_t num_pages = obj->size >> PAGE_SHIFT;
> -	pgoff_t page_offset = vmf->pgoff - vma->vm_pgoff; /* page offset within VMA */
> -
> -	if (drm_WARN_ON(obj->dev, !shmem->pages || page_offset >= num_pages))
> -		return;
> -
> -	file_update_time(vma->vm_file);
> -	folio_mark_dirty(page_folio(shmem->pages[page_offset]));
> -}
> -
>  static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
>  				 unsigned long pfn)
>  {
> @@ -581,23 +566,15 @@ static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
>  
>  		if (aligned &&
>  		    folio_test_pmd_mappable(page_folio(pfn_to_page(pfn)))) {
> -			vm_fault_t ret;
> -
>  			pfn &= PMD_MASK >> PAGE_SHIFT;
>  
> -			/* Unlike PTEs which are automatically upgraded to
> +			/*
> +			 * Unlike PTEs which are automatically upgraded to
>  			 * writeable entries, the PMD upgrades go through
>  			 * .huge_fault(). Make sure we pass the "write" info
>  			 * along in that case.
> -			 * This also means we have to record the write fault
> -			 * here, instead of in .pfn_mkwrite().
>  			 */

We can drop the comment altogether since we no longer have this
read-only -> rw upgrade now that pfn_mkwrite is gone.

> -			ret = vmf_insert_pfn_pmd(vmf, pfn,
> -						 vmf->flags & FAULT_FLAG_WRITE);
> -			if (ret == VM_FAULT_NOPAGE && (vmf->flags & FAULT_FLAG_WRITE))
> -				drm_gem_shmem_record_mkwrite(vmf);
> -
> -			return ret;
> +			return vmf_insert_pfn_pmd(vmf, pfn, false);
>  		}
>  #endif
>  	}
> @@ -635,8 +612,18 @@ static vm_fault_t drm_gem_shmem_any_fault(struct vm_fault *vmf, unsigned int ord
>  	pfn = page_to_pfn(page);
>  
>  	ret = try_insert_pfn(vmf, order, pfn);
> -	if (ret == VM_FAULT_NOPAGE)
> +	if (ret == VM_FAULT_NOPAGE) {
>  		folio_mark_accessed(folio);

Please add a blank line here.

> +		/*
> +		 * Immediately record write access to the buffer. The
> +		 * natural place would be pfn_mkwrite at the time when
> +		 * the access happens, but this breaks KVM.
> +		 */
> +		if (vma->vm_flags & VM_WRITE) {
> +			file_update_time(vma->vm_file);
> +			folio_mark_dirty(folio);
> +		}
> +	}
>  
>  out:
>  	dma_resv_unlock(obj->resv);
> @@ -683,12 +670,6 @@ static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
>  	drm_gem_vm_close(vma);
>  }
>  
> -static vm_fault_t drm_gem_shmem_pfn_mkwrite(struct vm_fault *vmf)
> -{
> -	drm_gem_shmem_record_mkwrite(vmf);
> -	return 0;
> -}
> -
>  const struct vm_operations_struct drm_gem_shmem_vm_ops = {
>  	.fault = drm_gem_shmem_fault,
>  #ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
> @@ -696,7 +677,6 @@ const struct vm_operations_struct drm_gem_shmem_vm_ops = {
>  #endif
>  	.open = drm_gem_shmem_vm_open,
>  	.close = drm_gem_shmem_vm_close,
> -	.pfn_mkwrite = drm_gem_shmem_pfn_mkwrite,
>  };
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
>