On 2022/12/1 15:21, Zhenyu Wang wrote:
On 2022.12.01 12:18:29 +0800, Yi Liu wrote:
On 2022/12/1 11:25, Zhenyu Wang wrote:
On 2022.11.29 02:58:30 -0800, Yi Liu wrote:
vfio container registers .dma_unmap() callback after the device is opened.
So it's fine for mdev drivers to initialize internal mapping cache in
.open_device(). See vfio_device_container_register().
Now with iommufd an access ops with an unmap callback is registered
when the device is bound to iommufd which is before .open_device()
is called. This implies gvt's .dma_unmap() could be called before its
internal mapping cache is initialized.
The fix is moving gvt mapping cache initialization to vGPU init. While
at it also move ptable initialization together.
Cc: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>
Cc: Zhi Wang <zhi.a.wang@xxxxxxxxx>
Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
Cc: intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx
Reviewed-by: Zhi Wang <zhi.a.wang@xxxxxxxxx>
Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
---
drivers/gpu/drm/i915/gvt/kvmgt.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 7a45e5360caf..f563e5dbe66f 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -671,9 +671,6 @@ static int intel_vgpu_open_device(struct vfio_device *vfio_dev)
vgpu->attached = true;
- kvmgt_protect_table_init(vgpu);
- gvt_cache_init(vgpu);
-
vgpu->track_node.track_write = kvmgt_page_track_write;
vgpu->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
kvm_page_track_register_notifier(vgpu->vfio_device.kvm,
@@ -1451,9 +1448,17 @@ static int intel_vgpu_init_dev(struct vfio_device *vfio_dev)
struct intel_vgpu *vgpu = vfio_dev_to_vgpu(vfio_dev);
struct intel_vgpu_type *type =
container_of(mdev->type, struct intel_vgpu_type, type);
+ int ret;
vgpu->gvt = kdev_to_i915(mdev->type->parent->dev)->gvt;
- return intel_gvt_create_vgpu(vgpu, type->conf);
+ ret = intel_gvt_create_vgpu(vgpu, type->conf);
+ if (ret)
+ return ret;
+
+ kvmgt_protect_table_init(vgpu);
+ gvt_cache_init(vgpu);
+
+ return 0;
I'm fine with this change, but could we add some sanity check at close
time to ensure we clean up any internal cache? Btw, do we need to reset
rbtree root pointer?
I noticed there is gvt_cache_destroy() in intel_vgpu_close_device(). This
cleans up the internal cache. So even the rbtree root is valid, it is an
empty per close_device(). isn't it?
I'd like to see an explicit sanity check on vgpu->nr_cache_entries and
reset rb root at close time, which matches current code behavior, but
not need to do re-init.
do you mean check vgpu->nr_cache_entries before calling
gvt_cache_destroy()? I think it should be possible non-zero, so even
non-zero is detected, nothing should be done. But if non-zero nr_cache_entries
is detected after gvt_cache_destroy(), this should be a problem as
gvt_cache_destroy() should make nr_cache_entries be zero. Is there any
chance that it is still non-zero after gvt_cache_destroy()?
static void gvt_cache_destroy(struct intel_vgpu *vgpu)
{
struct gvt_dma *dma;
struct rb_node *node = NULL;
for (;;) {
mutex_lock(&vgpu->cache_lock);
node = rb_first(&vgpu->gfn_cache);
if (!node) {
mutex_unlock(&vgpu->cache_lock);
break;
}
dma = rb_entry(node, struct gvt_dma, gfn_node);
gvt_dma_unmap_page(vgpu, dma->gfn, dma->dma_addr, dma->size);
__gvt_cache_remove_entry(vgpu, dma); //decrements
nr_cache_entries
mutex_unlock(&vgpu->cache_lock);
}
}
--
Regards,
Yi Liu
