[PATCH v3 12/15] drm/i915/gvt: Don't extend page_track to mpt layer

Tue Jan 23 03:52:37 UTC 2018

On Tue, Jan 23, 2018 at 11:10:54AM +0800, Zhenyu Wang wrote:
> On 2018.01.10 02:52:32 +0000, Du, Changbin wrote:
> > The problem is changing mpt protocol will beak xengt on next rebase. 
> > 
> > + Xiong for xengt.
> >
> 
> I think "page track" might be more general name for this either
> by WP or other page track utility, that we might change MPT hook as well.
> 
Already checked with Xiong, XEN's implementation is real WP. Anyway, we should
keep consistent with MPT which GVT comminicate with (Means we should not change
the MPT's semantics because of the backend). I also think page_track is better
since now only kvmgt is in upstream. So will change mpt definition. Please Xiong
be aware of this. Thanks!

> btw, is this one bisect-able? It looks you removed counter for tracked page
> which is still used in WP handler in this? Even I like your later change, but
> we still need to make each one bisect-able.
> 
You are right, will fix it to be bisect-able.

> > -----Original Message-----
> > From: Zhao, Yan Y 
> > Sent: Wednesday, January 10, 2018 10:46 AM
> > To: Du, Changbin <changbin.du at intel.com>
> > Cc: Du, Changbin <changbin.du at intel.com>; intel-gvt-dev at lists.freedesktop.org
> > Subject: RE: [PATCH v3 12/15] drm/i915/gvt: Don't extend page_track to mpt layer
> > 
> > In xengt, write-protect is write protect. But In kvmgt, write-protect is page-tracker, meaning the page is not write-protected, but we got notification on page write occurs.
> > So, I think we can change from "page_track -> write-protection -> page_track"
> > To " page_track -> page_track -> page_track"
> > 
> > After all, change xengt from wp to page track only requires 1 line of code.
> > 
> > > -----Original Message-----
> > > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On
> > > Behalf Of Du, Changbin
> > > Sent: Wednesday, January 10, 2018 10:20 AM
> > > To: Zhao, Yan Y <yan.y.zhao at intel.com>
> > > Cc: Du, Changbin <changbin.du at intel.com>; intel-gvt-
> > > dev at lists.freedesktop.org
> > > Subject: Re: [PATCH v3 12/15] drm/i915/gvt: Don't extend page_track to mpt
> > > layer
> > > 
> > > On Wed, Jan 10, 2018 at 09:11:04AM +0800, Zhao, Yan Y wrote:
> > > >
> > > > instead, what about rename mpt interface to page tracker?
> > > > 1. actually what we did in mpt.set_wp_page() is page tracker 2.  if
> > > > you change
> > > >
> > > > intel_gvt_hypervisor_enable_page_track() to
> > > intel_gvt_hypervisor_set_wp_page(), but actually the page is not write-
> > > protected, it will make future code writer confused.
> > > >
> > > Yes, change the mpt protocol. And it requires xengt change accordingly. Before
> > > that double confusing is worse: page_track -> write-protection -> page_track.
> > > and how does xengt call it?
> > > 
> > > > On 1/9/2018 5:42 PM, Du, Changbin wrote:
> > > > > On Tue, Jan 09, 2018 at 04:55:54PM +0800, Zhao, Yan Y wrote:
> > > > > > hi changbin,
> > > > > >
> > > > > > please see my comment inline.
> > > > > >
> > > > > > On 1/8/2018 6:24 PM, changbin.du at intel.com wrote:
> > > > > > > From: Changbin Du <changbin.du at intel.com>
> > > > > > >
> > > > > > > Don't extend page_track to mpt layer. Keep MPT simple and clean.
> > > > > > >
> > > > > > > Signed-off-by: Changbin Du <changbin.du at intel.com>
> > > > > > > ---
> > > > > > >    drivers/gpu/drm/i915/gvt/gtt.c | 19 +++++++++++++------
> > > > > > >    drivers/gpu/drm/i915/gvt/mpt.h | 42 ++++++++++--------------------------
> > > ------
> > > > > > >    2 files changed, 23 insertions(+), 38 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c
> > > > > > > b/drivers/gpu/drm/i915/gvt/gtt.c index 642d9de..9df9c9bc 100644
> > > > > > > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > > > > > > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > > > > > > @@ -677,8 +677,8 @@ static void ppgtt_free_spt(struct
> > > intel_vgpu_ppgtt_spt *spt)
> > > > > > >    		hash_del(&spt->guest_page.track.node);
> > > > > > >    	if (spt->guest_page.track.tracked)
> > > > > > > -		intel_gvt_hypervisor_disable_page_track(spt->vgpu,
> > > > > > > -				&spt->guest_page.track);
> > > > > > > +		intel_gvt_hypervisor_unset_wp_page(spt->vgpu,
> > > > > > > +				spt->guest_page.track.gfn);
> > > > > > unset_wp_page may confuse the users in kvmgt, since we actually
> > > > > > use the page tracker mechanism in kvmgt, which is not write
> > > > > > protection, because the guest page has been already changed before
> > > > > > we got notification. we have no way to protect the write.
> > > > > > so disable_page_track is better for kvmgt.
> > > > > Then the change should be made in the mpt protocol if they have
> > > > > different semantics. Since mpt should be synced with both xen and
> > > > > kvmgt, I'd leave this to future. In this patch, we are trying not to
> > > > > change the mpt semantics by just adding a simple wrapper layer.
> > > > >
> > > > > Anyway this is not so terriable since the page track is based on write-
> > > protection.
> > > > >
> > > >
> > > 
> > > --
> > > Thanks,
> > > Changbin Du
> > > _______________________________________________
> > > 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


-- 
Thanks,
Changbin Du