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

[Wang, Zhi A]

So far, we will not unpin any pages once they got pinned.

-----Original Message-----
From: Du, Changbin 
Sent: Friday, November 3, 2017 4:27 AM
To: Tian, Kevin <kevin.tian at intel.com>
Cc: Li, Weinan Z <weinan.z.li at intel.com>; intel-gvt-dev at lists.freedesktop.org; Wang, Zhi A <zhi.a.wang at intel.com>; Zhenyu Wang <zhenyuw at linux.intel.com>; Du, Changbin <changbin.du at intel.com>
Subject: Re: [PATCH v2] drm/i915/gvt: refine vgpu gtt sync policy to reduce invalidate times

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.

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