[PATCH v2] drm/i915/gvt: fix a bug of partially write ggtt enties
Zhenyu Wang
zhenyuw at linux.intel.com
Tue Jun 26 02:05:13 UTC 2018
On 2018.06.22 08:28:05 +0800, Zhao, Yan Y wrote:
> 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?
yep, and applied this to fixes.
Thanks!
>
>
> + 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
>
>
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20180626/bd3bc109/attachment.sig>
More information about the intel-gvt-dev
mailing list