On Tue, Aug 04, 2026 at 02:05:25PM +0200, Paolo Bonzini wrote:
> This ensures that fixup_user_fault() users see a writable PTE when
> they request one. The flip side is that vmw_bo_vm_fault() now has
> to record by hand the write fault, because .pfn_mkwrite() is
> not invoked.
>
> Prefaulting works as before because only the first entry comes
> out writable, while the following ones still end up executing
> the .pfn_mkwrite() callback.
>
> Cc: [email protected]
> Signed-off-by: Paolo Bonzini <[email protected]>
Only some quick thoughts while reading through this, as below.. even if
some of it may make sense, I think that may be more suitable as follow up.
This looks like a good fix for a regression already to me.
> ---
> drivers/gpu/drm/ttm/ttm_bo_vm.c | 7 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 42 ++++++++++++----------
> 2 files changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index a80510489c45..3ebde936ce60 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -191,6 +191,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
> unsigned long pfn;
> struct ttm_tt *ttm = NULL;
> struct page *page;
> + bool mkwrite;
> int err;
> pgoff_t i;
> vm_fault_t ret = VM_FAULT_NOPAGE;
> @@ -242,6 +243,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
> * Speculatively prefault a number of pages. Only error on
> * first page.
> */
> + mkwrite = !!(vmf->flags & FAULT_FLAG_WRITE);
> for (i = 0; i < num_prefault; ++i) {
> if (bo->resource->bus.is_iomem) {
> pfn = ttm_bo_io_mem_pfn(bo, page_offset);
> @@ -263,9 +265,10 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
> * at arbitrary times while the data is mmap'ed.
> * See vmf_insert_pfn_prot() for a discussion.
> */
> - ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
> + ret = vmf_insert_pfn_prot_mkwrite(vma, address, pfn, prot,
> mkwrite);
>
> - /* Never error on prefaulted PTEs */
> + /* Never error on prefaulted PTEs and never map them writable */
I got confused when reading 1st time, but I got it then noticing the mark
dirty was done by the caller.
Two small things I thought about here:
- Comparing to the time before introducing pfn_mkwrite(), this will cause
previously one fault (with prefaults marking all follow up ptes writable)
to be 1 writable plus N-1 read-only. May not be the most ideal if we
consider the 2nd WP faults on the rest N-1 later as slight overheads,
- Split the "mark WRITABLE" and "mark DIRTY" in code might be slightly
error prone, especially if this is a common function used by multiple
drivers, while there's only one driver that does the "mark DIRTY".
IIUC the other idea can be, do not reset @mkwrite here but instead move the
set dirty here, invoking whatever the vma's .pfn_mkwrite() is. So that we
stick two things together; maybe slightly less error prone and less dup
code when other drivers opt-in for pfn_mkwrite().
Not sure if it's a good idea, but just to raise it in case useful. Again,
I still think this is a solid fix to the problem already.
Other than that, FWIW the whole approach looks reasonable at least to me.
I agree in the fault processing we should best resolve the fault in one
shot if possible. In this context, FAULT_FLAG_WRITE is the flag showing
that a 2nd fault is required, then IMHO it's indeed better to resolve the
fault in one go, as proposed in this series.
Another thing I came to mind that may not really be relevant to this
regression alone, but maybe matters for the future to at least keep in
mnind: I wonder if there can be races happen while fixup_user_fault() is
resolving faults, causing the 2nd pfnmap follow code to fail once more,
say, some other thread modified the pgtable again (e.g. wr-protect with
write bit removed right after set).
So maybe pfnmap lookup and fixup_user_fault() should be done in a loop
until any of them hit real errors.. if any of such race may become a real
problem some day.
Looks like low possibility that threads will mess up with PFN maps.. but
just to raise this idea.
I believe currently our mm fault handler should be working like that with
handle_mm_fault(), hence neutral with such races (it'll loop a few more
rounds until race disappear). I recall there used to have thoughts adding
some n_retry_max counts to the fault handler, but we didn't really do that,
and it runs all fine over the years.
Thanks,
--
Peter Xu
