[PATCH v2] drm/i915/gvt: fix a bug of partially write ggtt enties

[Zhenyu Wang]

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?

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

-- 
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/20180620/6bbec813/attachment.sig>