[PATCH v2] drm/i915/gvt: fix a bug of partially write ggtt enties
Zhao, Yan Y
yan.y.zhao at intel.com
Fri Jun 22 00:28:05 UTC 2018
hi zhenyu
On 6/20/2018 3:05 PM, Zhenyu Wang wrote:
> On 2018.06.19 15:44:11 +0800, Zhao Yan wrote:
>> when guest writes ggtt entries, it could write 8 bytes a time if
>> gtt_entry_size is 8. But, qemu could split the 8 bytes into 2 consecutive
>> 4-byte writes.
>>
>> If each 4-byte partial write could trigger a host ggtt write, it is very
>> possible that a wrong combination is written to the host ggtt. E.g.
>> the higher 4 bytes is the old value, but the lower 4 bytes is the new
>> value, and this 8-byte combination is wrong but written to the ggtt, thus
>> causing bugs.
>>
>> To handle this condition, we just record the first 4-byte write, then wait
>> until the second 4-byte write comes and write the combined 64-bit data to
>> host ggtt table.
>>
>> To save memory space and to spot partial write as early as possible, we
>> don't keep this information for every ggtt index. Instread, we just record
>> the last ggtt write position, and assume the two 4-byte writes come in
>> consecutively for each vgpu.
>>
>> This assumption is right based on the characteristic of ggtt entry which
>> stores memory address. When gtt_entry_size is 8, the guest memory physical
>> address should be 64 bits, so any sane guest driver should write 8-byte
>> long data at a time, so 2 consecutive 4-byte writes at the same ggtt index
>> should be trapped in gvt.
>>
>> v2:
>> when incomplete ggtt entry write is located, e.g.
>> 1. guest only writes 4 bytes at a ggtt offset and no long writes the
>> rest 4 bytes.
>> 2. guest writes 4 bytes of a ggtt offset, then write at other ggtt
>> offsets, then return back to write the left 4 bytes of the first
>> ggtt offset.
>> add error handling logic to remap host entry to scratch page, and mark
>> guest virtual ggtt entry as not present. (zhenyu wang)
>>
>> Signed-off-by: Zhao Yan <yan.y.zhao at intel.com>
>> ---
>> drivers/gpu/drm/i915/gvt/gtt.c | 58 ++++++++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/gvt/gtt.h | 2 ++
>> 2 files changed, 60 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
>> index 78e55aa..b4d8a7d 100644
>> --- a/drivers/gpu/drm/i915/gvt/gtt.c
>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>> @@ -1591,6 +1591,7 @@ 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;
>> }
>> @@ -1615,6 +1616,7 @@ 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);
>> @@ -1867,6 +1869,62 @@ static int emulate_ggtt_mmio_write(struct intel_vgpu *vgpu, unsigned int off,
>> 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
>> + */
>> + 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);
> As have set to scratch page, still need to clear present on host?
yes, you are right, either setting to scratch page or clearing present
is actually enough.
But I still added "clear_present" here, based on two reasons:
1. aligning present status of host entry and guest entry (as I will
clear_present of guest entry immediately)
2. keeping consistence with existing code which also clear_present of a
entry to scratch page when a non-present guest entry is found.
So, do you think it's ok?
>> + 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;
>> +
>> + return 0;
>> + }
>> + }
>> +
>> if (ops->test_present(&e)) {
>> gfn = ops->get_pfn(&e);
>> m = e;
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.h b/drivers/gpu/drm/i915/gvt/gtt.h
>> index 3792f2b..97e6264 100644
>> --- a/drivers/gpu/drm/i915/gvt/gtt.h
>> +++ b/drivers/gpu/drm/i915/gvt/gtt.h
>> @@ -150,6 +150,8 @@ struct intel_vgpu_mm {
>> } ppgtt_mm;
>> struct {
>> void *virtual_ggtt;
>> + unsigned long last_partial_off;
>> + u64 last_partial_data;
>> } ggtt_mm;
>> };
>> };
>> --
>> 1.9.1
>>
>> _______________________________________________
>> intel-gvt-dev mailing list
>> intel-gvt-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>
>
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20180622/1aeff214/attachment.html>
More information about the intel-gvt-dev
mailing list