From 4b2dbbc22541e44e10e22836149050ab6dbd879e Mon Sep 17 00:00:00 2001 From: Changbin Du Date: Wed, 2 Aug 2017 15:06:37 +0800 Subject: drm/i915/gvt: Add carefully checking in GTT walker paths When debugging the gtt code, found the intel_vgpu_gma_to_gpa() can translate any given GMA though the GMA is not valid. This because the GTT ops suppress the possible errors, which may result in an invalid PT entry is retrieved by upper caller. This patch changed the prototype of pte ops to propagate status to callers. Then we make sure the GTT walker stop as early as when a error is detected to prevent undefined behavior. Signed-off-by: Changbin Du Signed-off-by: Zhenyu Wang --- drivers/gpu/drm/i915/gvt/gtt.c | 77 +++++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 27 deletions(-) (limited to 'drivers/gpu/drm/i915/gvt/gtt.c') diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c index 6166e34d892b..e397f5e0722f 100644 --- a/drivers/gpu/drm/i915/gvt/gtt.c +++ b/drivers/gpu/drm/i915/gvt/gtt.c @@ -259,7 +259,7 @@ static void write_pte64(struct drm_i915_private *dev_priv, writeq(pte, addr); } -static inline struct intel_gvt_gtt_entry *gtt_get_entry64(void *pt, +static inline int gtt_get_entry64(void *pt, struct intel_gvt_gtt_entry *e, unsigned long index, bool hypervisor_access, unsigned long gpa, struct intel_vgpu *vgpu) @@ -268,22 +268,23 @@ static inline struct intel_gvt_gtt_entry *gtt_get_entry64(void *pt, int ret; if (WARN_ON(info->gtt_entry_size != 8)) - return e; + return -EINVAL; if (hypervisor_access) { ret = intel_gvt_hypervisor_read_gpa(vgpu, gpa + (index << info->gtt_entry_size_shift), &e->val64, 8); - WARN_ON(ret); + if (WARN_ON(ret)) + return ret; } else if (!pt) { e->val64 = read_pte64(vgpu->gvt->dev_priv, index); } else { e->val64 = *((u64 *)pt + index); } - return e; + return 0; } -static inline struct intel_gvt_gtt_entry *gtt_set_entry64(void *pt, +static inline int gtt_set_entry64(void *pt, struct intel_gvt_gtt_entry *e, unsigned long index, bool hypervisor_access, unsigned long gpa, struct intel_vgpu *vgpu) @@ -292,19 +293,20 @@ static inline struct intel_gvt_gtt_entry *gtt_set_entry64(void *pt, int ret; if (WARN_ON(info->gtt_entry_size != 8)) - return e; + return -EINVAL; if (hypervisor_access) { ret = intel_gvt_hypervisor_write_gpa(vgpu, gpa + (index << info->gtt_entry_size_shift), &e->val64, 8); - WARN_ON(ret); + if (WARN_ON(ret)) + return ret; } else if (!pt) { write_pte64(vgpu->gvt->dev_priv, index, e->val64); } else { *((u64 *)pt + index) = e->val64; } - return e; + return 0; } #define GTT_HAW 46 @@ -445,21 +447,25 @@ static int gtt_entry_p2m(struct intel_vgpu *vgpu, struct intel_gvt_gtt_entry *p, /* * MM helpers. */ -struct intel_gvt_gtt_entry *intel_vgpu_mm_get_entry(struct intel_vgpu_mm *mm, +int intel_vgpu_mm_get_entry(struct intel_vgpu_mm *mm, void *page_table, struct intel_gvt_gtt_entry *e, unsigned long index) { struct intel_gvt *gvt = mm->vgpu->gvt; struct intel_gvt_gtt_pte_ops *ops = gvt->gtt.pte_ops; + int ret; e->type = mm->page_table_entry_type; - ops->get_entry(page_table, e, index, false, 0, mm->vgpu); + ret = ops->get_entry(page_table, e, index, false, 0, mm->vgpu); + if (ret) + return ret; + ops->test_pse(e); - return e; + return 0; } -struct intel_gvt_gtt_entry *intel_vgpu_mm_set_entry(struct intel_vgpu_mm *mm, +int intel_vgpu_mm_set_entry(struct intel_vgpu_mm *mm, void *page_table, struct intel_gvt_gtt_entry *e, unsigned long index) { @@ -472,7 +478,7 @@ struct intel_gvt_gtt_entry *intel_vgpu_mm_set_entry(struct intel_vgpu_mm *mm, /* * PPGTT shadow page table helpers. */ -static inline struct intel_gvt_gtt_entry *ppgtt_spt_get_entry( +static inline int ppgtt_spt_get_entry( struct intel_vgpu_ppgtt_spt *spt, void *page_table, int type, struct intel_gvt_gtt_entry *e, unsigned long index, @@ -480,20 +486,24 @@ static inline struct intel_gvt_gtt_entry *ppgtt_spt_get_entry( { struct intel_gvt *gvt = spt->vgpu->gvt; struct intel_gvt_gtt_pte_ops *ops = gvt->gtt.pte_ops; + int ret; e->type = get_entry_type(type); if (WARN(!gtt_type_is_entry(e->type), "invalid entry type\n")) - return e; + return -EINVAL; - ops->get_entry(page_table, e, index, guest, + ret = ops->get_entry(page_table, e, index, guest, spt->guest_page.gfn << GTT_PAGE_SHIFT, spt->vgpu); + if (ret) + return ret; + ops->test_pse(e); - return e; + return 0; } -static inline struct intel_gvt_gtt_entry *ppgtt_spt_set_entry( +static inline int ppgtt_spt_set_entry( struct intel_vgpu_ppgtt_spt *spt, void *page_table, int type, struct intel_gvt_gtt_entry *e, unsigned long index, @@ -503,7 +513,7 @@ static inline struct intel_gvt_gtt_entry *ppgtt_spt_set_entry( struct intel_gvt_gtt_pte_ops *ops = gvt->gtt.pte_ops; if (WARN(!gtt_type_is_entry(e->type), "invalid entry type\n")) - return e; + return -EINVAL; return ops->set_entry(page_table, e, index, guest, spt->guest_page.gfn << GTT_PAGE_SHIFT, @@ -792,13 +802,13 @@ static struct intel_vgpu_ppgtt_spt *ppgtt_find_shadow_page( #define for_each_present_guest_entry(spt, e, i) \ for (i = 0; i < pt_entries(spt); i++) \ - if (spt->vgpu->gvt->gtt.pte_ops->test_present( \ - ppgtt_get_guest_entry(spt, e, i))) + if (!ppgtt_get_guest_entry(spt, e, i) && \ + spt->vgpu->gvt->gtt.pte_ops->test_present(e)) #define for_each_present_shadow_entry(spt, e, i) \ for (i = 0; i < pt_entries(spt); i++) \ - if (spt->vgpu->gvt->gtt.pte_ops->test_present( \ - ppgtt_get_shadow_entry(spt, e, i))) + if (!ppgtt_get_shadow_entry(spt, e, i) && \ + spt->vgpu->gvt->gtt.pte_ops->test_present(e)) static void ppgtt_get_shadow_page(struct intel_vgpu_ppgtt_spt *spt) { @@ -1713,8 +1723,10 @@ unsigned long intel_vgpu_gma_to_gpa(struct intel_vgpu_mm *mm, unsigned long gma) if (!vgpu_gmadr_is_valid(vgpu, gma)) goto err; - ggtt_get_guest_entry(mm, &e, - gma_ops->gma_to_ggtt_pte_index(gma)); + ret = ggtt_get_guest_entry(mm, &e, + gma_ops->gma_to_ggtt_pte_index(gma)); + if (ret) + goto err; gpa = (pte_ops->get_pfn(&e) << GTT_PAGE_SHIFT) + (gma & ~GTT_PAGE_MASK); @@ -1724,7 +1736,9 @@ unsigned long intel_vgpu_gma_to_gpa(struct intel_vgpu_mm *mm, unsigned long gma) switch (mm->page_table_level) { case 4: - ppgtt_get_shadow_root_entry(mm, &e, 0); + ret = ppgtt_get_shadow_root_entry(mm, &e, 0); + if (ret) + goto err; gma_index[0] = gma_ops->gma_to_pml4_index(gma); gma_index[1] = gma_ops->gma_to_l4_pdp_index(gma); gma_index[2] = gma_ops->gma_to_pde_index(gma); @@ -1732,15 +1746,19 @@ unsigned long intel_vgpu_gma_to_gpa(struct intel_vgpu_mm *mm, unsigned long gma) index = 4; break; case 3: - ppgtt_get_shadow_root_entry(mm, &e, + ret = ppgtt_get_shadow_root_entry(mm, &e, gma_ops->gma_to_l3_pdp_index(gma)); + if (ret) + goto err; gma_index[0] = gma_ops->gma_to_pde_index(gma); gma_index[1] = gma_ops->gma_to_pte_index(gma); index = 2; break; case 2: - ppgtt_get_shadow_root_entry(mm, &e, + ret = ppgtt_get_shadow_root_entry(mm, &e, gma_ops->gma_to_pde_index(gma)); + if (ret) + goto err; gma_index[0] = gma_ops->gma_to_pte_index(gma); index = 1; break; @@ -1755,6 +1773,11 @@ unsigned long intel_vgpu_gma_to_gpa(struct intel_vgpu_mm *mm, unsigned long gma) (i == index - 1)); if (ret) goto err; + + if (!pte_ops->test_present(&e)) { + gvt_dbg_core("GMA 0x%lx is not present\n", gma); + goto err; + } } gpa = (pte_ops->get_pfn(&e) << GTT_PAGE_SHIFT) -- cgit v1.2.3 From 4d3e67bb6fa26e50eb087799d98ec232acfb630d Mon Sep 17 00:00:00 2001 From: Chuanxiao Dong Date: Fri, 4 Aug 2017 13:08:59 +0800 Subject: drm/i915/gvt: Refine the intel_vgpu_reset_gtt reset function When doing the VGPU reset, we don't need to do the gtt/ppgtt reset. This will make the GVT to do the ppgtt shadow every time for a workload and caused really bad performance after a VGPU reset. This patch will make sure ppgtt clean only happen at device module level reset to fix this. Signed-off-by: Chuanxiao Dong Signed-off-by: Zhenyu Wang --- drivers/gpu/drm/i915/gvt/gtt.c | 6 +----- drivers/gpu/drm/i915/gvt/gtt.h | 2 +- drivers/gpu/drm/i915/gvt/vgpu.c | 6 +++--- 3 files changed, 5 insertions(+), 9 deletions(-) (limited to 'drivers/gpu/drm/i915/gvt/gtt.c') diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c index e397f5e0722f..f862681c70d1 100644 --- a/drivers/gpu/drm/i915/gvt/gtt.c +++ b/drivers/gpu/drm/i915/gvt/gtt.c @@ -2352,13 +2352,12 @@ void intel_vgpu_reset_ggtt(struct intel_vgpu *vgpu) /** * intel_vgpu_reset_gtt - reset the all GTT related status * @vgpu: a vGPU - * @dmlr: true for vGPU Device Model Level Reset, false for GT Reset * * This function is called from vfio core to reset reset all * GTT related status, including GGTT, PPGTT, scratch page. * */ -void intel_vgpu_reset_gtt(struct intel_vgpu *vgpu, bool dmlr) +void intel_vgpu_reset_gtt(struct intel_vgpu *vgpu) { int i; @@ -2370,9 +2369,6 @@ void intel_vgpu_reset_gtt(struct intel_vgpu *vgpu, bool dmlr) */ intel_vgpu_free_mm(vgpu, INTEL_GVT_MM_PPGTT); - if (!dmlr) - return; - intel_vgpu_reset_ggtt(vgpu); /* clear scratch page for security */ diff --git a/drivers/gpu/drm/i915/gvt/gtt.h b/drivers/gpu/drm/i915/gvt/gtt.h index abb41ee1409b..30a4c8d16026 100644 --- a/drivers/gpu/drm/i915/gvt/gtt.h +++ b/drivers/gpu/drm/i915/gvt/gtt.h @@ -212,7 +212,7 @@ extern void intel_vgpu_clean_gtt(struct intel_vgpu *vgpu); void intel_vgpu_reset_ggtt(struct intel_vgpu *vgpu); extern int intel_gvt_init_gtt(struct intel_gvt *gvt); -extern void intel_vgpu_reset_gtt(struct intel_vgpu *vgpu, bool dmlr); +void intel_vgpu_reset_gtt(struct intel_vgpu *vgpu); extern void intel_gvt_clean_gtt(struct intel_gvt *gvt); extern struct intel_vgpu_mm *intel_gvt_find_ppgtt_mm(struct intel_vgpu *vgpu, diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c index 90c14e6e3ea0..5b44d123bf24 100644 --- a/drivers/gpu/drm/i915/gvt/vgpu.c +++ b/drivers/gpu/drm/i915/gvt/vgpu.c @@ -502,11 +502,11 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr, /* full GPU reset or device model level reset */ if (engine_mask == ALL_ENGINES || dmlr) { - intel_vgpu_reset_gtt(vgpu, dmlr); - /*fence will not be reset during virtual reset */ - if (dmlr) + if (dmlr) { + intel_vgpu_reset_gtt(vgpu); intel_vgpu_reset_resource(vgpu); + } intel_vgpu_reset_mmio(vgpu, dmlr); populate_pvinfo_page(vgpu); -- cgit v1.2.3 From 6b3816d69628becb7ff35978aa0751798b4a940a Mon Sep 17 00:00:00 2001 From: Tina Zhang Date: Mon, 14 Aug 2017 15:24:14 +0800 Subject: drm/i915/gvt: Fix guest i915 full ppgtt blocking issue Guest i915 full ppgtt functionality was blocking by an issue, which would lead to gpu hardware hang. 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. Changes in v2: - Move VGT_CAPS_FULL_PPGTT introduction to patch 2/4. (Joonas) Changes since v2: - Divide the whole patch set into two separate patch series, with one patch in i915 side to check guest i915 full ppgtt capability and enable it when this capability is supported by the device model, and the other one in gvt side which fixs the blocking issue and enables the device model to provide the capability to guest. And this patch focuses on gvt side. (Joonas) - Change the title from "reorder the shadow ppgtt update process by adding entry first" to "Fix guest i915 full ppgtt blocking issue". (Tina) Changes since v3: - Rebase to the latest branch. Changes since v4: - Tested by Tina Zhang. Changes since v5: - Rebase to the latest branch. v6: - Update full 48bit ppgtt definition Cc: Tina Zhang Signed-off-by: Tina Zhang Signed-off-by: Zhenyu Wang --- drivers/gpu/drm/i915/gvt/gtt.c | 45 +++++++++++++++++++++++++---------------- drivers/gpu/drm/i915/gvt/vgpu.c | 1 + 2 files changed, 29 insertions(+), 17 deletions(-) (limited to 'drivers/gpu/drm/i915/gvt/gtt.c') diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c index f862681c70d1..e6dfc3331f4b 100644 --- a/drivers/gpu/drm/i915/gvt/gtt.c +++ b/drivers/gpu/drm/i915/gvt/gtt.c @@ -989,29 +989,26 @@ fail: } 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; @@ -1021,12 +1018,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; } @@ -1246,22 +1241,37 @@ 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; + /* + * Adding the new entry first and then removing the old one, that can + * guarantee the ppgtt table is validated during the window between + * adding and removal. + */ + 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", @@ -1333,7 +1343,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; @@ -1349,7 +1359,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; } diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c index 5b44d123bf24..5896ead8529e 100644 --- a/drivers/gpu/drm/i915/gvt/vgpu.c +++ b/drivers/gpu/drm/i915/gvt/vgpu.c @@ -43,6 +43,7 @@ void populate_pvinfo_page(struct intel_vgpu *vgpu) vgpu_vreg(vgpu, vgtif_reg(version_minor)) = 0; vgpu_vreg(vgpu, vgtif_reg(display_ready)) = 0; vgpu_vreg(vgpu, vgtif_reg(vgt_id)) = vgpu->id; + vgpu_vreg(vgpu, vgtif_reg(vgt_caps)) = VGT_CAPS_FULL_48BIT_PPGTT; vgpu_vreg(vgpu, vgtif_reg(avail_rs.mappable_gmadr.base)) = vgpu_aperture_gmadr_base(vgpu); vgpu_vreg(vgpu, vgtif_reg(avail_rs.mappable_gmadr.size)) = -- cgit v1.2.3