[PATCH v3] drm/i915/gvt: clear the vGPU reset logic

[Gao, Ping A]

On 2017/2/21 14:30, Du, Changbin wrote:
> On Tue, Feb 21, 2017 at 01:37:39PM +0800, Ping Gao wrote:
>> Releasing shadow PPGTT pages is not enough when vGPU reset, the
>> guest page table tracking data should has same life-cycle with
>> all the shadow PPGTT pages; Otherwise there is no chance to
>> re-shadow the PPGTT pages without free the guest page table
>> tracking data.
>>
>> This patch clear the PPGTT reset logic and make the vGPU reset in
>> working order.
>>
>> v2: refactor some logic to avoid code duplicated.
>> v3: remove useless macro and add comments from Christophe.
>>
>> Signed-off-by: Ping Gao <ping.a.gao at intel.com>
>> ---
>>  drivers/gpu/drm/i915/gvt/gtt.c | 39 ++++++++++++++++++++++++++++-----------
>>  1 file changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
>> index 28c9234..acd02f7 100644
>> --- a/drivers/gpu/drm/i915/gvt/gtt.c
>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>> @@ -2015,6 +2015,22 @@ int intel_vgpu_init_gtt(struct intel_vgpu *vgpu)
>>  	return create_scratch_page_tree(vgpu);
>>  }
>>  
>> +static void intel_vgpu_free_mm(struct intel_vgpu *vgpu, int type)
>> +{
>> +	struct list_head *pos, *n;
>> +	struct intel_vgpu_mm *mm;
>> +
>> +	list_for_each_safe(pos, n, &vgpu->gtt.mm_list_head) {
>> +		mm = container_of(pos, struct intel_vgpu_mm, list);
> A list_for_each_entry_safe can be used here. :)

Thanks for comments,  if you has special reason to replace all the
list_for_each_safe in this file, better to cook a separate patch to
address that. Let's keep focus.

>> +		if (mm->type == type) {
>> +			vgpu->gvt->gtt.mm_free_page_table(mm);
>> +			list_del(&mm->list);
>> +			list_del(&mm->lru_list);
>> +			kfree(mm);
>> +		}
>> +	}
>> +}
>> +
>>  /**
>>   * intel_vgpu_clean_gtt - clean up per-vGPU graphics memory virulization
>>   * @vgpu: a vGPU
>> @@ -2027,18 +2043,18 @@ int intel_vgpu_init_gtt(struct intel_vgpu *vgpu)
>>   */
>>  void intel_vgpu_clean_gtt(struct intel_vgpu *vgpu)
>>  {
>> -	struct list_head *pos, *n;
>> -	struct intel_vgpu_mm *mm;
>> -
>>  	ppgtt_free_all_shadow_page(vgpu);
>> -	release_scratch_page_tree(vgpu);
>>  
>> -	list_for_each_safe(pos, n, &vgpu->gtt.mm_list_head) {
>> -		mm = container_of(pos, struct intel_vgpu_mm, list);
>> -		vgpu->gvt->gtt.mm_free_page_table(mm);
>> -		list_del(&mm->list);
>> -		list_del(&mm->lru_list);
>> -		kfree(mm);
>> +	/* Shadow pages are only created when there is no page
>> +	 * table tracking data, so remove page tracking data after
>> +	 * removing the shadow pages.This comment is for vGPU reset
>> +	 * path.
>> +	 */
>> +	intel_vgpu_free_mm(vgpu, INTEL_GVT_MM_PPGTT);
>> +
>> +	if (vgpu->resetting == false) {
>> +		intel_vgpu_free_mm(vgpu, INTEL_GVT_MM_GGTT);
>> +		release_scratch_page_tree(vgpu);
>>  	}
> This func may read a little confusing. It try to mix reset work with
> clean work, while the name is 'clean'. How about leave
> intel_vgpu_clean_gtt just do clean work, and add below 2 statements
> into intel_vgpu_reset_gtt()?
>   ppgtt_free_all_shadow_page(vgpu);
>   intel_vgpu_free_mm(vgpu, INTEL_GVT_MM_PPGTT);

I think it's no need to distinguish clear/reset so bound, otherwise
clear and reset function would be has some redundant logic in them.
Reset is also kind of clear.

>
> This is more clear for me.
>
>
>>  }
>>  
>> @@ -2321,7 +2337,8 @@ void intel_vgpu_reset_gtt(struct intel_vgpu *vgpu, bool dmlr)
>>  {
>>  	int i;
>>  
>> -	ppgtt_free_all_shadow_page(vgpu);
>> +	intel_vgpu_clean_gtt(vgpu);
>> +
>>  	if (!dmlr)
>>  		return;
>>  
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> intel-gvt-dev mailing list
>> intel-gvt-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev