On 8/4/26 14:05, 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]>
> ---
> 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 */
> + mkwrite = false;
As far as I can see that is a really bad idea, we do want the prefaulted PTEs
writeable.
The whole prefaulting is essentially an optimization for cases where taking a
fault has massively overhead (virtualization for example).
Wasn't vmf_insert_pfn_prot() making them writeable before?
Regards,
Christian.
> if (unlikely((ret & VM_FAULT_ERROR))) {
> if (i == 0)
> return VM_FAULT_NOPAGE;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> index 45561bc1c9ef..3099558c0762 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> @@ -398,15 +398,33 @@ void vmw_bo_dirty_clear_res(struct vmw_resource *res)
> dirty->end = res_start;
> }
>
> +static vm_fault_t vmw_bo_dirty_mkwrite(struct vm_fault *vmf, struct
> ttm_buffer_object *bo)
> +{
> + unsigned long page_offset;
> + struct vmw_bo *vbo = to_vmw_bo(&bo->base);
> +
> + page_offset = vmf->pgoff - drm_vma_node_start(&bo->base.vma_node);
> + if (unlikely(page_offset >= PFN_UP(bo->resource->size)))
> + return VM_FAULT_SIGBUS;
> +
> + if (vbo->dirty && vbo->dirty->method == VMW_BO_DIRTY_MKWRITE &&
> + !test_bit(page_offset, &vbo->dirty->bitmap[0])) {
> + struct vmw_bo_dirty *dirty = vbo->dirty;
> +
> + __set_bit(page_offset, &dirty->bitmap[0]);
> + dirty->start = min(dirty->start, page_offset);
> + dirty->end = max(dirty->end, page_offset + 1);
> + }
> + return 0;
> +}
> +
> vm_fault_t vmw_bo_vm_mkwrite(struct vm_fault *vmf)
> {
> struct vm_area_struct *vma = vmf->vma;
> struct ttm_buffer_object *bo = (struct ttm_buffer_object *)
> vma->vm_private_data;
> vm_fault_t ret;
> - unsigned long page_offset;
> unsigned int save_flags;
> - struct vmw_bo *vbo = to_vmw_bo(&bo->base);
>
> /*
> * mkwrite() doesn't handle the VM_FAULT_RETRY return value correctly.
> @@ -419,22 +437,7 @@ vm_fault_t vmw_bo_vm_mkwrite(struct vm_fault *vmf)
> if (ret)
> return ret;
>
> - page_offset = vmf->pgoff - drm_vma_node_start(&bo->base.vma_node);
> - if (unlikely(page_offset >= PFN_UP(bo->resource->size))) {
> - ret = VM_FAULT_SIGBUS;
> - goto out_unlock;
> - }
> -
> - if (vbo->dirty && vbo->dirty->method == VMW_BO_DIRTY_MKWRITE &&
> - !test_bit(page_offset, &vbo->dirty->bitmap[0])) {
> - struct vmw_bo_dirty *dirty = vbo->dirty;
> -
> - __set_bit(page_offset, &dirty->bitmap[0]);
> - dirty->start = min(dirty->start, page_offset);
> - dirty->end = max(dirty->end, page_offset + 1);
> - }
> -
> -out_unlock:
> + ret = vmw_bo_dirty_mkwrite(vmf, bo);
> dma_resv_unlock(bo->base.resv);
> return ret;
> }
> @@ -484,6 +487,9 @@ vm_fault_t vmw_bo_vm_fault(struct vm_fault *vmf)
> prot = vm_get_page_prot(vma->vm_flags);
>
> ret = ttm_bo_vm_fault_reserved(vmf, prot, num_prefault);
> + if (ret == VM_FAULT_NOPAGE && (vmf->flags & FAULT_FLAG_WRITE))
> + WARN_ON_ONCE(vmw_bo_dirty_mkwrite(vmf, bo));
> +
> if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
> return ret;
>
