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

[Zhang, Tina]

> -----Original Message-----
> From: Wang, Zhi A
> Sent: Thursday, May 4, 2017 5:43 PM
> To: Zhang, Tina <tina.zhang at intel.com>; intel-gvt-dev at lists.freedesktop.org
> Subject: Re: [PATCH v1 1/2] drm/i915/gvt: reorder the shadow ppgtt update
> process by adding entry first
> 
> 
> 于 05/04/17 17:35, 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.
> >
> > 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..81b01ce 100644
> > --- a/drivers/gpu/drm/i915/gvt/gtt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gtt.c
> > @@ -694,7 +694,6 @@ static void ppgtt_free_all_shadow_page(struct
> intel_vgpu *vgpu)
> >   	struct hlist_node *n;
> >   	struct intel_vgpu_shadow_page *sp;
> >   	int i;
> > -
> Do not remove the blank line here, or checkpatch.pl will complain...
Sorry for this thoughtless. Curiously, this patch has passed the checkpatch.pl. Anyway, the blank line will be removed in the next version.
> >   	hash_for_each_safe(vgpu->gtt.shadow_page_hash_table, i, n, sp, node)
> >   		ppgtt_free_shadow_page(shadow_page_to_ppgtt_spt(sp));
> >   }
> > @@ -975,29 +974,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 +1003,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 +1226,32 @@ 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);
> >
> Better add some comments here. :P
Thanks. I'm going to add the comments here.
> > -	ret = ppgtt_handle_guest_entry_removal(gpt, index);
> > -	if (ret)
> > -		goto fail;
> > +	ppgtt_get_shadow_entry(spt, &se, index);
> >
> >   	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 +1323,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 +1339,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;
> >   		}