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

[Zhao, Yan Y]

hi changbin

Please see my comments 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.
>   	list_del_init(&spt->post_shadow_list);
>   	free_spt(spt);
> @@ -926,10 +926,11 @@ static struct intel_vgpu_ppgtt_spt *ppgtt_populate_spt_by_guest_entry(
>   			goto fail;
>   		}
>   
> -		ret = intel_gvt_hypervisor_enable_page_track(vgpu, &spt->guest_page.track);
> +		ret = intel_gvt_hypervisor_set_wp_page(vgpu, spt->guest_page.track.gfn);
>   		if (ret)
>   			goto fail;
>   
> +		spt->guest_page.track.tracked = true;
>   		ret = ppgtt_populate_spt(spt);
>   		if (ret)
>   			goto fail;
> @@ -1196,9 +1197,10 @@ static int ppgtt_set_guest_page_sync(struct intel_vgpu_ppgtt_spt *spt)
>   	struct intel_vgpu_oos_page *oos_page = spt->guest_page.oos_page;
>   	int ret;
>   
> -	ret = intel_gvt_hypervisor_enable_page_track(spt->vgpu, &spt->guest_page.track);
> +	ret = intel_gvt_hypervisor_set_wp_page(spt->vgpu, spt->guest_page.track.gfn);
>   	if (ret)
>   		return ret;
The same as above
> +	spt->guest_page.track.tracked = true;
>   
>   	trace_oos_change(spt->vgpu->id, "set page sync", oos_page->id,
>   			 spt, spt->guest_page.type);
> @@ -1234,6 +1236,7 @@ static int ppgtt_allocate_oos_page(struct intel_vgpu_ppgtt_spt *spt)
>   static int ppgtt_set_guest_page_oos(struct intel_vgpu_ppgtt_spt *spt)
>   {
>   	struct intel_vgpu_oos_page *oos_page = spt->guest_page.oos_page;
> +	int ret;
>   
>   	if (WARN(!oos_page, "shadow PPGTT page should have a oos page\n"))
>   		return -EINVAL;
> @@ -1242,7 +1245,11 @@ static int ppgtt_set_guest_page_oos(struct intel_vgpu_ppgtt_spt *spt)
>   			 spt, spt->guest_page.type);
>   
>   	list_add_tail(&oos_page->vm_list, &spt->vgpu->gtt.oos_page_list_head);
> -	return intel_gvt_hypervisor_disable_page_track(spt->vgpu, &spt->guest_page.track);
> +	ret = intel_gvt_hypervisor_unset_wp_page(spt->vgpu, spt->guest_page.track.gfn);
> +	if (ret)
> +		return ret;
> +	spt->guest_page.track.tracked = false;
> +	return 0;
>   }
>   
>   /**
> @@ -1926,7 +1933,7 @@ int intel_vgpu_write_protect_handler(struct intel_vgpu *vgpu, u64 pa,
>   		if (t) {
>   			if (unlikely(vgpu->failsafe)) {
>   				/* remove write protection to prevent furture traps */
> -				intel_gvt_hypervisor_disable_page_track(vgpu, t);
> +				intel_gvt_hypervisor_unset_wp_page(vgpu, t->gfn);
>   			} else {
>   				ret = t->handler(t, pa, p_data, bytes);
>   				if (ret) {
> diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
> index 81aff4e..1973158 100644
> --- a/drivers/gpu/drm/i915/gvt/mpt.h
> +++ b/drivers/gpu/drm/i915/gvt/mpt.h
> @@ -154,54 +154,32 @@ static inline unsigned long intel_gvt_hypervisor_virt_to_mfn(void *p)
>   }
>   
>   /**
> - * intel_gvt_hypervisor_enable - set a guest page to write-protected
> + * intel_gvt_hypervisor_set_wp_page - set a guest page to write-protected
>    * @vgpu: a vGPU
> - * @t: page track data structure
> + * @gfn: the gfn of guest
>    *
>    * Returns:
>    * Zero on success, negative error code if failed.
>    */
> -static inline int intel_gvt_hypervisor_enable_page_track(
> -		struct intel_vgpu *vgpu,
> -		struct intel_vgpu_page_track *t)
> +static inline int intel_gvt_hypervisor_set_wp_page(
> +		struct intel_vgpu *vgpu, unsigned long gfn)
>   {
> -	int ret;
> -
> -	if (t->tracked)
> -		return 0;
> -
> -	ret = intel_gvt_host.mpt->set_wp_page(vgpu->handle, t->gfn);
> -	if (ret)
> -		return ret;
> -	t->tracked = true;
> -	atomic_inc(&vgpu->gtt.n_tracked_guest_page);
> -	return 0;
> +	return intel_gvt_host.mpt->set_wp_page(vgpu->handle, gfn);
>   }
>   
>   /**
> - * intel_gvt_hypervisor_disable_page_track - remove the write-protection of a
> + * intel_gvt_hypervisor_unset_wp_page - remove the write-protection of a
>    * guest page
>    * @vgpu: a vGPU
> - * @t: page track data structure
> + * @gfn: the gfn of guest
>    *
>    * Returns:
>    * Zero on success, negative error code if failed.
>    */
> -static inline int intel_gvt_hypervisor_disable_page_track(
> -		struct intel_vgpu *vgpu,
> -		struct intel_vgpu_page_track *t)
> +static inline int intel_gvt_hypervisor_unset_wp_page(
> +		struct intel_vgpu *vgpu, unsigned long gfn)
>   {
> -	int ret;
> -
> -	if (!t->tracked)
> -		return 0;
> -
> -	ret = intel_gvt_host.mpt->unset_wp_page(vgpu->handle, t->gfn);
> -	if (ret)
> -		return ret;
> -	t->tracked = false;
> -	atomic_dec(&vgpu->gtt.n_tracked_guest_page);
> -	return 0;
> +	return intel_gvt_host.mpt->unset_wp_page(vgpu->handle, gfn);
>   }
>   
>   /**