[PATCH 1/2] drm/i915/gvt: reorder the shadow ppgtt update process by adding entry first
Zhang, Tina
tina.zhang at intel.com
Thu May 11 00:49:43 UTC 2017
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;
> > }
More information about the intel-gvt-dev
mailing list