[PATCH v2] drm/i915/gvt: refine vgpu gtt sync policy to reduce invalidate times

Fri Nov 3 02:41:13 UTC 2017

On Fri, Nov 03, 2017 at 10:33:33AM +0800, Zhenyu Wang wrote:
> On 2017.11.03 10:27:08 +0800, Du, Changbin wrote:
> > I am thinking there is a security hole for delayed flush (as well as let
> > guest triger flush). Since we already reshadowed the updated PTE, and the old
> > pages are unpinned, so linux is free to move the pages of qemu. It gives the
> > guest a chance to overwrite the pages out of the guest before the TLB is flushed.
> > Then guest can attack the host and other VMs by writing aperture space.
> >
> 
> As in Weinan's patch, aperture change is always invalidated.
>

So this patch is for PPGTT? If I remember correctly, Won't PPGTT be
flushed automatically by HW when switch context?

> > On Fri, Nov 03, 2017 at 02:03:59AM +0000, Tian, Kevin wrote:
> > > The assumption of this change is that, all GGTT addresses beyond
> > > aperture are only accessed by device when executing a workload,
> > > so we can delay update of those entries until the point of detecting
> > > a workload submission. However such assumption may not be 
> > > true, at least for display. What about guest chooses a way to flip
> > > framebuffers not by changing base address while instead by changing
> > > GGTT mapping on the same base address? If such guest framebuffer 
> > > is already programmed to physical display plane (as required in local
> > > display usage e.g. automotive), then delay of update would lead to
> > > incorrect content being shown. Current driver might not behave
> > > like that, but in GVT-g side we should make sure it's sane.
> > > 
> > > > From: Weinan Li
> > > > Sent: Friday, November 3, 2017 9:01 AM
> > > > 
> > > > Add delayed_update flag in gvt->gtt to indicate there is cached gtt entries
> > > > need to be flushed before vgpu workload submittion, then don't need to
> > > > trigger invalidate gtt every time gtt entry write, it can save time in vgpu
> > > > reset or vgpu has frequently gtt entries updating case.
> > > > 
> > > > v2 : use flag instead of atomic_t, code refine (Zhi)
> > > > 
> > > > Signed-off-by: Weinan Li <weinan.z.li at intel.com>
> > > > Cc: Zhi Wang <zhi.a.wang at intel.com>
> > > > Cc: Zhenyu Wang <zhenyuw at linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/gvt/gtt.c | 20 ++++++++++++++++++--
> > > >  drivers/gpu/drm/i915/gvt/gtt.h |  1 +
> > > >  2 files changed, 19 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c
> > > > b/drivers/gpu/drm/i915/gvt/gtt.c
> > > > index 6fa9271..b4cb5ae 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > > > @@ -1293,6 +1293,15 @@ static int ppgtt_set_guest_page_oos(struct
> > > > intel_vgpu *vgpu,
> > > >  	return intel_gvt_hypervisor_disable_page_track(vgpu, &gpt->track);
> > > >  }
> > > > 
> > > > +int intel_vgpu_flush_mm(struct intel_gvt *gvt, bool force)
> > > > +{
> > > > +	if (force || gvt->gtt.delayed_update) {
> > > > +		gvt->gtt.delayed_update = false;
> > > > +		gtt_invalidate(gvt->dev_priv);
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  /**
> > > >   * intel_vgpu_sync_oos_pages - sync all the out-of-synced shadow for
> > > > vGPU
> > > >   * @vgpu: a vGPU
> > > > @@ -1405,6 +1414,8 @@ int intel_vgpu_flush_post_shadow(struct
> > > > intel_vgpu *vgpu)
> > > >  	unsigned long index;
> > > >  	int ret;
> > > > 
> > > > +	intel_vgpu_flush_mm(vgpu->gvt, false);
> > > > +
> > > >  	list_for_each_safe(pos, n, &vgpu->gtt.post_shadow_list_head) {
> > > >  		spt = container_of(pos, struct intel_vgpu_ppgtt_spt,
> > > >  				post_shadow_list);
> > > > @@ -1953,7 +1964,6 @@ static int emulate_gtt_mmio_write(struct
> > > > intel_vgpu *vgpu, unsigned int off,
> > > >  	/* the VM may configure the whole GM space when ballooning is
> > > > used */
> > > >  	if (!vgpu_gmadr_is_valid(vgpu, gma))
> > > >  		return 0;
> > > > -
> > > >  	ggtt_get_guest_entry(ggtt_mm, &e, g_gtt_index);
> > > > 
> > > >  	memcpy((void *)&e.val64 + (off & (info->gtt_entry_size - 1)), p_data,
> > > > @@ -1975,8 +1985,14 @@ static int emulate_gtt_mmio_write(struct
> > > > intel_vgpu *vgpu, unsigned int off,
> > > >  	}
> > > > 
> > > >  	ggtt_set_shadow_entry(ggtt_mm, &m, g_gtt_index);
> > > > -	gtt_invalidate(gvt->dev_priv);
> > > > +
> > > > +	if (vgpu_gmadr_is_aperture(vgpu, gma))
> > > > +		gtt_invalidate(gvt->dev_priv);
> > > > +	else
> > > > +		gvt->gtt.delayed_update = true;
> > > > +
> > > >  	ggtt_set_guest_entry(ggtt_mm, &e, g_gtt_index);
> > > > +
> > > >  	return 0;
> > > >  }
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.h
> > > > b/drivers/gpu/drm/i915/gvt/gtt.h
> > > > index 416b2f8..f36f485 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/gtt.h
> > > > +++ b/drivers/gpu/drm/i915/gvt/gtt.h
> > > > @@ -88,6 +88,7 @@ struct intel_gvt_gtt {
> > > > 
> > > >  	struct page *scratch_page;
> > > >  	unsigned long scratch_mfn;
> > > > +	bool delayed_update;
> > > >  };
> > > > 
> > > >  enum {
> > > > --
> > > > 1.9.1
> > > > 
> > > > _______________________________________________
> > > > 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
> > 
> > -- 
> > Thanks,
> > Changbin Du
> 
> 
> 
> > _______________________________________________
> > 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

-- 
Thanks,
Changbin Du
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20171103/bc2e0b1d/attachment.sig>