| Message ID | 1539841231-3157-1-git-send-email-xiong.y.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Add ppgtt to GVT GEM context | expand |
On 2018.10.18 13:40:31 +0800, Xiong Zhang wrote: > Currently the guest couldn't boot up under GVT-g environment as the > following call trace exists: > [ 272.504762] BUG: unable to handle kernel NULL pointer dereference at 0000000000000100 > [ 272.504834] Call Trace: > [ 272.504852] execlists_context_pin+0x2b2/0x520 [i915] > [ 272.504869] intel_gvt_scan_and_shadow_workload+0x50/0x4d0 [i915] > [ 272.504887] intel_vgpu_create_workload+0x3e2/0x570 [i915] > [ 272.504901] intel_vgpu_submit_execlist+0xc0/0x2a0 [i915] > [ 272.504916] elsp_mmio_write+0xc7/0x130 [i915] > [ 272.504930] intel_vgpu_mmio_reg_rw+0x24a/0x4c0 [i915] > [ 272.504944] intel_vgpu_emulate_mmio_write+0xac/0x240 [i915] > [ 272.504947] intel_vgpu_rw+0x22d/0x270 [kvmgt] > [ 272.504949] intel_vgpu_write+0x164/0x1f0 [kvmgt] > > GVT GEM context is created by i915_gem_context_create_gvt() which > doesn't allocate ppgtt. So GVT GEM context structure doesn't have > a valid i915_hw_ppgtt. > > This patch create ppgtt table at GVT GEM context creation, then assign > shadow ppgtt's root table address to this ppgtt when shadow ppgtt will > be used on GPU. So GVT GEM context has valid ppgtt address. But note > that this ppgtt only contain valid ppgtt root table address, the table > entry in this ppgtt structure are invalid. > > Fixes:4a3d3f6785be("drm/i915: Match code to comment and enforce ppgtt for execlists") > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com> Any more comment for this? We need it for current gvt broken on drm-tip, and it requires to change i915 for gvt ppgtt allocation, so I assume it's better to be merged by i915 directly, or do you like a gvt pull instead? Thanks. > --- > drivers/gpu/drm/i915/gvt/scheduler.c | 29 +++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_gem_context.c | 2 +- > 2 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c > index ea34003..b7e0529 100644 > --- a/drivers/gpu/drm/i915/gvt/scheduler.c > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c > @@ -334,6 +334,29 @@ static void release_shadow_wa_ctx(struct intel_shadow_wa_ctx *wa_ctx) > i915_gem_object_put(wa_ctx->indirect_ctx.obj); > } > > +static int set_context_ppgtt_from_shadow(struct intel_vgpu_workload *workload, > + struct i915_gem_context *ctx) > +{ > + struct intel_vgpu_mm *mm = workload->shadow_mm; > + struct i915_hw_ppgtt *ppgtt = ctx->ppgtt; > + int i = 0; > + > + if (mm->type != INTEL_GVT_MM_PPGTT || > + !mm->ppgtt_mm.shadowed) > + return -1; > + > + if (mm->ppgtt_mm.root_entry_type == GTT_TYPE_PPGTT_ROOT_L4_ENTRY) > + px_dma(&ppgtt->pml4) = mm->ppgtt_mm.shadow_pdps[0]; > + else { > + for (i = 0; i < GVT_RING_CTX_NR_PDPS; i++) { > + px_dma(ppgtt->pdp.page_directory[i]) = > + mm->ppgtt_mm.shadow_pdps[i]; > + } > + } > + > + return 0; > +} > + > /** > * intel_gvt_scan_and_shadow_workload - audit the workload by scanning and > * shadow it as well, include ringbuffer,wa_ctx and ctx. > @@ -358,6 +381,12 @@ int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload) > if (workload->req) > return 0; > > + ret = set_context_ppgtt_from_shadow(workload, shadow_ctx); > + if (ret < 0) { > + gvt_vgpu_err("workload shadow ppgtt isn't ready\n"); > + return ret; > + } > + > /* pin shadow context by gvt even the shadow context will be pinned > * when i915 alloc request. That is because gvt will update the guest > * context from shadow context when workload is completed, and at that > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 8cbe580..b97963d 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -457,7 +457,7 @@ i915_gem_context_create_gvt(struct drm_device *dev) > if (ret) > return ERR_PTR(ret); > > - ctx = __create_hw_context(to_i915(dev), NULL); > + ctx = i915_gem_create_context(to_i915(dev), NULL); > if (IS_ERR(ctx)) > goto out; > > -- > 2.7.4 >
Quoting Zhenyu Wang (2018-10-19 04:05:20) > On 2018.10.18 13:40:31 +0800, Xiong Zhang wrote: > > Currently the guest couldn't boot up under GVT-g environment as the > > following call trace exists: > > [ 272.504762] BUG: unable to handle kernel NULL pointer dereference at 0000000000000100 > > [ 272.504834] Call Trace: > > [ 272.504852] execlists_context_pin+0x2b2/0x520 [i915] > > [ 272.504869] intel_gvt_scan_and_shadow_workload+0x50/0x4d0 [i915] > > [ 272.504887] intel_vgpu_create_workload+0x3e2/0x570 [i915] > > [ 272.504901] intel_vgpu_submit_execlist+0xc0/0x2a0 [i915] > > [ 272.504916] elsp_mmio_write+0xc7/0x130 [i915] > > [ 272.504930] intel_vgpu_mmio_reg_rw+0x24a/0x4c0 [i915] > > [ 272.504944] intel_vgpu_emulate_mmio_write+0xac/0x240 [i915] > > [ 272.504947] intel_vgpu_rw+0x22d/0x270 [kvmgt] > > [ 272.504949] intel_vgpu_write+0x164/0x1f0 [kvmgt] > > > > GVT GEM context is created by i915_gem_context_create_gvt() which > > doesn't allocate ppgtt. So GVT GEM context structure doesn't have > > a valid i915_hw_ppgtt. > > > > This patch create ppgtt table at GVT GEM context creation, then assign > > shadow ppgtt's root table address to this ppgtt when shadow ppgtt will > > be used on GPU. So GVT GEM context has valid ppgtt address. But note > > that this ppgtt only contain valid ppgtt root table address, the table > > entry in this ppgtt structure are invalid. > > > > Fixes:4a3d3f6785be("drm/i915: Match code to comment and enforce ppgtt for execlists") > > > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > > Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com> > > Any more comment for this? We need it for current gvt broken on drm-tip, > and it requires to change i915 for gvt ppgtt allocation, so I assume it's > better to be merged by i915 directly, or do you like a gvt pull instead? You only needed ctx->ppgtt being set I thought, as you previously ignored the initial PD bits in the context image and overwrote the registers anyway. Do you want what appears to be a significant change to gvt itself to enter from i915? -Chris
> Quoting Zhenyu Wang (2018-10-19 04:05:20) > > On 2018.10.18 13:40:31 +0800, Xiong Zhang wrote: > > > Currently the guest couldn't boot up under GVT-g environment as the > > > following call trace exists: > > > [ 272.504762] BUG: unable to handle kernel NULL pointer dereference > > > at 0000000000000100 [ 272.504834] Call Trace: > > > [ 272.504852] execlists_context_pin+0x2b2/0x520 [i915] [ > > > 272.504869] intel_gvt_scan_and_shadow_workload+0x50/0x4d0 [i915] > [ > > > 272.504887] intel_vgpu_create_workload+0x3e2/0x570 [i915] [ > > > 272.504901] intel_vgpu_submit_execlist+0xc0/0x2a0 [i915] [ > > > 272.504916] elsp_mmio_write+0xc7/0x130 [i915] [ 272.504930] > > > intel_vgpu_mmio_reg_rw+0x24a/0x4c0 [i915] [ 272.504944] > > > intel_vgpu_emulate_mmio_write+0xac/0x240 [i915] [ 272.504947] > > > intel_vgpu_rw+0x22d/0x270 [kvmgt] [ 272.504949] > > > intel_vgpu_write+0x164/0x1f0 [kvmgt] > > > > > > GVT GEM context is created by i915_gem_context_create_gvt() which > > > doesn't allocate ppgtt. So GVT GEM context structure doesn't have a > > > valid i915_hw_ppgtt. > > > > > > This patch create ppgtt table at GVT GEM context creation, then > > > assign shadow ppgtt's root table address to this ppgtt when shadow > > > ppgtt will be used on GPU. So GVT GEM context has valid ppgtt > > > address. But note that this ppgtt only contain valid ppgtt root > > > table address, the table entry in this ppgtt structure are invalid. > > > > > > Fixes:4a3d3f6785be("drm/i915: Match code to comment and enforce > > > ppgtt for execlists") > > > > > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > > > Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com> > > > > Any more comment for this? We need it for current gvt broken on > > drm-tip, and it requires to change i915 for gvt ppgtt allocation, so I > > assume it's better to be merged by i915 directly, or do you like a gvt pull > instead? > > You only needed ctx->ppgtt being set I thought, as you previously ignored > the initial PD bits in the context image and overwrote the registers anyway. > > Do you want what appears to be a significant change to gvt itself to enter > from i915? [Zhang, Xiong Y] For 48 bit guest ppgtt, we only need ctx->ppgtt being set. But for 32 bit guest ppgtt, i915 call execlists_update_context_pdps() which is behind gvt pdp updates, if ctx->ppgtt isn't correct, 32bit ppgtt guest will be broken. thanks > -Chris
Quoting Zhang, Xiong Y (2018-10-19 11:11:23) > > Quoting Zhenyu Wang (2018-10-19 04:05:20) > > > On 2018.10.18 13:40:31 +0800, Xiong Zhang wrote: > > > > Currently the guest couldn't boot up under GVT-g environment as the > > > > following call trace exists: > > > > [ 272.504762] BUG: unable to handle kernel NULL pointer dereference > > > > at 0000000000000100 [ 272.504834] Call Trace: > > > > [ 272.504852] execlists_context_pin+0x2b2/0x520 [i915] [ > > > > 272.504869] intel_gvt_scan_and_shadow_workload+0x50/0x4d0 [i915] > > [ > > > > 272.504887] intel_vgpu_create_workload+0x3e2/0x570 [i915] [ > > > > 272.504901] intel_vgpu_submit_execlist+0xc0/0x2a0 [i915] [ > > > > 272.504916] elsp_mmio_write+0xc7/0x130 [i915] [ 272.504930] > > > > intel_vgpu_mmio_reg_rw+0x24a/0x4c0 [i915] [ 272.504944] > > > > intel_vgpu_emulate_mmio_write+0xac/0x240 [i915] [ 272.504947] > > > > intel_vgpu_rw+0x22d/0x270 [kvmgt] [ 272.504949] > > > > intel_vgpu_write+0x164/0x1f0 [kvmgt] > > > > > > > > GVT GEM context is created by i915_gem_context_create_gvt() which > > > > doesn't allocate ppgtt. So GVT GEM context structure doesn't have a > > > > valid i915_hw_ppgtt. > > > > > > > > This patch create ppgtt table at GVT GEM context creation, then > > > > assign shadow ppgtt's root table address to this ppgtt when shadow > > > > ppgtt will be used on GPU. So GVT GEM context has valid ppgtt > > > > address. But note that this ppgtt only contain valid ppgtt root > > > > table address, the table entry in this ppgtt structure are invalid. > > > > > > > > Fixes:4a3d3f6785be("drm/i915: Match code to comment and enforce > > > > ppgtt for execlists") > > > > > > > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > > > > Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com> > > > > > > Any more comment for this? We need it for current gvt broken on > > > drm-tip, and it requires to change i915 for gvt ppgtt allocation, so I > > > assume it's better to be merged by i915 directly, or do you like a gvt pull > > instead? > > > > You only needed ctx->ppgtt being set I thought, as you previously ignored > > the initial PD bits in the context image and overwrote the registers anyway. > > > > Do you want what appears to be a significant change to gvt itself to enter > > from i915? > [Zhang, Xiong Y] For 48 bit guest ppgtt, we only need ctx->ppgtt being set. > But for 32 bit guest ppgtt, i915 call execlists_update_context_pdps() which is behind gvt pdp updates, if ctx->ppgtt isn't correct, 32bit ppgtt guest will be broken. The code implies that gvt doesn't support 32b guests. -Chris
> Quoting Zhang, Xiong Y (2018-10-19 11:11:23) > > > Quoting Zhenyu Wang (2018-10-19 04:05:20) > > > > On 2018.10.18 13:40:31 +0800, Xiong Zhang wrote: > > > > > Currently the guest couldn't boot up under GVT-g environment as > > > > > the following call trace exists: > > > > > [ 272.504762] BUG: unable to handle kernel NULL pointer > > > > > dereference at 0000000000000100 [ 272.504834] Call Trace: > > > > > [ 272.504852] execlists_context_pin+0x2b2/0x520 [i915] [ > > > > > 272.504869] intel_gvt_scan_and_shadow_workload+0x50/0x4d0 > > > > > [i915] > > > [ > > > > > 272.504887] intel_vgpu_create_workload+0x3e2/0x570 [i915] [ > > > > > 272.504901] intel_vgpu_submit_execlist+0xc0/0x2a0 [i915] [ > > > > > 272.504916] elsp_mmio_write+0xc7/0x130 [i915] [ 272.504930] > > > > > intel_vgpu_mmio_reg_rw+0x24a/0x4c0 [i915] [ 272.504944] > > > > > intel_vgpu_emulate_mmio_write+0xac/0x240 [i915] [ 272.504947] > > > > > intel_vgpu_rw+0x22d/0x270 [kvmgt] [ 272.504949] > > > > > intel_vgpu_write+0x164/0x1f0 [kvmgt] > > > > > > > > > > GVT GEM context is created by i915_gem_context_create_gvt() > > > > > which doesn't allocate ppgtt. So GVT GEM context structure > > > > > doesn't have a valid i915_hw_ppgtt. > > > > > > > > > > This patch create ppgtt table at GVT GEM context creation, then > > > > > assign shadow ppgtt's root table address to this ppgtt when > > > > > shadow ppgtt will be used on GPU. So GVT GEM context has valid > > > > > ppgtt address. But note that this ppgtt only contain valid ppgtt > > > > > root table address, the table entry in this ppgtt structure are invalid. > > > > > > > > > > Fixes:4a3d3f6785be("drm/i915: Match code to comment and enforce > > > > > ppgtt for execlists") > > > > > > > > > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > > > > > Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com> > > > > > > > > Any more comment for this? We need it for current gvt broken on > > > > drm-tip, and it requires to change i915 for gvt ppgtt allocation, > > > > so I assume it's better to be merged by i915 directly, or do you > > > > like a gvt pull > > > instead? > > > > > > You only needed ctx->ppgtt being set I thought, as you previously > > > ignored the initial PD bits in the context image and overwrote the > registers anyway. > > > > > > Do you want what appears to be a significant change to gvt itself to > > > enter from i915? > > [Zhang, Xiong Y] For 48 bit guest ppgtt, we only need ctx->ppgtt being set. > > But for 32 bit guest ppgtt, i915 call execlists_update_context_pdps() which > is behind gvt pdp updates, if ctx->ppgtt isn't correct, 32bit ppgtt guest will be > broken. > > The code implies that gvt doesn't support 32b guests. [Zhang, Xiong Y] shadow ppgtt has some code to handle 32b ppgtt. Actually I didn't find any guest use 32b ppgtt, linux guest force to use 48 bit ppgtt, even 32 bit win7 driver use 48 bit also. And I asked others whether 32 bit should be supported or not, the answer is yes. So I added the code in gvt. > -Chris > _______________________________________________ > intel-gvt-dev mailing list > intel-gvt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
Quoting Xiong Zhang (2018-10-18 06:40:31) > Currently the guest couldn't boot up under GVT-g environment as the > following call trace exists: > [ 272.504762] BUG: unable to handle kernel NULL pointer dereference at 0000000000000100 > [ 272.504834] Call Trace: > [ 272.504852] execlists_context_pin+0x2b2/0x520 [i915] > [ 272.504869] intel_gvt_scan_and_shadow_workload+0x50/0x4d0 [i915] > [ 272.504887] intel_vgpu_create_workload+0x3e2/0x570 [i915] > [ 272.504901] intel_vgpu_submit_execlist+0xc0/0x2a0 [i915] > [ 272.504916] elsp_mmio_write+0xc7/0x130 [i915] > [ 272.504930] intel_vgpu_mmio_reg_rw+0x24a/0x4c0 [i915] > [ 272.504944] intel_vgpu_emulate_mmio_write+0xac/0x240 [i915] > [ 272.504947] intel_vgpu_rw+0x22d/0x270 [kvmgt] > [ 272.504949] intel_vgpu_write+0x164/0x1f0 [kvmgt] > > GVT GEM context is created by i915_gem_context_create_gvt() which > doesn't allocate ppgtt. So GVT GEM context structure doesn't have > a valid i915_hw_ppgtt. > > This patch create ppgtt table at GVT GEM context creation, then assign > shadow ppgtt's root table address to this ppgtt when shadow ppgtt will > be used on GPU. So GVT GEM context has valid ppgtt address. But note > that this ppgtt only contain valid ppgtt root table address, the table > entry in this ppgtt structure are invalid. > > Fixes:4a3d3f6785be("drm/i915: Match code to comment and enforce ppgtt for execlists") > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com> And pushed. Thanks very much for the fixup, -Chris
diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c index ea34003..b7e0529 100644 --- a/drivers/gpu/drm/i915/gvt/scheduler.c +++ b/drivers/gpu/drm/i915/gvt/scheduler.c @@ -334,6 +334,29 @@ static void release_shadow_wa_ctx(struct intel_shadow_wa_ctx *wa_ctx) i915_gem_object_put(wa_ctx->indirect_ctx.obj); } +static int set_context_ppgtt_from_shadow(struct intel_vgpu_workload *workload, + struct i915_gem_context *ctx) +{ + struct intel_vgpu_mm *mm = workload->shadow_mm; + struct i915_hw_ppgtt *ppgtt = ctx->ppgtt; + int i = 0; + + if (mm->type != INTEL_GVT_MM_PPGTT || + !mm->ppgtt_mm.shadowed) + return -1; + + if (mm->ppgtt_mm.root_entry_type == GTT_TYPE_PPGTT_ROOT_L4_ENTRY) + px_dma(&ppgtt->pml4) = mm->ppgtt_mm.shadow_pdps[0]; + else { + for (i = 0; i < GVT_RING_CTX_NR_PDPS; i++) { + px_dma(ppgtt->pdp.page_directory[i]) = + mm->ppgtt_mm.shadow_pdps[i]; + } + } + + return 0; +} + /** * intel_gvt_scan_and_shadow_workload - audit the workload by scanning and * shadow it as well, include ringbuffer,wa_ctx and ctx. @@ -358,6 +381,12 @@ int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload) if (workload->req) return 0; + ret = set_context_ppgtt_from_shadow(workload, shadow_ctx); + if (ret < 0) { + gvt_vgpu_err("workload shadow ppgtt isn't ready\n"); + return ret; + } + /* pin shadow context by gvt even the shadow context will be pinned * when i915 alloc request. That is because gvt will update the guest * context from shadow context when workload is completed, and at that diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 8cbe580..b97963d 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -457,7 +457,7 @@ i915_gem_context_create_gvt(struct drm_device *dev) if (ret) return ERR_PTR(ret); - ctx = __create_hw_context(to_i915(dev), NULL); + ctx = i915_gem_create_context(to_i915(dev), NULL); if (IS_ERR(ctx)) goto out;
Currently the guest couldn't boot up under GVT-g environment as the following call trace exists: [ 272.504762] BUG: unable to handle kernel NULL pointer dereference at 0000000000000100 [ 272.504834] Call Trace: [ 272.504852] execlists_context_pin+0x2b2/0x520 [i915] [ 272.504869] intel_gvt_scan_and_shadow_workload+0x50/0x4d0 [i915] [ 272.504887] intel_vgpu_create_workload+0x3e2/0x570 [i915] [ 272.504901] intel_vgpu_submit_execlist+0xc0/0x2a0 [i915] [ 272.504916] elsp_mmio_write+0xc7/0x130 [i915] [ 272.504930] intel_vgpu_mmio_reg_rw+0x24a/0x4c0 [i915] [ 272.504944] intel_vgpu_emulate_mmio_write+0xac/0x240 [i915] [ 272.504947] intel_vgpu_rw+0x22d/0x270 [kvmgt] [ 272.504949] intel_vgpu_write+0x164/0x1f0 [kvmgt] GVT GEM context is created by i915_gem_context_create_gvt() which doesn't allocate ppgtt. So GVT GEM context structure doesn't have a valid i915_hw_ppgtt. This patch create ppgtt table at GVT GEM context creation, then assign shadow ppgtt's root table address to this ppgtt when shadow ppgtt will be used on GPU. So GVT GEM context has valid ppgtt address. But note that this ppgtt only contain valid ppgtt root table address, the table entry in this ppgtt structure are invalid. Fixes:4a3d3f6785be("drm/i915: Match code to comment and enforce ppgtt for execlists") Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com> --- drivers/gpu/drm/i915/gvt/scheduler.c | 29 +++++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_gem_context.c | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-)