[PATCH] drm/i915/gvt: support inconsecutive partial PTE update
Hang Yuan
hang.yuan at linux.intel.com
Mon Sep 17 05:06:27 UTC 2018
On 09/17/2018 10:58 AM, Zhenyu Wang wrote:
> On 2018.09.14 19:08:25 +0800, hang.yuan at linux.intel.com wrote:
>> From: Hang Yuan <hang.yuan at linux.intel.com>
>>
>> Previously we assumed two 4-byte writes to the same PTE coming in sequence.
>> But recently we observed inconsecutive partial write happening as well. So
>> this patch enhances the previous solution. It now uses a list to save more
>> partial writes. If one partial write can be combined with another one in
>> the list to construct a full PTE, update its shadow entry. Otherwise, save
>> the partial write in the list.
>>
>> This patch also update ggtt_mmio_write more to remove unnecessary virtual
>> PTE read and add old shadow PTE invalidation on PTE update case.
>>
>> v2: invalidate old entry and flush ggtt (Zhenyu)
>>
>> Signed-off-by: Hang Yuan <hang.yuan at linux.intel.com>
>> Cc: Yan Zhao <yan.y.zhao at intel.com>
>> Cc: Xiaolin Zhang <xiaolin.zhang at intel.com>
>> Cc: Zhenyu Wang <zhenyu.z.wang at intel.com>
>> ---
>> drivers/gpu/drm/i915/gvt/gtt.c | 119 +++++++++++++++++++++--------------------
>> drivers/gpu/drm/i915/gvt/gtt.h | 9 +++-
>> 2 files changed, 69 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
>> index 2402395..cf11f8f 100644
>> --- a/drivers/gpu/drm/i915/gvt/gtt.c
>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>> @@ -1905,7 +1905,6 @@ static struct intel_vgpu_mm *intel_vgpu_create_ggtt_mm(struct intel_vgpu *vgpu)
>> vgpu_free_mm(mm);
>> return ERR_PTR(-ENOMEM);
>> }
>> - mm->ggtt_mm.last_partial_off = -1UL;
>>
>> return mm;
>> }
>> @@ -1930,7 +1929,6 @@ void _intel_vgpu_mm_release(struct kref *mm_ref)
>> invalidate_ppgtt_mm(mm);
>> } else {
>> vfree(mm->ggtt_mm.virtual_ggtt);
>> - mm->ggtt_mm.last_partial_off = -1UL;
>> }
>>
>> vgpu_free_mm(mm);
>> @@ -2168,6 +2166,8 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>> struct intel_gvt_gtt_entry e, m;
>> dma_addr_t dma_addr;
>> int ret;
>> + struct intel_gvt_partial_pte *partial_pte, *pos, *n;
>> + bool partial_update = false;
>>
>> if (bytes != 4 && bytes != 8)
>> return -EINVAL;
>> @@ -2178,68 +2178,61 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>> if (!vgpu_gmadr_is_valid(vgpu, gma))
>> return 0;
>>
>> - ggtt_get_guest_entry(ggtt_mm, &e, g_gtt_index);
>> -
>> + e.type = GTT_TYPE_GGTT_PTE;
>> memcpy((void *)&e.val64 + (off & (info->gtt_entry_size - 1)), p_data,
>> bytes);
>>
>> /* If ggtt entry size is 8 bytes, and it's split into two 4 bytes
>> - * write, we assume the two 4 bytes writes are consecutive.
>> - * Otherwise, we abort and report error
>> + * write, save the first 4 bytes in a list and update virtual
>> + * PTE. Only update shadow PTE when the second 4 bytes comes.
>> */
>> if (bytes < info->gtt_entry_size) {
>> - if (ggtt_mm->ggtt_mm.last_partial_off == -1UL) {
>> - /* the first partial part*/
>> - ggtt_mm->ggtt_mm.last_partial_off = off;
>> - ggtt_mm->ggtt_mm.last_partial_data = e.val64;
>> - return 0;
>> - } else if ((g_gtt_index ==
>> - (ggtt_mm->ggtt_mm.last_partial_off >>
>> - info->gtt_entry_size_shift)) &&
>> - (off != ggtt_mm->ggtt_mm.last_partial_off)) {
>> - /* the second partial part */
>> -
>> - int last_off = ggtt_mm->ggtt_mm.last_partial_off &
>> - (info->gtt_entry_size - 1);
>> -
>> - memcpy((void *)&e.val64 + last_off,
>> - (void *)&ggtt_mm->ggtt_mm.last_partial_data +
>> - last_off, bytes);
>> -
>> - ggtt_mm->ggtt_mm.last_partial_off = -1UL;
>> - } else {
>> - int last_offset;
>> -
>> - gvt_vgpu_err("failed to populate guest ggtt entry: abnormal ggtt entry write sequence, last_partial_off=%lx, offset=%x, bytes=%d, ggtt entry size=%d\n",
>> - ggtt_mm->ggtt_mm.last_partial_off, off,
>> - bytes, info->gtt_entry_size);
>> -
>> - /* set host ggtt entry to scratch page and clear
>> - * virtual ggtt entry as not present for last
>> - * partially write offset
>> - */
>> - last_offset = ggtt_mm->ggtt_mm.last_partial_off &
>> - (~(info->gtt_entry_size - 1));
>> -
>> - ggtt_get_host_entry(ggtt_mm, &m, last_offset);
>> - ggtt_invalidate_pte(vgpu, &m);
>> - ops->set_pfn(&m, gvt->gtt.scratch_mfn);
>> - ops->clear_present(&m);
>> - ggtt_set_host_entry(ggtt_mm, &m, last_offset);
>> - ggtt_invalidate(gvt->dev_priv);
>> -
>> - ggtt_get_guest_entry(ggtt_mm, &e, last_offset);
>> - ops->clear_present(&e);
>> - ggtt_set_guest_entry(ggtt_mm, &e, last_offset);
>> -
>> - ggtt_mm->ggtt_mm.last_partial_off = off;
>> - ggtt_mm->ggtt_mm.last_partial_data = e.val64;
>> + bool found = false;
>> +
>> + list_for_each_entry_safe(pos, n,
>> + &ggtt_mm->ggtt_mm.partial_pte_list, list) {
>> + if (g_gtt_index == pos->offset >>
>> + info->gtt_entry_size_shift) {
>> + if (off != pos->offset) {
>> + /* the second partial part*/
>> +
>> + int last_off = pos->offset &
>> + (info->gtt_entry_size - 1);
>> +
>> + memcpy((void *)&e.val64 + last_off,
>> + (void *)&pos->data + last_off,
>> + bytes);
>> +
>> + list_del(&pos->list);
>> + kfree(pos);
>> + } else {
>> + /* update of the first partial part */
>> +
>> + pos->data = e.val64;
>> + ggtt_set_guest_entry(ggtt_mm, &e,
>> + g_gtt_index);
>> + return 0;
>> + }
>> + found = true;
>> + break;
>
> Move this two lines in above if (off != pos->offset) is easier to read.
Ok.
>> + }
>> + }
>>
>> - return 0;
>> + if (!found) {
>> + /* the first partial part */
>> +
>> + partial_pte = kzalloc(sizeof(*partial_pte), GFP_KERNEL);
>> + if (!partial_pte)
>> + return -ENOMEM;
>> + partial_pte->offset = off;
>> + partial_pte->data = e.val64;
>> + list_add_tail(&partial_pte->list,
>> + &ggtt_mm->ggtt_mm.partial_pte_list);
>> + partial_update = true;
>> }
>> }
>>
>> - if (ops->test_present(&e)) {
>> + if (!partial_update && (ops->test_present(&e))) {
>> gfn = ops->get_pfn(&e);
>> m = e;
>>
>> @@ -2263,16 +2256,18 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>> } else
>> ops->set_pfn(&m, dma_addr >> PAGE_SHIFT);
>> } else {
>> - ggtt_get_host_entry(ggtt_mm, &m, g_gtt_index);
>> - ggtt_invalidate_pte(vgpu, &m);
>> ops->set_pfn(&m, gvt->gtt.scratch_mfn);
>> ops->clear_present(&m);
>> }
>>
>> out:
>> + ggtt_set_guest_entry(ggtt_mm, &e, g_gtt_index);
>> +
>> + ggtt_get_host_entry(ggtt_mm, &e, g_gtt_index);
>> + ggtt_invalidate_pte(vgpu, &e);
>
> For normal 8b size update of ggtt entry, does this invalidate of old pte page
> always required? If yes, looks better split this as a separate fix instead.
Yes. If old PTE is present, it will be invalidated. I will split the
patch. Thanks for the review.
>> +
>> ggtt_set_host_entry(ggtt_mm, &m, g_gtt_index);
>> ggtt_invalidate(gvt->dev_priv);
>> - ggtt_set_guest_entry(ggtt_mm, &e, g_gtt_index);
>> return 0;
>> }
>>
>> @@ -2430,6 +2425,8 @@ int intel_vgpu_init_gtt(struct intel_vgpu *vgpu)
>>
>> intel_vgpu_reset_ggtt(vgpu, false);
>>
>> + INIT_LIST_HEAD(>t->ggtt_mm->ggtt_mm.partial_pte_list);
>> +
>> return create_scratch_page_tree(vgpu);
>> }
>>
>> @@ -2454,6 +2451,14 @@ static void intel_vgpu_destroy_all_ppgtt_mm(struct intel_vgpu *vgpu)
>>
>> static void intel_vgpu_destroy_ggtt_mm(struct intel_vgpu *vgpu)
>> {
>> + struct intel_gvt_partial_pte *pos;
>> +
>> + list_for_each_entry(pos,
>> + &vgpu->gtt.ggtt_mm->ggtt_mm.partial_pte_list, list) {
>> + gvt_dbg_mm("partial PTE update on hold 0x%lx : 0x%llx\n",
>> + pos->offset, pos->data);
>> + kfree(pos);
>> + }
>> intel_vgpu_destroy_mm(vgpu->gtt.ggtt_mm);
>> vgpu->gtt.ggtt_mm = NULL;
>> }
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.h b/drivers/gpu/drm/i915/gvt/gtt.h
>> index 7a9b361..a11bfee 100644
>> --- a/drivers/gpu/drm/i915/gvt/gtt.h
>> +++ b/drivers/gpu/drm/i915/gvt/gtt.h
>> @@ -133,6 +133,12 @@ enum intel_gvt_mm_type {
>>
>> #define GVT_RING_CTX_NR_PDPS GEN8_3LVL_PDPES
>>
>> +struct intel_gvt_partial_pte {
>> + unsigned long offset;
>> + u64 data;
>> + struct list_head list;
>> +};
>> +
>> struct intel_vgpu_mm {
>> enum intel_gvt_mm_type type;
>> struct intel_vgpu *vgpu;
>> @@ -157,8 +163,7 @@ struct intel_vgpu_mm {
>> } ppgtt_mm;
>> struct {
>> void *virtual_ggtt;
>> - unsigned long last_partial_off;
>> - u64 last_partial_data;
>> + struct list_head partial_pte_list;
>> } ggtt_mm;
>> };
>> };
>> --
>> 2.7.4
>>
>
More information about the intel-gvt-dev
mailing list