[PATCH 0/3] fbdev: Add deferred I/O support for contiguous kernel memory framebuffers

[PATCH 0/3] fbdev: Add deferred I/O support for contiguous kernel memory framebuffers

[mhkelley58]

From: Michael Kelley <mhklinux@outlook.com>

Current deferred I/O code works only for framebuffer memory that is
allocated with vmalloc(). The code assumes that the underlying page
refcount can be used by the mm subsystem to manage each framebuffer
page's lifecycle, which is consistent with vmalloc'ed memory, but not
with contiguous kernel memory from alloc_pages() or similar. When used
with contiguous kernel memory, current deferred I/O code eventually
causes the memory free lists to be scrambled, and a kernel panic ensues.
The problem is seen with the hyperv_fb driver when mmap'ing the
framebuffer into user space, as that driver uses alloc_pages() for the
framebuffer in some configurations. This patch set fixes the problem
by supporting contiguous kernel memory framebuffers with deferred I/O.

Patch 1 exports a 'mm' subsystem function needed by Patch 2.

Patch 2 is the changes to the fbdev deferred I/O code. More details
are in the commit message of Patch 2.

Patch 3 updates the hyperv_fb driver to use the new functionality
from Patch 2.

Michael Kelley (3):
  mm: Export vmf_insert_mixed_mkwrite()
  fbdev/deferred-io: Support contiguous kernel memory framebuffers
  fbdev: hyperv_fb: Fix mmap of framebuffers allocated using
    alloc_pages()

 drivers/video/fbdev/core/fb_defio.c | 126 +++++++++++++++++++++++-----
 drivers/video/fbdev/hyperv_fb.c     |   1 +
 include/linux/fb.h                  |   1 +
 mm/memory.c                         |   1 +
 4 files changed, 109 insertions(+), 20 deletions(-)

-- 
2.25.1

[PATCH 1/3] mm: Export vmf_insert_mixed_mkwrite()

[mhkelley58]

From: Michael Kelley <mhklinux@outlook.com>

Export vmf_insert_mixed_mkwrite() for use by fbdev deferred I/O code,
which can be built as a module. For consistency with the related function
vmf_insert_mixed(), export without the GPL qualifier.

Commit cd1e0dac3a3e ("mm: unexport vmf_insert_mixed_mkwrite") is
effectively reverted.

Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
 mm/memory.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/memory.c b/mm/memory.c
