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

Submitted by Zhao, Yan Y on June 19, 2018, 7:44 a.m.

Details

Message ID 1529394251-27488-1-git-send-email-yan.y.zhao@intel.com
State New
Headers show
Series "drm/i915/gvt: fix a bug of partially write ggtt enties" ( rev: 2 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

Zhao, Yan Y June 19, 2018, 7:44 a.m.
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@intel.com>
---
 drivers/gpu/drm/i915/gvt/gtt.c | 58 ++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/gtt.h |  2 ++
 2 files changed, 60 insertions(+)

Patch hide | download patch | download mbox

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);
+			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;
 	};
 };

Comments


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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
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@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>
>
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
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@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@lists.freedesktop.org
>         https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> 
>    
>     _______________________________________________
>     intel-gvt-dev mailing list
>     intel-gvt-dev@lists.freedesktop.org
>     https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> 

> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev