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

[Yi Liu <yi.l.liu@intel.com>]

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@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.

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