Re: [PATCH 1/8] drm/ttm: turn ttm_bo_device.vma_manager into a pointer
Am 05.09.19 um 09:05 schrieb Gerd Hoffmann: Rename the embedded struct vma_offset_manager, new name is _vma_manager. ttm_bo_device.vma_manager changed to a pointer. The ttm_bo_device_init() function gets an additional vma_manager argument which allows to initialize ttm with a different vma manager. When passing NULL the embedded _vma_manager is used. All callers are updated to pass NULL, so the behavior doesn't change. Signed-off-by: Gerd Hoffmann Reviewed-by: Christian König --- include/drm/ttm/ttm_bo_driver.h | 8 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 + drivers/gpu/drm/drm_vram_mm_helper.c| 1 + drivers/gpu/drm/nouveau/nouveau_ttm.c | 1 + drivers/gpu/drm/qxl/qxl_ttm.c | 1 + drivers/gpu/drm/radeon/radeon_ttm.c | 1 + drivers/gpu/drm/ttm/ttm_bo.c| 13 + drivers/gpu/drm/ttm/ttm_bo_vm.c | 6 +++--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 + 9 files changed, 24 insertions(+), 9 deletions(-) diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index e88e00c6cbf2..e365434f92b3 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -441,7 +441,8 @@ extern struct ttm_bo_global { * * @driver: Pointer to a struct ttm_bo_driver struct setup by the driver. * @man: An array of mem_type_managers. - * @vma_manager: Address space manager + * @vma_manager: Address space manager (pointer) + * @_vma_manager: Address space manager (enbedded) * lru_lock: Spinlock that protects the buffer+device lru lists and * ddestroy lists. * @dev_mapping: A pointer to the struct address_space representing the @@ -464,7 +465,8 @@ struct ttm_bo_device { /* * Protected by internal locks. */ - struct drm_vma_offset_manager vma_manager; + struct drm_vma_offset_manager *vma_manager; + struct drm_vma_offset_manager _vma_manager; /* * Protected by the global:lru lock. @@ -585,6 +587,7 @@ int ttm_bo_device_release(struct ttm_bo_device *bdev); * @glob: A pointer to an initialized struct ttm_bo_global. * @driver: A pointer to a struct ttm_bo_driver set up by the caller. * @mapping: The address space to use for this bo. + * @vma_manager: A pointer to a vma manager or NULL. * @file_page_offset: Offset into the device address space that is available * for buffer data. This ensures compatibility with other users of the * address space. @@ -596,6 +599,7 @@ int ttm_bo_device_release(struct ttm_bo_device *bdev); int ttm_bo_device_init(struct ttm_bo_device *bdev, struct ttm_bo_driver *driver, struct address_space *mapping, + struct drm_vma_offset_manager *vma_manager, bool need_dma32); /** diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index fb09314bcfd4..34ee5d725faf 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1728,6 +1728,7 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) r = ttm_bo_device_init(&adev->mman.bdev, &amdgpu_bo_driver, adev->ddev->anon_inode->i_mapping, + NULL, adev->need_dma32); if (r) { DRM_ERROR("failed initializing buffer object driver(%d).\n", r); diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c index c911781d6728..56fd1519eb35 100644 --- a/drivers/gpu/drm/drm_vram_mm_helper.c +++ b/drivers/gpu/drm/drm_vram_mm_helper.c @@ -172,6 +172,7 @@ int drm_vram_mm_init(struct drm_vram_mm *vmm, struct drm_device *dev, ret = ttm_bo_device_init(&vmm->bdev, &bo_driver, dev->anon_inode->i_mapping, +NULL, true); if (ret) return ret; diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c index f0daf958e03a..e67eb10843d1 100644 --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c @@ -236,6 +236,7 @@ nouveau_ttm_init(struct nouveau_drm *drm) ret = ttm_bo_device_init(&drm->ttm.bdev, &nouveau_bo_driver, dev->anon_inode->i_mapping, +NULL, drm->client.mmu.dmabits <= 32 ? true : false); if (ret) { NV_ERROR(drm, "error initialising bo driver, %d\n", ret); diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c index 9b24514c75aa..69da0eea6e4c 100644 --- a/drivers/gpu/drm/qxl/qxl_ttm.c +++ b/drivers/gpu/drm/qxl/qxl_ttm.c @@ -325,6 +325,7 @@ int qxl_ttm_init(struct qxl_device *qdev) r = ttm_bo_device_init(&qdev->mman.bdev,
Re: Xorg indefinitely hangs in kernelspace
Hi, On Mon, 9 Sep 2019 from Gerd Hoffmann > > Hmm, I think the patch is wrong. As far I know it is the qxl drivers's > job to call ttm_eu_backoff_reservation(). Doing that automatically in > ttm will most likely break other ttm users. > Perhaps. >So I guess the call is missing in the qxl driver somewhere, most likely >in some error handling code path given that this bug is a relatively >rare event. > >There is only a single ttm_eu_reserve_buffers() call in qxl. >So how about this? > No preference in either way if it is a right cure. BTW a quick peep at the mainline tree shows not every ttm_eu_reserve_buffers() pairs with ttm_eu_backoff_reservation() without qxl being taken in account. Hillf ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] Revert "vhost: access vq metadata through kernel virtual address"
On 2019/9/6 下午9:46, Michael S. Tsirkin wrote: On Thu, Sep 05, 2019 at 08:27:35PM +0800, Jason Wang wrote: It was reported that metadata acceleration introduces several issues, so this patch reverts commit ff466032dc9e5a61217f22ea34b2df932786bbfc, 73f628ec9e6bcc45b77c53fe6d0c0ec55eaf82af and 0b4a7092ffe568a55bf8f3cefdf79ff666586d91. We will rework it on the next version. Cc: Jason Gunthorpe Signed-off-by: Jason Wang I am confused by the above. What I see upstream is 7f466032dc. commit 7f466032dc9e5a61217f22ea34b2df932786bbfc Author: Jason Wang Date: Fri May 24 04:12:18 2019 -0400 vhost: access vq metadata through kernel virtual address so this is what I reverted. Pls take a look, and let me know if you see issues. Thanks! Yes, my fault. Thanks --- drivers/vhost/vhost.c | 515 +- drivers/vhost/vhost.h | 41 2 files changed, 3 insertions(+), 553 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 0536f8526359..791562e03fe0 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -298,160 +298,6 @@ static void vhost_vq_meta_reset(struct vhost_dev *d) __vhost_vq_meta_reset(d->vqs[i]); } -#if VHOST_ARCH_CAN_ACCEL_UACCESS -static void vhost_map_unprefetch(struct vhost_map *map) -{ - kfree(map->pages); - map->pages = NULL; - map->npages = 0; - map->addr = NULL; -} - -static void vhost_uninit_vq_maps(struct vhost_virtqueue *vq) -{ - struct vhost_map *map[VHOST_NUM_ADDRS]; - int i; - - spin_lock(&vq->mmu_lock); - for (i = 0; i < VHOST_NUM_ADDRS; i++) { - map[i] = rcu_dereference_protected(vq->maps[i], - lockdep_is_held(&vq->mmu_lock)); - if (map[i]) - rcu_assign_pointer(vq->maps[i], NULL); - } - spin_unlock(&vq->mmu_lock); - - synchronize_rcu(); - - for (i = 0; i < VHOST_NUM_ADDRS; i++) - if (map[i]) - vhost_map_unprefetch(map[i]); - -} - -static void vhost_reset_vq_maps(struct vhost_virtqueue *vq) -{ - int i; - - vhost_uninit_vq_maps(vq); - for (i = 0; i < VHOST_NUM_ADDRS; i++) - vq->uaddrs[i].size = 0; -} - -static bool vhost_map_range_overlap(struct vhost_uaddr *uaddr, -unsigned long start, -unsigned long end) -{ - if (unlikely(!uaddr->size)) - return false; - - return !(end < uaddr->uaddr || start > uaddr->uaddr - 1 + uaddr->size); -} - -static void vhost_invalidate_vq_start(struct vhost_virtqueue *vq, - int index, - unsigned long start, - unsigned long end) -{ - struct vhost_uaddr *uaddr = &vq->uaddrs[index]; - struct vhost_map *map; - int i; - - if (!vhost_map_range_overlap(uaddr, start, end)) - return; - - spin_lock(&vq->mmu_lock); - ++vq->invalidate_count; - - map = rcu_dereference_protected(vq->maps[index], - lockdep_is_held(&vq->mmu_lock)); - if (map) { - if (uaddr->write) { - for (i = 0; i < map->npages; i++) - set_page_dirty(map->pages[i]); - } - rcu_assign_pointer(vq->maps[index], NULL); - } - spin_unlock(&vq->mmu_lock); - - if (map) { - synchronize_rcu(); - vhost_map_unprefetch(map); - } -} - -static void vhost_invalidate_vq_end(struct vhost_virtqueue *vq, - int index, - unsigned long start, - unsigned long end) -{ - if (!vhost_map_range_overlap(&vq->uaddrs[index], start, end)) - return; - - spin_lock(&vq->mmu_lock); - --vq->invalidate_count; - spin_unlock(&vq->mmu_lock); -} - -static int vhost_invalidate_range_start(struct mmu_notifier *mn, - const struct mmu_notifier_range *range) -{ - struct vhost_dev *dev = container_of(mn, struct vhost_dev, -mmu_notifier); - int i, j; - - if (!mmu_notifier_range_blockable(range)) - return -EAGAIN; - - for (i = 0; i < dev->nvqs; i++) { - struct vhost_virtqueue *vq = dev->vqs[i]; - - for (j = 0; j < VHOST_NUM_ADDRS; j++) - vhost_invalidate_vq_start(vq, j, - range->start, - range->end); - } - - return 0; -} - -static void vhost_invalidate_range_end(struct mmu_notifier *mn, - const struct mmu_notifier_range *range) -{ -
Re: [PATCH 0/2] Revert and rework on the metadata accelreation
On 2019/9/6 下午9:15, David Miller wrote: From: Jason Wang Date: Fri, 6 Sep 2019 18:02:35 +0800 On 2019/9/5 下午9:59, Jason Gunthorpe wrote: I think you should apply the revert this cycle and rebase the other patch for next.. Jason Yes, the plan is to revert in this release cycle. Then you should reset patch #1 all by itself targetting 'net'. Thanks for the reminding. I want the patch to go through Michael's vhost tree, that's why I don't put 'net' prefix. For next time, maybe I can use "vhost" as a prefix for classification? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH untested] vhost: block speculation of translated descriptors
On 2019/9/8 下午7:05, Michael S. Tsirkin wrote: iovec addresses coming from vhost are assumed to be pre-validated, but in fact can be speculated to a value out of range. Userspace address are later validated with array_index_nospec so we can be sure kernel info does not leak through these addresses, but vhost must also not leak userspace info outside the allowed memory table to guests. Following the defence in depth principle, make sure the address is not validated out of node range. Signed-off-by: Michael S. Tsirkin --- drivers/vhost/vhost.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 5dc174ac8cac..0ee375fb7145 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2072,7 +2072,9 @@ static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len, size = node->size - addr + node->start; _iov->iov_len = min((u64)len - s, size); _iov->iov_base = (void __user *)(unsigned long) - (node->userspace_addr + addr - node->start); + (node->userspace_addr + +array_index_nospec(addr - node->start, + node->size)); s += size; addr += size; ++ret; I've tried this on Kaby Lake smap off metadata acceleration off using testpmd (virtio-user) + vhost_net. I don't see obvious performance difference with TX PPS. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] vhost: re-introducing metadata acceleration through kernel virtual address
On 2019/9/9 下午12:45, Michael S. Tsirkin wrote: Since idx can be speculated, I guess we need array_index_nospec here? So we have ACQUIRE(mmu_lock) get idx RELEASE(mmu_lock) ACQUIRE(mmu_lock) read array[idx] RELEASE(mmu_lock) Then I think idx can't be speculated consider we've passed RELEASE + ACQUIRE? I don't think memory barriers have anything to do with speculation, they are architectural. Oh right. Let me add array_index_nospec() in next version. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Xorg indefinitely hangs in kernelspace
On Mon, 9 Sep 2019 from Gerd Hoffmann > > Hmm, I think the patch is wrong. Hmm...it should have added change only in the error path, leaving locks for drivers to release if job is done with no error returned. > As far I know it is the qxl drivers's > job to call ttm_eu_backoff_reservation(). Like other drivers, qxl is currently doing the right. > Doing that automatically in > ttm will most likely break other ttm users. > You are right. They are responsible for doing backoff if error happens while validating buffers afterwards. --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -111,8 +111,10 @@ int ttm_eu_reserve_buffers(struct ww_acq list_for_each_entry(entry, list, head) { struct ttm_buffer_object *bo = entry->bo; + bool lockon; ret = __ttm_bo_reserve(bo, intr, (ticket == NULL), ticket); + lockon = !ret; if (!ret && unlikely(atomic_read(&bo->cpu_writers) > 0)) { reservation_object_unlock(bo->resv); @@ -151,6 +153,7 @@ int ttm_eu_reserve_buffers(struct ww_acq ret = 0; } } + lockon = !ret; if (!ret && entry->num_shared) ret = reservation_object_reserve_shared(bo->resv, @@ -163,6 +166,8 @@ int ttm_eu_reserve_buffers(struct ww_acq ww_acquire_done(ticket); ww_acquire_fini(ticket); } + if (lockon) + ttm_eu_backoff_reservation_reverse(list, entry); return ret; } -- ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/2] Revert and rework on the metadata accelreation
On Mon, Sep 09, 2019 at 03:18:01PM +0800, Jason Wang wrote: > > On 2019/9/6 下午9:15, David Miller wrote: > > From: Jason Wang > > Date: Fri, 6 Sep 2019 18:02:35 +0800 > > > > > On 2019/9/5 下午9:59, Jason Gunthorpe wrote: > > > > I think you should apply the revert this cycle and rebase the other > > > > patch for next.. > > > > > > > > Jason > > > Yes, the plan is to revert in this release cycle. > > Then you should reset patch #1 all by itself targetting 'net'. > > > Thanks for the reminding. I want the patch to go through Michael's vhost > tree, that's why I don't put 'net' prefix. For next time, maybe I can use > "vhost" as a prefix for classification? That's fine by me. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 7/7] drm/vram: fix Kconfig
Am 04.09.19 um 07:47 schrieb Gerd Hoffmann: > select isn't recursive, so we can't turn on DRM_TTM + DRM_TTM_HELPER > in config DRM_VRAM_HELPER, we have to select them on the vram users > instead. > > Signed-off-by: Gerd Hoffmann > --- > drivers/gpu/drm/Kconfig | 2 -- > drivers/gpu/drm/ast/Kconfig | 2 ++ > drivers/gpu/drm/bochs/Kconfig | 2 ++ > drivers/gpu/drm/hisilicon/hibmc/Kconfig | 3 ++- > drivers/gpu/drm/mgag200/Kconfig | 2 ++ > drivers/gpu/drm/vboxvideo/Kconfig | 2 ++ > 6 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 1be8ad30d8fe..cd11a3bde19c 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -168,8 +168,6 @@ config DRM_TTM > config DRM_VRAM_HELPER > tristate > depends on DRM > - select DRM_TTM > - select DRM_TTM_HELPER > help > Helpers for VRAM memory management > > diff --git a/drivers/gpu/drm/ast/Kconfig b/drivers/gpu/drm/ast/Kconfig > index 829620d5326c..fbcf2f45cef5 100644 > --- a/drivers/gpu/drm/ast/Kconfig > +++ b/drivers/gpu/drm/ast/Kconfig > @@ -4,6 +4,8 @@ config DRM_AST > depends on DRM && PCI && MMU > select DRM_KMS_HELPER > select DRM_VRAM_HELPER > + select DRM_TTM > + select DRM_TTM_HELPER > help >Say yes for experimental AST GPU driver. Do not enable >this driver without having a working -modesetting, > diff --git a/drivers/gpu/drm/bochs/Kconfig b/drivers/gpu/drm/bochs/Kconfig > index 32b043abb668..7bcdf294fed8 100644 > --- a/drivers/gpu/drm/bochs/Kconfig > +++ b/drivers/gpu/drm/bochs/Kconfig > @@ -4,6 +4,8 @@ config DRM_BOCHS > depends on DRM && PCI && MMU > select DRM_KMS_HELPER > select DRM_VRAM_HELPER > + select DRM_TTM > + select DRM_TTM_HELPER > help > Choose this option for qemu. > If M is selected the module will be called bochs-drm. > diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig > b/drivers/gpu/drm/hisilicon/hibmc/Kconfig > index f20eedf0073a..8ad9a5b12e40 100644 > --- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig > +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig > @@ -4,7 +4,8 @@ config DRM_HISI_HIBMC > depends on DRM && PCI && MMU > select DRM_KMS_HELPER > select DRM_VRAM_HELPER > - > + select DRM_TTM > + select DRM_TTM_HELPER > help > Choose this option if you have a Hisilicon Hibmc soc chipset. > If M is selected the module will be called hibmc-drm. > diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig > index 76fee0fbdcae..aed11f4f4c55 100644 > --- a/drivers/gpu/drm/mgag200/Kconfig > +++ b/drivers/gpu/drm/mgag200/Kconfig > @@ -4,6 +4,8 @@ config DRM_MGAG200 > depends on DRM && PCI && MMU > select DRM_KMS_HELPER > select DRM_VRAM_HELPER > + select DRM_TTM > + select DRM_TTM_HELPER > help >This is a KMS driver for the MGA G200 server chips, it > does not support the original MGA G200 or any of the desktop > diff --git a/drivers/gpu/drm/vboxvideo/Kconfig > b/drivers/gpu/drm/vboxvideo/Kconfig > index 56ba510f21a2..45fe135d6e43 100644 > --- a/drivers/gpu/drm/vboxvideo/Kconfig > +++ b/drivers/gpu/drm/vboxvideo/Kconfig > @@ -4,6 +4,8 @@ config DRM_VBOXVIDEO > depends on DRM && X86 && PCI > select DRM_KMS_HELPER > select DRM_VRAM_HELPER > + select DRM_TTM > + select DRM_TTM_HELPER > select GENERIC_ALLOCATOR > help > This is a KMS driver for the virtual Graphics Card used in > Thanks for fixing the Kconfigs for VRAM helpers. Acked-by: Thomas Zimmermann -- Thomas Zimmermann Graphics Driver Developer SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg) signature.asc Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 0/4] Merge VRAM MM and GEM VRAM source files
VRAM MM and GEM VRAM are only used with each other. This patch set moves VRAM MM into GEM VRAM source files and cleans up the helper's public interface. Thomas Zimmermann (4): drm/vram: Move VRAM memory manager to GEM VRAM implementation drm/vram: Have VRAM MM call GEM VRAM functions directly drm/vram: Unexport internal functions of VRAM MM drm/vram: Unconditonally set BO call-back functions Documentation/gpu/drm-mm.rst | 12 - drivers/gpu/drm/Makefile | 3 +- drivers/gpu/drm/ast/ast_drv.c | 1 - drivers/gpu/drm/ast/ast_main.c| 1 - drivers/gpu/drm/ast/ast_ttm.c | 3 +- drivers/gpu/drm/bochs/bochs.h | 1 - drivers/gpu/drm/bochs/bochs_mm.c | 3 +- drivers/gpu/drm/drm_gem_vram_helper.c | 361 ++ drivers/gpu/drm/drm_vram_helper_common.c | 8 +- drivers/gpu/drm/drm_vram_mm_helper.c | 309 --- .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 1 - drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 3 +- drivers/gpu/drm/mgag200/mgag200_drv.h | 1 - drivers/gpu/drm/mgag200/mgag200_ttm.c | 3 +- drivers/gpu/drm/vboxvideo/vbox_drv.h | 2 - drivers/gpu/drm/vboxvideo/vbox_ttm.c | 3 +- include/drm/drm_gem_vram_helper.h | 90 - include/drm/drm_vram_mm_helper.h | 108 -- 18 files changed, 375 insertions(+), 538 deletions(-) delete mode 100644 drivers/gpu/drm/drm_vram_mm_helper.c delete mode 100644 include/drm/drm_vram_mm_helper.h -- 2.23.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 3/4] drm/vram: Unexport internal functions of VRAM MM
The init, cleanup and mmap functions of VRAM MM are only used internally. Remove them from the public interface. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/drm_gem_vram_helper.c | 38 --- include/drm/drm_gem_vram_helper.h | 7 - 2 files changed, 5 insertions(+), 40 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index ed1ffbec5d02..73e81e3a8724 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -844,19 +844,8 @@ static struct ttm_bo_driver bo_driver = { * struct drm_vram_mm */ -/** - * drm_vram_mm_init() - Initialize an instance of VRAM MM. - * @vmm: the VRAM MM instance to initialize - * @dev: the DRM device - * @vram_base: the base address of the video memory - * @vram_size: the size of the video memory in bytes - * - * Returns: - * 0 on success, or - * a negative error code otherwise. - */ -int drm_vram_mm_init(struct drm_vram_mm *vmm, struct drm_device *dev, -uint64_t vram_base, size_t vram_size) +static int drm_vram_mm_init(struct drm_vram_mm *vmm, struct drm_device *dev, + uint64_t vram_base, size_t vram_size) { int ret; @@ -875,34 +864,17 @@ int drm_vram_mm_init(struct drm_vram_mm *vmm, struct drm_device *dev, return 0; } -EXPORT_SYMBOL(drm_vram_mm_init); -/** - * drm_vram_mm_cleanup() - Cleans up an initialized instance of VRAM MM. - * @vmm: the VRAM MM instance to clean up - */ -void drm_vram_mm_cleanup(struct drm_vram_mm *vmm) +static void drm_vram_mm_cleanup(struct drm_vram_mm *vmm) { ttm_bo_device_release(&vmm->bdev); } -EXPORT_SYMBOL(drm_vram_mm_cleanup); -/** - * drm_vram_mm_mmap() - Helper for implementing &struct file_operations.mmap() - * @filp: the mapping's file structure - * @vma: the mapping's memory area - * @vmm: the VRAM MM instance - * - * Returns: - * 0 on success, or - * a negative error code otherwise. - */ -int drm_vram_mm_mmap(struct file *filp, struct vm_area_struct *vma, -struct drm_vram_mm *vmm) +static int drm_vram_mm_mmap(struct file *filp, struct vm_area_struct *vma, + struct drm_vram_mm *vmm) { return ttm_bo_mmap(filp, vma, &vmm->bdev); } -EXPORT_SYMBOL(drm_vram_mm_mmap); /* * Helpers for integration with struct drm_device diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h index 52e3f2aff490..3c204f17a0b8 100644 --- a/include/drm/drm_gem_vram_helper.h +++ b/include/drm/drm_gem_vram_helper.h @@ -169,13 +169,6 @@ static inline struct drm_vram_mm *drm_vram_mm_of_bdev( return container_of(bdev, struct drm_vram_mm, bdev); } -int drm_vram_mm_init(struct drm_vram_mm *vmm, struct drm_device *dev, -uint64_t vram_base, size_t vram_size); -void drm_vram_mm_cleanup(struct drm_vram_mm *vmm); - -int drm_vram_mm_mmap(struct file *filp, struct vm_area_struct *vma, -struct drm_vram_mm *vmm); - /* * Helpers for integration with struct drm_device */ -- 2.23.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 4/4] drm/vram: Unconditonally set BO call-back functions
The statement's condition is always true. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/drm_gem_vram_helper.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 73e81e3a8724..13717ae65da5 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -91,8 +91,7 @@ static int drm_gem_vram_init(struct drm_device *dev, int ret; size_t acc_size; - if (!gbo->bo.base.funcs) - gbo->bo.base.funcs = &drm_gem_vram_object_funcs; + gbo->bo.base.funcs = &drm_gem_vram_object_funcs; ret = drm_gem_object_init(dev, &gbo->bo.base, size); if (ret) -- 2.23.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/4] drm/vram: Move VRAM memory manager to GEM VRAM implementation
The separation between GEM VRAM objects and and the memory manager is artificial, as they are only used with each other. Copying both implementations into the same file is a first step to simplifying the code. This patch only moves code without functional changes. Signed-off-by: Thomas Zimmermann --- Documentation/gpu/drm-mm.rst | 12 - drivers/gpu/drm/Makefile | 3 +- drivers/gpu/drm/ast/ast_drv.c | 1 - drivers/gpu/drm/ast/ast_main.c| 1 - drivers/gpu/drm/ast/ast_ttm.c | 1 - drivers/gpu/drm/bochs/bochs.h | 1 - drivers/gpu/drm/drm_gem_vram_helper.c | 303 + drivers/gpu/drm/drm_vram_mm_helper.c | 309 -- .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 1 - drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 1 - drivers/gpu/drm/mgag200/mgag200_drv.h | 1 - drivers/gpu/drm/vboxvideo/vbox_drv.h | 2 - include/drm/drm_gem_vram_helper.h | 85 + include/drm/drm_vram_mm_helper.h | 76 - 14 files changed, 389 insertions(+), 408 deletions(-) delete mode 100644 drivers/gpu/drm/drm_vram_mm_helper.c diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst index b664f054c259..9e61fef3cf5b 100644 --- a/Documentation/gpu/drm-mm.rst +++ b/Documentation/gpu/drm-mm.rst @@ -400,18 +400,6 @@ GEM VRAM Helper Functions Reference .. kernel-doc:: drivers/gpu/drm/drm_gem_vram_helper.c :export: -VRAM MM Helper Functions Reference --- - -.. kernel-doc:: drivers/gpu/drm/drm_vram_mm_helper.c - :doc: overview - -.. kernel-doc:: include/drm/drm_vram_mm_helper.h - :internal: - -.. kernel-doc:: drivers/gpu/drm/drm_vram_mm_helper.c - :export: - VMA Offset Manager == diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 82ff826b33cc..bb8cf02a604b 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -33,8 +33,7 @@ drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o drm_vram_helper-y := drm_gem_vram_helper.o \ -drm_vram_helper_common.o \ -drm_vram_mm_helper.o +drm_vram_helper_common.o obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o drm_probe_helper.o \ diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c index 6ed6ff49efc0..e0e8770462bc 100644 --- a/drivers/gpu/drm/ast/ast_drv.c +++ b/drivers/gpu/drm/ast/ast_drv.c @@ -35,7 +35,6 @@ #include #include #include -#include #include "ast_drv.h" diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 50de8e47659c..21715d6a9b56 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -33,7 +33,6 @@ #include #include #include -#include #include "ast_drv.h" diff --git a/drivers/gpu/drm/ast/ast_ttm.c b/drivers/gpu/drm/ast/ast_ttm.c index c52d92294171..08ba0a917593 100644 --- a/drivers/gpu/drm/ast/ast_ttm.c +++ b/drivers/gpu/drm/ast/ast_ttm.c @@ -30,7 +30,6 @@ #include #include -#include #include "ast_drv.h" diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h index 68483a2fc12c..917767173ee6 100644 --- a/drivers/gpu/drm/bochs/bochs.h +++ b/drivers/gpu/drm/bochs/bochs.h @@ -10,7 +10,6 @@ #include #include #include -#include /* -- */ diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 0990f0e03213..202f74453580 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -14,6 +15,11 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; * * This library provides a GEM buffer object that is backed by video RAM * (VRAM). It can be used for framebuffer devices with dedicated memory. + * + * The data structure &struct drm_vram_mm and its helpers implement a memory + * manager for simple framebuffer devices with dedicated video memory. Buffer + * objects are either placed in video RAM or evicted to system memory. The rsp. + * buffer object is provided by &struct drm_gem_vram_object. */ /* @@ -734,3 +740,300 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs = { .vmap = drm_gem_vram_object_vmap, .vunmap = drm_gem_vram_object_vunmap }; + +/* + * VRAM memory manager + */ + +/* + * TTM TT + */ + +static void backend_func_destroy(struct ttm_tt *tt) +{ + ttm_tt_fini(tt); + kfree(tt); +} + +static struct ttm_backend_func backend_func = { + .destroy = backend_func_destroy +}; + +/* + * TTM BO device + */ + +static struct ttm_tt
[PATCH 2/4] drm/vram: Have VRAM MM call GEM VRAM functions directly
VRAM MM and GEM VRAM buffer objects are only used with each other; connected via 3 function pointers. Simplifiy this code by making the memory manager call the rsp. functions from the BOs directly; and remove the functions from he BO's public interface. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_ttm.c | 2 +- drivers/gpu/drm/bochs/bochs_mm.c| 3 +- drivers/gpu/drm/drm_gem_vram_helper.c | 119 ++-- drivers/gpu/drm/drm_vram_helper_common.c| 8 +- drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 2 +- drivers/gpu/drm/mgag200/mgag200_ttm.c | 3 +- drivers/gpu/drm/vboxvideo/vbox_ttm.c| 3 +- include/drm/drm_gem_vram_helper.h | 24 +--- include/drm/drm_vram_mm_helper.h| 32 -- 9 files changed, 44 insertions(+), 152 deletions(-) delete mode 100644 include/drm/drm_vram_mm_helper.h diff --git a/drivers/gpu/drm/ast/ast_ttm.c b/drivers/gpu/drm/ast/ast_ttm.c index 08ba0a917593..fad34106083a 100644 --- a/drivers/gpu/drm/ast/ast_ttm.c +++ b/drivers/gpu/drm/ast/ast_ttm.c @@ -41,7 +41,7 @@ int ast_mm_init(struct ast_private *ast) vmm = drm_vram_helper_alloc_mm( dev, pci_resource_start(dev->pdev, 0), - ast->vram_size, &drm_gem_vram_mm_funcs); + ast->vram_size); if (IS_ERR(vmm)) { ret = PTR_ERR(vmm); DRM_ERROR("Error initializing VRAM MM; %d\n", ret); diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c index 8f9bb886f7ad..1b74f530b07c 100644 --- a/drivers/gpu/drm/bochs/bochs_mm.c +++ b/drivers/gpu/drm/bochs/bochs_mm.c @@ -11,8 +11,7 @@ int bochs_mm_init(struct bochs_device *bochs) struct drm_vram_mm *vmm; vmm = drm_vram_helper_alloc_mm(bochs->dev, bochs->fb_base, - bochs->fb_size, - &drm_gem_vram_mm_funcs); + bochs->fb_size); return PTR_ERR_OR_ZERO(vmm); } diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 202f74453580..ed1ffbec5d02 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -5,7 +5,6 @@ #include #include #include -#include #include static const struct drm_gem_object_funcs drm_gem_vram_object_funcs; @@ -462,68 +461,25 @@ static bool drm_is_gem_vram(struct ttm_buffer_object *bo) return (bo->destroy == ttm_buffer_object_destroy); } -/** - * drm_gem_vram_bo_driver_evict_flags() - \ - Implements &struct ttm_bo_driver.evict_flags - * @bo:TTM buffer object. Refers to &struct drm_gem_vram_object.bo - * @pl:TTM placement information. - */ -void drm_gem_vram_bo_driver_evict_flags(struct ttm_buffer_object *bo, - struct ttm_placement *pl) +static void drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo, + struct ttm_placement *pl) { - struct drm_gem_vram_object *gbo; - - /* TTM may pass BOs that are not GEM VRAM BOs. */ - if (!drm_is_gem_vram(bo)) - return; - - gbo = drm_gem_vram_of_bo(bo); drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM); *pl = gbo->placement; } -EXPORT_SYMBOL(drm_gem_vram_bo_driver_evict_flags); -/** - * drm_gem_vram_bo_driver_verify_access() - \ - Implements &struct ttm_bo_driver.verify_access - * @bo:TTM buffer object. Refers to &struct drm_gem_vram_object.bo - * @filp: File pointer. - * - * Returns: - * 0 on success, or - * a negative errno code otherwise. - */ -int drm_gem_vram_bo_driver_verify_access(struct ttm_buffer_object *bo, -struct file *filp) +static int drm_gem_vram_bo_driver_verify_access(struct drm_gem_vram_object *gbo, + struct file *filp) { - struct drm_gem_vram_object *gbo = drm_gem_vram_of_bo(bo); - return drm_vma_node_verify_access(&gbo->bo.base.vma_node, filp->private_data); } -EXPORT_SYMBOL(drm_gem_vram_bo_driver_verify_access); -/** - * drm_gem_vram_bo_driver_move_notify() - - * Implements &struct ttm_bo_driver.move_notify - * @bo:TTM buffer object. Refers to &struct drm_gem_vram_object.bo - * @evict: True, if the BO is being evicted from graphics memory; - * false otherwise. - * @new_mem: New memory region, or NULL on destruction - */ -void drm_gem_vram_bo_driver_move_notify(struct ttm_buffer_object *bo, - bool evict, - struct ttm_mem_reg *new_mem) +static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo, + bool evict, +
Re: [RFC PATCH untested] vhost: block speculation of translated descriptors
On Mon, Sep 09, 2019 at 03:19:55PM +0800, Jason Wang wrote: > > On 2019/9/8 下午7:05, Michael S. Tsirkin wrote: > > iovec addresses coming from vhost are assumed to be > > pre-validated, but in fact can be speculated to a value > > out of range. > > > > Userspace address are later validated with array_index_nospec so we can > > be sure kernel info does not leak through these addresses, but vhost > > must also not leak userspace info outside the allowed memory table to > > guests. > > > > Following the defence in depth principle, make sure > > the address is not validated out of node range. > > > > Signed-off-by: Michael S. Tsirkin > > --- > > drivers/vhost/vhost.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > index 5dc174ac8cac..0ee375fb7145 100644 > > --- a/drivers/vhost/vhost.c > > +++ b/drivers/vhost/vhost.c > > @@ -2072,7 +2072,9 @@ static int translate_desc(struct vhost_virtqueue *vq, > > u64 addr, u32 len, > > size = node->size - addr + node->start; > > _iov->iov_len = min((u64)len - s, size); > > _iov->iov_base = (void __user *)(unsigned long) > > - (node->userspace_addr + addr - node->start); > > + (node->userspace_addr + > > +array_index_nospec(addr - node->start, > > + node->size)); > > s += size; > > addr += size; > > ++ret; > > > I've tried this on Kaby Lake smap off metadata acceleration off using > testpmd (virtio-user) + vhost_net. I don't see obvious performance > difference with TX PPS. > > Thanks Should I push this to Linus right now then? It's a security thing so maybe we better do it ASAP ... what's your opinion? -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Virtio-fs] [PATCH 15/18] virtiofs: Make virtio_fs object refcounted
On Sun, Sep 08, 2019 at 07:10:03PM +0800, piaojun wrote: > > > On 2019/9/6 3:48, Vivek Goyal wrote: > > This object is used both by fuse_connection as well virt device. So make > > this object reference counted and that makes it easy to define life cycle > > of the object. > > > > Now deivce can be removed while filesystem is still mounted. This will > > cleanup all the virtqueues but virtio_fs object will still be around and > > will be cleaned when filesystem is unmounted and sb/fc drops its reference. > > > > Removing a device also stops all virt queues and any new reuqest gets > > error -ENOTCONN. All existing in flight requests are drained before > > ->remove returns. > > > > Signed-off-by: Vivek Goyal > > --- > > fs/fuse/virtio_fs.c | 52 + > > 1 file changed, 43 insertions(+), 9 deletions(-) > > > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > > index 01bbf2c0e144..29ec2f5bbbe2 100644 > > --- a/fs/fuse/virtio_fs.c > > +++ b/fs/fuse/virtio_fs.c > > @@ -37,6 +37,7 @@ struct virtio_fs_vq { > > > > /* A virtio-fs device instance */ > > struct virtio_fs { > > + struct kref refcount; > > struct list_head list;/* on virtio_fs_instances */ > > char *tag; > > struct virtio_fs_vq *vqs; > > @@ -63,6 +64,27 @@ static inline struct fuse_pqueue *vq_to_fpq(struct > > virtqueue *vq) > > return &vq_to_fsvq(vq)->fud->pq; > > } > > > > +static void release_virtiofs_obj(struct kref *ref) > > +{ > > + struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount); > > + > > + kfree(vfs->vqs); > > + kfree(vfs); > > +} > > + > > +static void virtio_fs_put(struct virtio_fs *fs) > > +{ > > + mutex_lock(&virtio_fs_mutex); > > + kref_put(&fs->refcount, release_virtiofs_obj); > > + mutex_unlock(&virtio_fs_mutex); > > +} > > + > > +static void virtio_fs_put(struct fuse_iqueue *fiq) > > +{ > > + struct virtio_fs *vfs = fiq->priv; > > + virtiofs_put(vfs); > > +} > > It's a little confusing that virtiofs_put() looks like virtiofs_put(), > and could we use __virtio_fs_put to replace virtio_fs_put? Fixed this in follow up patch I posted. https://www.redhat.com/archives/virtio-fs/2019-September/msg00091.html Vivek ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 08/18] virtiofs: Drain all pending requests during ->remove time
On Fri, Sep 06, 2019 at 10:18:49AM -0400, Michael S. Tsirkin wrote: > On Fri, Sep 06, 2019 at 10:17:05AM -0400, Vivek Goyal wrote: > > On Fri, Sep 06, 2019 at 11:52:10AM +0100, Stefan Hajnoczi wrote: > > > On Thu, Sep 05, 2019 at 03:48:49PM -0400, Vivek Goyal wrote: > > > > +static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq) > > > > +{ > > > > + WARN_ON(fsvq->in_flight < 0); > > > > + > > > > + /* Wait for in flight requests to finish.*/ > > > > + while (1) { > > > > + spin_lock(&fsvq->lock); > > > > + if (!fsvq->in_flight) { > > > > + spin_unlock(&fsvq->lock); > > > > + break; > > > > + } > > > > + spin_unlock(&fsvq->lock); > > > > + usleep_range(1000, 2000); > > > > + } > > > > > > I think all contexts that call this allow sleeping so we could avoid > > > usleep here. > > > > usleep_range() is supposed to be used from non-atomic context. > > > > https://github.com/torvalds/linux/blob/master/Documentation/timers/timers-howto.rst > > > > What construct you are thinking of? > > > > Vivek > > completion + signal on vq callback? Yes. Time-based sleep() is sub-optimal because we could wake up exactly when in_flight is decremented from the vq callback. This avoids unnecessary sleep wakeups and the extra time spent sleeping after in_flight has been decremented. Stefan signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 15/18] virtiofs: Make virtio_fs object refcounted
On Fri, Sep 06, 2019 at 09:50:32AM -0400, Vivek Goyal wrote: > On Fri, Sep 06, 2019 at 01:03:09PM +0100, Stefan Hajnoczi wrote: > > On Thu, Sep 05, 2019 at 03:48:56PM -0400, Vivek Goyal wrote: > > > This object is used both by fuse_connection as well virt device. So make > > > this object reference counted and that makes it easy to define life cycle > > > of the object. > > > > > > Now deivce can be removed while filesystem is still mounted. This will > > > cleanup all the virtqueues but virtio_fs object will still be around and > > > will be cleaned when filesystem is unmounted and sb/fc drops its > > > reference. > > > > > > Removing a device also stops all virt queues and any new reuqest gets > > > error -ENOTCONN. All existing in flight requests are drained before > > > ->remove returns. > > > > > > Signed-off-by: Vivek Goyal > > > --- > > > fs/fuse/virtio_fs.c | 52 + > > > 1 file changed, 43 insertions(+), 9 deletions(-) > > > > > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > > > index 01bbf2c0e144..29ec2f5bbbe2 100644 > > > --- a/fs/fuse/virtio_fs.c > > > +++ b/fs/fuse/virtio_fs.c > > > @@ -37,6 +37,7 @@ struct virtio_fs_vq { > > > > > > /* A virtio-fs device instance */ > > > struct virtio_fs { > > > + struct kref refcount; > > > struct list_head list;/* on virtio_fs_instances */ > > > char *tag; > > > struct virtio_fs_vq *vqs; > > > @@ -63,6 +64,27 @@ static inline struct fuse_pqueue *vq_to_fpq(struct > > > virtqueue *vq) > > > return &vq_to_fsvq(vq)->fud->pq; > > > } > > > > > > +static void release_virtiofs_obj(struct kref *ref) > > > +{ > > > + struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount); > > > + > > > + kfree(vfs->vqs); > > > + kfree(vfs); > > > +} > > > + > > > +static void virtiofs_put(struct virtio_fs *fs) > > > > Why do the two function names above contain "virtiofs" instead > > of "virtio_fs"? I'm not sure if this is intentional and is supposed to > > mean something, but it's confusing. > > > > > +{ > > > + mutex_lock(&virtio_fs_mutex); > > > + kref_put(&fs->refcount, release_virtiofs_obj); > > > + mutex_unlock(&virtio_fs_mutex); > > > +} > > > + > > > +static void virtio_fs_put(struct fuse_iqueue *fiq) > > > > Minor issue: this function name is confusingly similar to > > virtiofs_put(). Please rename to virtio_fs_fiq_put(). > > Fixed with ->release semantics. Replaced "virtiofs" with "virtio_fs". > > > Subject: virtiofs: Make virtio_fs object refcounted > > This object is used both by fuse_connection as well virt device. So make > this object reference counted and that makes it easy to define life cycle > of the object. > > Now deivce can be removed while filesystem is still mounted. This will > cleanup all the virtqueues but virtio_fs object will still be around and > will be cleaned when filesystem is unmounted and sb/fc drops its reference. > > Removing a device also stops all virt queues and any new reuqest gets > error -ENOTCONN. All existing in flight requests are drained before > ->remove returns. > > Signed-off-by: Vivek Goyal > --- > fs/fuse/virtio_fs.c | 52 > +++- > 1 file changed, 43 insertions(+), 9 deletions(-) > > Index: rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c > === > --- rhvgoyal-linux-fuse.orig/fs/fuse/virtio_fs.c 2019-09-06 > 09:24:21.177245246 -0400 > +++ rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c 2019-09-06 09:40:53.309245246 > -0400 > @@ -37,6 +37,7 @@ struct virtio_fs_vq { > > /* A virtio-fs device instance */ > struct virtio_fs { > + struct kref refcount; > struct list_head list;/* on virtio_fs_instances */ > char *tag; > struct virtio_fs_vq *vqs; > @@ -63,6 +64,27 @@ static inline struct fuse_pqueue *vq_to_ > return &vq_to_fsvq(vq)->fud->pq; > } > > +static void release_virtio_fs_obj(struct kref *ref) > +{ > + struct virtio_fs *vfs = container_of(ref, struct virtio_fs, refcount); > + > + kfree(vfs->vqs); > + kfree(vfs); > +} > + > +static void virtio_fs_put(struct virtio_fs *fs) > +{ > + mutex_lock(&virtio_fs_mutex); > + kref_put(&fs->refcount, release_virtio_fs_obj); > + mutex_unlock(&virtio_fs_mutex); > +} > + > +static void virtio_fs_fiq_release(struct fuse_iqueue *fiq) > +{ > + struct virtio_fs *vfs = fiq->priv; > + virtio_fs_put(vfs); > +} > + > static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq) > { > WARN_ON(fsvq->in_flight < 0); > @@ -156,8 +178,10 @@ static struct virtio_fs *virtio_fs_find_ > mutex_lock(&virtio_fs_mutex); > > list_for_each_entry(fs, &virtio_fs_instances, list) { > - if (strcmp(fs->tag, tag) == 0) > + if (strcmp(fs->tag, tag) == 0) { > + kref_get(&fs->refcount); > goto found; > + } > } > > fs = NULL; /* not
Re: [PATCH 16/18] virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path
On Fri, Sep 06, 2019 at 09:51:31AM -0400, Vivek Goyal wrote: > On Fri, Sep 06, 2019 at 01:05:34PM +0100, Stefan Hajnoczi wrote: > > On Thu, Sep 05, 2019 at 03:48:57PM -0400, Vivek Goyal wrote: > > > It is possible that a mount is in progress and device is being removed at > > > the same time. Use virtio_fs_mutex to avoid races. > > > > > > This also takes care of bunch of races and removes some TODO items. > > > > > > Signed-off-by: Vivek Goyal > > > --- > > > fs/fuse/virtio_fs.c | 32 ++-- > > > 1 file changed, 22 insertions(+), 10 deletions(-) > > > > Let's move to a per-device mutex in the future. That way a single > > device that fails to drain/complete requests will not hang all other > > virtio-fs instances. This is fine for now. > > Good point. For now I updated the patch so that it applies cleanly > after previous two patches changed. > > Subject: virtiofs: Use virtio_fs_mutex for races w.r.t ->remove and mount path > > It is possible that a mount is in progress and device is being removed at > the same time. Use virtio_fs_mutex to avoid races. > > This also takes care of bunch of races and removes some TODO items. > > Signed-off-by: Vivek Goyal > --- > fs/fuse/virtio_fs.c | 32 ++-- > 1 file changed, 22 insertions(+), 10 deletions(-) > > Index: rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c > === > --- rhvgoyal-linux-fuse.orig/fs/fuse/virtio_fs.c 2019-09-06 > 09:40:53.309245246 -0400 > +++ rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c 2019-09-06 09:43:25.335245246 > -0400 > @@ -13,7 +13,9 @@ > #include > #include "fuse_i.h" > > -/* List of virtio-fs device instances and a lock for the list */ > +/* List of virtio-fs device instances and a lock for the list. Also provides > + * mutual exclusion in device removal and mounting path > + */ > static DEFINE_MUTEX(virtio_fs_mutex); > static LIST_HEAD(virtio_fs_instances); > > @@ -72,17 +74,19 @@ static void release_virtio_fs_obj(struct > kfree(vfs); > } > > +/* Make sure virtiofs_mutex is held */ > static void virtio_fs_put(struct virtio_fs *fs) > { > - mutex_lock(&virtio_fs_mutex); > kref_put(&fs->refcount, release_virtio_fs_obj); > - mutex_unlock(&virtio_fs_mutex); > } > > static void virtio_fs_fiq_release(struct fuse_iqueue *fiq) > { > struct virtio_fs *vfs = fiq->priv; > + > + mutex_lock(&virtio_fs_mutex); > virtio_fs_put(vfs); > + mutex_unlock(&virtio_fs_mutex); > } > > static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq) > @@ -596,9 +600,8 @@ static void virtio_fs_remove(struct virt > struct virtio_fs *fs = vdev->priv; > > mutex_lock(&virtio_fs_mutex); > + /* This device is going away. No one should get new reference */ > list_del_init(&fs->list); > - mutex_unlock(&virtio_fs_mutex); > - > virtio_fs_stop_all_queues(fs); > virtio_fs_drain_all_queues(fs); > vdev->config->reset(vdev); > @@ -607,6 +610,7 @@ static void virtio_fs_remove(struct virt > vdev->priv = NULL; > /* Put device reference on virtio_fs object */ > virtio_fs_put(fs); > + mutex_unlock(&virtio_fs_mutex); > } > > #ifdef CONFIG_PM_SLEEP > @@ -978,10 +982,15 @@ static int virtio_fs_fill_super(struct s > .no_force_umount = true, > }; > > - /* TODO lock */ > - if (fs->vqs[VQ_REQUEST].fud) { > - pr_err("virtio-fs: device already in use\n"); > - err = -EBUSY; > + mutex_lock(&virtio_fs_mutex); > + > + /* After holding mutex, make sure virtiofs device is still there. > + * Though we are holding a refernce to it, drive ->remove might > + * still have cleaned up virtual queues. In that case bail out. > + */ > + err = -EINVAL; > + if (list_empty(&fs->list)) { > + pr_info("virtio-fs: tag <%s> not found\n", fs->tag); > goto err; > } > > @@ -1007,7 +1016,6 @@ static int virtio_fs_fill_super(struct s > > fc = fs->vqs[VQ_REQUEST].fud->fc; > > - /* TODO take fuse_mutex around this loop? */ > for (i = 0; i < fs->nvqs; i++) { > struct virtio_fs_vq *fsvq = &fs->vqs[i]; > > @@ -1020,6 +1028,7 @@ static int virtio_fs_fill_super(struct s > /* Previous unmount will stop all queues. Start these again */ > virtio_fs_start_all_queues(fs); > fuse_send_init(fc, init_req); > + mutex_unlock(&virtio_fs_mutex); > return 0; > > err_free_init_req: > @@ -1027,6 +1036,7 @@ err_free_init_req: > err_free_fuse_devs: > virtio_fs_free_devs(fs); > err: > + mutex_unlock(&virtio_fs_mutex); > return err; > } > > @@ -1100,7 +1110,9 @@ static int virtio_fs_get_tree(struct fs_ > > fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL); > if (!fc) { > + mutex_lock(&virtio_fs_mutex); > virtio_fs_put(fs); > + mutex_
Re: [Virtio-fs] [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
On Sun, Sep 08, 2019 at 07:53:55PM +0800, piaojun wrote: > > > On 2019/9/6 19:52, Miklos Szeredi wrote: > > On Fri, Sep 6, 2019 at 12:36 PM Stefan Hajnoczi wrote: > >> > >> On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote: > >>> On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal wrote: > > Hi, > > Michael Tsirkin pointed out issues w.r.t various locking related TODO > items and races w.r.t device removal. > > In this first round of cleanups, I have taken care of most pressing > issues. > > These patches apply on top of following. > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4 > > I have tested these patches with mount/umount and device removal using > qemu monitor. For example. > >>> > >>> Is device removal mandatory? Can't this be made a non-removable > >>> device? Is there a good reason why removing the virtio-fs device > >>> makes sense? > >> > >> Hot plugging and unplugging virtio PCI adapters is common. I'd very > >> much like removal to work from the beginning. > > > > Can you give an example use case? > > I think VirtFS migration need hot plugging, or it may cause QEMU crash > or some problems. Live migration is currently unsupported. Hot unplugging the virtio-fs device would allow the guest to live migrate successfully, so it's a useful feature to work around the missing live migration support. Is this what you mean? Stefan signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Virtio-fs] [PATCH 00/18] virtiofs: Fix various races and cleanups round 1
On Mon, Sep 09, 2019 at 06:14:55PM +0200, Stefan Hajnoczi wrote: > On Sun, Sep 08, 2019 at 07:53:55PM +0800, piaojun wrote: > > > > > > On 2019/9/6 19:52, Miklos Szeredi wrote: > > > On Fri, Sep 6, 2019 at 12:36 PM Stefan Hajnoczi > > > wrote: > > >> > > >> On Fri, Sep 06, 2019 at 10:15:14AM +0200, Miklos Szeredi wrote: > > >>> On Thu, Sep 5, 2019 at 9:49 PM Vivek Goyal wrote: > > > > Hi, > > > > Michael Tsirkin pointed out issues w.r.t various locking related TODO > > items and races w.r.t device removal. > > > > In this first round of cleanups, I have taken care of most pressing > > issues. > > > > These patches apply on top of following. > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#virtiofs-v4 > > > > I have tested these patches with mount/umount and device removal using > > qemu monitor. For example. > > >>> > > >>> Is device removal mandatory? Can't this be made a non-removable > > >>> device? Is there a good reason why removing the virtio-fs device > > >>> makes sense? > > >> > > >> Hot plugging and unplugging virtio PCI adapters is common. I'd very > > >> much like removal to work from the beginning. > > > > > > Can you give an example use case? > > > > I think VirtFS migration need hot plugging, or it may cause QEMU crash > > or some problems. > > Live migration is currently unsupported. Hot unplugging the virtio-fs > device would allow the guest to live migrate successfully, so it's a > useful feature to work around the missing live migration support. > > Is this what you mean? > > Stefan Exactly. That's what I also said. To add to that, hotplug can not be negotiated, so it's not a feature we can easily add down the road if old guests crash on unplug. Thus a driver that does not support unplug should not claim to support removal. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] scsi: virtio_scsi: unplug LUNs when events missed
On 06/09/19 10:54, Stefan Hajnoczi wrote: > On Thu, Sep 05, 2019 at 06:19:28PM +, Matt Lupfer wrote: >> The event handler calls scsi_scan_host() when events are missed, which >> will hotplug new LUNs. However, this function won't remove any >> unplugged LUNs. The result is that hotunplug doesn't work properly when >> the number of unplugged LUNs exceeds the event queue size (currently 8). >> >> Scan existing LUNs when events are missed to check if they are still >> present. If not, remove them. >> >> Signed-off-by: Matt Lupfer >> --- >> drivers/scsi/virtio_scsi.c | 33 + >> 1 file changed, 33 insertions(+) > > Please include a changelog in future patch revisions. For example: > > Signed-off-by: ... > --- > v2: > * Replaced magic constants with sd.h constants [Michael] > > Just C and virtio code review, no SCSI specifics: > > Reviewed-by: Stefan Hajnoczi > Acked-by: Paolo Bonzini signature.asc Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: DANGER WILL ROBINSON, DANGER
On 05/09/19 20:09, Jerome Glisse wrote: > Not sure i understand, you are saying that the solution i outline above > does not work ? If so then i think you are wrong, in the above solution > the importing process mmap a device file and the resulting vma is then > populated using insert_pfn() and constantly keep synchronize with the > target process through mirroring which means that you never have to look > at the struct page ... you can mirror any kind of memory from the remote > process. If insert_pfn in turn calls MMU notifiers for the target VMA (which would be the KVM MMU notifier), then that would work. Though I guess it would be possible to call MMU notifier update callbacks around the call to insert_pfn. Paolo ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] drm/virtio: Use vmalloc for command buffer allocations.
On Thu, Sep 5, 2019 at 10:18 PM Gerd Hoffmann wrote: > > > +/* How many bytes left in this page. */ > > +static unsigned int rest_of_page(void *data) > > +{ > > + return PAGE_SIZE - offset_in_page(data); > > +} > > Not needed. > > > +/* Create sg_table from a vmalloc'd buffer. */ > > +static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size, int > > *sg_ents) > > +{ > > + int nents, ret, s, i; > > + struct sg_table *sgt; > > + struct scatterlist *sg; > > + struct page *pg; > > + > > + *sg_ents = 0; > > + > > + sgt = kmalloc(sizeof(*sgt), GFP_KERNEL); > > + if (!sgt) > > + return NULL; > > + > > + nents = DIV_ROUND_UP(size, PAGE_SIZE) + 1; > > Why +1? This is part of handling offsets within the vmalloc buffer and to maintain parity with the !is_vmalloc_addr/existing case (sg_init_one handles offsets within pages internally). I had left it in because this is being used for all sg/descriptor generation and I wasn't sure if someone in the future might do something like: buf = vmemdup_user() offset = find_interesting(buf) queue(buf + offset) To respond specifically to your question, if we handle offsets, a vmalloc_to_sgt(size = PAGE_SIZE + 2) could end up with 3 sg_ents with the +1 being to account for that extra page. I'll just remove all support for offsets in v3 of the patch and comment that functionality will be different based on where the buffer was originally allocated from. > > > + ret = sg_alloc_table(sgt, nents, GFP_KERNEL); > > + if (ret) { > > + kfree(sgt); > > + return NULL; > > + } > > + > > + for_each_sg(sgt->sgl, sg, nents, i) { > > + pg = vmalloc_to_page(data); > > + if (!pg) { > > + sg_free_table(sgt); > > + kfree(sgt); > > + return NULL; > > + } > > + > > + s = rest_of_page(data); > > + if (s > size) > > + s = size; > > vmalloc memory is page aligned, so: As per above, will remove with v3. > > s = min(PAGE_SIZE, size); > > > + sg_set_page(sg, pg, s, offset_in_page(data)); > > Offset is always zero. As per above, will remove with v3. > > > + > > + size -= s; > > + data += s; > > + *sg_ents += 1; > > sg_ents isn't used anywhere. It's used for outcnt. > > > + > > + if (size) { > > + sg_unmark_end(sg); > > + } else { > > + sg_mark_end(sg); > > + break; > > + } > > That looks a bit strange. I guess you need only one of the two because > the other is the default? I was being overly paranoid and not wanting to make assumptions about the initial state of the table. I'll simplify. > > > static int virtio_gpu_queue_fenced_ctrl_buffer(struct virtio_gpu_device > > *vgdev, > > struct virtio_gpu_vbuffer > > *vbuf, > > struct virtio_gpu_ctrl_hdr > > *hdr, > > struct virtio_gpu_fence *fence) > > { > > struct virtqueue *vq = vgdev->ctrlq.vq; > > + struct scatterlist *vout = NULL, sg; > > + struct sg_table *sgt = NULL; > > int rc; > > + int outcnt = 0; > > + > > + if (vbuf->data_size) { > > + if (is_vmalloc_addr(vbuf->data_buf)) { > > + sgt = vmalloc_to_sgt(vbuf->data_buf, vbuf->data_size, > > + &outcnt); > > + if (!sgt) > > + return -ENOMEM; > > + vout = sgt->sgl; > > + } else { > > + sg_init_one(&sg, vbuf->data_buf, vbuf->data_size); > > + vout = &sg; > > + outcnt = 1; > > outcnt must be set in both cases. outcnt is set by vmalloc_to_sgt. > > > +static int virtio_gpu_queue_ctrl_buffer(struct virtio_gpu_device *vgdev, > > + struct virtio_gpu_vbuffer *vbuf) > > +{ > > + return virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, NULL, NULL); > > +} > > Changing virtio_gpu_queue_ctrl_buffer to call > virtio_gpu_queue_fenced_ctrl_buffer should be done in a separate patch. Will do. Thanks, David ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH untested] vhost: block speculation of translated descriptors
On 2019/9/9 下午10:45, Michael S. Tsirkin wrote: On Mon, Sep 09, 2019 at 03:19:55PM +0800, Jason Wang wrote: On 2019/9/8 下午7:05, Michael S. Tsirkin wrote: iovec addresses coming from vhost are assumed to be pre-validated, but in fact can be speculated to a value out of range. Userspace address are later validated with array_index_nospec so we can be sure kernel info does not leak through these addresses, but vhost must also not leak userspace info outside the allowed memory table to guests. Following the defence in depth principle, make sure the address is not validated out of node range. Signed-off-by: Michael S. Tsirkin --- drivers/vhost/vhost.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 5dc174ac8cac..0ee375fb7145 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2072,7 +2072,9 @@ static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len, size = node->size - addr + node->start; _iov->iov_len = min((u64)len - s, size); _iov->iov_base = (void __user *)(unsigned long) - (node->userspace_addr + addr - node->start); + (node->userspace_addr + +array_index_nospec(addr - node->start, + node->size)); s += size; addr += size; ++ret; I've tried this on Kaby Lake smap off metadata acceleration off using testpmd (virtio-user) + vhost_net. I don't see obvious performance difference with TX PPS. Thanks Should I push this to Linus right now then? It's a security thing so maybe we better do it ASAP ... what's your opinion? Yes, you can. Acked-by: Jason Wang ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH untested] vhost: block speculation of translated descriptors
On Tue, Sep 10, 2019 at 09:52:10AM +0800, Jason Wang wrote: > > On 2019/9/9 下午10:45, Michael S. Tsirkin wrote: > > On Mon, Sep 09, 2019 at 03:19:55PM +0800, Jason Wang wrote: > > > On 2019/9/8 下午7:05, Michael S. Tsirkin wrote: > > > > iovec addresses coming from vhost are assumed to be > > > > pre-validated, but in fact can be speculated to a value > > > > out of range. > > > > > > > > Userspace address are later validated with array_index_nospec so we can > > > > be sure kernel info does not leak through these addresses, but vhost > > > > must also not leak userspace info outside the allowed memory table to > > > > guests. > > > > > > > > Following the defence in depth principle, make sure > > > > the address is not validated out of node range. > > > > > > > > Signed-off-by: Michael S. Tsirkin > > > > --- > > > >drivers/vhost/vhost.c | 4 +++- > > > >1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > index 5dc174ac8cac..0ee375fb7145 100644 > > > > --- a/drivers/vhost/vhost.c > > > > +++ b/drivers/vhost/vhost.c > > > > @@ -2072,7 +2072,9 @@ static int translate_desc(struct vhost_virtqueue > > > > *vq, u64 addr, u32 len, > > > > size = node->size - addr + node->start; > > > > _iov->iov_len = min((u64)len - s, size); > > > > _iov->iov_base = (void __user *)(unsigned long) > > > > - (node->userspace_addr + addr - node->start); > > > > + (node->userspace_addr + > > > > +array_index_nospec(addr - node->start, > > > > + node->size)); > > > > s += size; > > > > addr += size; > > > > ++ret; > > > > > > I've tried this on Kaby Lake smap off metadata acceleration off using > > > testpmd (virtio-user) + vhost_net. I don't see obvious performance > > > difference with TX PPS. > > > > > > Thanks > > Should I push this to Linus right now then? It's a security thing so > > maybe we better do it ASAP ... what's your opinion? > > > Yes, you can. > > Acked-by: Jason Wang And should I include Tested-by: Jason Wang ? > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization