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

[Zhenyu Wang]

On 2018.06.08 16:09:48 +0800, Zhao Yan wrote:
> when guest writes ggtt entries, it could write 8 bytes a time if
> gtt_entry_size is 8. But, when vfio traps the writes, it could be split
> 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, we don't keep this information for every ggtt index,
> 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 fact that if gtt_entry_size is 8,
> the guest memory physical address should be 64 bits, so when writing an
> 8-byte address into guest ggtt, 2 consecutive 4-byte writes at the same
> ggtt index should be trapped.

Instead of assuming consecutive write behavior, could we track entry status
by 'present' bit? If one partial write is found, we just clear 'present' bit
for that entry, and only when full entry is updated, we will set 'present'.
We'll initialize ggtt entry for scratch, so those entries without 'present'
set are thought as partial updated.

> 
> Signed-off-by: Zhao Yan <yan.y.zhao at intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/gtt.c | 34 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/gvt/gtt.h |  2 ++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
> index 78e55aa..8be8c38 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,38 @@ 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 {
> +			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);
> +			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/20180608/07c8947a/attachment.sig>