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

[Boris Brezillon <boris.brezillon@collabora.com>]

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);
>