[PATCH] drm/i915/gvt: support inconsecutive partial PTE update

Hang Yuan hang.yuan at linux.intel.com
Mon Sep 17 10:53:16 UTC 2018


On 09/17/2018 01:06 PM, intel-gvt-dev-bounces at lists.freedesktop.org wrote:
> 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.
Recalled there is coding style warning if move 'break' in 'if' branch 
and follow by 'else' branch. So have the two lines after 'else' branch.



More information about the intel-gvt-dev mailing list