Re: [PATCH v3 2/8] drm/i915: Adds graphic address space ballooning logic

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 11/13/2014 12:02 PM, Yu Zhang wrote:
> With Intel GVT-g, the global graphic memory space is partitioned by
> multiple vGPU instances in different VMs. The ballooning code is called
> in i915_gem_setup_global_gtt(), utilizing the drm mm allocator APIs to
> mark the graphic address space which are partitioned out to other vGPUs
> as reserved.
>
> v2:
> take Chris and Daniel's comments:
> 	- no guard page between different VMs
> 	- use drm_mm_reserve_node() to do the reservation for ballooning,
> 	instead of the previous drm_mm_insert_node_in_range_generic()
>
> v3:
> take Daniel's comments:
> 	- move ballooning functions into i915_vgpu.c
> 	- add kerneldoc to ballooning functions
>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c |  17 +++-
>   drivers/gpu/drm/i915/i915_vgpu.c    | 149 ++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_vgpu.h    |   2 +
>   3 files changed, 165 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index de12017..2dfac13 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -27,6 +27,7 @@
>   #include <drm/drmP.h>
>   #include <drm/i915_drm.h>
>   #include "i915_drv.h"
> +#include "i915_vgpu.h"
>   #include "i915_trace.h"
>   #include "intel_drv.h"
>
> @@ -1683,6 +1684,16 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
>
>   	/* Subtract the guard page ... */
>   	drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
> +
> +	dev_priv->gtt.base.start = start;
> +	dev_priv->gtt.base.total = end - start;
> +
> +	if (intel_vgpu_active(dev)) {
> +		ret = intel_vgt_balloon(dev);
> +		if (ret)
> +			return ret;
> +	}
> +

Out of curiosity, what will be the mechanism to prevent a vGPU instance 
from ignoring the ballooning data? Must be something in the hypervisor 
blocking pass-through access to such domains?

And probably GPU reset should also be disallowed for vGPU instances?

>   	if (!HAS_LLC(dev))
>   		dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
>
> @@ -1702,9 +1713,6 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
>   		vma->bound |= GLOBAL_BIND;
>   	}
>
> -	dev_priv->gtt.base.start = start;
> -	dev_priv->gtt.base.total = end - start;
> -
>   	/* Clear any non-preallocated blocks */
>   	drm_mm_for_each_hole(entry, &ggtt_vm->mm, hole_start, hole_end) {
>   		DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
> @@ -1756,6 +1764,9 @@ void i915_global_gtt_cleanup(struct drm_device *dev)
>   	}
>
>   	if (drm_mm_initialized(&vm->mm)) {
> +		if (intel_vgpu_active(dev))
> +			intel_vgt_deballoon();
> +
>   		drm_mm_takedown(&vm->mm);
>   		list_del(&vm->global_link);
>   	}
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
> index 3f6b797..ff5fba3 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.c
> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> @@ -83,3 +83,152 @@ void i915_check_vgpu(struct drm_device *dev)
>   	dev_priv->vgpu.active = true;
>   	DRM_INFO("Virtual GPU for Intel GVT-g detected.\n");
>   }
> +
> +struct _balloon_info_ {
> +	/*
> +	 * There are up to 2 regions per low/high graphic memory that
> +	 * might be ballooned. Here, index 0/1 is for low
> +	 * graphic memory, 2/3 for high graphic memory.
> +	 */
> +	struct drm_mm_node space[4];
> +} bl_info;

This should be static I think.

