Re: [[RESEND] iommufd PATCH v2 1/2] i915/gvt: Move gvt mapping cache initialization to intel_vgpu_init_dev()

[Zhenyu Wang <zhenyuw@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3162 bytes --]

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@linux.intel.com>
> > > Cc: Zhi Wang <zhi.a.wang@intel.com>
> > > Cc: Kevin Tian <kevin.tian@intel.com>
> > > Cc: intel-gvt-dev@lists.freedesktop.org
> > > Reviewed-by: Zhi Wang <zhi.a.wang@intel.com>
> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > ---
> > >   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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]