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. > > > } > > > static void intel_vgpu_release_dev(struct vfio_device *vfio_dev) > > > -- > > > 2.34.1 > > > > > -- > Regards, > Yi Liu
Attachment:
signature.asc
Description: PGP signature