index 9d0ba6fe73c1..883ad53d077e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2660,6 +2660,7 @@ vm_fault_t vmf_insert_mixed_mkwrite(struct vm_area_struct *vma,
 {
 	return __vm_insert_mixed(vma, addr, pfn, true);
 }
+EXPORT_SYMBOL(vmf_insert_mixed_mkwrite);
 
 /*
  * maps a range of physical memory into the requested pages. the old
-- 
2.25.1

[PATCH 2/3] fbdev/deferred-io: Support contiguous kernel memory framebuffers

[mhkelley58]

From: Michael Kelley <mhklinux@outlook.com>

Current defio code works only for framebuffer memory that is allocated
with vmalloc(). The code assumes that the underlying page refcount can
be used by the mm subsystem to manage each framebuffer page's lifecycle,
including freeing the page if the refcount goes to 0. This approach is
consistent with vmalloc'ed memory, but not with contiguous kernel memory
allocated via alloc_pages() or similar. The latter such memory pages
usually have a refcount of 0 when allocated, and would be incorrectly
freed page-by-page if used with defio. That free'ing corrupts the memory
free lists and Linux eventually panics. Simply bumping the refcount after
allocation doesn’t work because when the framebuffer memory is freed,
__free_pages() complains about non-zero refcounts.

Commit 37b4837959cb ("video: deferred io with physically contiguous
memory") from the year 2008 purported to add support for contiguous
kernel memory framebuffers. The motivating device, sh_mobile_lcdcfb, uses
dma_alloc_coherent() to allocate framebuffer memory, which is likely to
use alloc_pages(). It's unclear to me how this commit actually worked at
the time, unless dma_alloc_coherent() was pulling from a CMA pool instead
of alloc_pages(). Or perhaps alloc_pages() worked differently or on the
arm32 architecture on which sh_mobile_lcdcfb is used.

In any case, for x86 and arm64 today, commit 37b4837959cb9 is not
sufficient to support contiguous kernel memory framebuffers. The problem
can be seen with the hyperv_fb driver, which may allocate the framebuffer
memory using vmalloc() or alloc_pages(), depending on the configuration
of the Hyper-V guest VM (Gen 1 vs. Gen 2) and the size of the framebuffer.

Fix this limitation by adding defio support for contiguous kernel memory
framebuffers. A driver with a framebuffer allocated from contiguous
kernel memory must set the FBINFO_KMEMFB flag to indicate such.

Tested with the hyperv_fb driver in both configurations -- with a vmalloc()
framebuffer and with an alloc_pages() framebuffer on x86. Also verified a
vmalloc() framebuffer on arm64. Hardware is not available to me to verify
that the older arm32 devices still work correctly, but the path for
vmalloc() framebuffers is essentially unchanged.

Even with these changes, defio does not support framebuffers in MMIO
space, as defio code depends on framebuffer memory pages having
corresponding 'struct page's.

Fixes: 3a6fb6c4255c ("video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.")
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
 drivers/video/fbdev/core/fb_defio.c | 126 +++++++++++++++++++++++-----
 include/linux/fb.h                  |   1 +
 2 files changed, 107 insertions(+), 20 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 4fc93f253e06..0879973a4572 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -8,11 +8,38 @@
  * for more details.
  */
 
+/*
+ * Deferred I/O ("defio") allows framebuffers that are mmap()'ed to user space
+ * to batch user space writes into periodic updates to the underlying
+ * framebuffer hardware or other implementation (such as with a virtualized
+ * framebuffer in a VM). At each batch interval, a callback is invoked in the
+ * framebuffer's kernel driver, and the callback is supplied with a list of
+ * pages that have been modified in the preceding interval. The callback can
+ * use this information to update the framebuffer hardware as necessary. The
+ * batching can improve performance and reduce the overhead of updating the
+ * hardware.
+ *
+ * Defio is supported on framebuffers allocated using vmalloc() and allocated
+ * as contiguous kernel memory using alloc_pages(), kmalloc(), or
+ * dma_alloc_coherent(), the latter of which might allocate from CMA. These
+ * memory allocations all have corresponding "struct page"s. Framebuffers
+ * in MMIO space are *not* supported because MMIO space does not have
+ * corrresponding "struct page"s.
+ *
+ * For framebuffers allocated using vmalloc(), struct fb_info must have
+ * "screen_buffer" set to the vmalloc address of the framebuffer. For
+ * framebuffers allocated from contiguous kernel memory, FBINFO_KMEMFB must
+ * be set, and "fix.smem_start" must be set to the physical address of the
+ * frame buffer. In both cases, "fix.smem_len" must be set to the framebuffer
+ * size in bytes.
+ */
+
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/string.h>
 #include <linux/mm.h>
+#include <linux/pfn_t.h>
 #include <linux/vmalloc.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
@@ -37,7 +64,7 @@ static struct page *fb_deferred_io_get_page(struct fb_info *info, unsigned long
 	else if (info->fix.smem_start)
 		page = pfn_to_page((info->fix.smem_start + offs) >> PAGE_SHIFT);
 
-	if (page)
+	if (page && !(info->flags & FBINFO_KMEMFB))
 		get_page(page);
 
 	return page;
@@ -137,6 +164,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
 
 	BUG_ON(!info->fbdefio->mapping);
 
+	if (info->flags & FBINFO_KMEMFB)
+		/*
+		 * In this path, the VMA is marked VM_PFNMAP, so mm assumes
+		 * there is no struct page associated with the page. The
+		 * PFN must be directly inserted and the created PTE will be
+		 * marked "special".
+		 */
+		return vmf_insert_pfn(vmf->vma, vmf->address, page_to_pfn(page));
+
 	vmf->page = page;
 	return 0;
 }
@@ -163,13 +199,14 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
 
 /*
  * Adds a page to the dirty list. Call this from struct
- * vm_operations_struct.page_mkwrite.
+ * vm_operations_struct.page_mkwrite or .pfn_mkwrite.
  */
-static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
+static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, struct vm_fault *vmf,
 					    struct page *page)
 {
 	struct fb_deferred_io *fbdefio = info->fbdefio;
 	struct fb_deferred_io_pageref *pageref;
+	unsigned long offset = vmf->pgoff << PAGE_SHIFT;
 	vm_fault_t ret;
 
 	/* protect against the workqueue changing the page list */
@@ -182,20 +219,34 @@ static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, unsigned long
 	}
 
 	/*
-	 * We want the page to remain locked from ->page_mkwrite until
-	 * the PTE is marked dirty to avoid mapping_wrprotect_range()
-	 * being called before the PTE is updated, which would leave
-	 * the page ignored by defio.
-	 * Do this by locking the page here and informing the caller
-	 * about it with VM_FAULT_LOCKED.
+	 * The PTE must be marked writable before the defio deferred work runs
+	 * again and potentially marks the PTE write-protected. If the order
+	 * should be switched, the PTE would become writable without defio
+	 * tracking the page, leaving the page forever ignored by defio.
+	 *
+	 * For vmalloc() framebuffers, the associated struct page is locked
+	 * before releasing the defio lock. mm will later mark the PTE writaable
+	 * and release the struct page lock. The struct page lock prevents
+	 * the page from being prematurely being marked write-protected.
+	 *
+	 * For FBINFO_KMEMFB framebuffers, mm assumes there is no struct page,
+	 * so the PTE must be marked writable while the defio lock is held.
 	 */
