Re: [PATCH 2/4] mm: add vmf_insert_pfn_order() and vmf_pfn_order_ok() helpers

["David Hildenbrand (Arm)" <david@kernel.org>]

On 11/11/26 12:30, Eliav Farber wrote:
> ->fault/->huge_fault handlers of VM_PFNMAP mappings that want huge PFN
> mappings open-code the same steps: check that the order-aligned fault
> window lies within the VMA and that the pfn is aligned to the order,
> then call vmf_insert_pfn(), vmf_insert_pfn_pmd() or vmf_insert_pfn_pud()
> depending on the order, guarded by
> CONFIG_ARCH_SUPPORTS_{PMD,PUD}_PFNMAP, and fall back to a smaller order
> otherwise. vfio-pci has its own is_aligned_for_order() and
> vfio_pci_vmf_insert_pfn() for this.
> 
> Add vmf_pfn_order_ok() for the window and alignment check, and
> vmf_insert_pfn_order() for the order dispatch. The latter also checks
> pfn_modify_allowed() for huge orders: the order-0 vmf_insert_pfn() does
> this, but vmf_insert_pfn_pmd() and vmf_insert_pfn_pud() do not. The two
> are separate so that a driver can apply its own policy (access checks,
> device locking) between them.
> 
> vmf_insert_pfn_order() passes the fault's write flag to
> vmf_insert_pfn_pmd() and vmf_insert_pfn_pud(), as the DAX and DRM
> ->huge_fault handlers do. With write=false, insert_pmd() leaves a
> read-only entry in place and returns VM_FAULT_NOPAGE, so a store to a
> mapping that mm write-protected for tracking (soft-dirty, uffd-wp)
> faults forever; wp_huge_pmd() splits the entry only when the handler
> returns VM_FAULT_FALLBACK. Passing it marks the entry young, dirty and,
> where the VMA allows, writable, as wp_page_reuse() does at PTE level.
> The order-0 path needs nothing: wp_pfn_shared() resolves that fault.
> 
> The helper also declines the huge zero folio's pfn, which
> vmf_insert_pfn_pmd() refuses with SIGBUS, so that the fault is retried
> at PTE order where it maps.
> 
> They will be used by the /dev/mem and UIO huge_fault handlers added
> later in this series.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  include/linux/mm.h | 28 ++++++++++++++++++++++++++
>  mm/memory.c        | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index c11bdee1596a..57437b317709 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -5010,6 +5010,34 @@ vm_fault_t vmf_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
>  			unsigned long pfn);
>  vm_fault_t vmf_insert_mixed_mkwrite(struct vm_area_struct *vma,
>  		unsigned long addr, unsigned long pfn);
> +vm_fault_t vmf_insert_pfn_order(struct vm_fault *vmf, unsigned long pfn,
> +				unsigned int order);
> +
> +/**
> + * vmf_pfn_order_ok - check that a pfn can be mapped at a given fault order
> + * @vmf: The fault being handled.
> + * @pfn: The pfn to map at the start of the order-aligned fault window.
> + * @order: The fault order (0 for a single page).
> + *
> + * For a huge (order > 0) fault, the order-aligned window around @vmf->address
> + * must lie within the VMA and @pfn must be aligned to @order. @pfn is only
> + * looked at once the window is known to lie within the VMA, so a caller may
> + * compute it before calling, even when the window starts below vm_start.
> + *
> + * Return: true if vmf_insert_pfn_order() may map @pfn at @order, false if the
> + * fault should fall back to a smaller order.
> + */
> +static inline bool vmf_pfn_order_ok(const struct vm_fault *vmf,
> +				    unsigned long pfn, unsigned int order)
> +{
> +	const struct vm_area_struct *vma = vmf->vma;
> +	unsigned long addr = vmf->address & ~((PAGE_SIZE << order) - 1);
> +
> +	return !order || (addr >= vma->vm_start &&
> +			  addr + (PAGE_SIZE << order) <= vma->vm_end &&
> +			  IS_ALIGNED(pfn, 1UL << order));

To make this somewhat consumable for a human,

if (!order)
	return true;
return start >= vma->vm_start && end <= vma->vm_end;

And maybe rejecting IS_ALIGNED(pfn, 1UL << order) with WARN_ONCE() as this
sounds like the user is doing something stupid?

But looking again, this all looks like sanity checks where the user is just
doing something stupid?

Why is that required at all? I'd get rid of this entirely.


> +}
> +
>  int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long len);
>  
>  static inline vm_fault_t vmf_insert_page(struct vm_area_struct *vma,
> diff --git a/mm/memory.c b/mm/memory.c
> index ab29ec719ae1..0c583442e049 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3002,6 +3002,55 @@ vm_fault_t vmf_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
>  }
>  EXPORT_SYMBOL(vmf_insert_pfn);
>  
> +/**
> + * vmf_insert_pfn_order - insert a pfn mapping of a given fault order
> + * @vmf: The fault being handled.
> + * @pfn: The pfn to map at the start of the order-aligned fault window.
> + * @order: The fault order (0 for a single page).
> + *
> + * Helper for ->fault and ->huge_fault handlers of VM_PFNMAP mappings: maps @pfn
> + * with a PTE for order 0, or with a PMD/PUD for PMD_ORDER/PUD_ORDER where the
> + * architecture supports huge PFN mappings. The caller must have checked the
> + * window and the pfn alignment with vmf_pfn_order_ok().
> + *
> + * vma cannot be a COW mapping.
> + *
> + * Return: vm_fault_t value; %VM_FAULT_FALLBACK if @order cannot be mapped, so
> + * that the fault is retried at a smaller order.
> + */
> +vm_fault_t vmf_insert_pfn_order(struct vm_fault *vmf, unsigned long pfn,
> +				unsigned int order)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	bool write = vmf->flags & FAULT_FLAG_WRITE;
> +
> +	if (!order)
> +		return vmf_insert_pfn(vma, vmf->address, pfn);
> +
> +	/*
> +	 * vmf_insert_pfn_pmd() refuses the huge zero folio's pfn with SIGBUS,
> +	 * while a PTE mapping of it is fine, so fall back to a smaller order.
> +	 */
> +	if (is_huge_zero_pfn(pfn))
> +		return VM_FAULT_FALLBACK;
> +
> +	/*
> +	 * vmf_insert_pfn_pmd()/_pud() don't call pfn_modify_allowed(), unlike
> +	 * the order-0 path, so check it explicitly for the L1TF mitigation.
> +	 */
> +	if (!pfn_modify_allowed(pfn, vma->vm_page_prot))
> +		return VM_FAULT_FALLBACK;
> +
> +	if (IS_ENABLED(CONFIG_ARCH_SUPPORTS_PMD_PFNMAP) && order == PMD_ORDER)
> +		return vmf_insert_pfn_pmd(vmf, pfn, write);
> +
> +	if (IS_ENABLED(CONFIG_ARCH_SUPPORTS_PUD_PFNMAP) && order == PUD_ORDER)
> +		return vmf_insert_pfn_pud(vmf, pfn, write);
> +
> +	return VM_FAULT_FALLBACK;
> +}
> +EXPORT_SYMBOL_GPL(vmf_insert_pfn_order);
> +
>  static bool vm_mixed_ok(struct vm_area_struct *vma, unsigned long pfn,
>  			bool mkwrite)
>  {


Why do we even care about the order? Why isn't it sufficient that we get called
with "insert this pfn range" and we see how to best map it (pfn/pmd/pud)?

Why does the caller even have to care about *how* something is mapped?

-- 
Cheers,

David