[PATCH 2/2] drm/i915/gvt: A workaround for an regression caused by one i915 PPGTT optimization

[Wang, Zhi A]

Actually the requirement of lazy shadow is not letting invalid PTE case disappear since it's the behavior of guest.

The actual gap here is: we have to stop trapping invalid PTE page immediately.

That's why I call lazy shadow is a fix of root cause.

-----Original Message-----
From: Du, Changbin 
Sent: Thursday, October 19, 2017 9:08 AM
To: Wang, Zhi A <zhi.a.wang at intel.com>
Cc: Du, Changbin <changbin.du at intel.com>; Zhi Wang <zhi.wang.linux at gmail.com>; intel-gvt-dev at lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm/i915/gvt: A workaround for an regression caused by one i915 PPGTT optimization

On Wed, Oct 18, 2017 at 11:07:24PM -0700, Wang, Zhi A wrote:
> Practically yes. But we cannot assume guest is always behaving good. That's the reason why I call it a workaround and the root cause of fix should be the lazy shadow, which I hope I can make it work this week.
>
I'd think that's an optimization. Lazy shadowing doesn't mean invalid PTE case is gone, right? (dont aussme guest's behaviour.)

> Thanks,
> Zhi.
> 
> -----Original Message-----
> From: intel-gvt-dev 
> [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On Behalf Of Du, 
> Changbin
> Sent: Thursday, October 19, 2017 5:55 AM
> To: Zhi Wang <zhi.wang.linux at gmail.com>
> Cc: Du, Changbin <changbin.du at intel.com>; 
> intel-gvt-dev at lists.freedesktop.org; Wang, Zhi A 
> <zhi.a.wang at intel.com>
> Subject: Re: [PATCH 2/2] drm/i915/gvt: A workaround for an regression 
> caused by one i915 PPGTT optimization
> 
> I think this is not a workaround but a fix. GTE always has valid value is not a true assumption. Shadowing code should cover this case, and it is not an error (the error msg should remove).
> 
> On Thu, Oct 19, 2017 at 02:30:09AM +0800, Zhi Wang wrote:
> > In the commit:
> > 
> > commit 14826673247eaf36b16fd821fac27efa663f3fa6
> > Author: Chris Wilson <chris at chris-wilson.co.uk>
> > Date:   Fri Sep 8 19:16:22 2017 +0100
> > 
> >     drm/i915: Only initialize partially filled pagetables
> > 
> >     If we know that we will completely fill a pagetable (i.e. we are
> >     inserting a complete set of 512 pages), we can skip prefilling that PT
> >     with scratch entries. If we have to abort the insertion prior to writing
> >     the real entries, we will teardown the pagetable and remove it from the
> >     page directory (so that we will restart the allocation next time).
> > 
> >     We could do similar tricks for the PD and PDP, but the likelihood of a
> >     single insertion covering the entire 512 entries diminishes, as do the
> >     cycle savings. The saving are even greater (relatively) when we are
> >     preallocating page tables for huge pages, as then we never need to fill
> >     the page table.
> > 
> > It will link an un-initialized PTE page into a PPGTT page table 
> > tracked by GVT-g, which leads to linux guest failing to boot. Since 
> > the fix of root casue still needs some time to be ready, a temporary 
> > workaround is introduced first.
> > 
> > Signed-off-by: Zhi Wang <zhi.a.wang at intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/gtt.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c 
> > b/drivers/gpu/drm/i915/gvt/gtt.c index 6fa9271..d24d52d 100644
> > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > @@ -767,7 +767,6 @@ static int ppgtt_write_protection_handler(void
> > *data, u64 pa,  {
> >  	struct intel_vgpu_page_track *t = data;
> >  	struct intel_vgpu_guest_page *p = page_track_to_guest_page(t);
> > -	int ret;
> >  
> >  	if (bytes != 4 && bytes != 8)
> >  		return -EINVAL;
> > @@ -775,11 +774,9 @@ static int ppgtt_write_protection_handler(void *data, u64 pa,
> >  	if (!t->tracked)
> >  		return -EINVAL;
> >  
> > -	ret = ppgtt_handle_guest_write_page_table_bytes(p,
> > +	ppgtt_handle_guest_write_page_table_bytes(p,
> >  		pa, p_data, bytes);
> > -	if (ret)
> > -		return ret;
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  static int reclaim_one_mm(struct intel_gvt *gvt);
> > --
> > 2.7.4
> > 
> > _______________________________________________
> > intel-gvt-dev mailing list
> > intel-gvt-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> --
> Thanks,
> Changbin Du

--
Thanks,
Changbin Du