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

Tue Jan 23 04:11:48 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!
[Zhang, Xiong Y] Once you decide to rename it, I could change it easily in xengt.c.
Another thing: both kvmgt and gtt have page track, it is redundancy in kvmgt, we could drop it from kvmgt and enhance page track function in gtt, so both kvmgt and xengt could use it.

> 
> > 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