-	lock_page(pageref->page);
+	if (info->flags & FBINFO_KMEMFB) {
+		unsigned long pfn = page_to_pfn(pageref->page);
+
+		ret = vmf_insert_mixed_mkwrite(vmf->vma, vmf->address,
+					       __pfn_to_pfn_t(pfn, PFN_SPECIAL));
+	} else {
+		lock_page(pageref->page);
+		ret = VM_FAULT_LOCKED;
+	}
 
 	mutex_unlock(&fbdefio->lock);
 
 	/* come back after delay to process the deferred IO */
 	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
-	return VM_FAULT_LOCKED;
+	return ret;
 
 err_mutex_unlock:
 	mutex_unlock(&fbdefio->lock);
@@ -207,10 +258,10 @@ static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, unsigned long
  * @fb_info: The fbdev info structure
  * @vmf: The VM fault
  *
- * This is a callback we get when userspace first tries to
- * write to the page. We schedule a workqueue. That workqueue
- * will eventually mkclean the touched pages and execute the
- * deferred framebuffer IO. Then if userspace touches a page
+ * This is a callback we get when userspace first tries to write to a
+ * page. We schedule a workqueue. That workqueue will eventually do
+ * mapping_wrprotect_range() on the written pages and execute the
+ * deferred framebuffer IO. Then if userspace writes to a page
  * again, we repeat the same scheme.
  *
  * Returns:
@@ -218,12 +269,11 @@ static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, unsigned long
  */
 static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
 {
-	unsigned long offset = vmf->pgoff << PAGE_SHIFT;
 	struct page *page = vmf->page;
 
 	file_update_time(vmf->vma->vm_file);
 
-	return fb_deferred_io_track_page(info, offset, page);
+	return fb_deferred_io_track_page(info, vmf, page);
 }
 
 /* vm_ops->page_mkwrite handler */
@@ -234,9 +284,25 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
 	return fb_deferred_io_page_mkwrite(info, vmf);
 }
 
+/*
+ * Similar to fb_deferred_io_mkwrite(), but for first writes to pages
+ * in VMAs that have VM_PFNMAP set.
+ */
+static vm_fault_t fb_deferred_io_pfn_mkwrite(struct vm_fault *vmf)
+{
+	struct fb_info *info = vmf->vma->vm_private_data;
+	unsigned long offset = vmf->pgoff << PAGE_SHIFT;
+	struct page *page = phys_to_page(info->fix.smem_start + offset);
+
+	file_update_time(vmf->vma->vm_file);
+
+	return fb_deferred_io_track_page(info, vmf, page);
+}
+
 static const struct vm_operations_struct fb_deferred_io_vm_ops = {
 	.fault		= fb_deferred_io_fault,
 	.page_mkwrite	= fb_deferred_io_mkwrite,
+	.pfn_mkwrite	= fb_deferred_io_pfn_mkwrite,
 };
 
 static const struct address_space_operations fb_deferred_io_aops = {
@@ -246,11 +312,31 @@ static const struct address_space_operations fb_deferred_io_aops = {
 int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
+	vm_flags_t flags = VM_DONTEXPAND | VM_DONTDUMP;
 
 	vma->vm_ops = &fb_deferred_io_vm_ops;
-	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTDUMP);
-	if (!(info->flags & FBINFO_VIRTFB))
-		vm_flags_set(vma, VM_IO);
+	if (info->flags & FBINFO_KMEMFB) {
+		/*
+		 * I/O fault path calls vmf_insert_pfn(), which bug checks
+		 * if the vma is not marked shared. mmap'ing the framebuffer
+		 * as PRIVATE doesn't really make sense anyway, though doing
+		 * so isn't harmful for vmalloc() framebuffers. So there's
+		 * no prohibition for that case.
+		 */
+		if (!(vma->vm_flags & VM_SHARED))
+			return -EINVAL;
+		/*
+		 * Set VM_PFNMAP so mm code will not try to manage the pages'
+		 * lifecycles. We don't want individual pages to be freed
+		 * based on refcount. Instead the memory must be returned to
+		 * the free pool in the usual way. Cf. the implementation of
+		 * remap_pfn_range() and remap_pfn_range_internal().
+		 */
+		flags |= VM_PFNMAP | VM_IO;
+	} else if (!(info->flags & FBINFO_VIRTFB)) {
+		flags |= VM_IO;
+	}
+	vm_flags_set(vma, flags);
 	vma->vm_private_data = info;
 	return 0;
 }
