On 8/4/26 14:05, Paolo Bonzini wrote:
> Right now, users of .pfn_mkwrite() have no way to create a PTE
> that has gone through maybe_mkwrite(). Because vma_set_page_prot()
> will have cleared the writable PTE bit, users of fixup_user_fault()
> will see a read-only PTE and have no clue that the page needs
> a *second* fault to reach its final status.
>
> Handling this in fixup_user_fault() is problematic: the information
> about the presence of *_mkwrite is only recorded in vma->vm_page_prot,
> which is an opaque pgprot_t, therefore only follow_pfnmap_start()
> knows how to retrieve it.
>
> There are actually some preexisting functions that suggest how this
> is supposed to be handled, namely vmf_insert_page_mkwrite() and
> vmf_insert_pfn_pmd(). Fixing the drivers requires similar variants
> of vm_insert_pfn(), namely vmf_insert_pfn_mkwrite() for the common
> case where vma->vm_page_prot is okay, and vmf_insert_pfn_prot_mkwrite()
> when really all parameters are needed. This makes it possible
> to fix drivers that use .pfn_mkwrite together with
> vmf_insert_pfn() and vmf_insert_pfn_prot().
>
> Since vmf_insert_pfn_prot_mkwrite() is the most general variant
> and all the others are just special cases, turn them into inline
> functions in the header.
>
> Fixes: 28e3918179aa ("drm/gem-shmem: Track folio accessed/dirty status in
> mmap")
> Cc: [email protected]
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> include/linux/mm.h | 81 +++++++++++++++++++++++++++++++++++++++++---
> mm/huge_memory.c | 2 +-
> mm/memory.c | 84 ++++++++++++++++++++--------------------------
> 3 files changed, 114 insertions(+), 53 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 485df9c2dbdd..01184a4bdd6f 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4544,16 +4544,89 @@ int vm_map_pages_zero(struct vm_area_struct *vma,
> struct page **pages,
> unsigned long num);
> vm_fault_t vmf_insert_page_mkwrite(struct vm_fault *vmf, struct page *page,
> bool write);
> -vm_fault_t vmf_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
> - unsigned long pfn);
> -vm_fault_t vmf_insert_pfn_prot(struct vm_area_struct *vma, unsigned long
> addr,
> - unsigned long pfn, pgprot_t pgprot);
> +vm_fault_t vmf_insert_pfn_prot_mkwrite(struct vm_area_struct *vma, unsigned
> long addr,
> + unsigned long pfn, pgprot_t pgprot, bool mkwrite);
> 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);
> int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned
> long len);
>
To not inflate mm.h too much, can we just try removing all details that can also
be had in vmf_insert_pfn_prot_mkwrite() doc, and refer to that?
> +
> +/**
> + * vmf_insert_pfn_prot - insert single pfn into user vma with specified
> pgprot
> + * @vma: user vma to map to
> + * @addr: target user address of this page
> + * @pfn: source kernel pfn
> + * @pgprot: pgprot flags for the inserted page
> + *
> + * This is exactly like vmf_insert_pfn(), except that it allows drivers
> + * to override pgprot on a per-page basis. For more information,
> + * see vmf_insert_pfn_prot_mkwrite().
For example, I would keep this statement here for all 3 variants.
> + *
> + * This only makes sense for IO mappings, and it makes no sense for
> + * COW mappings. In general, using multiple vmas is preferable;
> + * vmf_insert_pfn_prot should only be used if using multiple VMAs is
> + * impractical.
Can we just move that for vmf_insert_pfn_prot_mkwrite() and document it when
pgprot != vma->vm_page_prot ?
> + *
> + * Context: Process context. May allocate using %GFP_KERNEL.
> + * Return: vm_fault_t value.
> + */
> +static inline vm_fault_t vmf_insert_pfn_prot(struct vm_area_struct *vma,
> + unsigned long addr, unsigned long pfn, pgprot_t pgprot)
> +{
> + return vmf_insert_pfn_prot_mkwrite(vma, addr, pfn, pgprot, false);
> +}
> +
> +/**
> + * vmf_insert_pfn_mkwrite - insert single pfn into user vma, possibly
> writable
> + * @vma: user vma to map to
> + * @addr: target user address of this page
> + * @pfn: source kernel pfn
> + * @write: whether the PTE should be installed writable
> + *
> + * Like vmf_insert_pfn(), except that @write allows installing a writable
> + * PTE even when @vma is under write notification. For more information,
> + * see vmf_insert_pfn_prot_mkwrite().
> + *
> + * Note that neither .pfn_mkwrite() nor .page_mkwrite() is invoked, so the
> + * caller must itself do whatever they would have done if @write is true.
Similarly move that to vmf_insert_pfn_prot_mkwrite().
> + *
> + * Context: Process context. May allocate using %GFP_KERNEL.
> + * Return: vm_fault_t value.
> + */
> +static inline vm_fault_t vmf_insert_pfn_mkwrite(struct vm_area_struct *vma,
> + unsigned long addr, unsigned long pfn, bool write)
> +{
> + return vmf_insert_pfn_prot_mkwrite(vma, addr, pfn, vma->vm_page_prot,
> write);
> +}
> +
> +/**
> + * vmf_insert_pfn - insert single pfn into user vma
> + * @vma: user vma to map to
> + * @addr: target user address of this page
> + * @pfn: source kernel pfn
> + *
> + * Similar to vm_insert_page, this allows drivers to insert individual pages
> + * they've allocated into a user vma. Same comments apply.
I know that you are moving this doc, but some things stick out:
Wouldn't it be better to also refer to vmf_insert_pfn() instead, like all the
other variants?
> + *
> + * This function should only be called from a vm_ops->fault handler, and
> + * in that case the handler should return the result of this function.
Isn't this the same for the other ones as well?
> + *
> + * vma cannot be a COW mapping.
Isn't this the same for all of them?
> + *
> + * As this is called only for pages that do not currently exist, we
> + * do not need to flush old virtual caches or the TLB.
Isn't this an implementation detail?
> + *
> + * Context: Process context. May allocate using %GFP_KERNEL.
> + * Return: vm_fault_t value.
> + */
> +static inline vm_fault_t vmf_insert_pfn(struct vm_area_struct *vma,
> + unsigned long addr, unsigned long pfn)
> +{
> + return vmf_insert_pfn_mkwrite(vma, addr, pfn, false);
> +}
> +
Apart from that LGTM.
--
Cheers,
David
