Re: [PATCH v2 3/6] drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is in use

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