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

["Yu, Zhang" <yu.c.zhang@linux.intel.com>]

On 12/12/2014 9:00 PM, Tvrtko Ursulin wrote:
>
> 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?
Well, although we have range check logic in the host side(which checks 
the legality of a GM address), the correctness of a GM from vGPU side 
are supposed to be guaranteed by the drm mm allocator - all those 
ballooned out spaces are marked as reserved.
>
> 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.
Yes, you are right.
>
>> +/**
>> + * 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.
OK. :)
>
> 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.
Well, I prefer VGT. :)
>
>> +
>> +    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 ?
Yes. Thanks. :)
>
>> +    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 /
Got it.
>
>> + * 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 ?
OK. Will do. :)
>
>> +
>> +    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?
Oh, yes. :)
>
>> +    /* 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.
Hah. Guess I need a better comments above. Let me think how to organize.  :)
>
> 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?
>
Well, for memory virtualization, the word 'ballooning' does mean this - 
it can shrink and grow.
Yet for our Intel GVT-g, the 'ballooning' is used, to emphasis that we 
are not just partitioning the GM space to each vgpu, the vgpu can also 
perceive its GM address space as starting from zero(although the usable 
GM spaces may probably not).
By now, we do not grow/shrink. :)

>> +    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