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

[Zhang, Xiong Y]

> -----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 ?
--- 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