drm/i915: Don't deballoon unused ggtt drm_mm_node in linux guest

Submitted by Xiong Zhang on March 29, 2018, 10:58 a.m.

Details

Message ID 1522321121-17533-1-git-send-email-xiong.y.zhang@intel.com
State New
Headers show
Series "drm/i915: Don't deballoon unused ggtt drm_mm_node in linux guest" ( rev: 1 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

Xiong Zhang March 29, 2018, 10:58 a.m.
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(+)

Patch hide | download patch | download mbox

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,

Comments

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.