> +/**
> + * intel_vgt_deballoon - deballoon reserved graphics address trunks
> + *
> + * This function is called to deallocate the ballooned-out graphic memory, when
> + * driver is unloaded or when ballooning fails.
> + */
> +void intel_vgt_deballoon(void)
> +{
> +	int i;
> +
> +	DRM_INFO("VGT deballoon.\n");

Would debug be more appropriate? I don't see much value of saying this 
on driver unload - it's not that it is optional at this point.

Also for all logging, is intended human readable name VGT or vGT? If the 
latter it would be nicer to log it in that form.

> +
> +	for (i = 0; i < 4; i++) {
> +		if (bl_info.space[i].allocated)
> +			drm_mm_remove_node(&bl_info.space[i]);
> +	}
> +
> +	memset(&bl_info, 0, sizeof(bl_info));
> +}
> +
> +static int vgt_balloon_space(struct drm_mm *mm,
> +			     struct drm_mm_node *node,
> +			     unsigned long start, unsigned long end)
> +{
> +	unsigned long size = end - start;
> +
> +	if (start == end)
> +		return -EINVAL;
> +
> +	DRM_INFO("balloon space: range [ 0x%lx - 0x%lx ] %lu KB.\n",
> +		 start, end, size / 1024);

KiB ?

> +	node->start = start;
> +	node->size = size;
> +
> +	return drm_mm_reserve_node(mm, node);
> +}
> +
> +/**
> + * intel_vgt_balloon - balloon out reserved graphics address trunks
> + * @dev: drm device
> + *
> + * This function is called at the initialization stage, to balloon out the
> + * graphic address space allocated to other VMs, by marking these spaces as
> + * reserved.
> + *
> + * The ballooning related knowledges(starting address and size of the low/high

s/knowledges\(/knowledge /

> + * graphic memory) are depicted in the vgt_if structure in a reserved MMIO
> + * range.
> + *
> + * Returns:
> + * zero on success, non-zero if configuration invalid or ballooning failed
> + */
> +int intel_vgt_balloon(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct i915_address_space *ggtt_vm = &dev_priv->gtt.base;
> +	unsigned long ggtt_vm_end = ggtt_vm->start + ggtt_vm->total;
> +
> +	unsigned long low_gm_base, low_gm_size, low_gm_end;
> +	unsigned long high_gm_base, high_gm_size, high_gm_end;
> +	int ret;
> +
> +	low_gm_base = I915_READ(vgtif_reg(avail_rs.low_gmadr.my_base));
> +	low_gm_size = I915_READ(vgtif_reg(avail_rs.low_gmadr.my_size));
> +	high_gm_base = I915_READ(vgtif_reg(avail_rs.high_gmadr.my_base));
> +	high_gm_size = I915_READ(vgtif_reg(avail_rs.high_gmadr.my_size));

Get rid of my_ prefix ?

> +
> +	low_gm_end = low_gm_base + low_gm_size;
> +	high_gm_end = high_gm_base + high_gm_size;
> +
> +	DRM_INFO("VGT ballooning configuration:\n");
> +	DRM_INFO("Low graphic memory: base 0x%lx size %ldKB\n",
> +		 low_gm_base, low_gm_size / 1024);
> +	DRM_INFO("High graphic memory: base 0x%lx size %ldKB\n",
> +		 high_gm_base, high_gm_size / 1024);
> +
> +	if (low_gm_base < ggtt_vm->start
> +	    || low_gm_end > dev_priv->gtt.mappable_end
> +	    || high_gm_base < dev_priv->gtt.mappable_end
> +	    || high_gm_end > ggtt_vm_end) {
> +		DRM_ERROR("Invalid ballooning configuration!\n");
> +		return -EINVAL;
> +	}
> +
> +	memset(&bl_info, 0, sizeof(bl_info));

If bl_info is static then you don't need this memset?

> +	/* High graphic memory ballooning */
> +	if (high_gm_base > dev_priv->gtt.mappable_end) {
> +		ret = vgt_balloon_space(&ggtt_vm->mm,
> +					&bl_info.space[2],
> +					dev_priv->gtt.mappable_end,
> +					high_gm_base);
> +
> +		if (ret)
> +			goto err;
> +	}
> +
> +	/*
> +	 * No need to partition out the last physical page,
> +	 * because it is reserved to the guard page.
> +	 */
> +	if (high_gm_end < ggtt_vm_end - PAGE_SIZE) {
> +		ret = vgt_balloon_space(&ggtt_vm->mm,
> +					&bl_info.space[3],
> +					high_gm_end,
> +					ggtt_vm_end - PAGE_SIZE);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	/* Low graphic memory ballooning */
> +	if (low_gm_base > ggtt_vm->start) {
> +		ret = vgt_balloon_space(&ggtt_vm->mm,
> +					&bl_info.space[0],
> +					ggtt_vm->start, low_gm_base);
> +
> +		if (ret)
> +			goto err;
> +	}
> +
> +	if (low_gm_end < dev_priv->gtt.mappable_end) {
> +		ret = vgt_balloon_space(&ggtt_vm->mm,
> +					&bl_info.space[1],
> +					low_gm_end,
> +					dev_priv->gtt.mappable_end);
> +
> +		if (ret)
> +			goto err;
> +	}

Okay, I've figured it out. :) I suppose going back to patch 1, where it 
says "Each VM can only have one portion of continuous area for now", 
with the emphasis on _one_. That threw me off thinking you have two 
"balloons" meaning forbidden areas. And then with low and high naming I 
got the wrong idea that one balloon marks the bottom inaccessible part, 
and the other top. I didn't figure out the whole low == mappable, high 
== non-mappable split. I suppose it was my inexperience, but if you can 
think of a way on how to improve the comment, even ASCII art would be 
very nice if possible.

Actually ballooning in the first place confuses me because I thought 
prior use for that in virtualisation was for memory which can shrink and 
grow at runtime. Did I get that wrong?

> +	DRM_INFO("VGT balloon successfully\n");
> +	return 0;
> +
> +err:
> +	DRM_ERROR("VGT balloon fail\n");
> +	intel_vgt_deballoon();
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
> index 5f41d01c..f538b18 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.h
> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
> @@ -81,5 +81,7 @@ struct vgt_if {
>   	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>
>   extern void i915_check_vgpu(struct drm_device *dev);
> +extern int intel_vgt_balloon(struct drm_device *dev);
> +extern void intel_vgt_deballoon(void);
>
>   #endif /* _I915_VGPU_H_ */
>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx