Re: [PATCH v3 5/6] drm/gem-shmem: Track folio accessed/dirty status in mmap

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2777 bytes --]

Hi

Am 26.05.26 um 16:56 schrieb Igor Torrente:
> Hi Thomas,
>
> On 5/26/26 11:44, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 20.05.26 um 15:11 schrieb Igor Torrente:
>> [...]
>>>
>>> I'm not familiar with the drm_gem, code so I don't any more 
>>> insightful information
>>> to share.  But hopefully we can find a better fix for this.
>>
>> Do you have huge pages enabled ?
>
> No, I didn't even compile my kernel with huge pages. I've already been 
> burned by that before

I see.

The attached patch is a cleaned up version of Boris' fix. Please test 
and report back on the results.

Best regards
Thomas

>
>
> BR,
> Igor Torrente
>
>>
>> Best regards
>> Thomas
>>
>>>
>>> BR,
>>> Igor Torrente
>>>
>>>> ---
>>>>   drivers/gpu/drm/drm_gem_shmem_helper.c | 20 ++++++++++++++++++++
>>>>   1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c 
>>>> b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>> index c3a054899ba3..0c86ad40a049 100644
>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>> @@ -598,6 +598,9 @@ static vm_fault_t drm_gem_shmem_fault(struct 
>>>> vm_fault *vmf)
>>>>       if (ret != VM_FAULT_NOPAGE)
>>>>           ret = vmf_insert_pfn(vma, vmf->address, pfn);
>>>>   +    if (likely(!(ret & VM_FAULT_ERROR)))
>>>> +        folio_mark_accessed(folio);
>>>> +
>>>>   out:
>>>>       dma_resv_unlock(obj->resv);
>>>>   @@ -638,10 +641,27 @@ 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)
>>>> +{
>>>> +    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);
>>>> +    pgoff_t page_offset = vmf->pgoff - vma->vm_pgoff; /* page 
>>>> offset within VMA */
>>>> +    struct page *page = shmem->pages[page_offset];
>>>> +    struct folio *folio = page_folio(page);
>>>> +
>>>> +    file_update_time(vma->vm_file);
>>>> +
>>>> +    folio_mark_dirty(folio);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   const struct vm_operations_struct drm_gem_shmem_vm_ops = {
>>>>       .fault = drm_gem_shmem_fault,
>>>>       .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);
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)


[-- Attachment #2: 0001-drm-gem-shmem-Always-assume-writable-mmap-drop-pfn_m.patch --]
[-- Type: text/x-patch, Size: 3562 bytes --]

From 4478b3e07515194812806c5bfc3b7432d8f8406a Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <tzimmermann@suse.de>
Date: Wed, 27 May 2026 08:07:19 +0200
Subject: [PATCH] drm/gem-shmem: Always assume writable mmap; drop pfn_mkwrite

Using pfn_mkwrite breaks KVM with "error: kvm run failed Bad address".
Seen on a Mali-G610 GPU. Fix this by always marking the mapped pages
as written.
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 46 +++++++-------------------
 1 file changed, 12 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..07e117673124 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,16 @@ 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,
+						  vmf->flags & FAULT_FLAG_WRITE);
 		}
 #endif
 	}
@@ -635,8 +613,15 @@ 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);
+		/*
+		 * Always record write access to the buffer. The natural
+		 * place would be pfn_mkwrite, but this breaks KVM.
+		 */
+		file_update_time(vma->vm_file);
+		folio_mark_dirty(folio);
+	}
 
 out:
 	dma_resv_unlock(obj->resv);
@@ -683,12 +668,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 +675,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