Thank you for your contribution! Sashiko AI review found 2 potential issues to consider: - [Medium] drm/virtio: virtio_gpu_userptr_restore() skips DMA remap after failed map - [Low] drm/virtio: duplicated virtio_gpu_userptr_free() and dead userptr struct fields --
commit a322ca4c583b559738cbbb3a5e62c08918a07dd8 Author: Honglei Huang <[email protected]> drm/virtio: implement userptr resource support This commit adds virtio_gpu_object_userptr so the guest kernel can pin an existing userspace mapping with FOLL_LONGTERM and pass its backing pages to the host via VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB, including hibernation restore support. > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h > b/drivers/gpu/drm/virtio/virtgpu_drv.h > index 9df4c7117341e..a29429b41561f 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_drv.h > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h [ ... ] > @@ -138,12 +139,42 @@ struct virtio_gpu_object_vram { > struct drm_mm_node vram_node; > }; > > +struct virtio_gpu_object_userptr; > + > +struct virtio_gpu_object_userptr_ops { > + int (*get_pages)(struct virtio_gpu_object_userptr *userptr); > + void (*put_pages)(struct virtio_gpu_object_userptr *userptr); > +}; > + > +struct virtio_gpu_object_userptr { > + struct virtio_gpu_object base; > + const struct virtio_gpu_object_userptr_ops *ops; > + /* Protects @pages, @sgt and @dma_mapped. */ > + struct mutex lock; > + > + uint64_t start; > + uint32_t npages; > + uint32_t bo_handle; > + uint32_t flags; > + > + struct virtio_gpu_device *vgdev; > + struct drm_file *file; > + struct page **pages; > + struct sg_table *sgt; > + bool dma_mapped; > + enum dma_data_direction dma_dir; > + struct mm_struct *mm; > +}; [ ... ] > diff --git a/drivers/gpu/drm/virtio/virtgpu_userptr.c > b/drivers/gpu/drm/virtio/virtgpu_userptr.c > new file mode 100644 > index 0000000000000..b8606932550eb > --- /dev/null > +++ b/drivers/gpu/drm/virtio/virtgpu_userptr.c > @@ -0,0 +1,350 @@ [ ... ] > +static void virtio_gpu_userptr_free(struct drm_gem_object *obj) > +{ > + struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj); > + struct virtio_gpu_device *vgdev = obj->dev->dev_private; > + > + /* > + * Pages stay pinned until RESOURCE_UNREF finishes. Its callback > + * calls virtio_gpu_cleanup_object to drop them. > + */ > + if (bo->created) { > + virtio_gpu_remove_from_restore_list(bo); > + virtio_gpu_cmd_unref_resource(vgdev, bo, false); > + virtio_gpu_notify(vgdev); > + return; > + } > + > + virtio_gpu_cleanup_object(bo); > +} [Severity: Low] This isn't a bug, but virtio_gpu_userptr_free() is identical to virtio_gpu_free_object() in drivers/gpu/drm/virtio/virtgpu_object.c and could reuse it directly if exported. In struct virtio_gpu_object_userptr, bo_handle is never used and file is only written in virtio_gpu_userptr_init() and never read (leaving an unreferenced struct drm_file pointer on an object that can outlive the file descriptor until RESOURCE_UNREF finishes). Also, struct virtio_gpu_object_userptr_ops has only one static implementation, so virtio_gpu_userptr_get_pages() and virtio_gpu_userptr_put_pages() can be called directly without the ops vtable. [ ... ] > +static int > +virtio_gpu_userptr_get_entries(struct virtio_gpu_device *vgdev, > + struct virtio_gpu_object_userptr *userptr, > + struct virtio_gpu_mem_entry **ents, > + unsigned int *nents) > +{ > + bool use_dma_api = virtio_gpu_use_dma_api(vgdev->vdev); > + struct scatterlist *sg; > + unsigned int count; > + int si; > + > + count = use_dma_api ? userptr->sgt->nents : userptr->sgt->orig_nents; > + if (!count) > + return -EINVAL; > + > + *ents = kvmalloc_array(count, sizeof(**ents), GFP_KERNEL); > + if (!*ents) > + return -ENOMEM; > + > + if (use_dma_api) { > + for_each_sgtable_dma_sg(userptr->sgt, sg, si) { > + (*ents)[si].addr = cpu_to_le64(sg_dma_address(sg)); > + (*ents)[si].length = cpu_to_le32(sg_dma_len(sg)); > + (*ents)[si].padding = 0; > + } > + } else { > + for_each_sgtable_sg(userptr->sgt, sg, si) { > + (*ents)[si].addr = cpu_to_le64(sg_phys(sg)); > + (*ents)[si].length = cpu_to_le32(sg->length); > + (*ents)[si].padding = 0; > + } > + } > + > + *nents = count; > + return 0; > +} > + > +int virtio_gpu_userptr_restore(struct virtio_gpu_device *vgdev, > + struct virtio_gpu_object *bo, > + struct virtio_gpu_mem_entry **ents, > + unsigned int *nents) > +{ > + struct virtio_gpu_object_userptr *userptr = to_virtio_gpu_userptr(bo); > + int ret; > + > + mutex_lock(&userptr->lock); > + if (!userptr->sgt || !userptr->pages) { > + mutex_unlock(&userptr->lock); > + return -EINVAL; > + } > + > + if (userptr->dma_mapped) { > + struct device *dev = drm_dev_dma_dev(vgdev->ddev); > + > + dma_unmap_sgtable(dev, userptr->sgt, userptr->dma_dir, > + DMA_ATTR_REQUIRE_COHERENT); > + userptr->dma_mapped = false; > + > + ret = dma_map_sgtable(dev, userptr->sgt, userptr->dma_dir, > + DMA_ATTR_REQUIRE_COHERENT); > + if (ret) { > + mutex_unlock(&userptr->lock); > + return ret; > + } > + userptr->dma_mapped = true; > + } > + > + ret = virtio_gpu_userptr_get_entries(vgdev, userptr, ents, nents); > + mutex_unlock(&userptr->lock); > + return ret; > +} [Severity: Medium] If dma_map_sgtable() fails in virtio_gpu_userptr_restore(), will a later restore attempt send unmapped DMA addresses to the host? When dma_map_sgtable() fails after dma_unmap_sgtable(), userptr->dma_mapped is left false while userptr->sgt->nents remains non-zero and the object stays on vgdev->obj_restore_list. A subsequent virtio_gpu_userptr_restore() call then skips remapping because userptr->dma_mapped is false, but virtio_gpu_userptr_get_entries() checks virtio_gpu_use_dma_api(vgdev->vdev) and reads the unmapped sg_dma_address(sg) entries anyway. Checking virtio_gpu_use_dma_api(vgdev->vdev) to decide whether dma_map_sgtable() is needed, and only checking userptr->dma_mapped before calling dma_unmap_sgtable(), would avoid this. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
