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

[Hang Yuan]

On 09/13/2018 04:28 PM, Zhenyu Wang wrote:
> On 2018.09.13 15:51:19 +0800, intel-gvt-dev-bounces at lists.freedesktop.org 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 will enhance the previous solution. Use a list to save first
>> part of PTE. Find and combine a full PTE when the second part comes. Then
>> update the shadow PTE.
>>
>> 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 | 106 ++++++++++++++++++++++-------------------
>>   drivers/gpu/drm/i915/gvt/gtt.h |   9 +++-
>>   2 files changed, 63 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
>> index 2402395..e02b568 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,7 @@ 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;
>>   
>>   	if (bytes != 4 && bytes != 8)
>>   		return -EINVAL;
>> @@ -2178,63 +2177,60 @@ 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);
>> +		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;
>> +			}
>> +		}
>>   
>> -			ggtt_get_guest_entry(ggtt_mm, &e, last_offset);
>> -			ops->clear_present(&e);
>> -			ggtt_set_guest_entry(ggtt_mm, &e, last_offset);
>> +		if (!found) {
>> +			/* the first partial part */
>>   
>> -			ggtt_mm->ggtt_mm.last_partial_off = off;
>> -			ggtt_mm->ggtt_mm.last_partial_data = e.val64;
>> +			partial_pte = kzalloc(sizeof(*partial_pte), GFP_KERNEL);
>> +			if (!partial_pte)
>> +				return -ENOMEM;
>> +			partial_pte->offset = off;
>> +			partial_pte->data = e.val64;
>> +			list_add_tail(&partial_pte->list,
>> +				&ggtt_mm->ggtt_mm.partial_pte_list);
>>   
>> +			ggtt_set_guest_entry(ggtt_mm, &e, g_gtt_index);
>> +			ops->set_pfn(&e, gvt->gtt.scratch_mfn);
>> +			ggtt_set_host_entry(ggtt_mm, &e, g_gtt_index);
> 
> Looks this doesn't properly invalidate old entry and flush ggtt like old code did.

Yes, will fix it. Thanks for the comments.