diff --git a/include/linux/fb.h b/include/linux/fb.h
index cd653862ab99..ea2092757a18 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -402,6 +402,7 @@ struct fb_tile_ops {
 
 /* hints */
 #define FBINFO_VIRTFB		0x0004 /* FB is System RAM, not device. */
+#define FBINFO_KMEMFB		0x0008 /* FB is allocated in contig kernel mem */
 #define FBINFO_PARTIAL_PAN_OK	0x0040 /* otw use pan only for double-buffering */
 #define FBINFO_READS_FAST	0x0080 /* soft-copy faster than rendering */
 
-- 
2.25.1

[PATCH 3/3] fbdev: hyperv_fb: Fix mmap of framebuffers allocated using alloc_pages()

[mhkelley58]

From: Michael Kelley <mhklinux@outlook.com>

Framebuffer memory allocated using alloc_pages() was added to hyperv_fb in
commit 3a6fb6c4255c ("video: hyperv: hyperv_fb: Use physical memory for fb
on HyperV Gen 1 VMs.") in kernel version 5.6. But mmap'ing such
framebuffers into user space has never worked due to limitations in the
kind of memory that fbdev deferred I/O works with. As a result of the
limitation, hyperv_fb's usage results in memory free lists becoming corrupt
and Linux eventually panics.

With support for framebuffers allocated using alloc_pages() recently added
to fbdev deferred I/O, fix the problem by setting the flag telling fbdev
deferred I/O to use the new support.

Fixes: 3a6fb6c4255c ("video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.")
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
 drivers/video/fbdev/hyperv_fb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 75338ffc703f..1698221f857e 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -1020,6 +1020,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
 			info->fix.smem_len = screen_fb_size;
 			info->screen_base = par->mmio_vp;
 			info->screen_size = screen_fb_size;
+			info->flags |= FBINFO_KMEMFB;
 
 			par->need_docopy = false;
 			goto getmem_done;
-- 
2.25.1

Re: [PATCH 1/3] mm: Export vmf_insert_mixed_mkwrite()

[Christoph Hellwig]

On Tue, Apr 08, 2025 at 11:36:44AM -0700, mhkelley58@gmail.com wrote:
> From: Michael Kelley <mhklinux@outlook.com>
> 
> Export vmf_insert_mixed_mkwrite() for use by fbdev deferred I/O code,

But they are using this on dma coherent memory, where you can't legally
get at the page.  As told last time you need to fix that first before
hacking around that code.

> which can be built as a module. For consistency with the related function
> vmf_insert_mixed(), export without the GPL qualifier.

No.  All advanced new Linux functionality must be _GPL.  Don't try to
sneak around that.

Re: [PATCH 2/3] fbdev/deferred-io: Support contiguous kernel memory framebuffers

[Christoph Hellwig]

On Tue, Apr 08, 2025 at 11:36:45AM -0700, mhkelley58@gmail.com wrote:
> In any case, for x86 and arm64 today, commit 37b4837959cb9 is not
> sufficient to support contiguous kernel memory framebuffers. The problem
> can be seen with the hyperv_fb driver, which may allocate the framebuffer
> memory using vmalloc() or alloc_pages(), depending on the configuration
> of the Hyper-V guest VM (Gen 1 vs. Gen 2) and the size of the framebuffer.

And neither is this code.  You need to provide the functionality at
the DMA layer as users must not poke into the returned DMA coherent
memory.

RE: [PATCH 1/3] mm: Export vmf_insert_mixed_mkwrite()

[Michael Kelley]

From: Christoph Hellwig <hch@lst.de> Sent: Wednesday, April 9, 2025 3:50 AM
> 
> On Tue, Apr 08, 2025 at 11:36:44AM -0700, mhkelley58@gmail.com wrote:
> > From: Michael Kelley <mhklinux@outlook.com>
> >
> > Export vmf_insert_mixed_mkwrite() for use by fbdev deferred I/O code,
> 
> But they are using this on dma coherent memory, where you can't legally
> get at the page.  As told last time you need to fix that first before
> hacking around that code.

Hmmm. What's the reference to "as told last time"? I don't think I've had
this conversation before.

For the hyperv_fb driver, the memory in question is allocated with a direct call
to alloc_pages(), not via dma_alloc_coherent(). There's no DMA in this scenario.
The memory is shared with the Hyper-V host and designated as the memory
for the virtual framebuffer device. It is then mapped into user space using the
mmap() system call against /dev/fb0. User space writes to the memory are
eventually (and I omit the details) picked up by the Hyper-V host and displayed.

Is your point that memory dma_alloc_coherent() memory must be treated as
a black box, and can't be deconstructed into individual pages? If so, that makes
sense to me. But must the same treatment be applied to memory from
alloc_pages()? This is where I need some education.

> 
> > which can be built as a module. For consistency with the related function
> > vmf_insert_mixed(), export without the GPL qualifier.
> 
> No.  All advanced new Linux functionality must be _GPL.  Don't try to
> sneak around that.

No problem. Just trying to be consistent, not sneak around anything.

Thanks,

Michael

Re: [PATCH 1/3] mm: Export vmf_insert_mixed_mkwrite()

[Christoph Hellwig]

On Wed, Apr 09, 2025 at 02:10:26PM +0000, Michael Kelley wrote:
> Hmmm. What's the reference to "as told last time"? I don't think I've had
> this conversation before.

Hmm, there was a conversation about deferred I/O, and I remember the
drm folks even defending their abuse of vmalloc_to_page on dma coherent
memory against the documentation in the most silly way.  Maybe that was
a different discussion of the same thing.

> 
> For the hyperv_fb driver, the memory in question is allocated with a direct call
> to alloc_pages(), not via dma_alloc_coherent(). There's no DMA in this scenario.
> The memory is shared with the Hyper-V host and designated as the memory
> for the virtual framebuffer device. It is then mapped into user space using the
> mmap() system call against /dev/fb0. User space writes to the memory are
> eventually (and I omit the details) picked up by the Hyper-V host and displayed.

Oh, great.

> Is your point that memory dma_alloc_coherent() memory must be treated as
> a black box, and can't be deconstructed into individual pages? If so, that makes
> sense to me.

Yes.

> But must the same treatment be applied to memory from
> alloc_pages()? This is where I need some education.

No, that's just fine.

RE: [PATCH 1/3] mm: Export vmf_insert_mixed_mkwrite()

[Michael Kelley]

From: Christoph Hellwig <hch@lst.de> Sent: Thursday, April 10, 2025 12:42 AM
> 
> On Wed, Apr 09, 2025 at 02:10:26PM +0000, Michael Kelley wrote:
> > Hmmm. What's the reference to "as told last time"? I don't think I've had
> > this conversation before.
> 
> Hmm, there was a conversation about deferred I/O, and I remember the
> drm folks even defending their abuse of vmalloc_to_page on dma coherent
> memory against the documentation in the most silly way.  Maybe that was
> a different discussion of the same thing.

Yes, must have been a different discussion.

Turns out the hyperv_fb driver in a different configuration *is* using
dma_alloc_coherent() to get framebuffer memory, which is then managed
by deferred I/O. dma_alloc_coherent() is used as a wrapper around
cma_alloc() but only when the framebuffer size is > 4 MiB. The deferred
I/O code doesn't do vmalloc_to_page() since the CPU address from
dma_alloc_coherent() isn't a vmalloc addr.

That combo works OK, so wasn't something to be fixed in this patch set.
I'll see about a separate patch to just use cma_alloc() directly for that
case, though cma_alloc() and cma_release() would need to be EXPORTed.

> 
> >
> > For the hyperv_fb driver, the memory in question is allocated with a direct call
> > to alloc_pages(), not via dma_alloc_coherent(). There's no DMA in this scenario.
> > The memory is shared with the Hyper-V host and designated as the memory
> > for the virtual framebuffer device. It is then mapped into user space using the
> > mmap() system call against /dev/fb0. User space writes to the memory are
> > eventually (and I omit the details) picked up by the Hyper-V host and displayed.
> 
> Oh, great.
> 
> > Is your point that memory dma_alloc_coherent() memory must be treated as
> > a black box, and can't be deconstructed into individual pages? If so, that makes
> > sense to me.
> 
> Yes.

I'll add some comments to the fbdev deferred I/O code saying not to use it
with a framebuffer allocated with dma_alloc_coherent().

Thanks,

Michael

> 
> > But must the same treatment be applied to memory from
> > alloc_pages()? This is where I need some education.
> 
> No, that's just fine.