[PATCH v2] drm/i915/gvt: cancel dma map only for not present ggtt entry
Zhang, Xiaolin
xiaolin.zhang at intel.com
Thu May 16 02:52:29 UTC 2019
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
More information about the intel-gvt-dev
mailing list