[PATCH] drm/i915/gvt: set shadow entry to scratch page when guest driver only update partial of the GGTT entry

[Chen, Xiaoguang]

Hi Kevin,

>-----Original Message-----
>From: Tian, Kevin
>Sent: Wednesday, March 22, 2017 11:01 AM
>To: Chen, Xiaoguang <xiaoguang.chen at intel.com>; intel-gvt-
>dev at lists.freedesktop.org
>Cc: Wang, Zhi A <zhi.a.wang at intel.com>; Chen, Xiaoguang
><xiaoguang.chen at intel.com>
>Subject: RE: [PATCH] drm/i915/gvt: set shadow entry to scratch page when guest
>driver only update partial of the GGTT entry
>
>> From: Xiaoguang Chen
>> Sent: Tuesday, March 21, 2017 5:59 PM
>>
>> Sometimes guest driver only update partial of GGTT entry then access
>> it. In this situation the gfn we got from the GGTT table is invalid
>> and can not be
>
>Is partial update caused by splitting 64bit entry update into two 32bit entry
>updates, or just because only need to change partial fields when the PTE is
>already present?
Yes. Partial update is due to split 64bit entry update into two 32bit entry updates.

>
>"is invalid" and "can not" are too strong assertive here. Even in such case it should
>be "may be".
OK.

>
>> translate to a valid mfn.
>> p2m translation should happen only when the whole parts of a GGTT
>> entry got updated.
>
>Based on this description how do you judge there is a full update? I don't say the
>patch actually records any state to help later judgement.
>
>Or do you really mean that "Invalid P2M translation can be a valid scenario, as
>guest may do split update (e.g. splitting one 64bit entry update to two 32bit entry
>update). In such situation, we should update shadow PTE to scratch page against
>any P2M translation error.
>Suppose guest driver shouldn't use this partial entry for real purpose.
>Later when guest driver completes the other part update, the success of new
>P2M translation will lead to a real shadow PTE sync."?
It is a valid scenario when guest driver update one 64bit entry by updating two 32bit entries. 
If guest driver use the partial entry for real purpose then problem will occur but that's the responsibility of guest driver.


>
>
>>
>> Setting the shadow entry pointing to a scratch page instead if only partial of
>> GGTT entry updated.
>
>
>>
>> Signed-off-by: Zhi Wang <zhi.a.wang at intel.com>
>> SIgned-off-by: Xiaoguang Chen <xiaoguang.chen at intel.com>
>> ---
>>  drivers/gpu/drm/i915/gvt/gtt.c | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/gtt.c b/drivers/gpu/drm/i915/gvt/gtt.c
>> index da73127..119cd30 100644
>> --- a/drivers/gpu/drm/i915/gvt/gtt.c
>> +++ b/drivers/gpu/drm/i915/gvt/gtt.c
>> @@ -1837,11 +1837,21 @@ static int emulate_gtt_mmio_write(struct
>> intel_vgpu *vgpu, unsigned int off,
>>  		ret = gtt_entry_p2m(vgpu, &e, &m);
>>  		if (ret) {
>>  			gvt_vgpu_err("fail to translate guest gtt entry\n");
>> -			return ret;
>> +			/* guest driver may read/write the entry when only
>> +			 * partial of the guest entry got updated.
>> +			 * In this situation the gfn we got from GGTT table is
>> +			 * invalid and can not be translated to a valid mfn.
>> +			 * p2m translation should happen only after the
>> whole
>> +			 * parts of a GGTT entry got updated.
>> +			 *
>> +			 * Setting the shadow entry pointing to a scratch page
>> +			 * instead while GGTT entry only got partial update.
>> +			 */
>> +			ops->set_pfn(&m, gvt->gtt.scratch_ggtt_mfn);
>>  		}
>>  	} else {
>>  		m = e;
>> -		m.val64 = 0;
>> +		ops->set_pfn(&m, gvt->gtt.scratch_ggtt_mfn);
>
>This looks more than what commit message described. It's about
>using scratch page when guest PTE is non-present. Need include
>such info in the description.
OK.

>
>>  	}
>>
>>  	ggtt_set_shadow_entry(ggtt_mm, &m, g_gtt_index);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> intel-gvt-dev mailing list
>> intel-gvt-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev