On Mon, Oct 14, 2024 at 11:47:58PM -0700, Christoph Hellwig wrote: > Hi Lorenzo, > > sorry for only replying to this so late. No worries, and thanks for taking a look! :) > > On Fri, Sep 27, 2024 at 01:51:11PM +0100, Lorenzo Stoakes wrote: > > The existing generic pagewalk logic permits the walking of page tables, > > invoking callbacks at individual page table levels via user-provided > > mm_walk_ops callbacks. > > > > This is useful for traversing existing page table entries, but precludes > > the ability to establish new ones. > > > > Existing mechanism for performing a walk which also installs page table > > entries if necessary are heavily duplicated throughout the kernel, each > > with semantic differences from one another and largely unavailable for use > > elsewhere. > > I do like the idea of having common code for installing page tables! > Awesome. > Minor nits below: > > > +int walk_page_range_mm(struct mm_struct *mm, unsigned long start, > > unsigned long end, const struct mm_walk_ops *ops, > > void *private) > > It would be good to have a minimum level of documentation for this > function, including how it differs from walk_page_range and why > it should remain internal. Will add on respin! > > > + /* For internal use only. */ > > + if (ops->install_pte) > > + return -EINVAL; > > And this should probably be expanded a bit, including that no exported > symbol should allow inserting arbitrary PTEs. Maybe best done with > a helper to share that comment with the other places that have this > check. > Yeah a helper makes sense actually, a more general 'are these ops valid?' thing. Will update on next respin with some explanation. The next iteration I plan to un-RFC as seems generally the concept is unopposed for this series, will make these changes then. Thanks!
