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

[Zhang, Xiaolin]

On 05/16/2019 10:30 AM, Zhang, Xiong Y wrote:
>
>> -----Original Message-----
>> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On
>> Behalf Of Zhang, Xiaolin
>> Sent: Thursday, May 16, 2019 8:21 AM
>> To: Zhenyu Wang <zhenyuw at linux.intel.com>
>> Cc: intel-gvt-dev at lists.freedesktop.org
>> Subject: Re: [PATCH v2] drm/i915/gvt: cancel dma map only for not present
>> ggtt entry
>>
>> 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.
> [Zhang, Xiong Y] if guest just modify ggtt, not clear then write, where will the old ggtt entry be unmapped ?
> With this fix, it seems the old ggtt entry will never be unmapped.
>
> Still don't clear why graphic access it after ggtt has dropped it. How about this change ?
this doesn't work either after a test.
> --- a/drivers/gpu/drm/i915/gvt/gtt.c
> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> @@ -2275,10 +2275,11 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>         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);
> +
> +       ggtt_invalidate_pte(vgpu, &e);
>         return 0;
>  }
>
> thanks
>>>> 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);
>>>
>>>
>> _______________________________________________
>> intel-gvt-dev mailing list
>> intel-gvt-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev