| Message ID | 1522321121-17533-1-git-send-email-xiong.y.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Xiong Zhang (2018-03-29 11:58:41) > Four drm_mm_node are used to reserve guest ggtt space, but some of them > may aren't initialized and used in intel_vgt_balloon(), so these unused may be skipped and not initialised due to space constraints, > drm_mm_node couldn't be removed through drm_mm_remove_node(). > > Fixes: ff8f797557c7("drm/i915: return the correct usable aperture size under gvt environment") > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> Had to read through and work out what the problem was; whether it was a bug elsewhere in vgpu or deliberate, so amending the commit msg to be clear would be helpful. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Who actually consumes bl_info? It just looks like being a balloon being set adrift. -Chris
> Quoting Xiong Zhang (2018-03-29 11:58:41) > > Four drm_mm_node are used to reserve guest ggtt space, but some of > > them may aren't initialized and used in intel_vgt_balloon(), so these > > unused > > may be skipped and not initialised due to space constraints, [Zhang, Xiong Y] OK, I will apply it. thanks > > > drm_mm_node couldn't be removed through drm_mm_remove_node(). > > > > Fixes: ff8f797557c7("drm/i915: return the correct usable aperture size > > under gvt environment") > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > > Had to read through and work out what the problem was; whether it was a > bug elsewhere in vgpu or deliberate, so amending the commit msg to be > clear would be helpful. [Zhang, Xiong Y] bl_info.space[i] is initialized only on specific condition in intel_vgt_balloon(). When guest i915 driver is unloaded, intel_vgt_deballon() go through all four bl_info.space[3:0] and call drm_mm_remove_node() for each. If one isn't initialized, warning and reference null pointer occur in drm_mm_remove_node(). I will update commit message. Thanks. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > Who actually consumes bl_info? It just looks like being a balloon being set > adrift. [Zhang, Xiong Y] bl_info is internal static variable, only intel_vgt_deballoon() consume it at driver unload, it deballoon reserved ggtt space. > -Chris
+ Zhi and Zhenyu Quoting Xiong Zhang (2018-03-29 13:58:41) > Four drm_mm_node are used to reserve guest ggtt space, but some of them > may aren't initialized and used in intel_vgt_balloon(), so these unused > drm_mm_node couldn't be removed through drm_mm_remove_node(). I'm not sure how this slipped by in previous review, but is there an explanation why we have; static struct _balloon_info_ bl_info; ... and are not even initializing it? This should definitely find it's way into dev_priv and be properly initialized. Regards, Joonas > > Fixes: ff8f797557c7("drm/i915: return the correct usable aperture size under gvt environment") > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > --- > drivers/gpu/drm/i915/i915_vgpu.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c > index 5fe9f3f..7545686 100644 > --- a/drivers/gpu/drm/i915/i915_vgpu.c > +++ b/drivers/gpu/drm/i915/i915_vgpu.c > @@ -100,6 +100,9 @@ static struct _balloon_info_ bl_info; > static void vgt_deballoon_space(struct i915_ggtt *ggtt, > struct drm_mm_node *node) > { > + if (!node->allocated) > + return; > + > DRM_DEBUG_DRIVER("deballoon space: range [0x%llx - 0x%llx] %llu KiB.\n", > node->start, > node->start + node->size, > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> + Zhi and Zhenyu > > Quoting Xiong Zhang (2018-03-29 13:58:41) > > Four drm_mm_node are used to reserve guest ggtt space, but some of > > them may aren't initialized and used in intel_vgt_balloon(), so these > > unused drm_mm_node couldn't be removed through > drm_mm_remove_node(). > > I'm not sure how this slipped by in previous review, but is there an > explanation why we have; > > static struct _balloon_info_ bl_info; > > ... and are not even initializing it? > > This should definitely find it's way into dev_priv and be properly initialized. [Zhang, Xiong Y] how about move bl_info into struct i915_virtual_gpu ? so that we could get it through dev_priv->vgpu.bl_info thanks > > Regards, Joonas > > > > > Fixes: ff8f797557c7("drm/i915: return the correct usable aperture size > > under gvt environment") > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > > --- > > drivers/gpu/drm/i915/i915_vgpu.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_vgpu.c > > b/drivers/gpu/drm/i915/i915_vgpu.c > > index 5fe9f3f..7545686 100644 > > --- a/drivers/gpu/drm/i915/i915_vgpu.c > > +++ b/drivers/gpu/drm/i915/i915_vgpu.c > > @@ -100,6 +100,9 @@ static struct _balloon_info_ bl_info; static void > > vgt_deballoon_space(struct i915_ggtt *ggtt, > > struct drm_mm_node *node) { > > + if (!node->allocated) > > + return; > > + > > DRM_DEBUG_DRIVER("deballoon space: range [0x%llx - > 0x%llx] %llu KiB.\n", > > node->start, > > node->start + node->size, > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > intel-gvt-dev mailing list > intel-gvt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
On 2018.03.30 07:01:02 +0000, Zhang, Xiong Y wrote: > > + Zhi and Zhenyu > > > > Quoting Xiong Zhang (2018-03-29 13:58:41) > > > Four drm_mm_node are used to reserve guest ggtt space, but some of > > > them may aren't initialized and used in intel_vgt_balloon(), so these > > > unused drm_mm_node couldn't be removed through > > drm_mm_remove_node(). > > > > I'm not sure how this slipped by in previous review, but is there an > > explanation why we have; > > > > static struct _balloon_info_ bl_info; > > > > ... and are not even initializing it? > > > > This should definitely find it's way into dev_priv and be properly initialized. > [Zhang, Xiong Y] how about move bl_info into struct i915_virtual_gpu ? so that we could get it through dev_priv->vgpu.bl_info > yep, seems fine to me.
diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c index 5fe9f3f..7545686 100644 --- a/drivers/gpu/drm/i915/i915_vgpu.c +++ b/drivers/gpu/drm/i915/i915_vgpu.c @@ -100,6 +100,9 @@ static struct _balloon_info_ bl_info; static void vgt_deballoon_space(struct i915_ggtt *ggtt, struct drm_mm_node *node) { + if (!node->allocated) + return; + DRM_DEBUG_DRIVER("deballoon space: range [0x%llx - 0x%llx] %llu KiB.\n", node->start, node->start + node->size,
Four drm_mm_node are used to reserve guest ggtt space, but some of them may aren't initialized and used in intel_vgt_balloon(), so these unused drm_mm_node couldn't be removed through drm_mm_remove_node(). Fixes: ff8f797557c7("drm/i915: return the correct usable aperture size under gvt environment") Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> --- drivers/gpu/drm/i915/i915_vgpu.c | 3 +++ 1 file changed, 3 insertions(+)