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

[Zhenyu Wang]

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.

> 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
-------------- 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/20171103/869a5ea9/attachment.sig>