[PATCH v2] drm/i915/gvt: cancel dma map only for not present ggtt entry
Zhenyu Wang
zhenyuw at linux.intel.com
Mon May 20 04:55:46 UTC 2019
On 2019.05.16 02:52:29 +0000, Zhang, Xiaolin wrote:
> 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.
I have the same concern as Xiong. In your change, we will only do unmap
when guest explicitly clear entry which set !present bit, but we can't
rely on guest driver behavior for this.
Current code just trys to do unmap whenever guest changed target entry.
So could you elaborate what's the real reason for this issue?
> > --- 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
>
>
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20190520/d9fd9fbb/attachment.sig>
More information about the intel-gvt-dev
mailing list