[PATCH 1/2] drm/i915/gvt: reorder the shadow ppgtt update process by adding entry first

[Zhang, Tina]

Thanks for Zhi's comments. I'm going to rewrite this patch and move it to a new patch set including 4 related patches.

> -----Original Message-----
> From: Wang, Zhi A
> Sent: Tuesday, May 9, 2017 10:26 AM
> To: Zhang, Tina <tina.zhang at intel.com>; intel-gvt-dev at lists.freedesktop.org
> Subject: Re: [PATCH 1/2] drm/i915/gvt: reorder the shadow ppgtt update
> process by adding entry first
> 
> 
> 于 05/09/17 10:22, Tina Zhang 写道:
> > Guest i915 driver may update the ppgtt table just before this workload
> > is going to be submitted to the hardware by device model. This case
> > wasn't handled well by device model before, due to the small time
> > window between removing old ppgtt entry and adding the new one. Errors
> > occur when the workload is executed by hardware during that small time
> > window. This patch is to remove this time window by adding the new
> > ppgtt entry first and then remove the old one.
> >
> > v2. add some comments. (Zhi) (Zhenyu)
> >
> > Signed-off-by: Tina Zhang <tina.zhang at intel.com>
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/gtt.c
> > b/drivers/gpu/drm/i915/gvt/gtt.c index c6f0077..1b91e69 100644
> > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > @@ -975,29 +975,26 @@ static int ppgtt_populate_shadow_page(struct
> intel_vgpu_ppgtt_spt *spt)
> >   }
> >
> >   static int ppgtt_handle_guest_entry_removal(struct intel_vgpu_guest_page
> *gpt,
> > -		unsigned long index)
> > +		struct intel_gvt_gtt_entry *se, unsigned long index)
> >   {
> >   	struct intel_vgpu_ppgtt_spt *spt = guest_page_to_ppgtt_spt(gpt);
> >   	struct intel_vgpu_shadow_page *sp = &spt->shadow_page;
> >   	struct intel_vgpu *vgpu = spt->vgpu;
> >   	struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
> > -	struct intel_gvt_gtt_entry e;
> >   	int ret;
> >
> > -	ppgtt_get_shadow_entry(spt, &e, index);
> > -
> > -	trace_gpt_change(spt->vgpu->id, "remove", spt, sp->type, e.val64,
> > +	trace_gpt_change(spt->vgpu->id, "remove", spt, sp->type, se->val64,
> >   			 index);
> >
> > -	if (!ops->test_present(&e))
> > +	if (!ops->test_present(se))
> >   		return 0;
> >
> > -	if (ops->get_pfn(&e) == vgpu->gtt.scratch_pt[sp->type].page_mfn)
> > +	if (ops->get_pfn(se) == vgpu->gtt.scratch_pt[sp->type].page_mfn)
> >   		return 0;
> >
> > -	if (gtt_type_is_pt(get_next_pt_type(e.type))) {
> > +	if (gtt_type_is_pt(get_next_pt_type(se->type))) {
> >   		struct intel_vgpu_ppgtt_spt *s =
> > -			ppgtt_find_shadow_page(vgpu, ops->get_pfn(&e));
> > +			ppgtt_find_shadow_page(vgpu, ops->get_pfn(se));
> >   		if (!s) {
> >   			gvt_vgpu_err("fail to find guest page\n");
> >   			ret = -ENXIO;
> > @@ -1007,12 +1004,10 @@ static int
> ppgtt_handle_guest_entry_removal(struct intel_vgpu_guest_page *gpt,
> >   		if (ret)
> >   			goto fail;
> >   	}
> > -	ops->set_pfn(&e, vgpu->gtt.scratch_pt[sp->type].page_mfn);
> > -	ppgtt_set_shadow_entry(spt, &e, index);
> >   	return 0;
> >   fail:
> >   	gvt_vgpu_err("fail: shadow page %p guest entry 0x%llx type %d\n",
> > -			spt, e.val64, e.type);
> > +			spt, se->val64, se->type);
> >   	return ret;
> >   }
> >
> > @@ -1232,22 +1227,39 @@ static int
> ppgtt_handle_guest_write_page_table(
> >   {
> >   	struct intel_vgpu_ppgtt_spt *spt = guest_page_to_ppgtt_spt(gpt);
> >   	struct intel_vgpu *vgpu = spt->vgpu;
> > +	int type = spt->shadow_page.type;
> >   	struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
> > +	struct intel_gvt_gtt_entry se;
> >
> >   	int ret;
> >   	int new_present;
> >
> >   	new_present = ops->test_present(we);
> >
> > -	ret = ppgtt_handle_guest_entry_removal(gpt, index);
> > -	if (ret)
> > -		goto fail;
> > +	/*
> > +	 * Guest i915 driver may update the ppgtt table just before the related
> > +	 * workload going to be submitted to the hardware by device model. If
> > +	 * the submisson is happened during the process of updating ppgtt table,
> > +	 * a provision must be made for guaranteeing the validation of ppgttt,
> > +	 * by adding new ppgtt entry first and then removing the old one.
> > +	 */
> > +	ppgtt_get_shadow_entry(spt, &se, index);
> >
> As we have improved the process of shadow updating, the developer who might
> read this code will be confused. Better just introduce the process of building
> new shadow. For the background, he can find it from the commit message.
Sensible. Thanks.

> >   	if (new_present) {
> >   		ret = ppgtt_handle_guest_entry_add(gpt, we, index);
> >   		if (ret)
> >   			goto fail;
> >   	}
> > +
> > +	ret = ppgtt_handle_guest_entry_removal(gpt, &se, index);
> > +	if (ret)
> > +		goto fail;
> > +
> > +	if (!new_present) {
> > +		ops->set_pfn(&se, vgpu->gtt.scratch_pt[type].page_mfn);
> > +		ppgtt_set_shadow_entry(spt, &se, index);
> > +	}
> > +
> >   	return 0;
> >   fail:
> >   	gvt_vgpu_err("fail: shadow page %p guest entry 0x%llx type %d.\n",
> > @@ -1319,7 +1331,7 @@ static int
> ppgtt_handle_guest_write_page_table_bytes(void *gp,
> >   	struct intel_vgpu *vgpu = spt->vgpu;
> >   	struct intel_gvt_gtt_pte_ops *ops = vgpu->gvt->gtt.pte_ops;
> >   	const struct intel_gvt_device_info *info = &vgpu->gvt->device_info;
> > -	struct intel_gvt_gtt_entry we;
> > +	struct intel_gvt_gtt_entry we, se;
> >   	unsigned long index;
> >   	int ret;
> >
> > @@ -1335,7 +1347,8 @@ static int
> ppgtt_handle_guest_write_page_table_bytes(void *gp,
> >   			return ret;
> >   	} else {
> >   		if (!test_bit(index, spt->post_shadow_bitmap)) {
> > -			ret = ppgtt_handle_guest_entry_removal(gpt, index);
> > +			ppgtt_get_shadow_entry(spt, &se, index);
> > +			ret = ppgtt_handle_guest_entry_removal(gpt, &se,
> index);
> >   			if (ret)
> >   				return ret;
> >   		}