On Mon, Oct 12, 2026 at 05:03:02PM +0200, David Hildenbrand (Arm) wrote:
> On 10/9/26 20:04, Lorenzo Stoakes (ARM) wrote:
> > The existing kernel page mapping mmap actions allow for partial and full
> > mapping of an array of struct page pointers.
> >
> > However some drivers require the mapping of discontiguous ranges. Permit
> > this by providing discontig_kernel_page_ops which allows a driver to
> > specify how the operation should begin and how batches of pages should be
> > retrieved.
> >
> > It uses the minimum exposed interface to do so, providing address, page
> > offset and both vm_private_data state and a local private state object.
> >
> > ops->init can establish state for the operation, and ops->get outputs the
> > pages to map and their count. Should an error arise the core unmaps the
> > VMA, invoking vm_ops->close, which is therefore where any state established
> > by ops->init is released.
> >
> > Also update the compatibility layer to do this too via
> > compat_mmap_action_abort().
> >
> > Batches may not exceed the VMA, but may map less than its full range in
> > case the driver wishes to allow the user to map an area larger than the
> > available data.
> >
> > To use it, users invoke mmap_action_map_discontig_kernel_pages() with
> > initial local private state and a set of operations.
> >
> > Users can then use one of the provided helper functions to perform an
> > action:
> >
> > * discontig_kernel_map_abort() - Abort and leave the mapping as it has
> > been accumulated so far.
> > * discontig_kernel_map_page() - Map a single page, or a compound page given
> > its head page.
> > * discontig_kernel_map_page_range() - Maps a struct page ** array of a
> > specified count, mapping only those pages which fit in the VMA.
> >
> > While here, update vm_insert_page() and vm_insert_pages() to assert the
> > mmap write lock is held rather than the bizarre assert !mmap_read_trylock()
> > and use the new VMA API so all code paths are consistent.
> >
> > Also convert a BUG_ON() to a VM_WARN_ON_ONCE() in vm_insert_page().
> >
> > The userland VMA tests are updated accordingly.
> >
> > Acked-by: Zi Yan <[email protected]>
> > Signed-off-by: Lorenzo Stoakes (ARM) <[email protected]>
>
>
> Finally finding time to go over this. In the future I want to have more time
> to
> review such massive series, as it causes severe stress to be forced to review
> this last minute before it goes upstream.
>
> So if I push back in order to give me some more room to breathe in the future
> it's nothing personal, I just have to keep my sanity somehow.
>
> I hate being forced to review because we pick up stuff way to early. There is
> just way too much other stuff going on.
>
> I'm planning on picking up most stuff I maintain in the next cycle. For
> cross-component stuff (esp. VMA) I expect Andrew to pick stuff up until we
> tackle this once cycle later.
OK I understand, sorry for sending such a large series.
I already delayed it from last cycle and sent early in rc2, but I understand
there's been a lot of volume in this one.
I've been working evenings and weekends myself to keep up with review (and
failing), so I get how thankless it can be (LPC was also unhelpfully timed!)
I will try to split things out more in future, this had a lot of dependencies so
it wasn't clear that made sense + wouldn't just cause more stress over a longer
period otherwise.
>
>
> > +static inline
> > +void mmap_action_map_discontig_kernel_pages(struct vm_area_desc *desc,
> > + void *init_private, const struct discontig_kernel_page_ops *ops)
> > +{
> > + struct mmap_action *action = &desc->action;
> > +
> > + action->type = MMAP_DISCONTIG_KERNEL_PAGES;
> > + action->map_kernel_discontig.init_private = init_private;
> > + action->map_kernel_discontig.ops = ops;
> > +}
> > +
> > int mmap_action_prepare(struct vm_area_desc *desc);
> > int mmap_action_complete(struct vm_area_struct *vma,
> > struct mmap_action *action, bool is_compat);
>
> There is a mixture of two tab vs. different in your changes.
OK. mm is fairly inconsistent about this, I will convert all to 2 tabs.
>
> >
> > +static inline void
> > +discontig_kernel_map_abort(struct discontig_kernel_page_state *state)
> > +{
> > + state->action = DISCONTIG_KERNEL_PAGE_ABORT;
> > +}
> > +
> > +static inline void
> > +discontig_kernel_map_page(struct discontig_kernel_page_state *state,
> > + struct page *page)
> > +{
> > + struct folio *folio = page_folio(page);
> > +
> > + if (folio_test_large(folio)) {
> > + VM_WARN_ON_ONCE(page != folio_page(folio, 0));
> > + state->action = DISCONTIG_KERNEL_PAGE_MAP_COMPOUND_PAGE;
> > + state->__folio = folio;
> > + state->__nr_pages = min(state->nr_pages_remain,
> > + folio_nr_pages(folio));
> > + } else {
> > + state->action = DISCONTIG_KERNEL_PAGE_MAP_PAGE;
> > + state->__page = page;
> > + state->__nr_pages = 1;
> > + }
>
> Why are both separate? Isn't it sufficient to represent that through _page +
> _nr_pages > 1 ? See below.
Ack yup that works.
>
> > +}
> > +
> > +static inline void
> > +discontig_kernel_map_page_range(struct discontig_kernel_page_state *state,
> > + struct page **page_arr, unsigned long nr_pages)
>
> Is this actually "map_pages"? A range is rather a number of consectuive pages
> part of the same folio/compound page, no?
>
> (we call insert_pages() later ... )
Sure, can change.
>
> > +{
> > + state->action = DISCONTIG_KERNEL_PAGE_MAP_PAGE_RANGE;
> > + state->__page_arr = page_arr;
> > + state->__nr_pages = min(nr_pages, state->nr_pages_remain);
>
> So we expect all of them to be non-compound?
Yup this matches existing insert_pages() behaviour.
>
>
> [...]
>
> > diff --git a/mm/internal.h b/mm/internal.h
> > index fc4e249e2343..557e629db8c5 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -1515,6 +1515,9 @@ int simple_ioremap_prepare(struct vm_area_desc *desc);
> > int map_kernel_pages_prepare(struct vm_area_desc *desc);
> > int map_kernel_pages_complete(struct vm_area_struct *vma,
> > struct mmap_action *action);
> > +int map_discontig_kernel_pages_prepare(struct vm_area_desc *desc);
> > +int map_discontig_kernel_pages_complete(struct vm_area_struct *vma,
> > + struct mmap_action *action);
> >
> > static inline int io_remap_pfn_range_prepare(struct vm_area_desc *desc)
> > {
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 448342883e9d..e5c1d1c35bc7 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -2599,27 +2599,34 @@ int vm_insert_pages(struct vm_area_struct *vma,
> > unsigned long addr,
> >
> > if (!range_in_vma(vma, addr, end))
> > return -EFAULT;
> > - if (!(vma->vm_flags & VM_MIXEDMAP)) {
> > - VM_WARN_ON_ONCE(mmap_read_trylock(vma->vm_mm));
> > - VM_WARN_ON_ONCE(vma->vm_flags & VM_PFNMAP);
> > - vm_flags_set(vma, VM_MIXEDMAP);
> > + if (!vma_test(vma, VMA_MIXEDMAP_BIT)) {
> > + mmap_assert_write_locked(vma->vm_mm);
> > + VM_WARN_ON_ONCE(vma_test(vma, VMA_PFNMAP_BIT));
>
> I would prefer to have these changes in a separate patch. Or is it strictly
> required to have this in this very patch?
That was because Suren raised the 'assert on mmap_read_trylock()' thing and I
wanted to get the other cases here to be consistent.
I will split it out into another patch.
>
>
> > + vma_start_write(vma);
> > + vma_set_flags(vma, VMA_MIXEDMAP_BIT);
> > }
> > /* Defer page refcount checking till we're about to map that page. */
> > return insert_pages(vma, addr, pages, num, vma->vm_page_prot);
> > }
> > EXPORT_SYMBOL(vm_insert_pages);
> >
> > +static void __map_kernel_pages_prepare(struct vm_area_desc *desc)
> > +{
> > + if (vma_desc_test(desc, VMA_MIXEDMAP_BIT))
> > + return;
> > +
> > + mmap_assert_write_locked(desc->mm);
> > + VM_WARN_ON_ONCE(vma_desc_test(desc, VMA_PFNMAP_BIT));
> > + vma_desc_set_flags(desc, VMA_MIXEDMAP_BIT);
> > +}
> > +
> > int map_kernel_pages_prepare(struct vm_area_desc *desc)
> > {
> > const struct mmap_action *action = &desc->action;
> > const unsigned long addr = action->map_kernel.start;
> > unsigned long nr_pages, end;
> >
> > - if (!vma_desc_test(desc, VMA_MIXEDMAP_BIT)) {
> > - VM_WARN_ON_ONCE(mmap_read_trylock(desc->mm));
> > - VM_WARN_ON_ONCE(vma_desc_test(desc, VMA_PFNMAP_BIT));
> > - vma_desc_set_flags(desc, VMA_MIXEDMAP_BIT);
> > - }
> > + __map_kernel_pages_prepare(desc);
> >
> > nr_pages = action->map_kernel.nr_pages;
> > end = addr + PAGE_SIZE * nr_pages;
> > @@ -2640,6 +2647,98 @@ int map_kernel_pages_complete(struct vm_area_struct
> > *vma,
> > &nr_pages, vma->vm_page_prot);
> > }
> >
> > +int map_discontig_kernel_pages_prepare(struct vm_area_desc *desc)
> > +{
> > + const struct mmap_action *action = &desc->action;
> > + const struct discontig_kernel_page_ops *ops =
> > + action->map_kernel_discontig.ops;
> > +
> > + /* At minimum need to be able to get pages. */
> > + if (WARN_ON_ONCE(!ops || !ops->get))
> > + return -EINVAL;
> > +
> > + __map_kernel_pages_prepare(desc);
> > + return 0;
> > +}
> > +
> > +static int apply_discontig_action(struct vm_area_struct *vma,
> > + struct discontig_kernel_page_state *state)
> > +{
> > + unsigned long nr_pages = state->__nr_pages;
> > + unsigned long addr = state->addr;
> > + unsigned long i;
> > +
> > + if (state->action == DISCONTIG_KERNEL_PAGE_MAP_PAGE)
> > + return insert_page(vma, addr, state->__page,
> > + vma->vm_page_prot, /*mkwrite=*/false);
> > + if (state->action == DISCONTIG_KERNEL_PAGE_MAP_PAGE_RANGE)
> > + return insert_pages(vma, addr, state->__page_arr,
> > + &nr_pages, vma->vm_page_prot);
> > +
> > + /* Compound folio - have to iterate through each page. */
> > + for (i = 0; i < nr_pages; i++, addr += PAGE_SIZE) {
> > + struct page *page = folio_page(state->__folio, i);
> > + int err;
> > +
> > + err = insert_page(vma, addr, page, vma->vm_page_prot,
> > + /*mkwrite=*/false);
> > + if (err)
> > + return err;
> > + }
>
> Summarizing some of my comments below, I wonder if this could be:
>
> DISCONTIG_KERNEL_PAGE_MAP_PAGES - array of individual apges
> DISCONTIG_KERNEL_PAGE_MAP_CONTIG_PAGES - 1..N contiguous pages
OK.
>
>
> if (state->action == DISCONTIG_KERNEL_PAGE_MAP_PAGES)
> return insert_pages(vma, addr, state->__page_arr,
> &nr_pages, vma->vm_page_prot);
>
> VM_WARN_ON_ONCE(state->action != DISCONTIG_KERNEL_PAGE_MAP_CONTIG_PAGES)
> for (i = 0; i < nr_pages; i++, addr += PAGE_SIZE) {
> struct page *page = state->__page + i;
> int err;
>
> err = insert_page(vma, addr, page, vma->vm_page_prot,
> /*mkwrite=*/false);
> if (err)
> return err;
> }
> ...
OK. Will respin for v6.
>
> --
> Cheers,
>
> David
--
Cheers, Lorenzo
