[PATCH v2] drm/i915/gvt: cancel dma map only for not present ggtt entry

[Zhang, Xiaolin]

On 05/15/2019 05:02 PM, Zhenyu Wang wrote:
> On 2019.05.15 16:19:21 +0800, Xiaolin Zhang wrote:
>> only cancel ggtt entry dma map for invalid ggtt entry, don't do it
>> for paritial update or old ggtt entry.
>>
>> this change can address DMA "fault reason 23" issue for win guest with
>> intel iommu on.
>>
>> v2: update handling of ggtt partial update (Zhenyu, Yan, Henry)
>>
>> Signed-off-by: Xiaolin Zhang <xiaolin.zhang at intel.com>
>> ---
>>  drivers/gpu/drm/i915/gvt/gtt.c | 21 ++++++++++++---------
>>  1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
>> index c2f7d20f6346..4856b9fec411 100644
>> --- a/drivers/gpu/drm/i915/gvt/gtt.c
>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>> @@ -2243,10 +2243,21 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>>  		}
>>  	}
>>  
>> -	if (!partial_update && (ops->test_present(&e))) {
>> +	if (!ops->test_present(&e)) {
>> +		ggtt_get_host_entry(ggtt_mm, &m, g_gtt_index);
>> +		ggtt_invalidate_pte(vgpu, &m);
>> +		ops->set_pfn(&m, gvt->gtt.scratch_mfn);
>> +		ops->clear_present(&m);
>> +	} else {
>>  		gfn = ops->get_pfn(&e);
>>  		m = e;
>>  
>> +		if (partial_update) {
>> +			ops->set_pfn(&m, gvt->gtt.scratch_mfn);
>> +			ops->clear_present(&m);
>> +			goto out;
>> +		}
>> +
>>  		/* one PTE update may be issued in multiple writes and the
>>  		 * first write may not construct a valid gfn
>>  		 */
>> @@ -2266,17 +2277,9 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>>  			ops->set_pfn(&m, gvt->gtt.scratch_mfn);
>>  		} else
>>  			ops->set_pfn(&m, dma_addr >> PAGE_SHIFT);
>> -	} else {
>> -		ops->set_pfn(&m, gvt->gtt.scratch_mfn);
>> -		ops->clear_present(&m);
>>  	}
>> -
>>  out:
>>  	ggtt_set_guest_entry(ggtt_mm, &e, g_gtt_index);
>> -
>> -	ggtt_get_host_entry(ggtt_mm, &e, g_gtt_index);
>> -	ggtt_invalidate_pte(vgpu, &e);
>> -
>>  	ggtt_set_host_entry(ggtt_mm, &m, g_gtt_index);
>>  	ggtt_invalidate(gvt->dev_priv);
>>  	return 0;
>> -- 
> How about below simpler change?
>
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 08c74e65836b..8f27db9d7a1e 100644
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -2269,16 +2269,16 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>  			ops->set_pfn(&m, gvt->gtt.scratch_mfn);
>  		} else
>  			ops->set_pfn(&m, dma_addr >> PAGE_SHIFT);
> -	} else {
> +	} else
>  		ops->set_pfn(&m, gvt->gtt.scratch_mfn);
> -		ops->clear_present(&m);
> -	}
>  
>  out:
>  	ggtt_set_guest_entry(ggtt_mm, &e, g_gtt_index);
>  
> -	ggtt_get_host_entry(ggtt_mm, &e, g_gtt_index);
> -	ggtt_invalidate_pte(vgpu, &e);
> +	if (!partial_update) {
> +		ggtt_get_host_entry(ggtt_mm, &e, g_gtt_index);
> +		ggtt_invalidate_pte(vgpu, &e);
> +	}
this change does not work after a test and this change was  pointed out
by Henry before except your clear present. Another test is that if we
change "if (!partial_update)" to "if (!ops->test_present(&e))" after 
label out, it will work. but this change is essential to the same my
patch set and is not good readable than my patch set per my opinion.
>  
>  	ggtt_set_host_entry(ggtt_mm, &m, g_gtt_index);
>  	ggtt_invalidate(gvt->dev_priv);
>
>