On 1/19/25 23:23, Dmitry Osipenko wrote: > On 1/19/25 23:02, Dmitry Osipenko wrote: >> On 1/19/25 19:18, Sasha Finkelstein wrote: >>> On Sun, 19 Jan 2025 at 12:50, Dmitry Osipenko >>> <dmitry.osipenko@xxxxxxxxxxxxx> wrote: >>>>> ret = io_remap_pfn_range(vma, vma->vm_start, >>>>> - vram->vram_node.start >> PAGE_SHIFT, >>>>> + (vram->vram_node.start >> PAGE_SHIFT) + vma->vm_pgoff, >>>>> vm_size, vma->vm_page_prot); >>>>> return ret; >>>>> } >>>> >>>> The vma->vm_pgoff is fake in DRM, it's used for looking up DRM GEM >>>> object based on the vma->vm_pgoff value when mmap is invoked. >>> >>> If my understanding is correct, vm_pgoff gets "unfaked" by >>> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/virtio/virtgpu_vram.c#L48 >>> >>>> vma->vm_pgoff should be treated as zero here. Hence we can map a part of >>>> GEM, but only from its start. See drm_gem_mmap(). >>> >>> I've had a "v0" (not on ml) of this patch that always treated vma->vm_pgoff as >>> zero. This broke when anything tried to mmap with a non-zero offset. Adding >>> vm_pgoff made it work correctly. >> >> I've tested this patch. Partial mapping with a non-zero offset doesn't >> work because drm_gem_mmap() rejects it. I'd want to see your sample code >> that performs mmaping, maybe I'm missing something. >> >>>> Please correct vma->vm_pgoff in v2. >>> >>> I need apps to be able to mmap with a non-zero offset for my usecase. >>> While the correct value may be something else other than what is in >>> the current patch, 0 is definitely incorrect for at least some workloads. >> >> drm_gem_mmap() uses drm_vma_offset_exact_lookup_locked() that doesn't >> allow vma->vm_pgoff != node.start. AFAICT, no one driver supports >> mapping with a non-zero offset, perhaps for a reason that I don't know >> about. > > See now that a non-zero mapping of a dmabuf might work. Will test it. Works for dmabuf > > - /* Partial mappings of GEM buffers don't happen much in practice. */ > - if (vm_size != vram->vram_node.size) > + if (vm_size > vram->vram_node.size) > return -EINVAL; This check should include the vm_pgoff, like that: if (check_add_overflow(vma->vm_pgoff << PAGE_SHIFT, vm_size, &vm_end)) return -EINVAL; if (vm_end > vram->vram_node.size) return -EINVAL; The size and offset are actually validated before this code is reached, but doesn't hurt to keep the check around. I corrected the check and applied patch to misc-next, thanks! -- Best regards, Dmitry
